diff --git a/weed/s3api/copy_source_decode_test.go b/weed/s3api/copy_source_decode_test.go index df081a80b..5d7713e74 100644 --- a/weed/s3api/copy_source_decode_test.go +++ b/weed/s3api/copy_source_decode_test.go @@ -137,6 +137,46 @@ func TestCopySourceDecodingPlusSign(t *testing.T) { } } +// TestCopySourceRejectsTraversal verifies `.`/`..` segments in a copy source +// are rejected so the source stays within the parsed bucket. +func TestCopySourceRejectsTraversal(t *testing.T) { + tests := []struct { + name string + rawCopySource string + wantErr bool + }{ + {"clean source passes", "bucket-a/dir/key", false}, + {"leading slash source passes", "/bucket-a/dir/key", false}, + {"source with versionId passes", "bucket-a/dir/key?versionId=v1", false}, + + {"dotdot escapes bucket", "bucket-a/../bucket-secret/flag", true}, + {"leading slash dotdot escapes bucket", "/bucket-a/../bucket-secret/flag", true}, + {"percent-encoded dotdot escapes bucket", "bucket-a/%2e%2e/bucket-secret/flag", true}, + {"encoded slash dotdot escapes bucket", "bucket-a/..%2fbucket-secret/flag", true}, + {"backslash dotdot escapes bucket", "bucket-a/..\\bucket-secret/flag", true}, + {"nested dotdot escapes bucket", "bucket-a/dir/../../bucket-secret/flag", true}, + {"bare dot segment rejected", "bucket-a/./key", true}, + {"dotdot bucket rejected", "../bucket-secret/flag", true}, + {"dotdot with versionId escapes bucket", "bucket-a/../bucket-secret/flag?versionId=v1", true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cpSrcPath, err := url.PathUnescape(tc.rawCopySource) + if err != nil { + cpSrcPath = tc.rawCopySource + } + srcBucket, srcObject, _ := pathToBucketObjectAndVersion(tc.rawCopySource, cpSrcPath) + + gotErr := ValidateCopySource(cpSrcPath, srcBucket, srcObject) != nil + if gotErr != tc.wantErr { + t.Errorf("ValidateCopySource(%q) error = %v, want %v (parsed bucket=%q object=%q)", + tc.rawCopySource, gotErr, tc.wantErr, srcBucket, srcObject) + } + }) + } +} + // TestCopySourceRoutingWithSpecialChars tests that mux variable extraction // correctly handles special characters like ! (%21) in both the URL path // and the X-Amz-Copy-Source header. diff --git a/weed/s3api/s3api_copy_validation.go b/weed/s3api/s3api_copy_validation.go index deb292a2a..15b456629 100644 --- a/weed/s3api/s3api_copy_validation.go +++ b/weed/s3api/s3api_copy_validation.go @@ -265,6 +265,15 @@ func ValidateCopySource(copySource string, srcBucket, srcObject string) error { } } + // `.`/`..` segments are collapsed by the filer's path join; reject them as + // IsValidObjectKey does for the request URL so the source stays in-bucket. + if !s3_constants.IsValidBucketName(srcBucket) || !s3_constants.IsValidObjectKey(srcObject) { + return &CopyValidationError{ + Code: s3err.ErrInvalidCopySource, + Message: "Copy source contains invalid path segments", + } + } + return nil } diff --git a/weed/s3api/s3api_object_handlers_copy.go b/weed/s3api/s3api_object_handlers_copy.go index 610f00d0b..8b87d9779 100644 --- a/weed/s3api/s3api_object_handlers_copy.go +++ b/weed/s3api/s3api_object_handlers_copy.go @@ -764,12 +764,10 @@ func (s3a *S3ApiServer) CopyObjectPartHandler(w http.ResponseWriter, r *http.Req } } - // If source object is empty or bucket is empty, reply back invalid copy source. - // Note: srcObject can be "/" for root-level objects, but empty string means parsing failed - if srcObject == "" || srcBucket == "" { - glog.Errorf("CopyObjectPart: Invalid copy source - srcBucket=%q, srcObject=%q (original header: %q)", - srcBucket, srcObject, r.Header.Get("X-Amz-Copy-Source")) - s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource) + // Validate the copy source as CopyObject does. + if err := ValidateCopySource(cpSrcPath, srcBucket, srcObject); err != nil { + glog.V(2).Infof("CopyObjectPartHandler validation error: %v", err) + s3err.WriteErrorResponse(w, r, MapCopyValidationError(err)) return }