s3 cp/sync --recursive: parent-directory-escape guard misses bare '..' key, causing unhandled crash
- Dominant language
- Python
- Stars
- 17.3k
- Forks
- 4.6k
- Avg merge
- 1d 2h
- Merged PRs (30d)
- 13
Description
## 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'`:
```python
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 `/..` (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:
```python
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
Contributor guide
Research direction
Read _warn_parent_reference and its call sites in awscli/customizations/s3/s3handler.py, then inspect the related unit tests in tests/unit/customizations/s3/test_s3handler.py. Run those tests and confirm that keys normalizing to exactly '..' are warned about and skipped during recursive cp/sync instead of reaching the downloader and crashing.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- aws, python
- Domain
- cli, cloud
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 35/100