fix(cli): reject and never write through symlinked cds get destinations - #509
Merged
Conversation
_find_conflicts() previously used Path.exists(), which follows symlinks and returns False for a dangling link. A pre-planted symlink at a destination path (e.g. profiles/foo/profile.yaml -> ~/.ssh/config) would therefore silently bypass the conflict check even without --force, and shutil.copy2()/write_text() would then write through the link, clobbering an arbitrary file outside the intended destination tree. - cli/getter.py: _find_conflicts() now treats any symlink destination (dangling or not) as a conflict. A new _write_actions() helper unlinks any symlink at a destination before shutil.copy2() so --force can never write through a symlink either; _write_tracking_manifest() applies the same guard to the .cds/get-manifest.json path. - tests/test_getter.py: added regression tests confirming a symlinked destination is rejected without --force, is replaced (not written through) with --force, and that the tracking manifest path gets the same treatment. Skipped on Windows, where creating symlinks requires elevated privileges; still exercised by the Ubuntu/macOS CI runners. Resolves #474 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
_add_copy_action() previously called .resolve() on the combined destination path (destination_root / repo_relative), which follows a symlink planted at the destination leaf and silently swaps the CopyAction's destination for the symlink's resolved target instead of the symlink path itself. That defeated the is_symlink() conflict/ write-through guards added for #474: the CI (ubuntu-latest) run of the new regression tests caught this, since Windows locally could not create the test symlinks to exercise it. destination_root is already an absolute, resolved path by the time it reaches _build_copy_plan()/_add_copy_action() (see fetch_profile()), so joining without a further .resolve() is sufficient and keeps the destination path intact for the symlink checks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SemTiOne
approved these changes
Aug 25, 2026
SemTiOne
left a comment
SemTiOne
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. 2 non-blocking issue below. Also resolve the conflict.
Collaborator
|
There's a conflict. |
Collaborator
|
Oh conflict again. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cds getcopied fetched files withshutil.copy2and wrote the tracking manifest withwrite_text, both of which follow a symlink at the destination path._find_conflictsuseddestination.exists(), which follows symlinks and returnsFalsefor a dangling symlink. A pre-planted symlink at a destination path (e.g.profiles/foo/profile.yaml -> ~/.ssh/config) therefore silently bypassed the conflict check even without--force, and the subsequentcopy2wrote through the link, clobbering an arbitrary file outside the intended destination tree. With--force, any symlink destination was overwritten through the link the same way. Shared working directories (CI workspaces, mounted home dirs) are the realistic attack surface.Fix
_find_conflictsnow treatsdestination.is_symlink()as a conflict unconditionally (dangling or not), so it's always reported and blocks the operation without--force._write_actions()helper unlinks any symlink found at a destination path before callingshutil.copy2, so even with--forcethe destination is always replaced with a regular file rather than written through._write_tracking_manifestgets the same treatment for.cds/get-manifest.json.Testing
tests/test_getter.py:--force, leaving the symlink and its target untouched;--force, the symlinked destination is replaced with a regular file (not written through), leaving the former target untouched;python -m unittest tests.test_getter -v— 17 pass, 4 skipped (symlink tests skip on Windows; permission test also pre-existing Windows skip)python -m ruff check cli/getter.py tests/test_getter.py— cleanpython -m unittest discover -s tests -p "test_*.py"— only the pre-existing 9 baseline Windows/docker-less failures (unrelated)Resolves #474