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) + } + } +}