From fe3cfbfce910049719524c47b0cd087de48d249f Mon Sep 17 00:00:00 2001 From: alexgalie Date: Mon, 11 May 2026 19:31:02 +0200 Subject: [PATCH] fix: forward slash url encoded used as bucket/key separator handle %2F ("/") url encoded separator for bucket/key Fixes #2024 --------- Co-authored-by: Galie Alexandru MTANA --- auth/access-control.go | 12 +++- auth/access-control_test.go | 129 ++++++++++++++++++++++++++++++++++++ backend/common.go | 29 ++++---- backend/common_test.go | 32 +++++++++ 4 files changed, 185 insertions(+), 17 deletions(-) create mode 100644 auth/access-control_test.go diff --git a/auth/access-control.go b/auth/access-control.go index e4fc53c8..7747f9d7 100644 --- a/auth/access-control.go +++ b/auth/access-control.go @@ -18,6 +18,7 @@ import ( "context" "encoding/json" "errors" + "net/url" "strings" "github.com/aws/aws-sdk-go-v2/service/s3" @@ -37,8 +38,15 @@ func VerifyObjectCopyAccess(ctx context.Context, be backend.Backend, copySource if err := VerifyAccess(ctx, be, opts); err != nil { return err } - // Verify source bucket access - srcBucket, srcObject, found := strings.Cut(copySource, "/") + // Verify source bucket access. + // URL-decode the copy source before splitting so that clients which send + // the bucket/key separator as "%2F" are handled correctly. + // Callers are expected to have already stripped any leading '/'. + decodedSrc, err := url.QueryUnescape(copySource) + if err != nil { + return s3err.GetAPIError(s3err.ErrInvalidCopySourceEncoding) + } + srcBucket, srcObject, found := strings.Cut(decodedSrc, "/") if !found { return s3err.GetAPIError(s3err.ErrInvalidCopySourceBucket) } diff --git a/auth/access-control_test.go b/auth/access-control_test.go new file mode 100644 index 00000000..67b1837b --- /dev/null +++ b/auth/access-control_test.go @@ -0,0 +1,129 @@ +// 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 auth + +import ( + "context" + "encoding/json" + "testing" + + "github.com/aws/aws-sdk-go-v2/service/s3" + "github.com/aws/aws-sdk-go-v2/service/s3/types" + "github.com/stretchr/testify/assert" + "github.com/versity/versitygw/backend" + "github.com/versity/versitygw/s3err" +) + +// noBucketPolicyBackend is a test stub that returns ErrNoSuchBucketPolicy for +// GetBucketPolicy and serves a configurable ACL for GetBucketAcl. +type noBucketPolicyBackend struct { + backend.BackendUnsupported + srcAcl ACL +} + +func (b noBucketPolicyBackend) GetBucketPolicy(_ context.Context, _ string) ([]byte, error) { + return nil, s3err.GetAPIError(s3err.ErrNoSuchBucketPolicy) +} + +func (b noBucketPolicyBackend) GetBucketAcl(_ context.Context, _ *s3.GetBucketAclInput) ([]byte, error) { + return json.Marshal(b.srcAcl) +} + +func TestVerifyObjectCopyAccess_URLEncodedSlashSeparator(t *testing.T) { + const testUser = "testuser" + + // Source bucket ACL: grants READ to testUser. + srcAcl := ACL{ + Owner: "owner", + Grantees: []Grantee{ + { + Access: testUser, + Permission: PermissionRead, + Type: types.TypeCanonicalUser, + }, + }, + } + + be := noBucketPolicyBackend{srcAcl: srcAcl} + + // Destination bucket ACL: testUser is the owner (DisableACL=true path). + opts := AccessOptions{ + Acl: ACL{Owner: testUser}, + AclPermission: PermissionWrite, + IsRoot: false, + Acc: Account{Access: testUser, Role: RoleUser}, + Bucket: "dst-bucket", + Object: "dst-key", + Actions: []Action{PutObjectAction}, + DisableACL: true, + } + + tests := []struct { + name string + copySource string + }{ + { + name: "percent-encoded slash (%2F) as bucket/key separator", + copySource: "my-namespace-test-container%2Ftest-blob", + }, + { + name: "%2F separator with encoded chars in key", + copySource: "src-bucket%2Fmy%20folder%2Fmy-key", + }, + { + name: "%2F separator with versionId", + copySource: "src-bucket%2Fsrc-key?versionId=abc123", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := VerifyObjectCopyAccess(context.Background(), be, tt.copySource, opts) + assert.NoError(t, err, + "should accept %%2F as the bucket/key separator in x-amz-copy-source") + }) + } +} + +func TestVerifyObjectCopyAccess_LiteralSlashSeparator(t *testing.T) { + const testUser = "testuser" + + srcAcl := ACL{ + Owner: "owner", + Grantees: []Grantee{ + { + Access: testUser, + Permission: PermissionRead, + Type: types.TypeCanonicalUser, + }, + }, + } + + be := noBucketPolicyBackend{srcAcl: srcAcl} + + opts := AccessOptions{ + Acl: ACL{Owner: testUser}, + AclPermission: PermissionWrite, + IsRoot: false, + Acc: Account{Access: testUser, Role: RoleUser}, + Bucket: "dst-bucket", + Object: "dst-key", + Actions: []Action{PutObjectAction}, + DisableACL: true, + } + + err := VerifyObjectCopyAccess(context.Background(), be, "src-bucket/src-key", opts) + assert.NoError(t, err, "literal slash separator should work") +} diff --git a/backend/common.go b/backend/common.go index 14fbc89f..9560678b 100644 --- a/backend/common.go +++ b/backend/common.go @@ -232,29 +232,28 @@ func ParseCopySource(copySourceHeader string) (string, string, string, error) { copySourceHeader = copySourceHeader[1:] } - var copySource, versionId string + // 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 { - copySource = copySourceHeader + rawSource = copySourceHeader } else { - copySource = copySourceHeader[:i] + rawSource = copySourceHeader[:i] versionId = copySourceHeader[i+11:] } - srcBucket, srcObject, ok := strings.Cut(copySource, "/") - if !ok { - return "", "", "", s3err.GetAPIError(s3err.ErrInvalidCopySourceBucket) + // 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) + if err != nil { + return "", "", "", s3err.GetAPIError(s3err.ErrInvalidCopySourceEncoding) } - 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) + srcBucket, srcObject, ok := strings.Cut(decoded, "/") + if !ok { + return "", "", "", s3err.GetAPIError(s3err.ErrInvalidCopySourceBucket) } return srcBucket, srcObject, versionId, nil diff --git a/backend/common_test.go b/backend/common_test.go index c4481038..d6065fd6 100644 --- a/backend/common_test.go +++ b/backend/common_test.go @@ -87,6 +87,38 @@ func TestParseCopySource(t *testing.T) { wantVersionId: "xyz789", wantErr: false, }, + { + name: "percent-encoded slash as bucket/key separator", + copySourceHeader: "my-namespace-test-container%2Ftest-blob", + wantBucket: "my-namespace-test-container", + wantObject: "test-blob", + wantVersionId: "", + wantErr: false, + }, + { + name: "percent-encoded slash separator with leading slash", + copySourceHeader: "/my-namespace-test-container%2Ftest-blob", + wantBucket: "my-namespace-test-container", + wantObject: "test-blob", + wantVersionId: "", + wantErr: false, + }, + { + name: "percent-encoded slash separator with versionId", + copySourceHeader: "my-bucket%2Fmy-object?versionId=abc123", + wantBucket: "my-bucket", + wantObject: "my-object", + wantVersionId: "abc123", + wantErr: false, + }, + { + name: "percent-encoded slash separator with encoded object key", + copySourceHeader: "my-bucket%2Fmy%20folder%2Fmy%20object", + wantBucket: "my-bucket", + wantObject: "my folder/my object", + wantVersionId: "", + wantErr: false, + }, { name: "invalid URL encoding - incomplete escape", copySourceHeader: "mybucket/object%",