fix: Adds PartNumber validation for CompleteMultipartUploads parts. A…

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
 
-		if *input.MultipartUpload.Parts[i].ETag != *block.Name {
+		block, ok := uncommittedBlocks[*part.PartNumber]
+		if !ok {
 			return nil, s3err.GetAPIError(s3err.ErrInvalidPart)
 		}
-		if *input.MultipartUpload.Parts[i].PartNumber != int32(ptNumber) {
+
+		if *part.ETag != *block.Name {
 			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)

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)

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.",

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,

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 {