diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 4ec6172..be87dcf 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -478,6 +478,16 @@ exists precisely to skip `commit-msg`. about to publish, and **execs through**. Put a link named for the command on PATH ahead of the real one and `argv[0]` does the rest. +That link is on PATH for the whole machine, while a `[[shim]]` is a line in one +repository's policy — so **where nothing declares the command, the command +simply runs**: no policy in this directory, or a policy that declares a shim for +some other command. Neither is a could-not-look, so neither is exit `2`; the +policy was read and it said this command is not one it stands in front of. A +policy that exists and cannot be *read* still exits `2`, because the +declaration that could not be read might have been the one. Asked for by name — +`uphold shim faux …` — an undeclared command is still an error, since nothing is +standing in front of anything and the caller asked. + ```toml [[shim]] command = "gh" diff --git a/policy/principles.toml b/policy/principles.toml index 3a25e1d..e2eadd5 100644 --- a/policy/principles.toml +++ b/policy/principles.toml @@ -129,20 +129,28 @@ git.hooks = ["pre-push"] [rule.no-private-repo-names] builtin = "no-private-repo-names" # `visibility` is declared rather than looked up, and it is a settled fact -# rather than a placeholder: publication was considered and declined, so this -# repository stays private and the guard stays out of scope. +# rather than a placeholder: this repository was published, so the guard is in +# scope on every commit written here. +# +# It said `private` for a day after the flip, and that is how the one finding +# below was written. The declaration is the whole scope test -- a rule that +# believes its repository is private refuses nothing, silently, and reports a +# clean tree while doing it -- so a declaration left behind by a visibility +# change is not a stale comment, it is the guard switched off. Flipping a +# repository means editing this line in the same change. # # Declared rather than looked up because a lookup makes every commit wait on a -# forge, and because the answer is a decision rather than a reading. The line to -# change if that decision is revisited is this one -- and the check to run -# before changing it is `uphold audit --for-publication`, which asks what a -# flip would republish under the visibility it is flipping TO. Every guard here -# asks "is the target public NOW", so nothing else re-examines a name that was -# correctly allowed at write time. +# forge, and because the answer is a decision rather than a reading. The check +# to run before changing it is `uphold audit --for-publication`, which asks what +# a flip would republish under the visibility it is flipping TO. Every guard +# here asks "is the target public NOW", so nothing else re-examines a name that +# was correctly allowed at write time. # -# It has an answer today, and it is not clean: two retained pull-request head -# refs carry a private organisation's name, and no history rewrite reaches them. -visibility = "private" +# What the flip published, and what no rewrite reaches: commit 188970b2, on the +# retained head ref of the pull request that merged it, names a private +# organisation in its message. The squashed commit on the default branch does +# not. The remaining copy is the forge's to remove, not a rewrite's. +visibility = "public" # Declared, not looked up. A forge lookup answers for `owner/repo`; these are # organisations, and the form that got past a hand audit of this repository was # an organisation named on its own, in a sentence about what it carries. A @@ -160,12 +168,15 @@ git.hooks = ["commit-msg"] [rule.no-private-repo-names-staged] builtin = "no-private-repo-names-staged" -visibility = "private" +# The same settled fact as the variant above, and it has to be written again +# here: each variant carries its own scope, so one left at `private` is one seam +# standing down while its siblings refuse. +visibility = "public" git.hooks = ["pre-commit"] [rule.no-private-repo-names-in-files] builtin = "no-private-repo-names-in-files" -visibility = "private" +visibility = "public" # Not pre-commit: this asks the forge about every distinct name anywhere in the # tree, not the few a commit touches, and a check that adds twenty seconds to # every commit is one somebody deletes. That trade-off is a line in this file diff --git a/src/audit.rs b/src/audit.rs index b0fe11d..9b90327 100644 --- a/src/audit.rs +++ b/src/audit.rs @@ -168,6 +168,17 @@ fn history(root: &Path) -> Result> { /// untouched and a clone does not carry them by default -- so an audit that /// only read local refs would report clean over exactly the surface that /// survives the fix. +/// +/// Pruned, for the reason `refresh_origin` states about branches and this half +/// did not honour: `refs/audit/pull/*` is a local destination that nothing else +/// writes and nothing else deletes, so a ref left by an earlier run outlives the +/// pull request it named -- and outlives the remote it came from, if `origin` +/// was ever repointed. Every commit under it was then read as one this forge +/// serves and reported as `would be republished`, which is the "cries wolf about +/// the unpublishable" failure named a few lines up: findings whose only fix is +/// to delete something the reader would discover was never published there. +/// `--prune` with an explicit refspec prunes that refspec's destination, so a +/// stale ref goes on the next run rather than being audited forever. fn retained_pull_refs(root: &Path) -> Result<(Vec, Vec)> { let mut surfaces = Vec::new(); let mut unreadable = Vec::new(); @@ -175,6 +186,7 @@ fn retained_pull_refs(root: &Path) -> Result<(Vec, Vec)> { .args([ "fetch", "-q", + "--prune", "origin", "+refs/pull/*/head:refs/audit/pull/*", ]) diff --git a/src/main.rs b/src/main.rs index 2ce8b3b..9cca270 100644 --- a/src/main.rs +++ b/src/main.rs @@ -201,7 +201,7 @@ fn run() -> Result { .file_name() .filter(|name| !name.is_empty() && name.to_str() != Some("uphold")) { - return shim_command(text_of(name)?, &arguments); + return shim_command(text_of(name)?, &arguments, shim::Invoked::AsTheCommand); } let Some((first, rest)) = arguments.split_first() else { @@ -250,7 +250,7 @@ fn run() -> Result { let (name, shimmed) = rest .split_first() .ok_or_else(|| Fatal::new(format!("shim needs a command\n\n{USAGE}")))?; - shim_command(text_of(name)?, shimmed) + shim_command(text_of(name)?, shimmed, shim::Invoked::ByName) } other => Err(Fatal::new(format!( "unknown subcommand {other:?}\n\n{USAGE}" @@ -626,9 +626,27 @@ fn effective_rules_command(as_json: bool) -> Result { Ok(Exit::Clean) } -fn shim_command(name: &str, argv: &[OsString]) -> Result { +fn shim_command(name: &str, argv: &[OsString], invoked: shim::Invoked) -> Result { let working = std::env::current_dir()?; - let (root, policy_path) = discover(&working).ok_or_else(|| no_policy_here(&working))?; + // No policy where the command was typed means no repository here declares + // anything to stand in front of it. Run as the command, that is the command + // running: the link is on PATH for the whole machine -- `/tmp`, somebody + // else's checkout, a shell that never enters a participating repository -- + // and refusing there protects nothing, breaks `git` everywhere, and gets + // the link removed, which is how the seam is lost in the repositories that + // DID declare it. Asked for by name, it is still an error, because the + // caller asked this repository for a shim it does not have. + // + // A policy that exists and cannot be read is a different answer and still + // fatal both ways: `config::load` below says so, because a declaration that + // could not be read might have been the one standing in front of this + // command. + let Some((root, policy_path)) = discover(&working) else { + return match invoked { + shim::Invoked::AsTheCommand => shim::exec_through(name, argv), + shim::Invoked::ByName => Err(no_policy_here(&working)), + }; + }; let policy = config::load(&root, &policy_path)?; // The shimmed command's arguments stay bytes all the way to the exec. On // Unix an argument is an arbitrary byte string -- `git add` on a file named @@ -638,7 +656,7 @@ fn shim_command(name: &str, argv: &[OsString]) -> Result { // command that runs is the command that was typed. Where the shim has // something to CHECK, it refuses the untranslatable argument itself, in the // words of what it could not read. - shim::run(&root, &policy, name, argv) + shim::run(&root, &policy, name, argv, invoked) } fn main() { diff --git a/src/shim.rs b/src/shim.rs index 6fbcf3d..681d9d4 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -790,6 +790,45 @@ fn file_identity(path: &Path) -> Option { path.canonicalize().ok() } +/// How this process was reached, which is the only thing that differs when +/// nothing here declares the command. +/// +/// Both are the same seam and the same reading. One is a command being run -- +/// through a link named for it, on a PATH that spans the whole machine -- and +/// the other is a question asked about this repository, typed with the answer +/// in mind. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Invoked { + /// Through a link named for the command: argv[0] decided. + AsTheCommand, + /// As `uphold shim `, with the command named as an argument. + ByName, +} + +/// Run the command with nothing standing in front of it. +/// +/// The transparent path, for the two answers that are not "check this": no +/// policy where the command was typed, and a policy that declares no shim for +/// it. Both are readings rather than failures to read, and a shim installed for +/// the whole machine meets them constantly -- every directory outside a +/// participating repository is one of them. +/// +/// No stdin is replayed because none was collected: reading it belongs to the +/// checking path, and a command whose text nothing here reads must be handed +/// the descriptor it was given rather than a copy of what this process drained +/// out of it. +pub(crate) fn exec_through(name: &str, argv: &[OsString]) -> Result { + let own = std::env::current_exe().ok(); + let Some(real) = real_command(name, own.as_deref()) else { + return Err(Fatal::new(format!( + "nothing here stands in front of {name}, and there is no {name} on PATH to run" + ))); + }; + let mut command = Command::new(&real); + command.args(argv); + hand_off(&mut command, name, None) +} + /// The real command, found by walking PATH past ourselves. /// /// "Past ourselves" is a question about the FILE, not about the directory. A @@ -1083,7 +1122,13 @@ fn hand_off(command: &mut Command, name: &str, stdin: Option<&[u8]>) -> Result Result { +pub(crate) fn run( + root: &Path, + policy: &Policy, + name: &str, + argv: &[OsString], + invoked: Invoked, +) -> Result { let words: Vec = argv .iter() .map(|argument| argument.to_string_lossy().into_owned()) @@ -1102,14 +1147,35 @@ pub(crate) fn run(root: &Path, policy: &Policy, name: &str, argv: &[OsString]) - .map(|shim| (shim.command.as_str(), shim)) .collect(); let Some(shim) = shims.get(name) else { - return Err(Fatal::new(format!( - "no shim declares the command {name:?}; this policy declares {}", - if shims.is_empty() { - String::from("none") - } else { - shims.keys().copied().collect::>().join(", ") - } - ))); + // Nothing here declares this command. The reading is the same either + // way -- an absent declaration is a place the rule does not run, the + // same way an absent `[git]` table is, and it is not a could-not-look, + // so it is not exit 2 by the rule that governs those. What differs is + // what was asked. + // + // Run AS the command, the answer lets it run. The link is on PATH for + // the whole machine while a `[[shim]]` is a line in one repository's + // policy, so refusing an undeclared command meant `git` exiting 2 in + // every repository that had not declared one, and in every directory + // that is not a repository at all. What gets installed after that is + // nothing, which loses the seam everywhere rather than where it was + // undeclared. + // + // Asked for BY NAME, the answer is an error: `uphold shim faux ...` + // names a shim this repository does not have, nothing is standing in + // front of anything, and the caller is entitled to hear that rather + // than watch a typo run. + return match invoked { + Invoked::AsTheCommand => exec_through(name, argv), + Invoked::ByName => Err(Fatal::new(format!( + "no shim declares the command {name:?}; this policy declares {}", + if shims.is_empty() { + String::from("none") + } else { + shims.keys().copied().collect::>().join(", ") + } + ))), + }; }; // Only the rules that name THIS command line. A checker used to be diff --git a/tests/audit_publication_cli.rs b/tests/audit_publication_cli.rs index 1751982..35ef8c8 100644 --- a/tests/audit_publication_cli.rs +++ b/tests/audit_publication_cli.rs @@ -241,3 +241,62 @@ hooks = ["pre-commit"] "a literal owner outside the first variant was never objected to:\n{report}" ); } + +/// A `refs/audit/pull/*` ref the forge no longer serves is not audited. +/// +/// That destination is written by this subcommand and by nothing else, so a ref +/// left by an earlier run stays until something prunes it -- including after +/// `origin` is repointed at a different repository, which is when every ref +/// under it names a pull request the current forge never had. Read unpruned, +/// those commits are reported as `would be republished`, and the reader's only +/// fix is to delete something that was never published where the report says it +/// was. It is the same defect the branch half was fixed for, on the ref set that +/// half does not cover. +#[test] +fn a_pull_ref_the_forge_no_longer_serves_is_not_audited() { + let root = repository(); + // A real remote, because the fetch is what prunes: with no origin at all the + // subcommand reports the surface unreadable and never walks a ref. + let origin = + std::env::temp_dir().join(format!("uphold-publication-origin-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&origin); + git(&root, &["init", "-q", "--bare", origin.to_str().unwrap()]); + git( + &root, + &["remote", "add", "origin", origin.to_str().unwrap()], + ); + + std::fs::write(root.join("a.txt"), "nothing to see\n").unwrap(); + git(&root, &["add", "-A"]); + git(&root, &["commit", "-qm", "one", "--no-verify"]); + git(&root, &["push", "-q", "origin", "main"]); + + // A commit the remote does not have, parked under the audit's own + // destination the way a previous run against another forge would leave it. + std::fs::write(root.join("STALE.md"), "PrivateOrg\n").unwrap(); + git(&root, &["add", "-A"]); + git( + &root, + &[ + "commit", + "-qm", + "a pull request on some other forge", + "--no-verify", + ], + ); + let stale = Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(&root) + .output() + .unwrap(); + let stale = String::from_utf8_lossy(&stale.stdout).trim().to_owned(); + git(&root, &["update-ref", "refs/audit/pull/9", &stale]); + git(&root, &["reset", "-q", "--hard", "HEAD~1"]); + + let output = audit(&root); + let report = text(&output); + assert!( + !report.contains("STALE.md") && !report.contains("some other forge"), + "a pruned pull ref was still read as a surface this forge serves:\n{report}" + ); +} diff --git a/tests/shim_cli.rs b/tests/shim_cli.rs index 285155a..cb0ad20 100644 --- a/tests/shim_cli.rs +++ b/tests/shim_cli.rs @@ -255,6 +255,12 @@ fn the_bypass_names_the_checker_it_switched_off() { assert!(stdout(&output).contains("faux ran:")); } +/// Asked BY NAME for a shim this repository does not have, and told so. +/// +/// The other half of this pair lets the same reading through, and the two are +/// not in tension: what differs is what was asked. `uphold shim unknown ...` is +/// a question typed with an answer in mind, and nothing is standing in front of +/// anything, so the caller hears it rather than watching a typo run. #[test] fn a_command_no_shim_declares_is_refused_rather_than_silently_passed_through() { let root = workspace(POLICY); @@ -267,6 +273,100 @@ fn a_command_no_shim_declares_is_refused_rather_than_silently_passed_through() { ); } +/// A link is on PATH for the whole machine; a `[[shim]]` is a line in one +/// repository's policy. +/// +/// So the undeclared case is the ordinary one, not the exception: every +/// directory outside a participating repository reaches it, and so does every +/// participating repository that declares a shim for some OTHER command. While +/// that answer was an error, installing the link as the documentation describes +/// made the command exit 2 nearly everywhere it was typed -- and what gets +/// installed after that is nothing, which loses the seam in the repositories +/// that did declare it. +#[test] +fn a_command_this_policy_does_not_declare_still_runs_when_the_link_is_the_command() { + let root = workspace(POLICY); + // The real command, behind a link named for it. Two directories, because one + // cannot hold two files of the same name -- and the link has to come first. + std::fs::create_dir_all(root.join("front")).unwrap(); + let stub = root.join("bin/undeclared"); + std::fs::write(&stub, "#!/bin/sh\necho \"undeclared ran: $*\"\n").unwrap(); + let mut permissions = std::fs::metadata(&stub).unwrap().permissions(); + std::os::unix::fs::PermissionsExt::set_mode(&mut permissions, 0o755); + std::fs::set_permissions(&stub, permissions).unwrap(); + let link = root.join("front/undeclared"); + std::os::unix::fs::symlink(env!("CARGO_BIN_EXE_uphold"), &link).unwrap(); + + let path = format!( + "{}:{}:{}", + root.join("front").display(), + root.join("bin").display(), + std::env::var("PATH").unwrap_or_default() + ); + let output = Command::new(&link) + .args(["publish", "--now"]) + .current_dir(&root) + .env("PATH", path) + .env_remove("UPHOLD_ALLOW") + .output() + .unwrap(); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert!( + stdout(&output).contains("undeclared ran: publish --now"), + "{}{}", + stdout(&output), + stderr(&output) + ); +} + +/// No policy where the command was typed is not a repository refusing; it is a +/// directory that declares nothing. +/// +/// `/tmp`, somebody else's checkout, a shell that never enters a participating +/// repository -- a machine-wide link meets these far more often than it meets a +/// declaration. Refusing here protects nothing and breaks the command +/// everywhere. +#[test] +fn a_directory_with_no_policy_at_all_does_not_break_the_command() { + let root = workspace(POLICY); + // Outside `root`, deliberately: a subdirectory of it would find the policy + // by walking up, which is the case this test is not about. + let elsewhere = std::env::temp_dir().join(format!( + "uphold-shim-no-policy-{}-{}", + std::process::id(), + root.file_name().unwrap().to_string_lossy() + )); + let _ = std::fs::remove_dir_all(&elsewhere); + std::fs::create_dir_all(&elsewhere).unwrap(); + + // Named for the stub, so PATH resolution finds the link first and the real + // command second, exactly as an install puts them. + std::fs::create_dir_all(root.join("front")).unwrap(); + let front = root.join("front/faux"); + std::os::unix::fs::symlink(env!("CARGO_BIN_EXE_uphold"), &front).unwrap(); + + let path = format!( + "{}:{}:{}", + root.join("front").display(), + root.join("bin").display(), + std::env::var("PATH").unwrap_or_default() + ); + let output = Command::new(&front) + .args(["pr", "create", "-t", "anything"]) + .current_dir(&elsewhere) + .env("PATH", path) + .env_remove("UPHOLD_ALLOW") + .output() + .unwrap(); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert!( + stdout(&output).contains("faux ran:"), + "{}{}", + stdout(&output), + stderr(&output) + ); +} + #[test] fn a_checker_that_could_not_look_is_not_a_pass() { // Exit 2 is the third answer, and folding it into either of the others is