diff --git a/s3api/controllers/object-delete.go b/s3api/controllers/object-delete.go index 58bf0f54..e34a1789 100644 --- a/s3api/controllers/object-delete.go +++ b/s3api/controllers/object-delete.go @@ -36,6 +36,14 @@ func (c S3ApiController) DeleteObjectTagging(ctx fiber.Ctx) (*Response, error) { isBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) + if err := utils.ValidateVersionId(ctx); err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + action := auth.DeleteObjectTaggingAction if versionId != "" { action = auth.DeleteObjectVersionTaggingAction @@ -131,6 +139,14 @@ func (c S3ApiController) DeleteObject(ctx fiber.Ctx) (*Response, error) { isBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) + if err := utils.ValidateVersionId(ctx); err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + action := auth.DeleteObjectAction if versionId != "" { action = auth.DeleteObjectVersionAction diff --git a/s3api/controllers/object-delete_test.go b/s3api/controllers/object-delete_test.go index ac09cb22..543c5990 100644 --- a/s3api/controllers/object-delete_test.go +++ b/s3api/controllers/object-delete_test.go @@ -33,6 +33,23 @@ func TestS3ApiController_DeleteObjectTagging(t *testing.T) { input testInput output testOutput }{ + { + name: "empty versionId query param", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "versionId": "", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, ""), + }, + }, { name: "verify access fails", input: testInput{ @@ -207,6 +224,23 @@ func TestS3ApiController_DeleteObject(t *testing.T) { input testInput output testOutput }{ + { + name: "empty versionId query param", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "versionId": "", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, ""), + }, + }, { name: "verify access fails", input: testInput{ diff --git a/s3api/controllers/object-get.go b/s3api/controllers/object-get.go index c109d499..d8be1bb2 100644 --- a/s3api/controllers/object-get.go +++ b/s3api/controllers/object-get.go @@ -40,6 +40,14 @@ func (c S3ApiController) GetObjectTagging(ctx fiber.Ctx) (*Response, error) { parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) isPublicBucket := utils.ContextKeyPublicBucket.IsSet(ctx) + if err := utils.ValidateVersionId(ctx); err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + action := auth.GetObjectTaggingAction if versionId != "" { action = auth.GetObjectVersionTaggingAction @@ -101,6 +109,14 @@ func (c S3ApiController) GetObjectRetention(ctx fiber.Ctx) (*Response, error) { parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) isPublicBucket := utils.ContextKeyPublicBucket.IsSet(ctx) + if err := utils.ValidateVersionId(ctx); err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + err := c.verifyAccess(ctx, auth.AccessOptions{ Acl: parsedAcl, AclPermission: auth.PermissionRead, @@ -147,6 +163,14 @@ func (c S3ApiController) GetObjectLegalHold(ctx fiber.Ctx) (*Response, error) { parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) isPublicBucket := utils.ContextKeyPublicBucket.IsSet(ctx) + if err := utils.ValidateVersionId(ctx); err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + err := c.verifyAccess(ctx, auth.AccessOptions{ Acl: parsedAcl, AclPermission: auth.PermissionRead, @@ -289,6 +313,14 @@ func (c S3ApiController) GetObjectAttributes(ctx fiber.Ctx) (*Response, error) { parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) isPublicBucket := utils.ContextKeyPublicBucket.IsSet(ctx) + if err := utils.ValidateVersionId(ctx); err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + action := auth.GetObjectAttributesAction if versionId != "" { action = auth.GetObjectVersionAttributesAction @@ -412,6 +444,14 @@ func (c S3ApiController) GetObject(ctx fiber.Ctx) (*Response, error) { }, s3err.GetAPIError(s3err.ErrAnonymousResponseHeaders) } + if err := utils.ValidateVersionId(ctx); err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + action := auth.GetObjectAction if ctx.Request().URI().QueryArgs().Has("versionId") { action = auth.GetObjectVersionAction diff --git a/s3api/controllers/object-get_test.go b/s3api/controllers/object-get_test.go index 0808bbde..33709ea2 100644 --- a/s3api/controllers/object-get_test.go +++ b/s3api/controllers/object-get_test.go @@ -40,6 +40,23 @@ func TestS3ApiController_GetObjectTagging(t *testing.T) { input testInput output testOutput }{ + { + name: "empty versionId query param", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "versionId": "", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, ""), + }, + }, { name: "verify access fails", input: testInput{ @@ -142,6 +159,23 @@ func TestS3ApiController_GetObjectRetention(t *testing.T) { input testInput output testOutput }{ + { + name: "empty versionId query param", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "versionId": "", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, ""), + }, + }, { name: "verify access fails", input: testInput{ @@ -245,6 +279,23 @@ func TestS3ApiController_GetObjectLegalHold(t *testing.T) { input testInput output testOutput }{ + { + name: "empty versionId query param", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "versionId": "", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, ""), + }, + }, { name: "verify access fails", input: testInput{ @@ -552,6 +603,23 @@ func TestS3ApiController_GetObjectAttributes(t *testing.T) { input testInput output testOutput }{ + { + name: "empty versionId query param", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "versionId": "", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, ""), + }, + }, { name: "verify access fails", input: testInput{ @@ -674,6 +742,23 @@ func TestS3ApiController_GetObject(t *testing.T) { input testInput output testOutput }{ + { + name: "empty versionId query param", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "versionId": "", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, ""), + }, + }, { name: "verify access fails", input: testInput{ diff --git a/s3api/controllers/object-head.go b/s3api/controllers/object-head.go index 8776ae64..5a8ec4bf 100644 --- a/s3api/controllers/object-head.go +++ b/s3api/controllers/object-head.go @@ -71,6 +71,14 @@ func (c S3ApiController) HeadObject(ctx fiber.Ctx) (*Response, error) { }, s3err.GetAPIError(s3err.ErrAnonymousResponseHeaders) } + if err := utils.ValidateVersionId(ctx); err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + action := auth.GetObjectAction if ctx.Request().URI().QueryArgs().Has("versionId") { action = auth.GetObjectVersionAction diff --git a/s3api/controllers/object-head_test.go b/s3api/controllers/object-head_test.go index 067b51ec..1a0f3246 100644 --- a/s3api/controllers/object-head_test.go +++ b/s3api/controllers/object-head_test.go @@ -39,6 +39,23 @@ func TestS3ApiController_HeadObject(t *testing.T) { input testInput output testOutput }{ + { + name: "empty versionId query param", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "versionId": "", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, ""), + }, + }, { name: "verify access fails", input: testInput{ diff --git a/s3api/controllers/object-put.go b/s3api/controllers/object-put.go index c5046656..97174b05 100644 --- a/s3api/controllers/object-put.go +++ b/s3api/controllers/object-put.go @@ -42,6 +42,14 @@ func (c S3ApiController) PutObjectTagging(ctx fiber.Ctx) (*Response, error) { IsBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) + if err := utils.ValidateVersionId(ctx); err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + action := auth.PutObjectTaggingAction if versionId != "" { action = auth.PutObjectVersionTaggingAction @@ -96,6 +104,14 @@ func (c S3ApiController) PutObjectRetention(ctx fiber.Ctx) (*Response, error) { IsBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) + if err := utils.ValidateVersionId(ctx); err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + err := c.verifyAccess(ctx, auth.AccessOptions{ Acl: parsedAcl, AclPermission: auth.PermissionWrite, @@ -161,6 +177,14 @@ func (c S3ApiController) PutObjectLegalHold(ctx fiber.Ctx) (*Response, error) { IsBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) + if err := utils.ValidateVersionId(ctx); err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + err := c.verifyAccess(ctx, auth.AccessOptions{ Acl: parsedAcl, AclPermission: auth.PermissionWrite, diff --git a/s3api/controllers/object-put_test.go b/s3api/controllers/object-put_test.go index 7893a9a9..d93e7e23 100644 --- a/s3api/controllers/object-put_test.go +++ b/s3api/controllers/object-put_test.go @@ -53,6 +53,23 @@ func TestS3ApiController_PutObjectTagging(t *testing.T) { input testInput output testOutput }{ + { + name: "empty versionId query param", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "versionId": "", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, ""), + }, + }, { name: "verify access fails", input: testInput{ @@ -172,6 +189,23 @@ func TestS3ApiController_PutObjectRetention(t *testing.T) { input testInput output testOutput }{ + { + name: "empty versionId query param", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "versionId": "", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, ""), + }, + }, { name: "verify access fails", input: testInput{ @@ -301,6 +335,23 @@ func TestS3ApiController_PutObjectLegalHold(t *testing.T) { input testInput output testOutput }{ + { + name: "empty versionId query param", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "versionId": "", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, ""), + }, + }, { name: "verify access fails", input: testInput{ diff --git a/s3api/utils/utils.go b/s3api/utils/utils.go index f34edcf4..8c45d2dd 100644 --- a/s3api/utils/utils.go +++ b/s3api/utils/utils.go @@ -962,6 +962,20 @@ func GetQueryParam(ctx fiber.Ctx, key string) *string { return &value } +// ValidateVersionId ensures the versionId query parameter, if specified, isn't +// empty. S3 rejects both "?versionId=" and the valueless "?versionId" form, and +// rejects the request if any of the repeated values is empty. +func ValidateVersionId(ctx fiber.Ctx) error { + for _, val := range ctx.Request().URI().QueryArgs().PeekMulti("versionId") { + if len(val) == 0 { + debuglogger.Logf("empty versionId query parameter") + return s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, "") + } + } + + return nil +} + // ApplyOverride returns the override value if it exists and status is 200, otherwise returns original func ApplyOverride(original, override *string) *string { if override != nil { diff --git a/s3err/invalid-argument.go b/s3err/invalid-argument.go index 3171514e..1947c7e9 100644 --- a/s3err/invalid-argument.go +++ b/s3err/invalid-argument.go @@ -52,6 +52,7 @@ const ( InvalidArgMetadataDirective InvalidArgTaggingDirective InvalidArgVersionId + InvalidArgEmptyVersionId InvalidArgChecksumPart InvalidArgMissingUploadId InvalidArgUploadIdMarker @@ -175,6 +176,10 @@ var invalidArgErrResponses = map[InvalidArgErrorCode]InvalidArgumentError{ Description: "Invalid version id specified", ArgumentName: "versionId", }, + InvalidArgEmptyVersionId: { + Description: "Version id cannot be the empty string", + ArgumentName: "versionId", + }, InvalidArgChecksumPart: { Description: "Invalid Base64 or multiple checksums present in request", ArgumentName: "Checksum", diff --git a/tests/integration/DeleteObject.go b/tests/integration/DeleteObject.go index c1160d9c..a25e0b9a 100644 --- a/tests/integration/DeleteObject.go +++ b/tests/integration/DeleteObject.go @@ -383,3 +383,7 @@ func DeleteObject_expected_bucket_owner(s *S3Conf) error { return err }) } + +func DeleteObject_empty_version_id(s *S3Conf) error { + return testEmptyVersionId(s, "DeleteObject_empty_version_id", http.MethodDelete, "", nil) +} diff --git a/tests/integration/DeleteObjectTagging.go b/tests/integration/DeleteObjectTagging.go index 4b521bb2..6ac7aa17 100644 --- a/tests/integration/DeleteObjectTagging.go +++ b/tests/integration/DeleteObjectTagging.go @@ -196,3 +196,7 @@ func DeleteObjectTagging_expected_bucket_owner(s *S3Conf) error { return nil }) } + +func DeleteObjectTagging_empty_version_id(s *S3Conf) error { + return testEmptyVersionId(s, "DeleteObjectTagging_empty_version_id", http.MethodDelete, "tagging", nil) +} diff --git a/tests/integration/GetObject.go b/tests/integration/GetObject.go index fb15b633..ab24187e 100644 --- a/tests/integration/GetObject.go +++ b/tests/integration/GetObject.go @@ -1767,3 +1767,7 @@ func GetObject_ranged_with_checksum_mode(s *S3Conf) error { return nil }) } + +func GetObject_empty_version_id(s *S3Conf) error { + return testEmptyVersionId(s, "GetObject_empty_version_id", http.MethodGet, "", nil) +} diff --git a/tests/integration/GetObjectAttributes.go b/tests/integration/GetObjectAttributes.go index 23bc1d4b..94e5f083 100644 --- a/tests/integration/GetObjectAttributes.go +++ b/tests/integration/GetObjectAttributes.go @@ -20,6 +20,7 @@ import ( "crypto/rand" "errors" "fmt" + "net/http" "strings" "github.com/aws/aws-sdk-go-v2/service/s3" @@ -273,3 +274,7 @@ func GetObjectAttributes_checksums(s *S3Conf) error { return nil }) } + +func GetObjectAttributes_empty_version_id(s *S3Conf) error { + return testEmptyVersionId(s, "GetObjectAttributes_empty_version_id", http.MethodGet, "attributes", nil) +} diff --git a/tests/integration/GetObjectLegalHold.go b/tests/integration/GetObjectLegalHold.go index 52da802b..f314ff8b 100644 --- a/tests/integration/GetObjectLegalHold.go +++ b/tests/integration/GetObjectLegalHold.go @@ -17,6 +17,7 @@ package integration import ( "context" "fmt" + "net/http" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" @@ -143,3 +144,7 @@ func GetObjectLegalHold_success(s *S3Conf) error { return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: key, removeOnlyLeglHold: true}}) }, withLock()) } + +func GetObjectLegalHold_empty_version_id(s *S3Conf) error { + return testEmptyVersionId(s, "GetObjectLegalHold_empty_version_id", http.MethodGet, "legal-hold", nil) +} diff --git a/tests/integration/GetObjectRetention.go b/tests/integration/GetObjectRetention.go index b454492e..6a1e94dc 100644 --- a/tests/integration/GetObjectRetention.go +++ b/tests/integration/GetObjectRetention.go @@ -17,6 +17,7 @@ package integration import ( "context" "fmt" + "net/http" "time" "github.com/aws/aws-sdk-go-v2/service/s3" @@ -157,3 +158,7 @@ func GetObjectRetention_success(s *S3Conf) error { return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: key, isCompliance: true}}) }, withLock()) } + +func GetObjectRetention_empty_version_id(s *S3Conf) error { + return testEmptyVersionId(s, "GetObjectRetention_empty_version_id", http.MethodGet, "retention", nil) +} diff --git a/tests/integration/GetObjectTagging.go b/tests/integration/GetObjectTagging.go index d1893b2a..c847badb 100644 --- a/tests/integration/GetObjectTagging.go +++ b/tests/integration/GetObjectTagging.go @@ -17,6 +17,7 @@ package integration import ( "context" "fmt" + "net/http" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" @@ -128,3 +129,7 @@ func GetObjectTagging_success(s *S3Conf) error { return nil }) } + +func GetObjectTagging_empty_version_id(s *S3Conf) error { + return testEmptyVersionId(s, "GetObjectTagging_empty_version_id", http.MethodGet, "tagging", nil) +} diff --git a/tests/integration/HeadObject.go b/tests/integration/HeadObject.go index 0f5ca82d..8a283fea 100644 --- a/tests/integration/HeadObject.go +++ b/tests/integration/HeadObject.go @@ -1510,3 +1510,7 @@ func HeadObject_overrides_fail_public(s *S3Conf) error { return nil }, withAnonymousClient()) } + +func HeadObject_empty_version_id(s *S3Conf) error { + return testEmptyVersionId(s, "HeadObject_empty_version_id", http.MethodHead, "", nil) +} diff --git a/tests/integration/PutObjectLegalHold.go b/tests/integration/PutObjectLegalHold.go index 3ac028f4..270812a5 100644 --- a/tests/integration/PutObjectLegalHold.go +++ b/tests/integration/PutObjectLegalHold.go @@ -16,6 +16,7 @@ package integration import ( "context" + "net/http" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" @@ -149,3 +150,7 @@ func PutObjectLegalHold_success(s *S3Conf) error { return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: key, removeOnlyLeglHold: true}}) }, withLock()) } + +func PutObjectLegalHold_empty_version_id(s *S3Conf) error { + return testEmptyVersionId(s, "PutObjectLegalHold_empty_version_id", http.MethodPut, "legal-hold", []byte("ON")) +} diff --git a/tests/integration/PutObjectRetention.go b/tests/integration/PutObjectRetention.go index 03d43692..21bfc91d 100644 --- a/tests/integration/PutObjectRetention.go +++ b/tests/integration/PutObjectRetention.go @@ -17,6 +17,7 @@ package integration import ( "context" "fmt" + "net/http" "time" "github.com/aws/aws-sdk-go-v2/service/s3" @@ -648,3 +649,7 @@ func PutObjectRetention_success(s *S3Conf) error { return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: key, isCompliance: true}}) }, withLock()) } + +func PutObjectRetention_empty_version_id(s *S3Conf) error { + return testEmptyVersionId(s, "PutObjectRetention_empty_version_id", http.MethodPut, "retention", []byte("GOVERNANCE2222-01-01T00:00:00Z")) +} diff --git a/tests/integration/PutObjectTagging.go b/tests/integration/PutObjectTagging.go index c5144698..fbf30372 100644 --- a/tests/integration/PutObjectTagging.go +++ b/tests/integration/PutObjectTagging.go @@ -17,6 +17,7 @@ package integration import ( "context" "fmt" + "net/http" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" @@ -229,3 +230,7 @@ func PutObjectTagging_success(s *S3Conf) error { return nil }) } + +func PutObjectTagging_empty_version_id(s *S3Conf) error { + return testEmptyVersionId(s, "PutObjectTagging_empty_version_id", http.MethodPut, "tagging", []byte("keyval")) +} diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 6b652ba2..9c9f0522 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -234,6 +234,7 @@ func TestHeadObject(ts *TestState) { ts.Run(HeadObject_mp_part_number_resp_status) ts.Run(HeadObject_non_mp_part_number_1_success) ts.Run(HeadObject_empty_object_part_number_1) + ts.Run(HeadObject_empty_version_id) } func TestGetObjectAttributes(ts *TestState) { @@ -248,6 +249,7 @@ func TestGetObjectAttributes(ts *TestState) { if !ts.conf.azureTests { ts.Run(GetObjectAttributes_checksums) } + ts.Run(GetObjectAttributes_empty_version_id) } func TestGetObject(ts *TestState) { @@ -281,6 +283,7 @@ func TestGetObject(ts *TestState) { ts.Run(GetObject_mp_part_number_resp_status) ts.Run(GetObject_non_mp_part_number_1_success) ts.Run(GetObject_empty_object_part_number_1) + ts.Run(GetObject_empty_version_id) } func TestListObjects(ts *TestState) { @@ -350,6 +353,7 @@ func TestDeleteObject(ts *TestState) { ts.Run(DeleteObject_success_status_code) ts.Run(DeleteObject_incorrect_expected_bucket_owner) ts.Run(DeleteObject_expected_bucket_owner) + ts.Run(DeleteObject_empty_version_id) } func TestDeleteObjects(ts *TestState) { @@ -406,6 +410,7 @@ func TestPutObjectTagging(ts *TestState) { ts.Run(PutObjectTagging_tag_count_limit) ts.Run(PutObjectTagging_invalid_tags) ts.Run(PutObjectTagging_success) + ts.Run(PutObjectTagging_empty_version_id) } func TestGetObjectTagging(ts *TestState) { @@ -413,6 +418,7 @@ func TestGetObjectTagging(ts *TestState) { ts.Run(GetObjectTagging_unset_tags) ts.Run(GetObjectTagging_invalid_parent) ts.Run(GetObjectTagging_success) + ts.Run(GetObjectTagging_empty_version_id) } func TestDeleteObjectTagging(ts *TestState) { @@ -420,6 +426,7 @@ func TestDeleteObjectTagging(ts *TestState) { ts.Run(DeleteObjectTagging_success_status) ts.Run(DeleteObjectTagging_success) ts.Run(DeleteObjectTagging_expected_bucket_owner) + ts.Run(DeleteObjectTagging_empty_version_id) } func TestCreateMultipartUpload(ts *TestState) { @@ -782,6 +789,7 @@ func TestPutObjectRetention(ts *TestState) { ts.Run(PutObjectRetention_shorten_compliance_denied) ts.Run(PutObjectRetention_rewrite_same_date) ts.Run(PutObjectRetention_success) + ts.Run(PutObjectRetention_empty_version_id) } func TestGetObjectRetention(ts *TestState) { @@ -790,6 +798,7 @@ func TestGetObjectRetention(ts *TestState) { ts.Run(GetObjectRetention_disabled_lock) ts.Run(GetObjectRetention_unset_config) ts.Run(GetObjectRetention_success) + ts.Run(GetObjectRetention_empty_version_id) } func TestPutObjectLegalHold(ts *TestState) { @@ -799,6 +808,7 @@ func TestPutObjectLegalHold(ts *TestState) { ts.Run(PutObjectLegalHold_invalid_status) ts.Run(PutObjectLegalHold_unset_bucket_object_lock_config) ts.Run(PutObjectLegalHold_success) + ts.Run(PutObjectLegalHold_empty_version_id) } func TestGetObjectLegalHold(ts *TestState) { @@ -807,6 +817,7 @@ func TestGetObjectLegalHold(ts *TestState) { ts.Run(GetObjectLegalHold_disabled_lock) ts.Run(GetObjectLegalHold_unset_config) ts.Run(GetObjectLegalHold_success) + ts.Run(GetObjectLegalHold_empty_version_id) } func TestNotImplementedActions(ts *TestState) { @@ -2884,6 +2895,7 @@ func GetIntTests() IntTests { "HeadObject_mp_part_number_exceeds_parts_count": HeadObject_mp_part_number_exceeds_parts_count, "HeadObject_mp_part_number_success": HeadObject_mp_part_number_success, "HeadObject_mp_part_number_resp_status": HeadObject_mp_part_number_resp_status, + "HeadObject_empty_version_id": HeadObject_empty_version_id, "HeadObject_non_mp_part_number_1_success": HeadObject_non_mp_part_number_1_success, "HeadObject_empty_object_part_number_1": HeadObject_empty_object_part_number_1, "GetObjectAttributes_non_existing_bucket": GetObjectAttributes_non_existing_bucket, @@ -2894,6 +2906,7 @@ func GetIntTests() IntTests { "GetObjectAttributes_empty_attrs": GetObjectAttributes_empty_attrs, "GetObjectAttributes_existing_object": GetObjectAttributes_existing_object, "GetObjectAttributes_checksums": GetObjectAttributes_checksums, + "GetObjectAttributes_empty_version_id": GetObjectAttributes_empty_version_id, "GetObject_non_existing_key": GetObject_non_existing_key, "GetObject_directory_object_noslash": GetObject_directory_object_noslash, "GetObject_with_range": GetObject_with_range, @@ -2921,6 +2934,7 @@ func GetIntTests() IntTests { "GetObject_mp_part_number_resp_status": GetObject_mp_part_number_resp_status, "GetObject_non_mp_part_number_1_success": GetObject_non_mp_part_number_1_success, "GetObject_empty_object_part_number_1": GetObject_empty_object_part_number_1, + "GetObject_empty_version_id": GetObject_empty_version_id, "ListObjects_non_existing_bucket": ListObjects_non_existing_bucket, "ListObjects_with_prefix": ListObjects_with_prefix, "ListObjects_truncated": ListObjects_truncated, @@ -2970,6 +2984,7 @@ func GetIntTests() IntTests { "DeleteObject_directory_object": DeleteObject_directory_object, "DeleteObject_success": DeleteObject_success, "DeleteObject_success_status_code": DeleteObject_success_status_code, + "DeleteObject_empty_version_id": DeleteObject_empty_version_id, "DeleteObject_incorrect_expected_bucket_owner": DeleteObject_incorrect_expected_bucket_owner, "DeleteObject_expected_bucket_owner": DeleteObject_expected_bucket_owner, "DeleteObjects_empty_input": DeleteObjects_empty_input, @@ -3018,13 +3033,16 @@ func GetIntTests() IntTests { "PutObjectTagging_tag_count_limit": PutObjectTagging_tag_count_limit, "PutObjectTagging_invalid_tags": PutObjectTagging_invalid_tags, "PutObjectTagging_success": PutObjectTagging_success, + "PutObjectTagging_empty_version_id": PutObjectTagging_empty_version_id, "GetObjectTagging_non_existing_object": GetObjectTagging_non_existing_object, "GetObjectTagging_unset_tags": GetObjectTagging_unset_tags, "GetObjectTagging_invalid_parent": GetObjectTagging_invalid_parent, "GetObjectTagging_success": GetObjectTagging_success, + "GetObjectTagging_empty_version_id": GetObjectTagging_empty_version_id, "DeleteObjectTagging_non_existing_object": DeleteObjectTagging_non_existing_object, "DeleteObjectTagging_success_status": DeleteObjectTagging_success_status, "DeleteObjectTagging_success": DeleteObjectTagging_success, + "DeleteObjectTagging_empty_version_id": DeleteObjectTagging_empty_version_id, "DeleteObjectTagging_expected_bucket_owner": DeleteObjectTagging_expected_bucket_owner, "CreateMultipartUpload_non_existing_bucket": CreateMultipartUpload_non_existing_bucket, "CreateMultipartUpload_long_metadata": CreateMultipartUpload_long_metadata, @@ -3291,22 +3309,26 @@ func GetIntTests() IntTests { "PutObjectRetention_shorten_compliance_denied": PutObjectRetention_shorten_compliance_denied, "PutObjectRetention_rewrite_same_date": PutObjectRetention_rewrite_same_date, "PutObjectRetention_success": PutObjectRetention_success, + "PutObjectRetention_empty_version_id": PutObjectRetention_empty_version_id, "GetObjectRetention_non_existing_bucket": GetObjectRetention_non_existing_bucket, "GetObjectRetention_non_existing_object": GetObjectRetention_non_existing_object, "GetObjectRetention_disabled_lock": GetObjectRetention_disabled_lock, "GetObjectRetention_unset_config": GetObjectRetention_unset_config, "GetObjectRetention_success": GetObjectRetention_success, + "GetObjectRetention_empty_version_id": GetObjectRetention_empty_version_id, "PutObjectLegalHold_non_existing_bucket": PutObjectLegalHold_non_existing_bucket, "PutObjectLegalHold_non_existing_object": PutObjectLegalHold_non_existing_object, "PutObjectLegalHold_invalid_body": PutObjectLegalHold_invalid_body, "PutObjectLegalHold_invalid_status": PutObjectLegalHold_invalid_status, "PutObjectLegalHold_unset_bucket_object_lock_config": PutObjectLegalHold_unset_bucket_object_lock_config, "PutObjectLegalHold_success": PutObjectLegalHold_success, + "PutObjectLegalHold_empty_version_id": PutObjectLegalHold_empty_version_id, "GetObjectLegalHold_non_existing_bucket": GetObjectLegalHold_non_existing_bucket, "GetObjectLegalHold_non_existing_object": GetObjectLegalHold_non_existing_object, "GetObjectLegalHold_disabled_lock": GetObjectLegalHold_disabled_lock, "GetObjectLegalHold_unset_config": GetObjectLegalHold_unset_config, "GetObjectLegalHold_success": GetObjectLegalHold_success, + "GetObjectLegalHold_empty_version_id": GetObjectLegalHold_empty_version_id, "PutBucketAnalyticsConfiguration_not_implemented": PutBucketAnalyticsConfiguration_not_implemented, "GetBucketAnalyticsConfiguration_not_implemented": GetBucketAnalyticsConfiguration_not_implemented, "ListBucketAnalyticsConfiguration_not_implemented": ListBucketAnalyticsConfiguration_not_implemented, diff --git a/tests/integration/utils.go b/tests/integration/utils.go index 6100f766..a04b4d02 100644 --- a/tests/integration/utils.go +++ b/tests/integration/utils.go @@ -496,6 +496,61 @@ func checkHTTPResponseApiErr(resp *http.Response, expected s3err.S3Error) error return compareS3ApiError(expected, &errResp) } +// testEmptyVersionId verifies an action rejects an empty versionId query +// parameter. The SDK drops empty query parameters, so the request has to be +// signed and sent by hand. subresource is the action's query flag, e.g. +// "tagging", and is empty for the actions addressed by the bare object path. +// body is required for the actions whose routes are guarded by the checksum +// middleware, which rejects empty bodies before the controller runs. +func testEmptyVersionId(s *S3Conf, testName, method, subresource string, body []byte) error { + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + _, err := putObjects(s3client, []string{obj}, bucket) + if err != nil { + return err + } + + var headers map[string]string + if len(body) != 0 { + sum := md5.Sum(body) + headers = map[string]string{ + "Content-Md5": base64.StdEncoding.EncodeToString(sum[:]), + } + } + + query := "versionId=" + if subresource != "" { + query = fmt.Sprintf("%v&%v", subresource, query) + } + + req, err := createSignedReq(method, s.endpoint, + fmt.Sprintf("%v/%v?%v", bucket, obj, query), s.awsID, s.awsSecret, + "s3", s.awsRegion, "", body, time.Now(), headers) + if err != nil { + return err + } + + resp, err := s.httpClient.Do(req) + if err != nil { + return err + } + + expected := s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, "") + // HEAD responses carry no body: only the status code is verifiable + if method == http.MethodHead { + resp.Body.Close() + if resp.StatusCode != expected.StatusCode() { + return fmt.Errorf("expected response status code to be %v, instead got %v", + expected.StatusCode(), resp.StatusCode) + } + + return nil + } + + return checkHTTPResponseApiErr(resp, expected) + }) +} + func checkIAMAuthRequest(s *S3Conf, req *http.Request, expected iamerr.APIError) error { resp, err := s.httpClient.Do(req) if err != nil { @@ -1438,6 +1493,15 @@ func getPtr[T any](str T) *T { return &str } +// getNonEmptyPtr returns nil for an empty string, rather than a pointer to +// the empty string. +func getNonEmptyPtr(s string) *string { + if s == "" { + return nil + } + return &s +} + func checksumHeaderName(algo types.ChecksumAlgorithm) string { return fmt.Sprintf("x-amz-checksum-%s", strings.ToLower(string(algo))) } @@ -2942,7 +3006,7 @@ func cleanupLockedObjects(client *s3.Client, bucket string, objs []objToDelete) _, err := client.PutObjectLegalHold(ctx, &s3.PutObjectLegalHoldInput{ Bucket: &bucket, Key: &obj.key, - VersionId: getPtr(obj.versionId), + VersionId: getNonEmptyPtr(obj.versionId), LegalHold: &types.ObjectLockLegalHold{ Status: types.ObjectLockLegalHoldStatusOff, // Disable legal hold }, @@ -2980,7 +3044,7 @@ func cleanupLockedObjects(client *s3.Client, bucket string, objs []objToDelete) _, err := client.PutObjectRetention(ctx, &s3.PutObjectRetentionInput{ Bucket: &bucket, Key: &obj.key, - VersionId: getPtr(obj.versionId), + VersionId: getNonEmptyPtr(obj.versionId), BypassGovernanceRetention: getBoolPtr(true), Retention: &types.ObjectLockRetention{ Mode: types.ObjectLockRetentionModeGovernance, @@ -3021,7 +3085,7 @@ func waitOutComplianceRetention(client *s3.Client, bucket string, obj objToDelet out, err := client.GetObjectRetention(ctx, &s3.GetObjectRetentionInput{ Bucket: &bucket, Key: &obj.key, - VersionId: getPtr(obj.versionId), + VersionId: getNonEmptyPtr(obj.versionId), }) cancel() @@ -3048,7 +3112,7 @@ func waitOutComplianceRetention(client *s3.Client, bucket string, obj objToDelet _, err := client.PutObjectRetention(ctx, &s3.PutObjectRetentionInput{ Bucket: &bucket, Key: &obj.key, - VersionId: getPtr(obj.versionId), + VersionId: getNonEmptyPtr(obj.versionId), Retention: &types.ObjectLockRetention{ Mode: types.ObjectLockRetentionModeCompliance, RetainUntilDate: &retDate, @@ -3092,7 +3156,7 @@ func lockObject(client *s3.Client, mode objectLockMode, bucket, object, versionI _, err := client.PutObjectLegalHold(ctx, &s3.PutObjectLegalHoldInput{ Bucket: &bucket, Key: &object, - VersionId: getPtr(versionId), + VersionId: getNonEmptyPtr(versionId), LegalHold: &types.ObjectLockLegalHold{ Status: types.ObjectLockLegalHoldStatusOn, }, @@ -3110,7 +3174,7 @@ func lockObject(client *s3.Client, mode objectLockMode, bucket, object, versionI _, err := client.PutObjectRetention(ctx, &s3.PutObjectRetentionInput{ Bucket: &bucket, Key: &object, - VersionId: getPtr(versionId), + VersionId: getNonEmptyPtr(versionId), Retention: &types.ObjectLockRetention{ Mode: m, RetainUntilDate: &date,