diff --git a/backend/azure/azure.go b/backend/azure/azure.go index bef5dd7..102e94c 100644 --- a/backend/azure/azure.go +++ b/backend/azure/azure.go @@ -1197,26 +1197,46 @@ func (az *Azure) CompleteMultipartUpload(ctx context.Context, input *s3.Complete return nil, s3err.GetAPIError(s3err.ErrInvalidPart) } + uncommittedBlocks := map[int32]*blockblob.Block{} + for _, el := range blockList.UncommittedBlocks { + ptNumber, err := decodeBlockId(backend.GetStringFromPtr(el.Name)) + if err != nil { + return nil, fmt.Errorf("invalid block name: %w", err) + } + + uncommittedBlocks[int32(ptNumber)] = el + } + slices.SortFunc(blockList.UncommittedBlocks, func(a *blockblob.Block, b *blockblob.Block) int { ptNumber, _ := decodeBlockId(*a.Name) nextPtNumber, _ := decodeBlockId(*b.Name) return ptNumber - nextPtNumber }) + // The initialie values is the lower limit of partNumber: 0 + var partNumber int32 last := len(blockList.UncommittedBlocks) - 1 - for i, block := range blockList.UncommittedBlocks { - ptNumber, err := decodeBlockId(*block.Name) - if err != nil { + for i, part := range input.MultipartUpload.Parts { + if part.PartNumber == nil { + return nil, s3err.GetAPIError(s3err.ErrInvalidPart) + } + if *part.PartNumber < 1 { + return nil, s3err.GetAPIError(s3err.ErrInvalidCompleteMpPartNumber) + } + if *part.PartNumber <= partNumber { + return nil, s3err.GetAPIError(s3err.ErrInvalidPartOrder) + } + partNumber = *part.PartNumber + + block, ok := uncommittedBlocks[*part.PartNumber] + if !ok { return nil, s3err.GetAPIError(s3err.ErrInvalidPart) } - if *input.MultipartUpload.Parts[i].ETag != *block.Name { + if *part.ETag != *block.Name { return nil, s3err.GetAPIError(s3err.ErrInvalidPart) } - if *input.MultipartUpload.Parts[i].PartNumber != int32(ptNumber) { - return nil, s3err.GetAPIError(s3err.ErrInvalidPart) - } - // all parts except the last need to be greater, thena + // all parts except the last need to be greater, than // the minimum allowed size (5 Mib) if i < last && *block.Size < backend.MinPartSize { return nil, s3err.GetAPIError(s3err.ErrEntityTooSmall) diff --git a/backend/posix/posix.go b/backend/posix/posix.go index af0c3e7..5b3ac88 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -1427,10 +1427,21 @@ func (p *Posix) CompleteMultipartUpload(ctx context.Context, input *s3.CompleteM // check all parts ok last := len(parts) - 1 var totalsize int64 + + // The initialie values is the lower limit of partNumber: 0 + var partNumber int32 for i, part := range parts { - if part.PartNumber == nil || *part.PartNumber < 1 { + if part.PartNumber == nil { return nil, s3err.GetAPIError(s3err.ErrInvalidPart) } + if *part.PartNumber < 1 { + return nil, s3err.GetAPIError(s3err.ErrInvalidCompleteMpPartNumber) + } + if *part.PartNumber <= partNumber { + return nil, s3err.GetAPIError(s3err.ErrInvalidPartOrder) + } + + partNumber = *part.PartNumber partObjPath := filepath.Join(objdir, uploadID, fmt.Sprintf("%v", *part.PartNumber)) fullPartPath := filepath.Join(bucket, partObjPath) @@ -1493,10 +1504,6 @@ func (p *Posix) CompleteMultipartUpload(ctx context.Context, input *s3.CompleteM defer f.cleanup() for _, part := range parts { - if part.PartNumber == nil || *part.PartNumber < 1 { - return nil, s3err.GetAPIError(s3err.ErrInvalidPart) - } - partObjPath := filepath.Join(objdir, uploadID, fmt.Sprintf("%v", *part.PartNumber)) fullPartPath := filepath.Join(bucket, partObjPath) pf, err := os.Open(fullPartPath) diff --git a/s3err/s3err.go b/s3err/s3err.go index b8c7571..e0e9621 100644 --- a/s3err/s3err.go +++ b/s3err/s3err.go @@ -78,6 +78,8 @@ const ( ErrInvalidPart ErrEmptyParts ErrInvalidPartNumber + ErrInvalidPartOrder + ErrInvalidCompleteMpPartNumber ErrInternalError ErrInvalidCopyDest ErrInvalidCopySource @@ -272,6 +274,16 @@ var errorCodeResponse = map[ErrorCode]APIError{ Description: "Part number must be an integer between 1 and 10000, inclusive.", HTTPStatusCode: http.StatusBadRequest, }, + ErrInvalidPartOrder: { + Code: "InvalidPartOrder", + Description: "The list of parts was not in ascending order. Parts must be ordered by part number.", + HTTPStatusCode: http.StatusBadRequest, + }, + ErrInvalidCompleteMpPartNumber: { + Code: "InvalidArgument", + Description: "PartNumber must be >= 1", + HTTPStatusCode: http.StatusBadRequest, + }, ErrInvalidCopyDest: { Code: "InvalidRequest", Description: "This copy request is illegal because it is trying to copy an object to itself without changing the object's metadata, storage class, website redirect location or encryption attributes.", diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index cc1c420..f94dd40 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -388,10 +388,12 @@ func TestAbortMultipartUpload(s *S3Conf) { func TestCompleteMultipartUpload(s *S3Conf) { CompletedMultipartUpload_non_existing_bucket(s) + CompleteMultipartUpload_incorrect_part_number(s) CompleteMultipartUpload_invalid_part_number(s) CompleteMultipartUpload_invalid_ETag(s) CompleteMultipartUpload_small_upload_size(s) CompleteMultipartUpload_empty_parts(s) + CompleteMultipartUpload_incorrect_parts_order(s) //TODO: remove the condition after implementing checksums in azure if !s.azureTests { CompleteMultipartUpload_invalid_checksum_type(s) @@ -971,6 +973,9 @@ func GetIntTests() IntTests { "CompletedMultipartUpload_non_existing_bucket": CompletedMultipartUpload_non_existing_bucket, "CompleteMultipartUpload_invalid_part_number": CompleteMultipartUpload_invalid_part_number, "CompleteMultipartUpload_invalid_ETag": CompleteMultipartUpload_invalid_ETag, + "CompleteMultipartUpload_small_upload_size": CompleteMultipartUpload_small_upload_size, + "CompleteMultipartUpload_empty_parts": CompleteMultipartUpload_empty_parts, + "CompleteMultipartUpload_incorrect_parts_order": CompleteMultipartUpload_incorrect_parts_order, "CompleteMultipartUpload_invalid_checksum_type": CompleteMultipartUpload_invalid_checksum_type, "CompleteMultipartUpload_invalid_checksum_part": CompleteMultipartUpload_invalid_checksum_part, "CompleteMultipartUpload_multiple_checksum_part": CompleteMultipartUpload_multiple_checksum_part, diff --git a/tests/integration/tests.go b/tests/integration/tests.go index 136b5da..040f76b 100644 --- a/tests/integration/tests.go +++ b/tests/integration/tests.go @@ -8547,8 +8547,8 @@ func CompletedMultipartUpload_non_existing_bucket(s *S3Conf) error { }) } -func CompleteMultipartUpload_invalid_part_number(s *S3Conf) error { - testName := "CompleteMultipartUpload_invalid_part_number" +func CompleteMultipartUpload_incorrect_part_number(s *S3Conf) error { + testName := "CompleteMultipartUpload_incorrect_part_number" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { obj := "my-obj" out, err := createMp(s3client, bucket, obj) @@ -9472,6 +9472,90 @@ func CompleteMultipartUpload_empty_parts(s *S3Conf) error { }) } +func CompleteMultipartUpload_incorrect_parts_order(s *S3Conf) error { + testName := "CompleteMultipartUpload_incorrect_parts_order" + 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 + } + + parts, _, err := uploadParts(s3client, 15*1024*1024, 3, bucket, obj, *out.UploadId) + if err != nil { + return err + } + + compParts := []types.CompletedPart{} + for _, el := range parts { + compParts = append(compParts, types.CompletedPart{ + ETag: el.ETag, + PartNumber: el.PartNumber, + }) + } + + compParts[0], compParts[1] = compParts[1], compParts[0] + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CompleteMultipartUpload(ctx, &s3.CompleteMultipartUploadInput{ + Bucket: &bucket, + Key: &obj, + UploadId: out.UploadId, + MultipartUpload: &types.CompletedMultipartUpload{ + Parts: compParts, + }, + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidPartOrder)); err != nil { + return err + } + + return nil + }) +} + +func CompleteMultipartUpload_invalid_part_number(s *S3Conf) error { + testName := "CompleteMultipartUpload_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 + } + + parts, _, err := uploadParts(s3client, 5*1024*1024, 1, bucket, obj, *out.UploadId) + if err != nil { + return err + } + + invPartNumber := int32(-4) + + compParts := []types.CompletedPart{} + for _, el := range parts { + compParts = append(compParts, types.CompletedPart{ + ETag: el.ETag, + PartNumber: &invPartNumber, + }) + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CompleteMultipartUpload(ctx, &s3.CompleteMultipartUploadInput{ + Bucket: &bucket, + Key: &obj, + UploadId: out.UploadId, + MultipartUpload: &types.CompletedMultipartUpload{ + Parts: compParts, + }, + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidCompleteMpPartNumber)); err != nil { + return err + } + + return nil + }) +} + func CompleteMultipartUpload_success(s *S3Conf) error { testName := "CompleteMultipartUpload_success" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {