fix(shell): harden read-only Git approval against executable config hooks

docs/design/2026-08-08-read-only-git-config-safety.md

@@ -1,9 +1,28 @@
 # Read-only Git config safety
 
-Issue #8575 proves two repository-local configuration paths that turn an otherwise read-only command into program execution: `diff.external` for `git diff`, and `core.fsmonitor` for `git status`.
+Repository-local Git configuration can turn commands that look read-only into program execution. The classifier therefore treats a read-only Git invocation as safe only when the repository configuration cannot activate an executable helper for that invocation.
 
-The classifier will ask only for those reproduced command/config pairs. It will query effective local and worktree values through `git config --includes --show-scope`, so Git owns config syntax, include handling, precedence, and worktree behavior. Git probe and parse errors fail closed; a cwd that cannot be entered has no config execution path.
+The config probe uses `git config --includes --show-scope --null --get-regexp`, so Git owns config parsing, includes, precedence, and worktree behavior. Only effective `local` and `worktree` executable values are considered repository-controlled execution hooks; probe/parse failures fail closed. Empty helper values are ignored. Boolean `core.fsmonitor` and pager values keep their non-command semantics. For Git boolean switches that activate execution paths, explicit false values and numeric zero are false while other values fail closed as true/risky, covering Git-accepted spellings such as `-1`, `+1`, `0x1`, and unit-suffixed non-zero values.
 
-Commands that change directory before the relevant Git command also ask. The classifier will not simulate shell cwd state or resolve `git -C`; the latter is already outside the read-only allowlist.
+The covered execution paths are:
 
-Other execution-bearing Git settings are follow-up work only after an independent reproduction identifies the affected read-only subcommand.
+- `diff.external` and `diff.<driver>.command` (including an empty driver subsection such as `diff..command`) for `git diff`.
+- `diff.<driver>.textconv` for `git blame`, `git diff`, `git log`, and `git show`.
+- command-form `core.fsmonitor` for index/worktree consumers. Broad worktree/index risks use a default-deny consumer model so newly discovered Git flags do not silently create new auto-approval entrances; this covers, among others, `git blame`, `git diff`, worktree `git grep`, `git ls-files`, `git status`, and dirty/broken `git describe` paths. Proven non-consumers are maintained through one shared safe-subcommand set rather than duplicated lists.
+- `filter.<driver>.clean` / `filter.<driver>.process` for worktree-content consumers. These share the same default-deny/non-consumer model as fsmonitor rather than a list of only currently known flag spellings, covering `git ls-files` option abbreviations and dirty/broken `git describe` paths.
+- command-form `core.pager` / `pager.<cmd>` for Git commands otherwise classified read-only.
+- repository-local/worktree `gpg.program` / `gpg.<format>.program` when effective Git configuration requests signature verification for `git log` or `git show`. Activation is resolved from effective `log.showSignature`, `format.pretty`, and `pretty.<name>` values, so a user/global pretty format containing `%G*` still protects against a repository-local GPG program while a user/global GPG program alone is not treated as repository-controlled.
+- partial-clone/promisor state (`extensions.partialClone`, `remote.<name>.promisor`, and `remote.<name>.partialCloneFilter`). Promisor repositories are fail-closed for every otherwise read-only Git command: even an apparently commit-only `git log -1` can lazy-fetch when the referenced commit object is missing locally, so there is no auto-approval exemption.
+- `merge.<driver>.driver`, together with remerge-capable `git log` / `git show` paths. `--remerge-diff`, `--diff-merges=remerge`, and the documented short spelling `--diff-merges=r` are recognized directly as helper-capable options in both AST and regex classifiers.
+- repository-local/worktree `core.alternateRefsCommand`. Git can execute it while evaluating alternate refs for otherwise read-only history commands such as `git log --alternate-refs`, so this risk is deliberately fail-closed for every read-only Git command rather than tied to a hand-maintained flag list.
+- repository-local/worktree `core.hooksPath`. Read-oriented commands can still update index metadata and invoke hooks such as `post-index-change`; because the set of consumers is wider than a stable subcommand list, this risk is also fail-closed for every read-only Git command.
+
+`git remote show <name>` is handled structurally rather than by config inspection because it can contact the remote and invoke transport helpers. It is not auto-approved unless `-n` / `--no-query` occurs before the `--` option terminator and therefore really prevents the query. The same rule is mirrored in the regex fallback classifier.
+
+Commands that change directory before a relevant Git command ask instead of trying to simulate shell cwd state. Parser fallback is deliberately more conservative: when repository config contains any covered execution risk, a fallback-classified Git command asks rather than silently auto-executing. The fallback checks `Object.values(risk).some(Boolean)` so future risk fields cannot be accidentally omitted from its fail-closed gate. Regression tests keep the AST and fallback helper-option rules in lockstep for remote-show, remerge, and `%G*` cases.
+
+Leading environment assignments and shell substitutions are checked on the raw command before `stripShellWrapper()` can discard them. The normal shell permission path returns `ask`; memory-scoped/dream worker permission evaluation returns `deny` because those agents run under forced-YOLO semantics where an `ask` decision would not provide a reliable stop. This prevents `GIT_CONFIG_* ... bash -c 'git …'` from opening a sibling auto-approval path. Explicit user-configured `Bash(...)` allow rules in the normal permission manager remain a separate authorization layer and can intentionally override the default `ask` decision; this PR does not change that pre-existing rule-matching behavior.
+
+AST assignment handling also distinguishes a pure top-level assignment from an assignment inside a brace group. A standalone `FOO=bar` remains read-only, but `{ FOO=bar; } && command` is not: brace groups execute in the current shell, so the assignment can persist and change how the following command resolves or behaves. Such grouped assignments therefore classify as unknown instead of inheriting the standalone-assignment exemption.
+
+When the repository-config probe finds active risk bits, a debug-log entry records the working directory and active risk names so confirmation changes can be diagnosed without reverse-engineering the gate.

packages/core/src/memory/memory-scoped-agent-config.shell-safety.test.ts

@@ -0,0 +1,72 @@
+/**
+ * @license
+ * Copyright 2026 Qwen Team
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { execFileSync } from 'node:child_process';
+import { mkdtempSync, rmSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import path from 'node:path';
+import { afterEach, describe, expect, it } from 'vitest';
+import type { Config } from '../config/config.js';
+import type { PermissionManager } from '../permissions/permission-manager.js';
+import { ToolNames } from '../tools/tool-names.js';
+import { createMemoryScopedAgentConfig } from './memory-scoped-agent-config.js';
+
+describe('memory-scoped shell safety', () => {
+  const tempDirs: string[] = [];
+
+  const createRepo = (): string => {
+    const cwd = mkdtempSync(path.join(tmpdir(), 'qwen-memory-shell-'));
+    tempDirs.push(cwd);
+    execFileSync('git', ['init', '-q'], { cwd });
+    return cwd;
+  };
+
+  const permissionManager = (config: Config): PermissionManager => {
+    const pm = config.getPermissionManager?.();
+    if (!pm) throw new Error('missing permission manager');
+    return pm;
+  };
+
+  afterEach(() => {
+    for (const cwd of tempDirs.splice(0)) {
+      rmSync(cwd, { recursive: true, force: true });
+    }
+  });
+
+  it('rejects raw env assignments before shell wrapper stripping', async () => {
+    const projectRoot = createRepo();
+    const pm = permissionManager(
+      createMemoryScopedAgentConfig({} as Config, projectRoot, {
+        allowShell: true,
+      }),
+    );
+
+    await expect(
+      pm.evaluate({
+        toolName: ToolNames.SHELL,
+        cwd: projectRoot,
+        command: "bash -c 'git status'",
+      }),
+    ).resolves.toBe('allow');
+
+    await expect(
+      pm.evaluate({
+        toolName: ToolNames.SHELL,
+        cwd: projectRoot,
+        command:
+          "GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=diff.external GIT_CONFIG_VALUE_0=/tmp/evil bash -c 'git diff'",
+      }),
+    ).resolves.toBe('deny');
+
+    await expect(
+      pm.evaluate({
+        toolName: ToolNames.SHELL,
+        cwd: projectRoot,
+        command: "FOO=$(printf x) bash -c 'git status'",
+      }),
+    ).resolves.toBe('deny');
+  });
+});

packages/core/src/memory/memory-scoped-agent-config.ts

@@ -17,14 +17,19 @@ import type {
 } from '../permissions/types.js';
 import { ToolNames } from '../tools/tool-names.js';
 import { isShellCommandReadOnlyASTInDirectory } from '../utils/shellAstParser.js';
-import { stripShellWrapper } from '../utils/shell-utils.js';
+import {
+  hasShellSubstitution,
+  stripShellWrapper,
+} from '../utils/shell-utils.js';
 import {
   AUTO_MEMORY_PINNED_DIRNAME,
   getAutoMemoryRoot,
   getAutoMemoryTrustedAnchor,
   getUserAutoMemoryRoot,
 } from './paths.js';
 
+const LEADING_ENV_ASSIGNMENT_RE = /^\s*[A-Za-z_][A-Za-z0-9_]*=/;
+
 type MemoryScopedPermissionManager = Pick<
   PermissionManager,
   | 'evaluate'
@@ -301,6 +306,17 @@ async function evaluateScopedDecision(
       if (!opts.allowShell || !ctx.command) {
         return 'deny';
       }
+      // The scoped permission manager runs under forced-YOLO agents, where an
+      // `ask` decision can still auto-approve. Reject raw substitutions and
+      // leading environment assignments before stripShellWrapper() can erase
+      // them; this mirrors the main shell permission boundary without a second
+      // fail-open path for GIT_CONFIG_* command-scope injection.
+      if (
+        hasShellSubstitution(ctx.command) ||
+        LEADING_ENV_ASSIGNMENT_RE.test(ctx.command)
+      ) {
+        return 'deny';
+      }
       const isReadOnly = await isShellCommandReadOnlyASTInDirectory(
         stripShellWrapper(ctx.command),
         ctx.cwd ?? projectRoot,

packages/core/src/tools/shell.test.ts

@@ -8000,6 +8000,20 @@ describe('ShellTool', () => {
       expect(await invocation.getDefaultPermission()).toBe('ask');
     });
 
+    it('should keep env-prefixed Git wrappers confirmable before stripping', async () => {
+      for (const command of [
+        `GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=diff.external GIT_CONFIG_VALUE_0=/tmp/helper bash -c 'git diff'`,
+        `GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.fsmonitor GIT_CONFIG_VALUE_0=/tmp/helper git status`,
+      ]) {
+        const invocation = shellTool.build({
+          command,
+          is_background: false,
+        });
+
+        expect(await invocation.getDefaultPermission()).toBe('ask');
+      }
+    });
+
     it('should request confirmation for a non-read-only command and return details', async () => {
       const params = { command: 'npm install', is_background: false };
       const invocation = shellTool.build(params);

packages/core/src/tools/shell.ts

@@ -2043,6 +2043,7 @@ export class ShellToolInvocation extends BaseToolInvocation<
    * AST-based permission check for the shell command.
    * - Substitution-bearing commands (any form, including inside an
    *   env-prefix wrapper that `stripShellWrapper` would discard) → 'ask'
+   * - Commands with leading environment assignments → 'ask' before wrapper stripping
    * - Read-only commands (via AST analysis) → 'allow'
    * - All other commands → 'ask'
    */
@@ -2054,7 +2055,13 @@ export class ShellToolInvocation extends BaseToolInvocation<
     // `echo ok`, which the AST classifies as read-only. Without this
     // gate the command auto-executes silently with no confirmation
     // dialog and no warning. See PR #4386 R6 (cid 3298521039).
-    if (hasShellSubstitution(this.params.command)) {
+    // GIT_CONFIG_COUNT/GIT_CONFIG_KEY_* can inject executable Git config
+    // only into the spawned child. Keep all leading env assignments visible
+    // to the permission boundary instead of stripping them first.
+    if (
+      hasShellSubstitution(this.params.command) ||
+      LEADING_ENV_ASSIGNMENT_RE.test(this.params.command)
+    ) {
       return 'ask';
     }