s3: close seaweedfs-quota policy-confusion gap (#11409)

* s3: count seaweedfs-quota as an operation subresource

PUT /bucket?policy&seaweedfs-quota was not rejected by
hasAmbiguousSubresource because operationSubresources omitted the
seaweedfs-quota key. The router then picks the policy route
(registered first) while the IAM action resolver may resolve the
request to s3:PutBucketQuota, letting a quota-only identity write a
bucket policy. Reject the combination before routing, matching the
fix for policy&tagging (#10987).

* s3: resolve seaweedfs-quota after other bucket subresources

The quota routes are registered last among the bucket subresource
routes, but the action resolver found seaweedfs-quota inside the
unordered bucketQueryActions map, so a request carrying it alongside
another selector could be authorized as the quota operation while the
router served the earlier-registered handler. Resolve it explicitly at
the end so the resolver agrees with the router, mirroring how
list-type is handled.

* s3: count resolver subresources in the ambiguity guard

hasAmbiguousSubresource only counted operationSubresources, so adding
a query parameter to the action resolver without updating that list
reopened the authorize-one-serve-another gap. Count bucketQueryActions
keys as operation selectors too, and add a test that walks the
registered routes and fails on any query key that is neither an
operation subresource nor a known modifier.

weed/s3api/s3_action_resolver.go

@@ -116,11 +116,6 @@ var bucketQueryActions = map[string]map[string]string{
 		http.MethodPut:    s3_constants.S3_ACTION_PUT_BUCKET_OWNERSHIP_CONTROLS,
 		http.MethodDelete: s3_constants.S3_ACTION_PUT_BUCKET_OWNERSHIP_CONTROLS, // DELETE uses same permission as PUT
 	},
-	// SeaweedFS extension: bucket quota subresource
-	"seaweedfs-quota": {
-		http.MethodGet: s3_constants.S3_ACTION_GET_BUCKET_QUOTA,
-		http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_QUOTA,
-	},
 }
 
 // resolveFromQueryParameters checks query parameters to determine specific S3 actions
@@ -267,6 +262,17 @@ func resolveFromQueryParameters(query url.Values, method string, hasObject bool)
 		return s3_constants.S3_ACTION_DELETE_OBJECT
 	}
 
+	// SeaweedFS extension: the quota routes are registered last among the
+	// bucket subresource routes, so resolve seaweedfs-quota last to match.
+	if !hasObject && query.Has("seaweedfs-quota") {
+		switch method {
+		case http.MethodGet:
+			return s3_constants.S3_ACTION_GET_BUCKET_QUOTA
+		case http.MethodPut:
+			return s3_constants.S3_ACTION_PUT_BUCKET_QUOTA
+		}
+	}
+
 	return ""
 }
 

weed/s3api/s3_action_resolver_test.go

@@ -185,6 +185,33 @@ func TestResolveS3Action_ListType(t *testing.T) {
 	}
 }
 
+// The seaweedfs-quota routes are registered last among the bucket subresource
+// routes, so the resolver must resolve it last as well: a request combining it
+// with another selector is served by that selector's handler, and authorization
+// must name the same operation.
+func TestResolveS3Action_Quota(t *testing.T) {
+	tests := []struct {
+		name   string
+		method string
+		query  string
+		want   string
+	}{
+		{"quota get", http.MethodGet, "seaweedfs-quota", s3_constants.S3_ACTION_GET_BUCKET_QUOTA},
+		{"quota put", http.MethodPut, "seaweedfs-quota", s3_constants.S3_ACTION_PUT_BUCKET_QUOTA},
+		{"policy wins over quota", http.MethodPut, "policy&seaweedfs-quota", s3_constants.S3_ACTION_PUT_BUCKET_POLICY},
+		{"quota after policy wins too", http.MethodPut, "seaweedfs-quota&policy", s3_constants.S3_ACTION_PUT_BUCKET_POLICY},
+		{"tagging wins over quota", http.MethodPut, "tagging&seaweedfs-quota", s3_constants.S3_ACTION_PUT_BUCKET_TAGGING},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			r, _ := http.NewRequest(tt.method, "http://localhost/bucket?"+tt.query, nil)
+			if got := ResolveS3Action(r, s3_constants.ACTION_ADMIN, "bucket", ""); got != tt.want {
+				t.Errorf("ResolveS3Action() = %q, want %q", got, tt.want)
+			}
+		})
+	}
+}
+
 // A base action naming another service carries no S3 request shape, so a query
 // parameter on the request must not redirect it to an S3 action.
 func TestResolveS3ActionKeepsNonS3Service(t *testing.T) {

weed/s3api/s3api_ambiguous_subresource_test.go

@@ -3,8 +3,10 @@ package s3api
 import (
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 
+	"github.com/gorilla/mux"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -27,6 +29,7 @@ func TestAmbiguousSubresource(t *testing.T) {
 		"uploads=&prefix=a&x-id=CreateMultipartUpload",
 		"list-type=2&prefix=a&continuation-token=x",
 		"acl=&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Signature=deadbeef",
+		"seaweedfs-quota=",
 	} {
 		req, _ := http.NewRequest("GET", "http://localhost/bucket/key?"+query, nil)
 		assert.False(t, hasAmbiguousSubresource(req.URL.Query()), "%q names one operation", query)
@@ -49,6 +52,9 @@ func TestAmbiguousSubresource(t *testing.T) {
 		"list-type=2&tagging=",
 		"ownershipControls=&list-type=2",
 		"list-type=2&versions=",
+		"policy=&seaweedfs-quota=",
+		"seaweedfs-quota=&policy=",
+		"seaweedfs-quota=&tagging=",
 	} {
 		req, _ := http.NewRequest("PUT", "http://localhost/bucket?"+query, nil)
 		assert.True(t, hasAmbiguousSubresource(req.URL.Query()), "%q names two operations", query)
@@ -101,3 +107,36 @@ func TestListTypeOwnershipControlsRejectedBeforeHandler(t *testing.T) {
 	handler.ServeHTTP(rec, req)
 	assert.True(t, served, "a plain list-type request must still be served")
 }
+
+// Every query key a route can be selected by must be classified: an operation
+// subresource counted by the ambiguity guard, or a parameter that only
+// modifies the operation it accompanies. A selector left out of both
+// authorizes as one operation while the router serves another.
+func TestRouteQueryKeysAreClassified(t *testing.T) {
+	// Action selects STS routes on the root router, id narrows a config
+	// subresource, partNumber accompanies uploadId; none select an S3
+	// operation on their own.
+	modifiers := map[string]bool{
+		"Action": true, "id": true, "partNumber": true,
+	}
+
+	router := mux.NewRouter()
+	setupRoutingTestServer(t).registerRouter(router)
+	err := router.Walk(func(route *mux.Route, _ *mux.Router, _ []*mux.Route) error {
+		templates, err := route.GetQueriesTemplates()
+		if err != nil {
+			return nil
+		}
+		for _, q := range templates {
+			key, _, _ := strings.Cut(q, "=")
+			if modifiers[key] {
+				continue
+			}
+			_, resolved := bucketQueryActions[key]
+			assert.True(t, operationSubresources[key] || resolved,
+				"route query key %q selects an operation but is not counted by hasAmbiguousSubresource", key)
+		}
+		return nil
+	})
+	require.NoError(t, err)
+}

weed/s3api/s3api_path_validation.go

@@ -53,14 +53,17 @@ var operationSubresources = map[string]bool{
 	"location": true, "logging": true, "metrics": true, "notification": true,
 	"object-lock": true, "ownershipControls": true, "policy": true, "policyStatus": true,
 	"publicAccessBlock": true, "renameObject": true, "replication": true,
-	"requestPayment": true, "retention": true, "tagging": true, "uploadId": true,
+	"requestPayment": true, "retention": true, "seaweedfs-quota": true,
+	"tagging": true, "uploadId": true,
 	"uploads": true, "versioning": true, "versions": true, "website": true,
 }
 
 func hasAmbiguousSubresource(query url.Values) bool {
 	seen := 0
 	for key := range query {
-		if !operationSubresources[key] {
+		// bucketQueryActions keys select an operation by definition, so they
+		// count even if operationSubresources was not updated for them.
+		if _, ok := bucketQueryActions[key]; !ok && !operationSubresources[key] {
 			continue
 		}
 		if seen++; seen > 1 {