From cec96c02ea3e5908eda90bbb734ef7833f785e13 Mon Sep 17 00:00:00 2001
From: Chris Lu <chrislusf@users.noreply.github.com>
Date: Wed, 21 Oct 2026 22:45:53 -0700
Subject: [PATCH] s3: reject requests carrying more than one routed subresource
 (#11942)

* s3: reject requests carrying more than one routed subresource

Routing picks the first matching subresource route while authorization
resolves a single action from the query, and the two can disagree. A
bucket-level PUT ?tagging&policy is authorized as s3:PutBucketTagging but
served by PutBucketPolicyHandler, letting a tagger rewrite the bucket
policy; the same shape lets a list grant read the policy via
?list-type=2&policy or ?versions&policy, and lets an object ACL writer
retag via PUT ?acl&tagging. Declining ambiguous requests keeps the
authorized action and the served handler identical.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

* s3: reuse hasAmbiguousSubresource in the routed-subresource guard

The standalone key list dropped versioning, so ?versioning&policy still
reached route matching and auth with two operations. The bucket-level
check already keys off operationSubresources plus bucketQueryActions;
running the same predicate keeps the lists from drifting apart.

---------

Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
---
 weed/s3api/s3api_server.go                    |  3 +
 weed/s3api/s3api_subresource_ambiguity.go     | 20 +++++++
 .../s3api/s3api_subresource_ambiguity_test.go | 56 +++++++++++++++++++
 3 files changed, 79 insertions(+)
 create mode 100644 weed/s3api/s3api_subresource_ambiguity.go
 create mode 100644 weed/s3api/s3api_subresource_ambiguity_test.go

diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go
index c485e8814e..d06fed4424 100644
--- a/weed/s3api/s3api_server.go
+++ b/weed/s3api/s3api_server.go
@@ -921,6 +921,9 @@ func (s3a *S3ApiServer) UnifiedPostHandler(w http.ResponseWriter, r *http.Reques
 func (s3a *S3ApiServer) registerRouter(router *mux.Router) {
 	// runs before subrouter matching so Queries() matchers see the fixed query
 	router.Use(util_http.EscapeSemicolonsInQuery)
+	// a request naming two subresources routes one way and authorizes another;
+	// reject it rather than guess
+	router.Use(rejectAmbiguousSubresource)
 	// API Router
 	apiRouter := router.PathPrefix("/").Subrouter()
 	apiRouter.Use(request_id.Middleware)
diff --git a/weed/s3api/s3api_subresource_ambiguity.go b/weed/s3api/s3api_subresource_ambiguity.go
new file mode 100644
index 0000000000..34e326f4e3
--- /dev/null
+++ b/weed/s3api/s3api_subresource_ambiguity.go
@@ -0,0 +1,20 @@
+package s3api
+
+import (
+	"net/http"
+
+	"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
+)
+
+// rejectAmbiguousSubresource applies the same one-operation-per-request rule
+// validateRequestPath enforces inside bucket routes to the routes outside them,
+// so the check runs once for every request the router serves.
+func rejectAmbiguousSubresource(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if hasAmbiguousSubresource(r.URL.Query()) {
+			s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest)
+			return
+		}
+		next.ServeHTTP(w, r)
+	})
+}
diff --git a/weed/s3api/s3api_subresource_ambiguity_test.go b/weed/s3api/s3api_subresource_ambiguity_test.go
new file mode 100644
index 0000000000..d461f9076a
--- /dev/null
+++ b/weed/s3api/s3api_subresource_ambiguity_test.go
@@ -0,0 +1,56 @@
+package s3api
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+func TestRejectAmbiguousSubresource(t *testing.T) {
+	passthrough := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusTeapot) })
+	for _, query := range []string{
+		"policy",
+		"policy=",
+		"tagging&versionId=v1",
+		"list-type=2&prefix=a&continuation-token=x",
+		"uploadId=abc&partNumber=3",
+		"uploads",
+		"acl&X-Amz-Signature=x&X-Amz-Expires=900",
+		"analytics&id=weekly",
+		"delete",
+		"versions&versionId=",
+		"versioning",
+		"prefix=a&expected-bucket-owner=123",
+		"tagging&tagging=x",
+		"",
+	} {
+		r := httptest.NewRequest(http.MethodPut, "http://localhost/bucket?"+query, nil)
+		w := httptest.NewRecorder()
+		rejectAmbiguousSubresource(passthrough).ServeHTTP(w, r)
+		if w.Code != http.StatusTeapot {
+			t.Errorf("%q should pass through, got %d", query, w.Code)
+		}
+	}
+
+	for _, query := range []string{
+		"tagging&policy",
+		"policy&tagging",
+		"list-type=2&policy",
+		"versions&policy",
+		"versioning&policy",
+		"delete&policy",
+		"cors&policy",
+		"acl&tagging",
+		"tagging&seaweedfs-quota",
+		"uploadId=x&acl",
+		"retention&legal-hold",
+		"ownershipControls&encryption",
+	} {
+		r := httptest.NewRequest(http.MethodPut, "http://localhost/bucket?"+query, nil)
+		w := httptest.NewRecorder()
+		rejectAmbiguousSubresource(passthrough).ServeHTTP(w, r)
+		if w.Code != http.StatusBadRequest {
+			t.Errorf("%q should be rejected, got %d", query, w.Code)
+		}
+	}
+}