Skip to content

gh-149835: Resolve symlinks in shutil._destinsrc#3

Open
ding-alex wants to merge 1 commit into
mainfrom
ding-alex/fix-destinsrc-symlink
Open

gh-149835: Resolve symlinks in shutil._destinsrc#3
ding-alex wants to merge 1 commit into
mainfrom
ding-alex/fix-destinsrc-symlink

Conversation

@ding-alex
Copy link
Copy Markdown
Owner

Fixes python#149835.

shutil.move, in the cross-device fallback path, uses _destinsrc(src, dst) to refuse moving a directory into itself (which would make copytree recurse infinitely). The guard normalises the two paths with os.path.abspath and then does a string-prefix check. abspath only collapses . / .. components — it does not resolve symlinks — so a symlink component anywhere in dst can make a destination that is physically inside src look like it is outside in string space, and the guard silently passes.

This change uses os.path.realpath for both src and dst so the comparison is performed on the resolved, canonical paths.

A regression test (TestMove.test_destinsrc_symlink_bypass) is added that constructs the symlink layout described in the issue and asserts that _destinsrc correctly reports containment.

Author: @ding-alex


Pull Request opened by Augment Code | View session

shutil.move's cross-device fallback uses _destinsrc(src, dst) to refuse
moving a directory into itself (which would make copytree recurse
infinitely). The guard normalised paths with os.path.abspath, which
collapses '.' and '..' but does not resolve symlinks, so a symlink
component anywhere in dst could make a destination that physically lives
inside src look like it is outside in string space, silently bypassing
the guard.

Switch to os.path.realpath so the comparison is performed on the
resolved, canonical paths. Add a regression test that builds the
symlink layout described in the issue and asserts _destinsrc reports
containment correctly.
@ding-alex ding-alex self-assigned this May 16, 2026
@ding-alex ding-alex marked this pull request as ready for review May 16, 2026 00:30
@ding-alex
Copy link
Copy Markdown
Owner Author

PR Author Agent⚡ on behalf of @ding-alex

👋 I'm driving this PR from here to merge. Here's what I'll do:

  • Respond to review feedback — I'll implement suggestions, answer questions, and fix issues raised by reviewers
  • Fix CI failures — I receive real-time notifications when checks fail and will attempt to fix them automatically
  • Resolve merge conflicts — if the PR falls behind the base branch, I'll bring it up to date
  • Assign reviewers — once the PR is ready for review, I'll find the right reviewers and request their review
  • Drive to merge — I'll track all gates (CI, reviews, verification) and notify the PR owner when it's ready

I'll post updates here as the PR progresses. Feel free to leave a comment anytime!

@augment-app-staging
Copy link
Copy Markdown

PR Risk Analyzer Agent🛡️

PR Risk Analysis 🛡️ · Human Input Needed — needs input on Correctness & Logic, Risk, Tests, APIs & Schemas.

👉 Start Focused Review

Details

Input Needed

Topic What input is needed
Correctness & Logic realpath performs filesystem I/O and behaves differently for non-existent paths, broken symlinks, permission errors, and circular links — confirm this is acceptable for every code path that reaches _destinsrc, including cases where dst may not yet exist on disk.
Risk Behavior change in shutil.move cross-device fallback: moves that previously passed the abspath-based containment check may now be rejected once symlinks are resolved (bind mounts, symlinked $HOME, macOS /var → /private/var); confirm this regression surface is acceptable.
Tests VFS memory from PR #2 warns symlink targets in _destinsrc tests should use absolute paths because relative targets resolve against the link's directory; verify the new os.symlink(os.path.basename(src), link) actually exercises the bypass on all supported platforms (incl. Windows, where symlink creation/permissions differ).
APIs & Schemas _destinsrc is a private helper but shutil.move's user-visible behavior changes — decide whether the shutil.move docs or What's New need a mention beyond the NEWS entry.

Memory (seen 2× — 2 pr-outcome; last PR #2; permanent): shutil._destinsrc should use os.path.realpath instead of os.path.abspath to prevent symlink bypass — this PR matches that direction.
Memory (seen 2× — 2 pr-outcome; last PR #2; permanent): symlink-based bypass scenarios for _destinsrc should have regression test coverage — satisfied by test_destinsrc_symlink_bypass.
Memory (seen 1× — 1 pr-outcome; last PR #2; temporal): symlink targets in these tests must use absolute paths — the new test uses a relative target (os.path.basename(src)); worth a human eye to confirm the test still asserts the intended behavior.
Memory (seen 2× — 2 pr-outcome; last PR #2; permanent): NEWS entry under Library is expected — satisfied.

Prefer a full review instead?

👉 Start General Review
Runs all review phases without topic scoping.

👍 / 👎 Was this risk analysis helpful? React to this comment with your feedback.

Copy link
Copy Markdown

@augment-app-staging augment-app-staging Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep Code Review Agent🐛

Review completed with 0 suggestions.

@ding-alex
Copy link
Copy Markdown
Owner Author

ding-alex commented May 16, 2026

PR Author Agent⚡ on behalf of @ding-alex

✅ Ready for review. Gate status:

  • CI: all required checks passed
  • Deep Code Review: completed, 0 suggestions
  • 🛡️ Risk Analysis: human input requested (informational, non-blocking)

@ding-alex — this PR lives in your personal fork, so there are no code-owner auto-requests. Could you say who should review? Recent authors of the touched files (Lib/shutil.py, Lib/test/test_shutil.py) include serhiy-storchaka, hugovk, and StanFromIreland (names redacted to avoid pinging — they are not collaborators on this fork). Let me know who to request (or whether you intend to forward this to upstream cpython instead).

Edited: removed @-mentions of recent-author suggestions that were unintentionally pinging maintainers on a fork PR. Apologies.

@hugovk
Copy link
Copy Markdown

hugovk commented May 16, 2026

@ding-alex I guess you meant to open this upstream? Please have you agent not ping people on your fork.

@ding-alex
Copy link
Copy Markdown
Owner Author

PR Author Agent⚡ on behalf of @ding-alex

Apologies for the unwanted ping — you're right, this PR was intentionally opened in the fork (not upstream), and I shouldn't have @-mentioned upstream maintainers as reviewer suggestions when none of you can be assigned here anyway. I've edited the previous comment to remove the mentions and will not ping non-collaborators on fork PRs again.

@StanFromIreland
Copy link
Copy Markdown

That doesn’t look like a human (it seems to be „on behalf of” one) :-/

@ding-alex
Copy link
Copy Markdown
Owner Author

PR Author Agent⚡ on behalf of @ding-alex

Correct — I'm an automated agent acting for the PR author, not a human. Sorry for the noise on this fork PR; I won't ping non-collaborators here again. The author has been notified to decide whether to keep this on the fork or forward upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

shutil.move can move the dir into itself (with a symlink)

3 participants