Repository navigation
The editor pass reads back the command line it was opened for word for word, and the shim's temporary files are private to their owner - #339
Merged
Conversation
…r word The shim hands the command line an editor was opened for to the editor pass through UPHOLD_SHIM_EDITOR_ARGV. It packed the words with a bare space and unpacked them by splitting on whitespace, on the stated ground that the words only picked checkers. They do more than that: the editor pass collects them again, and a target flag among them decides which repository the forge is asked about. A title of `ok -R acme/private` therefore came back as four words, the `-R` named a target the real command never had, the forge answered "private" for it, and every `public-target` rule stood down. The body typed into the editor, bound for the public repository origin names, was read by nobody and published with exit 0. The words are now packed with shell_words::join and split back with shell_words::split, so they return as the words they went in as, including a word that opens with `#`. A value that does not split back is refused with exit 2 before the editor opens, rather than read as an empty command line, which is the checkers of no command line at all. The comment that called quoting unnecessary is corrected, and the reference page now says what the editor pass does with the words. Claude-Session: https://claude.ai/code/session_01TG5hbR4Re6gcEZTpBCBymc
When the shim reads a body off its own stdin, it hands the bytes to the real command through a file in the temporary directory: created with create_new, unlinked as soon as it is open, and inherited as a descriptor. create_new kept the shim from adopting a file somebody else had planted, but the file it created got the default mode, 0666 less the umask, which on most machines is world-readable. Between the open and the unlink the file has a name in a directory everyone can list, so any local user who opened it in that window could read the body of a publishing command. On unix the file is now created at 0600, so that window hands the body to nobody else. The comment beside the open says what create_new protects against and what the mode adds, and a unit test asks the descriptor for its mode. Claude-Session: https://claude.ai/code/session_01TG5hbR4Re6gcEZTpBCBymc
…s a path somebody reached first The configurations handed to zizmor and gitleaks, and the manifests handed to guarddog, are written to a temporary file for the one command that reads them. The name was the process id and a counter, which anyone on the machine can work out, and the file was filled with fs::write, which opens whatever already sits at the path. A link planted there first was followed and its target truncated and overwritten, and the file was left at whatever mode the umask gave, usually world-readable. The file is now created with create_new, which refuses any path that exists, a dangling link included, and on unix at 0600. The name carries the clock's nanoseconds as well, which makes a collision rarer; a taken name is a reason to draw another, up to sixteen times, and never to open it. Every name taken is refused with exit 2 and a pointer to TMPDIR. A file whose write fails is still removed on the way out. The doc comment that describes the guard sat above GITLEAKS_VERSION, so rustdoc attached it to that constant; it now sits on the module. Unit tests plant a file, a link to a file and a dangling link on the first names and check that none of them is touched, that the free name is used at 0600 and removed with its guard, and that a directory with every name taken is refused. Claude-Session: https://claude.ai/code/session_01TG5hbR4Re6gcEZTpBCBymc
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe editor handoff now preserves argument boundaries and rejects malformed saved arguments. Temporary-file creation now uses exclusive creation, retries occupied names, and sets Unix permissions to 0600. ChangesEditor Handoff
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No identified issue needs to be fixed before merge; normal checks remain appropriate. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (99.34%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #339 +/- ##
==========================================
+ Coverage 94.32% 94.34% +0.02%
==========================================
Files 46 46
Lines 22820 22953 +133
==========================================
+ Hits 21524 21656 +132
- Misses 1296 1297 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 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.
The command line an editor was opened for crossed into the editor pass through
UPHOLD_SHIM_EDITOR_ARGV, packed with bare spaces and unpacked by splitting on whitespace. The editor pass collects those words again and resolves the target from them, so a title likeok -R acme/privatecame back as four words, the-Ramong them named a private repository the real command never targeted, thepublic-targetcheck stood down, and a body bound for a public repository was published unchecked. The argv is now packed withshell_words::joinand read back withshell_words::split; a value that cannot be split is refused with exit 2, and it is read before the editor opens.The stdin a shim replays is held in a temporary file that is now created 0600 on unix, so only its owner can read it.
The supply-chain temporary files are created with
create_newat 0600, under a name carrying a nanosecond stamp. A name that is already taken is passed over and another drawn, a bounded number of times, and a path that already exists (a planted file, a link, a dangling link) is never opened. The doc comment that had drifted onto the gitleaks pin moved back onto thetempfile_guardmodule.Tests added:
src/shim.rs:the_stdin_a_shim_ate_is_readable_by_its_owner_alone,the_command_line_an_editor_was_opened_for_comes_back_word_for_word,a_command_line_that_cannot_be_split_back_is_refused_not_guessedsrc/supply.rs:a_temporary_file_never_opens_a_path_somebody_reached_first,a_temporary_directory_with_every_name_taken_is_refused,a_temporary_file_holds_what_it_was_given_and_goes_with_its_guardtests/shim_handoff_cli.rs:a_title_that_spells_a_flag_does_not_redirect_the_editor_pass,an_editor_argv_that_cannot_be_split_is_refusedhttps://claude.ai/code/session_01TG5hbR4Re6gcEZTpBCBymc
Summary by CodeRabbit