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..c88676f 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` @@ -3812,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)); @@ -3874,10 +3908,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 +4045,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 +4080,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, @@ -5209,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")); @@ -6342,6 +6393,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/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"); + } } 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