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 <noreply@anthropic.com>

* s3: test an empty versionId against a version-only policy

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3: cover a bare versionId on HEAD

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

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:

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.