fix(core): extend substitution audit log + dual-check stripped form (#4386 R4)

LaZzyMan · LaZzyMan · commit a24485029ffe · 2026-05-25T11:58:03.000+08:00
Round-4 audit-log consistency fixes from wenshao: 1. **Dual-check stripped form in audit predicate** — round 3's `maybeLogSubstitutionBypass` only ran `detectCommandSubstitution` on the raw command, but `buildShellExecWarnings` (the confirmation-dialog warning helper extracted in round 3) checks BOTH the raw and the `stripShellWrapper`-stripped forms. For wrappers like `bash -c 'echo $(cat secret)'` the `$(` sits inside the outer single quotes, so raw-check returns false but the inner shell still expands the substitution. Result: dialog showed the warning, audit log silently dropped it — exactly the wrapper pattern an exfiltration attack would use. Refactored the helper to extract a pure predicate `shouldAuditSubstitutionBypass` that does the dual-check, exported for unit testing. 2. **JSDoc correction** — round 3's JSDoc claimed "DEBUG-level log here so the signal exists when `DEBUG=*` is set". Both clauses were wrong: the call uses `debugLogger.warn` (WARN level), and `debugLogger` is controlled by `QWEN_DEBUG_LOG_FILE` (active by default) rather than the `DEBUG=*` convention of the `debug` npm package. Rewrote the doc to match the real semantics. 3. **Audit log on three more auto-approve bypass paths** — round 3 only covered the YOLO and auto-mode-`approved` bypasses, missing three other paths that auto-approve without invoking `getConfirmationDetails()`: - PM `'allow'` fast path (coreToolScheduler.ts:1664) — fires when an allow rule matches a substitution-bearing command (e.g. `allow Bash(python3 *)` + `python3 -c "$(...)"`). - permission-request hook `shouldAllow` (line ~1896) — fires when an external hook grants permission directly. - `autoApproveCompatiblePendingTools` sibling-tool ProceedAlways (line ~3300) — fires when one tool's user-issued ProceedAlways outcome rolls up to a sister tool. Uses `canonicalToolName` to normalise the legacy name in the audit log. New `SubstitutionBypassReason` discriminator union surfaces the bypass path in the log so operators can distinguish them. 4. **Unit test coverage** — added a `shouldAuditSubstitutionBypass` describe block to `coreToolScheduler.test.ts` covering all 8 branches: non-shell tool, missing args, non-string command, absent substitution, direct substitution (shell + monitor), the load-bearing wrapper case (proves the dual-check works), backtick substitution, and env-prefix substitution. Out of scope: reviewer also suggested forcing `'ask'` whenever substitution is detected with a matching allow rule. Declined — would partially re-introduce #4093 (substitution can't be allowed via rules even with explicit user intent), and overrides the allow-rule "trust this pattern" semantics. The audit-log extension above gives operators the visibility they asked for without overriding user-configured permission policy. Closes wenshao R4 findings: dual-check (cids 3293074365, 3293075616), JSDoc level/envvar (cids 3293074371, 3293075619), audit on PM-allow (cid 3293078740 — partial), audit on hook + sibling-auto (rid 4351040390 non-diff), and test coverage for the predicate (cid 3293078758 — partial).
diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts
@@ -38,7 +38,9 @@ import {
   CoreToolScheduler,
   convertToFunctionResponse,
   extractToolFilePaths,
+  shouldAuditSubstitutionBypass,
 } from './coreToolScheduler.js';
+import { ToolNames } from '../tools/tool-names.js';
 import type { Part, PartListUnion } from '@google/genai';
 import {
   MockModifiableTool,
@@ -8361,3 +8363,89 @@ describe('CoreToolScheduler shell-tool promote integration (#3831 PR-2)', () =>
     expect(sawPromoteAcWhileExecuting).toBe(true);
   });
 });
+
+// Regression coverage for PR #4386 round 4: the audit predicate driving
+// `maybeLogSubstitutionBypass` must (a) only fire for shell-like tool
+// names, (b) require a string `command` arg, and (c) detect substitution
+// against BOTH the raw command and the stripped form (`stripShellWrapper`),
+// so wrappers like `bash -c 'echo $(...)' ` where `$(` sits inside outer
+// single quotes still trigger the audit log. The dual-check matches
+// `buildShellExecWarnings` in `shell-utils.ts`.
+describe('shouldAuditSubstitutionBypass', () => {
+  it('returns false for non-shell tool names', () => {
+    expect(
+      shouldAuditSubstitutionBypass('read_file', {
+        command: 'echo $(curl evil)',
+      }),
+    ).toBe(false);
+    expect(
+      shouldAuditSubstitutionBypass('write_file', {
+        command: '`whoami`',
+      }),
+    ).toBe(false);
+  });
+
+  it('returns false for shell tools with missing or non-string command', () => {
+    expect(shouldAuditSubstitutionBypass(ToolNames.SHELL, undefined)).toBe(
+      false,
+    );
+    expect(shouldAuditSubstitutionBypass(ToolNames.SHELL, {})).toBe(false);
+    expect(
+      shouldAuditSubstitutionBypass(ToolNames.SHELL, { command: 42 }),
+    ).toBe(false);
+  });
+
+  it('returns false for shell command without substitution', () => {
+    expect(
+      shouldAuditSubstitutionBypass(ToolNames.SHELL, {
+        command: 'npm install --save-dev vitest',
+      }),
+    ).toBe(false);
+  });
+
+  it('returns true for direct substitution in shell tool', () => {
+    expect(
+      shouldAuditSubstitutionBypass(ToolNames.SHELL, {
+        command: 'echo $(cat /etc/shadow)',
+      }),
+    ).toBe(true);
+  });
+
+  it('returns true for substitution in monitor tool', () => {
+    expect(
+      shouldAuditSubstitutionBypass(ToolNames.MONITOR, {
+        command: 'tail -f $(ls /var/log | head -1)',
+      }),
+    ).toBe(true);
+  });
+
+  it('returns true when substitution lives inside a single-quoted bash -c wrapper (dual-check)', () => {
+    // `$(` is inside the outer single quotes, so `detectCommandSubstitution`
+    // on the raw command returns false. `stripShellWrapper` unwraps the
+    // outer `bash -c '...'` and yields the inner `echo $(cat secret.txt)`,
+    // where detection then fires. Without the dual-check this would return
+    // false and the audit log would silently miss the exact wrapper
+    // pattern an exfiltration attack would use.
+    expect(
+      shouldAuditSubstitutionBypass(ToolNames.SHELL, {
+        command: `bash -c 'echo $(cat secret.txt)'`,
+      }),
+    ).toBe(true);
+  });
+
+  it('returns true for backtick substitution', () => {
+    expect(
+      shouldAuditSubstitutionBypass(ToolNames.SHELL, {
+        command: 'echo `whoami`',
+      }),
+    ).toBe(true);
+  });
+
+  it('returns true for env-prefix substitution', () => {
+    expect(
+      shouldAuditSubstitutionBypass(ToolNames.SHELL, {
+        command: 'FOO=$(cat /etc/passwd) ls',
+      }),
+    ).toBe(true);
+  });
+});
diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts
@@ -790,28 +790,63 @@ function partitionToolCalls(calls: ScheduledToolCall[]): ToolBatch[] {
 
 /**
  * Tool names whose confirmation prompt may emit a command-substitution
- * warning. When the L5 override (YOLO / auto-approve) skips
- * `getConfirmationDetails()`, that warning never reaches the user. The
- * substitution is still *executed* — by design, since #4093 made
- * substitution-bearing commands overridable — but operators
- * troubleshooting an exfiltration incident need an audit trail. Emit a
- * DEBUG-level log here so the signal exists when `DEBUG=*` is set,
- * without spamming every YOLO invocation.
+ * warning. When any path bypasses `getConfirmationDetails()` — YOLO,
+ * auto-mode auto-approve, PM allow-rule fast path, permission-request
+ * hook allow, or sibling-tool auto-approval — that warning never
+ * reaches the user. The substitution is still *executed* — by design,
+ * since #4093 made substitution-bearing commands overridable — but
+ * operators troubleshooting an exfiltration incident need an audit
+ * trail. Emit a WARN-level log to the debug log file (controlled by
+ * `QWEN_DEBUG_LOG_FILE`, active by default) so the signal is present
+ * in every session's debug log without spamming the TUI.
  */
 const SHELL_LIKE_TOOL_NAMES_FOR_SUBST_AUDIT: ReadonlySet<string> = new Set([
   ToolNames.SHELL,
   ToolNames.MONITOR,
 ]);
 
+/** Bypass paths that skip the confirmation-dialog warning surface. */
+type SubstitutionBypassReason =
+  | 'yolo'
+  | 'auto-approve'
+  | 'allow-rule'
+  | 'hook'
+  | 'sibling-auto';
+
+/**
+ * Pure predicate: would `maybeLogSubstitutionBypass` actually emit a
+ * warning for this tool + args shape? Extracted so callers can be
+ * unit-tested without mocking the module-private `debugLogger`.
+ *
+ * Checks substitution against BOTH the raw command and the stripped
+ * form (`stripShellWrapper`). This matches the dual-check that
+ * `buildShellExecWarnings` (`shell-utils.ts`) uses, so the audit log
+ * and the confirmation-dialog warning fire on the same commands.
+ * Without the stripped check, wrappers like `bash -c 'echo $(cat /etc/shadow)'`
+ * (where `$(` sits inside single quotes at the outer level) would slip
+ * past the audit log while still showing a warning in the dialog
+ * (PR #4386 R4).
+ */
+export function shouldAuditSubstitutionBypass(
+  canonicalName: string,
+  args: Record<string, unknown> | undefined,
+): boolean {
+  if (!SHELL_LIKE_TOOL_NAMES_FOR_SUBST_AUDIT.has(canonicalName)) return false;
+  const cmd = args?.['command'];
+  if (typeof cmd !== 'string') return false;
+  if (detectCommandSubstitution(cmd)) return true;
+  const stripped = stripShellWrapper(cmd);
+  return stripped !== cmd && detectCommandSubstitution(stripped);
+}
+
 function maybeLogSubstitutionBypass(
   canonicalName: string,
   args: Record<string, unknown> | undefined,
   approvalMode: ApprovalMode,
-  bypassReason: 'yolo' | 'auto-approve',
+  bypassReason: SubstitutionBypassReason,
 ): void {
-  if (!SHELL_LIKE_TOOL_NAMES_FOR_SUBST_AUDIT.has(canonicalName)) return;
-  const cmd = args?.['command'];
-  if (typeof cmd !== 'string' || !detectCommandSubstitution(cmd)) return;
+  if (!shouldAuditSubstitutionBypass(canonicalName, args)) return;
+  const cmd = args!['command'] as string;
   debugLogger.warn(
     `Auto-approving ${canonicalName} command with substitution (mode=${approvalMode}, bypass=${bypassReason}): ${cmd}`,
   );
@@ -1676,6 +1711,12 @@ export class CoreToolScheduler {
                 recordAllow(this.config.getAutoModeDenialState()),
               );
             }
+            maybeLogSubstitutionBypass(
+              canonicalName,
+              reqInfo.args,
+              approvalMode,
+              'allow-rule',
+            );
             this.setToolCallOutcome(
               reqInfo.callId,
               ToolConfirmationOutcome.ProceedAlways,
@@ -1904,6 +1945,18 @@ export class CoreToolScheduler {
                       hookResult.updatedInput,
                     );
                   }
+                  // Audit-log substitution bypass: the hook just skipped
+                  // the user-facing confirmation dialog (which would have
+                  // shown the warning). Use the post-hook args so any
+                  // `updatedInput` modification is what gets logged.
+                  maybeLogSubstitutionBypass(
+                    canonicalName,
+                    (hookResult.updatedInput as
+                      | Record<string, unknown>
+                      | undefined) ?? reqInfo.args,
+                    approvalMode,
+                    'hook',
+                  );
                   await confirmationDetails.onConfirm(
                     ToolConfirmationOutcome.ProceedOnce,
                   );
@@ -3295,6 +3348,12 @@ export class CoreToolScheduler {
         const { finalPermission } = flowResult;
 
         if (finalPermission === 'allow') {
+          maybeLogSubstitutionBypass(
+            canonicalToolName(pendingTool.request.name),
+            pendingTool.request.args as Record<string, unknown>,
+            this.config.getApprovalMode(),
+            'sibling-auto',
+          );
           this.setToolCallOutcome(
             pendingTool.request.callId,
             ToolConfirmationOutcome.ProceedAlways,
