Compare commits

...

7 Commits

  • test(hooks): regression coverage for round 1 review fixes
    9 new cases locking in the behavior added by the previous two
    commits. Each was verified to fail before the fix and pass after.
    
    Greptile — quote-aware depth counting:
      - blocks $(echo ")"; (npm run dev))
      - blocks (echo ")"; npm run dev)
      - allows $(echo "(npm run dev)") — () inside double-quoted body is literal
    
    Greptile — brace groups:
      - blocks { npm run dev; }
      - blocks echo hi && { npm run dev; }
      - allows {npm run dev} — bash brace-group syntax requires a space after {
    
    CodeRabbit — missing package-manager variants:
      - blocks yarn run dev (yarn 1.x convention)
      - blocks bun dev (bun bare form)
    
    CodeRabbit nitpick — symmetric quote test:
      - blocks echo "$(npm run dev)" — double-quoted substitution still substitutes
    
    The `{npm run dev}` allow case is intentional: bash treats `{` as
    a reserved word only when followed by whitespace. The pre-fix code
    already passed this through, but until now we never asserted it,
    so a future change to brace handling could silently start blocking
    literal `{npm` tokens.
  • fix(hooks): cover brace groups + yarn-run/bun-bare dev variants
    Two false-negatives surfaced in PR #1889 review:
    
    1. Brace-group bypass (Greptile).
       `{ npm run dev; }` evaluates the dev command in the *current*
       shell — semantically distinct from `( ... )` but with the same
       effect for this hook. `splitShellSegments` correctly cleaves the
       group at `;` into `["{ npm run dev", "}"]`, but the first segment's
       leading token under `readToken` is the bare `{`, which was not in
       `DEV_COMMAND_WORDS`, so the dev-pattern check was skipped.
    
       Fix: treat `{` and `}` as no-op tokens in `getLeadingCommandWord`
       so we keep walking to the real command word. Matches how shell
       itself parses brace groups (the braces are reserved words, not
       commands). Bash requires a space after `{` and a terminator before
       `}` for an actual group, so `{npm run dev}` correctly remains
       allowed (single token `{npm`, not in `DEV_COMMAND_WORDS`).
    
    2. Missing yarn-run / bun-bare variants (CodeRabbit).
       Both `yarn dev` *and* `yarn run dev` are valid (the latter is what
       `package.json` actually wires `dev` to under yarn 1.x). The same
       `(run )?` symmetry applies to bun. The previous `DEV_PATTERN` only
       matched `yarn\s+dev` and `bun\s+run\s+dev`, allowing the cross
       forms to pass through silently.
    
       Fix: `yarn(?:\s+run)?\s+dev` and `bun(?:\s+run)?\s+dev` — same
       shape `pnpm(?:\s+run)?\s+dev` was already using.
    
    Verified after this commit (every form now exits 2):
    
      { npm run dev; }
      { npm run dev ; }
      echo hi && { npm run dev; }
      ({ npm run dev; })
      $( { npm run dev; } )
      yarn run dev
      bun dev
    
    Verified still allowed (no regression):
    
      echo "{ npm run dev; }"   # literal inside double quotes
      {npm run dev}             # not a brace group per bash syntax
  • fix(lib): track quote state inside command-substitution depth counters
    Greptile flagged a bypass in PR #1889: `$(echo ")"; (npm run dev))`
    threaded the depth-counting loops in `extractCommandSubstitutions`
    and `extractSubshellGroups` to terminate early, because a literal `)`
    inside double quotes was treated as a real closing paren. The
    truncated body then ended in a dangling `"` that toggled `inDouble`
    in the outer scan, masking the subsequent `(npm run dev)` group from
    extraction.
    
    Reproduced (before this commit) by piping the synthetic PreToolUse
    payload `{"tool_input":{"command":"$(echo \")\"; (npm run dev))"}}`
    into `scripts/hooks/pre-bash-dev-server-block.js` and observing
    exit 0 (allow) where the dev pattern is clearly present.
    
    Fix: each `$(...)` and `(...)` body loop now tracks its own
    single/double quote state and only treats `(` / `)` as depth
    delimiters when outside quotes. The quoted `)` no longer closes
    the group early, the body now extends to the real closing paren,
    and the outer scan's quote state remains untouched.
    
    After this commit:
      $ echo '{"tool_input":{"command":"$(echo \")\"; (npm run dev))"}}' \
          | node scripts/hooks/pre-bash-dev-server-block.js; echo $?
      2
    
    The symmetric form `$(echo "(npm run dev)")` correctly remains
    allowed (bash does not honor `(...)` inside double quotes).
  • test(hooks): regression coverage for dev-server-block subshell bypass
    Lock in the behavior added by the previous commit. Each new case was
    verified to fail before the fix and pass after.
    
    Bypasses now blocked (exit 2):
    - \$(npm run dev)              command substitution
    - \`npm run dev\`              backtick substitution
    - echo \$(npm run dev)         substitution inside an argument
    - (npm run dev)               plain subshell group
    - \$(echo a; npm run dev)      substitution containing a sequenced segment
    - (pnpm dev)                  plain subshell group, alt package manager
    
    Allow cases — explicitly proven NOT to regress so the fix doesn't
    over-block legitimate uses:
    - (tmux new-session -d -s dev "npm run dev")   tmux launcher inside ()
    - git commit -m '(npm run dev)'                literal in single quotes
    - echo "(npm run dev)"                         literal in double quotes
      (bash does NOT subshell () inside double quotes)
    - git commit -m '\$(npm run dev) fix'          literal in single quotes
    
    Single- and double-quote allow cases are important: they distinguish a
    real subshell construct from one that's just text inside a string,
    which is what `extractSubshellGroups` / `extractCommandSubstitutions`
    quote-awareness is for.
  • fix(hooks): close subshell bypass in pre-bash-dev-server-block
    Before this commit the dev-server-block hook ran the leading-command
    and dev-pattern check only against the top-level segments returned by
    `splitShellSegments`, which doesn't split on `$(...)`, backticks, or
    plain `(...)`. That left the policy bypassable by wrapping a dev
    command in any of those constructs:
    
      $(npm run dev)
      `npm run dev`
      echo $(npm run dev)
      (npm run dev)
    
    Each verified by piping a synthetic PreToolUse payload into the hook
    on this branch: every form above returned exit 0 (allow) where a plain
    `npm run dev` correctly returned exit 2 (block).
    
    Fix: expand the check space before running the leading-command rule.
    A small BFS walks the raw command, harvesting bodies from
    `extractCommandSubstitutions` (`$(...)` and backticks) and from
    `extractSubshellGroups` (plain `(...)`), then splits each harvested
    body through `splitShellSegments` and feeds the result into the
    existing `isBlockedDevSegment` check.
    
    This preserves every existing allow case (`tmux new-session -d -s dev
    "npm run dev"`, quoted-string mentions like `git commit -m "npm run
    dev fix"`, `echo hi`) because the leading-command rule is unchanged —
    only the set of segments it runs against grew.
    
    Known limitation, not fixed here: `eval "$(echo npm run dev)"` still
    slips through because the substitution body's leading command is
    `echo`, and statically modeling echo's output to recover the executed
    command is out of scope. The same class affects `gateguard-fact-force`
    (via `eval "$(echo rm -rf /)"` etc.) and is best addressed in both
    hooks together as a follow-up rather than as a one-off here.
  • feat(lib): add extractSubshellGroups for plain (...) subshells
    `extractCommandSubstitutions` only walks `$(...)` and backticks — the two
    shell constructs whose bodies are captured as strings. Bash also has
    plain `(...)` subshells (e.g. `(npm run dev)`), where the body executes
    in a child shell but is not value-captured. Our PreToolUse hooks need
    to peer inside those too, because a `(...)` group bypasses the
    top-level segment splitter just like `$(...)` does.
    
    This commit adds a sibling extractor with the same conventions as
    `extractCommandSubstitutions`:
    
    - single quotes literal — `'(npm run dev)'` is a string, ignored
    - double quotes literal for parens — `"(npm run dev)"` is a string
      (bash only honors `$(...)`, not bare `(...)`, inside double quotes)
    - skips `$(...)` and backtick spans so we don't double-extract
      bodies the other helper already handles
    - recurses into its own bodies for nested groups
    
    No consumer yet; the next commit wires both extractors into
    `scripts/hooks/pre-bash-dev-server-block.js` to close the subshell
    bypass surface.
  • feat(lib): extract shell command-substitution parser to shared lib
    Extract the `extractCommandSubstitutions` function originally
    introduced in scripts/hooks/gateguard-fact-force.js (PR #1853
    round 2) into scripts/lib/shell-substitution.js so other PreToolUse
    hooks can reuse the same single-quote-aware, double-quote-aware,
    nested-subshell-aware parser without duplicating it.
    
    No behavior change in this commit — the function body is copied
    verbatim and exposed via `module.exports`. The next commit wires it
    into scripts/hooks/pre-bash-dev-server-block.js to close that hook's
    own subshell-bypass holes.
    
    gateguard-fact-force.js still defines its own private copy of the
    function; consolidating both call sites onto this shared lib is a
    follow-up worth doing once this PR lands, but is intentionally out
    of scope here to keep the diff focused on the dev-server-block fix.
3 changed files with 399 additions and 11 deletions
+49 -11
View File
@@ -4,6 +4,10 @@
const MAX_STDIN = 1024 * 1024;
const path = require('path');
const { splitShellSegments } = require('../lib/shell-split');
const {
extractCommandSubstitutions,
extractSubshellGroups
} = require('../lib/shell-substitution');
const DEV_COMMAND_WORDS = new Set([
'npm',
@@ -123,6 +127,8 @@ function getLeadingCommandWord(segment) {
continue;
}
if (token === '{' || token === '}') continue;
if (/^[A-Za-z_][A-Za-z0-9_]*=.*/.test(token)) continue;
const normalizedToken = normalizeCommandWord(token);
@@ -154,23 +160,55 @@ process.stdin.on('data', chunk => {
}
});
const TMUX_LAUNCHER = /^\s*tmux\s+(new|new-session|new-window|split-window)\b/;
const DEV_PATTERN = /\b(npm\s+run\s+dev|pnpm(?:\s+run)?\s+dev|yarn(?:\s+run)?\s+dev|bun(?:\s+run)?\s+dev)\b/;
/**
* Collect every command-line segment we should evaluate. Returns the top-level
* segments first, then segments harvested from `$(...)` / backtick command
* substitutions and plain `(...)` subshell groups, recursively.
*
* Without this expansion the leading-command and dev-pattern check below only
* sees the outermost command, so wrappers like `$(npm run dev)` and
* `(npm run dev)` (which still spawn a dev server) sneak past.
*/
function collectCheckSegments(cmd) {
const segments = [...splitShellSegments(cmd)];
const queue = [cmd];
const seen = new Set();
while (queue.length) {
const current = queue.shift();
if (seen.has(current)) continue;
seen.add(current);
for (const body of extractCommandSubstitutions(current)) {
for (const seg of splitShellSegments(body)) segments.push(seg);
queue.push(body);
}
for (const body of extractSubshellGroups(current)) {
for (const seg of splitShellSegments(body)) segments.push(seg);
queue.push(body);
}
}
return segments;
}
function isBlockedDevSegment(segment) {
const commandWord = getLeadingCommandWord(segment);
if (!commandWord || !DEV_COMMAND_WORDS.has(commandWord)) return false;
return DEV_PATTERN.test(segment) && !TMUX_LAUNCHER.test(segment);
}
process.stdin.on('end', () => {
try {
const input = JSON.parse(raw);
const cmd = String(input.tool_input?.command || '');
if (process.platform !== 'win32') {
const segments = splitShellSegments(cmd);
const tmuxLauncher = /^\s*tmux\s+(new|new-session|new-window|split-window)\b/;
const devPattern = /\b(npm\s+run\s+dev|pnpm(?:\s+run)?\s+dev|yarn\s+dev|bun\s+run\s+dev)\b/;
const hasBlockedDev = segments.some(segment => {
const commandWord = getLeadingCommandWord(segment);
if (!commandWord || !DEV_COMMAND_WORDS.has(commandWord)) {
return false;
}
return devPattern.test(segment) && !tmuxLauncher.test(segment);
});
const segments = collectCheckSegments(cmd);
const hasBlockedDev = segments.some(isBlockedDevSegment);
if (hasBlockedDev) {
console.error('[Hook] BLOCKED: Dev server must run in tmux for log access');
+246
View File
@@ -0,0 +1,246 @@
'use strict';
/**
* Extract executable command-substitution bodies from a shell line.
*
* Single quotes are literal, so substitutions inside them are ignored;
* double quotes still permit substitutions, so those bodies are scanned
* before quoted text is stripped. Returns each substitution body plus
* any nested substitutions discovered recursively.
*
* Originally introduced in scripts/hooks/gateguard-fact-force.js
* (PR #1853 round 2). Extracted to a shared lib so other PreToolUse
* hooks that need the same "scan inside `$(...)` and backticks"
* behavior can reuse it without duplicating the parser.
*
* @param {string} input
* @returns {string[]}
*/
function extractCommandSubstitutions(input) {
const source = String(input || '');
const substitutions = [];
let inSingle = false;
let inDouble = false;
for (let i = 0; i < source.length; i++) {
const ch = source[i];
const prev = source[i - 1];
if (ch === '\\' && !inSingle) {
i += 1;
continue;
}
if (ch === "'" && !inDouble && prev !== '\\') {
inSingle = !inSingle;
continue;
}
if (ch === '"' && !inSingle && prev !== '\\') {
inDouble = !inDouble;
continue;
}
if (inSingle) {
continue;
}
if (ch === '`') {
let body = '';
i += 1;
while (i < source.length) {
const inner = source[i];
if (inner === '\\') {
body += inner;
if (i + 1 < source.length) {
body += source[i + 1];
i += 2;
continue;
}
}
if (inner === '`') {
break;
}
body += inner;
i += 1;
}
if (body.trim()) {
substitutions.push(body);
substitutions.push(...extractCommandSubstitutions(body));
}
continue;
}
if (ch === '$' && source[i + 1] === '(') {
let depth = 1;
let body = '';
let bodyInSingle = false;
let bodyInDouble = false;
i += 2;
while (i < source.length && depth > 0) {
const inner = source[i];
const innerPrev = source[i - 1];
if (inner === '\\' && !bodyInSingle) {
body += inner;
if (i + 1 < source.length) {
body += source[i + 1];
i += 2;
continue;
}
}
if (inner === "'" && !bodyInDouble && innerPrev !== '\\') {
bodyInSingle = !bodyInSingle;
} else if (inner === '"' && !bodyInSingle && innerPrev !== '\\') {
bodyInDouble = !bodyInDouble;
} else if (!bodyInSingle && !bodyInDouble) {
if (inner === '(') {
depth += 1;
} else if (inner === ')') {
depth -= 1;
if (depth === 0) {
break;
}
}
}
body += inner;
i += 1;
}
if (body.trim()) {
substitutions.push(body);
substitutions.push(...extractCommandSubstitutions(body));
}
}
}
return substitutions;
}
/**
* Extract bodies of plain `(...)` subshell groups.
*
* Bash treats `(npm run dev)` as a subshell that executes its contents, but
* the regex-light segment splitters used by our PreToolUse hooks don't peer
* inside those parens. This helper finds top-level `(...)` groups (skipping
* `$(...)` command substitutions and backticks, which `extractCommandSubstitutions`
* already covers) and returns each body, recursing for nested groups.
*
* Quote semantics:
* - Single quotes are literal: `'( ... )'` is a string, not a subshell.
* - Double quotes are literal *for parens*: `"( ... )"` is a string too —
* bash only honors `$( )` inside double quotes, not bare `( )`.
*
* @param {string} input
* @returns {string[]}
*/
function extractSubshellGroups(input) {
const source = String(input || '');
const groups = [];
let inSingle = false;
let inDouble = false;
for (let i = 0; i < source.length; i++) {
const ch = source[i];
const prev = source[i - 1];
if (ch === '\\' && !inSingle) {
i += 1;
continue;
}
if (ch === "'" && !inDouble && prev !== '\\') {
inSingle = !inSingle;
continue;
}
if (ch === '"' && !inSingle && prev !== '\\') {
inDouble = !inDouble;
continue;
}
if (inSingle || inDouble) {
continue;
}
if (ch === '$' && source[i + 1] === '(') {
let depth = 1;
let skipInSingle = false;
let skipInDouble = false;
i += 2;
while (i < source.length && depth > 0) {
const inner = source[i];
const innerPrev = source[i - 1];
if (inner === '\\' && !skipInSingle) {
i += 2;
continue;
}
if (inner === "'" && !skipInDouble && innerPrev !== '\\') {
skipInSingle = !skipInSingle;
} else if (inner === '"' && !skipInSingle && innerPrev !== '\\') {
skipInDouble = !skipInDouble;
} else if (!skipInSingle && !skipInDouble) {
if (inner === '(') depth += 1;
else if (inner === ')') depth -= 1;
}
i += 1;
}
i -= 1;
continue;
}
if (ch === '`') {
i += 1;
while (i < source.length && source[i] !== '`') {
if (source[i] === '\\' && i + 1 < source.length) {
i += 2;
continue;
}
i += 1;
}
continue;
}
if (ch === '(') {
let depth = 1;
let body = '';
let bodyInSingle = false;
let bodyInDouble = false;
i += 1;
while (i < source.length && depth > 0) {
const inner = source[i];
const innerPrev = source[i - 1];
if (inner === '\\' && !bodyInSingle) {
body += inner;
if (i + 1 < source.length) {
body += source[i + 1];
i += 2;
continue;
}
}
if (inner === "'" && !bodyInDouble && innerPrev !== '\\') {
bodyInSingle = !bodyInSingle;
} else if (inner === '"' && !bodyInSingle && innerPrev !== '\\') {
bodyInDouble = !bodyInDouble;
} else if (!bodyInSingle && !bodyInDouble) {
if (inner === '(') {
depth += 1;
} else if (inner === ')') {
depth -= 1;
if (depth === 0) {
break;
}
}
}
body += inner;
i += 1;
}
if (body.trim()) {
groups.push(body);
groups.push(...extractSubshellGroups(body));
}
}
}
return groups;
}
module.exports = { extractCommandSubstitutions, extractSubshellGroups };
@@ -89,6 +89,110 @@ function runTests() {
assert.strictEqual(result.code, 0, `Expected exit code 0, got ${result.code}`);
}) ? passed++ : failed++);
// --- Subshell bypass regression (issue: dev server slipped past via $(), ``, ()) ---
if (!isWindows) {
(test('blocks $(npm run dev) — command substitution', () => {
const result = runScript('$(npm run dev)');
assert.strictEqual(result.code, 2, `Expected exit code 2, got ${result.code}`);
assert.ok(result.stderr.includes('BLOCKED'), 'expected BLOCKED in stderr');
}) ? passed++ : failed++);
(test('blocks `npm run dev` — backtick substitution', () => {
const result = runScript('`npm run dev`');
assert.strictEqual(result.code, 2, `Expected exit code 2, got ${result.code}`);
}) ? passed++ : failed++);
(test('blocks echo $(npm run dev) — substitution nested in argument', () => {
const result = runScript('echo $(npm run dev)');
assert.strictEqual(result.code, 2, `Expected exit code 2, got ${result.code}`);
}) ? passed++ : failed++);
(test('blocks (npm run dev) — plain subshell group', () => {
const result = runScript('(npm run dev)');
assert.strictEqual(result.code, 2, `Expected exit code 2, got ${result.code}`);
}) ? passed++ : failed++);
(test('blocks $(echo a; npm run dev) — substitution with sequenced segments', () => {
const result = runScript('$(echo a; npm run dev)');
assert.strictEqual(result.code, 2, `Expected exit code 2, got ${result.code}`);
}) ? passed++ : failed++);
(test('blocks (pnpm dev) — plain subshell group with pnpm', () => {
const result = runScript('(pnpm dev)');
assert.strictEqual(result.code, 2, `Expected exit code 2, got ${result.code}`);
}) ? passed++ : failed++);
(test('allows tmux launcher inside subshell wrapping (exit code 0)', () => {
const result = runScript('(tmux new-session -d -s dev "npm run dev")');
assert.strictEqual(result.code, 0, `Expected exit code 0, got ${result.code}`);
}) ? passed++ : failed++);
(test('allows single-quoted "(npm run dev)" — literal string, not a subshell', () => {
const result = runScript("git commit -m '(npm run dev)'");
assert.strictEqual(result.code, 0, `Expected exit code 0, got ${result.code}`);
}) ? passed++ : failed++);
(test('allows double-quoted "(npm run dev)" — literal in double quotes (bash does not subshell)', () => {
const result = runScript('echo "(npm run dev)"');
assert.strictEqual(result.code, 0, `Expected exit code 0, got ${result.code}`);
}) ? passed++ : failed++);
(test("allows single-quoted '$(npm run dev)' — literal string, no substitution", () => {
const result = runScript("git commit -m '$(npm run dev) fix'");
assert.strictEqual(result.code, 0, `Expected exit code 0, got ${result.code}`);
}) ? passed++ : failed++);
}
// --- Round 1 review fixes (Greptile + CodeRabbit on PR #1889) ---
if (!isWindows) {
(test('blocks $(echo ")"; (npm run dev)) — quoted ) does not terminate $() early', () => {
const result = runScript('$(echo ")"; (npm run dev))');
assert.strictEqual(result.code, 2, `Expected exit code 2, got ${result.code}`);
}) ? passed++ : failed++);
(test('blocks (echo ")"; npm run dev) — quoted ) does not terminate (...) early', () => {
const result = runScript('(echo ")"; npm run dev)');
assert.strictEqual(result.code, 2, `Expected exit code 2, got ${result.code}`);
}) ? passed++ : failed++);
(test('allows $(echo "(npm run dev)") — () inside double-quoted substitution body is literal', () => {
const result = runScript('$(echo "(npm run dev)")');
assert.strictEqual(result.code, 0, `Expected exit code 0, got ${result.code}`);
}) ? passed++ : failed++);
(test('blocks { npm run dev; } — brace group runs in current shell', () => {
const result = runScript('{ npm run dev; }');
assert.strictEqual(result.code, 2, `Expected exit code 2, got ${result.code}`);
}) ? passed++ : failed++);
(test('blocks echo hi && { npm run dev; } — brace group after &&', () => {
const result = runScript('echo hi && { npm run dev; }');
assert.strictEqual(result.code, 2, `Expected exit code 2, got ${result.code}`);
}) ? passed++ : failed++);
(test('allows {npm run dev} — bash requires space after { to form a group', () => {
const result = runScript('{npm run dev}');
assert.strictEqual(result.code, 0, `Expected exit code 0, got ${result.code}`);
}) ? passed++ : failed++);
(test('blocks yarn run dev — yarn 1.x convention', () => {
const result = runScript('yarn run dev');
assert.strictEqual(result.code, 2, `Expected exit code 2, got ${result.code}`);
}) ? passed++ : failed++);
(test('blocks bun dev — bun bare form', () => {
const result = runScript('bun dev');
assert.strictEqual(result.code, 2, `Expected exit code 2, got ${result.code}`);
}) ? passed++ : failed++);
(test('blocks "$(npm run dev)" — double-quoted substitution still substitutes', () => {
const result = runScript('echo "$(npm run dev)"');
assert.strictEqual(result.code, 2, `Expected exit code 2, got ${result.code}`);
}) ? passed++ : failed++);
}
// --- Edge cases ---
(test('empty/invalid input passes through (exit code 0)', () => {