From 4e1f683358a8ecbc65f61c222150c8b7e3e338a1 Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 21 Aug 2026 00:11:19 +0900 Subject: [PATCH 1/2] Refuse a rule that matches its own declaration and selects the file it is in A policy file is a tracked file, so a rule's own `regexp` sits inside the corpus that rule scans. An unanchored literal therefore matches the line it is written on, and the run reports the policy file as violating the rule the policy file defines -- a real path, a real line number, and nothing in the tree. Reading that finding means working out that the rule is describing itself, and nothing in the message says so. WHAT THE MEASUREMENT CHANGED ABOUT THE CHECK. Over 82 policy files and 178 `regexp` rules in one workspace, 54 rules matched their own declaration textually and NONE of them selected the file it was written in: an `include` of `["cmd", "internal"]` with a `glob` of `["*.go"]` cannot reach `policy/`. Refusing on the text alone would have failed 54 rules that work. So the scope test comes first, and it is the same test `from_index` applies to every tracked path rather than a second reader of `files.*` -- see `selection::selects`, which is that test asked about one path. With the scope test, the count of rules in those 82 trees that this refuses is zero. Nothing that passes today starts failing. WHY IT IS A LOAD-TIME REFUSAL AND NOT A REPORT. At run time this is a violation like any other, and the tier that produces it cannot say what it is. `validate_shims` gives the argument for its own pair and it holds unchanged here: a load-time refusal is the only place either can be seen at all. The message names three cures, because a refusal that names none is a wall: exclude the policy file, narrow the include, or anchor the pattern. Ordered last, after `rule.validate`. A rule naming two checks is not yet a rule whose pattern means anything, and reporting a self-match on one answers a question the reader has not reached. Own rules only. A bundled set's rule is declared inside this binary and an `inherit.paths` rule in a file this rule may not select; neither has a declaration in the policy file to match. WHAT THE FLEET WAS DOING INSTEAD, WHICH IS WHY THIS IS WORTH A REFUSAL. Three workarounds have spread with nothing recording any of them as the answer: a narrow `include` that happens to miss the policy dir, an explicit `exclude = ["**/policy/principles.toml"]`, and breaking the literal with a one-character class -- `\bYubi[K]ey\b`, `Yubi[c]o`, `Windows Hell[o]`, `Touch I[D]`, `Bitward[e]n`, nine of them in a single pattern that is unreadable because of the workaround rather than because of the problem. The third is also applied where it does nothing. `^Sta[t]us:` appears in three repositories and the dodge is unnecessary in all three: the pattern is anchored, so it cannot match `regexp = '^Status:...'` -- that line begins with `regexp`. A defensive edit that changes nothing, transcribed three times, is what "the engine gives no signal either way" looks like from outside. EIGHT TEST FIXTURES CHANGED, AND EACH ONE IS AN INSTANCE. Five wrote an unanchored literal over the whole tree -- `regexp = 'SHOUT'`, `regexp = 'hunter2'`, `regexp = "local"` -- which is exactly the shape this refuses. Their subjects are provenance, shadowing, redaction and the shim seam, none of which the added `files.exclude` or narrowed `include` touches. Three more in `config.rs` needed nothing once the check moved after `rule.validate`. 549 tests pass, clippy and fmt clean, and `uphold scan` and `uphold check` pass on this repository. --- src/config.rs | 188 ++++++++++++++++++++++++++++++++++++++++- src/selection.rs | 48 +++++++++++ tests/base_sets_cli.rs | 12 ++- tests/scan_cli.rs | 7 ++ 4 files changed, 251 insertions(+), 4 deletions(-) diff --git a/src/config.rs b/src/config.rs index 13d4914..bbd5059 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2065,6 +2065,13 @@ pub(crate) fn load(root: &Path, policy_path: &Path) -> Result { rule.validate()?; } validate_shims(policy_path, &rules, &file.shims)?; + // Last, and after `rule.validate`, because this one reads a rule as the + // author meant it. A rule naming two checks or carrying a parameter its + // check cannot read is not yet a rule whose pattern means anything, and + // reporting a self-match on one would answer a question nobody had reached + // -- the structural refusal is the finding there, and it has to arrive + // first. + validate_no_self_match(root, policy_path, &rules)?; Ok(Policy { path: policy_path.to_path_buf(), @@ -2210,6 +2217,108 @@ pub(crate) fn bundled_ids() -> Result)>> { /// ACROSS files -- two `[inherit] paths` entries defining the same id -- and /// this is the only thing that detects it. (A repository rule sharing an /// inherited id is not a collision; it shadows, and was filtered above.) +/// A rule whose pattern matches its own declaration. +/// +/// A policy file is a tracked file, so a rule's own `regexp` is inside the +/// corpus that rule scans. An unanchored literal therefore matches the line it +/// is written on, and the run reports the policy file as violating the rule the +/// policy file defines. The report names a real path and a real line and is +/// about nothing in the tree, which is the most expensive kind of finding to +/// read: a reader has to work out that the rule is describing itself. +/// +/// It is refused here rather than reported at scan time for the reason +/// [`validate_shims`] gives about its own pair -- at run time this is a +/// violation like any other, and load is the only place it can be named as what +/// it is. +/// +/// **What is NOT refused, and why the scope test comes first.** Most rules +/// never select the policy file at all: an `include` of `["cmd", "internal"]` +/// with a `glob` of `["*.go"]` cannot reach it, and a pattern that matches its +/// own text under such a rule is harmless. Measured over 82 policy files and +/// 178 `regexp` rules in one workspace, 54 matched their own declaration +/// textually and none of them selected the file it was written in. Refusing on +/// the text alone would have failed 54 rules that work. +/// +/// Anchored patterns are the other quiet half: `^Status:` cannot match +/// `regexp = '^Status:...'`, because that line begins with `regexp`. Nothing +/// special is done for them -- they simply do not match, and the point of +/// saying so is that a `Sta[t]us` written to dodge a self-match it never had is +/// a defensive edit this check would have told the author to delete. +/// +/// Own rules only. A rule from a bundled set is declared inside this binary, +/// and a rule from `inherit.paths` in a file this rule may not select; neither +/// has a declaration in `policy_path` to match. +fn validate_no_self_match(root: &Path, policy_path: &Path, rules: &[Rule]) -> Result<()> { + let Ok(relative) = policy_path.strip_prefix(root) else { + // A policy outside the tree is not part of the corpus, so no rule can + // reach it. `--policy` is free to name one. + return Ok(()); + }; + let Ok(text) = std::fs::read_to_string(policy_path) else { + // Unreadable here means unreadable a moment ago too, and load has + // already failed on it with a better message than this one could give. + return Ok(()); + }; + + for rule in rules { + if rule.origin != Origin::Own { + continue; + } + let Some(pattern) = rule.regexp.as_deref() else { + continue; + }; + if !crate::selection::selects(root, rule, relative)? { + continue; + } + let Some(section) = declaration_of(&text, &rule.id) else { + continue; + }; + let query = crate::engine::Query::from_files(pattern, rule.files()); + let hits = crate::engine::search_text(§ion, &query, &rule.id)?; + if let Some(hit) = hits.first() { + return Err(Fatal::at( + policy_path, + format!( + "rule {:?} matches its own declaration ({:?}), and it selects the file that \ + declaration is in. Every run will report this file as violating this rule, \ + naming a line that is the rule rather than anything in the tree. Exclude \ + the policy file from this rule with `files.exclude`, narrow `files.include` \ + to what the rule is about, or anchor the pattern so it cannot match the key \ + it is written under.", + rule.id, + hit.text.trim() + ), + )); + } + } + Ok(()) +} + +/// The lines of one rule's own `[rule.]` table. +/// +/// Its sub-tables belong to it -- `[rule..files]` is where `exclude` is +/// written, and an author dodging a self-match often puts the pattern's own +/// text there -- so the section runs to the next table that is not one of them. +fn declaration_of(text: &str, id: &str) -> Option { + let header = format!("[rule.{id}]"); + let sub = format!("[rule.{id}."); + let lines: Vec<&str> = text.lines().collect(); + let start = lines.iter().position(|line| line.trim() == header)?; + let end = lines + .iter() + .enumerate() + .skip(start + 1) + .find(|(_, line)| { + let trimmed = line.trim_start(); + trimmed.starts_with('[') && !trimmed.starts_with(&sub) + }) + .map_or(lines.len(), |(index, _)| index); + // `get` rather than an index: `end` comes from a search that starts past + // `start`, so the range holds -- but a slice that can panic is a slice that + // will, and this one runs over author-written text. + Some(lines.get(start..end)?.join("\n")) +} + fn validate_unique(policy_path: &Path, rules: &[Rule]) -> Result<()> { let mut seen: BTreeMap<&str, Option> = BTreeMap::new(); for rule in rules { @@ -3138,8 +3247,13 @@ mod tests { [rule.mine] message = "local" + # Narrowed so `regexp` cannot match its own declaration. Any + # unanchored literal written as `regexp = "X"` contains X on that + # line, so a whole-repo fixture pattern is refused by + # `validate_no_self_match`; what this test is about is provenance, + # not selection. regexp = "local" - files.include = ["."] + files.include = ["src"] "#, ) .unwrap(); @@ -3168,6 +3282,10 @@ mod tests { regexp = "local" [rule.no-hardcoded-home-paths.files] + # See `provenance_answers_about_the_id_it_was_asked_about`: an + # unanchored literal fixture pattern matches its own declaration, + # and this test is about shadowing rather than selection. + include = ["src"] "#, ) .unwrap(); @@ -3277,6 +3395,74 @@ mod tests { ); } + #[test] + fn a_rule_that_matches_its_own_declaration_and_selects_it_is_refused() { + // The report this replaces named the policy file and a line number and + // was about nothing in the tree: the rule describing itself. There is + // no version of that finding a reader can act on, so it is refused + // where it can still be called what it is. + let error = policy_from( + "[rule.unanchored]\nregexp = 'YubiKey'\nmessage = \"m\"\n[rule.unanchored.files]\ninclude = [\".\"]\n", + ) + .unwrap_err(); + let text = error.to_string(); + assert!(text.contains("unanchored"), "{text}"); + assert!(text.contains("matches its own declaration"), "{text}"); + // The three cures, because a refusal that does not name one is a wall. + assert!(text.contains("files.exclude"), "{text}"); + assert!(text.contains("files.include"), "{text}"); + assert!(text.contains("anchor"), "{text}"); + } + + #[test] + fn an_anchored_pattern_cannot_match_the_key_it_is_written_under() { + // `^Status:` does not match `regexp = '^Status:...'` -- that line begins + // with `regexp`. Worth a test rather than a remark, because a dodge + // written to avoid a self-match that never existed has been transcribed + // across three repositories, and this is the fact that makes it + // deletable. + policy_from( + "[rule.anchored]\nregexp = '^Status:'\nmessage = \"m\"\n[rule.anchored.files]\ninclude = [\".\"]\n", + ) + .unwrap(); + } + + #[test] + fn a_self_matching_pattern_that_cannot_reach_the_policy_file_still_loads() { + // The case that decides whether this check is usable at all. Measured + // over 82 policy files, 54 `regexp` rules matched their own text and + // NONE of them selected the file it was written in -- an `include` of + // source directories does not reach `policy/`. Refusing on the text + // alone would have failed 54 rules that work. + for narrowing in [ + "[rule.narrow.files]\ninclude = [\"src\"]\n", + "[rule.narrow.files]\ninclude = [\".\"]\nglob = [\"*.go\"]\n", + "[rule.narrow.files]\ninclude = [\".\"]\nexclude = [\"**/rg-policy.toml\"]\n", + ] { + let text = format!("[rule.narrow]\nregexp = 'YubiKey'\nmessage = \"m\"\n{narrowing}"); + let outcome = policy_from(&text); + assert!( + outcome.is_ok(), + "{narrowing:?} should still load: {:?}", + outcome.err() + ); + } + } + + #[test] + fn a_declaration_runs_to_the_next_table_that_is_not_its_own_sub_table() { + // `[rule.x.files]` belongs to `rule.x`; `[rule.y]` does not. Getting + // this wrong in the widening direction would read a sibling's pattern + // as this rule's own text and refuse a rule that is fine. + let text = + "[rule.x]\nregexp = 'a'\n[rule.x.files]\ninclude = [\".\"]\n[rule.y]\nregexp = 'b'\n"; + let section = declaration_of(text, "x").unwrap(); + assert!(section.contains("regexp = 'a'"), "{section}"); + assert!(section.contains("include"), "{section}"); + assert!(!section.contains("regexp = 'b'"), "{section}"); + assert!(declaration_of(text, "absent").is_none()); + } + #[test] fn a_policy_that_cannot_be_parsed_is_an_error_and_not_an_empty_policy() { // The invariant stated directly, since the sweep above can only assert diff --git a/src/selection.rs b/src/selection.rs index fea3a15..71d4ba8 100644 --- a/src/selection.rs +++ b/src/selection.rs @@ -268,6 +268,54 @@ fn overrides_for(root: &Path, rule: &Rule, not_text: &[String]) -> Result Result { + let prefixes = include_prefixes(rule); + if !prefixes + .iter() + .any(|prefix| prefix.as_os_str().is_empty() || relative.starts_with(prefix)) + { + return Ok(false); + } + let overrides = overrides_for(root, rule, &[])?; + Ok(!overrides.matched(relative, false).is_ignore()) +} + +/// The repository-relative roots one rule searches under, as written. +/// +/// Split out of [`search_roots`] so [`selects`] can ask the same question +/// without the side effects that belong to a real search: the warning about an +/// include that is not there, and the refusal of one that leaves the tree. Both +/// are reports about a scan that is happening, and neither is true of a caller +/// that only wants to know whether a path is in scope. +fn include_prefixes(rule: &Rule) -> Vec { + let include = rule.include(); + if include.is_empty() { + return vec![PathBuf::new()]; + } + include + .iter() + .map(|spec| { + if spec == "." { + PathBuf::new() + } else { + PathBuf::from(spec) + } + }) + .collect() +} + /// The roots one rule searches under, refusing any that leaves the repository. fn search_roots(root: &Path, rule: &Rule) -> Result> { let include = rule.include(); diff --git a/tests/base_sets_cli.rs b/tests/base_sets_cli.rs index 2635e1b..9a6797b 100644 --- a/tests/base_sets_cli.rs +++ b/tests/base_sets_cli.rs @@ -289,8 +289,14 @@ fn an_override_of_a_set_this_policy_inherits_stays_silent() { #[test] fn a_rule_no_set_owns_is_nobody_elses_business() { + // `files.exclude` here, and in the two fixtures below, for the reason the + // bundled `credentials` set carries the same line: an unanchored literal + // written as `regexp = 'SHOUT'` contains SHOUT on its own declaration line, + // so a rule selecting the whole tree reports its own policy file. + // `validate_no_self_match` refuses that at load; what these tests are about + // is provenance and shadowing, which the exclusion leaves untouched. let root = repository(&format!( - "{AUDIT}\n[rule.no-shouting]\nmessage = \"quiet\"\nregexp = 'SHOUT'\nfiles.include = [\".\"]\n" + "{AUDIT}\n[rule.no-shouting]\nmessage = \"quiet\"\nregexp = 'SHOUT'\nfiles.include = [\".\"]\nfiles.exclude = [\"policy/**\"]\n" )); let output = guard(&root, &["--stage", "manual"]); @@ -305,7 +311,7 @@ fn a_refusal_from_an_inherited_set_names_the_set_it_arrived_from() { // file for something that was never in it. let root = repository( "[inherit]\nsets = [\"process-residue\"]\n\n\ - [rule.no-committed-secret-material]\nmessage = \"copied\"\nregexp = 'BEGIN PRIVATE KEY'\nfiles.include = [\".\"]\n", + [rule.no-committed-secret-material]\nmessage = \"copied\"\nregexp = 'BEGIN PRIVATE KEY'\nfiles.include = [\".\"]\nfiles.exclude = [\"policy/**\"]\n", ); let output = guard(&root, &["--stage", "manual"]); @@ -329,7 +335,7 @@ fn an_override_that_changes_the_check_is_reported_at_load() { // after. let root = repository( "[inherit]\nsets = [\"process-residue\"]\n\n\ - [rule.no-tracked-private-data-paths]\nmessage = \"mine\"\nregexp = 'private'\nfiles.include = [\".\"]\n", + [rule.no-tracked-private-data-paths]\nmessage = \"mine\"\nregexp = 'private'\nfiles.include = [\".\"]\nfiles.exclude = [\"policy/**\"]\n", ); let output = guard(&root, &["--stage", "manual"]); diff --git a/tests/scan_cli.rs b/tests/scan_cli.rs index 620368e..b9074d7 100644 --- a/tests/scan_cli.rs +++ b/tests/scan_cli.rs @@ -664,6 +664,11 @@ fn redaction_withholds_the_match_and_keeps_the_location() { regexp = 'hunter2' [rule.no-secret.files] + # Excluded for the reason the bundled sets carry the same line: an + # unanchored literal written as `regexp = '...'` contains its own text + # on that line, so a rule selecting the whole tree reports its own + # policy file. Refused at load by `validate_no_self_match`. + exclude = ["policy/**"] "#, ); write(&root, "a.txt", "password is hunter2\n"); @@ -1123,6 +1128,7 @@ fn the_effective_rules_are_what_inheritance_resolved_to() { message = "declared here" regexp = 'nothing-matches-this-either' files.include = ["."] + files.exclude = ["policy/**"] [rule.no-local-merge] builtin = "no-local-merge" @@ -1268,6 +1274,7 @@ fn a_rule_that_only_stands_in_front_of_a_command_names_the_shim_seam() { message = "no TODO" regexp = 'TODO' files.include = ["."] + files.exclude = ["policy/**"] "#, ); From ebf9fdc7f66af1c47e60c89b832233a089713e8e Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 21 Aug 2026 00:20:35 +0900 Subject: [PATCH 2/2] Exclude the policy file from the review test's fixture rule, like the other eight `tests/test_review.py` builds a policy carrying `regexp = 'TODO'` over the whole tree. That is the shape the new load-time refusal catches: the literal appears on its own `regexp` line, so the rule reports the file it is declared in. The test is about a claim naming a rule that does not exist, and it asserted exit 1 -- a policy violation. A load refusal is a `Fatal` and exits 2, the same as every other validator in `load`, so the assertion failed on the code rather than on anything it was testing. Found by the three hook jobs in CI, which run this repository's own lefthook commands and were the only jobs that reached this suite. Every consumer job passed, which is the half that matters: nothing outside this repository changed behaviour. --- tests/test_review.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_review.py b/tests/test_review.py index 89e9c7e..1c46dd8 100644 --- a/tests/test_review.py +++ b/tests/test_review.py @@ -225,6 +225,11 @@ def test_a_claim_no_seam_supplies_does_not_remove_its_principle_from_review(self message = "no TODO" regexp = 'TODO' files.include = ["."] + # An unanchored literal contains its own text on the `regexp` line, + # so a rule selecting the whole tree reports its own policy file and + # the engine refuses it at load. This test is about a claim naming a + # rule that does not exist, not about selection. + files.exclude = ["policy/**"] """, ) self.assertEqual(result.returncode, 1, result.stdout)