fix: address PR #1853 review feedback (force-if-includes, switch, subshell, +refspec)

Five additional review findings on top of the round-1 tokenizer fix.
Combined patch surface is small (one push branch, new switch branch,
exploded subshell handling); all six review issues are now closed.

P1 — --force --force-if-includes still destructive (Greptile, line 217):
  Previous logic treated --force-if-includes as a safety guarantee
  alongside --force-with-lease. Per git-scm.com/docs/git-push,
  --force-if-includes is a no-op WITHOUT --force-with-lease, so a
  combination of --force --force-if-includes is just --force. Push
  branch now treats only --force-with-lease as a lease, and reports
  force when --force / -f is present.

P2 — git switch destructive forms not detected (Greptile, line 234):
  Added a switch branch to isDestructiveGit covering:
    --discard-changes   (explicit discard)
    --force / -f        (ignore conflicts, overwrite)
    -C <branch>         (force-create, overwrites existing branch)

P0 — backtick + $(...) subshell bypass (CodeRabbit, line 64):
  Added explodeSubshells() that promotes `...` and $(...) contents
  to top-level segment separators. Run on both the SQL/dd regex
  input and the per-segment shell tokenizer input. Loops up to 4
  passes to catch a layer of nesting. Without this,
  `echo y | $(rm -rf /tmp)` slipped past the segment splitter
  because the destructive command lived inside a sub-expression.

P0 — +refspec force push (CodeRabbit, line 217):
  `git push origin +main`, `+refs/heads/main:refs/heads/main`, etc.
  force a non-fast-forward update of that specific ref. Push branch
  now also flags any positional arg starting with `+` that matches
  a refspec shape. Excludes bare `+` and numeric-only tokens.

P2 — missing --force --force-if-includes regression test
  (Greptile, line 1202): added.

Tests (+10 on top of the round-1 +10):
  Bypass-now-blocked:
    - git push --force --force-if-includes (force-if-includes is no-op
      without lease — bare force is still in effect)
    - git push origin +main (+refspec bare branch)
    - git push origin +refs/heads/main:refs/heads/main (+refspec full)
    - git switch --discard-changes
    - git switch --force
    - git switch -f (short form)
    - git switch -C (force-create)
    - echo y | `rm -rf /tmp` (backtick subshell)
    - echo y | $(rm -rf /tmp) (dollar-paren subshell)
  Still-allowed:
    - git switch feature (plain)

67/67 in gateguard-fact-force.test.js. 2380/2380 across the full
suite. yarn lint clean. All seven CI validators pass.

Refs #1843.
This commit is contained in:
Jamkris
2026-05-13 15:26:03 +09:00
parent 231c1fdbe8
commit 12353c52c4
2 changed files with 116 additions and 20 deletions
+47
View File
@@ -1213,6 +1213,53 @@ function runTests() {
'git push --force-if-includes');
})) passed++; else failed++;
// --- Review-round-2 findings ---
if (test('denies git push --force even with --force-if-includes present', () => {
expectDestructiveDeny('git push --force --force-if-includes origin main',
'git push --force --force-if-includes');
})) passed++; else failed++;
if (test('denies git push with +refspec prefix (bare branch)', () => {
expectDestructiveDeny('git push origin +main', 'git push origin +main');
})) passed++; else failed++;
if (test('denies git push with +refspec prefix (full ref)', () => {
expectDestructiveDeny('git push origin +refs/heads/main:refs/heads/main',
'git push origin +refs/heads/main:refs/heads/main');
})) passed++; else failed++;
if (test('denies git switch --discard-changes', () => {
expectDestructiveDeny('git switch --discard-changes feature',
'git switch --discard-changes');
})) passed++; else failed++;
if (test('denies git switch --force', () => {
expectDestructiveDeny('git switch --force main', 'git switch --force');
})) passed++; else failed++;
if (test('denies git switch -f short form', () => {
expectDestructiveDeny('git switch -f main', 'git switch -f');
})) passed++; else failed++;
if (test('denies git switch -C force-create', () => {
expectDestructiveDeny('git switch -C feature', 'git switch -C');
})) passed++; else failed++;
if (test('still allows plain git switch', () => {
expectAllow('git switch feature', 'git switch feature');
})) passed++; else failed++;
if (test('denies rm -rf nested inside a backtick subshell', () => {
expectDestructiveDeny('echo y | `rm -rf /tmp/junk`',
'backtick subshell');
})) passed++; else failed++;
if (test('denies rm -rf nested inside a $(...) subshell', () => {
expectDestructiveDeny('echo y | $(rm -rf /tmp/junk)',
'dollar-paren subshell');
})) passed++; else failed++;
// Cleanup only the temp directory created by this test file.
try {
if (fs.existsSync(stateDir)) {