[transfermanager] Allow downloading directories that contain filenames with double dots - #3396
Open
PolovinaD wants to merge 4 commits into
Open
[transfermanager] Allow downloading directories that contain filenames with double dots#3396PolovinaD wants to merge 4 commits into
PolovinaD wants to merge 4 commits into
Conversation
Author
|
Just following up on this PR. I've been using this change in production via the forked module and it has worked well so far. The workaround is manageable, but I'd like to simplify future SDK upgrades by returning to the upstream module. This also resolves an upload/download asymmetry where object keys containing Happy to make any changes or add more tests if needed. |
wty-Bryant
reviewed
Jun 1, 2026
| l.expectStart(t, in) | ||
| l.expectComplete(t, in, out, 2) | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
it's better to add other 2 cases combining path escaping :
"object key with double dots in name and path": {
destination: "double-dots-in-name-and-path",
objectsLists: [][]s3types.Object{
{
{
Key: aws.String("foo/bar/../baz..zoo"),
},
{
Key: aws.String("a..b"),
},
},
},
expectTokens: []string{""},
expectKeys: []string{"foo/bar/../baz..zoo", "a..b"},
expectFiles: []string{"foo/baz..zoo", "a..b"},
expectObjectsDownloaded: 2,
listenerValidationFn: func(t *testing.T, l *mockDirectoryListener, in, out any, err error) {
l.expectStart(t, in)
l.expectComplete(t, in, out, 2)
},
},
"object key with double dots in name and path escaping": {
destination: "double-dots-in-name-and-path-escaping",
objectsLists: [][]s3types.Object{
{
{
Key: aws.String("foo/../../baz..zoo"),
},
{
Key: aws.String("a..b"),
},
},
},
expectErr: "outside of destination",
listenerValidationFn: func(t *testing.T, l *mockDirectoryListener, in, out any, err error) {
// only validate failure listener since start listener
// might never be triggerred if the error response is returned first
l.expectFailed(t, in, err)
},
},
Author
There was a problem hiding this comment.
Thanks, that makes sense. I thought it was (partially) covered in error when path resolved from objects key out of destination scope above, but this is definitely more thorough. I updated it
…for filenames containing '..' when downloading a directory
wty-Bryant
force-pushed
the
fix/download-dir-double-dot-check
branch
from
June 4, 2026 19:27
305a122 to
0fec8c2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix overly strict path traversal check in DownloadDirectory.
Problem
The current implementation uses:
strings.Contains(relPath, "..")
This incorrectly rejects valid filenames such as:
Solution
Replace with:
!filepath.IsLocal(relPath)
This:
Test
Updated TestDownloadDirectory to verify files with '..' in their names are allowed.
All unit tests pass.