Describe the bug
aws s3 cp/aws s3 sync --recursive includes a protective check (_warn_parent_reference in awscli/customizations/s3/s3handler.py) meant to detect S3 object keys that would make the local destination path escape the target download directory (e.g. a key like ../../etc/foo), and to warn + skip the object instead of writing outside the destination.
The check only catches keys that normalize to '../something':
def _warn_parent_reference(self, fileinfo):
parent_prefix = '..' + os.path.sep
normalized = os.path.normpath('.' + os.path.sep + fileinfo.compare_key)
escapes_cwd = normalized.startswith(parent_prefix)
...
If a key normalizes to exactly '..' (no trailing component after it) — for example a legal, uploadable S3 key like prefix/.. — startswith('..' + os.sep) is False, so the guard does not fire and the object is not skipped/warned as intended.
Impact
For a recursive download of s3://bucket/prefix/ that contains an object literally named prefix/.., the computed local destination path becomes <destdir>/.. (the parent directory itself). Because the destination formatter always appends a trailing separator, this does not become an arbitrary-file overwrite in practice, but it does bypass the intended "File references a parent directory" warning/skip behavior, and the actual local write then fails with an unhandled OS error (e.g. IsADirectoryError), producing a confusing crash instead of the intended graceful warning for that one object during a recursive cp/sync.
Steps to reproduce
$ python3 -c "
import os
for k in ['..', '../foo.txt', 'a/../..']:
n = os.path.normpath('.' + os.sep + k)
print(k, '->', n, n.startswith('..' + os.sep))
"
.. -> .. False # should be treated as an escape, isn't
../foo.txt -> ../foo.txt True # correctly caught today
a/../.. -> .. False # should be treated as an escape, isn't
Any S3 key that normalizes to exactly .. (e.g. prefix/.., a/../..) slips past _warn_parent_reference and is handed to the downloader unguarded.
Suggested fix
Also match the exact '..' case:
escapes_cwd = normalized == '..' or normalized.startswith(parent_prefix)
I have a PR ready with this fix plus two new unit tests covering the previously-uncovered ..-only and a/../.. cases (existing coverage in tests/unit/customizations/s3/test_s3handler.py only exercised escapes that include a trailing filename component, e.g. ../foo.txt).
Environment
aws-cli develop branch (current)
- Verified against
awscli/customizations/s3/s3handler.py::_warn_parent_reference and its call sites in _get_warning_handlers
- Confirmed no existing unit test in
tests/unit/customizations/s3/test_s3handler.py exercises the bare .. case
Describe the bug
aws s3 cp/aws s3 sync --recursiveincludes a protective check (_warn_parent_referenceinawscli/customizations/s3/s3handler.py) meant to detect S3 object keys that would make the local destination path escape the target download directory (e.g. a key like../../etc/foo), and to warn + skip the object instead of writing outside the destination.The check only catches keys that normalize to
'../something':If a key normalizes to exactly
'..'(no trailing component after it) — for example a legal, uploadable S3 key likeprefix/..—startswith('..' + os.sep)isFalse, so the guard does not fire and the object is not skipped/warned as intended.Impact
For a recursive download of
s3://bucket/prefix/that contains an object literally namedprefix/.., the computed local destination path becomes<destdir>/..(the parent directory itself). Because the destination formatter always appends a trailing separator, this does not become an arbitrary-file overwrite in practice, but it does bypass the intended "File references a parent directory" warning/skip behavior, and the actual local write then fails with an unhandled OS error (e.g.IsADirectoryError), producing a confusing crash instead of the intended graceful warning for that one object during a recursivecp/sync.Steps to reproduce
Any S3 key that normalizes to exactly
..(e.g.prefix/..,a/../..) slips past_warn_parent_referenceand is handed to the downloader unguarded.Suggested fix
Also match the exact
'..'case:I have a PR ready with this fix plus two new unit tests covering the previously-uncovered
..-only anda/../..cases (existing coverage intests/unit/customizations/s3/test_s3handler.pyonly exercised escapes that include a trailing filename component, e.g.../foo.txt).Environment
aws-clidevelop branch (current)awscli/customizations/s3/s3handler.py::_warn_parent_referenceand its call sites in_get_warning_handlerstests/unit/customizations/s3/test_s3handler.pyexercises the bare..case