From 797376a2351562a3d850e9eebacb047e28453e2a Mon Sep 17 00:00:00 2001 From: Ben McClelland Date: Mon, 5 Aug 2024 11:03:41 -0700 Subject: [PATCH] fix: head/get/delete/copy directory object should fail when corresponding 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 --- backend/posix/posix.go | 33 +++++++- backend/scoutfs/scoutfs.go | 9 +++ s3api/controllers/base.go | 12 +++ tests/integration/group-tests.go | 8 ++ tests/integration/tests.go | 124 +++++++++++++++++++++++++++++++ 5 files changed, 182 insertions(+), 4 deletions(-) diff --git a/backend/posix/posix.go b/backend/posix/posix.go index c5cab07b..9decc8ef 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -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) } @@ -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) @@ -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 { @@ -1801,6 +1819,7 @@ 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) @@ -1808,6 +1827,9 @@ func (p *Posix) HeadObject(ctx context.Context, input *s3.HeadObjectInput) (*s3. 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) @@ -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) @@ -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{ @@ -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) } diff --git a/backend/scoutfs/scoutfs.go b/backend/scoutfs/scoutfs.go index 0c6255d0..5465b26f 100644 --- a/backend/scoutfs/scoutfs.go +++ b/backend/scoutfs/scoutfs.go @@ -487,6 +487,7 @@ 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) @@ -494,6 +495,9 @@ func (s *ScoutFS) HeadObject(ctx context.Context, input *s3.HeadObjectInput) (*s 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) @@ -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) @@ -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 diff --git a/s3api/controllers/base.go b/s3api/controllers/base.go index bae057d1..12efc74e 100644 --- a/s3api/controllers/base.go +++ b/s3api/controllers/base.go @@ -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{ @@ -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, @@ -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") { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index fcd38194..4321b7a1 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -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) } @@ -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) { @@ -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) } @@ -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) } @@ -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, @@ -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, @@ -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, @@ -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, diff --git a/tests/integration/tests.go b/tests/integration/tests.go index ccf1eff4..f5541cc8 100644 --- a/tests/integration/tests.go +++ b/tests/integration/tests.go @@ -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 { @@ -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 { @@ -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 { @@ -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 {