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/**"] "#, ); 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)