From e702a4860a7d5f4e010a12ceb6a96cbd841eb6ab Mon Sep 17 00:00:00 2001 From: Ben McClelland Date: Tue, 10 Feb 2026 12:04:35 -0800 Subject: [PATCH] fix: CopyObject with URL-encoded special chars CopyObject was failing with NoSuchKey when source keys contained special characters like {} or spaces. The X-Amz-Copy-Source header is URL-encoded by clients, but ParseCopySource wasn't decoding before filesystem access. Added url.QueryUnescape() to properly decode bucket and object names, fixing copy operations for keys with special characters. Fixing this also uncovered an errors with azure blob url encoding with similar special character handling. Added this fix in for the integration tests to pass. Fixes #1832 Fixes #1637 --- backend/azure/azure.go | 8 +- backend/common.go | 11 +++ backend/common_test.go | 150 +++++++++++++++++++++++++++++++ tests/integration/CopyObject.go | 90 +++++++++++++++++++ tests/integration/group-tests.go | 2 + 5 files changed, 259 insertions(+), 2 deletions(-) create mode 100644 backend/common_test.go diff --git a/backend/azure/azure.go b/backend/azure/azure.go index 7846c1f..25e2371 100644 --- a/backend/azure/azure.go +++ b/backend/azure/azure.go @@ -25,6 +25,7 @@ import ( "fmt" "io" "math" + "net/url" "os" "path/filepath" "sort" @@ -950,7 +951,7 @@ func (az *Azure) CopyObject(ctx context.Context, input s3response.CopyObjectInpu } } - if strings.Join([]string{*input.Bucket, *input.Key}, "/") == *input.CopySource { + if srcBucket == *input.Bucket && srcObj == *input.Key { if input.MetadataDirective != types.MetadataDirectiveReplace { return s3response.CopyObjectOutput{}, s3err.GetAPIError(s3err.ErrInvalidCopyDest) } @@ -1858,7 +1859,10 @@ func (az *Azure) getContainerURL(cntr string) string { } func (az *Azure) getBlobURL(cntr, blb string) string { - return fmt.Sprintf("%v/%v", az.getContainerURL(cntr), blb) + // URL-encode the blob name to handle special characters like {, }, #, spaces, etc. + // Use PathEscape to encode the blob name while preserving forward slashes + encodedBlob := url.PathEscape(blb) + return fmt.Sprintf("%v/%v", az.getContainerURL(cntr), encodedBlob) } func (az *Azure) getBlobClient(cntr, blb string) (*blob.Client, error) { diff --git a/backend/common.go b/backend/common.go index 44bd0d9..95846d9 100644 --- a/backend/common.go +++ b/backend/common.go @@ -247,6 +247,17 @@ func ParseCopySource(copySourceHeader string) (string, string, string, error) { return "", "", "", s3err.GetAPIError(s3err.ErrInvalidCopySourceBucket) } + var err error + // URL-decode the bucket and object names to handle special characters + srcBucket, err = url.QueryUnescape(srcBucket) + if err != nil { + return "", "", "", s3err.GetAPIError(s3err.ErrInvalidCopySourceEncoding) + } + srcObject, err = url.QueryUnescape(srcObject) + if err != nil { + return "", "", "", s3err.GetAPIError(s3err.ErrInvalidCopySourceEncoding) + } + return srcBucket, srcObject, versionId, nil } diff --git a/backend/common_test.go b/backend/common_test.go new file mode 100644 index 0000000..c448103 --- /dev/null +++ b/backend/common_test.go @@ -0,0 +1,150 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package backend + +import ( + "errors" + "testing" + + "github.com/versity/versitygw/s3err" +) + +func TestParseCopySource(t *testing.T) { + tests := []struct { + name string + copySourceHeader string + wantBucket string + wantObject string + wantVersionId string + wantErr bool + wantErrCode s3err.ErrorCode + }{ + { + name: "simple path", + copySourceHeader: "mybucket/myobject", + wantBucket: "mybucket", + wantObject: "myobject", + wantVersionId: "", + wantErr: false, + }, + { + name: "path with leading slash", + copySourceHeader: "/mybucket/myobject", + wantBucket: "mybucket", + wantObject: "myobject", + wantVersionId: "", + wantErr: false, + }, + { + name: "path with versionId", + copySourceHeader: "mybucket/myobject?versionId=abc123", + wantBucket: "mybucket", + wantObject: "myobject", + wantVersionId: "abc123", + wantErr: false, + }, + { + name: "URL-encoded curly braces", + copySourceHeader: "mybucket/myfolder/%7Be14c392b-09ad-4188-85f4-b779af00fb88%7D/testfile", + wantBucket: "mybucket", + wantObject: "myfolder/{e14c392b-09ad-4188-85f4-b779af00fb88}/testfile", + wantVersionId: "", + wantErr: false, + }, + { + name: "URL-encoded space", + copySourceHeader: "mybucket/my%20object", + wantBucket: "mybucket", + wantObject: "my object", + wantVersionId: "", + wantErr: false, + }, + { + name: "URL-encoded special chars", + copySourceHeader: "mybucket/obj%23%24%25%26", + wantBucket: "mybucket", + wantObject: "obj#$%&", + wantVersionId: "", + wantErr: false, + }, + { + name: "URL-encoded path with versionId", + copySourceHeader: "mybucket/my%20folder/my%20object?versionId=xyz789", + wantBucket: "mybucket", + wantObject: "my folder/my object", + wantVersionId: "xyz789", + wantErr: false, + }, + { + name: "invalid URL encoding - incomplete escape", + copySourceHeader: "mybucket/object%", + wantBucket: "", + wantObject: "", + wantVersionId: "", + wantErr: true, + wantErrCode: s3err.ErrInvalidCopySourceEncoding, + }, + { + name: "invalid URL encoding - invalid hex", + copySourceHeader: "mybucket/object%ZZ", + wantBucket: "", + wantObject: "", + wantVersionId: "", + wantErr: true, + wantErrCode: s3err.ErrInvalidCopySourceEncoding, + }, + { + name: "missing object", + copySourceHeader: "mybucket", + wantBucket: "", + wantObject: "", + wantVersionId: "", + wantErr: true, + wantErrCode: s3err.ErrInvalidCopySourceBucket, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotBucket, gotObject, gotVersionId, err := ParseCopySource(tt.copySourceHeader) + + if tt.wantErr { + if err == nil { + t.Errorf("ParseCopySource() error = nil, wantErr %v", tt.wantErr) + return + } + if !errors.Is(err, s3err.GetAPIError(tt.wantErrCode)) { + t.Errorf("ParseCopySource() error = %v, want error code %v", err, tt.wantErrCode) + } + return + } + + if err != nil { + t.Errorf("ParseCopySource() unexpected error = %v", err) + return + } + + if gotBucket != tt.wantBucket { + t.Errorf("ParseCopySource() gotBucket = %v, want %v", gotBucket, tt.wantBucket) + } + if gotObject != tt.wantObject { + t.Errorf("ParseCopySource() gotObject = %v, want %v", gotObject, tt.wantObject) + } + if gotVersionId != tt.wantVersionId { + t.Errorf("ParseCopySource() gotVersionId = %v, want %v", gotVersionId, tt.wantVersionId) + } + }) + } +} diff --git a/tests/integration/CopyObject.go b/tests/integration/CopyObject.go index ca764a4..9952708 100644 --- a/tests/integration/CopyObject.go +++ b/tests/integration/CopyObject.go @@ -1498,6 +1498,96 @@ func CopyObject_success(s *S3Conf) error { }) } +func CopyObject_with_special_characters(s *S3Conf) error { + testName := "CopyObject_with_special_characters" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + // Test copying objects with special characters that need URL encoding + // like curly braces, spaces, and other reserved characters + testCases := []struct { + name string + srcKey string + dstKey string + }{ + { + name: "curly braces", + srcKey: "myfolder/{e14c392b-09ad-4188-85f4-b779af00fb88}/testfile", + dstKey: "myfolder/{e14c392b-09ad-4188-85f4-b779af00fb88}/copy", + }, + { + name: "multiple special chars", + srcKey: "folder/{id}/file #1", + dstKey: "folder/{id}/file #1 copy", + }, + { + name: "ampersand and hash", + srcKey: "file&with#special", + dstKey: "file&with#special-copy", + }, + { + name: "spaces and brackets", + srcKey: "my file [2024].txt", + dstKey: "my file [2024] copy.txt", + }, + } + + for _, tc := range testCases { + // Create source object + dataLength := int64(100) + r, err := putObjectWithData(dataLength, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &tc.srcKey, + }, s3client) + if err != nil { + return fmt.Errorf("%s: failed to put source object: %w", tc.name, err) + } + + // Copy the object + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: &tc.dstKey, + CopySource: getPtr(fmt.Sprintf("%v/%v", bucket, tc.srcKey)), + }) + cancel() + if err != nil { + return fmt.Errorf("%s: failed to copy object: %w", tc.name, err) + } + + // Verify the copied object + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: &bucket, + Key: &tc.dstKey, + }) + cancel() + if err != nil { + return fmt.Errorf("%s: failed to get copied object: %w", tc.name, err) + } + + if out.ContentLength == nil { + return fmt.Errorf("%s: expected content-length to be set, instead got nil", tc.name) + } + if *out.ContentLength != dataLength { + return fmt.Errorf("%s: expected content-length %v, instead got %v", + tc.name, dataLength, *out.ContentLength) + } + + bdy, err := io.ReadAll(out.Body) + if err != nil { + return fmt.Errorf("%s: failed to read body: %w", tc.name, err) + } + out.Body.Close() + + outCsum := sha256.Sum256(bdy) + if outCsum != r.csum { + return fmt.Errorf("%s: invalid object data, checksum mismatch", tc.name) + } + } + + return nil + }) +} + func CopyObject_object_acl_not_supported(s *S3Conf) error { testName := "CopyObject_object_acl_not_supported" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 942671c..5e9dd12 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -347,6 +347,7 @@ func TestCopyObject(ts *TestState) { // azure doesn't support these metadata characters ts.Run(CopyObject_with_metadata) } + ts.Run(CopyObject_with_special_characters) ts.Run(CopyObject_success) } @@ -1404,6 +1405,7 @@ func GetIntTests() IntTests { "CopyObject_should_copy_the_existing_checksum": CopyObject_should_copy_the_existing_checksum, "CopyObject_should_replace_the_existing_checksum": CopyObject_should_replace_the_existing_checksum, "CopyObject_to_itself_by_replacing_the_checksum": CopyObject_to_itself_by_replacing_the_checksum, + "CopyObject_with_special_characters": CopyObject_with_special_characters, "CopyObject_success": CopyObject_success, "PutObjectTagging_non_existing_object": PutObjectTagging_non_existing_object, "PutObjectTagging_long_tags": PutObjectTagging_long_tags,