From fb656339589218d345ddde168ece824f30ff8c68 Mon Sep 17 00:00:00 2001 From: HackingGate Date: Sun, 23 Aug 2026 15:55:30 +0900 Subject: [PATCH] Read the push shim's refspecs off the positions the matcher found `git -c user.name=x push` was matched and then collected by argv index, so the branch it publishes was checked nowhere. VALUE_OPTIONS taught `reading` that `-c` takes the word after it, which is what makes that invocation match a `push:*` table at all. `collect_git_refs` was never given the same grammar: it skipped argv[0] as the subcommand and the next non-option word as the remote, so `git -c user.name=x push` read `user.name=x` as the remote and `push` as a refspec. The name actually going onto the forge was collected nowhere, and the fallback that reads it off HEAD did not run either, because a name had been collected. Measured against 1.8.0 as installed, in a tree whose policy refuses the branch name it is standing on: uphold shim git push refused uphold shim git -c user.name=x push published uphold shim git -C elsewhere push published uphold shim git --git-dir X push published One grammar, read once. `words` and the new `positional` are one walk in `scan`, which stops after two positional words for a matcher and after all of them for a collector; `collect_git_refs` reads the third onward. An option this grammar cannot classify still shifts the positions by one. The words after it are read regardless, and a shift that leaves no refspec at all falls back to HEAD, so what remains is an extra subject rather than a missing one. Both tests fail on the old collector: a unit test over four spellings of a global option, and a CLI test that drives real invocations to a stub git and asserts the command never ran. Claude-Session: https://claude.ai/code/session_01K6XKWdtY1VqQZE3E1GH15T --- src/shim.rs | 116 +++++++++++++++++++++++++++++++++++++++------- tests/shim_cli.rs | 91 ++++++++++++++++++++++++++++++++++++ 2 files changed, 189 insertions(+), 18 deletions(-) diff --git a/src/shim.rs b/src/shim.rs index 01d1cb9..c7ec258 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -306,6 +306,16 @@ struct Words { unclear_count: usize, } +/// One walk of argv: the positional words it found, and what it could not +/// classify on the way. `Words` is the first two of them named; a positional +/// collector is all of them. +#[derive(Debug)] +struct Scanned<'a> { + positional: Vec<&'a str>, + unclear: Option, + unclear_count: usize, +} + /// What a shim can say about an invocation from argv alone. #[derive(Debug)] enum Reading { @@ -364,6 +374,49 @@ impl Shim { /// create` loses `pr`. Both readings are tried, and where they disagree the /// caller hears that rather than a verdict. fn words(&self, argv: &[String], unknown_takes_value: bool) -> Words { + let scanned = self.scan(argv, unknown_takes_value, 2); + let mut found = scanned.positional.into_iter(); + Words { + verb: found.next().unwrap_or_default().to_owned(), + noun: found.next().unwrap_or_default().to_owned(), + unclear: scanned.unclear, + unclear_count: scanned.unclear_count, + } + } + + /// Every positional word of an invocation, in order: the subcommand, and + /// what follows it that is neither an option nor an option's value. + /// + /// `words` asks the same question and stops at two, because naming the + /// invocation is all a `match` list needs. A collector that reads its + /// subjects out of the POSITIONS -- `git push ...` -- + /// needs the rest of them, and needs them read with the grammar the matcher + /// used. Reading them by skipping every word that begins with `-` is the + /// same mistake [`VALUE_OPTIONS`] exists to refuse one question earlier: + /// `git -c user.name=x push` then yields remote `user.name=x` and refspec + /// `push`, so the branch actually being published is collected nowhere -- + /// and the fallback that reads it off `HEAD` does not run either, because a + /// name was collected. + /// + /// The bare reading, which is the one `reading` tries first. An option this + /// grammar cannot classify shifts the positions by one; the words after it + /// are still read, and a shift that leaves no refspec at all falls back to + /// `HEAD` rather than to nothing. + fn positional(&self, argv: &[String]) -> Vec { + self.scan(argv, false, usize::MAX) + .positional + .into_iter() + .map(str::to_owned) + .collect() + } + + /// One walk of argv, stopping once `stop_after` positional words are found. + fn scan<'a>( + &self, + argv: &'a [String], + unknown_takes_value: bool, + stop_after: usize, + ) -> Scanned<'a> { let mut found: Vec<&str> = Vec::new(); let mut unclear: Option = None; let mut unclear_count = 0usize; @@ -416,14 +469,12 @@ impl Shim { continue; } found.push(argument); - if found.len() == 2 { + if found.len() >= stop_after { break; } } - let mut found = found.into_iter(); - Words { - verb: found.next().unwrap_or_default().to_owned(), - noun: found.next().unwrap_or_default().to_owned(), + Scanned { + positional: found, unclear, unclear_count, } @@ -612,21 +663,22 @@ impl Shim { } /// Branch and tag names, which appear nowhere as a flag value. - #[expect( - clippy::unused_self, - reason = "one signature for every `collect` arm; the flags collector needs the table" - )] + /// + /// Read off the POSITIONS, and off the ones `positional` finds rather than + /// off argv's own indices: the subcommand is not always the first word -- + /// `git -c user.name=x push` and `git -C elsewhere push` both put two + /// before it -- and a collector that assumed it was read the option's value + /// as the remote and `push` itself as the branch being published. That is + /// the same reading [`VALUE_OPTIONS`] was written to end, arriving one + /// question later: the shim MATCHED those invocations and then checked a + /// name nobody was publishing, while the branch that was went out unread. fn collect_git_refs(&self, root: &Path, argv: &[String]) -> Result> { let mut names = Vec::new(); - let mut seen_remote = false; - for argument in argv.iter().skip(1) { - if argument.starts_with('-') { - continue; - } - if !seen_remote { - seen_remote = true; - continue; - } + // The subcommand, then the remote, then the refspecs. Both leading + // words are positions rather than names here: `push` is the verb this + // shim matched on, and a remote is a local nickname that is not itself + // published. + for argument in self.positional(argv).iter().skip(2) { // A refspec is `src:dst`; both halves are published, and // `refs/heads/` is noise rather than name. for half in argument.split(':') { @@ -1974,6 +2026,34 @@ mod tests { .unwrap()); } + #[test] + fn a_global_option_does_not_shift_which_word_the_branch_is() { + // One grammar, read once. `reading` learned that `-c` takes the word + // after it -- which is what made `git -c user.name=x push` match at all + // -- and the collector was still counting argv positions, so it read + // `user.name=x` as the remote and `push` as the branch. The name being + // published was checked nowhere, and the fallback that reads it off + // HEAD was skipped by the word the misreading had collected. + let push = git_push(); + let names = |line: &str| -> Vec { + push.collect(Path::new("."), &argv(line)) + .unwrap() + .subjects + .into_iter() + .map(|subject| subject.value) + .collect() + }; + for line in [ + "push origin topic", + "-c user.name=x push origin topic", + "-C elsewhere push origin topic", + "--git-dir /elsewhere/.git push origin topic", + "--no-pager push origin topic", + ] { + assert_eq!(names(line), vec![String::from("topic")], "{line}"); + } + } + #[test] fn a_refspec_publishes_both_halves_of_its_name() { let mut push = gh(); diff --git a/tests/shim_cli.rs b/tests/shim_cli.rs index 2d81e34..fac2ccd 100644 --- a/tests/shim_cli.rs +++ b/tests/shim_cli.rs @@ -764,6 +764,97 @@ fn a_git_global_option_before_the_subcommand_does_not_switch_the_shim_off() { ); } +/// A `git` shim that reads its subjects out of the positions, as the shipped +/// policy declares it. +const GIT_REFS_POLICY: &str = r#" +[rule.no-published-branch-name] +message = "that name goes onto a public forge in the ref list" +exec = 'if grep -q acme; then echo "the name names a private owner" >&2; exit 1; fi' + +[rule.no-published-branch-name.command] +before = ["git"] + +[[shim]] +command = "git" +match = ["push:*"] +scope = "always" +collect = "git-refs" +"#; + +/// A `git` that reports a push instead of making one, and is the real git for +/// everything else -- the shim reads `HEAD` through it. +fn forwarding_git(root: &Path) { + stub( + root, + "git", + &format!( + "#!/bin/sh\nfor word in \"$@\"; do\n [ \"$word\" = push ] && {{ echo \"git ran: $*\"; exit 0; }}\ndone\nexec {} \"$@\"\n", + support::real_git().display() + ), + ); +} + +#[test] +fn a_global_option_does_not_shift_which_word_the_branch_is() { + // The matcher learned git's global grammar and the collector did not. + // `git -c user.name=x push origin topic` was MATCHED -- that is what + // `VALUE_OPTIONS` bought -- and then collected by argv index: `user.name=x` + // read as the remote, `push` as the name being published, and the branch + // that actually goes out checked nowhere. The bare form is worse: a word + // WAS collected, so the fallback that reads the branch off `HEAD` did not + // run, and `git -c ... push` published a branch name through nothing. + let root = workspace(GIT_REFS_POLICY); + forwarding_git(&root); + Command::new(support::real_git()) + .args(["symbolic-ref", "HEAD", "refs/heads/fix/acme-outage"]) + .current_dir(&root) + .status() + .unwrap(); + + for form in [ + vec!["git", "push", "origin", "fix/acme-outage"], + vec![ + "git", + "-c", + "user.name=x", + "push", + "origin", + "fix/acme-outage", + ], + vec![ + "git", + "-C", + "elsewhere", + "push", + "origin", + "fix/acme-outage", + ], + vec![ + "git", + "--git-dir", + "elsewhere/.git", + "push", + "origin", + "fix/acme-outage", + ], + // The name appears nowhere in argv, so the subject comes off `HEAD`. + vec!["git", "-c", "user.name=x", "push"], + ] { + let output = shim(&root, &form); + assert_eq!(code(&output), 1, "{form:?}: {}", stderr(&output)); + assert!(!stdout(&output).contains("git ran:"), "{form:?}"); + } + + // And the other half: a name nothing refuses still reaches the command, + // past the same option. + let output = shim( + &root, + &["git", "-c", "user.name=x", "push", "origin", "fix/ordinary"], + ); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert!(stdout(&output).contains("git ran:"), "{}", stdout(&output)); +} + #[test] fn an_option_nothing_can_classify_is_said_out_loud_rather_than_passed_in_silence() { // TWO options nothing can classify, which is what leaves the subcommand