From e74d2c0d197e77a7595b673b433ca53d5380a190 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Mon, 21 Jul 2025 18:58:39 +0400 Subject: [PATCH] fix: fixes the invalid x-amz-mp-object-size header error in CompleteMultipartUpload. Fixes #1398 The `x-amz-mp-object-size` request header can have two erroneous states: an invalid value or a negative integer. AWS returns different error descriptions for each case. This PR fixes the error description for the invalid header value case. The invalid case can't be integration tested as SDK expects `int64` as the header value. --- s3api/controllers/base.go | 7 ++++++- s3api/controllers/bucket-delete.go | 1 + s3api/controllers/bucket-delete_test.go | 2 ++ s3api/controllers/bucket-get.go | 4 +++- s3api/controllers/bucket-list.go | 6 +++--- s3api/controllers/bucket-put.go | 12 ++++++------ s3api/controllers/object-delete.go | 14 +++++++------- s3api/controllers/object-head.go | 2 +- s3api/controllers/object-post.go | 21 ++++++++++----------- s3api/controllers/object-post_test.go | 4 ++-- s3api/controllers/object-put.go | 12 ++++++------ s3err/s3err.go | 10 +++++++++- tests/integration/tests.go | 2 +- 13 files changed, 57 insertions(+), 40 deletions(-) diff --git a/s3api/controllers/base.go b/s3api/controllers/base.go index cc46677f..eff9cb3d 100644 --- a/s3api/controllers/base.go +++ b/s3api/controllers/base.go @@ -42,10 +42,15 @@ type S3ApiController struct { } const ( + // time constants iso8601Format = "20060102T150405Z" iso8601TimeFormatExtended = "Mon Jan _2 15:04:05 2006" timefmt = "Mon, 02 Jan 2006 15:04:05 GMT" - maxXMLBodyLen = 4 * 1024 * 1024 + + maxXMLBodyLen = 4 * 1024 * 1024 + minPartNumber = 1 + maxPartNumber = 10000 + defaultMaxBuckets = int32(10000) ) var ( diff --git a/s3api/controllers/bucket-delete.go b/s3api/controllers/bucket-delete.go index 00a169dd..22ef9286 100644 --- a/s3api/controllers/bucket-delete.go +++ b/s3api/controllers/bucket-delete.go @@ -153,6 +153,7 @@ func (c S3ApiController) DeleteBucketCors(ctx *fiber.Ctx) (*Response, error) { return &Response{ MetaOpts: &MetaOptions{ BucketOwner: parsedAcl.Owner, + Status: http.StatusNoContent, }, }, err } diff --git a/s3api/controllers/bucket-delete_test.go b/s3api/controllers/bucket-delete_test.go index 19eb95da..50ae11d9 100644 --- a/s3api/controllers/bucket-delete_test.go +++ b/s3api/controllers/bucket-delete_test.go @@ -286,6 +286,7 @@ func TestS3ApiController_DeleteBucketCors(t *testing.T) { response: &Response{ MetaOpts: &MetaOptions{ BucketOwner: "root", + Status: http.StatusNoContent, }, }, err: s3err.GetAPIError(s3err.ErrAdminMethodNotSupported), @@ -300,6 +301,7 @@ func TestS3ApiController_DeleteBucketCors(t *testing.T) { response: &Response{ MetaOpts: &MetaOptions{ BucketOwner: "root", + Status: http.StatusNoContent, }, }, }, diff --git a/s3api/controllers/bucket-get.go b/s3api/controllers/bucket-get.go index cc60d267..f4da5c34 100644 --- a/s3api/controllers/bucket-get.go +++ b/s3api/controllers/bucket-get.go @@ -61,7 +61,9 @@ func (c S3ApiController) GetBucketTagging(ctx *fiber.Ctx) (*Response, error) { }, err } resp := s3response.Tagging{ - TagSet: s3response.TagSet{Tags: []s3response.Tag{}}, + TagSet: s3response.TagSet{ + Tags: make([]s3response.Tag, 0, len(tags)), + }, } for key, val := range tags { diff --git a/s3api/controllers/bucket-list.go b/s3api/controllers/bucket-list.go index f990dd1e..5cae8939 100644 --- a/s3api/controllers/bucket-list.go +++ b/s3api/controllers/bucket-list.go @@ -31,10 +31,10 @@ func (c S3ApiController) ListBuckets(ctx *fiber.Ctx) (*Response, error) { maxBucketsStr := ctx.Query("max-buckets") acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) - var maxBuckets int32 = 10000 + maxBuckets := defaultMaxBuckets if maxBucketsStr != "" { maxBucketsParsed, err := strconv.ParseInt(maxBucketsStr, 10, 32) - if err != nil || maxBucketsParsed < 0 || maxBucketsParsed > 10000 { + if err != nil || maxBucketsParsed < 0 || maxBucketsParsed > int64(defaultMaxBuckets) { debuglogger.Logf("error parsing max-buckets %q: %v", maxBucketsStr, err) return &Response{ MetaOpts: &MetaOptions{}, @@ -47,7 +47,7 @@ func (c S3ApiController) ListBuckets(ctx *fiber.Ctx) (*Response, error) { s3response.ListBucketsInput{ Owner: acct.Access, IsAdmin: acct.Role == auth.RoleAdmin, - MaxBuckets: int32(maxBuckets), + MaxBuckets: maxBuckets, ContinuationToken: cToken, Prefix: prefix, }) diff --git a/s3api/controllers/bucket-put.go b/s3api/controllers/bucket-put.go index 8a0caf43..7fbd0ea3 100644 --- a/s3api/controllers/bucket-put.go +++ b/s3api/controllers/bucket-put.go @@ -301,14 +301,14 @@ func (c S3ApiController) PutBucketAcl(ctx *fiber.Ctx) (*Response, error) { grantFullControl := ctx.Get("X-Amz-Grant-Full-Control") grantRead := ctx.Get("X-Amz-Grant-Read") grantReadACP := ctx.Get("X-Amz-Grant-Read-Acp") - granWrite := ctx.Get("X-Amz-Grant-Write") + grantWrite := ctx.Get("X-Amz-Grant-Write") grantWriteACP := ctx.Get("X-Amz-Grant-Write-Acp") // context locals parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) isRoot := utils.ContextKeyIsRoot.Get(ctx).(bool) - grants := grantFullControl + grantRead + grantReadACP + granWrite + grantWriteACP + grants := grantFullControl + grantRead + grantReadACP + grantWrite + grantWriteACP var input *auth.PutBucketAclInput err := auth.VerifyAccess(ctx.Context(), c.be, @@ -424,7 +424,7 @@ func (c S3ApiController) PutBucketAcl(ctx *fiber.Ctx) (*Response, error) { GrantFullControl: &grantFullControl, GrantRead: &grantRead, GrantReadACP: &grantReadACP, - GrantWrite: &granWrite, + GrantWrite: &grantWrite, GrantWriteACP: &grantWriteACP, } } else { @@ -459,11 +459,11 @@ func (c S3ApiController) CreateBucket(ctx *fiber.Ctx) (*Response, error) { grantFullControl := ctx.Get("X-Amz-Grant-Full-Control") grantRead := ctx.Get("X-Amz-Grant-Read") grantReadACP := ctx.Get("X-Amz-Grant-Read-Acp") - granWrite := ctx.Get("X-Amz-Grant-Write") + grantWrite := ctx.Get("X-Amz-Grant-Write") grantWriteACP := ctx.Get("X-Amz-Grant-Write-Acp") lockEnabled := strings.EqualFold(ctx.Get("X-Amz-Bucket-Object-Lock-Enabled"), "true") acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) - grants := grantFullControl + grantRead + grantReadACP + granWrite + grantWriteACP + grants := grantFullControl + grantRead + grantReadACP + grantWrite + grantWriteACP objectOwnership := types.ObjectOwnership( ctx.Get("X-Amz-Object-Ownership", string(types.ObjectOwnershipBucketOwnerEnforced)), ) @@ -518,7 +518,7 @@ func (c S3ApiController) CreateBucket(ctx *fiber.Ctx) (*Response, error) { GrantFullControl: &grantFullControl, GrantRead: &grantRead, GrantReadACP: &grantReadACP, - GrantWrite: &granWrite, + GrantWrite: &grantWrite, GrantWriteACP: &grantWriteACP, AccessControlPolicy: &auth.AccessControlPolicy{ Owner: &types.Owner{ diff --git a/s3api/controllers/object-delete.go b/s3api/controllers/object-delete.go index 93721315..76dfb167 100644 --- a/s3api/controllers/object-delete.go +++ b/s3api/controllers/object-delete.go @@ -32,7 +32,7 @@ func (c S3ApiController) DeleteObjectTagging(ctx *fiber.Ctx) (*Response, error) key := strings.TrimPrefix(ctx.Path(), fmt.Sprintf("/%s/", bucket)) acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) isRoot := utils.ContextKeyIsRoot.Get(ctx).(bool) - IsBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) + isBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) err := auth.VerifyAccess(ctx.Context(), c.be, @@ -45,7 +45,7 @@ func (c S3ApiController) DeleteObjectTagging(ctx *fiber.Ctx) (*Response, error) Bucket: bucket, Object: key, Action: auth.DeleteObjectTaggingAction, - IsBucketPublic: IsBucketPublic, + IsBucketPublic: isBucketPublic, }) if err != nil { return &Response{ @@ -71,7 +71,7 @@ func (c S3ApiController) AbortMultipartUpload(ctx *fiber.Ctx) (*Response, error) uploadId := ctx.Query("uploadId") acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) isRoot := utils.ContextKeyIsRoot.Get(ctx).(bool) - IsBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) + isBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) err := auth.VerifyAccess(ctx.Context(), c.be, @@ -84,7 +84,7 @@ func (c S3ApiController) AbortMultipartUpload(ctx *fiber.Ctx) (*Response, error) Bucket: bucket, Object: key, Action: auth.AbortMultipartUploadAction, - IsBucketPublic: IsBucketPublic, + IsBucketPublic: isBucketPublic, }) if err != nil { return &Response{ @@ -116,7 +116,7 @@ func (c S3ApiController) DeleteObject(ctx *fiber.Ctx) (*Response, error) { // context locals acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) isRoot := utils.ContextKeyIsRoot.Get(ctx).(bool) - IsBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) + isBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) //TODO: check s3:DeleteObjectVersion policy in case a use tries to delete a version of an object @@ -131,7 +131,7 @@ func (c S3ApiController) DeleteObject(ctx *fiber.Ctx) (*Response, error) { Bucket: bucket, Object: key, Action: auth.DeleteObjectAction, - IsBucketPublic: IsBucketPublic, + IsBucketPublic: isBucketPublic, }) if err != nil { return &Response{ @@ -152,7 +152,7 @@ func (c S3ApiController) DeleteObject(ctx *fiber.Ctx) (*Response, error) { }, }, bypass, - IsBucketPublic, + isBucketPublic, c.be, ) if err != nil { diff --git a/s3api/controllers/object-head.go b/s3api/controllers/object-head.go index 5b0aa304..57c749b0 100644 --- a/s3api/controllers/object-head.go +++ b/s3api/controllers/object-head.go @@ -63,7 +63,7 @@ func (c S3ApiController) HeadObject(ctx *fiber.Ctx) (*Response, error) { var partNumber *int32 if ctx.Request().URI().QueryArgs().Has("partNumber") { - if partNumberQuery < 1 || partNumberQuery > 10000 { + if partNumberQuery < minPartNumber || partNumberQuery > maxPartNumber { debuglogger.Logf("invalid part number: %d", partNumberQuery) return &Response{ MetaOpts: &MetaOptions{ diff --git a/s3api/controllers/object-post.go b/s3api/controllers/object-post.go index 658320a4..35fece82 100644 --- a/s3api/controllers/object-post.go +++ b/s3api/controllers/object-post.go @@ -36,7 +36,7 @@ func (c S3ApiController) RestoreObject(ctx *fiber.Ctx) (*Response, error) { key := strings.TrimPrefix(ctx.Path(), fmt.Sprintf("/%s/", bucket)) acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) isRoot := utils.ContextKeyIsRoot.Get(ctx).(bool) - IsBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) + isBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) err := auth.VerifyAccess(ctx.Context(), c.be, @@ -49,7 +49,7 @@ func (c S3ApiController) RestoreObject(ctx *fiber.Ctx) (*Response, error) { Bucket: bucket, Object: key, Action: auth.RestoreObjectAction, - IsBucketPublic: IsBucketPublic, + IsBucketPublic: isBucketPublic, }) if err != nil { return &Response{ @@ -87,7 +87,7 @@ func (c S3ApiController) SelectObjectContent(ctx *fiber.Ctx) (*Response, error) key := strings.TrimPrefix(ctx.Path(), fmt.Sprintf("/%s/", bucket)) acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) isRoot := utils.ContextKeyIsRoot.Get(ctx).(bool) - IsBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) + isBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) err := auth.VerifyAccess(ctx.Context(), c.be, @@ -100,7 +100,7 @@ func (c S3ApiController) SelectObjectContent(ctx *fiber.Ctx) (*Response, error) Bucket: bucket, Object: key, Action: auth.GetObjectAction, - IsBucketPublic: IsBucketPublic, + IsBucketPublic: isBucketPublic, }) if err != nil { return &Response{ @@ -237,7 +237,7 @@ func (c S3ApiController) CompleteMultipartUpload(ctx *fiber.Ctx) (*Response, err // context locals acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) isRoot := utils.ContextKeyIsRoot.Get(ctx).(bool) - IsBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) + isBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) err := auth.VerifyAccess(ctx.Context(), c.be, @@ -250,7 +250,7 @@ func (c S3ApiController) CompleteMultipartUpload(ctx *fiber.Ctx) (*Response, err Bucket: bucket, Object: key, Action: auth.PutObjectAction, - IsBucketPublic: IsBucketPublic, + IsBucketPublic: isBucketPublic, }) if err != nil { return &Response{ @@ -283,23 +283,22 @@ func (c S3ApiController) CompleteMultipartUpload(ctx *fiber.Ctx) (*Response, err var mpuObjectSize *int64 if mpuObjSizeHdr != "" { val, err := strconv.ParseInt(mpuObjSizeHdr, 10, 64) - //TODO: Not sure if invalid request should be returned if err != nil { - debuglogger.Logf("invalid value for 'x-amz-mp-objects-size' header: %v", err) + debuglogger.Logf("invalid value for 'x-amz-mp-object-size' header: %v", err) return &Response{ MetaOpts: &MetaOptions{ BucketOwner: parsedAcl.Owner, }, - }, s3err.GetAPIError(s3err.ErrInvalidRequest) + }, s3err.GetInvalidMpObjectSizeErr(mpuObjSizeHdr) } if val < 0 { - debuglogger.Logf("value for 'x-amz-mp-objects-size' header is less than 0: %v", val) + debuglogger.Logf("value for 'x-amz-mp-object-size' header is less than 0: %v", val) return &Response{ MetaOpts: &MetaOptions{ BucketOwner: parsedAcl.Owner, }, - }, s3err.GetInvalidMpObjectSizeErr(val) + }, s3err.GetNegatvieMpObjectSizeErr(val) } mpuObjectSize = &val diff --git a/s3api/controllers/object-post_test.go b/s3api/controllers/object-post_test.go index baca9f3e..3d96d7de 100644 --- a/s3api/controllers/object-post_test.go +++ b/s3api/controllers/object-post_test.go @@ -420,7 +420,7 @@ func TestS3ApiController_CompleteMultipartUpload(t *testing.T) { BucketOwner: "root", }, }, - err: s3err.GetAPIError(s3err.ErrInvalidRequest), + err: s3err.GetInvalidMpObjectSizeErr("invalid_mp_object_size"), }, }, { @@ -438,7 +438,7 @@ func TestS3ApiController_CompleteMultipartUpload(t *testing.T) { BucketOwner: "root", }, }, - err: s3err.GetInvalidMpObjectSizeErr(-4), + err: s3err.GetNegatvieMpObjectSizeErr(-4), }, }, { diff --git a/s3api/controllers/object-put.go b/s3api/controllers/object-put.go index 28444872..77397d95 100644 --- a/s3api/controllers/object-put.go +++ b/s3api/controllers/object-put.go @@ -51,7 +51,7 @@ func (c S3ApiController) PutObjectTagging(ctx *fiber.Ctx) (*Response, error) { Acc: acct, Bucket: bucket, Object: key, - Action: auth.PutBucketTaggingAction, + Action: auth.PutObjectTaggingAction, IsBucketPublic: IsBucketPublic, }) if err != nil { @@ -233,7 +233,7 @@ func (c S3ApiController) UploadPart(ctx *fiber.Ctx) (*Response, error) { }, err } - if partNumber < 1 || partNumber > 10000 { + if partNumber < minPartNumber || partNumber > maxPartNumber { debuglogger.Logf("invalid part number: %d", partNumber) return &Response{ MetaOpts: &MetaOptions{ @@ -349,7 +349,7 @@ func (c S3ApiController) UploadPartCopy(ctx *fiber.Ctx) (*Response, error) { }, err } - if partNumber < 1 || partNumber > 10000 { + if partNumber < minPartNumber || partNumber > maxPartNumber { debuglogger.Logf("invalid part number: %d", partNumber) return &Response{ MetaOpts: &MetaOptions{ @@ -389,7 +389,7 @@ func (c S3ApiController) PutObjectAcl(ctx *fiber.Ctx) (*Response, error) { grantFullControl := ctx.Get("X-Amz-Grant-Full-Control") grantRead := ctx.Get("X-Amz-Grant-Read") grantReadACP := ctx.Get("X-Amz-Grant-Read-Acp") - granWrite := ctx.Get("X-Amz-Grant-Write") + grantWrite := ctx.Get("X-Amz-Grant-Write") grantWriteACP := ctx.Get("X-Amz-Grant-Write-Acp") // context locals acct := utils.ContextKeyAccount.Get(ctx).(auth.Account) @@ -420,7 +420,7 @@ func (c S3ApiController) PutObjectAcl(ctx *fiber.Ctx) (*Response, error) { Key: &key, GrantFullControl: &grantFullControl, GrantRead: &grantRead, - GrantWrite: &granWrite, + GrantWrite: &grantWrite, ACL: types.ObjectCannedACL(acl), GrantReadACP: &grantReadACP, GrantWriteACP: &grantWriteACP, @@ -524,7 +524,7 @@ func (c S3ApiController) CopyObject(ctx *fiber.Ctx) (*Response, error) { } if taggingDirective != "" && taggingDirective != types.TaggingDirectiveCopy && taggingDirective != types.TaggingDirectiveReplace { - debuglogger.Logf("invalid tagging direcrive: %v", taggingDirective) + debuglogger.Logf("invalid tagging directive: %v", taggingDirective) return &Response{ MetaOpts: &MetaOptions{ BucketOwner: parsedAcl.Owner, diff --git a/s3err/s3err.go b/s3err/s3err.go index 4dd1a374..c2c3cc22 100644 --- a/s3err/s3err.go +++ b/s3err/s3err.go @@ -894,7 +894,7 @@ func GetIncorrectMpObjectSizeErr(expected, actual int64) APIError { } } -func GetInvalidMpObjectSizeErr(val int64) APIError { +func GetNegatvieMpObjectSizeErr(val int64) APIError { return APIError{ Code: "InvalidRequest", Description: fmt.Sprintf("Value for x-amz-mp-object-size header is less than zero: '%v'", val), @@ -902,6 +902,14 @@ func GetInvalidMpObjectSizeErr(val int64) APIError { } } +func GetInvalidMpObjectSizeErr(val string) APIError { + return APIError{ + Code: "InvalidRequest", + Description: fmt.Sprintf("Value for x-amz-mp-object-size header is invalid: '%s'", val), + HTTPStatusCode: http.StatusBadRequest, + } +} + func CreateExceedingRangeErr(objSize int64) APIError { return APIError{ Code: "InvalidArgument", diff --git a/tests/integration/tests.go b/tests/integration/tests.go index 65ac11f9..f2e1e023 100644 --- a/tests/integration/tests.go +++ b/tests/integration/tests.go @@ -11260,7 +11260,7 @@ func CompleteMultipartUpload_mpu_object_size(s *S3Conf) error { ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) _, err = s3client.CompleteMultipartUpload(ctx, input) cancel() - if err := checkApiErr(err, s3err.GetInvalidMpObjectSizeErr(invMpuSize)); err != nil { + if err := checkApiErr(err, s3err.GetNegatvieMpObjectSizeErr(invMpuSize)); err != nil { return err }