fix(hooks): close grouped command bypasses in gateguard (#1912)

Inspect executable bodies inside plain subshells and brace groups before applying destructive command classifiers.\n\nCo-authored-by: Jamkris <82251632+Jamkris@users.noreply.github.com>
This commit is contained in:
SeungHyun
2026-05-15 14:39:15 +09:00
committed by GitHub
parent e2992860ae
commit 8cfadfea28
3 changed files with 412 additions and 101 deletions

View File

@@ -1282,6 +1282,115 @@ function runTests() {
'double-quoted dollar-paren subshell');
})) passed++; else failed++;
// --- Subshell + brace-group bypass coverage ---
// Destructive commands inside `(...)` and `{ ...; }` execute the
// same way they do at the top level, so the destructive classifier
// must see inside those bodies too. Nested parens `((...))` are
// arithmetic-evaluation syntax in bash (not a nested subshell), but
// our parser depth-tracks them conservatively — i.e. the inner
// tokens are still scanned for destructive intent. That's safety
// over precision and the right default for this gate.
if (test('denies rm -rf inside plain (...) subshell group', () => {
expectDestructiveDeny('(rm -rf /tmp/junk)', 'plain subshell group');
})) passed++; else failed++;
if (test('denies rm -rf inside ((...)) — arithmetic eval, treated conservatively', () => {
expectDestructiveDeny('((rm -rf /tmp/junk))', 'arithmetic-eval parens');
})) passed++; else failed++;
if (test('denies rm -rf inside { ...; } brace group', () => {
expectDestructiveDeny('{ rm -rf /tmp/junk; }', 'brace group');
})) passed++; else failed++;
if (test('denies git push --force inside plain (...) subshell group', () => {
expectDestructiveDeny('(git push --force origin main)',
'git-force in subshell');
})) passed++; else failed++;
if (test('denies git push --force inside { ...; } brace group', () => {
expectDestructiveDeny('{ git push --force origin main; }',
'git-force in brace group');
})) passed++; else failed++;
if (test('denies rm -rf nested across () and {} (cross-syntax)', () => {
expectDestructiveDeny('(echo y; { rm -rf /tmp/junk; })',
'() containing {} cross-syntax');
})) passed++; else failed++;
if (test('denies rm -rf nested across $() and () (cross-syntax)', () => {
expectDestructiveDeny('$(echo y; (rm -rf /tmp/junk))',
'$() containing () cross-syntax');
})) passed++; else failed++;
// Negative cases — literals and non-destructive commands must NOT
// be promoted to destructive by the new grouping-body walker.
if (test('allows literal (rm -rf ...) inside single quotes', () => {
expectAllow("git commit -m '(rm -rf /tmp/junk)'",
'single-quoted subshell literal');
})) passed++; else failed++;
if (test('allows literal (rm -rf ...) inside double quotes', () => {
expectAllow('echo "(rm -rf /tmp/junk)"',
'double-quoted subshell literal');
})) passed++; else failed++;
if (test('allows literal { rm -rf ...; } inside double quotes', () => {
expectAllow('echo "{ rm -rf /tmp/junk; }"',
'double-quoted brace-group literal');
})) passed++; else failed++;
if (test('allows non-destructive (echo hello)', () => {
expectAllow('(echo hello)', 'non-destructive subshell');
})) passed++; else failed++;
if (test('allows non-destructive { echo hello; }', () => {
expectAllow('{ echo hello; }', 'non-destructive brace group');
})) passed++; else failed++;
if (test('allows {rm -rf} — no space after { is not a brace group', () => {
// bash treats `{rm` as a single token; no destructive intent
// can be statically derived from this form, and the command
// would not actually run rm at runtime either.
expectAllow('echo {rm -rf /tmp/junk}',
'no-space brace literal');
})) passed++; else failed++;
// --- Round 1 review fixes: brace-group span-skip + boundary ---
// Verifies the body-accumulation loop in `extractBraceGroups`
// correctly walks past `$(...)`, `(...)`, and backtick spans so
// a `}` inside one of those does not terminate the brace group
// early, plus the nested `{` boundary rule.
if (test('denies rm -rf in brace group with backtick containing }', () => {
expectDestructiveDeny('{ echo `echo }`; rm -rf /tmp/junk; }',
'brace + backtick containing }');
})) passed++; else failed++;
if (test('denies rm -rf in brace group with $() containing }', () => {
expectDestructiveDeny('{ echo $(echo "}"); rm -rf /tmp/junk; }',
'brace + $() containing }');
})) passed++; else failed++;
if (test('denies rm -rf in brace group with nested () containing }', () => {
expectDestructiveDeny('{ (echo "}"); rm -rf /tmp/junk; }',
'brace + () containing }');
})) passed++; else failed++;
if (test('denies rm -rf in brace group with $() body containing }', () => {
expectDestructiveDeny('{ x=$(echo a}b); rm -rf /tmp/junk; }',
'brace + $() body with }');
})) passed++; else failed++;
if (test('denies rm -rf when token like foo{ appears before brace group close', () => {
// tokens like `foo{` are not reserved-word `{` (no boundary,
// no whitespace after) — must not bump nested-depth and so
// must not delay brace-group close
expectDestructiveDeny('{ echo foo{bar; rm -rf /tmp/junk; }',
'foo{ token inside brace body');
})) passed++; else failed++;
// Cleanup only the temp directory created by this test file.
try {
if (fs.existsSync(stateDir)) {