From 2972da4c9d9f138b83e2d5dc3031da6d0b040099 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 18 Aug 2026 16:40:17 -0400 Subject: [PATCH] enhance(#3619): ratchet the platform seam with local/no-private-binary-resolution (epic #3411 Phase 3) (#3636) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore(#3619): ratchet the platform seam with local/no-private-binary-resolution Epic #3411 Phase 3, the ratchet. Scope revised with maintainer approval and recorded on the issue: the epic's literal ask was a rule rejecting a bare-name spawn outside the seam. Surveyed at ac1b6d679, ~30 such sites exist and none is a defect — git, gh and npm ship native .exe that CreateProcess resolves unaided, and the rest are POSIX-only tools. ADR-1703 rules 2 and 3 forbid grandfathering and escape hatches, so a literal rule would be unsuppressable and would force rewriting 30 correct calls. The epic's actual thesis was four private RESOLVERS, not four bare spawns. So the rule flags re-implementing resolution: reading PATHEXT in any casing from any object, and a hardcoded list carrying two or more of .exe/.cmd/.bat/.com — precisely the shapes fallow-runner's candidateNames and gsd-tools' PATHEXT string had before Phases 1 and 2 deleted them. Three boundaries were arrived at rather than assumed: two-or-more a single .endsWith('.cmd') is a classification, not a candidate set; runtime-hooks-surface derives .cmd shim paths that way boundary-aware a naive substring test flags .execute and .compacting, caught on src/host-integration.cts before it could become a false positive nobody could suppress suffix-anchored the seam exemption matches src/shell-command-projection.cts exactly; a substring match would also exempt the dispatch test file. Case I9 pins it. PATH scans are deliberately NOT flagged — membership checks (bin/install.js) are indistinguishable from resolution scans, and an unsound rule in a zero-escape-hatch architecture is worse than no rule. To make the ratchet strict with no carve-out, resolveExecutableBinary gained pathOverride: search THIS PATH, read everything else including PATHEXT from the ambient environment. resolveFallowBinary now supplies its own search path without hand-threading PATHEXT, which would itself have been a private read. The three alternatives were all worse: exempting the file is grandfathering, exempting the AST shape is a carve-out every future caller must replicate, and dropping the pass-through would silently ignore a user's real PATHEXT — buying a lint rule with a correctness regression. eslint-rules/** is outside the rule's globs rather than exempted, because portability-vocab.cjs owns the extension set. scripts/**/*.cjs got its own block so that exclusion does not leave a hole in the ratchet. Started green with nothing suppressed. Proven able to fail: a fixture with both signals reports two errors. Refs #3411 * fix(#3619): close the PATHEXT destructuring evasion and correct two overclaims Adversarial review found a trivial evasion of the rule's primary signal: the visitor only handled MemberExpression, so const { PATHEXT } = process.env const { PATHEXT: exts } = process.env const { Pathext } = opts.env were all unflagged. That is a common idiom, not an exotic bypass. An ObjectPattern visitor now catches it in every form — renamed, any casing, any receiver, string keys — while leaving a computed key alone, since it is not statically decidable. I10-I13 pin the invalid forms and V9/V10 pin PATH and the computed key. Two overclaims corrected, both mine: Standards review proved the docs were factually wrong. Both the ADR amendment and the CONTEXT.md entry asserted that tests/shell-command-projection-dispatch.test.cjs is still linted by this rule. It is not — the rule's surface is src, gsd-core/bin, scripts and hooks, and tests/** is deliberately outside it because test setup legitimately assigns process.env.PATHEXT (fallow-runner's P3 does exactly that). The suffix-vs-substring distinction is therefore proven by RuleTester case I9 feeding a synthetic filename, NOT by real coverage of that file. Both documents now say so. The rule's own docstring claimed the seam exemption matches the seam path 'exactly'. It is a suffix match, so a nested foo/src/shell-command-projection.cts would also be exempt. Suffix matching is kept — it is how sibling rules resolve paths and the nested case does not exist — but the docstring now states the boundary rather than overstating the precision. The evasion fix was verified by executing eslint against both destructuring forms in scripts/, not by inspection. Probe: 31/31. Refs #3411 * chore(#3619): backfill changeset pr number 3636 --------- Co-authored-by: sim --- .changeset/steady-wasps-jump.md | 5 + CONTEXT.md | 2 +- ...03-portability-enforcement-architecture.md | 33 ++ eslint-rules/lib/portability-vocab.cjs | 65 ++++ eslint-rules/no-private-binary-resolution.cjs | 166 +++++++++ eslint.config.mjs | 38 ++ src/fallow-runner.cts | 14 +- src/shell-command-projection.cts | 19 +- tests/fallow-runner.test.cjs | 39 ++ ...no-private-binary-resolution.rule.test.cjs | 341 ++++++++++++++++++ ...shell-command-projection-dispatch.test.cjs | 119 ++++++ 11 files changed, 832 insertions(+), 9 deletions(-) create mode 100644 .changeset/steady-wasps-jump.md create mode 100644 eslint-rules/no-private-binary-resolution.cjs create mode 100644 tests/no-private-binary-resolution.rule.test.cjs diff --git a/.changeset/steady-wasps-jump.md b/.changeset/steady-wasps-jump.md new file mode 100644 index 000000000..3e8408e45 --- /dev/null +++ b/.changeset/steady-wasps-jump.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3636 +--- +**A lint rule now keeps Windows binary resolution in one place** — re-implementing PATH/PATHEXT lookup outside the platform seam is rejected at lint time, so the four divergent resolvers epic #3411 removed cannot quietly come back. No change to how GSD behaves at runtime. (#3619) diff --git a/CONTEXT.md b/CONTEXT.md index 6211f4a60..74ecc90fe 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -825,7 +825,7 @@ The prompt-level data/instruction isolation seam for untrusted web/document ingr ## Shell Command Projection Module (expanded glossary entry, 2026-05-13) -Module owning all OS-facing I/O for the tool: runtime-aware command-text rendering (hook commands, PATH action lines, shim scripts), subprocess dispatch (execGit, execNpm, execTool, probeTty, isSpawnTimeout), Windows binary resolution (resolveExecutableBinary, projectSpawnInvocation), and platform file I/O (platformWriteSync, platformReadSync, platformEnsureDir). Single seam for platform-conditional logic — one place to fix any shell or file write regression across Windows, macOS, and Linux. WINDOWS BINARY RESOLUTION — which file a declared command name actually names, and what must be handed to `spawnSync` to start it — is owned here as of #3411 (epic #3411 Phase 1), which found the seam declaration untrue for this axis: four divergent implementations had grown outside it (`execNpm`'s `shell:true`, `execTool`'s absence of any handling, a private PATH+PATHEXT scan in `gsd-core/bin/gsd-tools.cjs`, and a fourth candidate-extension array in `fallow-runner.cts`), and #3275's fix to one provably never reached the others. `resolveExecutableBinary(name, {platform, env})` returns the RESOLVED PATH (the prior `hasBinary` computed it and threw it away): on win32 it tries PATHEXT entries ONLY and never the bare name, because npm global installs drop an extensionless POSIX `sh` shim beside `foo.CMD` and a bare-name-first scan resolves to it, leaving the ENOENT unchanged (#3275); a name already carrying a PATHEXT-listed extension is tried as-is BEFORE the append loop, and a suffix outside PATHEXT is not an extension. On POSIX it answers existence only, and `execTool` does not consult it there at all — the bare name goes to `spawnSync` unchanged and Node's own PATH search does the work, keeping macOS/Linux byte-identical for a symbol whose blast radius is CRITICAL (167 affected symbols, 53 files). `projectSpawnInvocation(command, args, {platform, env})` is the inseparable second half: `CreateProcess` cannot execute a `.cmd`/`.bat` at all, so resolution alone does not fix Windows, and exporting only the resolver would leave every caller to re-derive the mediation — which is how the four copies accumulated. It mediates through `ComSpec` with an EXPLICIT argv array (`/d /s /c`), never `shell: true`: that is CVE-2024-27980's argument-injection vector and Node 26's DEP0190. Mediation keys on the target — the resolved path, or the declared name when resolution found nothing — so a `.cmd` resident in the current directory (which PATH-only resolution misses but `cmd.exe /c` still finds) keeps working, while a BARE unresolved name is passed through untouched so the spawn fails with ENOENT rather than cmd.exe's exit 9009, preserving `_spawnResult`'s `': not found'` contract. `gsd-core/bin/gsd-tools.cjs`'s `resolveSpawnBinary` is the `bin/` entry point onto this and holds no copy of the logic. CALLERS CHOOSE HOW MUCH OF THE PROJECTION TO ADOPT, and the asymmetry is deliberate: `windowsVerbatimArguments` marks the cases where mediation was REQUIRED (the caller must take `command` and `args` together, or the batch file cannot start at all), whereas a merely-RESOLVED path is an offer a caller may decline. `execTool` declines it and keeps spawning the DECLARED name — libuv's `CreateProcess` path already performs PATH+PATHEXT search, so resolving a `.exe` there buys nothing while changing what 167 dependents observe being spawned; `tests/graphify.test.cjs` pins that contract by spying on `spawnSync`'s first argument, and the Windows CI lane caught the violation when `execTool` briefly adopted the resolved path (`python3` arriving as `C:\…\python3.EXE`). `deps.spawn` accepts it, because that lane's `hasBinary` probe answers from the same resolver and probe and spawn must agree on the exact file (#3445). Env lookups go through a case-insensitive read: Windows names the variable `Path`, `process.env` is a case-insensitive proxy that hides this, and `execTool`'s `{...process.env, ...opts.env}` spread produces a PLAIN object that keeps the OS casing and loses the proxy — an exact-case `env['PATH']` there returns undefined and the scan silently sees nothing. `resolveExecutableBinary` carries two OPT-IN options, both defaulting off so Phase 1's callers are byte-identical: `prependPaths` (directories searched before `env.PATH`, in order, with the identical per-directory candidate logic — this is how `node_modules/.bin`-first precedence is expressed without env surgery) and `requireExecutable` (POSIX-only additional `accessSync(X_OK)`; a no-op on win32, where mode bits do not mean execute). `requireExecutable` is opt-in rather than default because making it unconditional would break #3445's own suite, which stages candidates with plain `writeFileSync` and never sets an exec bit — the repo bans `chmod` in tests — so every one of those would resolve to null on POSIX. As of #3618 (epic #3411 Phase 2) `src/fallow-runner.cts` holds no resolver of its own: `resolveFallowBinary` is one seam call passing `prependPaths: [/node_modules/.bin]` and `requireExecutable: true`, preserving `.bin`-before-PATH precedence and the POSIX executability check. Its prior win32 candidate list ended in a BARE `fallow`; the seam does not, and dropping it is the fix — an extensionless file beside `fallow.cmd` is npm's POSIX `sh` shim that `CreateProcess` cannot run (#3275). Note the precedence was documented BACKWARDS (`PATH` then `.bin`) in `structural-pre-pass.md` and four INVENTORY translations until #3618 corrected them; the code was always `.bin` first. Known gap after Phase 2: no lint ratchet yet rejects a bare-binary spawn outside this seam (Phase 3, #3619). `isSpawnTimeout` is the single shared "did this subprocess time out" predicate (error.code==='ETIMEDOUT' only — cross-platform-safe; does not require signal==='SIGTERM'), consumed by worktree-safety.cts, worktree-base-ref.cts, commands.cts, and this module's own dispatchGsdCommand (#3050 — "Generative Fix Divergence"). `projectPathExportLine(targetDir)` is the single source of the `export PATH=":$PATH"` line for all three PATH-persistence lanes (repair, persist, win32 Git Bash) — it double-quote-escapes for the line's final rc-file context before any lane single-quotes it for its own `echo` transport, closing the #3118 command-substitution injection where a lane re-escaped the line itself and let a `$(…)` in the target dir execute on every new shell; the win32 cmd.exe lane fails closed (empty actions) whenever the target dir contains `"`, since that character is reserved on Windows and would otherwise close cmd's quoted region, and now tags that empty result with a typed `PATH_ACTION_REASON` (`win32_reserved_quote`) so it stays distinguishable from the unrelated "no target directory given" empty result (`no_target_dir`, #3118); the fish lane emits `fish_add_path -- ''` (`--` end-of-options separator, verified empirically against fish 4.8.1), since a leading-dash target dir is otherwise misparsed by fish's argparse-based option scanning regardless of quoting; `escapeTomlDoubleQuotedString` now escapes TOML's required control characters (U+0000-U+0008, U+000A-U+001F, U+007F), not just backslash and quote, closing a raw-newline/CR/NUL config.toml parse failure (#3118). Lives in `gsd-core/bin/lib/shell-command-projection.cjs`. See ADR-0009 (superseded "does not execute" constraint) and ADR-0010 (superseded File Operation Engine). +Module owning all OS-facing I/O for the tool: runtime-aware command-text rendering (hook commands, PATH action lines, shim scripts), subprocess dispatch (execGit, execNpm, execTool, probeTty, isSpawnTimeout), Windows binary resolution (resolveExecutableBinary, projectSpawnInvocation), and platform file I/O (platformWriteSync, platformReadSync, platformEnsureDir). Single seam for platform-conditional logic — one place to fix any shell or file write regression across Windows, macOS, and Linux. WINDOWS BINARY RESOLUTION — which file a declared command name actually names, and what must be handed to `spawnSync` to start it — is owned here as of #3411 (epic #3411 Phase 1), which found the seam declaration untrue for this axis: four divergent implementations had grown outside it (`execNpm`'s `shell:true`, `execTool`'s absence of any handling, a private PATH+PATHEXT scan in `gsd-core/bin/gsd-tools.cjs`, and a fourth candidate-extension array in `fallow-runner.cts`), and #3275's fix to one provably never reached the others. `resolveExecutableBinary(name, {platform, env})` returns the RESOLVED PATH (the prior `hasBinary` computed it and threw it away): on win32 it tries PATHEXT entries ONLY and never the bare name, because npm global installs drop an extensionless POSIX `sh` shim beside `foo.CMD` and a bare-name-first scan resolves to it, leaving the ENOENT unchanged (#3275); a name already carrying a PATHEXT-listed extension is tried as-is BEFORE the append loop, and a suffix outside PATHEXT is not an extension. On POSIX it answers existence only, and `execTool` does not consult it there at all — the bare name goes to `spawnSync` unchanged and Node's own PATH search does the work, keeping macOS/Linux byte-identical for a symbol whose blast radius is CRITICAL (167 affected symbols, 53 files). `projectSpawnInvocation(command, args, {platform, env})` is the inseparable second half: `CreateProcess` cannot execute a `.cmd`/`.bat` at all, so resolution alone does not fix Windows, and exporting only the resolver would leave every caller to re-derive the mediation — which is how the four copies accumulated. It mediates through `ComSpec` with an EXPLICIT argv array (`/d /s /c`), never `shell: true`: that is CVE-2024-27980's argument-injection vector and Node 26's DEP0190. Mediation keys on the target — the resolved path, or the declared name when resolution found nothing — so a `.cmd` resident in the current directory (which PATH-only resolution misses but `cmd.exe /c` still finds) keeps working, while a BARE unresolved name is passed through untouched so the spawn fails with ENOENT rather than cmd.exe's exit 9009, preserving `_spawnResult`'s `': not found'` contract. `gsd-core/bin/gsd-tools.cjs`'s `resolveSpawnBinary` is the `bin/` entry point onto this and holds no copy of the logic. CALLERS CHOOSE HOW MUCH OF THE PROJECTION TO ADOPT, and the asymmetry is deliberate: `windowsVerbatimArguments` marks the cases where mediation was REQUIRED (the caller must take `command` and `args` together, or the batch file cannot start at all), whereas a merely-RESOLVED path is an offer a caller may decline. `execTool` declines it and keeps spawning the DECLARED name — libuv's `CreateProcess` path already performs PATH+PATHEXT search, so resolving a `.exe` there buys nothing while changing what 167 dependents observe being spawned; `tests/graphify.test.cjs` pins that contract by spying on `spawnSync`'s first argument, and the Windows CI lane caught the violation when `execTool` briefly adopted the resolved path (`python3` arriving as `C:\…\python3.EXE`). `deps.spawn` accepts it, because that lane's `hasBinary` probe answers from the same resolver and probe and spawn must agree on the exact file (#3445). Env lookups go through a case-insensitive read: Windows names the variable `Path`, `process.env` is a case-insensitive proxy that hides this, and `execTool`'s `{...process.env, ...opts.env}` spread produces a PLAIN object that keeps the OS casing and loses the proxy — an exact-case `env['PATH']` there returns undefined and the scan silently sees nothing. `resolveExecutableBinary` carries two OPT-IN options, both defaulting off so Phase 1's callers are byte-identical: `prependPaths` (directories searched before `env.PATH`, in order, with the identical per-directory candidate logic — this is how `node_modules/.bin`-first precedence is expressed without env surgery) and `requireExecutable` (POSIX-only additional `accessSync(X_OK)`; a no-op on win32, where mode bits do not mean execute). `requireExecutable` is opt-in rather than default because making it unconditional would break #3445's own suite, which stages candidates with plain `writeFileSync` and never sets an exec bit — the repo bans `chmod` in tests — so every one of those would resolve to null on POSIX. As of #3618 (epic #3411 Phase 2) `src/fallow-runner.cts` holds no resolver of its own: `resolveFallowBinary` is one seam call passing `prependPaths: [/node_modules/.bin]` and `requireExecutable: true`, preserving `.bin`-before-PATH precedence and the POSIX executability check. Its prior win32 candidate list ended in a BARE `fallow`; the seam does not, and dropping it is the fix — an extensionless file beside `fallow.cmd` is npm's POSIX `sh` shim that `CreateProcess` cannot run (#3275). Note the precedence was documented BACKWARDS (`PATH` then `.bin`) in `structural-pre-pass.md` and four INVENTORY translations until #3618 corrected them; the code was always `.bin` first. As of #3619 (epic #3411 Phase 3) the seam's ownership is RATCHETED by `local/no-private-binary-resolution` (`eslint-rules/no-private-binary-resolution.cjs`, ADR-1703 catalog): re-implementing Windows binary resolution outside `src/shell-command-projection.cts` is an eslint error, keyed on the two unambiguous signals — reading `PATHEXT` in any casing from any object, and a hardcoded list carrying two or more of `.exe`/`.cmd`/`.bat`/`.com` (the shapes all four deleted resolvers actually had). `DEFECT.WINDOWS-PRIVATE-BINARY-RESOLUTION`. It deliberately does NOT flag a bare-name spawn — ~30 such sites exist and none is a defect, since `git`/`gh`/`npm` ship native `.exe` that `CreateProcess` resolves unaided — nor a `PATH` scan, which is indistinguishable from a legitimate membership check (`bin/install.js`). The extension threshold is TWO because a single `.endsWith('.cmd')` is a classification, not a candidate set (`runtime-hooks-surface.cts` derives `.cmd` shim paths that way), and matching is boundary-aware because a naive substring test flags `.execute` and `.compacting`. The seam exemption is path-SUFFIX anchored, not substring; the rule's own surface is `src/**/*.cts`, `gsd-core/bin/**/*.cjs`, `scripts/**/*.cjs`, and `hooks/**/*.js`, and `tests/**` is deliberately outside that surface — test setup legitimately assigns `process.env.PATHEXT` (`tests/fallow-runner.test.cjs`'s P3 case), so `tests/shell-command-projection-dispatch.test.cjs` is NOT linted by this rule at all; the suffix-vs-substring distinction is instead proven by RuleTester case I9's synthetic filename, not by real-world coverage of that test file. `eslint-rules/**` is outside the rule's globs entirely rather than exempted, because `lib/portability-vocab.cjs` owns the extension set. To make the ratchet strict with no carve-out, `resolveExecutableBinary` also gained `pathOverride` — "search THIS PATH, read everything else including PATHEXT from the ambient environment" — so `resolveFallowBinary` supplies its own search path without hand-threading PATHEXT, which would itself have been a private PATHEXT read. `pathOverride: ''` means an EMPTY search path, never a fallback to `env.PATH` (`!== undefined`, not truthiness). `isSpawnTimeout` is the single shared "did this subprocess time out" predicate (error.code==='ETIMEDOUT' only — cross-platform-safe; does not require signal==='SIGTERM'), consumed by worktree-safety.cts, worktree-base-ref.cts, commands.cts, and this module's own dispatchGsdCommand (#3050 — "Generative Fix Divergence"). `projectPathExportLine(targetDir)` is the single source of the `export PATH=":$PATH"` line for all three PATH-persistence lanes (repair, persist, win32 Git Bash) — it double-quote-escapes for the line's final rc-file context before any lane single-quotes it for its own `echo` transport, closing the #3118 command-substitution injection where a lane re-escaped the line itself and let a `$(…)` in the target dir execute on every new shell; the win32 cmd.exe lane fails closed (empty actions) whenever the target dir contains `"`, since that character is reserved on Windows and would otherwise close cmd's quoted region, and now tags that empty result with a typed `PATH_ACTION_REASON` (`win32_reserved_quote`) so it stays distinguishable from the unrelated "no target directory given" empty result (`no_target_dir`, #3118); the fish lane emits `fish_add_path -- ''` (`--` end-of-options separator, verified empirically against fish 4.8.1), since a leading-dash target dir is otherwise misparsed by fish's argparse-based option scanning regardless of quoting; `escapeTomlDoubleQuotedString` now escapes TOML's required control characters (U+0000-U+0008, U+000A-U+001F, U+007F), not just backslash and quote, closing a raw-newline/CR/NUL config.toml parse failure (#3118). Lives in `gsd-core/bin/lib/shell-command-projection.cjs`. See ADR-0009 (superseded "does not execute" constraint) and ADR-0010 (superseded File Operation Engine). Invariants: - Result shape: all exec* functions return `{ exitCode, stdout, stderr }`; never throw on non-zero exit code. diff --git a/docs/adr/1703-portability-enforcement-architecture.md b/docs/adr/1703-portability-enforcement-architecture.md index 986a5c73c..fee05e77b 100644 --- a/docs/adr/1703-portability-enforcement-architecture.md +++ b/docs/adr/1703-portability-enforcement-architecture.md @@ -76,6 +76,39 @@ Replace all three with a single coherent mechanism: **AST-based ESLint rules in | `require-userprofile-with-home` | `DEFECT.WINDOWS-TEST-PORTABILITY` (G6) | tests | | `normalize-path-in-content` | `DEFECT.WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT` (`RULESET.CONTENT-PATH-NORMALIZATION`) | `src/**/*.cts` | | `require-fs-op-fallback` | `DEFECT.WINDOWS-FS-OPS` | `src/**/*.cts`, build/install | +| `no-private-binary-resolution` | `DEFECT.WINDOWS-PRIVATE-BINARY-RESOLUTION` | `src/**/*.cts`, `gsd-core/bin/**`, `scripts/**`, `hooks/**` | + +**Amendment (2026-08-18, epic #3411 Phase 3 / #3619).** `no-private-binary-resolution` is the +first catalog entry added after the original seven, and it extends this architecture to a +**production** Windows-runtime class rather than a test-portability one. It flags the two +unambiguous signals of re-implementing Windows binary resolution outside the platform seam: +reading `PATHEXT` (in any casing, from any object), and a hardcoded list containing two or more of +`.exe`/`.cmd`/`.bat`/`.com`. Both are exactly the shapes the four resolvers that epic #3411 deleted +actually had. + +It deliberately does **not** flag a bare-name `spawn`, which is what #3411's own text asked for: +~30 such call sites exist and none is a defect (`git`, `gh`, `npm` ship native `.exe` on Windows), +so under rules 2 and 3 above a literal rule would be unsuppressable and would force rewriting +correct code. It also does not flag a `PATH` scan, which is used for legitimate membership checks +(`bin/install.js`) and cannot be soundly distinguished from a resolution scan. An unsound rule in a +zero-escape-hatch architecture is worse than no rule. + +Two scoping consequences worth recording, because both were arrived at rather than assumed: + +- **`eslint-rules/**` is outside this rule's surface** — `lib/portability-vocab.cjs` owns the + extension set per rule 4 and would otherwise flag itself. This is expressed by *not linting that + tree*, never by an exemption, so rule 3 holds. +- **The seam exemption is path-suffix-anchored**, matching `src/shell-command-projection.cts` + or any path ending in `/src/shell-command-projection.cts` — not a substring match. The rule's + own configured surface is `src/**/*.cts`, `gsd-core/bin/**/*.cjs`, `scripts/**/*.cjs`, and + `hooks/**/*.js`; `tests/**` is deliberately outside that surface, because test setup + legitimately assigns `process.env.PATHEXT` (`tests/fallow-runner.test.cjs`'s P3 case does this). + So the suffix-vs-substring distinction is proven only by case I9 of the rule's `RuleTester` + suite, which feeds the rule a synthetic filename directly — not by real-world linting of + `tests/shell-command-projection-dispatch.test.cjs`, which this rule never scans. + +The rule started **green** with nothing suppressed and nothing grandfathered — Phases 1 and 2 had +already removed every private resolver, which is what made a strict ratchet possible at all. **Taxonomy coverage.** This catalog addresses every `DEFECT.WINDOWS-*` class plus `DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE` in `CONTEXT.md`, to the extent each is *statically* diff --git a/eslint-rules/lib/portability-vocab.cjs b/eslint-rules/lib/portability-vocab.cjs index fc5f5f8ba..c9b639fc0 100644 --- a/eslint-rules/lib/portability-vocab.cjs +++ b/eslint-rules/lib/portability-vocab.cjs @@ -22,6 +22,14 @@ * deeper nesting (e.g. `String(path.join(...)).toLowerCase().replace(/\\/g,'/')`) * is not covered — only the outermost call and one level of String() cast are * visible to the rule. + * + * countWindowsExecutableExtensions matches by spelling (case-insensitive) + * against the fixed WINDOWS_EXECUTABLE_EXTENSIONS set below — it does not + * parse the string as a PATHEXT-style delimited list, so it also matches a + * bare substring occurrence (e.g. `.exe` inside `.exeFoo`). That is + * deliberate: the rule this feeds (local/no-private-binary-resolution) only + * needs "does this string carry two-or-more of these extensions", the same + * shape both deleted private resolvers had, not a strict grammar. */ /** @@ -74,6 +82,60 @@ const PATH_RETURNING_FNS = [ 'toPosixPath', ]; +/** + * Windows executable extensions (lowercase canonical). This is the fixed + * candidate-extension set both deleted private resolvers (`fallow-runner`'s + * `['fallow.exe','fallow.cmd','fallow.bat','fallow']` and `gsd-tools`' + * `'.EXE;.CMD;.BAT;.COM'`) hardcoded; local/no-private-binary-resolution + * flags a re-occurrence of two-or-more of these outside the seam. + */ +const WINDOWS_EXECUTABLE_EXTENSIONS = ['.exe', '.cmd', '.bat', '.com']; + +/** + * The Windows PATHEXT environment variable name. Windows environment variable + * names are case-insensitive, so a read of ANY casing of this name (`Pathext`, + * `pathext`, ...) is the signal local/no-private-binary-resolution matches on. + */ +const PATHEXT_VAR_NAME = 'PATHEXT'; + +/** + * Boundary-aware match for each WINDOWS_EXECUTABLE_EXTENSIONS entry, in the + * same order. A plain substring test misfires on ordinary prose/event-name + * tokens that merely start with the same three letters — e.g. `.execute` + * contains the substring `.exe`, and `.compacting` contains `.com`, neither + * of which is a Windows executable extension (a real false positive this + * caught in src/host-integration.cts's OPENCODE_EXTENSION_EVENTS list). The + * negative lookahead requires the match NOT be immediately followed by + * another letter, so `.exe` at the end of a token or before a delimiter + * (`;`, `.`, `'`, whitespace, end-of-string) still matches, but `.exe` inside + * `.execute` does not. Hand-written RegExp literals (not built from a + * runtime string) so this itself is not an adhoc-regex-escape construction. + */ +const WINDOWS_EXECUTABLE_EXTENSION_PATTERNS = [ + /\.exe(?![a-zA-Z])/i, + /\.cmd(?![a-zA-Z])/i, + /\.bat(?![a-zA-Z])/i, + /\.com(?![a-zA-Z])/i, +]; + +/** + * Returns how many DISTINCT entries of WINDOWS_EXECUTABLE_EXTENSIONS appear + * (case-insensitively, boundary-aware) in `str`. Used by + * local/no-private-binary-resolution to apply its two-or-more threshold — + * see the "Known boundaries" note above for the matching semantics. + * + * @param {string} str + * @returns {number} + */ +function countWindowsExecutableExtensions(str) { + if (typeof str !== 'string' || str.length === 0) return 0; + let count = 0; + for (const pattern of WINDOWS_EXECUTABLE_EXTENSION_PATTERNS) { + if (pattern.test(str)) count += 1; + } + return count; +} + /** * Returns true when `node` is a CallExpression whose callee matches one of the * PATH_RETURNING_FNS entries. @@ -337,6 +399,9 @@ function unwrapNonNormalizerMethodChain(node) { module.exports = { PATH_RETURNING_FNS, + WINDOWS_EXECUTABLE_EXTENSIONS, + PATHEXT_VAR_NAME, + countWindowsExecutableExtensions, isPathReturningCall, isPosixSlashStringLiteral, isPosixNormalizerCall, diff --git a/eslint-rules/no-private-binary-resolution.cjs b/eslint-rules/no-private-binary-resolution.cjs new file mode 100644 index 000000000..5e5dab887 --- /dev/null +++ b/eslint-rules/no-private-binary-resolution.cjs @@ -0,0 +1,166 @@ +'use strict'; + +const path = require('node:path'); +const { PATHEXT_VAR_NAME, countWindowsExecutableExtensions } = require('./lib/portability-vocab.cjs'); + +/** + * no-private-binary-resolution + * + * Epic #3411's actual thesis: four private Windows-binary-resolution + * implementations existed (execNpm's shell:true, execTool's absence of any + * handling, gsd-tools.cjs's private scan, and fallow-runner.cts's own + * candidate array). All four are gone; this rule stops a fifth from + * accreting by flagging the two unambiguous "I am re-implementing Windows + * binary resolution" shapes outside the platform seam + * (src/shell-command-projection.cts): + * + * 1. Reading PATHEXT from any object, any casing — Windows environment + * variable names are case-insensitive, and nothing reads PATHEXT for a + * reason other than locating an executable. This covers both member + * access (`env.PATHEXT`, `env['Pathext']`) and destructuring + * (`const { PATHEXT } = env`, `const { PATHEXT: exts } = env`), from + * any source object. + * 2. A hardcoded Windows executable-extension list (an ArrayExpression or + * a single string literal) carrying two or more of .exe/.cmd/.bat/.com + * — the exact shape both deleted implementations had. A SINGLE + * extension is deliberately not flagged: that is a classification + * (`p.endsWith('.cmd')`) or a shim-path derivation + * (`scriptPath.replace(/\.js$/, '.cmd')`), not a candidate list, and + * the tree has legitimate instances of both. + * + * The seam exemption is PATH-SUFFIX ANCHORED, not substring-matched: a file + * is exempt only when its repo-relative path IS `src/shell-command-projection.cts` + * or ENDS WITH `/src/shell-command-projection.cts` — so a file nested under + * any parent directory at that suffix (e.g. `foo/src/shell-command-projection.cts`) + * is exempt too, but a file that merely contains that string as a substring + * elsewhere in its own path, or as a `.bak` / `.test.cts` variant of the + * filename itself, is NOT exempt (case I9 pins this distinction) — a + * substring exemption would be the obvious wrong implementation here. + * + * See .gsd/phase/chore-3619-no-bare-binary-spawn/40-design.md for the full + * behavior table and rejected alternatives. + */ + +const SEAM_RELATIVE_PATH = 'src/shell-command-projection.cts'; + +/** + * True when `filename` IS the seam file, matched by path SUFFIX after + * normalizing separators to `/` — never by substring containment anywhere + * else in the path. + * + * @param {string} filename + * @returns {boolean} + */ +function isSeamFile(filename) { + if (typeof filename !== 'string' || filename.length === 0) return false; + const normalized = filename.split(path.sep).join('/'); + return normalized === SEAM_RELATIVE_PATH || normalized.endsWith(`/${SEAM_RELATIVE_PATH}`); +} + +/** True when `node` is a string Literal whose value case-insensitively equals PATHEXT_VAR_NAME. */ +function isPathextStringLiteral(node) { + return !!node && node.type === 'Literal' && typeof node.value === 'string' + && node.value.toLowerCase() === PATHEXT_VAR_NAME.toLowerCase(); +} + +/** True when `node` is an Identifier whose name case-insensitively equals PATHEXT_VAR_NAME. */ +function isPathextIdentifier(node) { + return !!node && node.type === 'Identifier' + && node.name.toLowerCase() === PATHEXT_VAR_NAME.toLowerCase(); +} + +/** + * True when a MemberExpression's property resolves to PATHEXT, any casing: + * - non-computed (dot access): property is always an Identifier — `env.PATHEXT`, `env.pathext` + * - computed (bracket access): property may be a string Literal — `env['PATHEXT']` — + * or an Identifier referencing a same-named local variable — `env[PATHEXT]` + * (row 10 in the design doc: a variable *named* PATHEXT, accepted false-positive risk) + */ +function memberExpressionReadsPathext(node) { + const property = node.property; + if (!node.computed) return isPathextIdentifier(property); + return isPathextStringLiteral(property) || isPathextIdentifier(property); +} + +/** + * True when an ObjectPattern `Property`'s key resolves to PATHEXT, any casing — + * regardless of what is being destructured (`process.env`, `env`, `opts.env`, + * anything) and regardless of renaming (`const { PATHEXT: exts } = ...`): + * - non-computed key: an Identifier (`{ PATHEXT }`, `{ Pathext: v }`) or a + * string Literal (`{ 'PATHEXT': v }`) + * - computed key (`{ [expr]: v }`): only a string Literal is statically + * decidable (`{ ['PATHEXT']: v }`); a variable expression like `{ [key]: v }` + * is NOT decidable and must not be reported (an Identifier that happens to + * be *named* PATHEXT is still accepted, mirroring the MemberExpression + * computed case above and its accepted false-positive risk). + */ +function objectPatternPropertyReadsPathext(property) { + if (!property || property.type !== 'Property') return false; + const key = property.key; + return isPathextIdentifier(key) || isPathextStringLiteral(key); +} + +/** @type {import('eslint').Rule.RuleModule} */ +const rule = { + meta: { + type: 'problem', + docs: { + description: 'Disallow re-implementing Windows binary resolution outside the platform seam', + category: 'Portability', + }, + schema: [], + messages: { + pathextRead: + 'Reading PATHEXT is Windows binary resolution — route through resolveExecutableBinary ' + + 'in src/shell-command-projection.cts instead. Four private resolvers is what epic #3411 removed.', + extensionList: + 'A hardcoded Windows executable-extension list is a private resolver candidate set — ' + + 'use the seam\'s PATHEXT handling in src/shell-command-projection.cts instead.', + }, + }, + + create(context) { + const filename = typeof context.filename === 'string' ? context.filename : context.getFilename(); + if (isSeamFile(filename)) return {}; + + return { + MemberExpression(node) { + if (memberExpressionReadsPathext(node)) { + context.report({ node, messageId: 'pathextRead' }); + } + }, + + ObjectPattern(node) { + for (const property of node.properties) { + if (objectPatternPropertyReadsPathext(property)) { + context.report({ node: property, messageId: 'pathextRead' }); + } + } + }, + + ArrayExpression(node) { + const stringValues = node.elements + .filter((el) => el && el.type === 'Literal' && typeof el.value === 'string') + .map((el) => el.value); + if (stringValues.length === 0) return; + if (countWindowsExecutableExtensions(stringValues.join(' ')) >= 2) { + context.report({ node, messageId: 'extensionList' }); + } + }, + + Literal(node) { + if (typeof node.value !== 'string') return; + // Elements of an ArrayExpression are handled by the ArrayExpression + // visitor above (which combines the whole array's contents) — do not + // double-report the same evidence from this node's own value alone. + const parent = node.parent; + if (parent && parent.type === 'ArrayExpression' && parent.elements.includes(node)) return; + if (countWindowsExecutableExtensions(node.value) >= 2) { + context.report({ node, messageId: 'extensionList' }); + } + }, + }; + }, +}; + +module.exports = rule; diff --git a/eslint.config.mjs b/eslint.config.mjs index a587ffda8..827704071 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -30,6 +30,7 @@ import noUnboundedSpawn from './eslint-rules/no-unbounded-spawn.cjs'; import noDuplicateFoldMarker from './eslint-rules/no-duplicate-fold-marker.cjs'; import requireSubprocessTimeout from './eslint-rules/require-subprocess-timeout.cjs'; import noExternalRequireInBin from './eslint-rules/no-external-require-in-bin.cjs'; +import noPrivateBinaryResolution from './eslint-rules/no-private-binary-resolution.cjs'; const localPlugin = { rules: { @@ -54,6 +55,7 @@ const localPlugin = { 'no-duplicate-fold-marker': noDuplicateFoldMarker, 'require-subprocess-timeout': requireSubprocessTimeout, 'no-external-require-in-bin': noExternalRequireInBin, + 'no-private-binary-resolution': noPrivateBinaryResolution, }, }; @@ -376,6 +378,11 @@ export default tseslint.config( // place a bad external import in an already-migrated module is still // visible to lint. 'local/no-external-require-in-bin': 'error', + // #3619 (epic #3411 Phase 3): flag a re-implemented Windows binary + // resolver — a PATHEXT read or a hardcoded exe-extension list — outside + // the platform seam (src/shell-command-projection.cts, exempt by path). + // See .gsd/phase/chore-3619-no-bare-binary-spawn/40-design.md. + 'local/no-private-binary-resolution': 'error', }, }, @@ -487,6 +494,35 @@ export default tseslint.config( }, rules: { 'local/no-external-require-in-bin': 'error', + // #3619 (epic #3411 Phase 3): see the src/**/*.cts block above for detail. + 'local/no-private-binary-resolution': 'error', + }, + }, + + // ── scripts/**/*.cjs only — no-private-binary-resolution ─────────────────── + // A NARROWER block than the combined CommonJS glob above, on purpose: + // eslint-rules/** is deliberately OUTSIDE this rule's surface, because + // eslint-rules/lib/portability-vocab.cjs is the single source of truth for + // the Windows executable-extension set (ADR-1703 rule 4) — its own + // WINDOWS_EXECUTABLE_EXTENSIONS vocabulary array would trip the rule it + // feeds. Registering on the shared `gsd-core/bin/**/*.cjs + scripts/**/*.cjs + // + eslint-rules/**/*.cjs + ...` block would flag that file; this block + // covers scripts/**/*.cjs only, so the rule still lints every other script + // in the tree without the vocabulary file self-flagging. + { + files: ['scripts/**/*.cjs'], + plugins: { + local: localPlugin, + }, + languageOptions: { + sourceType: 'commonjs', + globals: { + ...globals.node, + }, + }, + rules: { + // #3619 (epic #3411 Phase 3): see the src/**/*.cts block above for detail. + 'local/no-private-binary-resolution': 'error', }, }, @@ -505,6 +541,8 @@ export default tseslint.config( 'n/no-path-concat': 'error', // ADR-3212 Phase 1 (#3412): pattern-construction seam prohibition. 'local/no-adhoc-regex-escape': 'error', + // #3619 (epic #3411 Phase 3): see the src/**/*.cts block above for detail. + 'local/no-private-binary-resolution': 'error', // n/no-process-exit is deliberately OFF for hooks ONLY. // // A hook is a standalone process whose ENTIRE contract is its exit code: the diff --git a/src/fallow-runner.cts b/src/fallow-runner.cts index da8bfea45..b7e17e466 100644 --- a/src/fallow-runner.cts +++ b/src/fallow-runner.cts @@ -28,12 +28,14 @@ export interface ResolveFallowOpts { export function resolveFallowBinary({ cwd, envPath = process.env['PATH'] ?? '' }: ResolveFallowOpts): string | null { return resolveExecutableBinary('fallow', { prependPaths: [path.join(cwd, 'node_modules', '.bin')], - // PATHEXT is passed through (not spread from the rest of process.env) so - // win32 resolution still works, while avoiding the spread-loses-the- - // case-insensitive-proxy hazard the Windows lane caught in Phase 1. `?? ''` - // is intentional: the seam falls back to its own DEFAULT_PATHEXT when this - // is empty/absent, so an empty string here is a safe "let the seam decide". - env: { PATH: envPath, PATHEXT: process.env['PATHEXT'] ?? '' }, + // #3619 (epic #3411 Phase 3): pathOverride carries envPath as the search + // path while leaving `env` unset, so the seam falls back to its default + // `env` (process.env) for everything else — PATHEXT included. Ambient + // PATHEXT therefore still governs win32 resolution exactly as before, but + // this file never reads it itself: that's what keeps this module clean + // under local/no-private-binary-resolution, which forbids a PATHEXT read + // outside the seam. + pathOverride: envPath, requireExecutable: true, }); } diff --git a/src/shell-command-projection.cts b/src/shell-command-projection.cts index b7bba2ccb..84dca3b3d 100644 --- a/src/shell-command-projection.cts +++ b/src/shell-command-projection.cts @@ -754,12 +754,26 @@ function _buildVerbatimCmdLine(target: string, args: string[]): string { * with plain `fs.writeFileSync` and never set an exec bit (the repo bans * `chmod` in tests), so every one of them would resolve to `null` on POSIX. * + * **`opts.pathOverride`** — "search THIS PATH, but read everything else — + * PATHEXT included — from the ambient environment." When set (`!== undefined`), + * the PATH search segments come from splitting `pathOverride` on `path.delimiter` + * instead of from `env.PATH`; `pathOverride: ''` means an explicit EMPTY search + * path (zero segments), never a fallback to `env.PATH` — use `!== undefined`, + * not truthiness, to tell "caller supplied a PATH string" apart from "caller + * supplied nothing". `opts.prependPaths` still comes first. PATHEXT resolution + * is UNAFFECTED by this option — it still reads from `env` (which defaults to + * `process.env`) exactly as it does when `pathOverride` is omitted. This exists + * so a caller that already has its own search path in hand (e.g. + * `resolveFallowBinary`'s `envPath`) does not have to hand-thread PATHEXT + * alongside it — a private PATHEXT read is the very shape + * `local/no-private-binary-resolution` forbids outside this seam. + * * @returns the resolved path, or `null` when nothing matched. Callers fall back to * the declared name on `null` so a genuine ENOENT still surfaces (#3086). */ export function resolveExecutableBinary( name: string | null | undefined, - opts: { platform?: string; env?: NodeJS.ProcessEnv; prependPaths?: string[]; requireExecutable?: boolean } = {}, + opts: { platform?: string; env?: NodeJS.ProcessEnv; prependPaths?: string[]; requireExecutable?: boolean; pathOverride?: string } = {}, ): string | null { if (!name) return null; const requireExecutable = opts.requireExecutable ?? false; @@ -768,7 +782,8 @@ export function resolveExecutableBinary( return _isFile(name, requireExecutable, platform) ? name : null; } const env = opts.env ?? process.env; - const pathSegments = String(_envGet(env, 'PATH') || '').split(path.delimiter).filter(Boolean); + const rawPath = opts.pathOverride !== undefined ? opts.pathOverride : (_envGet(env, 'PATH') || ''); + const pathSegments = String(rawPath).split(path.delimiter).filter(Boolean); const segments = [...(opts.prependPaths ?? []), ...pathSegments]; if (platform !== 'win32') { diff --git a/tests/fallow-runner.test.cjs b/tests/fallow-runner.test.cjs index 0f2823bd5..cfe6c32e6 100644 --- a/tests/fallow-runner.test.cjs +++ b/tests/fallow-runner.test.cjs @@ -326,4 +326,43 @@ describe('resolveFallowBinary (#3618)', () => { cleanup(cwd); } }); + + // P3 (#3619, epic #3411 Phase 3): resolveFallowBinary switched from + // `env: { PATH: envPath, PATHEXT: process.env['PATHEXT'] ?? '' }` to + // `pathOverride: envPath` so this module never reads PATHEXT itself (the + // shape local/no-private-binary-resolution forbids outside the seam). The + // seam's `env` param is left unset, so it defaults to `process.env` inside + // resolveExecutableBinary — a REAL ambient process.env.PATHEXT must still + // govern win32 resolution exactly as it did before the switch. This is the + // row the whole switch could have silently broken. + // + // PATHEXT is meaningless on POSIX (R21), so per F4's pattern above, BOTH + // platform contracts are asserted explicitly rather than skipping either: + // a `t.skip()`/bare `return` would make this a vacuous pass on that lane. + test('P3: a real ambient process.env.PATHEXT still governs win32 resolution after the env -> pathOverride switch', () => { + const cwd = createTempDir(); + const pathDir = createTempDir(); + const originalPathext = process.env.PATHEXT; + try { + process.env.PATHEXT = '.XYZ'; + const distinctiveFixture = path.join(pathDir, 'fallow.XYZ'); + fs.writeFileSync(distinctiveFixture, ''); + const resolved = resolveFallowBinary({ cwd, envPath: pathDir }); + if (process.platform === 'win32') { + // win32: ambient process.env.PATHEXT ('.XYZ') is consulted, so the + // distinctively-extensioned fixture resolves — proving the pass-through + // was preserved by pathOverride, not silently dropped. + assert.equal(resolved, distinctiveFixture); + } else { + // POSIX: PATHEXT plays no role in resolution at all (resolveExecutableBinary + // matches the bare name 'fallow' exactly); a file named 'fallow.XYZ' is not + // that name, so nothing is found. + assert.equal(resolved, null); + } + } finally { + if (originalPathext === undefined) delete process.env.PATHEXT; else process.env.PATHEXT = originalPathext; + cleanup(cwd); + cleanup(pathDir); + } + }); }); diff --git a/tests/no-private-binary-resolution.rule.test.cjs b/tests/no-private-binary-resolution.rule.test.cjs new file mode 100644 index 000000000..3c67c7048 --- /dev/null +++ b/tests/no-private-binary-resolution.rule.test.cjs @@ -0,0 +1,341 @@ +'use strict'; + +/** + * no-private-binary-resolution.rule.test.cjs + * + * RuleTester unit tests for the local/no-private-binary-resolution ESLint rule. + * Ids (V1-V8, I1-I9) map to .gsd/phase/chore-3619-no-bare-binary-spawn/50-test-matrix.md. + * + * RuleTester feeds fixtures to the rule directly and does not scan this test + * file's own source, so the self-flagging problem that forced eslint.config.mjs + * to carve the eslint-rules directory out of the scripts .cjs block does not + * arise here (ADR-1703 rule 5 / 40-design.md). + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const { RuleTester } = require('eslint'); + +const rule = require('../eslint-rules/no-private-binary-resolution.cjs'); + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'commonjs', + }, +}); + +const OUTSIDE_SEAM_FILE = 'src/some-other-module.cts'; +const SEAM_FILE = 'src/shell-command-projection.cts'; + +// ─── module shape ───────────────────────────────────────────────────────────── + +describe('no-private-binary-resolution rule module', () => { + test('exports meta and create', () => { + assert.strictEqual(typeof rule.meta, 'object'); + assert.strictEqual(typeof rule.create, 'function'); + assert.strictEqual(rule.meta.type, 'problem'); + assert.ok(rule.meta.messages.pathextRead, 'pathextRead message must exist'); + assert.ok(rule.meta.messages.extensionList, 'extensionList message must exist'); + }); +}); + +// ─── VALID cases (V1-V8) ──────────────────────────────────────────────────── + +describe('no-private-binary-resolution: valid cases', () => { + test('V1: process.env.PATHEXT in src/shell-command-projection.cts — the seam is exempt', () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [ + { + code: `const ext = process.env.PATHEXT;`, + filename: SEAM_FILE, + }, + ], + invalid: [], + }); + }); + + test("V2: ['.exe'] — one extension is classification, not a candidate list", () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [ + { + code: `const exts = ['.exe'];`, + filename: OUTSIDE_SEAM_FILE, + }, + ], + invalid: [], + }); + }); + + test("V3: p.endsWith('.cmd') — one extension", () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [ + { + code: `const isCmd = p.endsWith('.cmd');`, + filename: OUTSIDE_SEAM_FILE, + }, + ], + invalid: [], + }); + }); + + test("V4: scriptPath.replace(/\\.js$/, '.cmd') — shim-path derivation (runtime-hooks-surface.cts shape)", () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [ + { + code: `const shimPath = scriptPath.replace(/\\.js$/, '.cmd');`, + filename: OUTSIDE_SEAM_FILE, + }, + ], + invalid: [], + }); + }); + + test('V5: pathEnv.split(path.delimiter) — PATH scans are deliberately not flagged', () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [ + { + code: `const segments = pathEnv.split(path.delimiter);`, + filename: OUTSIDE_SEAM_FILE, + }, + ], + invalid: [], + }); + }); + + test('V6: process.env.PATH — only PATHEXT is the signal', () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [ + { + code: `const p = process.env.PATH;`, + filename: OUTSIDE_SEAM_FILE, + }, + ], + invalid: [], + }); + }); + + test("V7: ['.exe', '.txt'] — one exe extension plus an unrelated one, below the two-or-more threshold", () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [ + { + code: `const exts = ['.exe', '.txt'];`, + filename: OUTSIDE_SEAM_FILE, + }, + ], + invalid: [], + }); + }); + + test("V8: '.execute'/'.compacting' are substrings only — the src/host-integration.cts:724 false-positive fix", () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [ + { + code: `const OPENCODE_EXTENSION_EVENTS = ['.execute', '.compacting'];`, + filename: OUTSIDE_SEAM_FILE, + }, + ], + invalid: [], + }); + }); + + test('V9: const { PATH } = process.env; — PATH is not the signal', () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [ + { + code: `const { PATH } = process.env;`, + filename: OUTSIDE_SEAM_FILE, + }, + ], + invalid: [], + }); + }); + + test('V10: const { [key]: v } = process.env; — computed key is not statically decidable', () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [ + { + code: `const { [key]: v } = process.env;`, + filename: OUTSIDE_SEAM_FILE, + }, + ], + invalid: [], + }); + }); +}); + +// ─── INVALID cases (I1-I9) ────────────────────────────────────────────────── + +describe('no-private-binary-resolution: invalid cases', () => { + test('I1: process.env.PATHEXT outside the seam — 1 error', () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [], + invalid: [ + { + code: `const ext = process.env.PATHEXT;`, + filename: OUTSIDE_SEAM_FILE, + errors: [{ messageId: 'pathextRead' }], + }, + ], + }); + }); + + test("I2: process.env['PATHEXT'] — bracket form", () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [], + invalid: [ + { + code: `const ext = process.env['PATHEXT'];`, + filename: OUTSIDE_SEAM_FILE, + errors: [{ messageId: 'pathextRead' }], + }, + ], + }); + }); + + test("I3: env['Pathext'] — Windows env names are case-insensitive", () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [], + invalid: [ + { + code: `const ext = env['Pathext'];`, + filename: OUTSIDE_SEAM_FILE, + errors: [{ messageId: 'pathextRead' }], + }, + ], + }); + }); + + test('I4: opts.env.pathext — lower case, non-process receiver', () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [], + invalid: [ + { + code: `const ext = opts.env.pathext;`, + filename: OUTSIDE_SEAM_FILE, + errors: [{ messageId: 'pathextRead' }], + }, + ], + }); + }); + + test("I5: ['a.exe','a.cmd','a.bat','a'] — the deleted fallow-runner shape verbatim", () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [], + invalid: [ + { + code: `const candidates = ['a.exe', 'a.cmd', 'a.bat', 'a'];`, + filename: OUTSIDE_SEAM_FILE, + errors: [{ messageId: 'extensionList' }], + }, + ], + }); + }); + + test("I6: '.EXE;.CMD;.BAT;.COM' — the deleted gsd-tools shape verbatim", () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [], + invalid: [ + { + code: `const exts = '.EXE;.CMD;.BAT;.COM';`, + filename: OUTSIDE_SEAM_FILE, + errors: [{ messageId: 'extensionList' }], + }, + ], + }); + }); + + test("I7: ['.cmd', '.bat'] — exactly two, the threshold boundary from below", () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [], + invalid: [ + { + code: `const exts = ['.cmd', '.bat'];`, + filename: OUTSIDE_SEAM_FILE, + errors: [{ messageId: 'extensionList' }], + }, + ], + }); + }); + + test('I8: a file that trips BOTH signals — two errors, not one', () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [], + invalid: [ + { + code: ` + const ext = process.env.PATHEXT; + const candidates = ['a.exe', 'a.cmd']; + `, + filename: OUTSIDE_SEAM_FILE, + errors: 2, + }, + ], + }); + }); + + test('I9: process.env.PATHEXT in a file whose path merely CONTAINS the seam name as a substring — still errors (path-anchored, not substring-matched)', () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [], + invalid: [ + { + code: `const ext = process.env.PATHEXT;`, + filename: 'tests/shell-command-projection-dispatch.test.cjs', + errors: [{ messageId: 'pathextRead' }], + }, + ], + }); + }); + + test('I10: const { PATHEXT } = process.env; — destructuring evades a MemberExpression-only check', () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [], + invalid: [ + { + code: `const { PATHEXT } = process.env;`, + filename: OUTSIDE_SEAM_FILE, + errors: [{ messageId: 'pathextRead' }], + }, + ], + }); + }); + + test('I11: const { PATHEXT: exts } = process.env; — renamed destructuring', () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [], + invalid: [ + { + code: `const { PATHEXT: exts } = process.env;`, + filename: OUTSIDE_SEAM_FILE, + errors: [{ messageId: 'pathextRead' }], + }, + ], + }); + }); + + test('I12: const { Pathext } = opts.env; — casing plus non-process receiver', () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [], + invalid: [ + { + code: `const { Pathext } = opts.env;`, + filename: OUTSIDE_SEAM_FILE, + errors: [{ messageId: 'pathextRead' }], + }, + ], + }); + }); + + test("I13: const { 'PATHEXT': v } = env; — string-key destructuring", () => { + ruleTester.run('no-private-binary-resolution', rule, { + valid: [], + invalid: [ + { + code: `const { 'PATHEXT': v } = env;`, + filename: OUTSIDE_SEAM_FILE, + errors: [{ messageId: 'pathextRead' }], + }, + ], + }); + }); +}); diff --git a/tests/shell-command-projection-dispatch.test.cjs b/tests/shell-command-projection-dispatch.test.cjs index f251b6855..727acbe77 100644 --- a/tests/shell-command-projection-dispatch.test.cjs +++ b/tests/shell-command-projection-dispatch.test.cjs @@ -635,6 +635,125 @@ describe('resolveExecutableBinary seam options (#3618)', () => { }); }); +// ─── resolveExecutableBinary pathOverride (#3619, epic #3411 Phase 3) ────── +// "Search THIS PATH, but read everything else — PATHEXT included — from the +// ambient environment." Added so resolveFallowBinary can hand the seam a +// search path without also hand-threading PATHEXT (a private PATHEXT read +// is exactly the shape local/no-private-binary-resolution forbids outside +// this seam). See .gsd/phase/chore-3619-no-bare-binary-spawn/50-test-matrix.md (O1-O7). + +describe('resolveExecutableBinary pathOverride (#3619)', () => { + test('O1: pathOverride omitted — identical to today, env.PATH is used', () => { + const dir = createTempDir(); + try { + fs.writeFileSync(path.join(dir, 'foo'), ''); + const resolved = resolveExecutableBinary('foo', { platform: 'linux', env: { PATH: dir } }); + assert.equal(resolved, path.join(dir, 'foo')); + } finally { + cleanup(dir); + } + }); + + test('O2: pathOverride set, binary in the pathOverride dir, env.PATH points elsewhere — resolves via pathOverride, env.PATH ignored', () => { + const dirA = createTempDir(); + const dirElsewhere = createTempDir(); + try { + fs.writeFileSync(path.join(dirA, 'foo'), ''); + const resolved = resolveExecutableBinary('foo', { + platform: 'linux', + env: { PATH: dirElsewhere }, + pathOverride: dirA, + }); + assert.equal(resolved, path.join(dirA, 'foo')); + } finally { + cleanup(dirA); + cleanup(dirElsewhere); + } + }); + + test("O3: pathOverride: '' with a populated env.PATH — null; empty means empty, not a fallback to env.PATH", () => { + const dir = createTempDir(); + try { + fs.writeFileSync(path.join(dir, 'foo'), ''); + const resolved = resolveExecutableBinary('foo', { + platform: 'linux', + env: { PATH: dir }, + pathOverride: '', + }); + assert.equal(resolved, null); + } finally { + cleanup(dir); + } + }); + + test('O4: pathOverride + prependPaths — prepended dirs still come first', () => { + const dirPrepend = createTempDir(); + const dirOverride = createTempDir(); + try { + fs.writeFileSync(path.join(dirPrepend, 'foo'), ''); + fs.writeFileSync(path.join(dirOverride, 'foo'), ''); + const resolved = resolveExecutableBinary('foo', { + platform: 'linux', + pathOverride: dirOverride, + prependPaths: [dirPrepend], + }); + assert.equal(resolved, path.join(dirPrepend, 'foo')); + assert.notEqual(resolved, path.join(dirOverride, 'foo')); + } finally { + cleanup(dirPrepend); + cleanup(dirOverride); + } + }); + + test('O5: pathOverride set, opts.env omitted, win32 — PATHEXT still comes from the ambient process.env, not DEFAULT_PATHEXT', () => { + const dir = createTempDir(); + const originalPathext = process.env.PATHEXT; + try { + process.env.PATHEXT = '.XYZ'; + fs.writeFileSync(path.join(dir, 'foo.XYZ'), ''); + const resolved = resolveExecutableBinary('foo', { platform: 'win32', pathOverride: dir }); + assert.equal(resolved, path.join(dir, 'foo.XYZ')); + } finally { + if (originalPathext === undefined) delete process.env.PATHEXT; else process.env.PATHEXT = originalPathext; + cleanup(dir); + } + }); + + test('O6: pathOverride AND env.PATH both set — pathOverride wins (assert WHICH path resolved, not merely that something resolved)', () => { + const dirOverride = createTempDir(); + const dirEnvPath = createTempDir(); + try { + fs.writeFileSync(path.join(dirOverride, 'foo'), ''); + fs.writeFileSync(path.join(dirEnvPath, 'foo'), ''); + const resolved = resolveExecutableBinary('foo', { + platform: 'linux', + env: { PATH: dirEnvPath }, + pathOverride: dirOverride, + }); + assert.equal(resolved, path.join(dirOverride, 'foo')); + assert.notEqual(resolved, path.join(dirEnvPath, 'foo')); + } finally { + cleanup(dirOverride); + cleanup(dirEnvPath); + } + }); + + test('O7: multi-segment pathOverride, binary in the second segment — first-match wins, in order', () => { + const dir1 = createTempDir(); + const dir2 = createTempDir(); + try { + fs.writeFileSync(path.join(dir2, 'foo'), ''); + const pathOverride = [dir1, dir2].join(path.delimiter); + const resolved = resolveExecutableBinary('foo', { platform: 'linux', pathOverride }); + assert.equal(resolved, path.join(dir2, 'foo')); + assert.notEqual(resolved, path.join(dir1, 'foo')); + } finally { + cleanup(dir1); + cleanup(dir2); + } + }); +}); + // ─── projectSpawnInvocation (#3411) ───────────────────────────────────────── // Mirrors the seam's own `_cmdQuoteToken` (a literal `"` is doubled) so