Refuse a path where a command name belongs - #69
Merged
Conversation
install_completions.py takes a command NAME, but the output path is `directory / pattern.format(bin=binary)` and pathlib discards the left operand when the right side is absolute. Passing a path -- which the flag `--from-path` invites -- therefore wrote the completion script over the binary it was asked to read. Found in rusticprofile, where it destroyed a working binary on a host taking hourly backups. This copy was identical apart from line endings. `--from-path` runs [binary], so an absolute path works for the READ and only breaks the write: generation succeeds, then destroys its own input, exit 0. Now refused before any file is written. Watched failing: neutering the guard fails --self-test and fails just standard-check, which just check depends on. etr's own recipes pass bare names and were never at risk. Also repairs this file's worktree copy, which was CRLF while the index was LF (stale from before .gitattributes). Template v3. Assisted-By: Claude Opus 5
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
1 participant
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.
Tooling only; no runtime change. 145 tests, unchanged.
scripts/install_completions.py→ template v3.Found in
rusticprofile, where this helper destroyed a working binary on a host taking hourly backups — a 3.6 MB executable replaced by a 21 KB bash completion script. etr's copy was identical apart from line endings and carried the same defect.The mechanism
binariesare command names, and the output path isdirectory / pattern.format(bin=binary).Path("/dest") / "/abs/path"discards the left operand, so an absolute argument relocates every write out of the completion directory and onto the path itself — which under--from-pathis the installed binary.It fails in the worst available order:
--from-pathruns[binary], so an absolute path works for the read and only breaks the write. Generation succeeds, then destroys its own input. Exit 0, nothing printed.--from-path, which invites exactly the argument that breaks it — so documenting it would not have prevented it.The fix
reject_path_like()refuses any argument containing a path separator or resolving absolute, before any file is written, and names the correct form in the error.Watched failing in both places it is enforced — neutering the condition fails
--self-test("rejects an absolute path — expected True, got False") and failsjust standard-check, whichjust checkdepends on. Re-running the original accident against a stand-in file now leaves it byte-identical instead of clobbered.A fourth self-test case pins the property — joining a directory with an absolute string yields the absolute string — so the check survives a rewrite of the guard.
Scope
etr's own recipes were never at risk:
install,install-tagandstandard-checkpass{{BINS}}, bare names. Checked, not assumed. The exposure is anyone invoking the helper directly, which is how it happened.Incidentally repaired: this file's worktree copy was CRLF while the index was LF (
git ls-files --eol→i/lf w/crlf), stale from before.gitattributeslanded — which is why this is a 50-line diff rather than a whole-file rewrite.Test plan
just prgreen — fmt + clippy--all-targets, 145 tests, man pages buildjust standard-checkpasses; fails when the guard is neuteredDevelopment.mdupdated and pushed before this PR (§4.11)