From 311bc3a6dfbd042751692bc3de8accc516fcf8c1 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 14 Jul 2026 11:46:37 -0700 Subject: [PATCH] Fix PutObjectAcl writing back to the wrong object for nested keys (#10333) * Fix PutObjectAcl writing back to the wrong object for nested keys PutObjectAcl set the update directory to the bucket root, so an ACL 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. Target the object's own parent directory. * Test object ACL update directory resolution for nested keys --- weed/s3api/s3api_object_handlers_acl.go | 54 +++++++++----------- weed/s3api/s3api_object_handlers_acl_test.go | 47 +++++++++++++++++ 2 files changed, 72 insertions(+), 29 deletions(-) create mode 100644 weed/s3api/s3api_object_handlers_acl_test.go diff --git a/weed/s3api/s3api_object_handlers_acl.go b/weed/s3api/s3api_object_handlers_acl.go index 021efa960..a3df6f79d 100644 --- a/weed/s3api/s3api_object_handlers_acl.go +++ b/weed/s3api/s3api_object_handlers_acl.go @@ -10,6 +10,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" + "github.com/seaweedfs/seaweedfs/weed/util" ) // GetObjectAclHandler Get object ACL @@ -268,35 +269,7 @@ func (s3a *S3ApiServer) PutObjectAclHandler(w http.ResponseWriter, r *http.Reque return } - // Calculate the correct directory for ACL update - var updateDirectory string - - if versioningConfigured { - if versionId != "" && versionId != "null" { - // Versioned object - update the specific version file 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 - } - } - } else { - // Non-versioned object - stored as regular file - updateDirectory = s3a.bucketDir(bucket) - } + updateDirectory := s3a.objectAclUpdateDirectory(bucket, object, versioningConfigured, versionId, entry) // Update the object with new ACL metadata err = s3a.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { @@ -320,3 +293,26 @@ func (s3a *S3ApiServer) PutObjectAclHandler(w http.ResponseWriter, r *http.Reque glog.V(3).Infof("PutObjectAclHandler: Successfully updated ACL for %s/%s by user %s", bucket, object, amzAccountId) 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 { + if versioningConfigured { + if versionId != "" && versionId != "null" { + return s3a.bucketDir(bucket) + "/" + object + s3_constants.VersionsFolder + } + var actualVersionId string + if entry.Extended != nil { + if versionIdBytes, exists := entry.Extended[s3_constants.ExtVersionIdKey]; exists { + actualVersionId = string(versionIdBytes) + } + } + if actualVersionId != "null" && actualVersionId != "" { + return s3a.bucketDir(bucket) + "/" + object + s3_constants.VersionsFolder + } + } + dir, _ := util.NewFullPath(s3a.bucketDir(bucket), object).DirAndName() + return dir +} diff --git a/weed/s3api/s3api_object_handlers_acl_test.go b/weed/s3api/s3api_object_handlers_acl_test.go new file mode 100644 index 000000000..27f210899 --- /dev/null +++ b/weed/s3api/s3api_object_handlers_acl_test.go @@ -0,0 +1,47 @@ +package s3api + +import ( + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "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) { + s3a := &S3ApiServer{option: &S3ApiServerOption{BucketsPath: "/buckets"}} + const bucket = "target-bucket" + bucketDir := "/buckets/target-bucket" + + versioned := func(id string) *filer_pb.Entry { + return &filer_pb.Entry{Extended: map[string][]byte{s3_constants.ExtVersionIdKey: []byte(id)}} + } + + tests := []struct { + name string + object string + versioningConfigured bool + versionId string + entry *filer_pb.Entry + want string + }{ + {"non-versioned nested key", "allowed/protected.txt", false, "", &filer_pb.Entry{}, bucketDir + "/allowed"}, + {"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"}, + {"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}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := s3a.objectAclUpdateDirectory(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) + } + }) + } +}