From 0ad83d5061418de8a32e7b1ad0acd32b22f9256b Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 15 Jul 2026 02:31:51 -0700 Subject: [PATCH] Fix object tagging writing back to the wrong object for nested keys (#10338) Put/DeleteObjectTagging set the update directory to the bucket root for a null version, so a tag change on allowed/protected.txt landed on protected.txt at the bucket root instead. A principal scoped to one nested key could overwrite a different object sharing the basename, the same class of scope bypass fixed for PutObjectAcl. Share the object's parent-directory resolver across both paths. --- weed/s3api/s3api_object_handlers_acl.go | 12 ++-- weed/s3api/s3api_object_handlers_acl_test.go | 14 +++-- weed/s3api/s3api_object_handlers_tagging.go | 66 +++----------------- 3 files changed, 22 insertions(+), 70 deletions(-) diff --git a/weed/s3api/s3api_object_handlers_acl.go b/weed/s3api/s3api_object_handlers_acl.go index a3df6f79d..d6dc3da6f 100644 --- a/weed/s3api/s3api_object_handlers_acl.go +++ b/weed/s3api/s3api_object_handlers_acl.go @@ -269,7 +269,7 @@ func (s3a *S3ApiServer) PutObjectAclHandler(w http.ResponseWriter, r *http.Reque return } - updateDirectory := s3a.objectAclUpdateDirectory(bucket, object, versioningConfigured, versionId, entry) + updateDirectory := s3a.objectMetadataUpdateDirectory(bucket, object, versioningConfigured, versionId, entry) // Update the object with new ACL metadata err = s3a.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { @@ -294,11 +294,11 @@ func (s3a *S3ApiServer) PutObjectAclHandler(w http.ResponseWriter, r *http.Reque writeSuccessResponseEmpty(w, r) } -// objectAclUpdateDirectory returns the filer directory holding the entry a PutObjectAcl -// update must target. A regular object lives under its full key's parent directory, so a -// nested key must not fall back to the bucket root, which would rewrite a different object -// sharing the same basename. -func (s3a *S3ApiServer) objectAclUpdateDirectory(bucket, object string, versioningConfigured bool, versionId string, entry *filer_pb.Entry) string { +// objectMetadataUpdateDirectory returns the filer directory holding the entry an in-place +// metadata update (ACL or tags) must target. A regular object lives under its full key's +// parent directory, so a nested key must not fall back to the bucket root, which would +// rewrite a different object sharing the same basename. +func (s3a *S3ApiServer) objectMetadataUpdateDirectory(bucket, object string, versioningConfigured bool, versionId string, entry *filer_pb.Entry) string { if versioningConfigured { if versionId != "" && versionId != "null" { return s3a.bucketDir(bucket) + "/" + object + s3_constants.VersionsFolder diff --git a/weed/s3api/s3api_object_handlers_acl_test.go b/weed/s3api/s3api_object_handlers_acl_test.go index 27f210899..dab718cd4 100644 --- a/weed/s3api/s3api_object_handlers_acl_test.go +++ b/weed/s3api/s3api_object_handlers_acl_test.go @@ -7,10 +7,11 @@ import ( "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" ) -// TestObjectAclUpdateDirectory guards against a PutObjectAcl scope bypass: a nested object -// key must resolve to its own parent directory, not the bucket root, otherwise updating the -// ACL of allowed/protected.txt would rewrite a different protected.txt at the bucket root. -func TestObjectAclUpdateDirectory(t *testing.T) { +// TestObjectMetadataUpdateDirectory guards against a metadata-update scope bypass shared by +// PutObjectAcl and Put/DeleteObjectTagging: a nested object key must resolve to its own +// parent directory, not the bucket root, otherwise updating the ACL or tags of +// allowed/protected.txt would rewrite a different protected.txt at the bucket root. +func TestObjectMetadataUpdateDirectory(t *testing.T) { s3a := &S3ApiServer{option: &S3ApiServerOption{BucketsPath: "/buckets"}} const bucket = "target-bucket" bucketDir := "/buckets/target-bucket" @@ -31,6 +32,7 @@ func TestObjectAclUpdateDirectory(t *testing.T) { {"non-versioned root key", "protected.txt", false, "", &filer_pb.Entry{}, bucketDir}, {"non-versioned deep key", "a/b/c/protected.txt", false, "", &filer_pb.Entry{}, bucketDir + "/a/b/c"}, {"null version nested key", "allowed/protected.txt", true, "", versioned("null"), bucketDir + "/allowed"}, + {"explicit null version nested key", "allowed/protected.txt", true, "null", versioned("null"), bucketDir + "/allowed"}, {"empty version nested key", "allowed/protected.txt", true, "", &filer_pb.Entry{}, bucketDir + "/allowed"}, {"specific version nested key", "allowed/protected.txt", true, "v1", versioned("v1"), bucketDir + "/allowed/protected.txt" + s3_constants.VersionsFolder}, {"latest versioned nested key", "allowed/protected.txt", true, "", versioned("v1"), bucketDir + "/allowed/protected.txt" + s3_constants.VersionsFolder}, @@ -38,9 +40,9 @@ func TestObjectAclUpdateDirectory(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := s3a.objectAclUpdateDirectory(bucket, tt.object, tt.versioningConfigured, tt.versionId, tt.entry) + got := s3a.objectMetadataUpdateDirectory(bucket, tt.object, tt.versioningConfigured, tt.versionId, tt.entry) if got != tt.want { - t.Errorf("objectAclUpdateDirectory(%q) = %q, want %q", tt.object, got, tt.want) + t.Errorf("objectMetadataUpdateDirectory(%q) = %q, want %q", tt.object, got, tt.want) } }) } diff --git a/weed/s3api/s3api_object_handlers_tagging.go b/weed/s3api/s3api_object_handlers_tagging.go index 1b1a36e93..976721888 100644 --- a/weed/s3api/s3api_object_handlers_tagging.go +++ b/weed/s3api/s3api_object_handlers_tagging.go @@ -199,35 +199,10 @@ func (s3a *S3ApiServer) PutObjectTaggingHandler(w http.ResponseWriter, r *http.R return } - // For versioned objects, determine the correct directory based on the version - var updateDirectory string - if versionId != "" { - // Specific version requested - if versionId == "null" { - // Null version (pre-versioning object) - stored as regular file - updateDirectory = s3a.bucketDir(bucket) - } else { - // Versioned object - stored in .versions directory - updateDirectory = s3a.bucketDir(bucket) + "/" + object + s3_constants.VersionsFolder - } - } else { - // Latest version in versioned bucket - could be null version or versioned object - // Extract version ID from the entry to determine where it's stored - var actualVersionId string - if entry.Extended != nil { - if versionIdBytes, exists := entry.Extended[s3_constants.ExtVersionIdKey]; exists { - actualVersionId = string(versionIdBytes) - } - } - - if actualVersionId == "null" || actualVersionId == "" { - // Null version (pre-versioning object) - stored as regular file - updateDirectory = s3a.bucketDir(bucket) - } else { - // Versioned object - stored in .versions directory - updateDirectory = s3a.bucketDir(bucket) + "/" + object + s3_constants.VersionsFolder - } - } + // For versioned objects, resolve the directory holding the entry to update. A null + // version lives under its full key's parent directory, so a nested key must not fall + // back to the bucket root and rewrite a different object sharing the same basename. + updateDirectory := s3a.objectMetadataUpdateDirectory(bucket, object, versioningConfigured, versionId, entry) // Remove old tags and add new ones for k := range entry.Extended { @@ -343,35 +318,10 @@ func (s3a *S3ApiServer) DeleteObjectTaggingHandler(w http.ResponseWriter, r *htt return } - // For versioned objects, determine the correct directory based on the version - var updateDirectory string - if versionId != "" { - // Specific version requested - if versionId == "null" { - // Null version (pre-versioning object) - stored as regular file - updateDirectory = s3a.bucketDir(bucket) - } else { - // Versioned object - stored in .versions directory - updateDirectory = s3a.bucketDir(bucket) + "/" + object + s3_constants.VersionsFolder - } - } else { - // Latest version in versioned bucket - could be null version or versioned object - // Extract version ID from the entry to determine where it's stored - var actualVersionId string - if entry.Extended != nil { - if versionIdBytes, exists := entry.Extended[s3_constants.ExtVersionIdKey]; exists { - actualVersionId = string(versionIdBytes) - } - } - - if actualVersionId == "null" || actualVersionId == "" { - // Null version (pre-versioning object) - stored as regular file - updateDirectory = s3a.bucketDir(bucket) - } else { - // Versioned object - stored in .versions directory - updateDirectory = s3a.bucketDir(bucket) + "/" + object + s3_constants.VersionsFolder - } - } + // For versioned objects, resolve the directory holding the entry to update. A null + // version lives under its full key's parent directory, so a nested key must not fall + // back to the bucket root and rewrite a different object sharing the same basename. + updateDirectory := s3a.objectMetadataUpdateDirectory(bucket, object, versioningConfigured, versionId, entry) // Remove all tags hasDeletion := false