Merge pull request #1286 from corbitsdev/cl-8999-secret-guard-dollar-tokens

fix(secret-guard): fail closed on dollar-prefixed secret paths

Author: TheGreatAxios

docs/ARCHITECTURE.md

@@ -411,7 +411,7 @@ tool call
 - **Path Escape** (`path-escape-plugin.ts`) — Canonicalizes path-like arguments against `cwd` and blocks `..` escapes, except into a root the permission layer's worktree-roots provider allowlists (e.g. a sibling git worktree of the same repo). `tool-output://` and `archive:///` refs pass through unresolved. Runs first so later plugins see resolved paths.
 - **Evidence archive** (`evidence-archive-search-plugin.ts`, `evidence-archive-path-guard.ts`) — Primary-session compaction evidence is a first-class search/read surface on `search_files` / `read_file` / `grep` via `archive:///` refs. Dump paths (`evidence-archive/`, `tool-output/archive-*`) stay blocked so the on-disk sidecar is not the retrieval API. Blob keys reject `/` so they cannot nest under `tool-output`.
 - **Tool-output URI** (`tool-output-uri-plugin.ts`) — Normalizes mistaken `read_file` blob URIs to `tool-output:///id` (corbits-only; interchange stays unpatched).
-- **Secret Guard** (`secret-guard-plugin.ts`) — Hard-denies path-keyed tool calls (`read_file`, `write_file`, …) that would put a sensitive file into (or write it from) the model context. Runs before the permission plugin, so the path-arg deny holds even under `--dangerously-skip-permissions`. An operator-chosen `--config` path cannot be covered by static patterns, so entry points pass the resolved active settings path as `extraDeniedPaths` (exact match, lexical plus realpath). Shell commands that _reference_ a sensitive path (tokenized so `cat .env`, `bun --env-file=.env run …`, and quote/env-assignment forms are detected) are not hard-denied here. Normal mode requires operator approval via the permission gate, and auto mode forces the same ask through the auto-shell policy (`sensitive-path` rule). Yolo/skip-permissions bypasses that prompt after catastrophic authorization checks, but path-keyed access to statically protected secret paths remains hard-denied. Shell detection is best-effort: token matching defeats quoting and env-assignment/redirection forms but not dynamic path construction (variable indirection, `printf` assembly). Tool-result secret scrub still redacts credential-shaped output.
+- **Secret Guard** (`secret-guard-plugin.ts`) — Hard-denies path-keyed tool calls (`read_file`, `write_file`, …) that would put a sensitive file into (or write it from) the model context. Runs before the permission plugin, so the path-arg deny holds even under `--dangerously-skip-permissions`. An operator-chosen `--config` path cannot be covered by static patterns, so entry points pass the resolved active settings path as `extraDeniedPaths` (exact match, lexical plus realpath). Shell commands that _reference_ a sensitive path (tokenized so `cat .env`, `bun --env-file=.env run …`, and quote/env-assignment forms are detected) are not hard-denied here. Normal mode requires operator approval via the permission gate, and auto mode forces the same ask through the auto-shell policy (`sensitive-path` rule). Yolo/skip-permissions bypasses that prompt after catastrophic authorization checks, but path-keyed access to statically protected secret paths remains hard-denied. Shell detection is best-effort: token matching defeats quoting and env-assignment/redirection forms but not dynamic path construction (bare-variable indirection, `printf` assembly). A leading `$HOME`/`${HOME}` or `$USER`/`${USER}` expands from the environment before matching, and any other path-shaped `$-token` fails closed to ask. Tool-result secret scrub still redacts credential-shaped output.
 - **Authorization** (`run-shell-authz.ts`, enforced by the permission gate) — Denies catastrophic shell command patterns by regex, and hard-blocks shell `find`, head-position `rg`, and recursive `grep -r` (they can walk huge trees and OOM the host). Bounded `grep`/`search_files` tools remain practical alternatives (timeout + output caps); the patterns match those three command shapes only — an `ls -R`, `fd`, or scripted `os.walk` is just as unbounded and is not caught, so the block message tells the model not to substitute one. The gate hard-denies these at the top of its verdict path — before auto-allow, prompting, grants, and skipPermissions — so no mode or stored grant can admit them.
 - **Permission** (`permission-plugin.ts`) — Delegates consequential calls to the permission gate.
 - **Shell Guard** (`shell-guard-plugin.ts`) — Corbits Code-only replacement for stock `run_shell` (interchange stays unpatched): 120s foreground default (`settings.shell.timeoutMs` overrides that default only; a positive per-call `timeout` is the bound with no ceiling — `maxTimeoutMs` does not clamp it), background `run_shell` has no default (only a per-call timeout arms a timer), 512KB display cap with head+tail retention (the process keeps running when the cap is hit), process-group kill on timeout, abort, and plugin dispose (live children tracked in the plugin and reaped by `posixTools.dispose`), and `background: true` — the call returns a `shell_id` at once (registry in `src/shell/background-shell.ts`), the process group keeps running past the turn, completion is process-exit (stdio-close is not required), delivered on a later turn via `buildShellBackgroundMessage`, and `shell_collect` retrieves or cancels with a 300s wait cap (schema advertised by `advertiseShellGuardTimeout` only when `shell_collect` is mounted; evaluated by the permission chain at start time like any shell call). Foreground expiry is exit 124 + `timedOut:true` plus a nudge to retry with `background:true`. Also applies a 10s wall-clock budget to `grep`/`search_files` (budget expiry and budget-torn-down aborts surface as explicit timeout errors, never as empty results; an unbounded workspace-root `search_files` walk is refused up front with scope guidance instead of running to the budget). Ripgrep detached spawns are not tracked.

src/permission/classify-security.test.ts

@@ -43,6 +43,9 @@ describe("isAutoAllowedShellCall — sensitive-path arguments", () => {
     ["cat .ssh/known_hosts", false],
     ["cat .netrc", false],
     ["cat .git-credentials", false],
+    ["cat $HOME/.ssh/id_rsa", false],
+    ["cat $HOME/.env", false],
+    ["cat ${HOME}/.aws/credentials", false],
     ["env FILE=.envrc sh -c 'cat \"$FILE\"'", false],
     ["sed -Enf.flaskenv input", false],
     ["sed --fil=.envrc input.txt", false],
@@ -290,6 +293,28 @@ describe("sensitive-path shell commands require approval, not a hard deny", () =
     expect(rule?.effect).toBe("ask");
   });
 
+  test("auto mode forces ask for dollar-prefixed home secret paths", () => {
+    for (const command of [
+      "cat $HOME/.ssh/id_rsa",
+      "cat $HOME/.env",
+      "cat ${HOME}/.aws/credentials",
+    ]) {
+      expect(autoShellRuleForCall(shellCall(command))).toMatchObject({
+        name: "sensitive-path",
+        effect: "ask",
+      });
+      expect(isAutoAllowedShellCall(shellCall(command))).toBe(false);
+    }
+  });
+
+  test("auto mode fails closed on path-shaped unknown-variable tokens", () => {
+    expect(autoShellRuleForCall(shellCall("cat $BASE/mylink"))).toMatchObject({
+      name: "sensitive-path",
+      effect: "ask",
+    });
+    expect(isAutoAllowedShellCall(shellCall("cat $BASE/mylink"))).toBe(false);
+  });
+
   test("auto mode asks for clustered bash secret reads", () => {
     for (const command of [
       "bash -lc \"cat \\$'.envrc'\"",

src/plugins/secret-guard-plugin.test.ts

@@ -1,5 +1,5 @@
 import { describe, test, expect } from "bun:test";
-import { mkdir, mkdtemp, rm } from "node:fs/promises";
+import { mkdir, mkdtemp, rm, symlink, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { createPosixTools } from "@intx/tools-posix";
@@ -10,6 +10,7 @@ import { loadProjectApprovals } from "../permission/store.js";
 import {
   secretGuardPlugin,
   isSensitivePath,
+  isSensitiveShellToken,
   commandReferencesSensitivePath,
 } from "./secret-guard-plugin.js";
 
@@ -287,6 +288,14 @@ describe("commandReferencesSensitivePath", () => {
     "cat release.keystore",
     "cat server.ppk",
     "cat service-account.json",
+    // Dollar-prefixed home tokens resolve before matching (CL-8999): the
+    // shell expands these to the operator's home at runtime, so they must
+    // ask exactly like their literal forms.
+    "cat $HOME/.ssh/id_rsa",
+    "cat $HOME/.env",
+    "cat ${HOME}/.aws/credentials",
+    // Literal tilde keeps asking (unchanged behavior, pinned here).
+    "cat ~/.ssh/id_rsa",
   ];
   test("flags every command touching a credential path", () => {
     expect(
@@ -308,6 +317,9 @@ describe("commandReferencesSensitivePath", () => {
     "sed --f=.envrc input.txt",
     "grep --fil=.envrc needle",
     "bun test",
+    // A bare variable with no path shape never names a file (CL-8999).
+    "echo $HOME",
+    "cat $HOMEPATH",
   ];
   test("allows every benign command", () => {
     expect(
@@ -316,6 +328,40 @@ describe("commandReferencesSensitivePath", () => {
   });
 });
 
+describe("dollar-prefixed secret paths", () => {
+  test("$HOME-prefixed symlink into a secret file asks like the literal form", async () => {
+    const dir = await mkdtemp(join(tmpdir(), "cl8999-dollar-"));
+    try {
+      await writeFile(join(dir, ".env"), "SECRET=x\n");
+      await symlink(join(dir, ".env"), join(dir, "mylink"));
+      const savedHome = process.env.HOME;
+      process.env.HOME = dir;
+      try {
+        expect(isSensitiveShellToken("mylink", dir)).toBe(true);
+        expect(isSensitiveShellToken("$HOME/mylink", dir)).toBe(true);
+        expect(isSensitiveShellToken("${HOME}/mylink", dir)).toBe(true);
+        expect(
+          commandReferencesSensitivePath("cat $HOME/mylink", dir),
+        ).not.toBeUndefined();
+      } finally {
+        if (savedHome === undefined) delete process.env.HOME;
+        else process.env.HOME = savedHome;
+      }
+    } finally {
+      await rm(dir, { recursive: true, force: true });
+    }
+  });
+
+  test("path-shaped tokens under unknown variables fail closed to ask", () => {
+    expect(isSensitiveShellToken("$BASE/mylink")).toBe(true);
+    expect(
+      commandReferencesSensitivePath("cat $BASE/mylink"),
+    ).not.toBeUndefined();
+    expect(isSensitiveShellToken("$HOME")).toBe(false);
+    expect(commandReferencesSensitivePath("echo $HOME")).toBeUndefined();
+  });
+});
+
 describe("secretGuardPlugin run_shell", () => {
   // Shell commands that mention a secret path are no longer hard-denied here —
   // they require operator approval at the permission gate. The plugin only

src/plugins/secret-guard-plugin.ts

@@ -211,6 +211,12 @@ export function createExtraDeniedPathMatcher(
 // symlink the matcher only ever saw as `*`. Perfect shell sandboxing is out
 // of scope; the goal is to force a prompt for the trivial, single-token
 // references that make exfiltration easy. Tool-result secret scrub still redacts credential-shaped output.
+// A leading $HOME/${HOME} or $USER/${USER} expands from the environment
+// before matching, so `$HOME/.ssh/id_rsa` meets the same legs as the literal
+// path; any other path-shaped $-token (`$BASE/mylink`) fails closed to ask
+// because the matcher cannot know what it expands to. A bare `$VAR` with no
+// path shape stays invisible — when the value is a secret path the matcher
+// never sees one token that names it.
 // Programs that only print directory names / metadata — listing a name never
 // dumps file contents. Single owner for this set: the resolve-leg skip below
 // and classify.ts's pure-listing exemption both read it, so a new names-only
@@ -225,6 +231,30 @@ export const PURE_DIRECTORY_LISTING_PROGRAMS = new Set(["ls", "tree"]);
 // pattern, so `cat *.txt` cannot resolve without running the shell — but a
 // glob CAN expand into a symlink at runtime, which stays a stated residual
 // (see the threat model below), not something this filter disproves.
+
+// CL-8999: the shell expands a leading $HOME (or ${HOME}) before opening the
+// path, so expand it here (string-only, from the environment — never shell
+// eval) and let the normal legs judge the result. $USER (or ${USER}) gets the
+// same treatment. Only these two prefixes expand: arbitrary variables stay
+// unexpanded and fail closed to ask as path-shaped tokens below.
+function expandLeadingDollarToken(token: string): string {
+  const home = process.env.HOME;
+  if (home !== undefined && home.length > 0) {
+    if (token === "$HOME" || token === "${HOME}") return home;
+    if (token.startsWith("$HOME/")) return home + token.slice("$HOME".length);
+    if (token.startsWith("${HOME}/"))
+      return home + token.slice("${HOME}".length);
+  }
+  const user = process.env.USER ?? process.env.USERNAME;
+  if (user !== undefined && user.length > 0) {
+    if (token === "$USER" || token === "${USER}") return user;
+    if (token.startsWith("$USER/")) return user + token.slice("$USER".length);
+    if (token.startsWith("${USER}/"))
+      return user + token.slice("${USER}".length);
+  }
+  return token;
+}
+
 function isPathLikeShellToken(token: string): boolean {
   if (
     token.startsWith("-") ||
@@ -303,9 +333,25 @@ export function isSensitiveShellToken(
   isExtraDenied: (value: string) => boolean = () => false,
   dialect: ShellDialect = nativeShellDialect(process.platform),
 ): boolean {
-  const expanded = expandHome(token);
+  const expanded = expandHome(expandLeadingDollarToken(token));
   if (isSensitivePath(expanded, dialect)) return true;
   if (isExtraDenied(expanded)) return true;
+  // CL-8999: a token the shell could still expand at runtime into a path
+  // ($BASE/mylink, or $HOME/mylink with HOME unset) never reaches either leg
+  // below — `$` tokens skip both the path-like and bare filters — so fail
+  // closed to ask when the `$` starts a live variable expansion ($VAR or
+  // ${VAR}) joined to a path separator. Quoted dollars (`'$'.envrc`), command
+  // substitutions (`$(...)`), ANSI-C quotes (`$'...'`), and special parameters
+  // ($?, $$, $1) never match, and neither do ADS streams (`.env.sample::$DATA`,
+  // no separator) or bare `$VAR` tokens with no path shape: a bare variable
+  // can never name a file the shell opens without expanding into something
+  // the matcher sees.
+  if (
+    expanded.includes("$") &&
+    /\$[{]?[A-Za-z_]/.test(expanded) &&
+    (expanded.includes("/") || expanded.includes("\\"))
+  )
+    return true;
   if (!resolveSymlinks) {
     if (!isBareProbeCandidate(expanded)) return false;
     return isExtraDenied(