diff --git a/apps/gateway/src/local-dev.spec.ts b/apps/gateway/src/local-dev.spec.ts index cd3cc083a..5ad753540 100644 --- a/apps/gateway/src/local-dev.spec.ts +++ b/apps/gateway/src/local-dev.spec.ts @@ -25,7 +25,7 @@ import { } from 'node:fs'; import { readFile, mkdtemp, realpath, rm } from 'node:fs/promises'; import { homedir, tmpdir } from 'node:os'; -import { dirname, join } from 'node:path'; +import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { types as utilTypes } from 'node:util'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -35,6 +35,7 @@ import type * as LocalDevNamespace from './local-dev.js'; const FORBIDDEN_DSN = 'postgresql://mosaic:mosaic@localhost:5433/mosaic'; const HERE = fileURLToPath(new URL('.', import.meta.url)); +const REQUIRED_DEV_LOCAL_SCRIPT = 'exec tsx src/local-dev.ts'; // --- Z1-Z5 instruments. Nothing real is reachable through these. ------------- const bootstrapMock = vi.fn(async () => {}); @@ -273,7 +274,7 @@ describe('KBN-101-02a local-dev — the channel has its own command', () => { // is the shape this item exists to remove. const devLocal = manifest.scripts?.['dev:local']; expect(devLocal, 'no dev:local script').toBeDefined(); - expect(devLocal).toContain('src/local-dev.ts'); + expect(devLocal).toBe(REQUIRED_DEV_LOCAL_SCRIPT); }); it('leaves the production-like dev script untouched', async () => { @@ -288,11 +289,12 @@ describe('KBN-101-02a local-dev — the channel has its own command', () => { /** * The package script, executed rather than read. * - * The string assertions above are satisfied by `"dev:local": "tsx src/local-dev.ts"` - * pointed at a module that exports `runLocalDev()` and never calls it — which is - * exactly what this file is today: running that command right now exits 0 having - * started nothing and refused nothing. A route that silently succeeds is worse - * than one that is missing, because the operator's shell says it worked. + * The exact script assertion above is satisfied by a `dev:local` route using the + * required exec form pointed at a module that exports `runLocalDev()` and never + * calls it - which is exactly what this file is today: running that command + * right now exits 0 having started nothing and refused nothing. A route that + * silently succeeds is worse than one that is missing, because the operator's + * shell says it worked. * * Safety of running the real route in-band: the child inherits nothing and * receives only the base allowlist plus names a case explicitly declares. No @@ -342,37 +344,34 @@ describe('KBN-101-02a local-dev — the package route refuses, not merely the mo const TRUSTED_SHELL = resolveTrustedShell(); - function resolveFixtureCommand(name: string): string { - for (const directory of ['/usr/bin', '/bin']) { - const candidate = join(directory, name); - if (existsSync(candidate) && statSync(candidate).isFile()) { - return candidate; - } - } - throw new Error(`fixture command ${name} was not found in /usr/bin or /bin`); - } - function resolveTsxExecutable(): string { - for (const candidate of [ - join(GATEWAY_DIR, 'node_modules', '.bin', 'tsx'), - join(REPO_ROOT, 'node_modules', '.bin', 'tsx'), - ]) { - if (existsSync(candidate) && statSync(candidate).isFile()) { + const candidates = [ + join(GATEWAY_DIR, 'node_modules', 'tsx', 'dist', 'cli.mjs'), + join(REPO_ROOT, 'node_modules', 'tsx', 'dist', 'cli.mjs'), + ]; + for (const candidate of candidates) { + if (existsSync(candidate)) { return candidate; } } - throw new Error('tsx was not found in the gateway or repository node_modules/.bin'); + throw new Error(`tsx CLI was not found at either candidate: ${candidates.join(', ')}`); + } + + function validatePathCommandTarget(name: string, target: string): string { + const resolved = realpathSync(target); + if (!existsSync(resolved)) { + throw new Error(`${name} PATH target does not exist after realpath: ${resolved}`); + } + if (!statSync(resolved).isFile()) { + throw new Error(`${name} PATH target is not a file: ${resolved}`); + } + accessSync(resolved, fsConstants.X_OK); + return resolved; } - const TSX_EXECUTABLE = resolveTsxExecutable(); const PATH_COMMAND_TARGETS = Object.freeze({ - node: process.execPath, - tsx: TSX_EXECUTABLE, - // The pnpm-generated `tsx` launcher uses these three commands to locate and - // dispatch its JavaScript entrypoint. - dirname: resolveFixtureCommand('dirname'), - sed: resolveFixtureCommand('sed'), - uname: resolveFixtureCommand('uname'), + node: validatePathCommandTarget('node', process.execPath), + tsx: validatePathCommandTarget('tsx', resolveTsxExecutable()), }); const PATH_COMMAND_NAMES = Object.freeze(Object.keys(PATH_COMMAND_TARGETS).sort()); @@ -419,13 +418,12 @@ describe('KBN-101-02a local-dev — the package route refuses, not merely the mo * The complete base environment for one child. Built from nothing; nothing is * inherited. A case may add only the explicitly declarable route inputs. * - * `PATH` names one fixture-owned directory containing symlinks only to the - * commands the route needs. The workspace `.bin` directory carries real agent - * runtimes this repository depends on, while Node ordinarily lives in - * `/usr/bin`; the former construction therefore put thousands of executables - * on `PATH` while claiming to prevent an unrelated same-name executable from - * answering. A `#!/usr/bin/env node` shebang needs `node` on `PATH`, but `env` - * itself needs no entry because the shebang invokes it by absolute path. + * `PATH` names one fixture-owned directory containing only `node` and the + * real TSX CLI. The pnpm-generated `.bin/tsx` wrapper is deliberately rejected: + * it injects a Node loader control, and wrapper parity is not a fixture goal. + * This fixture-owned command boundary executes the same TSX CLI directly. Its + * `#!/usr/bin/env node` shebang needs `node` on `PATH`, but `env` needs no entry + * because the shebang invokes it by absolute path. * * `XDG_CONFIG_DIRS` and `XDG_DATA_DIRS` are set to private empty directories * rather than left unset. Unset is not neutral for the XDG directory *lists*: @@ -479,9 +477,6 @@ describe('KBN-101-02a local-dev — the package route refuses, not merely the mo for (const [name, target] of Object.entries(PATH_COMMAND_TARGETS)) { symlinkSync(target, join(sandboxBin, name)); } - // pnpm's shell launcher locates `../tsx/dist/cli.mjs` relative to `$0`. - // Preserve that layout outside PATH after moving `$0` into the sandbox bin. - symlinkSync(join(dirname(TSX_EXECUTABLE), '..', 'tsx'), join(sandbox, 'tsx'), 'dir'); return sandbox; } @@ -559,6 +554,7 @@ describe('KBN-101-02a local-dev — the package route refuses, not merely the mo command: string, args: string[], extraEnv: Record = {}, + timeout = CHILD_TIMEOUT_MS, ): { status: number | null; signal: NodeJS.Signals | null; @@ -571,7 +567,7 @@ describe('KBN-101-02a local-dev — the package route refuses, not merely the mo cwd: GATEWAY_DIR, env: buildHermeticChildEnv(sandbox, extraEnvSnapshot), encoding: 'utf8', - timeout: CHILD_TIMEOUT_MS, + timeout, killSignal: 'SIGKILL', }); @@ -631,6 +627,10 @@ describe('KBN-101-02a local-dev — the package route refuses, not merely the mo expect.fail('the package manifest has no dev:local script yet'); } + expect( + script, + `dev:local must be the exact required route; found ${JSON.stringify(script)}`, + ).toBe(REQUIRED_DEV_LOCAL_SCRIPT); return spawnHermetic(TRUSTED_SHELL, ['-c', script], extraEnv); } @@ -812,6 +812,41 @@ describe('KBN-101-02a local-dev — the package route refuses, not merely the mo } }); + /** + * This control's arm is a single-process `exec node -e` sleeper. For that arm, + * SIGKILL reaches the process that matters and nothing survives to write; the + * two assertions below measure only that. The required route is itself + * multi-process: TSX spawns an esbuild service child and a loader/eval child, + * where the route code runs. Measured six out of six times under this exact + * spawn shape, that loader/eval child survives SIGKILL to its TSX parent, is + * reparented, and completes its work afterward. The exact `dev:local` script + * contract blocks injected shell content, but does not close this gap: + * the permitted launcher can leave a live process holding paths inside a + * sandbox the fixture is removing. Descendant containment is an open question + * deferred to the reviewer; no assertion in this file currently proves it. + */ + it('fixture control: timeout SIGKILLs the single-process arm and it writes no marker afterward', async () => { + const timeoutMs = 200; + const writeDelayMs = 1_000; + const markerName = 'timeout-survivor'; + const markerScript = + `const { writeFileSync } = require('node:fs');` + + `setTimeout(() => writeFileSync(process.env.TMPDIR + '/${markerName}', 'SURVIVED'), ${writeDelayMs});`; + const result = spawnHermetic( + TRUSTED_SHELL, + ['-c', `exec ${JSON.stringify(process.execPath)} -e ${JSON.stringify(markerScript)}`], + {}, + timeoutMs, + ); + const marker = join(result.sandbox, 'tmp', markerName); + + expect(result.signal, `the timeout did not send SIGKILL: ${result.output}`).toBe('SIGKILL'); + await new Promise((resolve) => setTimeout(resolve, writeDelayMs + 250)); + expect(existsSync(marker), 'the killed route process survived long enough to write').toBe( + false, + ); + }); + /** * The route-arm half of the same obligation, and it is PENDING EXECUTION until * GREEN. At RED head there is no `dev:local` script, so `runDevLocal` fails on diff --git a/docs/native-kanban-sot/KBN-101-DB-ROLE-SPLIT.md b/docs/native-kanban-sot/KBN-101-DB-ROLE-SPLIT.md index 65db2f64b..45e4c79fc 100644 --- a/docs/native-kanban-sot/KBN-101-DB-ROLE-SPLIT.md +++ b/docs/native-kanban-sot/KBN-101-DB-ROLE-SPLIT.md @@ -500,8 +500,13 @@ executable-resolution hole the allowlist exists to close. **`PATH` was the last part of the child environment still built as a denylist, and calling it an allowlist was false in a way that a measurement settles.** It named the two workspace `node_modules/.bin` directories, the directory of the running Node binary, and the shell's -directory. On this host that is 4,093 distinct executable names — because `dirname(process.execPath)` -is `/usr/bin`. Worse than the count: `apps/gateway/node_modules/.bin` carries real agent-runtime +directory. Measured on the author's host that is 4,093 distinct executable names, and on the +reviewer's 2,650 — because `dirname(process.execPath)` is `/usr/bin`. Both numbers are stated with +whose run produced them on purpose. A count of what is reachable through `/usr/bin` is a property +of a host and its install state, not of the candidate tree, and presenting either figure bare would +repeat this item's own defect: a number offered as a property of the artifact when it is a property +of the observation. What does not vary between the two runs is the shape — an unbounded system +directory on a fixture `PATH`. Worse than the count: `apps/gateway/node_modules/.bin` carries real agent-runtime executables this repository depends on directly — `pi` (`@mariozechner/pi-coding-agent`), `pi-ai`, `openai`, `anthropic-ai-sdk`. At GREEN, any accidental resolution of one of those names from the package script or its process tree reaches a real agent runtime with real capability. The hard @@ -509,17 +514,32 @@ timeout bounds how long that runs; it does not prevent it. A directory list is n allowlist, and the comment on that construction claimed it stopped exactly the resolution it permitted. -`PATH` is now a single fixture-owned directory inside the per-run sandbox, holding symlinks and -nothing else: `node` (to `process.execPath`), `tsx` (to the executable the package route actually -resolves), and `dirname`, `sed`, `uname`. The last three are not decoration — the pnpm-generated -`tsx` launcher is a `/bin/sh` script that calls all three to locate its own JavaScript entrypoint, -so a `PATH` without them does not run the route at all. The launcher also finds -`../tsx/dist/cli.mjs` relative to `$0`, so the sandbox carries one directory symlink to the `tsx` -package at that relative location; it is not on `PATH`. `env` needs no entry either, because a -`#!/usr/bin/env node` shebang invokes `env` by absolute path and only `node` is resolved through -`PATH`. Measured: five reachable executable names, down from 4,093; `command -v pi` reports the real -`.bin/pi` under the old construction and not-found under the new one; `tsx --version` still succeeds -on the narrowed `PATH`. +`PATH` is now a single fixture-owned directory inside the per-run sandbox, holding two symlinks and +nothing else: `node` to `process.execPath`, and `tsx` to the TSX CLI artifact itself +(`tsx/dist/cli.mjs`). `env` needs no entry, because a `#!/usr/bin/env node` shebang invokes `env` by +absolute path and only `node` is resolved through `PATH`. Both targets are resolved through +`realpath` and asserted to be executable files at module scope — before the first child is created, +not inside a case that may run after other cases have already spawned — and the symlinks are built +from those checked values, so the construction and the control's identity assertions cannot drift +apart. Measured: two reachable executable names; `command -v pi` reports the real `.bin/pi` under +the old construction and not-found under the new one; `tsx --version` still succeeds. + +Reaching two rather than five took a second correction, and the intermediate step is worth +recording because it was wrong in an instructive direction. The first narrowing symlinked +`apps/gateway/node_modules/.bin/tsx`, which is not the TSX CLI: it is a pnpm-generated `/bin/sh` +launcher. That forced three coreutils symlinks onto `PATH` (`dirname`, `sed`, `uname`, which the +launcher calls to locate its own entrypoint) and one off-`PATH` directory symlink to restore the +`$0`-relative package layout it expects. Five names and a layout fixture, all of them serving a +wrapper. The wrapper also **exports `NODE_PATH`** naming four real repository module roots into the +process it starts — so a fixture whose stated invariant is that the child receives no Node loader +controls was shipping a `tsx` that reinstated one. That was first recorded here as a bound; the +reviewer ruled it had to be fixed rather than bounded, and the ruling produced the smaller design +rather than a larger one. Executing the CLI directly deletes the wrapper, the `NODE_PATH` +injection, three symlinks and the layout fixture at once. Measured through the two-name `PATH`, the +child's key set is `HOME PATH PWD SHLVL TMPDIR XDG_CACHE_HOME XDG_CONFIG_DIRS XDG_CONFIG_HOME +XDG_DATA_DIRS XDG_DATA_HOME XDG_RUNTIME_DIR XDG_STATE_HOME` — neither `NODE_PATH` nor +`NODE_OPTIONS`. Wrapper parity was never a fixture goal; this is a fixture-owned command boundary +executing the same CLI the route executes. The control does not assert that in prose. It splits the child's **observed** `PATH`, reads each directory, collects every entry that is an executable file, and asserts that name set **equals** @@ -534,15 +554,17 @@ become something else. Two bounds on that claim, stated because the recurring defect in this item is prose crediting a mechanism with a property it lacks. First, the claim is about **command resolution**, not about -everything the route can reach: the pnpm `tsx` launcher exports `NODE_PATH` naming four real -repository module roots into the process it starts, and the last of them -(`node_modules/.pnpm/node_modules`) exposes 775 packages — `@mariozechner/pi-coding-agent`, -`@anthropic-ai` and `openai` among them. Narrowing `PATH` removes those runtimes as _commands_; it -does not remove them as _modules_, and nothing at this layer could, because the route legitimately -runs repository code against repository `node_modules`. Second, executable enumeration is a -mechanical coverage check and is separate from reading source: the review that passed this fixture -before the hole was found was bounded by paths whose directory contents it never listed, which is -precisely why the guarantee is now measured by the fixture rather than observed by a reviewer. +everything the route can reach. Removing the wrapper removes the `NODE_PATH` the fixture was +injecting, and the two-name child carries no loader control at all — but at GREEN the route runs +repository code against repository `node_modules`, and `node_modules/.pnpm/node_modules` holds 775 +packages including `@mariozechner/pi-coding-agent`, `@anthropic-ai` and `openai`. Those runtimes +are not resolvable as _commands_; they remain present as _modules_, and nothing at this layer could +change that without changing what the route is. "Not resolvable as a command" is the whole claim, +and "unreachable" is not a sentence this document gets to write. Second, executable enumeration is +a mechanical coverage check and is separate from reading source: the review that passed this +fixture before the hole was found was bounded by paths whose directory contents it never listed, +which is precisely why the guarantee is now measured by the fixture rather than observed by a +reviewer. This is the third real-runtime-reachability incident in this window — the FL-09 ambient OpenCode fall-through, the review command-substitution Claude launch, and now real agent executables on a @@ -562,9 +584,70 @@ the command — so none carries host state, but they are additions and the docum than rounding them away. The measured claim is: the shell-mediated child receives every allowlisted name with the value the builder constructed for it, and every name beyond the allowlist is one of those five. That measurement is of the control's own arm, which runs `sh -c 'exec node -e …'`. It -does not describe the GREEN route, which runs through the pnpm `tsx` launcher and will therefore -also carry the `NODE_PATH` that launcher exports. What the route's own process ends up holding is -unmeasured until there is a GREEN to measure. +does not describe the GREEN route. What the route's own process ends up holding is unmeasured until +there is a GREEN to measure. + +**The package-script contract was a substring check followed by arbitrary shell execution, and the +two halves were never read together.** The manifest case asserted the script _contained_ +`src/local-dev.ts`; the executing helper then handed the whole string to `sh -c`. So +`"curl … | sh; tsx src/local-dev.ts"` satisfied the assertion and was then run — as would anything +carrying `&`, `;`, `|`, a backtick, `$( )`, a redirect, or a second absolute command. Neither line +is wrong in isolation, which is why it survived several readings: a substring check is a reasonable +thing to write about a script you are only inspecting, and `sh -c` is a reasonable thing to write +about a script you have already validated. The defect exists only in the seam between them. The +contract is now a single module-level constant, `exec tsx src/local-dev.ts`, spelled exactly once in +the file, asserted with equality in the manifest case **and** re-asserted inside the executing +helper against the value about to run — the manifest is read twice, so the check that matters is the +one sitting on the string being executed. No metacharacter denylist backs it up: exact equality +already excludes every metacharacter anyone would enumerate, and a denylist here would repeat the +mistake the `PATH` allowlist was built to correct. + +**The hard timeout does not contain the route's process tree, and the required script is the reason +— not an attacker's.** `spawnSync`'s `timeout` with `killSignal: 'SIGKILL'` signals the process it +directly invoked and nothing below it. Two measurements bound what that is worth, and both were +produced by probing rather than by reading the documentation. + +The first is why the route must `exec`. With a backgrounded job — `node -e '…' & echo started` — +the shell returns 0 **immediately**, so the timeout never fires at all: the case observes a clean +exit, the descendant outlives the run, and sandbox teardown then races a live process still holding +paths inside the directory being removed. That is not merely a containment gap; it is a +**silent-pass** path, a control reporting success in exactly the scenario it exists to catch. +Requiring `exec ` at the head of the contract removes the intermediate shell so the signal reaches +the process that matters. + +The second is the one that stops this item from claiming the pair is sufficient. `exec` removes the +outer shell; it does not make TSX one process. The selected CLI spawns an `esbuild --service` child +and a `node --require preflight.cjs --import loader.mjs --eval` child, and the second is where the +route's own code runs. Under the fixture's exact spawn shape, four trials out of four, the parent +received `SIGKILL` and that loader child **survived it**, was reparented away from the dead parent, +and went on to complete its work afterward — writing its marker while the fixture believed the +child was gone. The esbuild child did not survive or accumulate in those runs, so the survivor is +the loader child specifically. An earlier probe of the author's appeared to show the whole tree +dying; it enumerated descendants by the killed parent's PID, and reparenting had already moved them +out of that set, so the survivors were live and the filter structurally could not see them. That +probe is recorded here rather than discarded, because a measurement that cannot observe the +violation it is testing for is the same defect class as a review bounded by paths it never listed. + +What follows is stated plainly because the temptation to round it off is exactly the failure mode +this item documents: **exact script equality plus `exec` is not process-tree cleanup.** It prohibits +_injected_ shell content, which is worth having on its own terms. But the launcher the contract +_permits_ is multi-process, so a timed-out route can still leave a live process inside a sandbox +being torn down. The fixture's timeout control asserts what it actually measures — a single-process +`exec node -e` sleeper is killed and writes nothing — and it is named for that property alone. No +assertion in this file establishes descendant containment, and the control is not offered as +evidence of it. + +One further measurement decides what any containment design may look like, so it is recorded here +rather than left for whoever writes that design to rediscover: every process in the TSX tree runs in +**the verifier's own process group and session**. Parent, esbuild child and loader child all carry +the test runner's `pgid` and `sid`, because a spawn inherits both by default. The consequence is +sharp — the obvious repair, signalling the negative process-group id, would signal the test runner +itself. Group containment is therefore not a matter of sending a different signal to the tree as it +exists; it requires the child to be given a group of its own at spawn time before any group-directed +signal is safe to send. Which design does that, and whether the resulting emptiness proof is +reuse-safe to the reviewer's standard, is under adjudication and deliberately not settled by the +author. This section will record the ruling and the residual — including the one no group-based +design can close, a descendant that calls `setsid()` for itself and leaves the group. **There is exactly one place in the file where a child process is created**, and that is the mechanism, not tidiness. A control that made its own spawn would leave the route's spawn free to be