From 948600c30c409f3a8674e8b3c865fec485ab3e53 Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 21 Aug 2026 13:09:43 +0900 Subject: [PATCH] Refuse two shim tables naming one command, which silently dropped one of them `shim::run` resolves the declared shims into a map keyed by command name, so a second `[[shim]]` naming a command already declared replaced the first and said nothing. The surviving table is whichever the map happened to keep, and the lost one is a set of verbs the repository believes are guarded. Reproduced before this was written. A policy declaring `gh` twice -- once for `pr:create` with `--body`, once for `issue:close` with `--comment`: uphold scan policy checks passed uphold shim gh issue close 1 --comment X refused by the checker uphold shim gh pr create --body X exec'd the real gh, unexamined Text the author had written a table for reached a forge with nothing in front of it, and the run that was supposed to notice reported a clean policy. WHY REFUSED RATHER THAN MERGED. Merging is a guess about which vocabulary wins where two tables disagree, and they disagree exactly where it matters: `-c` is a BOOLEAN on `gh pr review` ("Comment on a pull request") and takes a VALUE on `gh issue close` ("Leave a closing comment"). A merged `text_flags` misreads one of those whichever way it is built -- either `-c` swallows the next argument on a review, or a closing comment is published unread. That collision is the reason somebody would write a second table in the first place, so the refusal has to make it visible to the person writing it rather than resolve it silently. What the refusal cannot do is fix it: `text_flags` is per-command and gh's flag meanings are per-verb, and closing that gap is a schema question rather than a load check. No policy in the surveyed workspace declares one command twice, so nothing that loads today stops loading. 558 tests pass, clippy and fmt clean, every lefthook pre-commit command run directly. --- src/config.rs | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/src/config.rs b/src/config.rs index a99079d..7f6131a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2084,6 +2084,7 @@ pub(crate) fn load(root: &Path, policy_path: &Path) -> Result { for rule in &rules { rule.validate()?; } + validate_shim_commands(policy_path, &file.shims)?; 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 @@ -2416,6 +2417,44 @@ fn validate_unique(policy_path: &Path, rules: &[Rule]) -> Result<()> { /// same reason: a name that resolves to nothing is a decision that looks made. /// A load-time refusal is also the only place either can be seen at all -- /// at run time both are silence. +/// One command, one `[[shim]]`. +/// +/// The shims are resolved into a map keyed by command name, so a second table +/// naming a command already declared replaced the first and said nothing. Both +/// halves of that are bad and the silence is the worse one: the surviving table +/// is whichever the map happened to keep, and the lost one is a set of verbs the +/// repository believes are guarded. +/// +/// Reproduced before this was written. A policy declaring `gh` twice -- once for +/// `pr:create` with `--body`, once for `issue:close` with `--comment` -- loaded +/// clean, refused the comment, and exec'd `gh pr create --body ...` unexamined. +/// Text the author had written a table for reached a forge with nothing in +/// front of it. +/// +/// Refused rather than merged, because merging is a guess about which +/// vocabulary wins where the two disagree -- and they disagree exactly where it +/// matters. `-c` is a boolean on `gh pr review` and takes a value on +/// `gh issue close`, so a merged `text_flags` misreads one of them whichever way +/// it is built. One table per command makes that collision visible to whoever +/// writes the second one instead of resolving it silently. +fn validate_shim_commands(policy_path: &Path, shims: &[crate::shim::Shim]) -> Result<()> { + let mut seen: BTreeSet<&str> = BTreeSet::new(); + for shim in shims { + if !seen.insert(shim.command.as_str()) { + return Err(Fatal::at( + policy_path, + format!( + "two `[[shim]]` tables name the command {:?}. Only one of them stands in \ + front of it -- the other is dropped, and the verbs it names are published \ + with nothing checking them. Put every verb for one command in one table.", + shim.command + ), + )); + } + } + Ok(()) +} + fn validate_shims(policy_path: &Path, rules: &[Rule], shims: &[crate::shim::Shim]) -> Result<()> { let declared: BTreeSet<&str> = shims.iter().map(|shim| shim.command.as_str()).collect(); // The first word of a `before` entry is the command itself; the rest is as @@ -3455,6 +3494,40 @@ mod tests { ); } + #[test] + fn two_shim_tables_naming_one_command_are_refused() { + // The map they resolve into is keyed by command, so the second table + // replaced the first and said nothing. Reproduced before this was + // written: a policy declaring `gh` twice -- `pr:create` with `--body`, + // `issue:close` with `--comment` -- loaded clean, refused the comment, + // and exec'd `gh pr create --body ...` unexamined. + let error = policy_from( + "[rule.judge]\nmessage = \"m\"\nexec = \"/bin/false\"\ncommand.before = [\"gh\"]\n\n [[shim]]\ncommand = \"gh\"\nmatch = [\"pr:create\"]\ntext_flags = [\"-b\"]\n\n [[shim]]\ncommand = \"gh\"\nmatch = [\"issue:close\"]\ntext_flags = [\"-c\"]\n", + ) + .unwrap_err(); + let text = error.to_string(); + assert!(text.contains("two `[[shim]]` tables"), "{text}"); + assert!(text.contains("\"gh\""), "{text}"); + // It says what the silence cost, because "duplicate" alone reads as + // tidiness rather than as text published unchecked. + assert!(text.contains("nothing checking them"), "{text}"); + } + + #[test] + fn one_shim_table_may_name_every_verb_for_its_command() { + // The cure the refusal names, and the shape every policy already uses. + policy_from( + "[rule.judge]\nmessage = \"m\"\nexec = \"/bin/false\"\ncommand.before = [\"gh\"]\n\n [[shim]]\ncommand = \"gh\"\nmatch = [\"pr:create\", \"issue:close\"]\ntext_flags = [\"-b\"]\n", + ) + .unwrap(); + + // And two tables for two DIFFERENT commands are not the same thing. + policy_from( + "[rule.judge]\nmessage = \"m\"\nexec = \"/bin/false\"\ncommand.before = [\"gh\", \"glab\"]\n\n [[shim]]\ncommand = \"gh\"\nmatch = [\"pr:create\"]\ntext_flags = [\"-b\"]\n\n [[shim]]\ncommand = \"glab\"\nmatch = [\"mr:create\"]\ntext_flags = [\"-d\"]\n", + ) + .unwrap(); + } + #[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