From 51501683688b14ed9cca0a96dacda945e0a39b58 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 11 Nov 2026 10:21:28 -0800 Subject: [PATCH] s3: authorize an empty versionId as the current-object action (#12566) * s3: authorize an empty versionId as the current-object action GET, HEAD and DELETE treat an empty versionId as the current object, but the action resolver keyed on the parameter's presence and authorized it as GetObjectVersion/DeleteObjectVersion, so a Deny on GetObject or DeleteObject did not apply. Co-Authored-By: Claude Opus 5.5 * s3: test an empty versionId against a version-only policy Co-Authored-By: Claude Opus 5.5 * s3: cover a bare versionId on HEAD Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- weed/s3api/s3_action_resolver.go | 2 +- weed/s3api/s3_action_resolver_test.go | 66 +++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/weed/s3api/s3_action_resolver.go b/weed/s3api/s3_action_resolver.go index 129e3c1fbe..7559b55f15 100644 --- a/weed/s3api/s3_action_resolver.go +++ b/weed/s3api/s3_action_resolver.go @@ -238,7 +238,7 @@ func resolveFromQueryParameters(query url.Values, method string, hasObject bool) // Versioning operations - distinguish between versionId (specific version) and versions (list versions) // versionId: Used to access/delete a specific version of an object (e.g., GET /bucket/key?versionId=xyz) - if query.Has("versionId") { + if query.Get("versionId") != "" { if hasObject { switch method { case http.MethodGet, http.MethodHead: diff --git a/weed/s3api/s3_action_resolver_test.go b/weed/s3api/s3_action_resolver_test.go index d073355fd5..8d5f9f277b 100644 --- a/weed/s3api/s3_action_resolver_test.go +++ b/weed/s3api/s3_action_resolver_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" ) // TestMapBaseActionToS3Format_ServicePrefixPassthrough verifies that actions @@ -114,6 +115,71 @@ func TestResolveS3Action_AttributesBeforeVersionId(t *testing.T) { } } +// An empty versionId is served as the current object, so it must be +// authorized as the current-object action. +func TestResolveS3Action_EmptyVersionIdIsCurrentObject(t *testing.T) { + tests := []struct { + name string + method string + baseAction string + query string + want string + }{ + {"get empty", http.MethodGet, s3_constants.ACTION_READ, "versionId=", s3_constants.S3_ACTION_GET_OBJECT}, + {"get bare", http.MethodGet, s3_constants.ACTION_READ, "versionId", s3_constants.S3_ACTION_GET_OBJECT}, + {"head empty", http.MethodHead, s3_constants.ACTION_READ, "versionId=", s3_constants.S3_ACTION_GET_OBJECT}, + {"delete empty", http.MethodDelete, s3_constants.ACTION_WRITE, "versionId=", s3_constants.S3_ACTION_DELETE_OBJECT}, + {"delete bare", http.MethodDelete, s3_constants.ACTION_WRITE, "versionId", s3_constants.S3_ACTION_DELETE_OBJECT}, + {"duplicate empty first", http.MethodGet, s3_constants.ACTION_READ, "versionId=&versionId=v1", s3_constants.S3_ACTION_GET_OBJECT}, + {"get null version", http.MethodGet, s3_constants.ACTION_READ, "versionId=null", s3_constants.S3_ACTION_GET_OBJECT_VERSION}, + {"head null version", http.MethodHead, s3_constants.ACTION_READ, "versionId=null", s3_constants.S3_ACTION_GET_OBJECT_VERSION}, + {"delete null version", http.MethodDelete, s3_constants.ACTION_WRITE, "versionId=null", s3_constants.S3_ACTION_DELETE_OBJECT_VERSION}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r, _ := http.NewRequest(tt.method, "http://localhost/bucket/key?"+tt.query, nil) + if got := ResolveS3Action(r, tt.baseAction, "bucket", "key"); got != tt.want { + t.Errorf("ResolveS3Action() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestEmptyVersionIdDoesNotBypassCurrentObjectDeny(t *testing.T) { + iam := &IdentityAccessManagement{isAuthEnabled: true} + if err := iam.PutPolicy("VersionOnly", `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3:GetObjectVersion","s3:DeleteObjectVersion"],"Resource":"arn:aws:s3:::abc/*"}, + {"Effect":"Deny","Action":["s3:GetObject","s3:DeleteObject"],"Resource":"arn:aws:s3:::abc/*"}]}`); err != nil { + t.Fatal(err) + } + identity := &Identity{Name: "versioner", PolicyNames: []string{"VersionOnly"}, Account: &Account{Id: "test-account"}} + tests := []struct { + method string + action Action + query string + want s3err.ErrorCode + }{ + {http.MethodGet, s3_constants.ACTION_READ, "", s3err.ErrAccessDenied}, + {http.MethodGet, s3_constants.ACTION_READ, "?versionId=", s3err.ErrAccessDenied}, + {http.MethodGet, s3_constants.ACTION_READ, "?versionId", s3err.ErrAccessDenied}, + {http.MethodHead, s3_constants.ACTION_READ, "?versionId=", s3err.ErrAccessDenied}, + {http.MethodHead, s3_constants.ACTION_READ, "?versionId", s3err.ErrAccessDenied}, + {http.MethodDelete, s3_constants.ACTION_WRITE, "?versionId=", s3err.ErrAccessDenied}, + {http.MethodDelete, s3_constants.ACTION_WRITE, "?versionId", s3err.ErrAccessDenied}, + {http.MethodGet, s3_constants.ACTION_READ, "?versionId=v1", s3err.ErrNone}, + {http.MethodHead, s3_constants.ACTION_READ, "?versionId=null", s3err.ErrNone}, + {http.MethodDelete, s3_constants.ACTION_WRITE, "?versionId=v1", s3err.ErrNone}, + } + for _, tt := range tests { + t.Run(tt.method+tt.query, func(t *testing.T) { + r, _ := http.NewRequest(tt.method, "http://localhost/abc/key"+tt.query, nil) + if got := iam.VerifyActionPermission(r, identity, tt.action, "abc", "key"); got != tt.want { + t.Errorf("VerifyActionPermission() = %v, want %v", got, tt.want) + } + }) + } +} + // A tagging request carrying versionId targets a specific version and must // resolve to the version-tagging action so a grant limited to current-object // tagging cannot reach historical versions.