Skip to content

s3 cp/sync --recursive: parent-directory-escape guard misses bare '..' key, causing unhandled crash #10579

Description

@Adityaj0

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-triageThis issue or PR still needs to be triaged.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions