From 419fbc4ed1ec55fca9c5c0e3a6ecfeba7959a32b Mon Sep 17 00:00:00 2001 From: HackingGate Date: Thu, 20 Aug 2026 22:04:57 +0900 Subject: [PATCH 1/3] Read the owner and the visibility from a command, as the owner list already is `owner` and `visibility` are each written once per repository. Across one fleet that is 78 `owner` lines carrying seven distinct values -- 41 copies of one string inside a single organisation -- and a repository that flips its visibility leaves 77 other files saying what they always said. `owner_from` and `visibility_from` are the move `private_owners_from` already made: a command whose stdout is the value, run in the repository root, so a workspace fact is written once outside the tree instead of once per tree. What they are not is a lookup. Deriving the owner from `origin` is the defect rather than the fix -- repointing `origin` at somebody else's remote is the accident `prevent-public-push` exists to catch, and a derived allow-list is repointed by the same command. A command that asks a forge what a repository is today has the same shape one layer over: it hands three guards' one scope condition to a network call that answers nothing on a train and nothing in CI without a token. So what the command reads has to be a value somebody decided. Which is why every way it can fail to answer is exit 2, and there is no `..._optional` beside these. An unreadable private-owner list degrades to a narrower check; an unreadable owner degrades to the tautology above, and an unreadable visibility stands a disclosure guard down. A non-zero exit, an empty answer, a second line and a word that is not a visibility are each refused, and the refusal names the fallback it declined to take. Two refusals at load. `owner` beside `owner_from` is two statements of one fact, free to disagree with nothing here to notice -- the defect the field removes, arriving through the field. And neither may arrive by inheritance: a bundled set or an `inherit.paths` file carrying a command runs it in every inheriting repository on a version bump, with nothing in any of those trees to review, which is the reason `private-names` gives for not shipping `private_owners_from`. The answer is cached for the process and never to disk. The private-name family asks about visibility three times, once per variant, and a workspace answering from an organisation index should pay for that once. A cache outliving the run would be a stale answer with a longer life, which is what a declaration exists to avoid. Refs #54. --- CONTRIBUTING.md | 9 ++ docs/REFERENCE.md | 58 +++++++++ src/config.rs | 269 ++++++++++++++++++++++++++++++++++++++++- src/guard/names.rs | 21 ++-- src/guard/push.rs | 17 ++- tests/base_sets_cli.rs | 242 ++++++++++++++++++++++++++++++++++++ 6 files changed, 600 insertions(+), 16 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index dbf488b..71a1631 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -44,6 +44,15 @@ the guard working, not a misconfiguration. `visibility` decides whether the private-name guards fire here at all. A private fork sets `private` and they stand down; a public one leaves `public`. +A workspace holding many repositories writes both lines many times — measured +across one fleet, 78 `owner` lines for seven distinct values. `owner_from` and +`visibility_from` take a command whose stdout is the value instead, so the fact +lives once outside the tree. They move the declaration and never look it up: +every way the command can fail to answer is exit `2`, because what a missing +declaration falls back to is the owner read off `origin` and the forge's view of +a visibility that is about to change. See +[REFERENCE.md](docs/REFERENCE.md#reading-a-repository-fact-from-a-command). + A third line is about *your machine* rather than your fork. `private_owners_from` reads a file that is one operator's and will not exist in your clone, so the policy also sets `private_owners_optional = true`: the diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 75faea9..d2f46b9 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -672,6 +672,64 @@ private_owners_optional = true # only where this policy is cloned; see below A rule's own field wins where both are written. +### Reading a repository fact from a command + +`owner` and `visibility` are each written once per repository, and across one +fleet that is 78 `owner` lines carrying seven distinct values — 41 copies of one +string inside a single organisation. `owner_from` and `visibility_from` are the +same move `private_owners_from` already makes: a command whose stdout is the +value, run in the repository root, so a workspace fact is written once outside +the tree instead of once per tree. + +```toml +owner_from = "cat ${XDG_CONFIG_HOME:-$HOME/.config}/uphold/owner" +visibility_from = "cat .workspace/visibility-cache" +``` + +**They move a declaration; they do not look one up.** Deriving the owner from +`origin` is the defect rather than the fix — repointing `origin` at somebody +else's remote is the exact accident `prevent-public-push` exists to catch, and a +derived allow-list is repointed by the same command. The same applies to +visibility: a command that asks a forge what a repository is *today* hands three +guards' one scope condition to a network call, which answers nothing on a train, +nothing in CI without a token, and answers about the visibility a repository is +in the middle of changing. What the command reads must be a value somebody +decided, not the state being guarded. + +So every way the command can fail to answer is exit `2`: + +| what the command does | what happens | +|---|---| +| prints one value | that is the declaration, cached for the rest of the process | +| exits non-zero | exit `2`, naming the fallback it refused to take | +| exits `0` printing nothing | exit `2` — the file reads as a declaration and there is none | +| prints more than one line | exit `2` — one repository, one fact, and no first-line guess | +| prints a word that is not a visibility | exit `2`, naming the command rather than the file | + +There is deliberately **no `..._optional`** beside these, and the asymmetry with +`private_owners_from` is the point. An unreadable owner list degrades to a +narrower check, which can be reported and lived with. An unreadable owner +degrades to the tautology above, and an unreadable visibility degrades to +standing a disclosure guard down — neither is a degradation anybody should be +able to opt into. + +Two further refusals, both at load: + +- **`owner` beside `owner_from`** (or `visibility` beside `visibility_from`) is + refused. They are two statements of one fact, free to disagree, with nothing + anywhere to notice when they do — which is the defect the field exists to + remove, arriving through the field. +- **Neither may arrive by inheritance.** A bundled set or an `inherit.paths` + file carrying one is refused, for the reason `private-names` gives for not + shipping `private_owners_from`: a command arriving that way runs in every + inheriting repository on the strength of a version bump, with nothing in any + of those trees to review. + +The command runs at most once per process, on the first ask — the private-name +family asks about visibility three times, once per variant — and the answer is +never cached to disk. A declaration exists to avoid a stale answer, and a cache +outliving the run is a stale answer with a longer life. + **Why the owner list is worth declaring**, measured rather than asserted. A forge lookup only *adjudicates* names something already extracted, and a bare `owner/repo` is extracted only for declared owners and for this repository's diff --git a/src/config.rs b/src/config.rs index ff01d99..8286cdb 100644 --- a/src/config.rs +++ b/src/config.rs @@ -15,6 +15,8 @@ use std::collections::{BTreeMap, BTreeSet}; use std::path::{Path, PathBuf}; +use std::process::Command; +use std::sync::OnceLock; use serde::{Deserialize, Serialize}; @@ -801,7 +803,7 @@ impl Rule { /// The search scoping, or ripgrep's defaults where the table is absent. pub(crate) fn files(&self) -> &Files { - static DEFAULTS: std::sync::OnceLock = std::sync::OnceLock::new(); + static DEFAULTS: OnceLock = OnceLock::new(); self.files .as_ref() .map_or_else(|| DEFAULTS.get_or_init(Files::default), |files| files) @@ -1282,6 +1284,45 @@ pub(crate) struct PolicyFile { /// `guard::names::target_is_public`. #[serde(default)] pub visibility: Option, + /// A command whose stdout is this repository's owner. + /// + /// The duplication is measured rather than asserted: across one fleet, 78 + /// policy files declare an `owner` for seven distinct values -- 41 copies of + /// one string inside a single organisation. That is a workspace fact + /// transcribed once per repository, and it is the same shape + /// [`PolicyFile::private_owners_from`] already answered by reading from + /// outside the tree. + /// + /// It is NOT permission to derive the owner from `origin`. Deriving it is + /// the defect rather than the fix -- repointing `origin` at somebody else's + /// remote is the exact accident `prevent-public-push` exists to catch, and a + /// derived allow-list is repointed by the same command. This field moves a + /// DECLARATION out of the tree; it never reads one off the thing being + /// guarded. + /// + /// Refused beside a literal `owner`, and refused in a bundled set or an + /// inherited file. See [`refuse_two_statements_of_one_fact`] and + /// [`refuse_inherited_declaration`]. + #[serde(default)] + pub owner_from: Option, + /// A command whose stdout is this repository's visibility. + /// + /// Same mechanism as [`PolicyFile::owner_from`], for a host the built-in + /// lookup does not speak to or a workspace that would rather answer from a + /// cached organisation index than a request per repository. The word it + /// prints is held to `public`, `private` or `internal` exactly as a written + /// one is. + /// + /// What it must not become is a probe. This is a declaration read from + /// somewhere else, and a command that asks a forge what a repository is + /// TODAY hands the private-name family's one scope condition to a network + /// call -- which answers nothing on a train, nothing in CI without a token, + /// and answers about the visibility a repository is in the middle of + /// changing. So a command that cannot answer must fail, and failing is exit + /// 2: a quiet `private` would stand the whole family down, which is + /// fail-open on the one rule family where fail-open is unacceptable. + #[serde(default)] + pub visibility_from: Option, /// A command whose output lists the owners this workspace treats as /// private, declared once for the whole policy. /// @@ -1375,6 +1416,27 @@ pub(crate) struct Policy { /// What this repository's publications are visible to, from the policy /// file's own `visibility`. See [`PolicyFile::visibility`]. pub visibility: Option, + /// Where the owner is declared when it is not written down, from the policy + /// file's own `owner_from`. See [`PolicyFile::owner_from`]. + pub owner_from: Option, + /// Where the visibility is declared when it is not written down, from the + /// policy file's own `visibility_from`. See [`PolicyFile::visibility_from`]. + pub visibility_from: Option, + /// What `owner_from` answered, so it is asked at most once per process. + /// + /// Two slots and not one map keyed by field name, deliberately: these are + /// two facts behind two commands, and a single cache filled in one pass + /// would run the visibility command for a caller that only asked who this + /// repository belongs to. The resolution itself IS parameterised -- one + /// [`declared`] reads either -- so what is duplicated here is storage, not a + /// unit of behaviour. + /// + /// Per process rather than on disk. A declaration exists to avoid a stale + /// answer, and a cache that outlives the run is a stale answer with a + /// longer life. + pub resolved_owner: OnceLock, + /// What `visibility_from` answered. See [`Policy::resolved_owner`]. + pub resolved_visibility: OnceLock, /// Where the private-owner list comes from, from the policy file's own /// `private_owners_from`. See [`PolicyFile::private_owners_from`]. pub private_owners_from: Option, @@ -1395,6 +1457,55 @@ pub(crate) struct Policy { } impl Policy { + /// Who this repository belongs to, as this policy declares it. + /// + /// The written `owner` if there is one, else whatever `owner_from` answers, + /// else nothing -- and "nothing" is what makes a caller fall back to the + /// remote, which every caller of this is careful to say out loud. + /// + /// A rule's own `owner` is NOT consulted here. It is narrower than the + /// policy's on purpose, so the rule reads it first and reaches this only + /// when it has none. + pub(crate) fn declared_owner(&self, root: &Path) -> Result> { + declared( + root, + "owner", + self.owner.as_deref(), + self.owner_from.as_deref(), + &self.resolved_owner, + ) + } + + /// What this repository's publications are visible to, as this policy + /// declares it, held to the three spellings whichever way it arrived. + /// + /// A written `visibility` was already held to them at load, where a typo is + /// a diff somebody can fix. A command's answer cannot be checked then -- + /// there is no answer until it runs -- so it is checked here, and the + /// refusal names the command rather than the file, because the file is + /// right and the thing it points at is not. + pub(crate) fn declared_visibility(&self, root: &Path) -> Result> { + let value = declared( + root, + "visibility", + self.visibility.as_deref(), + self.visibility_from.as_deref(), + &self.resolved_visibility, + )?; + if let Some(word) = value.as_deref() { + if visibility_is_public(word).is_none() { + return Err(Fatal::new(format!( + "`visibility_from` answered {word:?}, which is not a visibility. The \ + command must print \"public\", \"private\" or \"internal\" -- the word \ + decides whether the guards that fire only on a published tree fire here \ + at all, so there is no reading of an unrecognised one that is safe to \ + guess at." + ))); + } + } + Ok(value) + } + /// The bundled set a rule id arrived from, for the one line a reader sees. /// /// By id, because a refusal carries the id and ids are one namespace -- @@ -1471,6 +1582,76 @@ fn parse(path: &Path, text: &str) -> Result { /// be a permission granted to the author by the author, which is not a /// permission. Refused rather than ignored: a field read by nothing is the /// shape this schema exists to make unrepresentable. +/// Refuse a file that states one repository fact twice. +/// +/// The whole argument for reading a declaration from outside the tree is that +/// the fact has ONE place it is written. A file carrying `owner` and +/// `owner_from` has kept the copy the field exists to remove, and nothing +/// anywhere reconciles the two -- so the day they disagree is a day one of them +/// is silently wrong and the guard reading it cannot tell. +/// +/// At load, like the visibility spelling above it, because it is a fact about +/// the file rather than about any run of any hook. +fn refuse_two_statements_of_one_fact(path: &Path, file: &PolicyFile) -> Result<()> { + for (field, literal, from) in [ + ("owner", file.owner.is_some(), file.owner_from.is_some()), + ( + "visibility", + file.visibility.is_some(), + file.visibility_from.is_some(), + ), + ] { + if literal && from { + return Err(Fatal::at( + path, + format!( + "`{field}` and `{field}_from` are both declared, and they are two \ + statements of one fact -- free to disagree, with nothing here to notice \ + when they do. Keep the one that is true of every checkout of this \ + repository: `{field}_from` where the value belongs to the workspace, \ + `{field}` where it belongs to the repository. Delete the other" + ), + )); + } + } + Ok(()) +} + +/// Refuse a command that speaks for a repository from a file that repository +/// did not write. +/// +/// `owner_from` and `visibility_from` run a shell command. A bundled set +/// carrying one would run it in every inheriting repository on the strength of a +/// version bump, with nothing in any of those trees to review -- which is the +/// reason `private-names` already gives for not shipping `private_owners_from`, +/// and it is not weakened by the command being one line shorter. +/// +/// Refused rather than dropped. `owner` and `visibility` in an inherited file +/// are read by nothing and say nothing about it, which is the shape this +/// repository refuses everywhere else; those two are load-bearing in trees that +/// already exist, and these two are new and can start correct. +fn refuse_inherited_declaration(path: &Path, file: &PolicyFile, kind: &str) -> Result<()> { + for field in ["owner_from", "visibility_from"] { + let declared = match field { + "owner_from" => file.owner_from.is_some(), + _ => file.visibility_from.is_some(), + }; + if declared { + return Err(Fatal::at( + path, + format!( + "`{field}` runs a shell command and this file is {kind}. A command \ + arriving that way runs in every repository that inherits it on the \ + strength of a version bump, with nothing in any of those trees to \ + review -- which is why a set does not ship `private_owners_from` \ + either. Write the line in the repository's own policy file" + ), + )); + } + } + Ok(()) +} + fn refuse_set_header(path: &Path, file: &PolicyFile) -> Result<()> { if file.set.is_some() { return Err(Fatal::at( @@ -1491,6 +1672,7 @@ fn refuse_set_header(path: &Path, file: &PolicyFile) -> Result<()> { fn parse_bundled(name: &str, text: &str) -> Result { let path = Path::new("").join(format!("{name}.toml")); let file = parse(&path, text)?; + refuse_inherited_declaration(&path, &file, "a bundled set")?; let allowed = file.set.clone().unwrap_or_default().stages; for rule in file.rules.values() { for hook in rule.hooks() { @@ -1518,6 +1700,85 @@ fn parse_bundled(name: &str, text: &str) -> Result { Ok(file) } +/// One repository-level fact, read from a command instead of written down. +/// +/// Shared by `owner_from` and `visibility_from` because those differ only in +/// which fact they carry. One function, so the trimming, the refusal of a second +/// line and the words a failure prints are stated once and cannot drift into two +/// slightly different contracts. +/// +/// There is deliberately no `..._optional` escape hatch here, and the asymmetry +/// with `private_owners_from` is the point. An unreadable private-owner list +/// degrades to a NARROWER check, which can be reported and lived with. An +/// unreadable owner degrades to the owner read off `origin` -- the tautology +/// `prevent-public-push` exists to refuse -- and an unreadable visibility +/// degrades to the forge's answer about a visibility that is about to change. +/// Neither of those is a degradation anybody should be able to opt into. +fn read_declaration(root: &Path, field: &str, command: &str) -> Result { + let output = Command::new("sh") + .arg("-c") + .arg(command) + .current_dir(root) + .output() + .map_err(|error| Fatal::new(format!("`{field}`: could not run {command:?}: {error}")))?; + if !output.status.success() { + return Err(Fatal::new(format!( + "`{field}` ran {command:?}, which exited {}: {}\n\nA source that failed declared \ + nothing, and what a missing declaration falls back to is the thing this field \ + exists to replace: the owner read off `origin`, or the forge's view of a \ + visibility that is about to change. Fix the source, or write the value down.", + output.status.code().unwrap_or(-1), + String::from_utf8_lossy(&output.stderr).trim() + ))); + } + let text = String::from_utf8_lossy(&output.stdout); + let mut values = text.lines().map(str::trim).filter(|line| !line.is_empty()); + let Some(value) = values.next() else { + return Err(Fatal::new(format!( + "`{field}` ran {command:?}, which exited 0 and printed nothing. Silence is not an \ + answer here: it would leave this repository having declared nothing while the \ + policy file reads as though it had declared something." + ))); + }; + if values.next().is_some() { + return Err(Fatal::new(format!( + "`{field}` ran {command:?}, which printed more than one line. This is one fact \ + about one repository, and taking the first line would pin the repository to \ + whatever the command happened to print first. Narrow it to the single value." + ))); + } + Ok(value.to_owned()) +} + +/// A declared fact: the written value, else the command's answer, else nothing. +/// +/// The command runs on the first ask and never again, because the guards ask +/// more than once -- the private-name family asks about visibility three times, +/// once per variant -- and a workspace that answers with an organisation index +/// is paying for a forge round trip each time. +fn declared( + root: &Path, + field: &str, + literal: Option<&str>, + from: Option<&str>, + cache: &OnceLock, +) -> Result> { + if let Some(value) = literal { + return Ok(Some(value.to_owned())); + } + let Some(command) = from else { + return Ok(None); + }; + if let Some(cached) = cache.get() { + return Ok(Some(cached.clone())); + } + let value = read_declaration(root, &format!("{field}_from"), command)?; + // `set` can only lose to a caller that filled the slot first, and that + // caller ran the same command against the same tree, so the loser's answer + // is the winner's answer. + Ok(Some(cache.get_or_init(|| value).clone())) +} + /// Is a declared visibility one that means "everyone can read this"? /// /// `None` where the word is not a visibility at all. One function because the @@ -1537,6 +1798,7 @@ pub(crate) fn load(root: &Path, policy_path: &Path) -> Result { let text = read_to_string(policy_path)?; let file = parse(policy_path, &text)?; refuse_set_header(policy_path, &file)?; + refuse_two_statements_of_one_fact(policy_path, &file)?; // Checked here rather than where a guard reads it. A misspelt visibility is // a fact about the file, and hearing about it when a hook fires means // hearing about it from whichever seam happened to run first, months after @@ -1587,6 +1849,7 @@ pub(crate) fn load(root: &Path, policy_path: &Path) -> Result { let extended = read_to_string(&path)?; let parsed = parse(&path, &extended)?; refuse_set_header(&path, &parsed)?; + refuse_inherited_declaration(&path, &parsed, "an inherited file")?; // Refused rather than merged, and refused rather than ignored. Only // `.rules` is merged below, so an inherited `[[shim]]` used to vanish -- // and vanish in the worst possible way, because the `exec` rule that @@ -1688,6 +1951,10 @@ pub(crate) fn load(root: &Path, policy_path: &Path) -> Result { path: policy_path.to_path_buf(), owner: file.owner.clone(), visibility: file.visibility.clone(), + owner_from: file.owner_from.clone(), + visibility_from: file.visibility_from.clone(), + resolved_owner: OnceLock::new(), + resolved_visibility: OnceLock::new(), private_owners_from: file.private_owners_from.clone(), private_owners_optional: file.private_owners_optional, redact_matches: file.redact_matches, diff --git a/src/guard/names.rs b/src/guard/names.rs index 94d90e1..9095f1c 100644 --- a/src/guard/names.rs +++ b/src/guard/names.rs @@ -361,11 +361,12 @@ fn target_is_public(root: &Path, policy: &Policy, rule: &Rule) -> Result, sources: &[(String, String)]) -> Result, sources: &[(String, String)]) -> Result) -> (Option, bool) { +fn workspace_owner(request: &Request<'_>) -> Result<(Option, bool)> { if let Some(owner) = request.rule.owner.as_deref() { - return (Some(owner.to_owned()), true); + return Ok((Some(owner.to_owned()), true)); } // The policy's own `owner`, which is the pin a rule arriving from a bundled // set can still reach: a set cannot be handed a parameter without writing // the rule out again, and identity is a property of the repository rather // than of any one rule in it. - if let Some(owner) = request.policy.owner.as_deref() { - return (Some(owner.to_owned()), true); + if let Some(owner) = request.policy.declared_owner(request.root)? { + return Ok((Some(owner), true)); } let derived = git::remote_url(request.root, "origin") .and_then(|url| git::owner_repo(&url)) .map(|(owner, _)| owner); - (derived, false) + Ok((derived, false)) } /// Where this push is actually going. @@ -66,7 +71,7 @@ pub(crate) fn prevent_public_push(request: &Request<'_>) -> Result Date: Thu, 20 Aug 2026 22:17:02 +0900 Subject: [PATCH 2/3] Check the declared visibility against the forge, in the one direction a probe can `visibility` is written into a policy file and read by three guards as the condition they fire under. The forge owns the fact, the file holds a copy, and nothing reconciles the two -- so a repository flipped to public goes on being judged by a file that says `private`, with the private-name guards standing down over a tree everybody can read. Across one fleet, 78 policies declare a visibility and nothing anywhere would notice if one of them stopped being true. `no-stale-visibility` refuses exactly one state: declared private, served public. It is a falsifier rather than a resolver, and building it the other way is what would make it dangerous. No probe can prove a repository is PRIVATE -- a 404 is a private repository, a deleted one, a renamed one, and a request that carried no credentials -- while every probe can disprove it, and that is the direction that leaks. So nothing is read back into the declaration. A forge that did not answer is exit 2, never a downgrade to "confirmed private": if a failed lookup could settle it, an offline laptop would flip the guards to `private` and disarm a disclosure check in silence, which is fail-open on the one family where fail-open is unacceptable. A policy declaring `public` has no claim of privacy to disprove, makes no request, and says so. It ships as a set of its own rather than joining `private-names`, for the reason `stale-pins` is a name of its own: it reaches a forge, so its verdict depends on where the machine running it is standing, and a repository that cannot live with a network-dependent gate should be able to take the family without it. The stages are `stale-pins`'s too -- pre-push and manual, never pre-commit, because a guard that adds a round trip to every commit is one somebody comments out. It asks through the same `gh` lookup the private-name family uses, and deliberately not a second mechanism. Two ways of asking one question are two answers free to disagree, and this rule's exit-state ranking rests on the distinction that lookup already draws between "the forge will not show us this" and "the forge would not talk to us". The test helper now puts the real git on the fixture's PATH. A `git` shim loads the policy of the tree it is invoked in and fails closed when that policy will not load, so on a machine with one installed the shim rather than the guard was deciding what `git remote get-url` answered in a fixture -- which made a guard that reads `origin` a test of the installed binary instead of this one. Closes #54. --- docs/REFERENCE.md | 42 ++++++- policy/base/sets.lock.json | 19 +++ policy/base/stale-visibility.toml | 62 ++++++++++ src/config.rs | 9 ++ src/guard/mod.rs | 13 ++- src/guard/names.rs | 15 ++- src/guard/visibility.rs | 138 ++++++++++++++++++++++ tests/base_sets_cli.rs | 184 +++++++++++++++++++++++++++++- 8 files changed, 467 insertions(+), 15 deletions(-) create mode 100644 policy/base/stale-visibility.toml create mode 100644 src/guard/visibility.rs diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index d2f46b9..cac6523 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -120,7 +120,7 @@ files.glob = ["*.yml", "*.yaml"] `inherit.sets` names bundled sets to inherit; it does not add settings. There is no `true` shorthand — naming the sets is cheap, and what a repository -inherits should be written in the repository. Fifteen are compiled into the +inherits should be written in the repository. Sixteen are compiled into the binary and mirrored in [`policy/base/`](../policy/base), each **named by what it refuses** so the name predicts the rule list: @@ -141,8 +141,9 @@ it refuses** so the name predicts the rule list: | `stale-pins` | a hook pinned at a revision its upstream has left, or at none — **installs `pre-push` and `manual`**, and reaches the network | | `unowned-push` | a push to an owner this repository has not named — **installs `pre-push`**, and refuses to run until the repository says who it is | | `private-names` | a private organisation or repository named in a commit message, a staged diff, or the tracked tree of a **public** repository — **installs five stages**, and refuses to run until the repository says whether it is published | +| `stale-visibility` | a policy declaring `private` over a repository the forge serves as **public** — **installs `pre-push` and `manual`**, and reaches the network. Refuses that one direction only; it can never confirm privacy | -The last six install git hooks. Taking one is a decision about what will be +The last seven install git hooks. Taking one is a decision about what will be refused and when, so each is named and argued separately: `stale-pins` reaches the network and cannot answer on a train, `invisible-characters` reads the tree at four stages and is the slowest thing in a hook, `unreviewed-history` stands @@ -175,6 +176,36 @@ with no network, and answers about the visibility a repository has *today*, which is the thing that changes on the day it matters. `visibility = "private"` is a real answer, not an opt-out: it says the condition does not hold here. +**What then checks the declaration** is `stale-visibility`, and it is a separate +set because it reaches the network. Declaring the visibility makes the guards +offline and deterministic, and it also makes the file a **cache with no +reconcile**: flip a repository to public and the policy goes on saying `private` +forever, with the three private-name guards standing down over a tree everybody +can read. + +The rule refuses **one direction and only one**, because that is the only +direction a probe can establish. No probe can prove a repository is private — a +`404` is a private repository, a deleted one, a renamed one, and a request that +carried no credentials — while any probe can disprove it, and disproving it is +what catches the leak. + +| declared | what the forge says | outcome | +|---|---|---| +| `public` | not asked | passes, and says no request was made | +| `private` / `internal` | `public` | **refused**, naming the flip | +| `private` / `internal` | `private` / `internal` | passes | +| `private` / `internal` | nothing it could be asked | **exit `2`** — never "confirmed private" | +| nothing declared | not asked | **exit `2`** — no claim to check | + +The last two rows are the design. If a failed lookup could settle the answer, +an offline laptop would flip the guards to `private` and disarm a disclosure +check in silence, which is fail-open on the one family where fail-open is +unacceptable. The declaration stays the input; the probe only ever refuses. + +It installs at `pre-push` and `manual` and never at `pre-commit`, for the reason +`stale-pins` gives: a guard that adds a network round trip to every commit is +one somebody comments out. + **Two different "unknowns", and only one of them is `refuse_unknown`'s.** A forge that *answers* `404` has told you something about the name: no repository it will show you is called that. A forge that could not be asked — no `gh`, no @@ -600,6 +631,7 @@ stamped on it, the range about to be pushed. | `no-merge-commit` | a commit finishing a merge or a squash merge | | `no-stale-hook-pins` | a pin left behind its upstream, or naming no ref — in `.pre-commit-config.yaml` **and** lefthook `remotes:`, at any depth in the tree; a pin it **could not check** is exit `2` | | `no-hand-copied-base-rule` | a rule this policy writes out by hand under an id a bundled set already ships, from a set it does not inherit. Reads the **policy**, not the tree. At `pre-commit` only what the change adds; at `manual` the whole sweep | +| `no-stale-visibility` | a declared `private` the forge no longer serves. Reads the **declaration** and the forge, not the tree; a forge that did not answer is exit `2` and never "confirmed private" | Declared like any other rule, in the same file and the same id namespace. **`git.hooks` is the whole registration.** @@ -645,7 +677,7 @@ enforced and is not. | `owner` | `prevent-public-push` | the owner this workspace is pinned to | | `allowed_owners` | `prevent-public-push` | owners a push may go to; defaults to the pinned owner | | `allowed_repos` | `prevent-public-push` | single repositories allowed through, `"owner/repo"` | -| `visibility` | the `no-private-repo-names` family | this repository's visibility, declared instead of looked up | +| `visibility` | the `no-private-repo-names` family, `no-stale-visibility` | this repository's visibility, declared instead of looked up | | `visibility_required` | the `no-private-repo-names` family | exit `2` rather than fall back to the forge when nothing has declared a visibility | | `private_owners` | the `no-private-repo-names` family | owners whose repositories are private regardless of what a forge says | | `private_owners_from` | the `no-private-repo-names` family | a command whose stdout is one private owner per line | @@ -654,7 +686,9 @@ enforced and is not. | `allow` | `prevent-unusual-unicode-in-files` | codepoints admitted, optionally under one glob — `"U+00A0:docs/captured/**"` | The "family" is `no-private-repo-names`, `-staged` and `-in-files`. No other -built-in reads any parameter. +built-in reads any parameter. `no-stale-visibility` reads `visibility` and +nothing else — everything else in that row is about judging names in text, which +the falsifier never does. **Three of these are also top-level policy fields**, and that is not a convenience. A rule arriving from a bundled set cannot be handed a parameter — diff --git a/policy/base/sets.lock.json b/policy/base/sets.lock.json index 7afeac3..21a4853 100644 --- a/policy/base/sets.lock.json +++ b/policy/base/sets.lock.json @@ -591,6 +591,25 @@ "manual" ] }, + { + "rules": [ + { + "builtin": "no-stale-visibility", + "git": { + "hooks": [ + "pre-push", + "manual" + ] + }, + "id": "no-stale-visibility" + } + ], + "set": "stale-visibility", + "stages": [ + "pre-push", + "manual" + ] + }, { "rules": [ { diff --git a/policy/base/stale-visibility.toml b/policy/base/stale-visibility.toml new file mode 100644 index 0000000..0806725 --- /dev/null +++ b/policy/base/stale-visibility.toml @@ -0,0 +1,62 @@ +# Base rule set: stale-visibility (a policy still claiming a privacy the forge +# has stopped serving) +# +# The guard that asks the forge whether this repository is public, and refuses +# when the policy says it is not. Pull in with: +# +# [inherit] +# sets = ["private-names", "stale-visibility"] +# +# WHY IT EXISTS. `visibility` is declared in the policy file and read by the +# three `private-names` guards as the condition they fire under. The forge owns +# that fact; the file holds a copy; nothing reconciles the two. A repository +# flipped from private to public goes on being judged by a file that says +# `private`, and the guards that would have caught a leak stand down over a +# repository everybody can read. Measured across one fleet: 78 policies declare +# a visibility, and nothing anywhere would notice if one of them stopped being +# true. +# +# IT DISPROVES AND NEVER CONFIRMS, AND THAT IS THE DESIGN. No probe can +# establish that a repository is PRIVATE -- a 404 is a private repository, a +# deleted one, a renamed one, and a request that carried no credentials, and a +# public index by construction lists only public repositories. Every probe can +# disprove privacy, and that is the direction that leaks. So this rule refuses +# exactly one state, declared private against a forge serving public, and reads +# nothing at all back into the declaration. +# +# The declaration therefore stays the input. A rule that let a failed lookup +# decide would let an offline laptop flip the guards to `private` and disarm a +# disclosure check in silence, which is fail-open on the one family where +# fail-open is unacceptable. Could not look is exit 2, never "confirmed +# private". +# +# A POLICY DECLARING `public` IS CLEAN WITHOUT A REQUEST. There is no claim of +# privacy to disprove and the private-name family is already at its strictest, +# so the rule says so and makes no network call. That is most repositories, and +# it is why the set costs them nothing. +# +# SEPARATE FROM `private-names`, AND THE REASON IS THE NETWORK -- the same +# reason `stale-pins` is a name of its own. This is the only other bundled rule +# that talks to a forge, so its verdict depends on where the machine running it +# is standing: on a train, in a runner with no credentials, behind a proxy, the +# question cannot be answered at all, and it answers exit 2 there rather than 0. +# A repository that cannot live with a network-dependent gate should inherit +# `private-names` and not this. +# +# THE STAGES ARE `stale-pins`'s, for the reason written there. `pre-push` is the +# last local moment before work is shared, and `manual` is the sweep. It is off +# `pre-commit` on purpose: a guard that adds a network round trip to every +# commit is one somebody comments out, and a check that gets switched off +# protects nothing. +# +# WHAT IT CANNOT SEE. It asks about `origin`, so a repository pushed somewhere +# `origin` does not name is not the repository being checked -- which is the +# same limit `prevent-public-push` has and refuses to derive its way around. +# It asks through `gh`, so a forge `gh` does not speak to answers Unavailable +# and the run exits 2 rather than passing. +[set] +stages = ["pre-push", "manual"] + +[rule.no-stale-visibility] +builtin = "no-stale-visibility" +git.hooks = ["pre-push", "manual"] diff --git a/src/config.rs b/src/config.rs index 8286cdb..b58338b 100644 --- a/src/config.rs +++ b/src/config.rs @@ -99,6 +99,15 @@ pub(crate) const BUNDLED: &[(&str, &str)] = &[ include_str!("../policy/base/invisible-characters.toml"), ), ("stale-pins", include_str!("../policy/base/stale-pins.toml")), + // The second network set, and separate from `private-names` for the reason + // `stale-pins` is separate from everything: a verdict that depends on + // where the machine running it is standing is a decision a repository + // takes on its own, not one it acquires by inheriting the family whose + // scope condition this checks. + ( + "stale-visibility", + include_str!("../policy/base/stale-visibility.toml"), + ), ( "unowned-push", include_str!("../policy/base/unowned-push.toml"), diff --git a/src/guard/mod.rs b/src/guard/mod.rs index bc8b558..79875c0 100644 --- a/src/guard/mod.rs +++ b/src/guard/mod.rs @@ -21,6 +21,7 @@ pub(crate) mod push; pub(crate) mod scope; pub(crate) mod sets; pub(crate) mod unicode; +pub(crate) mod visibility; use std::path::Path; @@ -50,6 +51,11 @@ pub(crate) const EVERY_BUILTIN: &[&str] = &[ "no-local-merge", "no-merge-commit", "no-stale-hook-pins", + // The only other built-in that reaches a network, and the only one whose + // subject is a claim the POLICY makes about the repository rather than + // about a file in it. It refuses one direction -- declared private, served + // public -- because that is the only direction a probe can establish. + "no-stale-visibility", // Reads the POLICY rather than the tree or what git is about to do: the // only check here whose subject is the repository's own declarations. See // `sets` for why that is a check at all. @@ -298,6 +304,7 @@ pub(crate) fn evaluate(request: &Request<'_>) -> Result> { "no-private-repo-names-in-files" => names::in_tracked(request), "prevent-public-push" => push::prevent_public_push(request), "no-stale-hook-pins" => crate::pins::stale(request), + "no-stale-visibility" => visibility::no_stale_visibility(request), "no-hand-copied-base-rule" => sets::no_hand_copied_base_rule(request), other => Err(Fatal::new(format!("no built-in called {other:?}"))), } @@ -323,6 +330,10 @@ pub(crate) fn parameters(builtin: &str) -> &'static [&'static str] { "public_repos", "refuse_unknown", ], + // `visibility` and nothing else: the rule reads the declaration and + // compares it to the forge, and every other field in the family is + // about judging names in text, which this one never does. + "no-stale-visibility" => &["visibility"], "prevent-unusual-unicode-in-files" => &["allow"], _ => &[], } @@ -426,7 +437,7 @@ mod tests { "{id} is in EVERY_BUILTIN with no dispatch arm" ); } - assert_eq!(EVERY_BUILTIN.len(), 14); + assert_eq!(EVERY_BUILTIN.len(), 15); } #[test] diff --git a/src/guard/names.rs b/src/guard/names.rs index 9095f1c..2a6a3b5 100644 --- a/src/guard/names.rs +++ b/src/guard/names.rs @@ -58,8 +58,13 @@ fn clean_repo(name: &str) -> String { .to_owned() } +/// Shared with `guard::visibility`, which asks the same forge the same question +/// about this repository's own name. One mechanism rather than two: two ways of +/// asking whether a repository is public are two answers free to disagree, and +/// the falsifier's exit-state ranking rests on the distinction drawn below +/// between an answer and no answer. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum Visibility { +pub(crate) enum Visibility { Public, Private, /// The forge ANSWERED, and no repository it will show us has this name. @@ -88,9 +93,9 @@ enum Visibility { /// canonical name is how that is told apart from a genuine sibling: same /// repository, not a leak. `None` when there was no answer to be canonical. #[derive(Debug, Clone, PartialEq, Eq)] -struct Resolved { - visibility: Visibility, - canonical: Option, +pub(crate) struct Resolved { + pub visibility: Visibility, + pub canonical: Option, } /// Any `host.tld/owner/repo` or its scp-like `host.tld:owner/repo`, with the @@ -281,7 +286,7 @@ impl OwnerMatchers { /// Both fields come back from one request. `full_name` is what the forge /// redirected to, which is the only way to notice that the name asked about and /// the repository doing the asking are the same repository under two names. -fn lookup(cache: &mut BTreeMap, owner: &str, repo: &str) -> Resolved { +pub(crate) fn lookup(cache: &mut BTreeMap, owner: &str, repo: &str) -> Resolved { let key = format!("{owner}/{repo}"); if let Some(known) = cache.get(&key) { return known.clone(); diff --git a/src/guard/visibility.rs b/src/guard/visibility.rs new file mode 100644 index 0000000..25a1938 --- /dev/null +++ b/src/guard/visibility.rs @@ -0,0 +1,138 @@ +//! Whether the visibility a policy DECLARES is still the one the forge serves. +//! +//! `visibility` is written into a policy file and read by three guards as the +//! condition they fire under. The forge owns the fact; the file holds a copy; +//! nothing reconciles the two, so a repository flipped from private to public +//! goes on being judged by a file that says `private` forever. Measured across +//! one fleet: 78 policies declare a visibility and nothing anywhere would notice +//! if one of them stopped being true. +//! +//! THIS IS A FALSIFIER AND NOT A RESOLVER, which is the whole design. +//! +//! No probe can prove a repository is PRIVATE. A 404 is private, deleted, +//! renamed, mistyped, or a token that was never presented, and a public index +//! by construction lists only public repositories. Every probe can DISPROVE it, +//! and that is the direction that leaks: the repository is actually public, the +//! policy says `private`, and the three `private-names` guards stand down. So +//! this rule refuses one direction and reads nothing back into the declaration. +//! +//! The declaration stays the input. If a failed lookup could flip the guards to +//! `private`, an offline laptop would silently disarm a disclosure guard, which +//! is fail-open on the one rule family where fail-open is unacceptable. Could +//! not look is exit 2 here, never a downgrade to "confirmed private". +//! +//! WHERE IT RUNS. `pre-push` and `manual`, exactly as `no-stale-hook-pins` is, +//! and for the identical reason written there: a guard that adds a network round +//! trip to every commit is one somebody comments out. No commit pays for this. + +use std::collections::BTreeMap; + +use super::names::{lookup, Visibility}; +use super::{Refusal, Request}; +use crate::config::visibility_is_public; +use crate::error::{Fatal, Result}; +use crate::git; + +/// Refuse a declared privacy the forge has stopped serving. +/// +/// `Ok(None)` for the two clean states -- a policy declaring `public`, which has +/// no claim of privacy to disprove, and a forge that agrees the repository is +/// private. `Ok(Some(_))` for the one refusable state. Everything else is +/// `Err`, because a claim that could not be checked has never been a claim that +/// passed. +pub(crate) fn no_stale_visibility(request: &Request<'_>) -> Result> { + let id = &request.rule.id; + + // The declaration is the subject. A rule that checks a claim against the + // world needs a claim, and with none there is nothing to falsify -- so this + // says so rather than passing, which would be a check that looked like it + // ran and examined nothing. + let declared = match request.rule.visibility.as_deref() { + Some(word) => word.to_owned(), + None => match request.policy.declared_visibility(request.root)? { + Some(word) => word, + None => { + return Err(Fatal::new(format!( + "{id}: nothing here declares this repository's visibility, so there is no \ + claim for this rule to check. Declare it once, at the top of the policy \ + file:\n\n visibility = \"private\" # or \"public\", or \"internal\"\n\n\ + or point `visibility_from` at a command that prints one word." + ))) + } + }, + }; + let Some(declared_public) = visibility_is_public(&declared) else { + return Err(Fatal::new(format!( + "{id}: visibility {declared:?} is not a visibility" + ))); + }; + + // Declared public: there is nothing here to falsify. The direction that + // leaks is a declaration of `private` over a repository that is public, and + // a policy already saying `public` has the private-name family at its + // strictest. Said out loud rather than returned silently, because a rule + // that reports nothing is indistinguishable from one that did not run. + if declared_public { + println!( + "{id}: declared {declared:?}, so there is no claim of privacy to disprove and no \ + request was made." + ); + return Ok(None); + } + + let Some(url) = git::remote_url(request.root, "origin") else { + return Err(Fatal::new(format!( + "{id}: this repository has no `origin` remote, so there is no name to ask the \ + forge about and the declared {declared:?} was checked against nothing.\n\n\ + Could not look is not a pass. Bypass this run deliberately with \ + UPHOLD_ALLOW={id}." + ))); + }; + let Some((owner, repo)) = git::owner_repo(&url) else { + return Err(Fatal::new(format!( + "{id}: could not read an owner and a repository out of the `origin` URL {url:?}, \ + so the declared {declared:?} was checked against nothing.\n\nCould not look is \ + not a pass. Bypass this run deliberately with UPHOLD_ALLOW={id}." + ))); + }; + + // The same lookup the private-name family uses, and deliberately not a + // second one. Two mechanisms answering one question are two answers free to + // disagree, and this one depends on the distinction that mechanism already + // draws: `gh` separates "the forge will not show us this" from "the forge + // would not talk to us", which is exactly the line between a fact and an + // absent check here. + let mut cache = BTreeMap::new(); + match lookup(&mut cache, &owner, &repo).visibility { + Visibility::Public => Ok(Some(Refusal { + id: id.clone(), + report: format!( + "the policy declares visibility {declared:?}, and the forge serves \ + {owner}/{repo} as PUBLIC.\n\nThe declaration is what the private-name \ + guards read as the condition they fire under, so while it says \ + {declared:?} they stand down -- over a repository everybody can read. \ + Change the declared visibility to \"public\" and let them run, or change \ + the repository back." + ), + })), + Visibility::Private => { + println!("{id}: declared {declared:?}, and the forge agrees."); + Ok(None) + } + // Neither of these disproves the declaration and neither confirms it. + // `Unknown` is the forge answering that it will show us no repository by + // this name, which for a repository declared private is the ordinary + // answer to an unauthenticated request -- and it is also the answer for + // one that was deleted or renamed. `Unavailable` is no answer at all. + // Both are the check not happening, and a check that did not happen has + // never been a pass anywhere in this binary. + Visibility::Unknown | Visibility::Unavailable => Err(Fatal::new(format!( + "{id}: the forge did not say whether {owner}/{repo} is public, so the declared \ + {declared:?} was not checked. A 404 is a private repository, a deleted one, a \ + renamed one, and a request that carried no credentials -- this rule can \ + disprove a claim of privacy and can never confirm one, so it does not read \ + silence as agreement.\n\nCould not look is not a pass. Authenticate `gh`, or \ + bypass this run deliberately with UPHOLD_ALLOW={id}." + ))), + } +} diff --git a/tests/base_sets_cli.rs b/tests/base_sets_cli.rs index 7b3bcb8..aa1cc48 100644 --- a/tests/base_sets_cli.rs +++ b/tests/base_sets_cli.rs @@ -68,14 +68,27 @@ fn code(output: &Output) -> i32 { /// each. A test that called the classifier directly would be asserting on the /// function rather than on the behaviour a repository gets. fn guard_with_gh(root: &Path, stderr_line: &str, args: &[&str]) -> Output { + guard_with_stub_gh(root, &format!("echo '{stderr_line}' >&2\nexit 1\n"), args) +} + +/// The same, for a stub that ANSWERS rather than fails. +/// +/// The falsifier's whole subject is the difference between a forge saying +/// `public`, a forge saying `private`, and a forge that said neither, so a +/// helper that can only produce the third would leave two of the three states +/// untested. `guard_with_gh` above is this one with its body written for it. +fn guard_with_stub_gh(root: &Path, body: &str, args: &[&str]) -> Output { let bin = root.join("stub-bin"); std::fs::create_dir_all(&bin).unwrap(); let gh = bin.join("gh"); - std::fs::write( - &gh, - format!("#!/bin/sh\necho '{stderr_line}' >&2\nexit 1\n"), - ) - .unwrap(); + std::fs::write(&gh, format!("#!/bin/sh\n{body}")).unwrap(); + // The real git, ahead of whatever the developer's PATH puts there. A `git` + // shim loads the policy of the tree it is invoked in and fails closed when + // that policy will not load -- so on a machine with one installed, the shim + // rather than the guard would decide what `git remote get-url` answers in + // this fixture, and a guard that reads `origin` would be testing the + // installed binary instead of this one. + let _ = std::os::unix::fs::symlink(support::real_git(), bin.join("git")); #[cfg(unix)] { use std::os::unix::fs::PermissionsExt as _; @@ -1079,3 +1092,164 @@ fn a_file_this_repository_did_not_write_may_not_carry_a_command_that_speaks_for_ stderr(&output) ); } + +// --------------------------------------------------------------------------- +// The declared visibility, checked against the forge that owns the fact. +// +// `stale-visibility` refuses exactly one state -- a policy claiming privacy over +// a repository the forge serves as public -- because that is the only direction +// a probe can establish and it is the direction that leaks. Every other answer +// is either clean or "could not look", and the cases below are mostly about +// keeping those two apart: a rule that read silence as agreement would disarm +// three disclosure guards on any machine with no credentials. +// --------------------------------------------------------------------------- + +/// A repository with an `origin` for the falsifier to ask about. +fn repository_with_origin(policy: &str) -> PathBuf { + let root = repository(policy); + git( + &root, + &[ + "remote", + "add", + "origin", + "https://github.com/acme/widget.git", + ], + ); + root +} + +/// A stub `gh api` answering the way the real one does: the `--jq` template the +/// lookup passes asks for visibility and full name as one tab-separated line. +fn gh_answers(visibility: &str) -> String { + format!("printf '{visibility}\\tacme/widget\\n'\nexit 0\n") +} + +#[test] +fn a_policy_declaring_public_has_no_claim_of_privacy_and_makes_no_request() { + // The state most repositories are in, and it must cost them nothing. The + // stub `gh` fails if it is called at all, so exit 0 here is evidence that + // no request was made rather than evidence that one succeeded. + let root = repository_with_origin( + "visibility = \"public\"\n\n[inherit]\nsets = [\"stale-visibility\"]\n", + ); + commit_one(&root); + + let output = guard_with_gh( + &root, + "gh: Bad credentials (HTTP 401)", + &["--stage", "pre-push"], + ); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert!( + String::from_utf8_lossy(&output.stdout).contains("no request was made"), + "{}", + String::from_utf8_lossy(&output.stdout) + ); +} + +#[test] +fn a_declared_privacy_the_forge_serves_as_public_is_refused() { + // The one refusable state, and the whole reason the rule exists: while the + // policy says `private` the three private-name guards stand down, over a + // repository everybody can read. + let root = repository_with_origin( + "visibility = \"private\"\n\n[inherit]\nsets = [\"stale-visibility\"]\n", + ); + commit_one(&root); + + let output = guard_with_stub_gh(&root, &gh_answers("public"), &["--stage", "pre-push"]); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + let text = stderr(&output); + assert!(text.contains("acme/widget"), "{text}"); + assert!(text.contains("PUBLIC"), "{text}"); + // And it says where the rule came from, which is the only thing a reader + // grepping the tree for the id would otherwise not find. + assert!(text.contains("[set: stale-visibility]"), "{text}"); +} + +#[test] +fn a_forge_that_agrees_the_repository_is_private_is_clean() { + let root = repository_with_origin( + "visibility = \"private\"\n\n[inherit]\nsets = [\"stale-visibility\"]\n", + ); + commit_one(&root); + + let output = guard_with_stub_gh(&root, &gh_answers("private"), &["--stage", "pre-push"]); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert!( + String::from_utf8_lossy(&output.stdout).contains("the forge agrees"), + "{}", + String::from_utf8_lossy(&output.stdout) + ); +} + +#[test] +fn a_name_the_forge_will_not_show_us_is_never_read_as_agreement() { + // The case the whole design turns on. A 404 is the ORDINARY answer for a + // genuinely private repository asked about without credentials -- and it is + // also the answer for one that was deleted, renamed, or never existed. A + // rule that read it as "confirmed private" would let an unauthenticated + // runner confirm any claim at all, which is fail-open on the one family + // where fail-open is unacceptable. + for stderr_line in ["gh: Not Found (HTTP 404)", "gh: Bad credentials (HTTP 401)"] { + let root = repository_with_origin( + "visibility = \"private\"\n\n[inherit]\nsets = [\"stale-visibility\"]\n", + ); + commit_one(&root); + + let output = guard_with_gh(&root, stderr_line, &["--stage", "pre-push"]); + assert_eq!(code(&output), 2, "{stderr_line}: {}", stderr(&output)); + let text = stderr(&output); + assert!(text.contains("Could not look is not a pass"), "{text}"); + // Named, because a guard that fails on a machine with no credentials + // and offers no way past it is a guard somebody deletes. + assert!(text.contains("UPHOLD_ALLOW=no-stale-visibility"), "{text}"); + } +} + +#[test] +fn a_repository_that_declares_no_visibility_has_no_claim_for_this_rule_to_check() { + // Not a pass. The subject of this rule is a declaration, and with none there + // is nothing to falsify -- which is a different answer from having checked + // and found nothing wrong. + let root = repository_with_origin("[inherit]\nsets = [\"stale-visibility\"]\n"); + commit_one(&root); + + let output = guard_with_stub_gh(&root, &gh_answers("public"), &["--stage", "pre-push"]); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + assert!( + stderr(&output).contains("no \nclaim") || stderr(&output).contains("no claim"), + "{}", + stderr(&output) + ); +} + +#[test] +fn the_visibility_read_from_a_command_is_the_claim_this_rule_checks() { + // The two halves of #54 meeting: the declaration comes from outside the + // tree, and it is still a declaration -- checked against the forge exactly + // as a written one is, and never replaced by what the forge said. + let root = repository_with_origin( + "visibility_from = \"printf 'private\\n'\"\n\n[inherit]\nsets = [\"stale-visibility\"]\n", + ); + commit_one(&root); + + let output = guard_with_stub_gh(&root, &gh_answers("public"), &["--stage", "pre-push"]); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!(stderr(&output).contains("PUBLIC"), "{}", stderr(&output)); +} + +#[test] +fn the_falsifier_never_runs_at_a_commit() { + // The stage decision, asserted rather than trusted to the set file. A guard + // that adds a network round trip to every commit is one somebody comments + // out, which is the reason `stale-pins` is off `pre-commit` too. + let root = repository_with_origin( + "visibility = \"private\"\n\n[inherit]\nsets = [\"stale-visibility\"]\n", + ); + commit_one(&root); + + let output = guard_with_stub_gh(&root, &gh_answers("public"), &["--stage", "pre-commit"]); + assert_eq!(code(&output), 0, "{}", stderr(&output)); +} From ab4b7a3dc36377a43e4e0b0142b47ce38ce4cbfd Mon Sep 17 00:00:00 2001 From: HackingGate Date: Thu, 20 Aug 2026 22:57:56 +0900 Subject: [PATCH 3/3] Say what the count is actually counting, and guard the one Unix call in a Unix file Both from the review on #68. ONE VALUE, NOT ONE LINE. The reference said more than one line exits 2 and the code drops blank lines before counting, so `printf 'acme\n\n'` passed as one answer while the documentation said it would not. The code is right and the sentence was wrong: a trailing blank line is what `cat` gives back for any file that ends with one, and counting it would make the field refuse the exact shape it exists for. Two non-empty lines are still two answers. Said in the code, in the refusal text, in the reference, and pinned by a test, because it reads as an oversight and is not one. THE ONE UNGUARDED UNIX CALL. The real-git symlink went in beside mode bits that were already `#[cfg(unix)]` and was not, which made it the single line that would stop the suite COMPILING off Unix rather than merely failing there. --- docs/REFERENCE.md | 2 +- src/config.rs | 11 ++++++++--- tests/base_sets_cli.rs | 38 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index cac6523..c64cbc0 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -737,7 +737,7 @@ So every way the command can fail to answer is exit `2`: | prints one value | that is the declaration, cached for the rest of the process | | exits non-zero | exit `2`, naming the fallback it refused to take | | exits `0` printing nothing | exit `2` — the file reads as a declaration and there is none | -| prints more than one line | exit `2` — one repository, one fact, and no first-line guess | +| prints more than one non-empty line | exit `2` — one repository, one fact, and no first-line guess. A trailing blank line is not a second value; `cat` gives one back for any file that ends with one | | prints a word that is not a visibility | exit `2`, naming the command rather than the file | There is deliberately **no `..._optional`** beside these, and the asymmetry with diff --git a/src/config.rs b/src/config.rs index b58338b..bd86dc6 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1741,6 +1741,11 @@ fn read_declaration(root: &Path, field: &str, command: &str) -> Result { ))); } let text = String::from_utf8_lossy(&output.stdout); + // Blank lines are dropped before the count, and that is the contract rather + // than an oversight in it: the rule is one VALUE, not one line of output. A + // trailing blank line is what `cat` gives back for a file that ends with + // one, and counting it would make this field refuse the exact shape it + // exists for. Two non-empty lines are still two answers and still refused. let mut values = text.lines().map(str::trim).filter(|line| !line.is_empty()); let Some(value) = values.next() else { return Err(Fatal::new(format!( @@ -1751,9 +1756,9 @@ fn read_declaration(root: &Path, field: &str, command: &str) -> Result { }; if values.next().is_some() { return Err(Fatal::new(format!( - "`{field}` ran {command:?}, which printed more than one line. This is one fact \ - about one repository, and taking the first line would pin the repository to \ - whatever the command happened to print first. Narrow it to the single value." + "`{field}` ran {command:?}, which printed more than one value. This is one fact \ + about one repository, and taking the first would pin the repository to whatever \ + the command happened to print first. Narrow it to the single value." ))); } Ok(value.to_owned()) diff --git a/tests/base_sets_cli.rs b/tests/base_sets_cli.rs index aa1cc48..2635e1b 100644 --- a/tests/base_sets_cli.rs +++ b/tests/base_sets_cli.rs @@ -88,7 +88,15 @@ fn guard_with_stub_gh(root: &Path, body: &str, args: &[&str]) -> Output { // rather than the guard would decide what `git remote get-url` answers in // this fixture, and a guard that reads `origin` would be testing the // installed binary instead of this one. - let _ = std::os::unix::fs::symlink(support::real_git(), bin.join("git")); + // + // Guarded like the mode bits below it: this file writes `#!/bin/sh` stubs + // and is Unix-shaped throughout, and an unguarded `std::os::unix` call in + // the middle of it would be the one line that stops the suite COMPILING + // elsewhere rather than merely failing. + #[cfg(unix)] + { + let _ = std::os::unix::fs::symlink(support::real_git(), bin.join("git")); + } #[cfg(unix)] { use std::os::unix::fs::PermissionsExt as _; @@ -954,12 +962,38 @@ fn a_source_that_answers_more_than_once_is_refused_rather_than_read_partly() { ); assert_eq!(code(&output), 2, "{}", stderr(&output)); assert!( - stderr(&output).contains("more than one line"), + stderr(&output).contains("more than one value"), "{}", stderr(&output) ); } +/// A trailing blank line is not a second answer. +/// +/// Pinned as a test because it reads as an oversight and is the contract: the +/// rule is one VALUE, not one line of output. `cat` gives a trailing blank line +/// back for any file that ends with one, so counting it would make the field +/// refuse the exact shape it exists for -- and the two-answer case beside it is +/// what the count is actually for. +#[test] +fn a_blank_line_after_the_answer_is_not_a_second_answer() { + let root = repository( + "owner_from = \"printf 'acme\\n\\n'\"\n\n[inherit]\nsets = [\"unowned-push\"]\n", + ); + commit_one(&root); + + let output = guard( + &root, + &[ + "--stage", + "pre-push", + "--remote-url", + "https://github.com/acme/widget.git", + ], + ); + assert_eq!(code(&output), 0, "{}", stderr(&output)); +} + #[test] fn a_source_that_answers_nothing_at_all_is_refused_rather_than_read_as_absent() { // Exit 0 and no output is the most dangerous shape a source has: the policy