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

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions