From f37863f90f1bc1a0a2404677ecdcc56a62eb7b4f Mon Sep 17 00:00:00 2001 From: HackingGate Date: Sat, 10 Oct 2026 00:45:56 +0900 Subject: [PATCH 1/3] The editor pass reads back the command line it was opened for word for 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 --- docs/REFERENCE.md | 10 ++-- src/shim.rs | 105 ++++++++++++++++++++++++++++++++---- tests/shim_handoff_cli.rs | 109 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 212 insertions(+), 12 deletions(-) diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 157345f..7de2916 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -2386,9 +2386,13 @@ to ` shim --as-editor `, and `--as-editor` is what tells the re-entered process that it is the editor. Two environment variables carry the data it then needs, both set by the shim on the command it execs: `UPHOLD_SHIM_EDITOR_REAL` (the user's actual editor command line) and -`UPHOLD_SHIM_EDITOR_ARGV` (the original argv words, read only to decide which -`command.before` rules apply). Neither routes anything, so a process that -inherits them does nothing with them. +`UPHOLD_SHIM_EDITOR_ARGV` (the original argv words, quoted as a shell would +quote them). The editor pass splits the words back exactly and reads them the +way the argv pass did: they decide which `command.before` rules apply, and a +target flag among them decides which repository the forge is asked about. A +value that does not split back is refused with exit 2 before the editor opens, +rather than read as no words at all. Neither variable routes anything, so a +process that inherits them does nothing with them. Routing an editor re-entry through the environment would be a defect: an environment is inherited by every descendant, and the diff --git a/src/shim.rs b/src/shim.rs index b099071..69bbe03 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -3794,6 +3794,30 @@ fn nonempty_env(name: &str) -> Option { .filter(|value| !value.trim().is_empty()) } +/// The command line an editor was opened for, as the words `install_editor` +/// packed. +/// +/// The inverse of the `shell_words::join` there, and refused rather than +/// guessed at when it is not one: these words are collected again, a target +/// flag among them picks the repository the forge is asked about, and a split +/// that disagrees with the packing is a destination nobody gave. An empty list +/// is no better a guess -- it is the checkers of no command line at all. An +/// absent variable is a different case and keeps the reading it always had: +/// nothing was packed, so there is nothing a split could have misread. +fn unpacked_argv(name: &str, packed: Option<&str>) -> Result> { + let Some(packed) = packed else { + return Ok(Vec::new()); + }; + shell_words::split(packed).map_err(|_| { + Fatal::new(format!( + "{name}: the command line this editor was opened for ({packed:?}, in \ + {EDITOR_ARGV}) could not be split back into its words, so which checkers stand \ + in front of it and where the body is going could not be established. Nothing was \ + published" + )) + }) +} + /// One word for a shell that is going to split what it is handed. /// /// The editor variable holds a command LINE rather than a path -- `code --wait` @@ -3874,10 +3898,16 @@ fn install_editor( .or_else(|| nonempty_env("EDITOR")) .unwrap_or_else(|| String::from("vi")); command.env(EDITOR_REAL, editor); - // Only the words that decide WHICH checkers stand in front of this command - // line. They are matched as a subsequence and never re-executed, so joining - // them is enough and quoting them would be pretending otherwise. - command.env(EDITOR_ARGV, argv.join(" ")); + // The command line the editor is being opened for, quoted so that it comes + // back as the words it went in as. The editor pass does more with them than + // pick checkers: it collects them again, and a target flag among them + // decides which repository the forge is asked about. Joined with bare + // spaces, a title of `ok -R acme/private` came back as four words naming a + // target the real command never had, the forge answered for that one, and + // a body bound somewhere public stood every `public-target` rule down. The + // words are never executed; the quoting is there so `unpacked_argv` is the + // exact inverse of this. + command.env(EDITOR_ARGV, shell_words::join(argv)); // These two are data the editor pass reads, and neither one routes anything: // a `git` that inherits them is a `git` that does nothing with them. What // says "you are the editor" is the flag below, which only the process the @@ -4005,6 +4035,10 @@ fn edit_and_check(root: &Path, policy: &Policy, name: &str, argv: &[String]) -> "{name}: re-entered as an editor with no file to edit" ))); }; + // Read before the editor opens rather than after it closes: a command line + // that cannot be read back is a checkpoint that already knows it cannot + // judge what is about to be typed into it. + let opened_for = unpacked_argv(name, nonempty_env(EDITOR_ARGV).as_deref())?; let editor = nonempty_env(EDITOR_REAL).unwrap_or_else(|| String::from("vi")); // Through a shell, exactly the way the command would have run it, and with // the marker removed: the child here is the user's own editor, and a second @@ -4036,11 +4070,6 @@ fn edit_and_check(root: &Path, policy: &Policy, name: &str, argv: &[String]) -> return Ok(Exit::Clean); } - let opened_for: Vec = nonempty_env(EDITOR_ARGV) - .unwrap_or_default() - .split_whitespace() - .map(str::to_owned) - .collect(); let subject = Subject { kind: "text", value: text, @@ -6342,6 +6371,64 @@ mod tests { assert!(environment.contains_key(EDITOR_REAL)); } + #[test] + fn the_command_line_an_editor_was_opened_for_comes_back_word_for_word() { + // Packed with bare spaces and split on whitespace, a title that spelled + // a flag came back as that flag, and the editor pass asked the forge + // about a repository the real command never named. Every word here + // carries something a whitespace split or a careless quote loses. + let words: Vec = [ + "pr", + "create", + "--title", + "ok -R acme/private", + "--body", + r#"it's "quoted" and \ back-slashed"#, + "", + " padded ", + "tab\there", + "line\nbreak", + "$HOME `id` *", + // An unquoted `#` opening a word starts a comment for the split, + // which would drop this word and every one after it. + "# a heading", + "trailing", + ] + .into_iter() + .map(String::from) + .collect(); + let mut command = Command::new("true"); + install_editor( + &mut command, + "faux", + "FAUX_EDITOR", + Some(Path::new("/opt/uphold")), + &words, + EditorPath::Opens, + ) + .unwrap(); + let packed = command + .get_envs() + .find(|(name, _)| *name == EDITOR_ARGV) + .and_then(|(_, value)| value) + .map(|value| value.to_string_lossy().into_owned()); + assert_eq!(unpacked_argv("faux", packed.as_deref()).unwrap(), words); + // Nothing packed is nothing misread. + assert_eq!(unpacked_argv("faux", None).unwrap(), Vec::::new()); + } + + #[test] + fn a_command_line_that_cannot_be_split_back_is_refused_not_guessed() { + // An unclosed quote is not a command line `install_editor` packed, and + // the empty list the old split fell back to is the checkers of no + // command line at all. + let error = unpacked_argv("faux", Some("pr create --title 'unclosed")) + .unwrap_err() + .to_string(); + assert!(error.contains("could not be split"), "{error}"); + assert!(error.contains("Nothing was published"), "{error}"); + } + /// `-c alias.cm= cm `: the alias written on the command line, /// which is read off argv and asks no process. fn aliased(value: &str, rest: &[&str]) -> Vec { diff --git a/tests/shim_handoff_cli.rs b/tests/shim_handoff_cli.rs index ab41577..91132c0 100644 --- a/tests/shim_handoff_cli.rs +++ b/tests/shim_handoff_cli.rs @@ -1163,6 +1163,115 @@ scope = "public-target" ); } +/// A forge that knows one private repository and calls everything else +/// public, recording what it was asked. +const ONE_PRIVATE_REPOSITORY: &str = "#!/bin/sh\nif [ \"$1\" = api ]; then\n echo \"$*\" >> \"$GH_CALLS\"\n case \"$*\" in\n *acme/private*) echo private ;;\n *) echo public ;;\n esac\n exit 0\nfi\necho \"gh ran: $*\"\n"; + +#[test] +fn a_title_that_spells_a_flag_does_not_redirect_the_editor_pass() { + // The command line the editor was opened for crosses into the editor pass + // through the environment, and it was packed with spaces and unpacked by + // splitting on them. A title of `ok -R acme/private` came back as four + // words, the `-R` among them named a target the real command never had, + // the forge called that one private, and the `public-target` rule stood + // down -- so a body bound for a PUBLIC repository was read by nobody and + // published with exit 0. The words come back as the words they were. + let policy = r#" +[rule.marker-on-a-public-target] +message = "remove the marker" +regexp = "Claude Code" +files.exclude = ["policy/**"] +command.before = ["faux"] + +[[shim]] +command = "faux" +match = ["pr:create"] +title_flags = ["-t", "--title"] +text_flags = ["-b", "--body"] +target_flags = ["-R", "--repo"] +target = "git-remote" +editor_env = "FAUX_EDITOR" +scope = "public-target" +"#; + let root = workspace( + policy, + &[ + ("faux", EDITING_COMMAND), + ("gh", ONE_PRIVATE_REPOSITORY), + ( + "dirty-editor", + "#!/bin/sh\nprintf 'Generated with Claude Code\\n' > \"$1\"\n", + ), + ], + ); + origin(&root, "https://github.com/acme/widget.git"); + let calls = root.join("gh-calls.log"); + let editor = root.join("bin/dirty-editor"); + let output = Run { + args: &["faux", "pr", "create", "--title", "ok -R acme/private"], + envs: &[ + ("GH_CALLS", &calls.to_string_lossy()), + ("EDITOR", &editor.to_string_lossy()), + ], + ..Run::default() + } + .go(&root); + + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!( + !stdout(&output).contains("faux published:"), + "a body bound for a public repository was published unchecked: {}", + stdout(&output) + ); + assert!( + stderr(&output).contains("marker-on-a-public-target"), + "{}", + stderr(&output) + ); + let asked = std::fs::read_to_string(&calls).unwrap_or_default(); + assert!( + !asked.contains("acme/private"), + "the forge was asked about a repository only the title named: {asked:?}" + ); +} + +#[test] +fn an_editor_argv_that_cannot_be_split_is_refused() { + // Anything else would be a guess at which command line the body was + // written for, and an empty guess consults the checkers of no command + // line at all. Refused before the editor opens, so nobody types a body + // into a checkpoint that already knows it cannot judge it. + let root = workspace( + EDITOR_MARKER_POLICY, + &[ + ("faux", EDITING_COMMAND), + ( + "ordinary-editor", + "#!/bin/sh\n: > \"$PWD/editor-opened\"\nprintf 'An ordinary body\\n' > \"$1\"\n", + ), + ], + ); + let editor = root.join("bin/ordinary-editor"); + let output = as_editor( + &root, + &editor, + &[( + "UPHOLD_SHIM_EDITOR_ARGV", + "faux pr create --title 'unclosed", + )], + ); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + assert!( + stderr(&output).contains("could not be split"), + "{}", + stderr(&output) + ); + assert!( + !root.join("editor-opened").exists(), + "the editor opened for a command line that could not be read" + ); +} + #[test] fn a_forge_that_could_not_answer_is_refused_before_the_command_runs() { // Unauthenticated, rate-limited, offline, or a host no resolver knows. The From df76630a6bb61bf73c613935b7f38bd91e2e3f22 Mon Sep 17 00:00:00 2001 From: HackingGate Date: Sat, 10 Oct 2026 00:47:05 +0900 Subject: [PATCH 2/3] The stdin a shim replays is held in a file only its owner can read 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 --- src/shim.rs | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/src/shim.rs b/src/shim.rs index 69bbe03..c88676f 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -3836,19 +3836,29 @@ fn shell_word(word: &str) -> String { /// already, seeks back to the start, and needs nobody alive. It is unlinked the /// moment it is open, so what the child inherits is the descriptor and the disk /// keeps nothing, however the process ends. +/// +/// Between the open and the unlink the file has a name, and anyone on the +/// machine can open a name in a shared temporary directory. On unix it is +/// created readable by its owner alone, so that window hands the body to +/// nobody else; elsewhere the platform's default applies. fn replayed(bytes: &[u8]) -> Result { let stamp = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .map_or(0, |since| since.as_nanos()); let path = std::env::temp_dir().join(format!("uphold-shim-stdin-{}-{stamp}", std::process::id())); - let mut file = std::fs::OpenOptions::new() - // Never adopt a file somebody else left on this path: a shared - // temporary directory is writable by everyone on the machine, and the - // body of a publishing command is exactly the thing not to hand over. - .create_new(true) - .read(true) - .write(true) + let mut options = std::fs::OpenOptions::new(); + // Never adopt a file somebody else left on this path: a shared temporary + // directory is writable by everyone on the machine, and a file planted + // here first -- or a link pointing somewhere else -- would be where the + // body of a publishing command was written. + options.create_new(true).read(true).write(true); + // And never create one somebody else can read. The default is 0666 less + // the umask, which on most machines is world-readable, and the open above + // leaves a window before the unlink in which the name can be opened. + #[cfg(unix)] + std::os::unix::fs::OpenOptionsExt::mode(&mut options, 0o600); + let mut file = options .open(&path) .map_err(|error| Fatal::at(&path, error))?; drop(std::fs::remove_file(&path)); @@ -5238,6 +5248,18 @@ mod tests { assert_eq!(read_back, body); } + #[cfg(unix)] + #[test] + fn the_stdin_a_shim_ate_is_readable_by_its_owner_alone() { + // The file has a name in a shared temporary directory until the unlink, + // and created at the default mode it was world-readable for that long. + // Asked of the descriptor, which is what outlives the name. + use std::os::unix::fs::PermissionsExt as _; + let file = replayed(b"a body nobody else should read\n").unwrap(); + let mode = file.metadata().unwrap().permissions().mode(); + assert_eq!(mode & 0o777, 0o600, "mode {mode:o}"); + } + #[test] fn a_private_field_that_is_false_does_not_make_a_package_private() { assert!(json_bool_field(r#"{"private": true}"#, "private")); From 2623ebb281443848bedf85389c0d4c2b277486a9 Mon Sep 17 00:00:00 2001 From: HackingGate Date: Sat, 10 Oct 2026 00:48:13 +0900 Subject: [PATCH 3/3] A supply-chain temporary file is created new, at 0600, and never opens 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 --- src/supply.rs | 161 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 148 insertions(+), 13 deletions(-) diff --git a/src/supply.rs b/src/supply.rs index 075599a..48679a1 100644 --- a/src/supply.rs +++ b/src/supply.rs @@ -1720,11 +1720,6 @@ fn guarddog(root: &Path, scope: &Scope, policy: &Policy) -> Result
{ Ok(Section::Clean) } -/// A file that exists for one child process and is removed on the way out. -/// -/// Hand-rolled rather than a crate, because this is the only place the binary -/// needs one and the contract is four lines: named, filled, handed to one -/// command, gone. /// The gitleaks release this binary is tested against. /// /// Pinned here, unlike the other five scanners, because gitleaks' verdict is @@ -1876,26 +1871,93 @@ fn gitleaks_could_not_look(code: i32, _stdout: &str, stderr: &str) -> Option Result { use std::sync::atomic::{AtomicUsize, Ordering}; static NEXT: AtomicUsize = AtomicUsize::new(0); - let path = std::env::temp_dir().join(format!( - "uphold-supply-{}-{}", - std::process::id(), - NEXT.fetch_add(1, Ordering::Relaxed) - )); - std::fs::write(&path, text).map_err(|error| Fatal::at(&path, error))?; - Ok(Self { path }) + let directory = std::env::temp_dir(); + let names = std::iter::repeat_with(|| { + let stamp = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map_or(0, |since| since.as_nanos()); + directory.join(format!( + "uphold-supply-{}-{stamp}-{}", + std::process::id(), + NEXT.fetch_add(1, Ordering::Relaxed) + )) + }); + Self::first_free(&directory, names, text) + } + + /// The first of `names` that did not exist, created and filled. + /// + /// Apart from `containing` so a test can choose the names, and so + /// plant something on them first. + pub(super) fn first_free( + directory: &Path, + names: impl IntoIterator, + text: &str, + ) -> Result { + for path in names.into_iter().take(ATTEMPTS) { + let mut options = std::fs::OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + std::os::unix::fs::OpenOptionsExt::mode(&mut options, 0o600); + match options.open(&path) { + Ok(mut file) => { + // Owned before it is filled, so a write that fails + // still removes the file on the way out. + let made = Self { path }; + file.write_all(text.as_bytes()) + .map_err(|error| Fatal::at(&made.path, error))?; + return Ok(made); + } + Err(error) if error.kind() == ErrorKind::AlreadyExists => {} + Err(error) => return Err(Fatal::at(&path, error)), + } + } + Err(Fatal::at( + directory, + format!( + "{ATTEMPTS} temporary file names in a row were already taken, so something \ + else is writing here and none of them was opened. Point TMPDIR at a \ + directory only you can write to" + ), + )) } } @@ -2247,4 +2309,77 @@ mod tests { ); assert!(ci.contains("must not miss one"), "{ci}"); } + + /// What `fs::write` did to a path somebody reached first: followed the + /// link and overwrote its target, or truncated the file sitting there. A + /// taken name is now passed over and the next one is used. + #[cfg(unix)] + #[test] + fn a_temporary_file_never_opens_a_path_somebody_reached_first() { + use std::os::unix::fs::PermissionsExt as _; + + let root = crate::fixture::scratch("supply-tempfile"); + std::fs::create_dir_all(&root).unwrap(); + let planted = root.join("planted"); + std::fs::write(&planted, "planted\n").unwrap(); + let victim = root.join("victim"); + std::fs::write(&victim, "victim\n").unwrap(); + let linked = root.join("linked"); + std::os::unix::fs::symlink(&victim, &linked).unwrap(); + // A link to nothing is a path that exists as far as `create_new` is + // concerned, and following it would create the file it points at. + let dangling = root.join("dangling"); + let nowhere = root.join("nowhere"); + std::os::unix::fs::symlink(&nowhere, &dangling).unwrap(); + let free = root.join("free"); + + let made = super::tempfile_guard::TempFile::first_free( + &root, + [planted.clone(), linked, dangling, free.clone()], + "scanner configuration\n", + ) + .unwrap(); + assert_eq!(made.path, free); + assert_eq!(std::fs::read_to_string(&planted).unwrap(), "planted\n"); + assert_eq!(std::fs::read_to_string(&victim).unwrap(), "victim\n"); + assert!(!nowhere.exists(), "a dangling link was followed"); + assert_eq!( + std::fs::read_to_string(&free).unwrap(), + "scanner configuration\n" + ); + let bits = std::fs::metadata(&free).unwrap().permissions().mode(); + assert_eq!(bits & 0o777, 0o600, "mode {bits:o}"); + drop(made); + assert!(!free.exists(), "the file outlived the guard"); + } + + /// Every name taken is somebody writing into the directory, and is said + /// so rather than drawn from forever or settled by opening one of them. + #[test] + fn a_temporary_directory_with_every_name_taken_is_refused() { + let root = crate::fixture::scratch("supply-tempfile-taken"); + std::fs::create_dir_all(&root).unwrap(); + let taken = root.join("taken"); + std::fs::write(&taken, "taken\n").unwrap(); + let error = super::tempfile_guard::TempFile::first_free( + &root, + std::iter::repeat(taken.clone()), + "scanner configuration\n", + ) + .err() + .unwrap() + .to_string(); + assert!(error.contains("already taken"), "{error}"); + assert_eq!(std::fs::read_to_string(&taken).unwrap(), "taken\n"); + } + + /// The path the scanners are actually handed, made the way they get it. + #[test] + fn a_temporary_file_holds_what_it_was_given_and_goes_with_its_guard() { + let made = super::tempfile_guard::TempFile::containing("a config\n").unwrap(); + let path = made.path.clone(); + assert_eq!(std::fs::read_to_string(&path).unwrap(), "a config\n"); + drop(made); + assert!(!path.exists(), "the file outlived the guard"); + } }