diff --git a/.changeset/bold-ibex-chatter.md b/.changeset/bold-ibex-chatter.md new file mode 100644 index 000000000..254951d2a --- /dev/null +++ b/.changeset/bold-ibex-chatter.md @@ -0,0 +1,5 @@ +--- +type: Security +pr: 4659 +--- +**The secret-read guard no longer lets a trailing dot or space alias past it** — Windows strips trailing dots and spaces from every path component, so `.env.`, `.env ` and `.secrets.` all resolve to the protected file while the guard treated them as unrelated names and allowed the read. Names are now normalized before classification, and the Read, Grep and Bash arms share one path-segmentation rule instead of two that disagreed on backslash paths. (#4651) diff --git a/.changeset/happy-herons-tumble.md b/.changeset/happy-herons-tumble.md new file mode 100644 index 000000000..e7e45092a --- /dev/null +++ b/.changeset/happy-herons-tumble.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4659 +--- +**Secret-free `.env` templates with a qualifier are readable again** — the read guard compared everything after `.env.` as one token against a set of final extensions, so a committed template like `.env.local.example` was refused and the reader was pushed toward the real secret file it exists to replace. Classification now keys on the final extension. (#4580) diff --git a/docs/USER-GUIDE.md b/docs/USER-GUIDE.md index 2d8ce1b4d..e159f47f7 100644 --- a/docs/USER-GUIDE.md +++ b/docs/USER-GUIDE.md @@ -459,7 +459,7 @@ GSD generates markdown files that become LLM system prompts. This means any user - `gsd-prompt-guard.js` — Scans Write/Edit calls to `.planning/` for injection patterns (always active, advisory-only) - `gsd-workflow-guard.js` — Warns on file edits outside GSD workflow context (opt-in via `hooks.workflow_guard`) - `gsd-write-guard.js` — Hard-blocks a whole-file `Write` that catastrophically shrinks a curated `.planning/` artifact (`ROADMAP.md`, milestone roadmaps, `STATE.md`) below 40% of its on-disk line count; files under 40 lines are exempt. The check is stateless per Write, comparing each payload against the file's *current* on-disk size — a single-shot collapse (the #973 shape) is blocked, but a sequence of individually-tolerated shrinks that erodes the file across several Writes is not detected. For a legitimate milestone reset or large deletion, bypass once with the single-use sentinel — write the target's path into `.planning/.gsd-allow-shrink` (fresh within 15 minutes; consumed by the allowed write) — or, interactively, with `GSD_ALLOW_PLANNING_SHRINK=1` in the runtime's environment. Scope the guarantee accordingly: this stops accidental and single-shot collapse, and is not a defense against a determined agent — the sentinel is a plain file, so anything with shell access can arm one; what it buys is that the bypass becomes a deliberate, path-bound, single-use and auditable action rather than a sentence to reason past (always active, blocking; #2255, fix 3 of #973) -- `gsd-secret-read-guard.js` — Hard-blocks reads of secret files — `.env`, `.env.` and `.secrets`, matched case-insensitively (`.ENV`, `.Secrets`) — through Read (`file_path`), Grep (an explicit `path`, or a `glob` that selects them, judged per brace alternative) and Bash (operands, input redirects, `$( )` / backtick / `<( )` bodies, and `git show :` shapes). A shell interpreter (`bash`/`sh`/`zsh`/`dash`/`ksh`) has its script scanned however it arrives — `-c '…'`, a `<( )` file operand, a heredoc / here-string, or a pipe from a knowable `echo`/`printf` source (`echo cat .env | bash`) — as do `eval`'s joined operands, a `source`/`.` process-substitution operand, and `find … | xargs cat` pipelines (upstream literal names become the sub-command's read operands). `.env.example` / `.env.sample` / `.env.template` / `.env.dist` stay readable (they are the templates GSD's own phase prompt reads — a real secret stored under one of those names is not protected), and existence checks (`[ -f .env ]`, `ls .env*`, `test`, `stat`, `rm`, `touch`, `echo`, …) pass. Not covered, by construction: `$VAR` indirection (`bash -c "$CMD"`), shell globs (`cat .e*`), interpreter one-liners, a piped script from a non-`echo`/`printf` source (`cat gen.sh | bash`, `curl … | sh`), reads inside scripts the agent runs, and a Grep `glob: '*'` reaching a `.env` that is not gitignored — none are statically resolvable by a hook. This replaces the `Read(.env)` / `Read(.env.*)` / `Read(.secrets)` permission deny rules the installer used to write: on Claude Code ≥ 2.1.259 any `Read()` deny rule makes every `cd DIR && grep …` compound prompt for approval even in `auto` mode, while a hook denial is not a permission rule and applies in `auto` and `bypassPermissions` alike (always active, blocking; #4221) +- `gsd-secret-read-guard.js` — Hard-blocks reads of secret files — `.env`, `.env.` and `.secrets`, matched case-insensitively (`.ENV`, `.Secrets`) — through Read (`file_path`), Grep (an explicit `path`, or a `glob` that selects them, judged per brace alternative) and Bash (operands, input redirects, `$( )` / backtick / `<( )` bodies, and `git show :` shapes). A shell interpreter (`bash`/`sh`/`zsh`/`dash`/`ksh`) has its script scanned however it arrives — `-c '…'`, a `<( )` file operand, a heredoc / here-string, or a pipe from a knowable `echo`/`printf` source (`echo cat .env | bash`) — as do `eval`'s joined operands, a `source`/`.` process-substitution operand, and `find … | xargs cat` pipelines (upstream literal names become the sub-command's read operands). A name is exempt when its **final extension** is `example`, `sample`, `template` or `dist`, so `.env.example` and equally `.env.local.example` / `.env.production.sample` stay readable (they are the templates GSD's own phase prompt reads). Stated cost: the exempt set is that unbounded family, not four fixed names — a real secret is not protected if it is named to end in one of those four extensions. Order matters, and only the last segment counts: `.env.example.local` is a secret and stays blocked. Trailing dots and spaces are stripped before the name is judged, because Windows resolves `.env.`, `.env ` and `.secrets.` to the protected file itself. Existence checks (`[ -f .env ]`, `ls .env*`, `test`, `stat`, `rm`, `touch`, `echo`, …) pass. Not covered, by construction: `$VAR` indirection (`bash -c "$CMD"`), shell globs (`cat .e*`), interpreter one-liners, a piped script from a non-`echo`/`printf` source (`cat gen.sh | bash`, `curl … | sh`), reads inside scripts the agent runs, and a Grep `glob: '*'` reaching a `.env` that is not gitignored — none are statically resolvable by a hook. This replaces the `Read(.env)` / `Read(.env.*)` / `Read(.secrets)` permission deny rules the installer used to write: on Claude Code ≥ 2.1.259 any `Read()` deny rule makes every `cd DIR && grep …` compound prompt for approval even in `auto` mode, while a hook denial is not a permission rule and applies in `auto` and `bypassPermissions` alike (always active, blocking; #4221) **CI Scanner:** `prompt-injection-scan.security.test.cjs` scans all agent, workflow, and command files for embedded injection vectors. diff --git a/hooks/gsd-secret-read-guard.js b/hooks/gsd-secret-read-guard.js index 44f212d5e..ab88c609e 100644 --- a/hooks/gsd-secret-read-guard.js +++ b/hooks/gsd-secret-read-guard.js @@ -26,12 +26,24 @@ // .env, .secrets, and .env. — EXCEPT .env.example / .env.sample / // .env.template / .env.dist, which are the non-secret templates GSD's own // phase prompt tells executors to read. -// Stated cost: this is narrower than the retired `Read(.env.*)` rule — a -// real secret stored in `.env.example` is not protected. +// Stated cost (#4580): the exemption matches the token's FINAL EXTENSION, +// not the whole name, so the trusted set is `.env..example` / +// `.sample` / `.template` / `.dist` — an unbounded family, not four fixed +// names. A real secret named `.env.prod-real-secrets.example` is NOT +// protected, and renaming any secret to end in one of those four +// extensions bypasses the guard across Read, Grep and Bash alike. This is +// the deliberate cost of #4580, which fixed the prior whole-name +// comparison wrongly refusing committed, secret-free templates like +// `.env.local.example`. // A token containing `:` is also tested on the part after its LAST `:`, // so `git show HEAD:.env`, `origin/main:config/.env` and `C:\proj\.env` -// are caught without git-specific parsing. No whitespace trimming: the -// commit message `fix: .env parsing` yields ` .env parsing`, not a name. +// are caught without git-specific parsing. Leading/interior whitespace is +// still NOT trimmed: the commit message `fix: .env parsing` yields +// ` .env parsing`, which is prose, not a name. TRAILING dots and spaces ARE +// stripped from the basename before classification (`.env.`, `.env..`, +// `.env `, `.env. ` all normalize to `.env`), because Win32 strips trailing +// dots and spaces from each path component, so these are aliases for the +// same on-disk file, not distinct names. // // Bash analysis is a two-pass token scan, not a shell: // pass 1 tokenizes with quote state, comments, redirect operators (with fd @@ -86,6 +98,7 @@ 'use strict'; const { HOOK_ON_CRASH, allow, deny, crash } = require('./lib/hook-exit.js'); +const { finalExtension, normalizeWindowsBasename, lastSegment } = require('./lib/filename-classification.js'); // Fail open on a hook-internal error (see header). Declared ONCE so the // outer catch states its policy explicitly (#3911). @@ -148,21 +161,18 @@ const GLOB_PROBES = [ // --------------------------------------------------------------------------- function isSecretBasename(name) { - if (name === '.env' || name === '.secrets') return true; - if (name.startsWith('.env.')) { - const suffix = name.slice('.env.'.length); - return suffix !== '' && !NON_SECRET_ENV_SUFFIXES.has(suffix.toLowerCase()); + // Win32 strips trailing dots/spaces per path component, so `.env.`, + // `.env ` etc. resolve to the real `.env` on Windows — normalize FIRST so + // those aliases can't bypass classification. + const n = normalizeWindowsBasename(name); + if (n === '.env' || n === '.secrets') return true; + if (n.startsWith('.env.')) { + const suffix = n.slice('.env.'.length); + return suffix !== '' && !NON_SECRET_ENV_SUFFIXES.has(finalExtension(suffix).toLowerCase()); } return false; } -// Last `/`- or `\`-separated segment, ignoring trailing separators. -function lastSegment(tok) { - const s = tok.replace(/[\\/]+$/, ''); - const i = Math.max(s.lastIndexOf('/'), s.lastIndexOf('\\')); - return i === -1 ? s : s.slice(i + 1); -} - // True when the token's basename — or the basename of the part after its // last `:` (git `:`, Windows drive) — is a secret name. Folded to // lower case once at the top so `.ENV` / `.Secrets` match on the @@ -247,7 +257,19 @@ function globAltSelectsSecret(alt) { if (/^[*?]+$/.test(alt)) return false; // pure wildcard: equivalent to no glob const wild = alt.search(/[*?[]/); const lit = wild === -1 ? alt : alt.slice(0, wild); - if (lit.startsWith('.env.')) return true; + // #4580: when there is no wildcard, `alt` (== `lit`) is a WHOLE literal + // filename, so classify it exactly the same way Read/Bash do (by its + // FINAL extension, via isSecretBasename) rather than by a `.env.`-prefix + // heuristic — that heuristic mis-blocked multi-dot templates like + // `.env.local.example`. When a wildcard IS present, `lit` is only a + // PARTIAL literal prefix (`.env.local.exam*` can still select + // `.env.local`), which cannot be classified exactly, so the original + // conservative prefix rule stays. + if (wild === -1) { + if (isSecretBasename(lit)) return true; + } else if (lit.startsWith('.env.')) { + return true; + } if (lit !== '' && ('.env.'.startsWith(lit) || '.secrets'.startsWith(lit))) return true; let re; try { @@ -260,10 +282,14 @@ function globAltSelectsSecret(alt) { // Returns null (allowed), 'secret-read', or 'glob-too-complex'. function classifyGrepGlob(glob) { - const segIdx = glob.replace(/\/+$/, '').lastIndexOf('/'); + // Segment via the SAME `lastSegment` helper Read/Bash use (namesSecret), + // rather than a hand-rolled forward-slash-only split — the two used to + // diverge on a backslash-bearing glob (`config\.env`), which `lastSegment` + // reduces to `.env` but a `/`-only split left untouched, letting it escape + // this arm's predicate while Read/Bash still blocked it. // Case-fold the last segment (GLOB_PROBES are lower case) so `.ENV*` and // `*.ENV` select the secret namespace on case-insensitive filesystems. - const segment = (segIdx === -1 ? glob : glob.slice(segIdx + 1)).toLowerCase(); + const segment = lastSegment(glob).toLowerCase(); const alts = expandBraces(segment); if (alts === null) return 'glob-too-complex'; return alts.some(globAltSelectsSecret) ? 'secret-read' : null; diff --git a/hooks/lib/filename-classification.js b/hooks/lib/filename-classification.js new file mode 100644 index 000000000..a31b9c9e2 --- /dev/null +++ b/hooks/lib/filename-classification.js @@ -0,0 +1,64 @@ +'use strict'; +// hooks/lib/filename-classification.js — hand-written, NOT generated. One +// tiny, deliberately-named filename slicer. +// +// WHY this exists: issue #4580 was caused by comparing "everything after the +// `.env.` prefix" — a multi-segment token like `local.example` — against a +// set whose members are FINAL EXTENSIONS (`example`). `.env.local.example` +// was classified by its full `local.example` tail, which is not in a set +// built from bare extensions, so the comparison silently failed. This module +// deliberately exposes ONLY the final-extension answer so the wrong token +// can't be picked by accident at a call site. The "everything after the +// first dot" form is intentionally NOT exported: no caller needs it, and an +// unused export is dead code. + +/** + * The segment after the LAST dot in `name`. A string with no dot IS its own + * final extension. Inert on non-strings/empty input: never throws, returns + * ''. + * + * @param {*} name + * @returns {string} + */ +function finalExtension(name) { + if (typeof name !== 'string' || name === '') return ''; + const i = name.lastIndexOf('.'); + return i === -1 ? name : name.slice(i + 1); +} + +/** + * Strips ALL trailing dots and spaces from `name`, repeatedly, from the end + * of the basename. + * + * WHY: Win32 strips trailing dots and trailing spaces from each path + * component when resolving a filesystem path — `.env.`, `.env..`, `.env `, + * and `.env. ` all resolve to the same on-disk file as `.env` on Windows. + * These are therefore ALIASES for the protected name, not distinct names, + * and a guard that classifies the literal string without normalizing first + * can be bypassed by any of them. This is not cosmetic tidying — it closes + * that Windows path-alias bypass. + * + * This runs UNCONDITIONALLY on every host platform (macOS, Linux, Windows), + * not only when actually running on Windows: the guard must behave + * identically everywhere, and a name is judged by what Win32 would resolve + * it to, regardless of what OS the hook happens to run on. + * + * @param {*} name + * @returns {string} + */ +function normalizeWindowsBasename(name) { + if (typeof name !== 'string' || name === '') return ''; + let end = name.length; + while (end > 0 && (name[end - 1] === '.' || name[end - 1] === ' ')) end--; + return name.slice(0, end); +} + +// Last `/`- or `\`-separated segment, ignoring trailing separators. A string +// with no separator IS its own last segment. +function lastSegment(tok) { + const s = tok.replace(/[\\/]+$/, ''); + const i = Math.max(s.lastIndexOf('/'), s.lastIndexOf('\\')); + return i === -1 ? s : s.slice(i + 1); +} + +module.exports = { finalExtension, normalizeWindowsBasename, lastSegment }; diff --git a/tests/filename-classification.test.cjs b/tests/filename-classification.test.cjs new file mode 100644 index 000000000..51e2437de --- /dev/null +++ b/tests/filename-classification.test.cjs @@ -0,0 +1,143 @@ +'use strict'; + +/** + * filename-classification.test.cjs + * + * Unit tests for hooks/lib/filename-classification.js. Exports one pure, + * inert helper used to classify `.env.` basenames by their FINAL + * extension only: + * + * finalExtension(name) -> segment after the LAST dot; the whole string + * when there is no dot; '' for empty/non-string. + * + * #4580 was caused by comparing the whole tail after the first dot (e.g. + * `local.example`) against a set of final extensions (e.g. `example`). + */ + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fc = require('./helpers/fast-check-setup.cjs'); +const { finalExtension, normalizeWindowsBasename } = require('../hooks/lib/filename-classification.js'); + +test('finalExtension: a dotless string is its own final extension', () => { + assert.equal(finalExtension('example'), 'example'); +}); + +test('finalExtension: single dot returns the segment after it', () => { + assert.equal(finalExtension('local.example'), 'example'); +}); + +test('finalExtension: multi-segment returns only the LAST segment', () => { + assert.equal(finalExtension('a.b.c.d'), 'd'); +}); + +test('finalExtension: empty string returns empty string', () => { + assert.equal(finalExtension(''), ''); +}); + +test('finalExtension: a trailing dot yields an empty final extension', () => { + assert.equal(finalExtension('example.'), ''); +}); + +test('finalExtension: a leading dot yields the segment after it', () => { + assert.equal(finalExtension('.example'), 'example'); +}); + +test('finalExtension: non-string / nullish inputs are inert, never throw', () => { + assert.equal(finalExtension(null), ''); + assert.equal(finalExtension(undefined), ''); + assert.equal(finalExtension(42), ''); +}); + +test('finalExtension returns the last segment, not the whole multi-dot suffix (#4580)', () => { + // #4580: the guard compared `local.example` (everything after the first + // dot) against a set of final extensions like `example`, so a correct + // implementation must return the last segment, not the whole token. + assert.equal(finalExtension('local.example'), 'example'); + assert.notEqual(finalExtension('local.example'), 'local.example'); +}); + +test('fc: finalExtension never contains a dot, is always a suffix of the input, and preserves content', () => { + fc.assert( + fc.property( + fc.string(), + (s) => { + const ext = finalExtension(s); + assert.equal(ext.includes('.'), false); + assert.equal(s.endsWith(ext), true); + const i = s.lastIndexOf('.'); + const expected = i === -1 ? s : s.slice(i + 1); + assert.equal(ext, expected); + }, + ), + { seed: 42, numRuns: 200 }, + ); +}); + +test('normalizeWindowsBasename: strips a single trailing dot', () => { + assert.equal(normalizeWindowsBasename('.env.'), '.env'); +}); + +test('normalizeWindowsBasename: strips repeated trailing dots', () => { + assert.equal(normalizeWindowsBasename('.env..'), '.env'); +}); + +test('normalizeWindowsBasename: strips a trailing space', () => { + assert.equal(normalizeWindowsBasename('.env '), '.env'); +}); + +test('normalizeWindowsBasename: strips a trailing dot-then-space', () => { + assert.equal(normalizeWindowsBasename('.env. '), '.env'); +}); + +test('normalizeWindowsBasename: strips a trailing space-then-dot', () => { + assert.equal(normalizeWindowsBasename('.env .'), '.env'); +}); + +test('normalizeWindowsBasename: strips a trailing dot off .secrets', () => { + assert.equal(normalizeWindowsBasename('.secrets.'), '.secrets'); +}); + +test('normalizeWindowsBasename: strips only the trailing dot, not interior dots', () => { + assert.equal(normalizeWindowsBasename('.env.local.'), '.env.local'); +}); + +test('normalizeWindowsBasename: all dots strips to empty string', () => { + assert.equal(normalizeWindowsBasename('...'), ''); +}); + +test('normalizeWindowsBasename: a name with no trailing dot/space is unchanged', () => { + assert.equal(normalizeWindowsBasename('.env'), '.env'); +}); + +test('normalizeWindowsBasename: .envrc is unchanged', () => { + assert.equal(normalizeWindowsBasename('.envrc'), '.envrc'); +}); + +test('normalizeWindowsBasename: empty string returns empty string', () => { + assert.equal(normalizeWindowsBasename(''), ''); +}); + +test('normalizeWindowsBasename: non-string / nullish inputs are inert, never throw', () => { + assert.equal(normalizeWindowsBasename(null), ''); + assert.equal(normalizeWindowsBasename(undefined), ''); + assert.equal(normalizeWindowsBasename(42), ''); +}); + +test('fc: normalizeWindowsBasename never ends with a dot or space, is always a prefix of the input, and only removes trailing dot/space characters', () => { + fc.assert( + fc.property( + fc.string(), + (s) => { + const n = normalizeWindowsBasename(s); + assert.equal(n.endsWith('.'), false); + assert.equal(n.endsWith(' '), false); + assert.equal(s.startsWith(n), true); + const removed = s.slice(n.length); + assert.match(removed, /^[. ]*$/); + if (!/[. ]$/.test(s)) assert.equal(n, s); + }, + ), + { seed: 42, numRuns: 200 }, + ); +}); diff --git a/tests/fixtures/install-tree/antigravity.json b/tests/fixtures/install-tree/antigravity.json index 65a8ac7fe..e1c2ad18f 100644 --- a/tests/fixtures/install-tree/antigravity.json +++ b/tests/fixtures/install-tree/antigravity.json @@ -595,6 +595,7 @@ "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", "hooks/lib/exit-code-registry.js", + "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", diff --git a/tests/fixtures/install-tree/augment.json b/tests/fixtures/install-tree/augment.json index 9fdeb8749..e00213de6 100644 --- a/tests/fixtures/install-tree/augment.json +++ b/tests/fixtures/install-tree/augment.json @@ -667,6 +667,7 @@ "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", "hooks/lib/exit-code-registry.js", + "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", diff --git a/tests/fixtures/install-tree/claude-local.json b/tests/fixtures/install-tree/claude-local.json index a52dd441d..38b7516b6 100644 --- a/tests/fixtures/install-tree/claude-local.json +++ b/tests/fixtures/install-tree/claude-local.json @@ -531,6 +531,7 @@ "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", "hooks/lib/exit-code-registry.js", + "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", diff --git a/tests/fixtures/install-tree/claude.json b/tests/fixtures/install-tree/claude.json index d2da97035..544c1247d 100644 --- a/tests/fixtures/install-tree/claude.json +++ b/tests/fixtures/install-tree/claude.json @@ -595,6 +595,7 @@ "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", "hooks/lib/exit-code-registry.js", + "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", diff --git a/tests/fixtures/install-tree/codebuddy.json b/tests/fixtures/install-tree/codebuddy.json index c11385747..2292bb9a3 100644 --- a/tests/fixtures/install-tree/codebuddy.json +++ b/tests/fixtures/install-tree/codebuddy.json @@ -667,6 +667,7 @@ "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", "hooks/lib/exit-code-registry.js", + "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", diff --git a/tests/fixtures/install-tree/hermes.json b/tests/fixtures/install-tree/hermes.json index 530029ab3..ef90f0107 100644 --- a/tests/fixtures/install-tree/hermes.json +++ b/tests/fixtures/install-tree/hermes.json @@ -595,6 +595,7 @@ "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", "hooks/lib/exit-code-registry.js", + "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", diff --git a/tests/fixtures/install-tree/kilo.json b/tests/fixtures/install-tree/kilo.json index ab1d87e09..40d0afd3b 100644 --- a/tests/fixtures/install-tree/kilo.json +++ b/tests/fixtures/install-tree/kilo.json @@ -667,6 +667,7 @@ "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", "hooks/lib/exit-code-registry.js", + "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", diff --git a/tests/fixtures/install-tree/kimi-code.json b/tests/fixtures/install-tree/kimi-code.json index 7bb433635..2191a96df 100644 --- a/tests/fixtures/install-tree/kimi-code.json +++ b/tests/fixtures/install-tree/kimi-code.json @@ -596,6 +596,7 @@ "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", "hooks/lib/exit-code-registry.js", + "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", diff --git a/tests/fixtures/install-tree/opencode.json b/tests/fixtures/install-tree/opencode.json index 135989b9f..8aaa4c28c 100644 --- a/tests/fixtures/install-tree/opencode.json +++ b/tests/fixtures/install-tree/opencode.json @@ -667,6 +667,7 @@ "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", "hooks/lib/exit-code-registry.js", + "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", diff --git a/tests/fixtures/install-tree/pi.json b/tests/fixtures/install-tree/pi.json index aa92e91c2..c49b00b7e 100644 --- a/tests/fixtures/install-tree/pi.json +++ b/tests/fixtures/install-tree/pi.json @@ -397,6 +397,7 @@ "gsd-hooks/lib/cli-exit.js", "gsd-hooks/lib/cursor-workspace.js", "gsd-hooks/lib/exit-code-registry.js", + "gsd-hooks/lib/filename-classification.js", "gsd-hooks/lib/git-cmd.js", "gsd-hooks/lib/git-probe.js", "gsd-hooks/lib/gsd-graphify-rebuild.sh", diff --git a/tests/fixtures/install-tree/qwen.json b/tests/fixtures/install-tree/qwen.json index 96d61bf75..fe9df2925 100644 --- a/tests/fixtures/install-tree/qwen.json +++ b/tests/fixtures/install-tree/qwen.json @@ -595,6 +595,7 @@ "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", "hooks/lib/exit-code-registry.js", + "hooks/lib/filename-classification.js", "hooks/lib/git-cmd.js", "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", diff --git a/tests/gsd-secret-read-guard.test.cjs b/tests/gsd-secret-read-guard.test.cjs index 1d8a3cbb4..850e323c8 100644 --- a/tests/gsd-secret-read-guard.test.cjs +++ b/tests/gsd-secret-read-guard.test.cjs @@ -70,13 +70,15 @@ function assertBlocked(r, label, { code = 'secret-read', tool, path: expectedPat describe('gsd-secret-read-guard: Read', () => { const blocks = ['.env', '/proj/.env', '.env.local', '/p/.env.production', '.secrets', 'C:\\proj\\.env', '/p/.secrets/', // Case-insensitive: these ARE the secret file on macOS/Windows. - '.ENV', '.Secrets', '.Env.production', '/P/.SECRETS']; + '.ENV', '.Secrets', '.Env.production', '/P/.SECRETS', + // Windows trailing-dot alias: strips to `.env` (#4651). + '.env.']; for (const p of blocks) { test(`blocks Read of ${JSON.stringify(p)}`, () => { assertBlocked(runHook(read(p)), p, { tool: 'Read', path: p }); }); } - const allows = ['.env.example', '.env.sample', '.env.template', '.env.dist', '.env.EXAMPLE', '.ENV.EXAMPLE', '.envrc', 'env', 'foo.env', '/p/src/index.ts', '.environment', '.env.']; + const allows = ['.env.example', '.env.sample', '.env.template', '.env.dist', '.env.EXAMPLE', '.ENV.EXAMPLE', '.envrc', 'env', 'foo.env', '/p/src/index.ts', '.environment']; for (const p of allows) { test(`allows Read of ${JSON.stringify(p)}`, () => { assertAllowed(runHook(read(p)), p); @@ -356,6 +358,150 @@ describe('gsd-secret-read-guard: Kimi vocabulary', () => { }); }); +describe('regressions: #4580 — final-extension classification', () => { + // #4580: isSecretBasename compares everything after `.env.` as ONE token + // against the template set {example, sample, template, dist}, so a + // multi-segment template name like `.env.local.example` compares the + // whole tail `local.example` against that set and wrongly blocks. The + // classification must key off the FINAL extension, not the full suffix. + const TEMPLATES = [ + '.env.local.example', + '.env.production.example', + '.env.staging.sample', + '.env.local.template', + '.ENV.Local.EXAMPLE', + 'cfg/.env.local.example', + '.env.dist.example', + '.env.example.example', + ]; + const SECRETS = [ + // CRITICAL: final extension is `local`, not a template — this IS a secret. + '.env.example.local', + '.env.local.', + '.env.local', + '.env', + '.secrets', + '.env.production', + ]; + + describe('Read arm', () => { + for (const p of TEMPLATES) { + test(`allows Read of ${JSON.stringify(p)}`, () => { + assertAllowed(runHook(read(p)), p); + }); + } + for (const p of SECRETS) { + test(`blocks Read of ${JSON.stringify(p)}`, () => { + assertBlocked(runHook(read(p)), p, { tool: 'Read', path: p }); + }); + } + describe('Windows trailing dot/space aliases are the protected file (#4651)', () => { + // Win32 strips trailing dots and spaces from each path component when + // resolving a filesystem path, so `.env.`, `.env..`, `.env `, etc. all + // resolve to the same on-disk file as `.env` — these are ALIASES, not + // distinct names, and must be blocked like the name they alias. + const aliasBlocks = ['.env.', '.env..', '.env ', '.env. ', '.env .', '.secrets.', '.secrets ', '.env.local.']; + for (const p of aliasBlocks) { + test(`blocks Read of Windows alias ${JSON.stringify(p)}`, () => { + assertBlocked(runHook(read(p)), p, { tool: 'Read', path: p }); + }); + } + // `.env.example.` aliases the already-trusted template `.env.example`, + // not the secret `.env` — it must stay allowed. + test('allows Read of .env.example. (aliases the trusted template)', () => { + assertAllowed(runHook(read('.env.example.')), '.env.example.'); + }); + }); + test('does not change unrelated allow: .envrc stays allowed', () => { + assertAllowed(runHook(read('.envrc')), '.envrc'); + }); + }); + + describe('Bash arm — git show HEAD:', () => { + test('allows a multi-segment template name via git show', () => { + assertAllowed(runHook(bash('git show HEAD:.env.local.example')), 'HEAD:.env.local.example'); + }); + test('still blocks a plain secret via git show', () => { + assertBlocked(runHook(bash('git show HEAD:.env.local')), 'HEAD:.env.local', { tool: 'Bash', path: 'HEAD:.env.local' }); + }); + }); + + describe('Grep glob arm — globAltSelectsSecret parity', () => { + const allowedGlobs = ['.env.local.example', 'sub/.env.local.example', '{.env.local.example,zzz.ts}']; + for (const g of allowedGlobs) { + test(`allows glob ${JSON.stringify(g)}`, () => { + assertAllowed(runHook(grep({ glob: g })), g); + }); + } + const blockedGlobs = ['.env.local', '.env', '.env.local.exam*', '.e*', '.env*', '*.local', '{.env.local,zzz.ts}']; + for (const g of blockedGlobs) { + test(`blocks glob ${JSON.stringify(g)}`, () => { + assertBlocked(runHook(grep({ glob: g })), g, { tool: 'Grep', path: g }); + }); + } + const allowedRegressionGlobs = ['*.example', '*', '?']; + for (const g of allowedRegressionGlobs) { + test(`allows glob ${JSON.stringify(g)} (regression)`, () => { + assertAllowed(runHook(grep({ glob: g })), g); + }); + } + }); + + // NOTE: Read and Bash both route through the shared `namesSecret` predicate, + // so they are not independent of each other here — only the Grep glob arm + // (classifyGrepGlob) is a genuinely separate implementation. This describe + // checks that all three still agree, not that Read/Bash are independent. + describe('cross-arm parity — Read, exact-literal Grep glob, and Bash must agree (Read/Bash share namesSecret; Grep glob is the independent arm)', () => { + for (const name of TEMPLATES) { + test(`Read, glob and Bash all allow ${JSON.stringify(name)}`, () => { + assertAllowed(runHook(read(name)), `read:${name}`); + assertAllowed(runHook(grep({ glob: name })), `glob:${name}`); + assertAllowed(runHook(bash('cat ' + name)), `bash:${name}`); + }); + } + for (const name of SECRETS) { + test(`Read, glob and Bash all block ${JSON.stringify(name)}`, () => { + assertBlocked(runHook(read(name)), `read:${name}`, { tool: 'Read', path: name }); + assertBlocked(runHook(grep({ glob: name })), `glob:${name}`, { tool: 'Grep', path: name }); + assertBlocked(runHook(bash('cat ' + name)), `bash:${name}`, { tool: 'Bash', path: name }); + }); + } + + // #4651: TEMPLATES/SECRETS above are all bare basenames, so this loop + // never exercised path segmentation and could not have caught the + // Read-vs-Grep-glob divergence on a backslash-bearing path (`lastSegment` + // splits on `/` AND `\`; classifyGrepGlob used to split on `/` only). + // Cover both separators explicitly. + const pathBlocks = ['config/.env', 'config\\.env']; + for (const name of pathBlocks) { + test(`Read and Grep glob agree: both block ${JSON.stringify(name)}`, () => { + assertBlocked(runHook(read(name)), `read:${name}`, { tool: 'Read', path: name }); + assertBlocked(runHook(grep({ glob: name })), `glob:${name}`, { tool: 'Grep', path: name }); + }); + } + const pathAllows = ['config/.env.local.example', 'config\\.env.local.example']; + for (const name of pathAllows) { + test(`Read and Grep glob agree: both allow ${JSON.stringify(name)}`, () => { + assertAllowed(runHook(read(name)), `read:${name}`); + assertAllowed(runHook(grep({ glob: name })), `glob:${name}`); + }); + } + }); +}); + +describe('regressions: #4651 — trailing-dot normalization must not touch prose', () => { + // Pins the header's "No whitespace trimming" guarantee for Bash PROSE: + // trailing-alias normalization applies to file-path/operand classification + // only, not to commit-message text, so leading/interior whitespace in a + // commit message must still read as prose, not as a secret operand. + test('allows a commit message mentioning .env', () => { + assertAllowed(runHook(bash('git commit -m "fix: .env parsing"')), 'commit message'); + }); + test('allows a commit message with .env at the end of prose', () => { + assertAllowed(runHook(bash('git commit -m "update .env"')), 'commit message 2'); + }); +}); + describe('gsd-secret-read-guard: scope and crash policy', () => { test('ignores other tools even when they name a secret file', () => { assertAllowed(runHook({ tool_name: 'Write', tool_input: { file_path: '.env', content: 'X=1' } }), 'Write');