diff --git a/Cargo.lock b/Cargo.lock index c8cb0d1..5ead6e5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -46,6 +46,12 @@ version = "0.23.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ac07cdecf99051d9a5238b80f35af32cdeba5b336e55d957b318b50137e18da5" +[[package]] +name = "bitflags" +version = "2.13.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3ded4057c258ba199e2d26386d3af3780957ecaee6c4ef4041c6b4b8b97c0b06" + [[package]] name = "bstr" version = "1.13.1" @@ -133,6 +139,16 @@ version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "877a4ace8713b0bcf2a4e7eec82529c029f1d0619886d18145fea96c3ffe5c0f" +[[package]] +name = "errno" +version = "0.3.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" +dependencies = [ + "libc", + "windows-sys", +] + [[package]] name = "find-msvc-tools" version = "0.1.11" @@ -254,6 +270,12 @@ version = "0.2.189" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3eaf3ede3fee6db1a4c2ee091bf8a8b4dccdc6d17f656fb07896ee72867612f2" +[[package]] +name = "linux-raw-sys" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32a66949e030da00e8c7d4434b251670a91556f4144941d37452769c25d58a53" + [[package]] name = "log" version = "0.4.34" @@ -420,6 +442,19 @@ version = "0.8.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d6f6ff9a378485b298a5286656da665ba74413d36db0979633275d2e708145d4" +[[package]] +name = "rustix" +version = "1.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "891efababe418670775f199f0d233d84843c227a0949a883ce15b37c78d6629d" +dependencies = [ + "bitflags", + "errno", + "libc", + "linux-raw-sys", + "windows-sys", +] + [[package]] name = "rustversion" version = "1.0.23" @@ -731,6 +766,7 @@ dependencies = [ "grep-searcher", "ignore", "regex", + "rustix", "serde", "serde-saphyr", "serde_json", diff --git a/Cargo.toml b/Cargo.toml index df59c60..09c58fa 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -113,6 +113,11 @@ tree-sitter-go = "0.25" # disagree, `expand_alias` says which case and how it is closed. shell-words = "1" +[target.'cfg(unix)'.dependencies] +# Git discovers hooks with access(X_OK), which checks the caller's permission +# class and ACLs. Use the same OS check through a safe API. +rustix = { version = "1", features = ["fs"] } + # `strip` is deliberately absent: stripping is a packaging step, and a stripped # binary with no build-id cannot be symbolicated from a core. `debug = 1` keeps # the line tables so a crash names the line it happened on. diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 2acb8f2..4acc372 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -2179,11 +2179,21 @@ be in scope, for a question that is none of the other three (the doubly nested form is the enum's own spelling). `collect = "git-refs"` replaces the argv walk for `git`, whose published text is positional. +For `gh`, an explicit repository flag takes precedence over `GH_REPO`, which +takes precedence over the current origin. An environment-selected destination +is checked for ownership and visibility as its own repository. Host-qualified +GitHub destinations bind visibility and ownership probes to that host; +unsupported enterprise hosts are refused. An explicit host takes precedence +over `GH_HOST`. Repository placeholders in API endpoints can still be expanded +by `gh` for visibility and text checks, but an ownership rule requires a +concrete destination and refuses unresolved placeholders. + **`public-target` asks this repository before it asks anybody else.** Where the policy declares a top-level `visibility`, it settles the question offline: `public` is in scope, `private` and `internal` are out of it, and nothing is spawned. A declaration is about one repository, so it is read only where no -`target_flags` value is present and the destination came from `origin`. A named +`target_flags` value is present and the destination came from `origin`; for +`gh`, neither `GH_REPO` nor `GH_HOST` may override the local context. A named destination always goes to the forge, including `gh -R this-owner/this-repo`, because the name carries no host and `--hostname` or `GH_HOST` can make `owner/repo` a repository on a different forge. A tree whose `origin` cannot be @@ -2792,6 +2802,9 @@ characters, so it is compared as a string and matches no tool. **Which calls arrive is the harness's decision, not this binary's.** Re-deciding it here would be a second matcher free to disagree with the first, and an operator would have to hold both. What arrives is checked. +Tool-name rules also run when arguments contain only numbers, booleans, nulls, +or empty containers. Such calls have no argument-text subject; an explicitly +supplied empty string still has one. ### What runs where there is no policy @@ -3244,6 +3257,10 @@ A report also needs a `results` object and a `risks` array; missing or mistyped fields are unread, never an empty risk list. npm tolerance requires the underlying `results.metadata_mismatch` and the risk's `threat_description` to agree exactly before their differences are accepted. +Each submitted registry dependency must appear in the report: npm aliases are +resolved to their package names and PyPI names are normalized. Missing names +are reported as unread even when other entries are clean. A reported risk +still refuses the run, with any missing coverage printed alongside it. Its older admission, "Some rules failed to run while scanning \" at exit 0 — the two email-domain rules time out routinely — is still read, and still names the packages and how many rules. None of that is a finding: it is @@ -3582,7 +3599,11 @@ refused, never repointed; and a `default_install_hook_types` naming a hook type outside the four is refused, because that type would otherwise stop firing while the install read as one that worked. lefthook is refused by name: it installs and owns its own hooks, and a `core.hooksPath` written here would -displace them. +displace them. When no `core.hooksPath` is set, installation also checks the +default hooks directory Git resolves, including the shared directory of linked +worktrees. Existing executable hooks there are named and refused before any +files or configuration change; migrate their behavior before retrying. +Executable `.sample` files and non-executable files are inert. **`--adopt`** is the one exception to "never replaced", and it replaces only the comments. A file in the directory without this command's marker is @@ -3607,6 +3628,9 @@ command and **not** matching (an older install, or edited since — rerunning and different — with the diff wherever the lines differ — and whether `core.hooksPath` actually points git at the directory. Exit `0` only when all four match and git runs them; `1` otherwise. +On Unix, executable access is checked for the invoking user, including the +applicable permission class and ACLs. An execute bit for another class alone +does not establish that Git can run the hook. ## `uphold probe` — can each hook refuse? diff --git a/src/guard/mod.rs b/src/guard/mod.rs index 1974ac8..a4a44d5 100644 --- a/src/guard/mod.rs +++ b/src/guard/mod.rs @@ -310,6 +310,7 @@ pub(crate) fn target_refusal( rule: &Rule, target: Option<&str>, forge: Option, + hostname: Option<&str>, asked: &mut BTreeMap, ) -> Result> { let Some(builtin) = rule.builtin() else { @@ -328,6 +329,16 @@ pub(crate) fn target_refusal( // `git::owner_repo` is the one parser, so `--repo owner/name`, a full forge // url and the remote all become the same pair. A value it cannot split is // not a destination this guard can judge. + if let Some(named) = target + && crate::shim::repository_placeholder("gh", named)? + { + return Err(Fatal::new(format!( + "rule {:?}: this API destination contains repository placeholders whose effective \ + identity could not be established for ownership checks. Nothing was published; \ + name the destination repository directly in the API endpoint", + rule.id + ))); + } let Some((owner, repo)) = target.and_then(crate::git::owner_repo) else { return Err(Fatal::new(format!( "rule {:?}: this invocation publishes to a repository and nothing here says \ @@ -344,7 +355,7 @@ pub(crate) fn target_refusal( root, policy, rule, - (&owner, &repo), + (&owner, &repo, hostname), forge, asked, push::Seam::Command, diff --git a/src/guard/push.rs b/src/guard/push.rs index 5ef8c5e..189a2dd 100644 --- a/src/guard/push.rs +++ b/src/guard/push.rs @@ -151,23 +151,35 @@ pub(crate) enum Owned { /// /// Spawned through [`crate::shim::inner_tool`] and never `Command::new`: the /// `gh` on PATH is this binary, and an unmarked probe re-enters the shim it is -/// standing behind. -pub(crate) fn forge_owns(cache: &mut BTreeMap, owner: &str, repo: &str) -> Owned { +/// standing behind. `hostname`, where known, binds both probes and the cache +/// entry to that server rather than gh's ambient default host. +pub(crate) fn forge_owns( + cache: &mut BTreeMap, + owner: &str, + repo: &str, + hostname: Option<&str>, +) -> Owned { let key = format!("{owner}/{repo}"); - if let Some(known) = cache.get(&key) { + let cache_key = hostname.map_or_else(|| key.clone(), |host| format!("{host}/{key}")); + if let Some(known) = cache.get(&cache_key) { return known.clone(); } - let answer = ask_forge(owner, &key); - cache.insert(key, answer.clone()); + let answer = ask_forge(owner, &key, hostname); + cache.insert(cache_key, answer.clone()); answer } /// The two requests, without the cache in front of them. -fn ask_forge(owner: &str, key: &str) -> Owned { - let login = match crate::shim::inner_tool("gh") - .args(["api", "user", "--jq", ".login"]) - .output() - { +fn ask_forge(owner: &str, key: &str, hostname: Option<&str>) -> Owned { + let probe = |endpoint: &str, field: &str| { + let mut command = crate::shim::inner_tool("gh"); + command.args(["api", endpoint, "--jq", field]); + if let Some(host) = hostname { + command.args(["--hostname", host]); + } + command.output() + }; + let login = match probe("user", ".login") { Ok(output) if output.status.success() => { String::from_utf8_lossy(&output.stdout).trim().to_owned() } @@ -185,10 +197,7 @@ fn ask_forge(owner: &str, key: &str) -> Owned { if login.eq_ignore_ascii_case(owner) { return Owned::Yes; } - match crate::shim::inner_tool("gh") - .args(["api", &format!("repos/{key}"), "--jq", ".permissions.admin"]) - .output() - { + match probe(&format!("repos/{key}"), ".permissions.admin") { Ok(output) if output.status.success() => { match String::from_utf8_lossy(&output.stdout).trim() { "true" => Owned::Yes, @@ -240,12 +249,12 @@ pub(crate) fn unowned( root: &Path, policy: &Policy, rule: &Rule, - destination: (&str, &str), + destination: (&str, &str, Option<&str>), forge: Option, asked: &mut BTreeMap, seam: Seam, ) -> Result> { - let (owner, repo) = destination; + let (owner, repo, hostname) = destination; let name = format!("{owner}/{repo}"); let (workspace, pinned) = workspace_owner(root, policy, rule)?; @@ -352,7 +361,7 @@ pub(crate) fn unowned( // host neither client answers for, gets exactly the answer it got before // this branch existed rather than an invented one. let verdict = match forge { - Some(Forge::GitHub) => Some(forge_owns(asked, owner, repo)), + Some(Forge::GitHub) => Some(forge_owns(asked, owner, repo, hostname)), Some(Forge::GitLab) | None => None, }; if verdict == Some(Owned::Yes) { @@ -470,7 +479,7 @@ pub(crate) fn prevent_public_push(request: &Request<'_>) -> Result) -> Result) -> Result) -> Result = collected .headlines @@ -231,7 +225,14 @@ pub(crate) fn run(harness: &str, found: Option<&(PathBuf, PathBuf)>) -> Result Result<()> { + let output = crate::shim::inner_tool("git") + .args(["rev-parse", "--path-format=absolute", "--git-path", "hooks"]) + .current_dir(root) + .output() + .map_err(|error| Fatal::new(format!("could not locate git's hooks: {error}")))?; + if !output.status.success() { + return Err(Fatal::new(format!( + "could not locate git's hooks: {}", + String::from_utf8_lossy(&output.stderr).trim() + ))); + } + let location = String::from_utf8(output.stdout) + .map_err(|error| Fatal::new(format!("git's hooks path is not UTF-8: {error}")))?; + let active = Path::new(location.trim_end_matches('\n')); + if let (Ok(active), Ok(target)) = ( + std::fs::canonicalize(active), + std::fs::canonicalize(root.join(directory)), + ) && active == target + { + return Ok(()); + } + let entries = match std::fs::read_dir(active) { + Ok(entries) => entries, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(Fatal::at(active, error)), + }; + let mut displaced = Vec::new(); + for entry in entries { + let entry = entry.map_err(|error| Fatal::at(active, error))?; + let path = entry.path(); + // Git's generated samples are inert, even when marked executable. + if path + .extension() + .is_some_and(|extension| extension == "sample") + { + continue; + } + if path.is_file() && executable(&path) { + displaced.push(path.display().to_string()); + } + } + if displaced.is_empty() { + return Ok(()); + } + displaced.sort(); + Err(Fatal::new(format!( + "setting core.hooksPath to {directory:?} would stop git running these existing \ + executable hooks:\n {}\nNothing was written. Migrate their behavior into the \ + runner's config or the target hook directory, then move the old hooks aside \ + and rerun `hooks --install`", + displaced.join("\n ") + ))) +} + +/// Git uses `access(X_OK)` to discover hooks. Any execute bit is insufficient: +/// an owner cannot borrow a group's or other's execute permission, and ACLs +/// and directory permissions also affect access. +#[cfg(unix)] +fn executable(path: &Path) -> bool { + rustix::fs::access(path, rustix::fs::Access::EXEC_OK).is_ok() +} + +#[cfg(not(unix))] +fn executable(path: &Path) -> bool { + path.is_file() +} + /// `uphold hooks --install --check`: what is in the directory, against what /// this binary would write there, and whether git runs any of it. /// @@ -751,17 +825,12 @@ fn check(root: &Path, runner: &str, directory: &str, hooks_path: Option<&str>) - println!(" {directory}/{stage}: absent"); continue; }; - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt as _; - let metadata = std::fs::metadata(&path).map_err(|error| Fatal::at(&path, error))?; - if metadata.permissions().mode() & 0o111 == 0 { - wrong += 1; - println!( - " {directory}/{stage}: not executable, so git ignores it. \ + if !executable(&path) { + wrong += 1; + println!( + " {directory}/{stage}: not executable, so git ignores it. \ Rerunning `hooks --install` restores its executable permissions" - ); - } + ); } let theirs = effective_lines(¤t); let ours = effective_lines(&written(runner, directory, stage)); diff --git a/src/main.rs b/src/main.rs index d8ff5a3..68947a0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -768,7 +768,7 @@ fn guard_command(arguments: &[OsString]) -> Result { // reports in: a finding reached through `--text` and one reached // through a guard are the same verdict on the same text. let refusals: Vec = - text::judged(text::Seam::Guard, &root, &policy, &source, &text, &[])? + text::judged(text::Seam::Guard, &root, &policy, &source, Some(&text), &[])? .into_iter() .map(|verdict| match verdict { text::Verdict::Guard(refusal) => refusal, diff --git a/src/shim.rs b/src/shim.rs index 43d480e..d4e8f27 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -950,6 +950,12 @@ impl ApiCall { } } +/// Whether a repository identity still depends on the command's API +/// placeholders. Resolution and ownership checks must use the same parser. +pub(crate) fn repository_placeholder(command: &str, text: &str) -> Result { + Ok(Placeholders::of(command, &[])?.asks_for_repository(text)) +} + /// `OWNER/REPO` out of a forge API path, in either forge's spelling. /// /// GitHub puts it in the path (`repos/OWNER/REPO/pulls/1`); GitLab addresses a @@ -3252,7 +3258,7 @@ impl Shim { // with no `origin` has no destination for a declaration to be // about, and that is the resolver's answer above rather than // this one's. - if let Some(standing) = declared_standing(root, policy, collected)? { + if let Some(standing) = declared_standing(root, policy, collected, self)? { return Ok(standing); } match self.visibility(root, &target).as_deref() { @@ -3344,11 +3350,19 @@ impl Shim { ))); }; match forge { - Forge::GitHub => forge_field( - "gh", - &["api", &format!("repos/{target}"), "--jq", ".visibility"], - None, - ), + Forge::GitHub => { + let name = git::owner_repo(target).map_or_else( + || target.to_owned(), + |(owner, repo)| format!("{owner}/{repo}"), + ); + let endpoint = format!("repos/{name}"); + let mut args = vec!["api", endpoint.as_str(), "--jq", ".visibility"]; + let host = git::host(target); + if let Some(host) = host.as_deref() { + args.extend(["--hostname", host]); + } + forge_field("gh", &args, None) + } // Deliberately not `--jq`: `glab api` is not `gh api`, and a shim // that is inert the day one flag differs is the defect this arm // exists to end. The project id is a path, so its separators are @@ -3362,10 +3376,39 @@ impl Shim { } fn resolve_target(&self, root: &Path, collected: &Collected) -> Result> { - if let Some(explicit) = collected.target.as_deref() - && !explicit.is_empty() - { - return Ok(Some(explicit.to_owned())); + let explicit = collected + .target + .as_deref() + .filter(|target| !target.is_empty()); + // `gh`'s repo override is command line first, then GH_REPO. In + // particular an environment-selected repository is never `origin`, + // even when its owner/name happen to match the checkout. + let gh_repo = self.command == "gh" && self.target != Target::None; + let environment = gh_repo + .then(|| { + std::env::var("GH_REPO") + .ok() + .filter(|value| !value.is_empty()) + }) + .flatten(); + if let Some(named) = explicit.or(environment.as_deref()) { + if gh_repo && repository_placeholder("gh", named)? { + // API placeholders are expanded by gh's own remote/default + // resolution, which is not established by an origin guess. + // Visibility can ask gh to expand these same placeholders; + // the ownership guard separately refuses unresolved names. + return Ok(Some(named.to_owned())); + } + return if gh_repo { + gh_repository_target(named).map(Some) + } else { + Ok(Some(named.to_owned())) + }; + } + if gh_repo { + return git::remote_url(root, "origin") + .map(|url| gh_repository_target(&url)) + .transpose(); } Ok(match self.target { Target::None => None, @@ -3384,6 +3427,60 @@ impl Shim { } } +/// Resolve the host-bearing spellings accepted by `gh` without treating an +/// enterprise repository as its same-named github.com counterpart. Enterprise +/// support is deliberately absent here, as it is in `Forge::of_url`. +fn gh_repository_target(named: &str) -> Result { + let parts: Vec<_> = named.split('/').collect(); + let host = if named.contains("://") || named.contains(':') { + git::host(named) + } else if parts.len() == 3 { + parts.first().map(|hostname| hostname.to_lowercase()) + } else { + None + }; + let environment_host = std::env::var("GH_HOST") + .ok() + .filter(|configured| !configured.is_empty()); + if let Some(hostname) = host.as_ref().or(environment_host.as_ref()) + && !matches!( + hostname.to_lowercase().as_str(), + "github.com" | "www.github.com" + ) + { + return Err(Fatal::new(format!( + "gh: destination {named:?} uses host {hostname:?}, whose repository authority \ + cannot be verified by this shim. Nothing was published; use a supported \ + GitHub destination with a matching GH_HOST" + ))); + } + let path = if named.contains("://") || named.contains(':') { + git::owner_repo(named).map(|(owner, repo)| format!("{owner}/{repo}")) + } else { + match parts.as_slice() { + [owner, repo] | [_, owner, repo] => Some(format!("{owner}/{repo}")), + _ => None, + } + }; + let Some(path) = path.filter(|path| { + path.split('/').all(|part| { + !part.is_empty() + && part + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || b"-_.".contains(&byte)) + }) + }) else { + return Err(Fatal::new(format!( + "gh: destination {named:?} could not be resolved as [HOST/]OWNER/REPO. \ + Nothing was published" + ))); + }; + Ok(match host { + Some(hostname) => format!("https://{hostname}/{path}"), + None => path, + }) +} + /// What one invocation is about to publish. #[derive(Debug, Clone, Default)] pub(crate) struct Collected { @@ -3458,7 +3555,8 @@ impl Forge { /// is the destination it will read the declaration for. A declaration is about /// ONE repository, so this reads it only where the destination is the one /// [`Shim::resolve_target`] took from `origin` -- which is to say where nothing -/// on the command line named a destination at all. A `target_flags` value is a +/// on the command line or in `GH_REPO` named a destination, and `GH_HOST` +/// did not change the host context. A `target_flags` value is a /// destination somebody typed, and a typed `owner/repo` carries no host: `-R /// acme/widget` under a `--hostname` or a `GH_HOST` is a repository on another /// forge that happens to share a name with this one, and there is nothing in @@ -3470,7 +3568,14 @@ fn declared_standing( root: &Path, policy: &Policy, collected: &Collected, + shim: &Shim, ) -> Result> { + if shim.command == "gh" + && (std::env::var("GH_REPO").is_ok_and(|repo| !repo.is_empty()) + || std::env::var("GH_HOST").is_ok_and(|host| !host.is_empty())) + { + return Ok(None); + } if collected .target .as_deref() @@ -6647,8 +6752,8 @@ pub(crate) fn run( // // Resolved ONCE, before the loop, and only where some rule judges a // destination. `resolve_target` reads `--repo`/`-R` off argv and - // otherwise `git remote get-url origin`; it asks no forge. An ordinary - // invocation still pays no round trip: `Scope::PublicTarget`'s + // otherwise `GH_REPO` for gh, then `git remote get-url origin`; it + // asks no forge. An ordinary invocation still pays no round trip: `Scope::PublicTarget`'s // `visibility()` is not called from here at all, and the forge question // inside `target_refusal` is reached only for a destination the // allow-list has ALREADY refused -- a call that was about to be stopped @@ -6665,6 +6770,9 @@ pub(crate) fn run( .is_some_and(|builtin| crate::guard::TARGET_GUARDS.contains(&builtin)) }) { let target = shim.resolve_target(root, &collected)?; + // A qualified override or origin carries its own host, which + // takes precedence over gh's ambient default for both probes. + let hostname = target.as_deref().and_then(git::host); let forge = shim.forge(root); let mut asked = BTreeMap::new(); for rule in &checkers { @@ -6690,6 +6798,7 @@ pub(crate) fn run( rule, target.as_deref(), forge, + hostname.as_deref(), &mut asked, )? { destination_refusals += 1; diff --git a/src/supply.rs b/src/supply.rs index 81f41f5..e74b83b 100644 --- a/src/supply.rs +++ b/src/supply.rs @@ -1476,8 +1476,8 @@ fn rules_that_did_not_run(output: &str) -> Option { /// What one `guarddog verify` run established. enum Read { - /// guarddog objected to something, described. - Risks(String), + /// guarddog objected to something, with any missing coverage described. + Risks(String, Option), /// guarddog did not answer, described. Unread(String), /// guarddog ran and objected to nothing. @@ -1509,9 +1509,14 @@ enum Read { /// is not held against the run; the index of each waiver that matched is /// recorded in the ledger, with every package a report named, so the section /// can report the waivers that did not. +#[expect( + clippy::too_many_arguments, + reason = "scan inputs and subprocess output meet at this reader" +)] fn guarddog_read( at: &str, ecosystem: &str, + expected: &BTreeSet, waivers: &[Waiver], ledger: &mut Ledger, code: Option, @@ -1535,17 +1540,23 @@ fn guarddog_read( "guarddog printed no report this could read at {at}" )); }; - // A bare `[]` against a manifest that had dependencies is what a total - // network failure looks like here, and it is not a clean bill of health. - if entries.is_empty() { + // Where the input could not be read, even an empty expected set cannot + // establish coverage. Known inputs are reconciled below, including `[]`. + if entries.is_empty() && expected.is_empty() { return Read::Unread(format!("guarddog reported on no dependency at all at {at}")); } let mut objected: Vec = Vec::new(); + let mut observed = BTreeSet::new(); for entry in &entries { let name = entry .get("dependency") .and_then(serde_json::Value::as_str) .unwrap_or("a dependency it did not name"); + observed.insert(if ecosystem == "pypi" { + references::normalise(name) + } else { + name.to_owned() + }); let Some(result) = entry.get("result") else { return Read::Unread(format!("guarddog said nothing about {name} at {at}")); }; @@ -1637,13 +1648,22 @@ fn guarddog_read( objected.push(format!("{name}{at_version} ({risks} risk(s))")); } } - if objected.is_empty() { - Read::Clean + let missing: Vec<&str> = expected.difference(&observed).map(String::as_str).collect(); + let unread = (!missing.is_empty()).then(|| { + format!( + "guarddog omitted {} at {at}: no report for submitted {ecosystem} dependency(s)", + missing.join(", ") + ) + }); + if !objected.is_empty() { + Read::Risks( + format!("{at}: guarddog objected to {}", objected.join(", ")), + unread, + ) + } else if let Some(reason) = unread { + Read::Unread(reason) } else { - Read::Risks(format!( - "{at}: guarddog objected to {}", - objected.join(", ") - )) + Read::Clean } } @@ -1791,6 +1811,7 @@ fn guarddog(root: &Path, scope: &Scope, policy: &Policy) -> Result
{ match guarddog_read( &at, "pypi", + &sorted.expected, waivers, &mut ledger, status.status.code(), @@ -1800,8 +1821,11 @@ fn guarddog(root: &Path, scope: &Scope, policy: &Policy) -> Result
{ Read::Clean => { ledger.read.insert("pypi"); } - Read::Risks(said) => { - ledger.read.insert("pypi"); + Read::Risks(said, unread) => { + if unread.is_none() { + ledger.read.insert("pypi"); + } + unrun = unrun.or(unread); println!(" FAILED: guarddog pypi: {said}"); refused = true; } @@ -1865,6 +1889,7 @@ fn guarddog(root: &Path, scope: &Scope, policy: &Policy) -> Result
{ match guarddog_read( &at, "npm", + &sorted.expected, waivers, &mut ledger, status.status.code(), @@ -1874,8 +1899,11 @@ fn guarddog(root: &Path, scope: &Scope, policy: &Policy) -> Result
{ Read::Clean => { ledger.read.insert("npm"); } - Read::Risks(said) => { - ledger.read.insert("npm"); + Read::Risks(said, unread) => { + if unread.is_none() { + ledger.read.insert("npm"); + } + unrun = unrun.or(unread); println!(" FAILED: guarddog npm: {said}"); refused = true; } @@ -2181,13 +2209,13 @@ mod tempfile_guard { #[cfg(test)] mod tests { + use super::{BTreeSet, Ledger, Read, guarddog_read, tolerated}; use super::{ DENY_FLOOR, Floor, GITLEAKS_DEFAULT, GITLEAKS_FOUND, GITLEAKS_VERSION, GUARDDOG_FLOOR, OSV_FLOOR, PRUNE, Scope, VET_FLOOR, Version, ZIZMOR_FLOOR, against_floor, find_named, find_workflow_dirs, gitleaks_could_not_look, guarddog_unpacks_under, interesting, parse_version, unscanned_ci, }; - use super::{Ledger, Read, guarddog_read, tolerated}; /// guarddog 3.2.0's reports, recorded against the public registries with /// only the unpack path rewritten: react-dom 19.2.0, whose registry @@ -2221,6 +2249,7 @@ mod tests { let read = guarddog_read( "web", ecosystem, + &BTreeSet::new(), &[], &mut Ledger::default(), Some(0), @@ -2228,7 +2257,7 @@ mod tests { "", ); match read { - Read::Risks(_) => Ok(true), + Read::Risks(..) => Ok(true), Read::Clean => Ok(false), Read::Unread(said) => Err(said), } diff --git a/src/supply/references.rs b/src/supply/references.rs index d21465a..99faa16 100644 --- a/src/supply/references.rs +++ b/src/supply/references.rs @@ -23,7 +23,7 @@ //! [`crate::config::PolicyFile::owner`] gives: a first-party set read off the //! remote is repointed by the same command that repoints the remote. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use std::path::Path; use std::process::Stdio; @@ -56,6 +56,8 @@ pub(super) struct Sorted { /// How many of the kept lines are requirements an index resolves, which is /// what guarddog can look up. Zero is a run with nothing to ask it. pub indexed: usize, + /// Canonical index package names that guarddog must report on. + pub expected: BTreeSet, /// Git sources, for the owner check and the remote. pub git: Vec, /// Direct references refused as they stand, each a line naming it. @@ -84,6 +86,13 @@ pub(super) fn sort(export: &str, directory: &Path, root: &Path, lock: Option<&st by_reference(name, reference, directory, root, &tags, &mut sorted) } else { sorted.indexed += 1; + let name = trimmed + .split(|character: char| { + !character.is_ascii_alphanumeric() && !matches!(character, '-' | '_' | '.') + }) + .next() + .unwrap_or(trimmed); + sorted.expected.insert(normalise(name)); true }; if keep { @@ -191,7 +200,7 @@ fn split_git(url: &str) -> Option<(String, String)> { } /// PEP 503's normalised name, which is how the lock and the export agree. -fn normalise(name: &str) -> String { +pub(super) fn normalise(name: &str) -> String { let mut out = String::with_capacity(name.len()); let mut run = false; for character in name.chars() { @@ -258,6 +267,8 @@ pub(super) struct NpmSorted { pub kept: Option, /// How many dependencies are left for npm to resolve. pub indexed: usize, + /// Real npm package names, with aliases merged as guarddog merges them. + pub expected: BTreeSet, /// Whether the manifest parsed as a JSON object, so that `indexed` counts /// its `dependencies` -- none at all, or only `devDependencies`, being /// zero -- rather than a file nobody could read. @@ -323,6 +334,11 @@ pub(super) fn sort_npm(manifest: &str, locks: &[String], include_dev: bool) -> N false }); sorted.indexed += dependencies.len(); + for (name, spec) in dependencies.iter() { + sorted + .expected + .insert(npm_identity(name, spec.as_str()).to_owned()); + } } if taken { sorted.kept = serde_json::to_string(&parsed).ok(); @@ -330,6 +346,31 @@ pub(super) fn sort_npm(manifest: &str, locks: &[String], include_dev: bool) -> N sorted } +/// The package guarddog 3.2.0's `resolve_npm_alias` asks the registry for. +/// Invalid alias spellings stay under their manifest name, as they do there. +fn npm_identity<'a>(name: &'a str, spec: Option<&'a str>) -> &'a str { + let Some(alias) = spec.and_then(|spec| spec.strip_prefix("npm:")) else { + return name; + }; + let after_scope = alias.strip_prefix('@').unwrap_or(alias); + let package_end = after_scope.find('@').map_or(alias.len(), |index| { + index + usize::from(alias.starts_with('@')) + }); + let Some(package) = alias.get(..package_end) else { + return name; + }; + let valid = !package.is_empty() + && !package.chars().any(char::is_whitespace) + && (!alias.starts_with('@') + || package.strip_prefix('@').is_some_and(|scoped| { + scoped.split_once('/').is_some_and(|(scope, scoped_name)| { + !scope.is_empty() && !scoped_name.is_empty() && !scope.contains('/') + }) + })) + && alias.get(package_end..) != Some("@"); + if valid { package } else { name } +} + /// The remote and the `#` fragment of an npm git dependency, or `None` for /// anything npm resolves from its registry, a path or a tarball URL. fn npm_git(spec: &str) -> Option<(String, Option<&str>)> { diff --git a/src/text.rs b/src/text.rs index c73fc79..d570df0 100644 --- a/src/text.rs +++ b/src/text.rs @@ -300,20 +300,23 @@ pub(crate) struct Headline { /// Everything a piece of published text is judged by, at one seam. /// /// The single assembly. `label` is what the subject is called in a report -- -/// the tool name at the hook, the source at `--text` -- and is reported rather -/// than matched on. `text` is the prose; `headlines` are the subjects the seam -/// could tell apart from it, which only the harness hook can, by a tool call's +/// the tool name at the hook, the source at `--text` -- and is also matched as +/// the hook's `tool` subject. `text` is the prose; `headlines` are the subjects +/// the seam could tell apart from it, which only the harness hook can, by a tool call's /// argument names. Every checker but the guards reads the two joined, as it -/// always has; the guards are handed each headline as one, so the lookalike -/// pass a title gets at the shim is the pass it gets here. +/// always has; `None` means the call has no argument text, while its tool name +/// remains a subject. The guards are handed each headline as one, so the +/// lookalike pass a title gets at the shim is the pass it gets here. pub(crate) fn judged( seam: Seam, root: &std::path::Path, policy: &Policy, label: &str, - text: &str, + text: Option<&str>, headlines: &[Headline], ) -> Result> { + let has_text = text.is_some() || !headlines.is_empty(); + let text = text.unwrap_or_default(); let joined; let whole = if headlines.is_empty() { text @@ -326,6 +329,9 @@ pub(crate) fn judged( joined.as_str() }; over_kinds(seam, |kind| { + if !has_text && kind != Judged::Patterns { + return Ok(Vec::new()); + } Ok(match kind { Judged::Literals => failures_in(root, policy, whole)? .into_iter() @@ -383,10 +389,12 @@ pub(crate) fn judged( .collect(), // Only the hook consults these here, and only the rules whose // `seams` names it -- see `Seam::runs_by_default`. - Judged::Patterns => patterns_over(policy, seam, label, whole, headlines)? - .into_iter() - .map(Verdict::Rule) - .collect(), + Judged::Patterns => { + patterns_over(policy, seam, label, has_text.then_some(whole), headlines)? + .into_iter() + .map(Verdict::Rule) + .collect() + } // The shim's, and the shim does not arrive here: it holds a subject // and a per-rule scope this function is not given, so it dispatches // rule by rule through `Judged::of` instead. @@ -467,7 +475,7 @@ fn patterns_over( policy: &Policy, seam: Seam, label: &str, - text: &str, + text: Option<&str>, headlines: &[Headline], ) -> Result> { // The shim runs its pattern rules itself, over the subjects it collected, @@ -478,24 +486,25 @@ fn patterns_over( } // Each with the label a report names it by: the kind alone for the call's // own two, the key for a headline, so a finding says which field it was in. - let mut subjects = vec![ - ( + let mut subjects = Vec::new(); + if let Some(text) = text { + subjects.push(( None, crate::shim::Subject { kind: "text", value: text.to_owned(), added: None, }, - ), - ( - None, - crate::shim::Subject { - kind: "tool", - value: label.to_owned(), - added: None, - }, - ), - ]; + )); + } + subjects.push(( + None, + crate::shim::Subject { + kind: "tool", + value: label.to_owned(), + added: None, + }, + )); subjects.extend(headlines.iter().map(|headline| { ( Some(headline.label.as_str()), @@ -545,7 +554,7 @@ fn patterns_over( pub(crate) fn check(found: Option<&(PathBuf, PathBuf)>, source: &str) -> Result { let text = read(source)?; let (root, policy) = load_for(found)?; - let verdicts = judged(Seam::Scan, &root, &policy, source, &text, &[])?; + let verdicts = judged(Seam::Scan, &root, &policy, source, Some(&text), &[])?; for verdict in &verdicts { match verdict { diff --git a/tests/hook_cli.rs b/tests/hook_cli.rs index 6571cac..aaad268 100644 --- a/tests/hook_cli.rs +++ b/tests/hook_cli.rs @@ -495,9 +495,9 @@ fn a_string_under_an_unknown_field_name_is_still_read() { ); } -/// A call carrying no text at all publishes nothing there is a rule about. +/// A call carrying no argument text is allowed when no rule refuses its tool. #[test] -fn a_call_with_no_strings_is_allowed_without_looking_further() { +fn a_call_with_no_strings_is_allowed_when_its_tool_is_allowed() { let root = workspace("hook-no-strings", Some(POLICY)); let output = hook( &root, @@ -627,6 +627,54 @@ fn a_regexp_rule_naming_the_hook_refuses_the_tool_call_by_its_name() { } } +/// Tool names remain subjects when the argument tree contains no strings. +#[test] +fn string_free_calls_still_judge_the_tool_without_inventing_argument_text() { + let policy = format!( + "{RELEASE_POLICY}\n\ + [rule.require-argument-text]\n\ + message = \"Supply nonempty text.\"\n\ + require_regexp = '.'\n\ + files.exclude = [\"policy/**\"]\n\ + subjects = [\"text\"]\n\ + seams = [\"hook\"]\n\ + command.before = [\"gh\"]\n" + ); + for harness in ["claude-code", "codex"] { + let root = workspace("hook-string-free-tool", Some(&policy)); + for input in [ + serde_json::json!({}), + serde_json::json!({"id": 7, "draft": false}), + serde_json::json!({"nested": [null, 7, {}]}), + Value::Null, + ] { + for tool in ["mcp__github__create_release", "mcp__github__get_release"] { + let event = serde_json::json!({"tool_name": tool, "tool_input": input}); + let output = hook(&root, harness, &event.to_string()); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + if tool == "mcp__github__create_release" { + let explanation = reason(&output); + assert!(explanation.contains("no-release-by-hand"), "{explanation}"); + assert!( + !explanation.contains("require-argument-text"), + "{explanation}" + ); + let response: Value = serde_json::from_str(&stdout(&output)).unwrap(); + assert_eq!( + response.pointer("/hookSpecificOutput/permissionDecision"), + Some(&Value::String("deny".to_owned())) + ); + } else { + assert_eq!(stdout(&output), "", "{}", stderr(&output)); + } + } + } + // An explicitly supplied empty string is still a text subject. + let output = hook(&root, harness, &event("mcp__github__get_release", "")); + assert!(reason(&output).contains("require-argument-text")); + } +} + /// Without `seams`, a `regexp` rule stays where it ran before the field /// existed: at the shim, and not over every tool call. #[test] diff --git a/tests/hooks_install_cli.rs b/tests/hooks_install_cli.rs index c91b832..af0eb2d 100644 --- a/tests/hooks_install_cli.rs +++ b/tests/hooks_install_cli.rs @@ -140,6 +140,88 @@ fn a_foreign_core_hookspath_is_refused_rather_than_repointed() { ); } +#[test] +#[cfg(unix)] +fn active_default_hooks_are_preserved_in_repositories_and_linked_worktrees() { + use std::os::unix::fs::PermissionsExt as _; + + for linked in [false, true] { + let main = repository(); + support::git(&main, &["config", "user.name", "Test"]); + support::git(&main, &["config", "user.email", "test@example.test"]); + support::git(&main, &["add", "-A"]); + support::git(&main, &["commit", "-qm", "fixture"]); + let root = if linked { + let worktree = support::scratch("hooks-install-worktree"); + support::git( + &main, + &[ + "worktree", + "add", + "-qb", + "linked", + worktree.to_str().unwrap(), + ], + ); + assert!(worktree.join(".git").is_file()); + worktree + } else { + main.clone() + }; + for stage in ["pre-commit", "post-commit"] { + let hook = main.join(".git/hooks").join(stage); + std::fs::write(&hook, format!("#!/bin/sh\necho ran >> {stage}-ran\n")).unwrap(); + std::fs::set_permissions(&hook, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + support::git(&root, &["commit", "--allow-empty", "-qm", "before"]); + + let stub = stub_runner("prek"); + let output = install(&root, &[], &stub); + assert_eq!(code(&output), 2, "{}", text(&output)); + let report = text(&output); + for stage in ["pre-commit", "post-commit"] { + assert!( + report.contains(&main.join(".git/hooks").join(stage).display().to_string()), + "{report}" + ); + } + assert!(report.contains("Migrate their behavior"), "{report}"); + assert!(report.contains("Nothing was written"), "{report}"); + assert!(!root.join(".githooks").exists()); + assert_eq!(hooks_path(&root), ""); + support::git(&root, &["commit", "--allow-empty", "-qm", "after"]); + for stage in ["pre-commit", "post-commit"] { + assert_eq!( + std::fs::read_to_string(root.join(format!("{stage}-ran"))).unwrap(), + "ran\nran\n" + ); + } + } +} + +#[test] +#[cfg(unix)] +fn inert_default_hooks_and_executable_samples_do_not_prevent_installation() { + use std::os::unix::fs::PermissionsExt as _; + + let root = repository(); + let stub = stub_runner("prek"); + let inert = root.join(".git/hooks/post-commit"); + std::fs::write(&inert, "#!/bin/sh\nexit 0\n").unwrap(); + std::fs::set_permissions(&inert, std::fs::Permissions::from_mode(0o644)).unwrap(); + let sample = root.join(".git/hooks/pre-commit.sample"); + std::fs::write(&sample, "#!/bin/sh\nexit 1\n").unwrap(); + std::fs::set_permissions(&sample, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let output = install(&root, &[], &stub); + assert_eq!(code(&output), 0, "{}", text(&output)); + assert_eq!(hooks_path(&root), ".githooks"); + assert_eq!( + std::fs::read_to_string(inert).unwrap(), + "#!/bin/sh\nexit 0\n" + ); +} + #[test] fn a_hook_type_outside_the_four_is_refused_rather_than_switched_off() { let root = repository(); @@ -551,6 +633,55 @@ fn check_refuses_hooks_git_ignores_without_changing_their_permissions() { } } +#[test] +#[cfg(unix)] +fn check_uses_the_owners_execute_access_instead_of_another_permission_class() { + use std::os::unix::fs::PermissionsExt as _; + + let root = repository(); + let stub = stub_runner("prek"); + let installed = install(&root, &[], &stub); + assert_eq!(code(&installed), 0, "{}", text(&installed)); + let hook = root.join(".githooks/pre-commit"); + for mode in [0o645, 0o654] { + std::fs::set_permissions(&hook, std::fs::Permissions::from_mode(mode)).unwrap(); + // Root can execute when ANY class permits it, so this fixture cannot + // reproduce the owner's denied access there. Ordinary Unix CI users + // exercise both permission classes against Git itself below. + if rustix::fs::access(&hook, rustix::fs::Access::EXEC_OK).is_ok() { + return; + } + let git = support::git_command(&root) + .args(["hook", "run", "--ignore-missing", "pre-commit"]) + .env("PATH", &stub) + .output() + .unwrap(); + assert!(git.status.success(), "{}", text(&git)); + assert!( + text(&git).contains("not set as executable"), + "{}", + text(&git) + ); + assert!(!text(&git).contains("runner ran"), "{}", text(&git)); + + let checked = install(&root, &["--check"], &stub); + assert_eq!(code(&checked), 1, "{}", text(&checked)); + assert!( + text(&checked).contains("pre-commit: not executable, so git ignores it"), + "{}", + text(&checked) + ); + assert_eq!( + std::fs::metadata(&hook).unwrap().permissions().mode() & 0o777, + mode + ); + } + let repaired = install(&root, &[], &stub); + assert_eq!(code(&repaired), 0, "{}", text(&repaired)); + let checked = install(&root, &["--check"], &stub); + assert_eq!(code(&checked), 0, "{}", text(&checked)); +} + #[test] fn adopt_and_check_together_are_a_usage_error() { let root = repository(); diff --git a/tests/shim_cli.rs b/tests/shim_cli.rs index 7485be9..52e33c4 100644 --- a/tests/shim_cli.rs +++ b/tests/shim_cli.rs @@ -92,6 +92,7 @@ fn shim_with(root: &Path, args: &[&str], variables: &[(&str, &str)]) -> Output { .env_remove("UPHOLD_ALLOW") .env_remove("UPHOLD_SHIM_INNER") .env_remove("GH_REPO") + .env_remove("GH_HOST") .env_remove("GITLAB_GROUP") .envs(variables.iter().copied()) .output() @@ -1088,7 +1089,7 @@ fn gh_without_policy(root: &Path) -> (PathBuf, String) { "gh", "#!/bin/sh\n\ case \"$*\" in\n\ - 'api repos/example-user/public-widget --jq .visibility') echo public ;;\n\ + 'api repos/example-user/public-widget --jq .visibility' | 'api repos/example-user/public-widget --jq .visibility --hostname github.com') echo public ;;\n\ 'api repos/'*) echo private ;;\n\ *) echo \"gh ran: $*\" ;;\n\ esac\n", @@ -1220,6 +1221,7 @@ fn with_no_policy_a_gist_description_cannot_publish_a_declared_private_name() { .env_remove("UPHOLD_SHIM_INNER") .env_remove("GH_EDITOR") .env_remove("GH_REPO") + .env_remove("GH_HOST") .output() .unwrap(); assert_eq!(code(&output), 1, "{flag}: {}", stderr(&output)); @@ -6173,6 +6175,187 @@ fn a_destination_this_workspace_does_not_own_is_refused_and_the_command_never_ru ); } +/// The real `gh` repository override, rather than a fixture command that +/// does not interpret `GH_REPO`. +fn gh_target_workspace() -> PathBuf { + let root = workspace(&TARGET_POLICY.replace("faux", "gh")); + support::git( + &root, + &[ + "remote", + "add", + "origin", + "https://github.com/example-user/widget.git", + ], + ); + stub_gh_says_no(&root); + root +} + +#[test] +fn gh_repo_selects_the_destination_before_the_checkout_origin() { + let root = gh_target_workspace(); + for (repository, expected) in [ + ("other-owner/their-repo", 1), + ("github.com/other-owner/their-repo", 1), + ("https://github.com/other-owner/their-repo", 1), + ("example-user/widget", 0), + ("github.com/example-user/widget", 0), + ] { + let output = shim_with( + &root, + &["gh", "issue", "create", "-b", "Ordinary body"], + &[("GH_REPO", repository)], + ); + assert_eq!(code(&output), expected, "{repository}: {}", stderr(&output)); + assert_eq!(stdout(&output).contains("gh ran:"), expected == 0); + if expected == 1 { + assert!( + stderr(&output).contains("other-owner/their-repo"), + "{}", + stderr(&output) + ); + } + } +} + +#[test] +fn an_explicit_gh_repository_takes_precedence_over_gh_repo() { + let root = gh_target_workspace(); + for (arguments, environment, expected) in [ + ( + vec!["--repo", "example-user/widget"], + "other-owner/their-repo", + 0, + ), + ( + vec!["--repo", "example-user/widget"], + "github.enterprise.example/other-owner/their-repo", + 0, + ), + (vec!["-Rexample-user/widget"], "other-owner/their-repo", 0), + ( + vec!["--repo=other-owner/their-repo"], + "example-user/widget", + 1, + ), + ( + vec!["-R", "other-owner/their-repo"], + "example-user/widget", + 1, + ), + ] { + let mut args = vec!["gh", "issue", "create", "-b", "Ordinary body"]; + args.extend(arguments); + let output = shim_with(&root, &args, &[("GH_REPO", environment)]); + assert_eq!(code(&output), expected, "{}", stderr(&output)); + assert_eq!(stdout(&output).contains("gh ran:"), expected == 0); + } +} + +#[test] +fn gh_repository_hosts_are_verified_before_an_owner_allowance() { + let root = gh_target_workspace(); + // The owner is allowed. A host dropped during resolution would therefore + // pass without a probe, even though it is another server's repository. + for variables in [ + vec![("GH_REPO", "github.enterprise.example/example-user/widget")], + vec![( + "GH_REPO", + "https://github.enterprise.example/example-user/widget", + )], + vec![ + ("GH_REPO", "example-user/widget"), + ("GH_HOST", "github.enterprise.example"), + ], + ] { + let output = shim_with( + &root, + &["gh", "issue", "create", "-b", "Ordinary body"], + &variables, + ); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + assert!(!stdout(&output).contains("gh ran:"), "{}", stdout(&output)); + assert!( + stderr(&output).contains("github.enterprise.example"), + "{}", + stderr(&output) + ); + assert!( + stderr(&output).contains("Nothing was published"), + "{}", + stderr(&output) + ); + } +} + +#[test] +fn gh_ownership_probes_follow_the_effective_repository_host() { + for (repository, explicit, ambient_host, expected_hostname) in [ + (Some("other-owner/their-repo"), None, "github.com", ""), + ( + Some("github.com/other-owner/their-repo"), + None, + "github.enterprise.example", + " --hostname github.com", + ), + ( + Some("https://github.com/other-owner/their-repo"), + None, + "github.enterprise.example", + " --hostname github.com", + ), + ( + Some("github.enterprise.example/ignored/repo"), + Some("github.com/other-owner/their-repo"), + "github.enterprise.example", + " --hostname github.com", + ), + (None, None, "", " --hostname github.com"), + ( + None, + None, + "github.enterprise.example", + " --hostname github.com", + ), + ] { + let root = gh_target_workspace(); + support::git( + &root, + &[ + "remote", + "set-url", + "origin", + "https://github.com/other-owner/their-repo.git", + ], + ); + stub( + &root, + "gh", + "#!/bin/sh\nprintf '%s\\n' \"$*\" >> \"$(dirname \"$0\")/../gh.asked\"\ncase \"$*\" in\n'api user --jq .login'*) echo not-the-owner ;;\n'api repos/other-owner/their-repo --jq .permissions.admin'*) echo true ;;\n*) echo \"gh ran: $*\" ;;\nesac\n", + ); + let mut arguments = vec!["gh", "issue", "create", "-b", "Ordinary body"]; + if let Some(named) = explicit { + arguments.extend(["--repo", named]); + } + let mut variables = vec![("GH_HOST", ambient_host)]; + if let Some(named) = repository { + variables.push(("GH_REPO", named)); + } + let output = shim_with(&root, &arguments, &variables); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert!(stdout(&output).contains("gh ran:"), "{}", stdout(&output)); + let queries = std::fs::read_to_string(root.join("gh.asked")).unwrap(); + for query in [ + "api user --jq .login", + "api repos/other-owner/their-repo --jq .permissions.admin", + ] { + let expected = format!("{query}{expected_hostname}"); + assert!(queries.lines().any(|line| line == expected), "{queries}"); + } + } +} + #[test] fn a_destination_under_this_workspaces_own_owner_passes_and_the_command_runs() { // The half that is easy to lose: a destination guard that refuses @@ -6968,8 +7151,8 @@ fn stub_gh_says_no(root: &Path) { "gh", "#!/bin/sh\n\ case \"$*\" in\n\ - 'api user --jq .login') echo not-the-owner ;;\n\ - 'api repos/'*' --jq .permissions.admin') echo false ;;\n\ + 'api user --jq .login'*) echo not-the-owner ;;\n\ + 'api repos/'*' --jq .permissions.admin'*) echo false ;;\n\ *) echo \"gh ran: $*\" ;;\n\ esac\n", ); @@ -6990,6 +7173,86 @@ target = "forge-repo" scope = "always" "#; +#[test] +fn an_api_repository_template_retains_text_checks_without_inventing_a_destination() { + for endpoint in ["repos/{owner}/{repo}/issues", "repos/:owner/:repo/issues"] { + for (policy, expected) in [(API_POLICY, 0), (API_TARGET_POLICY, 2)] { + let root = workspace(policy); + stub_gh_says_no(&root); + support::git( + &root, + &[ + "remote", + "add", + "origin", + "https://github.com/example-user/widget.git", + ], + ); + let output = shim( + &root, + &["gh", "api", "-X", "POST", endpoint, "-f", "title=Ordinary"], + ); + assert_eq!(code(&output), expected, "{}", stderr(&output)); + assert_eq!(stdout(&output).contains("gh ran:"), expected == 0); + if expected == 2 { + assert!( + stderr(&output).contains("repository placeholders"), + "{}", + stderr(&output) + ); + assert!( + stderr(&output).contains("Nothing was published"), + "{}", + stderr(&output) + ); + } + } + } +} + +#[test] +fn an_api_repository_template_keeps_public_target_visibility_checks() { + let root = workspace(&API_POLICY.replace("scope = \"always\"", "scope = \"public-target\"")); + support::git( + &root, + &[ + "remote", + "add", + "origin", + "https://github.com/example-user/widget.git", + ], + ); + stub( + &root, + "gh", + "#!/bin/sh\nprintf '%s\\n' \"$*\" >> \"$(dirname \"$0\")/../gh.asked\"\ncase \"$*\" in\n'api repos/{owner}/{repo} --jq .visibility' | 'api repos/:owner/:repo --jq .visibility') echo public ;;\n*) echo \"gh ran: $*\" ;;\nesac\n", + ); + for repository in ["{owner}/{repo}", ":owner/:repo"] { + let endpoint = format!("repos/{repository}/issues"); + for (title, expected) in [("Ordinary", 0), ("Generated with Claude Code", 1)] { + let output = shim( + &root, + &[ + "gh", + "api", + "-X", + "POST", + &endpoint, + "-f", + &format!("title={title}"), + ], + ); + assert_eq!(code(&output), expected, "{}", stderr(&output)); + assert_eq!(stdout(&output).contains("gh ran:"), expected == 0); + } + let queries = std::fs::read_to_string(root.join("gh.asked")).unwrap(); + assert!( + queries.contains(&format!("api repos/{repository} --jq .visibility")), + "{queries}" + ); + } +} + #[test] fn a_body_bound_for_a_repository_this_workspace_does_not_own_is_refused() { // The text is ordinary prose. What is wrong with this call is where it is @@ -7088,7 +7351,7 @@ fn a_destination_the_forge_says_the_operator_owns_is_published_to() { "gh", "#!/bin/sh\n\ case \"$*\" in\n\ - 'api user --jq .login') echo not-the-owner ;;\n\ + 'api user --jq .login'*) echo not-the-owner ;;\n\ 'api repos/'*' --jq .permissions.admin') echo true ;;\n\ *) echo \"gh ran: $*\" ;;\n\ esac\n", @@ -7210,8 +7473,8 @@ fn stub_gh_says_private_and_no(root: &Path) { "gh", "#!/bin/sh\n\ case \"$*\" in\n\ - 'api user --jq .login') echo not-the-owner ;;\n\ - 'api repos/'*' --jq .permissions.admin') echo false ;;\n\ + 'api user --jq .login'*) echo not-the-owner ;;\n\ + 'api repos/'*' --jq .permissions.admin'*) echo false ;;\n\ 'api repos/'*' --jq .visibility') echo private ;;\n\ *) echo \"gh ran: $*\" ;;\n\ esac\n", @@ -7479,6 +7742,47 @@ fn a_declaration_about_this_repository_is_not_read_at_somebody_elses() { ); } +#[test] +fn a_private_declaration_does_not_answer_for_the_gh_repo_destination() { + let root = declared_workspace( + &DECLARED_POLICY + .replace("faux", "gh") + .replace("git-remote", "forge-repo"), + "https://github.com/acme/widget.git", + ); + stub( + &root, + "gh", + "#!/bin/sh\nprintf '%s\\n' \"$*\" >> \"$(dirname \"$0\")/../gh.asked\"\ncase \"$*\" in\n'api repos/other-owner/public-widget --jq .visibility'*) echo public ;;\n*) echo \"gh ran: $*\" ;;\nesac\n", + ); + for (repository, ambient_host) in [ + ("other-owner/public-widget", "github.com"), + ( + "github.com/other-owner/public-widget", + "github.enterprise.example", + ), + ] { + let output = shim_with( + &root, + &["gh", "pr", "create", "-t", "Generated with Claude Code"], + &[("GH_REPO", repository), ("GH_HOST", ambient_host)], + ); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!(!stdout(&output).contains("gh ran:"), "{}", stdout(&output)); + } + let queries = std::fs::read_to_string(root.join("gh.asked")).unwrap(); + assert!( + queries.contains("api repos/other-owner/public-widget --jq .visibility\n"), + "{queries}" + ); + assert!( + queries + .contains("api repos/other-owner/public-widget --jq .visibility --hostname github.com"), + "{queries}" + ); + assert!(!queries.contains("repos/acme/widget"), "{queries}"); +} + #[test] fn a_repository_that_declares_nothing_still_asks_the_forge() { // The unchanged path, asserted rather than assumed: a policy with no diff --git a/tests/supply_chain_cli.rs b/tests/supply_chain_cli.rs index 603515e..19fe201 100644 --- a/tests/supply_chain_cli.rs +++ b/tests/supply_chain_cli.rs @@ -192,7 +192,7 @@ fn journal(root: &Path) -> String { /// answers `exit 0` no longer models the tool -- a clean run PRINTS a report /// whose `risks` list is empty, and printing nothing is a run that did not /// report, which is could-not-look. -const GUARDDOG_CLEAN: &str = "echo '[{\"dependency\":\"six\",\"result\":\ +const GUARDDOG_CLEAN: &str = "echo '[{\"dependency\":\"cookie\",\"result\":\ {\"errors\":{},\"issues\":0,\"results\":{},\"risks\":[]}}]'\nexit 0"; /// The same report with one risk in it, which is a finding at exit 0. @@ -200,6 +200,19 @@ const GUARDDOG_RISK: &str = "echo '[{\"dependency\":\"reqests\",\"result\":\ {\"errors\":{},\"issues\":1,\"results\":{},\"risks\":\ [{\"name\":\"typosquatting\",\"severity\":\"high\"}]}}]'\nexit 0"; +fn clean_dependencies(names: &[&str]) -> String { + let entries: Vec = names + .iter() + .map(|name| { + serde_json::json!({ + "dependency": name, + "result": {"errors": {}, "results": {}, "risks": []} + }) + }) + .collect(); + format!("echo '{}'\nexit 0", serde_json::json!(entries)) +} + fn recording(answer: &str) -> String { format!("echo \"$(basename \"$0\") [$PWD] $*\" >> \"$STUB_LOG\"\n{answer}") } @@ -728,7 +741,7 @@ fn dev_dependencies_are_sent_to_guarddog_where_it_is_told_to_read_them() { .unwrap(); let tools = stubs(&[ ("osv-scanner", "exit 0"), - ("guarddog", &recording(GUARDDOG_CLEAN)), + ("guarddog", &recording(&clean_dependencies(&["jose"]))), ]); let output = supply_with( &root, @@ -909,7 +922,7 @@ fn all_scans_every_manifest_even_where_the_range_holds_none_of_them() { ("osv-scanner", &recording("exit 0")), ("zizmor", &recording("exit 0")), ("uv", &recording("echo 'requests==2.0.0'")), - ("guarddog", &recording(GUARDDOG_CLEAN)), + ("guarddog", &recording(&clean_dependencies(&["requests"]))), ]); let output = supply(&root, Some(&tools)); assert_eq!(code(&output), 0, "{}", text(&output)); @@ -935,7 +948,7 @@ fn a_changed_python_lock_runs_guarddog_in_that_directory_and_not_the_others() { let tools = stubs(&[ ("osv-scanner", &recording("exit 0")), ("uv", &recording("echo 'requests==2.0.0'")), - ("guarddog", &recording(GUARDDOG_CLEAN)), + ("guarddog", &recording(&clean_dependencies(&["requests"]))), ]); let output = pushed(&root, &tools, &before, &after); assert_eq!(code(&output), 0, "{}", text(&output)); @@ -1070,7 +1083,7 @@ fn a_bumped_submodule_pointer_expands_into_the_members_own_manifests() { let tools = stubs(&[ ("osv-scanner", &recording("exit 0")), ("uv", &recording("echo 'requests==2.0.0'")), - ("guarddog", &recording(GUARDDOG_CLEAN)), + ("guarddog", &recording(&clean_dependencies(&["requests"]))), ]); let output = pushed(&root, &tools, &before, &after); assert_eq!(code(&output), 0, "{}", text(&output)); @@ -1104,7 +1117,7 @@ fn a_submodule_commit_the_store_does_not_have_widens_to_every_manifest_under_it( let tools = stubs(&[ ("osv-scanner", &recording("exit 0")), ("uv", &recording("echo 'requests==2.0.0'")), - ("guarddog", &recording(GUARDDOG_CLEAN)), + ("guarddog", &recording(&clean_dependencies(&["requests"]))), ]); let output = pushed(&root, &tools, &before, &after); assert_eq!(code(&output), 0, "{}", text(&output)); @@ -1173,7 +1186,7 @@ fn a_push_from_a_linked_worktree_reads_the_members_range_in_the_member() { let tools = stubs(&[ ("osv-scanner", &recording("exit 0")), ("uv", &recording("echo 'requests==2.0.0'")), - ("guarddog", &recording(GUARDDOG_CLEAN)), + ("guarddog", &recording(&clean_dependencies(&["requests"]))), ]); let mut path = tools.as_os_str().to_owned(); path.push(":/usr/bin:/bin"); @@ -1462,7 +1475,7 @@ fn guarddog_issues_without_risks_are_not_a_finding() { std::fs::create_dir_all(root.join("web")).unwrap(); std::fs::write( root.join("web/package.json"), - "{\"name\": \"web\", \"dependencies\": {\"cookie\": \"^1.1.1\"}}\n", + "{\"name\": \"web\", \"dependencies\": {\"six\": \"^1.1.1\"}}\n", ) .unwrap(); let tools = stubs(&[ @@ -1482,6 +1495,136 @@ fn guarddog_issues_without_risks_are_not_a_finding() { ); } +/// npm skips a dependency whose selector has no matching release. Neither a +/// duplicate entry nor an unrelated package can make up for the missing name. +#[test] +fn guarddog_must_report_every_submitted_npm_dependency() { + let root = repository(); + write( + &root, + "package.json", + r#"{"dependencies":{"cookie":"1.0.0","missing-package":"1.0.0"}}"#, + ); + for names in [ + vec![], + vec!["cookie"], + vec!["cookie", "cookie"], + vec!["cookie", "unrelated"], + ] { + let tools = stubs(&[ + ("osv-scanner", "exit 0"), + ("guarddog", &clean_dependencies(&names)), + ]); + let output = supply(&root, Some(&tools)); + let said = text(&output); + assert_eq!(code(&output), 2, "{names:?}: {said}"); + assert!(said.contains("guarddog omitted"), "{said}"); + assert!(said.contains("missing-package"), "{said}"); + assert!(said.contains(&root.display().to_string()), "{said}"); + assert!(!said.contains("all checks passed"), "{said}"); + } +} + +/// `PyPI`'s parser can return the packages before a failed registry request, +/// dropping both the failed dependency and everything after it. +#[test] +fn guarddog_partial_pypi_reports_name_every_omitted_dependency() { + let root = repository(); + write(&root, "uv.lock", "version = 1\n"); + let tools = stubs(&[ + ("osv-scanner", "exit 0"), + ( + "uv", + "printf '%s\\n' 'requests==2.0.0' 'registry-failed==1.0' 'after-failure==1.0'", + ), + ("guarddog", &clean_dependencies(&["requests"])), + ]); + let output = supply(&root, Some(&tools)); + let said = text(&output); + assert_eq!(code(&output), 2, "{said}"); + assert!( + said.contains("guarddog omitted after-failure, registry-failed"), + "{said}" + ); + assert!(said.contains("submitted pypi dependency"), "{said}"); + assert!(!said.contains("all checks passed"), "{said}"); +} + +/// Alias keys are not the names guarddog reports, and several selectors can +/// merge into one real npm package, including across dependency groups. +#[test] +fn guarddog_coverage_uses_real_npm_alias_names() { + let root = repository(); + write( + &root, + "package.json", + r#"{"dependencies":{"cookie":"1.0.0","renamed":"npm:cookie@1.0.0","scoped":"npm:@scope/package@^2","latest":"npm:@scope/package"},"devDependencies":{"development":"npm:cookie@^1"}}"#, + ); + let tools = stubs(&[ + ("osv-scanner", "exit 0"), + ( + "guarddog", + &clean_dependencies(&["cookie", "@scope/package"]), + ), + ]); + let output = supply_with( + &root, + &tools, + &[( + "GUARDDOG_NPM_INCLUDE_DEV_DEPENDENCIES", + String::from("true"), + )], + ); + assert_eq!(code(&output), 0, "{}", text(&output)); + assert!( + text(&output).contains("all checks passed"), + "{}", + text(&output) + ); +} + +/// Extras, markers and repeated punctuation do not change a `PyPI` identity. +/// Local project lines and comments remain outside the expected registry set. +#[test] +fn guarddog_coverage_normalises_pypi_names_and_ignores_local_requirements() { + let root = repository(); + write(&root, "uv.lock", "version = 1\n"); + let tools = stubs(&[ + ("osv-scanner", "exit 0"), + ( + "uv", + "printf '%s\\n' '# exported' '-e .' 'Example__Package.Name[extra]==1.0 ; python_version >= \"3.9\"' 'example-package-name==1.0'", + ), + ("guarddog", &clean_dependencies(&["EXAMPLE-package_NAME"])), + ]); + let output = supply(&root, Some(&tools)); + assert_eq!(code(&output), 0, "{}", text(&output)); + assert!( + text(&output).contains("all checks passed"), + "{}", + text(&output) + ); +} + +/// A reported risk stays a finding even when another dependency was omitted; +/// the output must also disclose the gap. +#[test] +fn guarddog_risks_survive_a_partial_report() { + let root = repository(); + write( + &root, + "package.json", + r#"{"dependencies":{"reqests":"2.0.0","missing-package":"1.0.0"}}"#, + ); + let tools = stubs(&[("osv-scanner", "exit 0"), ("guarddog", GUARDDOG_RISK)]); + let output = supply(&root, Some(&tools)); + let said = text(&output); + assert_eq!(code(&output), 1, "{said}"); + assert!(said.contains("objected to reqests"), "{said}"); + assert!(said.contains("guarddog omitted missing-package"), "{said}"); + assert!(!said.contains("all checks passed"), "{said}"); +} + /// A report must carry both the checks guarddog ran and its risk list. A /// missing or mistyped field is an incomplete answer, not an empty finding. #[test] @@ -1970,7 +2113,7 @@ fn a_waived_guarddog_finding_is_printed_and_not_held_against_the_run() { std::fs::create_dir_all(root.join("web")).unwrap(); std::fs::write( root.join("web/package.json"), - "{\"name\": \"web\", \"dependencies\": {\"cookie\": \"^1.1.1\"}}\n", + "{\"name\": \"web\", \"dependencies\": {\"swagger-ui-express\": \"5.0.1\"}}\n", ) .unwrap(); let tools = stubs(&[("osv-scanner", "exit 0"), ("guarddog", GUARDDOG_MISMATCH)]); @@ -2006,7 +2149,7 @@ fn a_waiver_for_another_version_does_not_apply_and_is_reported_stale() { std::fs::create_dir_all(root.join("web")).unwrap(); std::fs::write( root.join("web/package.json"), - "{\"name\": \"web\", \"dependencies\": {\"cookie\": \"^1.1.1\"}}\n", + "{\"name\": \"web\", \"dependencies\": {\"swagger-ui-express\": \"5.0.1\"}}\n", ) .unwrap(); let tools = stubs(&[("osv-scanner", "exit 0"), ("guarddog", GUARDDOG_MISMATCH)]); @@ -2203,7 +2346,8 @@ fn exporting(root: &Path, lines: &[&str]) -> PathBuf { &recording(&format!( "grep -q ' @ ' \"$5\" && {{ echo '[{{\"dependency\":\"example-kit\",\ \"result\":{{\"errors\":{{\"download-package\":\"Received status code: 404 \ - from PyPI\"}},\"issues\":0}}}}]'; exit 0; }}\n{GUARDDOG_CLEAN}" + from PyPI\"}},\"issues\":0}}}}]'; exit 0; }}\n{}", + clean_dependencies(&["requests"]) )), ), ]) diff --git a/tests/test_uphold_check.py b/tests/test_uphold_check.py index 57f979f..f7d9045 100644 --- a/tests/test_uphold_check.py +++ b/tests/test_uphold_check.py @@ -385,6 +385,59 @@ def drifted(root, *args): self.assertEqual(stdout.getvalue().strip(), "") +class AnUnavailableEngine(unittest.TestCase): + def test_unavailable_engine_prevents_review_emission_and_oscal(self): + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + (root / "policy").mkdir() + exemptions = [ + f'{key} = "No subject in this scratch repository."' + for key, record in uphold_check.load_records().items() + if record["enforcement"]["automatable"] == "yes" + ] + (root / "policy/upheld.toml").write_text( + "[review]\nmax_lines = 10000\n[review.no_subject_here]\n" + + "\n".join(exemptions) + + "\n", + encoding="utf-8", + ) + for status in (2, 101, -11): + for args in (["--review", "--emit"], ["--oscal"]): + with self.subTest(status=status, args=args): + failed = subprocess.CompletedProcess( + ["uphold", "check"], status, "", "engine unavailable" + ) + output, errors = io.StringIO(), io.StringIO() + with ( + contextlib.chdir(root), + mock.patch.object( + uphold_check, "engine", return_value=failed + ), + contextlib.redirect_stdout(output), + contextlib.redirect_stderr(errors), + ): + code = uphold_check.main(args) + self.assertEqual(code, 2) + self.assertEqual(output.getvalue(), "") + self.assertIn("engine unavailable", errors.getvalue()) + self.assertFalse((root / "REVIEW.md").exists()) + + def test_evaluated_refusal_remains_available_to_review_only(self): + refused = subprocess.CompletedProcess(["uphold", "check"], 1, "", "bad claim") + with mock.patch.object(uphold_check, "engine", return_value=refused): + self.assertEqual(uphold_check.engine_suppliers(ROOT, strict=False), {}) + with self.assertRaises(uphold_check.Refused): + uphold_check.engine_suppliers(ROOT) + + def test_unexpected_status_without_diagnostic_is_identified(self): + failed = subprocess.CompletedProcess(["uphold", "check"], -9, "", "") + with ( + mock.patch.object(uphold_check, "engine", return_value=failed), + self.assertRaisesRegex(uphold_check.CouldNotLook, "status -9"), + ): + uphold_check.engine_suppliers(ROOT, strict=False) + + class AnUnreadableDeclaration(unittest.TestCase): """`explicit-unknown`, claimed by `catalog-tests` in policy/upheld.toml. diff --git a/uphold_check.py b/uphold_check.py index 3a257f7..bb0be62 100755 --- a/uphold_check.py +++ b/uphold_check.py @@ -280,6 +280,11 @@ def engine_suppliers(root: Path, *, strict: bool = True) -> dict[str, list[str]] answered = engine(root, "check") if answered.returncode == 2: raise CouldNotLook(answered.stderr.strip() or "uphold check could not look") + if answered.returncode not in (0, 1): + raise CouldNotLook( + f"uphold check exited unexpectedly with status {answered.returncode}: " + f"{answered.stderr.strip() or 'no diagnostic was reported'}" + ) if answered.returncode != 0 and strict: raise Refused( "the enforcement claims do not reconcile, so there is nothing "