Skip to content

Commit

Permalink
fix: handle "x-amz-copy-source" header starting with '/' in s3api
Browse files Browse the repository at this point in the history
The "x-amz-copy-source" header may start with '/' as observed with
WinSCP. However, '/' is also the separator between the bucket and the
object path in "x-amz-copy-source".

Consider the following code in VerifyObjectCopyAccess():

    srcBucket, srcObject, found := strings.Cut(copySource, "/")

If `copySource` starts with '/', then `srcBucket` is set to an empty
string. Later, an error is returned because bucket "" does not exist.

This issue was fixed in the Posix and Azure backends by the following
commit:

 * 5e484f2 fix: Fixed CopySource parsing to handle the values starting with '/' in CopyObject action in posix and azure backends.

But the issue was not fixed in `VerifyObjectCopyAccess`.

This commit sanitizes "x-amz-copy-source" right after the header is
extracted in `s3api/controllers/base.go`. This ensures that the
`CopySource` argument passed to the backend functions UploadPartCopy()
and CopyObject() does not start with '/'. Since the backends no longer
need to strip away any leading '/' in `CopySource`, the parts of
commit 5e484f2 modifying the Posix and Azure backends are reverted.

Fixes issue #773.

Signed-off-by: Christophe Vu-Brugier <christophe.vu-brugier@seagate.com>
  • Loading branch information
cvubrugier committed Aug 29, 2024
1 parent 26ef99c commit 20940f0
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 14 deletions.
9 changes: 2 additions & 7 deletions backend/azure/azure.go
Original file line number Diff line number Diff line change
Expand Up @@ -692,12 +692,7 @@ func (az *Azure) CopyObject(ctx context.Context, input *s3.CopyObjectInput) (*s3
return nil, err
}

cpSrc := *input.CopySource
if cpSrc[0] == '/' {
cpSrc = cpSrc[1:]
}

if strings.Join([]string{*input.Bucket, *input.Key}, "/") == cpSrc && isMetaSame(mdmap, input.Metadata) {
if strings.Join([]string{*input.Bucket, *input.Key}, "/") == *input.CopySource && isMetaSame(mdmap, input.Metadata) {
return nil, s3err.GetAPIError(s3err.ErrInvalidCopyDest)
}

Expand All @@ -711,7 +706,7 @@ func (az *Azure) CopyObject(ctx context.Context, input *s3.CopyObjectInput) (*s3
return nil, err
}

resp, err := bclient.CopyFromURL(ctx, az.serviceURL+"/"+cpSrc, &blob.CopyFromURLOptions{
resp, err := bclient.CopyFromURL(ctx, az.serviceURL+"/"+*input.CopySource, &blob.CopyFromURLOptions{
BlobTags: tags,
Metadata: parseMetadata(input.Metadata),
})
Expand Down
8 changes: 1 addition & 7 deletions backend/posix/posix.go
Original file line number Diff line number Diff line change
Expand Up @@ -1983,13 +1983,7 @@ func (p *Posix) CopyObject(ctx context.Context, input *s3.CopyObjectInput) (*s3.
if input.ExpectedBucketOwner == nil {
return nil, s3err.GetAPIError(s3err.ErrInvalidRequest)
}

cpSrc := *input.CopySource
if cpSrc[0] == '/' {
cpSrc = cpSrc[1:]
}

srcBucket, srcObject, ok := strings.Cut(cpSrc, "/")
srcBucket, srcObject, ok := strings.Cut(*input.CopySource, "/")
if !ok {
return nil, s3err.GetAPIError(s3err.ErrInvalidCopySource)
}
Expand Down
3 changes: 3 additions & 0 deletions s3api/controllers/base.go
Original file line number Diff line number Diff line change
Expand Up @@ -1535,6 +1535,9 @@ func (c S3ApiController) PutActions(ctx *fiber.Ctx) error {

// Copy source headers
copySource := ctx.Get("X-Amz-Copy-Source")
if len(copySource) > 0 && copySource[0] == '/' {
copySource = copySource[1:]
}
copySrcIfMatch := ctx.Get("X-Amz-Copy-Source-If-Match")
copySrcIfNoneMatch := ctx.Get("X-Amz-Copy-Source-If-None-Match")
copySrcModifSince := ctx.Get("X-Amz-Copy-Source-If-Modified-Since")
Expand Down

0 comments on commit 20940f0

Please sign in to comment.