From cd3095d5b72e6b72ae773916f73f562da6672422 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=22DualFroz=22=20Fox?= Date: Mon, 5 Oct 2026 20:18:49 +0200 Subject: [PATCH] fix: reject ListMultipartUploads requests that also carry uploadId * fix: reject ListMultipartUploads requests that also carry uploadId A bucket GET with both the uploads and uploadId query parameters was routed to ListMultipartUploads and answered with the upload listing. S3 rejects the combination with 400 InvalidArgument, "Conflicting query string parameters: uploadId, uploads". Add an error route for the combination ahead of ListMultipartUploads, the way the object level '?uploads' request is rejected, and enable the REST test that covers it. Fixes #2155 * 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 | 11 ++++++ s3err/invalid-argument.go | 5 +++ tests/integration/ListMultipartUploads.go | 25 ++++++++++++++ tests/integration/general.go | 42 +++++++++++++++++++++++ tests/integration/group-tests.go | 4 +++ tests/test_rest_list_multipart_uploads.sh | 3 -- 6 files changed, 87 insertions(+), 3 deletions(-) diff --git a/s3api/router.go b/s3api/router.go index 3ec46da..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"), diff --git a/s3err/invalid-argument.go b/s3err/invalid-argument.go index e814bfb..7bf61cd 100644 --- a/s3err/invalid-argument.go +++ b/s3err/invalid-argument.go @@ -73,6 +73,7 @@ const ( InvalidArgSSECIncompatibleEncryption InvalidArgSSEInvalidEncryptionMethod InvalidArgSSECRequiresTLS + InvalidArgUploadIdWithUploads ) var invalidArgErrResponses = map[InvalidArgErrorCode]InvalidArgumentError{ @@ -212,6 +213,10 @@ var invalidArgErrResponses = map[InvalidArgErrorCode]InvalidArgumentError{ Description: "Invalid uploadId marker", ArgumentName: "upload-id-marker", }, + InvalidArgUploadIdWithUploads: { + Description: "Conflicting query string parameters: uploadId, uploads", + ArgumentName: "ResourceType", + }, InvalidArgCannedAcl: { Description: "", ArgumentName: "x-amz-acl", diff --git a/tests/integration/ListMultipartUploads.go b/tests/integration/ListMultipartUploads.go index f163875..07cb9fa 100644 --- a/tests/integration/ListMultipartUploads.go +++ b/tests/integration/ListMultipartUploads.go @@ -17,6 +17,7 @@ package integration import ( "context" "fmt" + "net/http" "sort" "strings" "time" @@ -579,6 +580,30 @@ func ListMultipartUploads_delimiter_no_matches(s *S3Conf) error { }) } +func ListMultipartUploads_with_upload_id(s *S3Conf) error { + testName := "ListMultipartUploads_with_upload_id" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + out, err := createMp(s3client, bucket, "my-obj") + if err != nil { + return err + } + + req, err := createSignedReq(http.MethodGet, s.endpoint, + fmt.Sprintf("%v?uploads&uploadId=%v", bucket, *out.UploadId), + s.awsID, s.awsSecret, "s3", s.awsRegion, "", nil, time.Now(), nil) + if err != nil { + return err + } + + resp, err := s.httpClient.Do(req) + if err != nil { + return err + } + + return checkHTTPResponseApiErr(resp, s3err.GetInvalidArgumentErr(s3err.InvalidArgUploadIdWithUploads, "uploadId")) + }) +} + func ListMultipartUploads_with_checksums(s *S3Conf) error { testName := "ListMultipartUploads_with_checksums" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { 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 bd053a9..96789c9 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -554,6 +554,7 @@ func TestListMultipartUploads(ts *TestState) { ts.Run(ListMultipartUploads_prefix) ts.Run(ListMultipartUploads_both_delimiter_and_prefix) ts.Run(ListMultipartUploads_delimiter_no_matches) + ts.Run(ListMultipartUploads_with_upload_id) //TODO: remove the condition after implementing checksums in azure if !ts.conf.azureTests { ts.Run(ListMultipartUploads_with_checksums) @@ -2190,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) @@ -3288,6 +3290,7 @@ func GetIntTests() IntTests { "ListMultipartUploads_delimiter_truncated": ListMultipartUploads_delimiter_truncated, "ListMultipartUploads_prefix": ListMultipartUploads_prefix, "ListMultipartUploads_both_delimiter_and_prefix": ListMultipartUploads_both_delimiter_and_prefix, + "ListMultipartUploads_with_upload_id": ListMultipartUploads_with_upload_id, "ListMultipartUploads_with_checksums": ListMultipartUploads_with_checksums, "AbortMultipartUpload_non_existing_bucket": AbortMultipartUpload_non_existing_bucket, "AbortMultipartUpload_incorrect_uploadId": AbortMultipartUpload_incorrect_uploadId, @@ -3792,6 +3795,7 @@ func GetIntTests() IntTests { "RouterGetUploadsWithKey": RouterGetUploadsWithKey, "RouterCopySourceNotAllowed": RouterCopySourceNotAllowed, "RouterListVersionsWithKey": RouterListVersionsWithKey, + "RouterUploadsWithUploadId": RouterUploadsWithUploadId, "RouterObjectAnnotationNotImplemented": RouterObjectAnnotationNotImplemented, "RouterRenameObjectNotImplemented": RouterRenameObjectNotImplemented, "RouterObjectTorrentMethodNotAllowed": RouterObjectTorrentMethodNotAllowed, diff --git a/tests/test_rest_list_multipart_uploads.sh b/tests/test_rest_list_multipart_uploads.sh index 6038dad..4689ad0 100644 --- a/tests/test_rest_list_multipart_uploads.sh +++ b/tests/test_rest_list_multipart_uploads.sh @@ -215,9 +215,6 @@ source ./tests/setup.sh # tags: curl,ListMultipartUploads,multipart,uploadId,invalid-query @test "REST - ListMultipartUploads - uploads/uploadId combo" { - if [ "$DIRECT" != "true" ]; then - skip "https://github.com/versity/versitygw/issues/2155" - fi run setup_bucket_and_file_v3 "$BUCKET_ONE_NAME" assert_success read -r bucket_name test_file <<< "$output"