From 008b8a49bae55ccb5da7fa7fa8d837ca58c37fcb Mon Sep 17 00:00:00 2001 From: niksis02 Date: Wed, 21 Oct 2026 19:38:20 +0400 Subject: [PATCH] fix: return NoSuchUpload for malformed upload IDs ahead of access and request checks Fixes #2533 `UploadPartCopy` and `CopyObject` returned `500 InternalError` when a non-root user copied from a source bucket with no stored ACL (such as a posix bucket directory created outside the gateway): `VerifyObjectCopyAccess` ran `json.Unmarshal` on the empty ACL bytes posix returns for such a bucket. It now parses them with `ParseACL`, like every other ACL reader. The source checks also ran before the backend looked at the upload ID, so a malformed `uploadId` got `AccessDenied`, `NoSuchBucket` or `InvalidArgument` instead of `NoSuchUpload`. S3 rejects an upload ID that is malformed, or bound to another key, right after the bucket existence and expected-bucket-owner checks, before authorization and nearly all request validation. That applies to every multipart operation that takes an upload ID. `Backend` gains `ValidateUploadID`, a format-only check that doesn't look up the upload. `BackendUnsupported` accepts every ID, so s3proxy and azure are unchanged, and posix's existing canonical UUID check becomes the method. `UploadPart`, `UploadPartCopy` and `AbortMultipartUpload` call it first. `ListParts` calls it after rejecting a `part-number-marker` or `max-parts` that isn't an integer (new `ValidateLimiterInteger`); negative values are still rejected after it. `CompleteMultipartUpload` calls it after rejecting unsupported `If-Match`/`If-None-Match` combinations, which are split out of `EvaluateObjectPutPreconditions` as `ValidateObjectPutPreconditionHeaders` and now checked before access verification and body parsing, as on S3. --- auth/access-control.go | 4 +- auth/access-control_test.go | 29 ++++ backend/azure/azure.go | 6 + backend/backend.go | 4 + backend/common.go | 42 +++-- backend/common_test.go | 40 +++++ backend/posix/posix.go | 20 +-- s3api/controllers/backend_moq_test.go | 44 ++++++ s3api/controllers/object-delete.go | 12 +- s3api/controllers/object-delete_test.go | 24 ++- s3api/controllers/object-get.go | 32 +++- s3api/controllers/object-get_test.go | 41 +++++ s3api/controllers/object-post.go | 26 +++- s3api/controllers/object-post_test.go | 46 ++++++ s3api/controllers/object-put.go | 27 +++- s3api/controllers/object-put_test.go | 50 ++++++ s3api/utils/utils.go | 33 +++- s3api/utils/utils_test.go | 22 +++ tests/integration/Access_Control.go | 154 +++++++++++++++++++ tests/integration/CompleteMultipartUpload.go | 53 +++++++ tests/integration/ListParts.go | 38 +++++ tests/integration/UploadPart.go | 28 ++++ tests/integration/UploadPartCopy.go | 60 +++++++- tests/integration/group-tests.go | 13 ++ 24 files changed, 804 insertions(+), 44 deletions(-) diff --git a/auth/access-control.go b/auth/access-control.go index 6d6dfdf6..f821329e 100644 --- a/auth/access-control.go +++ b/auth/access-control.go @@ -67,8 +67,8 @@ func VerifyObjectCopyAccess(ctx fiber.Ctx, be backend.Backend, copySource string return err } - var srcBucketAcl ACL - if err := json.Unmarshal(srcBucketACLBytes, &srcBucketAcl); err != nil { + srcBucketAcl, err := ParseACL(srcBucketACLBytes) + if err != nil { return err } diff --git a/auth/access-control_test.go b/auth/access-control_test.go index 9d4c0064..7229f97f 100644 --- a/auth/access-control_test.go +++ b/auth/access-control_test.go @@ -59,6 +59,16 @@ func (b noBucketPolicyBackend) GetBucketAcl(_ context.Context, _ *s3.GetBucketAc return json.Marshal(b.srcAcl) } +// noACLBackend is a test stub with no bucket policy whose GetBucketAcl +// returns no data, as posix does for a bucket with no stored ACL. +type noACLBackend struct { + noBucketPolicyBackend +} + +func (noACLBackend) GetBucketAcl(_ context.Context, _ *s3.GetBucketAclInput) ([]byte, error) { + return []byte{}, nil +} + type publicBucketPolicyBackend struct { backend.BackendUnsupported policy []byte @@ -600,6 +610,25 @@ func TestVerifyObjectCopyAccess_LiteralSlashSeparator(t *testing.T) { assert.NoError(t, err, "literal slash separator should work") } +func TestVerifyObjectCopyAccess_SourceBucketWithoutACL(t *testing.T) { + const testUser = "testuser" + + // The source bucket has no stored ACL, as a posix bucket directory + // created outside the gateway, so testUser has no grant on it. + opts := AccessOptions{ + Acl: ACL{Owner: testUser}, + AclPermission: PermissionWrite, + Acc: Account{Access: testUser, Role: RoleUser}, + Bucket: "dst-bucket", + Object: "dst-key", + Actions: []Action{PutObjectAction}, + } + + err := VerifyObjectCopyAccess(testFiberCtx(t), noACLBackend{}, "src-bucket/src-key", opts) + assert.Error(t, err) + assert.True(t, errors.Is(err, s3err.GetAPIError(s3err.ErrAccessDenied)), "unexpected error: %v", err) +} + // TestVerifyCreateBucketAccess_RootAndAdminBypass confirms root and admin // accounts may always create a bucket, with no iam backend consulted at // all — CreateBucket has no existing bucket to check a policy or ACL diff --git a/backend/azure/azure.go b/backend/azure/azure.go index 4d75ae8f..3564983b 100644 --- a/backend/azure/azure.go +++ b/backend/azure/azure.go @@ -2687,6 +2687,12 @@ func getAclFromMetadata(meta map[string]*string, key key) (*auth.ACL, error) { return &acl, nil } +// ValidateUploadID returns NoSuchUpload for an upload ID this backend never +// generates. Upload IDs are UUIDs in canonical form. +func (az *Azure) ValidateUploadID(uploadID string) error { + return backend.ValidateUUIDUploadID(uploadID) +} + func createMetaTmpPath(obj, uploadId string) string { objNameSum := sha256.Sum256([]byte(obj)) return filepath.Join(string(metaTmpMultipartPrefix), uploadId, fmt.Sprintf("%x", objNameSum)) diff --git a/backend/backend.go b/backend/backend.go index 9b992955..ad9ffe3a 100644 --- a/backend/backend.go +++ b/backend/backend.go @@ -31,6 +31,7 @@ type Backend interface { fmt.Stringer Shutdown() NormalizeObjectKey(bucket, object string) string + ValidateUploadID(uploadID string) error // bucket operations ListBuckets(context.Context, s3response.ListBucketsInput) (s3response.ListAllMyBucketsResult, error) @@ -118,6 +119,9 @@ func (BackendUnsupported) String() string { func (BackendUnsupported) NormalizeObjectKey(_, object string) string { return object } +func (BackendUnsupported) ValidateUploadID(string) error { + return nil +} func (BackendUnsupported) ListBuckets(context.Context, s3response.ListBucketsInput) (s3response.ListAllMyBucketsResult, error) { return s3response.ListAllMyBucketsResult{}, s3err.GetAPIError(s3err.ErrNotImplemented) } diff --git a/backend/common.go b/backend/common.go index 5568cac3..3fbb6d97 100644 --- a/backend/common.go +++ b/backend/common.go @@ -38,6 +38,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" + "github.com/google/uuid" "github.com/versity/versitygw/debuglogger" "github.com/versity/versitygw/s3err" "github.com/versity/versitygw/s3response" @@ -816,16 +817,8 @@ func EvaluateObjectPutPreconditions(etag string, ifMatch, ifNoneMatch *string, o return nil } - if ifNoneMatch != nil && ifMatch != nil { - return s3err.GetNotImplementedErr("If-Match,If-None-Match", s3err.NmpAdditionalMessageMultipleCondHeaders) - } - - if ifNoneMatch != nil && *ifNoneMatch != "*" { - return s3err.GetNotImplementedErr("If-None-Match", s3err.NmpAdditionalMessageIfNoneMatch) - } - - if ifMatch != nil && *ifMatch == "*" { - return s3err.GetNotImplementedErr("If-Match", s3err.NmpAdditionalMessageIfMatch) + if err := ValidateObjectPutPreconditionHeaders(ifMatch, ifNoneMatch); err != nil { + return err } if ifNoneMatch != nil && objExists { @@ -845,6 +838,35 @@ func EvaluateObjectPutPreconditions(etag string, ifMatch, ifNoneMatch *string, o return nil } +// ValidateUUIDUploadID returns NoSuchUpload for an upload ID that isn't a +// UUID in canonical form, the form uuid.New().String() generates. +func ValidateUUIDUploadID(uploadID string) error { + id, err := uuid.Parse(uploadID) + if err != nil || id.String() != uploadID { + return s3err.GetNoSuchUploadErr(uploadID) + } + return nil +} + +// ValidateObjectPutPreconditionHeaders rejects the if-match and +// if-none-match header combinations and values an object PUT doesn't +// support. It depends on the headers alone. +func ValidateObjectPutPreconditionHeaders(ifMatch, ifNoneMatch *string) error { + if ifNoneMatch != nil && ifMatch != nil { + return s3err.GetNotImplementedErr("If-Match,If-None-Match", s3err.NmpAdditionalMessageMultipleCondHeaders) + } + + if ifNoneMatch != nil && *ifNoneMatch != "*" { + return s3err.GetNotImplementedErr("If-None-Match", s3err.NmpAdditionalMessageIfNoneMatch) + } + + if ifMatch != nil && *ifMatch == "*" { + return s3err.GetNotImplementedErr("If-Match", s3err.NmpAdditionalMessageIfMatch) + } + + return nil +} + type ObjectDeletePreconditions struct { IfMatch *string IfMatchLastModTime *time.Time diff --git a/backend/common_test.go b/backend/common_test.go index eb0213d8..b998542c 100644 --- a/backend/common_test.go +++ b/backend/common_test.go @@ -381,3 +381,43 @@ func TestComputeMultipartETagFromPartETags(t *testing.T) { }) } } + +func TestValidateObjectPutPreconditionHeaders(t *testing.T) { + str := func(s string) *string { return &s } + + tests := []struct { + name string + ifMatch *string + ifNoneMatch *string + wantErr error + }{ + {name: "no headers"}, + {name: "if-match etag", ifMatch: str("etag")}, + {name: "if-none-match star", ifNoneMatch: str("*")}, + { + name: "both headers", + ifMatch: str("*"), + ifNoneMatch: str("etag"), + wantErr: s3err.GetNotImplementedErr("If-Match,If-None-Match", s3err.NmpAdditionalMessageMultipleCondHeaders), + }, + { + name: "if-none-match etag", + ifNoneMatch: str("etag"), + wantErr: s3err.GetNotImplementedErr("If-None-Match", s3err.NmpAdditionalMessageIfNoneMatch), + }, + { + name: "if-match star", + ifMatch: str("*"), + wantErr: s3err.GetNotImplementedErr("If-Match", s3err.NmpAdditionalMessageIfMatch), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateObjectPutPreconditionHeaders(tt.ifMatch, tt.ifNoneMatch) + if !reflect.DeepEqual(err, tt.wantErr) { + t.Fatalf("expected error %v, got %v", tt.wantErr, err) + } + }) + } +} diff --git a/backend/posix/posix.go b/backend/posix/posix.go index 3b1feb84..825d4f58 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -2446,7 +2446,7 @@ func (p *Posix) CompleteMultipartUploadWithCopy(ctx context.Context, input *s3.C return res, "", err } - if err := validateUploadID(uploadID); err != nil { + if err := p.ValidateUploadID(uploadID); err != nil { return res, "", err } @@ -3232,20 +3232,16 @@ func numberOfChecksums(part types.CompletedPart) (int, string) { return counter, builder.String() } -// validateUploadID returns NoSuchUpload for an upload ID this backend never +// ValidateUploadID returns NoSuchUpload for an upload ID this backend never // generates. Upload IDs are UUIDs in canonical form, and each one names its // upload's directory, so an ID holding a path separator or ".." would // otherwise reach a path outside the bucket. -func validateUploadID(uploadID string) error { - id, err := uuid.Parse(uploadID) - if err != nil || id.String() != uploadID { - return s3err.GetNoSuchUploadErr(uploadID) - } - return nil +func (p *Posix) ValidateUploadID(uploadID string) error { + return backend.ValidateUUIDUploadID(uploadID) } func (p *Posix) checkUploadIDExists(bucket, object, uploadID string) ([32]byte, error) { - if err := validateUploadID(uploadID); err != nil { + if err := p.ValidateUploadID(uploadID); err != nil { return [32]byte{}, err } @@ -3502,7 +3498,7 @@ func (p *Posix) AbortMultipartUpload(ctx context.Context, mpu *s3.AbortMultipart return err } - if err := validateUploadID(uploadID); err != nil { + if err := p.ValidateUploadID(uploadID); err != nil { return err } @@ -3871,7 +3867,7 @@ func (p *Posix) UploadPartWithPostFunc(ctx context.Context, input *s3.UploadPart return nil, err } - if err := validateUploadID(uploadID); err != nil { + if err := p.ValidateUploadID(uploadID); err != nil { return nil, err } @@ -4215,7 +4211,7 @@ func (p *Posix) UploadPartCopy(ctx context.Context, upi *s3.UploadPartCopyInput) return s3response.CopyPartResult{}, err } - if err := validateUploadID(*upi.UploadId); err != nil { + if err := p.ValidateUploadID(*upi.UploadId); err != nil { return s3response.CopyPartResult{}, err } diff --git a/s3api/controllers/backend_moq_test.go b/s3api/controllers/backend_moq_test.go index ba96058e..a3bb08da 100644 --- a/s3api/controllers/backend_moq_test.go +++ b/s3api/controllers/backend_moq_test.go @@ -197,6 +197,9 @@ var _ backend.Backend = &BackendMock{} // UploadPartCopyFunc: func(contextMoqParam context.Context, uploadPartCopyInput *s3.UploadPartCopyInput) (s3response.CopyPartResult, error) { // panic("mock out the UploadPartCopy method") // }, +// ValidateUploadIDFunc: func(uploadID string) error { +// panic("mock out the ValidateUploadID method") +// }, // } // // // use mockedBackend in code that requires backend.Backend @@ -378,6 +381,9 @@ type BackendMock struct { // UploadPartCopyFunc mocks the UploadPartCopy method. UploadPartCopyFunc func(contextMoqParam context.Context, uploadPartCopyInput *s3.UploadPartCopyInput) (s3response.CopyPartResult, error) + // ValidateUploadIDFunc mocks the ValidateUploadID method. + ValidateUploadIDFunc func(uploadID string) error + // calls tracks calls to the methods. calls struct { // AbortMultipartUpload holds details about calls to the AbortMultipartUpload method. @@ -830,6 +836,11 @@ type BackendMock struct { // UploadPartCopyInput is the uploadPartCopyInput argument value. UploadPartCopyInput *s3.UploadPartCopyInput } + // ValidateUploadID holds details about calls to the ValidateUploadID method. + ValidateUploadID []struct { + // UploadID is the uploadID argument value. + UploadID string + } } lockAbortMultipartUpload sync.RWMutex lockChangeBucketOwner sync.RWMutex @@ -889,6 +900,7 @@ type BackendMock struct { lockString sync.RWMutex lockUploadPart sync.RWMutex lockUploadPartCopy sync.RWMutex + lockValidateUploadID sync.RWMutex } // AbortMultipartUpload calls AbortMultipartUploadFunc. @@ -3064,3 +3076,35 @@ func (mock *BackendMock) UploadPartCopyCalls() []struct { mock.lockUploadPartCopy.RUnlock() return calls } + +// ValidateUploadID calls ValidateUploadIDFunc. +func (mock *BackendMock) ValidateUploadID(uploadID string) error { + if mock.ValidateUploadIDFunc == nil { + panic("BackendMock.ValidateUploadIDFunc: method is nil but Backend.ValidateUploadID was just called") + } + callInfo := struct { + UploadID string + }{ + UploadID: uploadID, + } + mock.lockValidateUploadID.Lock() + mock.calls.ValidateUploadID = append(mock.calls.ValidateUploadID, callInfo) + mock.lockValidateUploadID.Unlock() + return mock.ValidateUploadIDFunc(uploadID) +} + +// ValidateUploadIDCalls gets all the calls that were made to ValidateUploadID. +// Check the length with: +// +// len(mockedBackend.ValidateUploadIDCalls()) +func (mock *BackendMock) ValidateUploadIDCalls() []struct { + UploadID string +} { + var calls []struct { + UploadID string + } + mock.lockValidateUploadID.RLock() + calls = mock.calls.ValidateUploadID + mock.lockValidateUploadID.RUnlock() + return calls +} diff --git a/s3api/controllers/object-delete.go b/s3api/controllers/object-delete.go index a0834aca..5edd59b7 100644 --- a/s3api/controllers/object-delete.go +++ b/s3api/controllers/object-delete.go @@ -93,7 +93,17 @@ func (c S3ApiController) AbortMultipartUpload(ctx fiber.Ctx) (*Response, error) isBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) - err := c.verifyAccess(ctx, + // A malformed upload ID is rejected before access is verified. + err := c.be.ValidateUploadID(uploadId) + if 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-delete_test.go b/s3api/controllers/object-delete_test.go index d2f90113..ed72fede 100644 --- a/s3api/controllers/object-delete_test.go +++ b/s3api/controllers/object-delete_test.go @@ -143,6 +143,24 @@ func TestS3ApiController_AbortMultipartUpload(t *testing.T) { input testInput output testOutput }{ + { + name: "malformed upload id", + input: testInput{ + locals: accessDeniedLocals, + extraMockErr: s3err.GetNoSuchUploadErr("bad"), + queries: map[string]string{ + "uploadId": "bad", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetNoSuchUploadErr("bad"), + }, + }, { name: "verify access fails", input: testInput{ @@ -191,6 +209,9 @@ func TestS3ApiController_AbortMultipartUpload(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { be := &BackendMock{ + ValidateUploadIDFunc: func(uploadID string) error { + return tt.input.extraMockErr + }, AbortMultipartUploadFunc: func(contextMoqParam context.Context, abortMultipartUploadInput *s3.AbortMultipartUploadInput) error { return tt.input.beErr }, @@ -209,7 +230,8 @@ func TestS3ApiController_AbortMultipartUpload(t *testing.T) { tt.output.response, tt.output.err, ctxInputs{ - locals: tt.input.locals, + locals: tt.input.locals, + queries: tt.input.queries, }) }) } diff --git a/s3api/controllers/object-get.go b/s3api/controllers/object-get.go index 18db442e..8684036d 100644 --- a/s3api/controllers/object-get.go +++ b/s3api/controllers/object-get.go @@ -263,7 +263,37 @@ func (c S3ApiController) ListParts(ctx fiber.Ctx) (*Response, error) { parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) isPublicBucket := utils.ContextKeyPublicBucket.IsSet(ctx) - err := c.verifyAccess(ctx, auth.AccessOptions{ + // A part number marker or max parts that isn't an integer is rejected + // first, then a malformed upload ID, both before access is verified. + // Negative values are rejected after. + err := utils.ValidateLimiterInteger(partNumberMarker, utils.LimiterTypePartNumberMarker) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + + err = utils.ValidateLimiterInteger(maxPartsStr, utils.LimiterTypeMaxParts) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + + err = c.be.ValidateUploadID(uploadId) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + + err = c.verifyAccess(ctx, auth.AccessOptions{ Acl: parsedAcl, AclPermission: auth.PermissionRead, IsRoot: isRoot, diff --git a/s3api/controllers/object-get_test.go b/s3api/controllers/object-get_test.go index e6f0b76a..b4587071 100644 --- a/s3api/controllers/object-get_test.go +++ b/s3api/controllers/object-get_test.go @@ -478,6 +478,44 @@ func TestS3ApiController_ListParts(t *testing.T) { input testInput output testOutput }{ + { + name: "non integer max parts", + input: testInput{ + locals: accessDeniedLocals, + extraMockErr: s3err.GetNoSuchUploadErr("bad"), + queries: map[string]string{ + "max-parts": "foo", + "uploadId": "bad", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgMaxLimiter(string(utils.LimiterTypeMaxParts), "foo"), + }, + }, + { + name: "malformed upload id", + input: testInput{ + locals: accessDeniedLocals, + extraMockErr: s3err.GetNoSuchUploadErr("bad"), + queries: map[string]string{ + "max-parts": "-1", + "uploadId": "bad", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetNoSuchUploadErr("bad"), + }, + }, { name: "verify access fails", input: testInput{ @@ -567,6 +605,9 @@ func TestS3ApiController_ListParts(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { be := &BackendMock{ + ValidateUploadIDFunc: func(uploadID string) error { + return tt.input.extraMockErr + }, ListPartsFunc: func(contextMoqParam context.Context, listPartsInput *s3.ListPartsInput) (s3response.ListPartsResult, error) { if tt.input.headers != nil { assert.Equal(t, "invalid", *listPartsInput.SSECustomerAlgorithm) diff --git a/s3api/controllers/object-post.go b/s3api/controllers/object-post.go index 52fa9e8a..539f7ef6 100644 --- a/s3api/controllers/object-post.go +++ b/s3api/controllers/object-post.go @@ -24,6 +24,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/gofiber/fiber/v3" "github.com/versity/versitygw/auth" + "github.com/versity/versitygw/backend" "github.com/versity/versitygw/debuglogger" "github.com/versity/versitygw/s3api/utils" "github.com/versity/versitygw/s3err" @@ -290,7 +291,28 @@ func (c S3ApiController) CompleteMultipartUpload(ctx fiber.Ctx) (*Response, erro isBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) - err := c.verifyAccess(ctx, + // Unsupported conditional headers are rejected first, then a malformed + // upload ID, both before access is verified or the body is parsed. + ifMatch, ifNoneMatch := utils.ParsePreconditionMatchHeaders(ctx) + err := backend.ValidateObjectPutPreconditionHeaders(ifMatch, ifNoneMatch) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + + err = c.be.ValidateUploadID(uploadId) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + + err = c.verifyAccess(ctx, auth.AccessOptions{ Acl: parsedAcl, AclPermission: auth.PermissionWrite, @@ -380,8 +402,6 @@ func (c S3ApiController) CompleteMultipartUpload(ctx fiber.Ctx) (*Response, erro }, err } - ifMatch, ifNoneMatch := utils.ParsePreconditionMatchHeaders(ctx) - err = auth.CheckObjectAccess(ctx, bucket, acct, []types.ObjectIdentifier{{Key: &key}}, auth.BypassOverwrite, isBucketPublic, c.be, c.iam, true) if err != nil { return &Response{ diff --git a/s3api/controllers/object-post_test.go b/s3api/controllers/object-post_test.go index 9cc3e9fe..1267e10b 100644 --- a/s3api/controllers/object-post_test.go +++ b/s3api/controllers/object-post_test.go @@ -379,6 +379,45 @@ func TestS3ApiController_CompleteMultipartUpload(t *testing.T) { input testInput output testOutput }{ + { + name: "unsupported conditional header", + input: testInput{ + locals: accessDeniedLocals, + body: []byte("invalid_body"), + headers: map[string]string{ + "If-None-Match": "ETag", + }, + queries: map[string]string{ + "uploadId": "bad", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetNotImplementedErr("If-None-Match", s3err.NmpAdditionalMessageIfNoneMatch), + }, + }, + { + name: "malformed upload id", + input: testInput{ + locals: accessDeniedLocals, + body: []byte("invalid_body"), + queries: map[string]string{ + "uploadId": "bad", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetNoSuchUploadErr("bad"), + }, + }, { name: "verify access fails", input: testInput{ @@ -597,6 +636,12 @@ func TestS3ApiController_CompleteMultipartUpload(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { be := &BackendMock{ + ValidateUploadIDFunc: func(uploadID string) error { + if uploadID == "bad" { + return s3err.GetNoSuchUploadErr(uploadID) + } + return nil + }, CompleteMultipartUploadFunc: func(contextMoqParam context.Context, completeMultipartUploadInput *s3.CompleteMultipartUploadInput) (s3response.CompleteMultipartUploadResult, string, error) { return tt.input.beRes.(s3response.CompleteMultipartUploadResult), versionId, tt.input.beErr }, @@ -624,6 +669,7 @@ func TestS3ApiController_CompleteMultipartUpload(t *testing.T) { locals: tt.input.locals, body: tt.input.body, headers: tt.input.headers, + queries: tt.input.queries, }) }) } diff --git a/s3api/controllers/object-put.go b/s3api/controllers/object-put.go index 9cd73f44..300c0428 100644 --- a/s3api/controllers/object-put.go +++ b/s3api/controllers/object-put.go @@ -248,7 +248,19 @@ func (c S3ApiController) UploadPart(ctx fiber.Ctx) (*Response, error) { } decodedLength := ctx.Get("X-Amz-Decoded-Content-Length") - err := c.verifyAccess(ctx, + // A malformed upload ID is rejected before access is verified and + // before the part number, the x-amz-checksum headers or the body are + // checked. + err := c.be.ValidateUploadID(uploadId) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + + err = c.verifyAccess(ctx, auth.AccessOptions{ Acl: parsedAcl, AclPermission: auth.PermissionWrite, @@ -396,7 +408,18 @@ func (c S3ApiController) UploadPartCopy(ctx fiber.Ctx) (*Response, error) { IsBucketPublic := utils.ContextKeyPublicBucket.IsSet(ctx) parsedAcl := utils.ContextKeyParsedAcl.Get(ctx).(auth.ACL) - err := utils.ValidateCopySource(copySource) + // A malformed upload ID is rejected before access is verified and + // before the part number or anything about the copy source is checked. + err := c.be.ValidateUploadID(uploadId) + if err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + + err = utils.ValidateCopySource(copySource) if err != nil { return &Response{ MetaOpts: &MetaOptions{ diff --git a/s3api/controllers/object-put_test.go b/s3api/controllers/object-put_test.go index 6ac7059f..70e5ce19 100644 --- a/s3api/controllers/object-put_test.go +++ b/s3api/controllers/object-put_test.go @@ -470,6 +470,25 @@ func TestS3ApiController_UploadPart(t *testing.T) { input testInput output testOutput }{ + { + name: "malformed upload id", + input: testInput{ + locals: accessDeniedLocals, + extraMockErr: s3err.GetNoSuchUploadErr("bad"), + queries: map[string]string{ + "partNumber": "-2", + "uploadId": "bad", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetNoSuchUploadErr("bad"), + }, + }, { name: "verify access fails", input: testInput{ @@ -618,6 +637,9 @@ func TestS3ApiController_UploadPart(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { be := &BackendMock{ + ValidateUploadIDFunc: func(uploadID string) error { + return tt.input.extraMockErr + }, UploadPartFunc: func(contextMoqParam context.Context, uploadPartInput *s3.UploadPartInput) (*s3.UploadPartOutput, error) { return tt.input.beRes.(*s3.UploadPartOutput), tt.input.beErr }, @@ -648,6 +670,9 @@ func TestS3ApiController_UploadPart(t *testing.T) { func TestS3ApiController_UploadPartPlainBodyUsesContentLength(t *testing.T) { be := &BackendMock{ + ValidateUploadIDFunc: func(string) error { + return nil + }, UploadPartFunc: func(_ context.Context, input *s3.UploadPartInput) (*s3.UploadPartOutput, error) { assert.Equal(t, int64(5), *input.ContentLength) body, err := io.ReadAll(input.Body) @@ -700,6 +725,28 @@ func TestS3ApiController_UploadPartCopy(t *testing.T) { input testInput output testOutput }{ + { + name: "malformed upload id", + input: testInput{ + locals: accessDeniedLocals, + extraMockErr: s3err.GetNoSuchUploadErr("bad"), + headers: map[string]string{ + "X-Amz-Copy-Source": "bad%G1", + }, + queries: map[string]string{ + "partNumber": "-2", + "uploadId": "bad", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetNoSuchUploadErr("bad"), + }, + }, { name: "verify access fails", input: testInput{ @@ -861,6 +908,9 @@ func TestS3ApiController_UploadPartCopy(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { be := &BackendMock{ + ValidateUploadIDFunc: func(uploadID string) error { + return tt.input.extraMockErr + }, UploadPartCopyFunc: func(contextMoqParam context.Context, uploadPartCopyInput *s3.UploadPartCopyInput) (s3response.CopyPartResult, error) { return tt.input.beRes.(s3response.CopyPartResult), tt.input.beErr }, diff --git a/s3api/utils/utils.go b/s3api/utils/utils.go index b2257f66..50c39451 100644 --- a/s3api/utils/utils.go +++ b/s3api/utils/utils.go @@ -176,14 +176,9 @@ func ParseMaxLimiter(limiter string, lt LimiterType) (int32, error) { return defaultMaxLimiter, nil } - num, err := strconv.ParseInt(limiter, 10, 32) + num, err := parseLimiterInt(limiter, lt) if err != nil { - // versions_max_keys follows max-keys error semantics. - if lt == LimiterTypeVersionsMaxKeys { - lt = LimiterTypeMaxKeys - } - debuglogger.Logf("invalid %s provided: %s\n", lt, limiter) - return 0, s3err.GetInvalidArgMaxLimiter(string(lt), limiter) + return 0, err } // max-buckets has distinct range rules and errors. @@ -219,6 +214,30 @@ func ParseMaxLimiter(limiter string, lt LimiterType) (int32, error) { return int32(num), nil } +// ValidateLimiterInteger returns the InvalidArgument error ParseMaxLimiter +// gives a limiter that is set but isn't a 32-bit integer, without checking +// its range. +func ValidateLimiterInteger(limiter string, lt LimiterType) error { + if limiter == "" { + return nil + } + _, err := parseLimiterInt(limiter, lt) + return err +} + +func parseLimiterInt(limiter string, lt LimiterType) (int64, error) { + num, err := strconv.ParseInt(limiter, 10, 32) + if err != nil { + // versions_max_keys follows max-keys error semantics. + if lt == LimiterTypeVersionsMaxKeys { + lt = LimiterTypeMaxKeys + } + debuglogger.Logf("invalid %s provided: %s\n", lt, limiter) + return 0, s3err.GetInvalidArgMaxLimiter(string(lt), limiter) + } + return num, nil +} + type CustomHeader struct { Key string Value string diff --git a/s3api/utils/utils_test.go b/s3api/utils/utils_test.go index 5e3d6659..20f31ee5 100644 --- a/s3api/utils/utils_test.go +++ b/s3api/utils/utils_test.go @@ -437,6 +437,28 @@ func TestParseMaxLimiter(t *testing.T) { } } +func TestValidateLimiterInteger(t *testing.T) { + tests := []struct { + name string + limiter string + lt LimiterType + err error + }{ + {"empty", "", LimiterTypeMaxParts, nil}, + {"valid", "23", LimiterTypeMaxParts, nil}, + {"negative", "-1", LimiterTypeMaxParts, nil}, + {"above default", "2000", LimiterTypePartNumberMarker, nil}, + {"not an integer", "bla", LimiterTypePartNumberMarker, s3err.GetInvalidArgMaxLimiter(string(LimiterTypePartNumberMarker), "bla")}, + {"above int32", "2147483648", LimiterTypeMaxParts, s3err.GetInvalidArgMaxLimiter(string(LimiterTypeMaxParts), "2147483648")}, + {"versions max keys", "bla", LimiterTypeVersionsMaxKeys, s3err.GetInvalidArgMaxLimiter(string(LimiterTypeMaxKeys), "bla")}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.err, ValidateLimiterInteger(tt.limiter, tt.lt)) + }) + } +} + func TestFilterObjectAttributes(t *testing.T) { type args struct { attrs map[s3response.ObjectAttributes]struct{} diff --git a/tests/integration/Access_Control.go b/tests/integration/Access_Control.go index 7b202316..db0eb168 100644 --- a/tests/integration/Access_Control.go +++ b/tests/integration/Access_Control.go @@ -462,6 +462,160 @@ func AccessControl_copy_object_with_starting_slash_for_user(s *S3Conf) error { }) } +func AccessControl_UploadPartCopy_malformed_upload_id_without_source_access(s *S3Conf) error { + testName := "AccessControl_UploadPartCopy_malformed_upload_id_without_source_access" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + srcBucket := getBucketName() + err := setup(s, srcBucket) + if err != nil { + return err + } + objs, err := putObjects(s3client, []string{"src-obj"}, srcBucket) + if err != nil { + return err + } + + testuser := getUser("user") + err = createUsers(s, []user{testuser}) + if err != nil { + return err + } + if err := changeBucketsOwner(s, []string{bucket}, testuser.access); err != nil { + return err + } + + userClient := s.getUserClient(testuser) + obj := "my-obj" + mp, err := createMp(userClient, bucket, obj) + if err != nil { + return err + } + + // The user can't read the source bucket, but a malformed upload + // ID is reported first. + for i, test := range []struct { + uploadId string + expectedErr s3err.S3Error + }{ + {"../invalid-upload", s3err.GetAPIError(s3err.ErrNoSuchUpload)}, + {*mp.UploadId, s3err.GetAPIError(s3err.ErrAccessDenied)}, + } { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := userClient.UploadPartCopy(ctx, &s3.UploadPartCopyInput{ + Bucket: &bucket, + Key: &obj, + UploadId: &test.uploadId, + PartNumber: getPtr(int32(1)), + CopySource: getPtr(srcBucket + "/" + *objs[0].Key), + }) + cancel() + if err := checkApiErr(err, test.expectedErr); err != nil { + return fmt.Errorf("test case %d: %w", i+1, err) + } + } + + return teardown(s, srcBucket) + }) +} + +func AccessControl_multipart_malformed_upload_id_without_access(s *S3Conf) error { + testName := "AccessControl_multipart_malformed_upload_id_without_access" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + srcObjs, err := putObjects(s3client, []string{"src-obj"}, bucket) + if err != nil { + return err + } + mp, err := createMp(s3client, bucket, obj) + if err != nil { + return err + } + + testuser := getUser("user") + err = createUsers(s, []user{testuser}) + if err != nil { + return err + } + userClient := s.getUserClient(testuser) + + actions := []struct { + name string + call func(ctx context.Context, uploadId string) error + }{ + {"UploadPart", func(ctx context.Context, uploadId string) error { + _, err := userClient.UploadPart(ctx, &s3.UploadPartInput{ + Bucket: &bucket, + Key: &obj, + UploadId: &uploadId, + PartNumber: getPtr(int32(1)), + Body: strings.NewReader("part data"), + }) + return err + }}, + {"UploadPartCopy", func(ctx context.Context, uploadId string) error { + _, err := userClient.UploadPartCopy(ctx, &s3.UploadPartCopyInput{ + Bucket: &bucket, + Key: &obj, + UploadId: &uploadId, + PartNumber: getPtr(int32(1)), + CopySource: getPtr(bucket + "/" + *srcObjs[0].Key), + }) + return err + }}, + {"ListParts", func(ctx context.Context, uploadId string) error { + _, err := userClient.ListParts(ctx, &s3.ListPartsInput{ + Bucket: &bucket, + Key: &obj, + UploadId: &uploadId, + }) + return err + }}, + {"CompleteMultipartUpload", func(ctx context.Context, uploadId string) error { + _, err := userClient.CompleteMultipartUpload(ctx, &s3.CompleteMultipartUploadInput{ + Bucket: &bucket, + Key: &obj, + UploadId: &uploadId, + MultipartUpload: &types.CompletedMultipartUpload{ + Parts: []types.CompletedPart{ + {ETag: getPtr("etag"), PartNumber: getPtr(int32(1))}, + }, + }, + }) + return err + }}, + {"AbortMultipartUpload", func(ctx context.Context, uploadId string) error { + _, err := userClient.AbortMultipartUpload(ctx, &s3.AbortMultipartUploadInput{ + Bucket: &bucket, + Key: &obj, + UploadId: &uploadId, + }) + return err + }}, + } + + // The user has no access to the bucket, but a malformed upload ID + // is reported first. + for _, test := range []struct { + uploadId string + expectedErr s3err.S3Error + }{ + {"../invalid-upload", s3err.GetAPIError(s3err.ErrNoSuchUpload)}, + {*mp.UploadId, s3err.GetAPIError(s3err.ErrAccessDenied)}, + } { + for _, action := range actions { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + err := action.call(ctx, test.uploadId) + cancel() + if err := checkApiErr(err, test.expectedErr); err != nil { + return fmt.Errorf("%s with upload id %q: %w", action.name, test.uploadId, err) + } + } + } + + return nil + }) +} + func AccessControl_policy_normalizes_object_key_for_get_put_delete(s *S3Conf) error { testName := "AccessControl_policy_normalizes_object_key_for_get_put_delete" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/CompleteMultipartUpload.go b/tests/integration/CompleteMultipartUpload.go index ec98eb36..910f5502 100644 --- a/tests/integration/CompleteMultipartUpload.go +++ b/tests/integration/CompleteMultipartUpload.go @@ -102,6 +102,59 @@ func CompleteMultipartUpload_upload_id_path_traversal(s *S3Conf) error { }) } +func CompleteMultipartUpload_malformed_upload_id(s *S3Conf) error { + testName := "CompleteMultipartUpload_malformed_upload_id" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + parts := &types.CompletedMultipartUpload{ + Parts: []types.CompletedPart{ + {ETag: getPtr("etag"), PartNumber: getPtr(int32(1))}, + }, + } + + // Unsupported conditional headers are reported ahead of a malformed + // upload ID, a missing part list, a negative object size or an + // invalid checksum type after it. + for i, test := range []struct { + input *s3.CompleteMultipartUploadInput + expectedErr s3err.S3Error + }{ + { + &s3.CompleteMultipartUploadInput{IfNoneMatch: getPtr("etag")}, + s3err.GetNotImplementedErr("If-None-Match", s3err.NmpAdditionalMessageIfNoneMatch), + }, + { + &s3.CompleteMultipartUploadInput{IfMatch: getPtr("*")}, + s3err.GetNotImplementedErr("If-Match", s3err.NmpAdditionalMessageIfMatch), + }, + { + &s3.CompleteMultipartUploadInput{}, + s3err.GetAPIError(s3err.ErrNoSuchUpload), + }, + { + &s3.CompleteMultipartUploadInput{MultipartUpload: parts, MpuObjectSize: getPtr(int64(-1))}, + s3err.GetAPIError(s3err.ErrNoSuchUpload), + }, + { + &s3.CompleteMultipartUploadInput{MultipartUpload: parts, ChecksumType: types.ChecksumType("invalid")}, + s3err.GetAPIError(s3err.ErrNoSuchUpload), + }, + } { + test.input.Bucket = &bucket + test.input.Key = getPtr("my-obj") + test.input.UploadId = getPtr("../invalid-upload") + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.CompleteMultipartUpload(ctx, test.input) + cancel() + if err := checkApiErr(err, test.expectedErr); err != nil { + return fmt.Errorf("test case %d: %w", i+1, err) + } + } + + return nil + }) +} + func CompleteMultipartUpload_incorrect_part_number(s *S3Conf) error { testName := "CompleteMultipartUpload_incorrect_part_number" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/ListParts.go b/tests/integration/ListParts.go index c5663ca0..c7409066 100644 --- a/tests/integration/ListParts.go +++ b/tests/integration/ListParts.go @@ -63,6 +63,44 @@ func ListParts_upload_id_path_traversal(s *S3Conf) error { }) } +func ListParts_malformed_upload_id(s *S3Conf) error { + testName := "ListParts_malformed_upload_id" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + // A part number marker that isn't an integer is reported ahead of a + // malformed upload ID, a negative one or negative max parts after it. + for i, test := range []struct { + input *s3.ListPartsInput + expectedErr s3err.S3Error + }{ + { + &s3.ListPartsInput{PartNumberMarker: getPtr("invalid")}, + s3err.GetInvalidArgMaxLimiter("part-number-marker", "invalid"), + }, + { + &s3.ListPartsInput{PartNumberMarker: getPtr("-14")}, + s3err.GetAPIError(s3err.ErrNoSuchUpload), + }, + { + &s3.ListPartsInput{MaxParts: getPtr(int32(-3))}, + s3err.GetAPIError(s3err.ErrNoSuchUpload), + }, + } { + test.input.Bucket = &bucket + test.input.Key = getPtr("my-obj") + test.input.UploadId = getPtr("../invalid-upload") + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.ListParts(ctx, test.input) + cancel() + if err := checkApiErr(err, test.expectedErr); err != nil { + return fmt.Errorf("test case %d: %w", i+1, err) + } + } + + return nil + }) +} + func ListParts_incorrect_object_key(s *S3Conf) error { testName := "ListParts_incorrect_object_key" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/UploadPart.go b/tests/integration/UploadPart.go index a701c987..8035093d 100644 --- a/tests/integration/UploadPart.go +++ b/tests/integration/UploadPart.go @@ -143,6 +143,34 @@ func UploadPart_upload_id_path_traversal(s *S3Conf) error { }) } +func UploadPart_malformed_upload_id(s *S3Conf) error { + testName := "UploadPart_malformed_upload_id" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + // A malformed upload ID is reported ahead of an invalid part + // number and of invalid or mismatched checksums. + for i, input := range []*s3.UploadPartInput{ + {PartNumber: getPtr(int32(-10))}, + {PartNumber: getPtr(int32(1)), ChecksumCRC32: getPtr("invalid_base64!")}, + {PartNumber: getPtr(int32(1)), ChecksumCRC32: getPtr("AAAAAA==")}, + {PartNumber: getPtr(int32(1)), ContentMD5: getPtr("nZ8ysQ5XDHwZxKA3cpfRYQ==")}, + } { + input.Bucket = &bucket + input.Key = getPtr("my-obj") + input.UploadId = getPtr("../invalid-upload") + input.Body = strings.NewReader("part data") + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.UploadPart(ctx, input) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrNoSuchUpload)); err != nil { + return fmt.Errorf("test case %d: %w", i+1, err) + } + } + + return nil + }) +} + func UploadPart_multiple_checksum_headers(s *S3Conf) error { testName := "UploadPart_multiple_checksum_headers" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/UploadPartCopy.go b/tests/integration/UploadPartCopy.go index f42bea8d..145bc44b 100644 --- a/tests/integration/UploadPartCopy.go +++ b/tests/integration/UploadPartCopy.go @@ -138,6 +138,44 @@ func UploadPartCopy_upload_id_path_traversal(s *S3Conf) error { }) } +func UploadPartCopy_malformed_upload_id(s *S3Conf) error { + testName := "UploadPartCopy_malformed_upload_id" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + objs, err := putObjects(s3client, []string{"src-obj"}, bucket) + if err != nil { + return err + } + + // A malformed upload ID is reported ahead of an invalid part + // number and of any copy source error. + for i, test := range []struct { + copySource string + partNumber int32 + }{ + {bucket + "/" + *objs[0].Key, -10}, + {bucket, 1}, + {bucket + "/%ZZ", 1}, + {"non-existing-bucket/src-obj", 1}, + {bucket + "/non-existing-obj", 1}, + } { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.UploadPartCopy(ctx, &s3.UploadPartCopyInput{ + Bucket: &bucket, + Key: getPtr("my-obj"), + UploadId: getPtr("../invalid-upload"), + PartNumber: &test.partNumber, + CopySource: &test.copySource, + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrNoSuchUpload)); err != nil { + return fmt.Errorf("test case %d: %w", i+1, err) + } + } + + return nil + }) +} + func UploadPartCopy_incorrect_object_key(s *S3Conf) error { testName := "UploadPartCopy_incorrect_object_key" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -182,13 +220,19 @@ func UploadPartCopy_incorrect_object_key(s *S3Conf) error { func UploadPartCopy_invalid_part_number(s *S3Conf) error { testName := "UploadPartCopy_invalid_part_number" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + out, err := createMp(s3client, bucket, obj) + if err != nil { + return err + } + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) partNumber := int32(-10) - _, err := s3client.UploadPartCopy(ctx, &s3.UploadPartCopyInput{ + _, err = s3client.UploadPartCopy(ctx, &s3.UploadPartCopyInput{ Bucket: &bucket, CopySource: getPtr("bucket/key"), - UploadId: getPtr("uploadId"), - Key: getPtr("non-existing-object-key"), + UploadId: out.UploadId, + Key: &obj, PartNumber: &partNumber, }) cancel() @@ -203,6 +247,12 @@ func UploadPartCopy_invalid_part_number(s *S3Conf) error { func UploadPartCopy_invalid_copy_source(s *S3Conf) error { testName := "UploadPartCopy_invalid_copy_source" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "obj" + out, err := createMp(s3client, bucket, obj) + if err != nil { + return err + } + partNumber := int32(1) for _, test := range []struct { copySource string @@ -292,8 +342,8 @@ func UploadPartCopy_invalid_copy_source(s *S3Conf) error { ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) _, err := s3client.UploadPartCopy(ctx, &s3.UploadPartCopyInput{ Bucket: &bucket, - Key: getPtr("obj"), - UploadId: getPtr("mock-upload-id"), + Key: &obj, + UploadId: out.UploadId, CopySource: &test.copySource, PartNumber: &partNumber, }) diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 4638b831..d879c4e2 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -499,6 +499,7 @@ func TestUploadPart(ts *TestState) { ts.Run(UploadPart_non_existing_key) ts.Run(UploadPart_non_existing_mp_upload) ts.Run(UploadPart_upload_id_path_traversal) + ts.Run(UploadPart_malformed_upload_id) //TODO: remove the condition after implementing checksums in azure if !ts.conf.azureTests { ts.Run(UploadPart_multiple_checksum_headers) @@ -522,6 +523,7 @@ func TestUploadPartCopy(ts *TestState) { ts.Run(UploadPartCopy_non_existing_bucket) ts.Run(UploadPartCopy_incorrect_uploadId) ts.Run(UploadPartCopy_upload_id_path_traversal) + ts.Run(UploadPartCopy_malformed_upload_id) ts.Run(UploadPartCopy_incorrect_object_key) ts.Run(UploadPartCopy_invalid_part_number) ts.Run(UploadPartCopy_invalid_copy_source) @@ -545,6 +547,7 @@ func TestUploadPartCopy(ts *TestState) { func TestListParts(ts *TestState) { ts.Run(ListParts_incorrect_uploadId) ts.Run(ListParts_upload_id_path_traversal) + ts.Run(ListParts_malformed_upload_id) ts.Run(ListParts_incorrect_object_key) ts.Run(ListParts_invalid_max_parts) ts.Run(ListParts_invalid_part_number_marker) @@ -595,6 +598,7 @@ func TestAbortMultipartUpload(ts *TestState) { func TestCompleteMultipartUpload(ts *TestState) { ts.Run(CompletedMultipartUpload_non_existing_bucket) ts.Run(CompleteMultipartUpload_upload_id_path_traversal) + ts.Run(CompleteMultipartUpload_malformed_upload_id) ts.Run(CompleteMultipartUpload_incorrect_part_number) ts.Run(CompleteMultipartUpload_invalid_part_number) ts.Run(CompleteMultipartUpload_default_content_type) @@ -1172,6 +1176,7 @@ func TestScoutfs(ts *TestState) { ts.Run(CompletedMultipartUpload_non_existing_bucket) ts.Run(CompleteMultipartUpload_upload_id_path_traversal) + ts.Run(CompleteMultipartUpload_malformed_upload_id) ts.Run(CompleteMultipartUpload_incorrect_part_number) ts.Run(CompleteMultipartUpload_invalid_part_number) ts.Run(CompleteMultipartUpload_invalid_ETag) @@ -2004,6 +2009,8 @@ func TestAccessControl(ts *TestState) { ts.Run(AccessControl_root_PutBucketAcl) ts.Run(AccessControl_user_PutBucketAcl_with_policy_access) ts.Run(AccessControl_copy_object_with_starting_slash_for_user) + ts.Run(AccessControl_UploadPartCopy_malformed_upload_id_without_source_access) + ts.Run(AccessControl_multipart_malformed_upload_id_without_access) ts.Run(AccessControl_PutObject_with_tagging_policy) ts.Run(AccessControl_PutObject_with_legal_hold_policy) ts.Run(AccessControl_PutObject_with_retention_policy) @@ -3323,6 +3330,7 @@ func GetIntTests() IntTests { "UploadPart_non_existing_key": UploadPart_non_existing_key, "UploadPart_non_existing_mp_upload": UploadPart_non_existing_mp_upload, "UploadPart_upload_id_path_traversal": UploadPart_upload_id_path_traversal, + "UploadPart_malformed_upload_id": UploadPart_malformed_upload_id, "UploadPart_multiple_checksum_headers": UploadPart_multiple_checksum_headers, "UploadPart_invalid_checksum_header": UploadPart_invalid_checksum_header, "UploadPart_checksum_header_and_algo_mismatch": UploadPart_checksum_header_and_algo_mismatch, @@ -3340,6 +3348,7 @@ func GetIntTests() IntTests { "UploadPartCopy_non_existing_bucket": UploadPartCopy_non_existing_bucket, "UploadPartCopy_incorrect_uploadId": UploadPartCopy_incorrect_uploadId, "UploadPartCopy_upload_id_path_traversal": UploadPartCopy_upload_id_path_traversal, + "UploadPartCopy_malformed_upload_id": UploadPartCopy_malformed_upload_id, "UploadPartCopy_incorrect_object_key": UploadPartCopy_incorrect_object_key, "UploadPartCopy_invalid_part_number": UploadPartCopy_invalid_part_number, "UploadPartCopy_invalid_copy_source": UploadPartCopy_invalid_copy_source, @@ -3358,6 +3367,7 @@ func GetIntTests() IntTests { "UploadPartCopy_data_integrity_etag": UploadPartCopy_data_integrity_etag, "ListParts_incorrect_uploadId": ListParts_incorrect_uploadId, "ListParts_upload_id_path_traversal": ListParts_upload_id_path_traversal, + "ListParts_malformed_upload_id": ListParts_malformed_upload_id, "ListParts_incorrect_object_key": ListParts_incorrect_object_key, "ListParts_invalid_max_parts": ListParts_invalid_max_parts, "ListParts_invalid_part_number_marker": ListParts_invalid_part_number_marker, @@ -3391,6 +3401,7 @@ func GetIntTests() IntTests { "AbortMultipartUpload_if_match_initiated_time": AbortMultipartUpload_if_match_initiated_time, "CompletedMultipartUpload_non_existing_bucket": CompletedMultipartUpload_non_existing_bucket, "CompleteMultipartUpload_upload_id_path_traversal": CompleteMultipartUpload_upload_id_path_traversal, + "CompleteMultipartUpload_malformed_upload_id": CompleteMultipartUpload_malformed_upload_id, "CompleteMultipartUpload_invalid_part_number": CompleteMultipartUpload_invalid_part_number, "CompleteMultipartUpload_default_content_type": CompleteMultipartUpload_default_content_type, "CompleteMultipartUpload_overwrite_resets_attributes": CompleteMultipartUpload_overwrite_resets_attributes, @@ -3719,6 +3730,8 @@ func GetIntTests() IntTests { "AccessControl_root_PutBucketAcl": AccessControl_root_PutBucketAcl, "AccessControl_user_PutBucketAcl_with_policy_access": AccessControl_user_PutBucketAcl_with_policy_access, "AccessControl_copy_object_with_starting_slash_for_user": AccessControl_copy_object_with_starting_slash_for_user, + "AccessControl_UploadPartCopy_malformed_upload_id_without_source_access": AccessControl_UploadPartCopy_malformed_upload_id_without_source_access, + "AccessControl_multipart_malformed_upload_id_without_access": AccessControl_multipart_malformed_upload_id_without_access, "AccessControl_PutObject_with_tagging_policy": AccessControl_PutObject_with_tagging_policy, "AccessControl_PutObject_with_legal_hold_policy": AccessControl_PutObject_with_legal_hold_policy, "AccessControl_PutObject_with_retention_policy": AccessControl_PutObject_with_retention_policy,