From 210afacd128faafd85c6fc4dc4dcfc54551a683e Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 11 Sep 2026 22:21:58 -0700 Subject: [PATCH] s3: close list-type / ownership-controls routing mismatch (#11280) * s3: reject list-type paired with another operation subresource ?list-type=2&ownershipControls= routes to ListObjectsV2 (the list-type route is registered first) while the IAM action resolver resolves the ownershipControls selector to s3:GetBucketOwnershipControls. A principal denied s3:ListBucket but allowed s3:GetBucketOwnershipControls would therefore list the bucket. list-type selects an operation just like the other keys in operationSubresources, so add it there and reject the combination before routing, matching the fix for policy&tagging (#10987). * s3: resolve list-type to s3:ListBucket ahead of bucket subresources The router registers the ListObjectsV2 route ahead of the bucket subresource routes, so the action resolver should resolve list-type the same way. Without this, a request carrying list-type and another operation selector resolves to the subresource action (e.g. s3:GetBucketOwnershipControls) while being served by ListObjectsV2. The ambiguity guard rejects such combinations before routing, but resolving list-type to s3:ListBucket keeps the resolver aligned with the router, mirroring how versions is handled. * s3: match list-type=2 exactly in action resolver The router selects ListObjectsV2 only for list-type=2; other values fall through to the subresource routes. Resolve the same way so the action matches the handler for every list-type value, not just 2. --- weed/s3api/s3_action_resolver.go | 6 ++++ weed/s3api/s3_action_resolver_test.go | 27 ++++++++++++++++++ .../s3api/s3api_ambiguous_subresource_test.go | 28 +++++++++++++++++++ weed/s3api/s3api_path_validation.go | 6 ++-- 4 files changed, 64 insertions(+), 3 deletions(-) diff --git a/weed/s3api/s3_action_resolver.go b/weed/s3api/s3_action_resolver.go index 176d89de8..987a244cb 100644 --- a/weed/s3api/s3_action_resolver.go +++ b/weed/s3api/s3_action_resolver.go @@ -217,6 +217,12 @@ func resolveFromQueryParameters(query url.Values, method string, hasObject bool) } } + if query.Get("list-type") == "2" { + if method == http.MethodGet && !hasObject { + return s3_constants.S3_ACTION_LIST_BUCKET + } + } + // Check bucket-level query parameters using data-driven approach // These are strictly bucket-level operations, so only apply when !hasObject if !hasObject { diff --git a/weed/s3api/s3_action_resolver_test.go b/weed/s3api/s3_action_resolver_test.go index ef4caaf95..7aa0ee03d 100644 --- a/weed/s3api/s3_action_resolver_test.go +++ b/weed/s3api/s3_action_resolver_test.go @@ -158,6 +158,33 @@ func TestResolveS3Action_CreateBucket(t *testing.T) { } } +// list-type selects ListObjectsV2, which the router registers ahead of the +// bucket subresource routes. The resolver must align with routing and resolve +// it to s3:ListBucket even when an operation subresource like ownershipControls +// is also present, so authorization checks the listing action the handler runs. +func TestResolveS3Action_ListType(t *testing.T) { + tests := []struct { + name string + query string + want string + }{ + {"list-type alone", "list-type=2", s3_constants.S3_ACTION_LIST_BUCKET}, + {"list-type with listing params", "list-type=2&prefix=a&continuation-token=x", s3_constants.S3_ACTION_LIST_BUCKET}, + {"list-type with ownershipControls", "list-type=2&ownershipControls=", s3_constants.S3_ACTION_LIST_BUCKET}, + // The router only selects ListObjectsV2 for list-type=2; another value + // falls through to the subresource route, so ownershipControls wins. + {"list-type=1 with ownershipControls", "list-type=1&ownershipControls=", s3_constants.S3_ACTION_GET_BUCKET_OWNERSHIP_CONTROLS}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r, _ := http.NewRequest(http.MethodGet, "http://localhost/bucket?"+tt.query, nil) + if got := ResolveS3Action(r, s3_constants.ACTION_LIST, "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) { diff --git a/weed/s3api/s3api_ambiguous_subresource_test.go b/weed/s3api/s3api_ambiguous_subresource_test.go index e74153471..a51a9a643 100644 --- a/weed/s3api/s3api_ambiguous_subresource_test.go +++ b/weed/s3api/s3api_ambiguous_subresource_test.go @@ -45,6 +45,10 @@ func TestAmbiguousSubresource(t *testing.T) { "delete=&policy=", "uploads=&uploadId=xyz", "policy=&tagging=&cors=", + "list-type=2&ownershipControls=", + "list-type=2&tagging=", + "ownershipControls=&list-type=2", + "list-type=2&versions=", } { req, _ := http.NewRequest("PUT", "http://localhost/bucket?"+query, nil) assert.True(t, hasAmbiguousSubresource(req.URL.Query()), "%q names two operations", query) @@ -73,3 +77,27 @@ func TestAmbiguousSubresourceRejectedBeforeHandler(t *testing.T) { handler.ServeHTTP(rec, req) assert.True(t, served, "an unambiguous request must still be served") } + +// The listing disclosure: GET /bucket?list-type=2&ownershipControls= routes to +// ListObjectsV2 while resolving as s3:GetBucketOwnershipControls, so a principal +// denied s3:ListBucket but allowed the ownership-controls read would list the +// bucket. The guard has to reject the combined request before the listing handler. +func TestListTypeOwnershipControlsRejectedBeforeHandler(t *testing.T) { + served := false + handler := validateRequestPath(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + served = true + })) + + req, _ := http.NewRequest("GET", "http://localhost/bucket?list-type=2&ownershipControls=", nil) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + require.False(t, served, "a list-type+ownershipControls request must not reach a handler") + assert.Equal(t, http.StatusBadRequest, rec.Code) + + served = false + req, _ = http.NewRequest("GET", "http://localhost/bucket?list-type=2&prefix=a", nil) + rec = httptest.NewRecorder() + handler.ServeHTTP(rec, req) + assert.True(t, served, "a plain list-type request must still be served") +} diff --git a/weed/s3api/s3api_path_validation.go b/weed/s3api/s3api_path_validation.go index 692115b8f..6a76554c9 100644 --- a/weed/s3api/s3api_path_validation.go +++ b/weed/s3api/s3api_path_validation.go @@ -49,9 +49,9 @@ func hasPathSegmentQuery(rawQuery string) bool { var operationSubresources = map[string]bool{ "accelerate": true, "acl": true, "analytics": true, "attributes": true, "cors": true, "delete": true, "encryption": true, "intelligent-tiering": true, - "inventory": true, "legal-hold": true, "lifecycle": true, "location": true, - "logging": true, "metrics": true, "notification": true, "object-lock": true, - "ownershipControls": true, "policy": true, "policyStatus": true, + "inventory": true, "legal-hold": true, "lifecycle": true, "list-type": true, + "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, "uploads": true, "versioning": true, "versions": true, "website": true,