diff --git a/backend/common.go b/backend/common.go index de8d7efd..299735f6 100644 --- a/backend/common.go +++ b/backend/common.go @@ -238,28 +238,20 @@ func ParseCopySource(copySourceHeader string) (string, string, string, error) { copySourceHeader = copySourceHeader[1:] } - // Split the raw header on the versionId query parameter before any - // URL-decoding so that the '?' delimiter is not percent-encoded. - var rawSource, versionId string - i := strings.LastIndex(copySourceHeader, "?versionId=") - if i == -1 { - rawSource = copySourceHeader - } else { - rawSource = copySourceHeader[:i] - versionId = copySourceHeader[i+11:] - } - - // URL-decode the entire source path first so that clients that send the - // bucket/key separator as "%2F" (e.g. AWS .NET SDK v4) are handled - // correctly before we split on a literal '/'. - decoded, err := url.QueryUnescape(rawSource) + // URL-decode the entire header before splitting the versionId suffix so the + // backend interprets encoded separators the same way as controller + // validation. This also preserves encoded bucket/key separators such as + // "%2F" from AWS .NET SDK v4 clients. + decoded, err := url.QueryUnescape(copySourceHeader) if err != nil { - return "", "", "", s3err.GetInvalidArgumentErr(s3err.InvalidArgCopySourceEncoding, rawSource) + return "", "", "", s3err.GetInvalidArgumentErr(s3err.InvalidArgCopySourceEncoding, copySourceHeader) } - srcBucket, srcObject, ok := strings.Cut(decoded, "/") + decodedSource, versionId, _ := strings.Cut(decoded, "?versionId=") + + srcBucket, srcObject, ok := strings.Cut(decodedSource, "/") if !ok { - return "", "", "", s3err.GetInvalidArgumentErr(s3err.InvalidArgCopySourceBucket, rawSource) + return "", "", "", s3err.GetInvalidArgumentErr(s3err.InvalidArgCopySourceBucket, decodedSource) } return srcBucket, srcObject, versionId, nil diff --git a/backend/common_test.go b/backend/common_test.go index a0307471..af398c56 100644 --- a/backend/common_test.go +++ b/backend/common_test.go @@ -252,6 +252,14 @@ func TestParseCopySource(t *testing.T) { wantVersionId: "", wantErr: false, }, + { + name: "encoded versionId separator stays in version id", + copySourceHeader: "bucket/safe%3FversionId%3D..%2f..%2fsecret.txt", + wantBucket: "bucket", + wantObject: "safe", + wantVersionId: "../../secret.txt", + wantErr: false, + }, { name: "invalid URL encoding - incomplete escape", copySourceHeader: "mybucket/object%", diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 5e627c4d..ddfbae1d 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -1143,6 +1143,7 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_PutObject_success) // CopyObject action ts.Run(Versioning_CopyObject_invalid_versionId) + ts.Run(Versioning_CopyObject_encoded_versionid_separator_invalid_versionId) ts.Run(Versioning_CopyObject_success) ts.Run(Versioning_CopyObject_non_existing_version_id) ts.Run(Versioning_CopyObject_from_an_object_version) @@ -1205,6 +1206,7 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_Multipart_Upload_success) ts.Run(Versioning_Multipart_Upload_overwrite_an_object) ts.Run(Versioning_UploadPartCopy_invalid_versionId) + ts.Run(Versioning_UploadPartCopy_encoded_versionid_separator_invalid_versionId) ts.Run(Versioning_UploadPartCopy_non_existing_versionId) ts.Run(Versioning_UploadPartCopy_from_an_object_version) // Object lock configuration @@ -2038,6 +2040,7 @@ func GetIntTests() IntTests { "Versioning_PutObject_overwrite_null_versionId_obj": Versioning_PutObject_overwrite_null_versionId_obj, "Versioning_PutObject_success": Versioning_PutObject_success, "Versioning_CopyObject_invalid_versionId": Versioning_CopyObject_invalid_versionId, + "Versioning_CopyObject_encoded_versionid_separator_invalid_versionId": Versioning_CopyObject_encoded_versionid_separator_invalid_versionId, "Versioning_CopyObject_success": Versioning_CopyObject_success, "Versioning_CopyObject_non_existing_version_id": Versioning_CopyObject_non_existing_version_id, "Versioning_CopyObject_from_an_object_version": Versioning_CopyObject_from_an_object_version, @@ -2087,6 +2090,7 @@ func GetIntTests() IntTests { "Versioning_Multipart_Upload_success": Versioning_Multipart_Upload_success, "Versioning_Multipart_Upload_overwrite_an_object": Versioning_Multipart_Upload_overwrite_an_object, "Versioning_UploadPartCopy_invalid_versionId": Versioning_UploadPartCopy_invalid_versionId, + "Versioning_UploadPartCopy_encoded_versionid_separator_invalid_versionId": Versioning_UploadPartCopy_encoded_versionid_separator_invalid_versionId, "Versioning_UploadPartCopy_non_existing_versionId": Versioning_UploadPartCopy_non_existing_versionId, "Versioning_UploadPartCopy_from_an_object_version": Versioning_UploadPartCopy_from_an_object_version, "Versioning_object_lock_not_enabled_on_bucket_creation": Versioning_object_lock_not_enabled_on_bucket_creation, diff --git a/tests/integration/versioning.go b/tests/integration/versioning.go index cdf13cf6..274d5376 100644 --- a/tests/integration/versioning.go +++ b/tests/integration/versioning.go @@ -244,6 +244,31 @@ func Versioning_CopyObject_invalid_versionId(s *S3Conf) error { }, withVersioning(types.BucketVersioningStatusEnabled)) } +func Versioning_CopyObject_encoded_versionid_separator_invalid_versionId(s *S3Conf) error { + testName := "Versioning_CopyObject_encoded_versionid_separator_invalid_versionId" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + dstObj, srcObj := "dst-obj", "src-obj" + + srcObjLen := int64(2345) + _, err := putObjectWithData(srcObjLen, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &srcObj, + }, s3client) + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: &dstObj, + CopySource: getPtr(fmt.Sprintf("%v/%v%%3FversionId%%3D..%%2f..%%2fsecret.txt", bucket, srcObj)), + }) + cancel() + return checkApiErr(err, s3err.GetInvalidArgumentErr(s3err.InvalidArgVersionId, "../../secret.txt")) + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + func Versioning_CopyObject_success(s *S3Conf) error { testName := "Versioning_CopyObject_success" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -1903,6 +1928,36 @@ func Versioning_UploadPartCopy_invalid_versionId(s *S3Conf) error { }) } +func Versioning_UploadPartCopy_encoded_versionid_separator_invalid_versionId(s *S3Conf) error { + testName := "Versioning_UploadPartCopy_encoded_versionid_separator_invalid_versionId" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + dstObj, srcObj := "dst-obj", "src-obj" + _, err := putObjectWithData(10, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &srcObj, + }, s3client) + if err != nil { + return err + } + + mp, err := createMp(s3client, bucket, dstObj) + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.UploadPartCopy(ctx, &s3.UploadPartCopyInput{ + Bucket: &bucket, + Key: &dstObj, + UploadId: mp.UploadId, + PartNumber: getPtr(int32(1)), + CopySource: getPtr(fmt.Sprintf("%v/%v%%3FversionId%%3D..%%2f..%%2fsecret.txt", bucket, srcObj)), + }) + cancel() + return checkApiErr(err, s3err.GetInvalidArgumentErr(s3err.InvalidArgVersionId, "../../secret.txt")) + }) +} + func Versioning_UploadPartCopy_non_existing_versionId(s *S3Conf) error { testName := "Versioning_UploadPartCopy_non_existing_versionId" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {