From 5d293b1316b222878a320bd50b4721b770d049a2 Mon Sep 17 00:00:00 2001 From: DualFroz Date: Mon, 5 Oct 2026 09:47:50 +0200 Subject: [PATCH] fix: reject uploads with uploadId for every bucket and object operation Requests carrying both the uploads and uploadId query parameters were only rejected for a bucket GET, so DELETE, PUT, POST and HEAD on the bucket or object paths still ran real operations. S3 rejects the combination for every method on both paths, before authentication and before checking that the bucket exists. Register the error route for all methods on the bucket and object routers right after the CORS middleware is set up, ahead of every other route, and cover each method and path with an integration test. --- s3api/router.go | 16 ++++++++---- tests/integration/general.go | 42 ++++++++++++++++++++++++++++++++ tests/integration/group-tests.go | 2 ++ 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/s3api/router.go b/s3api/router.go index a20cb8a..fabeb52 100644 --- a/s3api/router.go +++ b/s3api/router.go @@ -102,6 +102,17 @@ func (sa *S3ApiRouter) Init() { objectRouter := sa.app.Group("/:bucket/*") applyBucketCORS := middlewares.ApplyBucketCORS(sa.be, middlewares.BucketFromPath, sa.corsAllowOrigin) + // '?uploads' together with '?uploadId' is rejected for every + // bucket and object operation + for _, method := range []string{http.MethodGet, http.MethodHead, http.MethodPut, http.MethodPost, http.MethodDelete} { + for _, router := range []fiber.Router{bucketRouter, objectRouter} { + router.Add([]string{method}, "", + middlewares.MatchQueryArgs("uploads", "uploadId"), + controllers.ProcessHandlers(ctrl.HandleErrorRoute(s3err.GetInvalidArgumentErr(s3err.InvalidArgUploadIdWithUploads, "uploadId")), metrics.ActionUndetected, services), + ) + } + } + // PUT bucket operations bucketRouter.Put("", middlewares.MatchQueryArgs("tagging"), @@ -725,11 +736,6 @@ func (sa *S3ApiRouter) Init() { middlewares.VerifyV4Signature(sa.root, sa.iam, sa.region, false, false, false), middlewares.ParseAcl(sa.be), )) - // bucket operation with both '?uploads' and '?uploadId' is rejected - bucketRouter.Get("", - middlewares.MatchQueryArgs("uploads", "uploadId"), - controllers.ProcessHandlers(ctrl.HandleErrorRoute(s3err.GetInvalidArgumentErr(s3err.InvalidArgUploadIdWithUploads, "uploadId")), metrics.ActionUndetected, services), - ) bucketRouter.Get("", middlewares.MatchQueryArgs("uploads"), controllers.ProcessHandlers( diff --git a/tests/integration/general.go b/tests/integration/general.go index 50ff019..ba22f10 100644 --- a/tests/integration/general.go +++ b/tests/integration/general.go @@ -195,6 +195,48 @@ func RouterCopySourceNotAllowed(s *S3Conf) error { }) } +func RouterUploadsWithUploadId(s *S3Conf) error { + testName := "RouterUploadsWithUploadId" + return actionHandlerNoSetup(s, testName, func(s3client *s3.Client, bucket string) error { + for _, method := range []string{ + http.MethodGet, + http.MethodHead, + http.MethodPut, + http.MethodPost, + http.MethodDelete, + } { + for _, path := range []string{ + "/bucket?uploads&uploadId=something", + "/bucket/object?uploads&uploadId=something", + } { + req, err := http.NewRequest(method, s.endpoint+path, nil) + if err != nil { + return fmt.Errorf("failed to make %s request to %s", method, path) + } + + resp, err := s.httpClient.Do(req) + if err != nil { + return fmt.Errorf("failed to send %s request to %s", method, path) + } + + if method == http.MethodHead { + // for head requests only check the status code + if resp.StatusCode != http.StatusBadRequest { + return fmt.Errorf("expected 400 status code for HEAD %s request, instead got %v", path, resp.StatusCode) + } + continue + } + + if err := checkHTTPResponseApiErr(resp, s3err.GetInvalidArgumentErr(s3err.InvalidArgUploadIdWithUploads, "uploadId")); err != nil { + return fmt.Errorf("%s %s: %w", method, path, err) + } + } + } + + return nil + }) +} + func RouterListVersionsWithKey(s *S3Conf) error { testName := "RouterListVersionsWithKey" return actionHandlerNoSetup(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 7dd4b5e..96789c9 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -2191,6 +2191,7 @@ func TestRouter(ts *TestState) { ts.Run(RouterGetUploadsWithKey) ts.Run(RouterCopySourceNotAllowed) ts.Run(RouterListVersionsWithKey) + ts.Run(RouterUploadsWithUploadId) ts.Run(RouterObjectAnnotationNotImplemented) ts.Run(RouterRenameObjectNotImplemented) ts.Run(RouterObjectTorrentMethodNotAllowed) @@ -3794,6 +3795,7 @@ func GetIntTests() IntTests { "RouterGetUploadsWithKey": RouterGetUploadsWithKey, "RouterCopySourceNotAllowed": RouterCopySourceNotAllowed, "RouterListVersionsWithKey": RouterListVersionsWithKey, + "RouterUploadsWithUploadId": RouterUploadsWithUploadId, "RouterObjectAnnotationNotImplemented": RouterObjectAnnotationNotImplemented, "RouterRenameObjectNotImplemented": RouterRenameObjectNotImplemented, "RouterObjectTorrentMethodNotAllowed": RouterObjectTorrentMethodNotAllowed,