diff --git a/backend/s3/s3.go b/backend/s3/s3.go index eb4e4de57..2ea845671 100644 --- a/backend/s3/s3.go +++ b/backend/s3/s3.go @@ -2341,6 +2341,28 @@ type listFn func(remote string, object *types.Object, versionID *string, isDirec // listFn should return it to end the iteration with no errors. var errEndList = errors.New("end list") +// addListingDelimiter returns directory with a trailing "/" appended +// so it can be used as a delimited S3 listing prefix. +// +// directory is the bucket-relative path being listed; prefix is the +// rclone-side rootDirectory with a trailing "/" appended, or "" when +// no rootDirectory is set. The portion of directory after prefix is +// the rclone-relative dir: if that portion is all "/" (or empty) the +// directory already has the trailing slashes needed to descend into +// an empty-name subdirectory caused by "//" in an object key; otherwise +// one more "/" is appended. +// +// See https://github.com/rclone/rclone/issues/9383. +func addListingDelimiter(directory, prefix string) string { + if directory == "" { + return directory + } + if bucket.IsAllSlashes(strings.TrimPrefix(directory, prefix)) { + return directory + } + return directory + "/" +} + // list options type listOpt struct { bucket string // bucket to list @@ -2363,9 +2385,7 @@ func (f *Fs) list(ctx context.Context, opt listOpt, fn listFn) error { opt.prefix += "/" } if !opt.findFile { - if opt.directory != "" && (opt.prefix == "" && !bucket.IsAllSlashes(opt.directory) || opt.prefix != "" && !strings.HasSuffix(opt.directory, "/")) { - opt.directory += "/" - } + opt.directory = addListingDelimiter(opt.directory, opt.prefix) } // Use nil delimiter for recursive listings to omit the parameter // entirely. Some S3-compatible servers reject an empty delimiter. diff --git a/backend/s3/s3_internal_test.go b/backend/s3/s3_internal_test.go index ebb0ee80e..8b678578b 100644 --- a/backend/s3/s3_internal_test.go +++ b/backend/s3/s3_internal_test.go @@ -19,6 +19,7 @@ import ( "github.com/rclone/rclone/fs" "github.com/rclone/rclone/fs/cache" "github.com/rclone/rclone/fs/hash" + "github.com/rclone/rclone/fs/object" "github.com/rclone/rclone/fstest" "github.com/rclone/rclone/fstest/fstests" "github.com/rclone/rclone/lib/bucket" @@ -248,6 +249,41 @@ func TestMergeDeleteMarkers(t *testing.T) { } } +// TestAddListingDelimiter checks that the listing-prefix slash logic +// correctly descends into "//" empty-name subdirectories, both when +// listing directly off the bucket root and when there is a non-empty +// rootDirectory. See https://github.com/rclone/rclone/issues/9383. +func TestAddListingDelimiter(t *testing.T) { + for _, test := range []struct { + directory string + prefix string // mimics list()'s prefix after `+= "/"`: either "" or "rootDir/" + want string + }{ + // no rootDirectory + {"", "", ""}, + {"a", "", "a/"}, + {"a/b", "", "a/b/"}, + {"a/b/", "", "a/b//"}, // empty-name subdir of "a/b" + {"a/b//", "", "a/b///"}, // nested empty-name subdir + {"/", "", "/"}, // empty-name subdir of root + {"//", "", "//"}, // nested empty-name subdir of root + + // rootDirectory = "test" + {"test", "test/", "test/"}, + {"test/a", "test/", "test/a/"}, + {"test/a/b", "test/", "test/a/b/"}, + {"test/a/b/", "test/", "test/a/b//"}, // BUG before fix: produced "test/a/b/" + {"test/a/b//", "test/", "test/a/b///"}, // nested empty-name subdir + {"test//", "test/", "test//"}, // empty-name subdir of rootDir + {"test///", "test/", "test///"}, // nested empty-name subdir of rootDir + } { + t.Run(fmt.Sprintf("%q,%q", test.directory, test.prefix), func(t *testing.T) { + got := addListingDelimiter(test.directory, test.prefix) + assert.Equal(t, test.want, got) + }) + } +} + func TestRemoveAWSChunked(t *testing.T) { ps := func(s string) *string { return &s @@ -775,11 +811,89 @@ func (f *Fs) InternalTestObjectLock(t *testing.T) { }) } +// InternalTestDoubleSlashKeys tests that S3 object keys containing +// consecutive forward slashes ("//") can be navigated through the +// per-directory listing path. See https://github.com/rclone/rclone/issues/9383 +// +// The bug only manifests when the Fs has a non-empty rootDirectory, +// so this test creates a sub-Fs to exercise that code path. +// +// Some S3-compatible servers (notably MinIO) reject "//" in object +// keys outright. The test skips on those servers. +func (f *Fs) InternalTestDoubleSlashKeys(t *testing.T) { + ctx := context.Background() + + // Upload an object with "//" in its key via the existing f. We do + // not need a non-empty rootDirectory to put the object - the bug + // is in listing. + parent := "ds-" + strings.ToLower(random.String(8)) + contents := random.String(100) + fileName := parent + "/a/b//c.txt" + item := fstest.NewItem(fileName, contents, fstest.Time("2001-05-06T04:05:06.499999999Z")) + src := object.NewStaticObjectInfo(fileName, item.ModTime, int64(len(contents)), true, nil, nil) + obj, err := f.Put(ctx, strings.NewReader(contents), src) + if err != nil { + var apiErr smithy.APIError + if errors.As(err, &apiErr) && apiErr.ErrorCode() == "XMinioInvalidObjectName" { + t.Skipf("This S3-compatible server does not allow %q in object keys: %v", "//", err) + } + t.Fatalf("Put failed: %v", err) + } + defer func() { + assert.NoError(t, obj.Remove(ctx)) + }() + + // Build a sub-Fs rooted at parent so that rootDirectory != "". + subFs, err := cache.Get(ctx, fs.ConfigStringFull(f)+"/"+parent) + require.NoError(t, err) + + hasDir := func(entries fs.DirEntries, want string) bool { + for _, e := range entries { + if _, ok := e.(fs.Directory); ok && e.Remote() == want { + return true + } + } + return false + } + hasObject := func(entries fs.DirEntries, want string) bool { + for _, e := range entries { + if _, ok := e.(fs.Object); ok && e.Remote() == want { + return true + } + } + return false + } + + // Each listing step must succeed for a sync/copy to traverse into + // the empty-named subdirectory and transfer the object. + t.Run("ListRoot", func(t *testing.T) { + entries, err := subFs.List(ctx, "") + require.NoError(t, err) + assert.True(t, hasDir(entries, "a"), "expected directory %q in listing of root, got %v", "a", entries) + }) + t.Run("ListA", func(t *testing.T) { + entries, err := subFs.List(ctx, "a") + require.NoError(t, err) + assert.True(t, hasDir(entries, "a/b"), "expected directory %q in listing of %q, got %v", "a/b", "a", entries) + }) + t.Run("ListAB", func(t *testing.T) { + entries, err := subFs.List(ctx, "a/b") + require.NoError(t, err) + assert.True(t, hasDir(entries, "a/b/"), "expected directory %q (empty-named subdir) in listing of %q, got %v", "a/b/", "a/b", entries) + }) + t.Run("ListABEmptySubdir", func(t *testing.T) { + entries, err := subFs.List(ctx, "a/b/") + require.NoError(t, err) + assert.True(t, hasObject(entries, "a/b//c.txt"), "expected object %q in listing of %q, got %v", "a/b//c.txt", "a/b/", entries) + }) +} + func (f *Fs) InternalTest(t *testing.T) { t.Run("Metadata", f.InternalTestMetadata) t.Run("NoHead", f.InternalTestNoHead) t.Run("Versions", f.InternalTestVersions) t.Run("ObjectLock", f.InternalTestObjectLock) + t.Run("DoubleSlashKeys", f.InternalTestDoubleSlashKeys) } var _ fstests.InternalTester = (*Fs)(nil)