Skip to content

Commit

Permalink
fix: head/get/delete/copy directory object should fail when correspon…
Browse files Browse the repository at this point in the history 
…ding file object exists

The API hanlders and backend were stripping trailing "/" in object
paths. So if an object exists and a request came in for head/get/delete/copy
for that same name but with a trailing "/" indicating request should
be for directory object, the "/" would be stripped and the request
would be handlied for the incorrect file object.

This fix adds in checks to handle the case with the training "/"
in the request.

Fixes #709
  • Loading branch information
@benmcclelland
benmcclelland committed Aug 5, 2024
1 parent cb992a4 commit 797376a
Show file tree
Hide file tree
Showing 5 changed files with 182 additions and 4 deletions.
33 changes: 29 additions & 4 deletions backend/posix/posix.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1523,7 +1523,20 @@ func (p *Posix) DeleteObject(_ context.Context, input *s3.DeleteObjectInput) err
return fmt.Errorf("stat bucket: %w", err)
}

err = os.Remove(filepath.Join(bucket, object))
objpath := filepath.Join(bucket, object)

fi, err := os.Stat(objpath)
if errors.Is(err, fs.ErrNotExist) {
return s3err.GetAPIError(s3err.ErrNoSuchKey)
}
if err != nil {
return fmt.Errorf("stat object: %w", err)
}
if strings.HasSuffix(object, "/") && !fi.IsDir() {
return s3err.GetAPIError(s3err.ErrNoSuchKey)
}

err = os.Remove(objpath)
if errors.Is(err, fs.ErrNotExist) {
return s3err.GetAPIError(s3err.ErrNoSuchKey)
}
Expand Down Expand Up @@ -1629,6 +1642,7 @@ func (p *Posix) GetObject(_ context.Context, input *s3.GetObjectInput) (*s3.GetO

object := *input.Key
objPath := filepath.Join(bucket, object)

fi, err := os.Stat(objPath)
if errors.Is(err, fs.ErrNotExist) {
return nil, s3err.GetAPIError(s3err.ErrNoSuchKey)
Expand All @@ -1637,6 +1651,10 @@ func (p *Posix) GetObject(_ context.Context, input *s3.GetObjectInput) (*s3.GetO
return nil, fmt.Errorf("stat object: %w", err)
}

if strings.HasSuffix(object, "/") && !fi.IsDir() {
return nil, s3err.GetAPIError(s3err.ErrNoSuchKey)
}

acceptRange := *input.Range
startOffset, length, err := backend.ParseRange(fi.Size(), acceptRange)
if err != nil {
Expand Down Expand Up @@ -1801,13 +1819,17 @@ func (p *Posix) HeadObject(ctx context.Context, input *s3.HeadObjectInput) (*s3.
}

objPath := filepath.Join(bucket, object)

fi, err := os.Stat(objPath)
if errors.Is(err, fs.ErrNotExist) {
return nil, s3err.GetAPIError(s3err.ErrNoSuchKey)
}
if err != nil {
return nil, fmt.Errorf("stat object: %w", err)
}
if strings.HasSuffix(object, "/") && !fi.IsDir() {
return nil, s3err.GetAPIError(s3err.ErrNoSuchKey)
}

userMetaData := make(map[string]string)
contentType, contentEncoding := p.loadUserMetaData(bucket, object, userMetaData)
Expand Down Expand Up @@ -1977,10 +1999,13 @@ func (p *Posix) CopyObject(ctx context.Context, input *s3.CopyObjectInput) (*s3.
}
defer f.Close()

fInfo, err := f.Stat()
fi, err := f.Stat()
if err != nil {
return nil, fmt.Errorf("stat object: %w", err)
}
if strings.HasSuffix(srcObject, "/") && !fi.IsDir() {
return nil, s3err.GetAPIError(s3err.ErrNoSuchKey)
}

meta := make(map[string]string)
p.loadUserMetaData(srcBucket, srcObject, meta)
Expand All @@ -2006,7 +2031,7 @@ func (p *Posix) CopyObject(ctx context.Context, input *s3.CopyObjectInput) (*s3.
}
}

contentLength := fInfo.Size()
contentLength := fi.Size()

etag, err := p.PutObject(ctx,
&s3.PutObjectInput{
Expand All @@ -2020,7 +2045,7 @@ func (p *Posix) CopyObject(ctx context.Context, input *s3.CopyObjectInput) (*s3.
return nil, err
}

fi, err := os.Stat(dstObjdPath)
fi, err = os.Stat(dstObjdPath)
if err != nil {
return nil, fmt.Errorf("stat dst object: %w", err)
}
Expand Down
9 changes: 9 additions & 0 deletions backend/scoutfs/scoutfs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -487,13 +487,17 @@ func (s *ScoutFS) HeadObject(ctx context.Context, input *s3.HeadObjectInput) (*s
}

objPath := filepath.Join(bucket, object)

fi, err := os.Stat(objPath)
if errors.Is(err, fs.ErrNotExist) {
return nil, s3err.GetAPIError(s3err.ErrNoSuchKey)
}
if err != nil {
return nil, fmt.Errorf("stat object: %w", err)
}
if strings.HasSuffix(object, "/") && !fi.IsDir() {
return nil, s3err.GetAPIError(s3err.ErrNoSuchKey)
}

userMetaData := make(map[string]string)
contentType, contentEncoding := s.loadUserMetaData(bucket, object, userMetaData)
Expand Down Expand Up @@ -604,6 +608,7 @@ func (s *ScoutFS) GetObject(_ context.Context, input *s3.GetObjectInput) (*s3.Ge
}

objPath := filepath.Join(bucket, object)

fi, err := os.Stat(objPath)
if errors.Is(err, fs.ErrNotExist) {
return nil, s3err.GetAPIError(s3err.ErrNoSuchKey)
Expand All @@ -612,6 +617,10 @@ func (s *ScoutFS) GetObject(_ context.Context, input *s3.GetObjectInput) (*s3.Ge
return nil, fmt.Errorf("stat object: %w", err)
}

if strings.HasSuffix(object, "/") && !fi.IsDir() {
return nil, s3err.GetAPIError(s3err.ErrNoSuchKey)
}

startOffset, length, err := backend.ParseRange(fi.Size(), acceptRange)
if err != nil {
return nil, err
Expand Down
12 changes: 12 additions & 0 deletions s3api/controllers/base.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ func (c S3ApiController) GetActions(ctx *fiber.Ctx) error {
if keyEnd != "" {
key = strings.Join([]string{key, keyEnd}, "/")
}
path := ctx.Path()
if path[len(path)-1:] == "/" && key[len(key)-1:] != "/" {
key = key + "/"
}

if ctx.Request().URI().QueryArgs().Has("tagging") {
err := auth.VerifyAccess(ctx.Context(), c.be, auth.AccessOptions{
Expand Down Expand Up @@ -2379,6 +2383,10 @@ func (c S3ApiController) DeleteActions(ctx *fiber.Ctx) error {
if keyEnd != "" {
key = strings.Join([]string{key, keyEnd}, "/")
}
path := ctx.Path()
if path[len(path)-1:] == "/" && key[len(key)-1:] != "/" {
key = key + "/"
}

if ctx.Request().URI().QueryArgs().Has("tagging") {
err := auth.VerifyAccess(ctx.Context(), c.be,
Expand Down Expand Up @@ -2577,6 +2585,10 @@ func (c S3ApiController) HeadObject(ctx *fiber.Ctx) error {
if keyEnd != "" {
key = strings.Join([]string{key, keyEnd}, "/")
}
path := ctx.Path()
if path[len(path)-1:] == "/" && key[len(key)-1:] != "/" {
key = key + "/"
}

var partNumber *int32
if ctx.Request().URI().QueryArgs().Has("partNumber") {
Expand Down
8 changes: 8 additions & 0 deletions tests/integration/group-tests.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ func TestHeadObject(s *S3Conf) {
HeadObject_invalid_part_number(s)
HeadObject_non_existing_mp(s)
HeadObject_mp_success(s)
HeadObject_non_existing_dir_object(s)
HeadObject_success(s)
}

Expand All @@ -160,6 +161,7 @@ func TestGetObject(s *S3Conf) {
GetObject_success(s)
GetObject_by_range_success(s)
GetObject_by_range_resp_status(s)
GetObject_non_existing_dir_object(s)
}

func TestListObjects(s *S3Conf) {
Expand All @@ -182,6 +184,7 @@ func TestListObjectsV2(s *S3Conf) {

func TestDeleteObject(s *S3Conf) {
DeleteObject_non_existing_object(s)
DeleteObject_non_existing_dir_object(s)
DeleteObject_success(s)
DeleteObject_success_status_code(s)
}
Expand All @@ -198,6 +201,7 @@ func TestCopyObject(s *S3Conf) {
CopyObject_copy_to_itself(s)
CopyObject_to_itself_with_new_metadata(s)
CopyObject_CopySource_starting_with_slash(s)
CopyObject_non_existing_dir_object(s)
CopyObject_success(s)
}

Expand Down Expand Up @@ -576,6 +580,7 @@ func GetIntTests() IntTests {
"HeadObject_invalid_part_number": HeadObject_invalid_part_number,
"HeadObject_non_existing_mp": HeadObject_non_existing_mp,
"HeadObject_mp_success": HeadObject_mp_success,
"HeadObject_non_existing_dir_object": HeadObject_non_existing_dir_object,
"HeadObject_success": HeadObject_success,
"GetObjectAttributes_non_existing_bucket": GetObjectAttributes_non_existing_bucket,
"GetObjectAttributes_non_existing_object": GetObjectAttributes_non_existing_object,
Expand All @@ -588,6 +593,7 @@ func GetIntTests() IntTests {
"GetObject_success": GetObject_success,
"GetObject_by_range_success": GetObject_by_range_success,
"GetObject_by_range_resp_status": GetObject_by_range_resp_status,
"GetObject_non_existing_dir_object": GetObject_non_existing_dir_object,
"ListObjects_non_existing_bucket": ListObjects_non_existing_bucket,
"ListObjects_with_prefix": ListObjects_with_prefix,
"ListObject_truncated": ListObject_truncated,
Expand All @@ -601,6 +607,7 @@ func GetIntTests() IntTests {
"ListObjectsV2_start_after_not_in_list": ListObjectsV2_start_after_not_in_list,
"ListObjectsV2_start_after_empty_result": ListObjectsV2_start_after_empty_result,
"DeleteObject_non_existing_object": DeleteObject_non_existing_object,
"DeleteObject_non_existing_dir_object": DeleteObject_non_existing_dir_object,
"DeleteObject_success": DeleteObject_success,
"DeleteObject_success_status_code": DeleteObject_success_status_code,
"DeleteObjects_empty_input": DeleteObjects_empty_input,
Expand All @@ -611,6 +618,7 @@ func GetIntTests() IntTests {
"CopyObject_copy_to_itself": CopyObject_copy_to_itself,
"CopyObject_to_itself_with_new_metadata": CopyObject_to_itself_with_new_metadata,
"CopyObject_CopySource_starting_with_slash": CopyObject_CopySource_starting_with_slash,
"CopyObject_non_existing_dir_object": CopyObject_non_existing_dir_object,
"CopyObject_success": CopyObject_success,
"PutObjectTagging_non_existing_object": PutObjectTagging_non_existing_object,
"PutObjectTagging_long_tags": PutObjectTagging_long_tags,
Expand Down
124 changes: 124 additions & 0 deletions tests/integration/tests.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2954,6 +2954,39 @@ func HeadObject_mp_success(s *S3Conf) error {
})
}

func HeadObject_non_existing_dir_object(s *S3Conf) error {
testName := "HeadObject_non_existing_dir_object"
return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
obj, dataLen := "my-obj", int64(1234567)
meta := map[string]string{
"key1": "val1",
"key2": "val2",
}

_, _, err := putObjectWithData(dataLen, &s3.PutObjectInput{
Bucket: &bucket,
Key: &obj,
Metadata: meta,
}, s3client)
if err != nil {
return err
}

obj = "my-obj/"
ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
_, err = s3client.HeadObject(ctx, &s3.HeadObjectInput{
Bucket: &bucket,
Key: &obj,
})
defer cancel()
if err := checkSdkApiErr(err, "NotFound"); err != nil {
return err
}

return nil
})
}

const defaultContentType = "binary/octet-stream"

func HeadObject_success(s *S3Conf) error {
Expand Down Expand Up @@ -3514,6 +3547,33 @@ func GetObject_by_range_resp_status(s *S3Conf) error {
})
}

func GetObject_non_existing_dir_object(s *S3Conf) error {
testName := "GetObject_non_existing_dir_object"
return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
dataLength, obj := int64(1234567), "my-obj"

_, _, err := putObjectWithData(dataLength, &s3.PutObjectInput{
Bucket: &bucket,
Key: &obj,
}, s3client)
if err != nil {
return err
}

obj = "my-obj/"
ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
_, err = s3client.GetObject(ctx, &s3.GetObjectInput{
Bucket: &bucket,
Key: &obj,
})
defer cancel()
if err := checkSdkApiErr(err, "NoSuchKey"); err != nil {
return err
}
return nil
})
}

func ListObjects_non_existing_bucket(s *S3Conf) error {
testName := "ListObjects_non_existing_bucket"
return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
Expand Down Expand Up @@ -3901,6 +3961,30 @@ func DeleteObject_non_existing_object(s *S3Conf) error {
})
}

func DeleteObject_non_existing_dir_object(s *S3Conf) error {
testName := "DeleteObject_non_existing_dir_object"
return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
obj := "my-obj"
err := putObjects(s3client, []string{obj}, bucket)
if err != nil {
return err
}

obj = "my-obj/"
ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
_, err = s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
Bucket: &bucket,
Key: &obj,
})
cancel()
if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrNoSuchKey)); err != nil {
return err
}

return nil
})
}

func DeleteObject_success(s *S3Conf) error {
testName := "DeleteObject_success"
return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
Expand Down Expand Up @@ -4283,6 +4367,46 @@ func CopyObject_CopySource_starting_with_slash(s *S3Conf) error {
})
}

func CopyObject_non_existing_dir_object(s *S3Conf) error {
testName := "CopyObject_non_existing_dir_object"
return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
dataLength, obj := int64(1234567), "my-obj"
dstBucket := getBucketName()
err := setup(s, dstBucket)
if err != nil {
return err
}

_, _, err = putObjectWithData(dataLength, &s3.PutObjectInput{
Bucket: &bucket,
Key: &obj,
}, s3client)
if err != nil {
return err
}

obj = "my-obj/"

ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
_, err = s3client.CopyObject(ctx, &s3.CopyObjectInput{
Bucket: &dstBucket,
Key: &obj,
CopySource: getPtr(fmt.Sprintf("%v/%v", bucket, obj)),
})
cancel()
if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrNoSuchKey)); err != nil {
return err
}

err = teardown(s, dstBucket)
if err != nil {
return nil
}

return nil
})
}

func CopyObject_success(s *S3Conf) error {
testName := "CopyObject_success"
return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
Expand Down

0 comments on commit 797376a

Please sign in to comment.