From 1d92e21e88aa6fb824e151d7b3462cb587052755 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 12:06:38 +0000 Subject: [PATCH 1/2] test(cli): stop spawned `os serve` children inheriting the vitest worker's TEST MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Any `packages/cli` e2e test that spawns a real `os serve` and builds the child env with `{ ...process.env, … }` carries the vitest WORKER's environment into the child. Vitest sets `TEST=true` on that worker unconditionally, independent of `NODE_ENV`, and better-auth 1.7.1 reads `TEST` directly: @better-auth/core/dist/env/env-impl.mjs:36 const isTest = () => nodeENV === "test" || toBoolean(env.TEST); better-auth/dist/context/create-context.mjs:210 skipOriginCheck: … isTest() ? true : false, so the child had better-auth's own origin/CSRF validation switched OFF, one layer below anything `serve.ts` or `plugin-auth` decide. `helpers/serve-process.ts` now exports `childEnv()`, which drops `TEST` and the whole `VITEST*` namespace before the caller's overrides go on, and `runServe()` builds through it. The four `os serve` spawners in this directory that roll their own child env are swept onto it too, including #11268's pin, which had to unset `TEST` by hand. Measured, same fixture and same probe, the env family the only difference — POST /api/v1/auth/sign-in/email with `Origin: https://evil.example.com`: { ...process.env, … } 401 INVALID_EMAIL_OR_PASSWORD (origin ACCEPTED) childEnv({ … }) 403 INVALID_ORIGIN only TEST stripped 403 INVALID_ORIGIN `TEST` alone is load-bearing; the `VITEST*` entries are hygiene. Both rows are pinned as real boots in `serve-process-child-env.e2e.test.ts`, so the repair stays distinguishable from a no-op. Part of #11267 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR --- .../cli/test/artifact-pinned-boot.e2e.test.ts | 14 +- packages/cli/test/helpers/serve-process.ts | 97 ++++++- ...serve-mcp-capability-collision.e2e.test.ts | 12 +- .../test/serve-mcp-stdio-answers.e2e.test.ts | 12 +- ...ve-node-env-production-default.e2e.test.ts | 25 +- .../test/serve-process-child-env.e2e.test.ts | 249 ++++++++++++++++++ .../serve-stdio-stdout-purity.e2e.test.ts | 12 +- 7 files changed, 392 insertions(+), 29 deletions(-) create mode 100644 packages/cli/test/serve-process-child-env.e2e.test.ts diff --git a/packages/cli/test/artifact-pinned-boot.e2e.test.ts b/packages/cli/test/artifact-pinned-boot.e2e.test.ts index 82fe269fa5..561b32f9d5 100644 --- a/packages/cli/test/artifact-pinned-boot.e2e.test.ts +++ b/packages/cli/test/artifact-pinned-boot.e2e.test.ts @@ -40,6 +40,7 @@ import { tmpdir } from 'node:os'; import { join, resolve, dirname } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { PROTOCOL_MAJOR } from '@objectstack/spec/kernel'; +import { childEnv } from './helpers/serve-process.js'; const HERE = dirname(fileURLToPath(import.meta.url)); const CLI = resolve(HERE, '../bin/run-dev.js'); @@ -122,8 +123,15 @@ function runServe(env: Record, opts: { migrateAndExit?: boolean ['--import', TSX_LOADER, CLI, 'serve'], { cwd, - env: { - ...process.env, + // `childEnv`, not a bare `...process.env`: the vitest worker + // exports `TEST=true`, and better-auth 1.7.1 reads it directly + // to switch its own origin/CSRF validation OFF in the child — + // see `helpers/serve-process.ts` for the measurement (#11267). + // This boot exits at `kernel:ready` (`OS_MIGRATE_AND_EXIT`) and + // never answers a request, so nothing here CHANGES; it is the + // hygiene half, so the next assertion added to this file starts + // from a child that is not lying about being a test runner. + env: childEnv({ NODE_ENV: 'production', OS_HOME: home, OS_DATABASE_URL: `file:${join(home, 'e2e.db')}`, @@ -136,7 +144,7 @@ function runServe(env: Record, opts: { migrateAndExit?: boolean // container carrying no app does not have. OS_ARTIFACT_PATH: join(cwd, 'dist/objectstack.json'), ...env, - }, + }), stdio: ['ignore', 'pipe', 'pipe'], }, ); diff --git a/packages/cli/test/helpers/serve-process.ts b/packages/cli/test/helpers/serve-process.ts index 3c9dffa506..7b212c0160 100644 --- a/packages/cli/test/helpers/serve-process.ts +++ b/packages/cli/test/helpers/serve-process.ts @@ -25,6 +25,96 @@ export function randomPort(): string { return String(40000 + Math.floor(Math.random() * 20000)); } +/** + * The variables vitest sets on its own WORKER process, which must never reach a + * spawned `os serve` child (#11267). + * + * ## Why this exists — measured, not defensive + * + * A child built with `{ ...process.env, … }` inherits the **vitest worker's** + * environment, and vitest sets `TEST=true` on that worker unconditionally, + * independent of `NODE_ENV`. better-auth 1.7.1 reads `TEST` **directly**: + * + * ```js + * // @better-auth/core/dist/env/env-impl.mjs:36 + * const isTest = () => nodeENV === "test" || toBoolean(env.TEST); + * // better-auth/dist/context/create-context.mjs:210 + * skipOriginCheck: options.advanced?.disableOriginCheck !== void 0 + * ? options.advanced.disableOriginCheck + * : isTest() ? true : false, + * ``` + * + * So an inherited `TEST=true` disables better-auth's origin/CSRF validation + * **entirely**, one layer below anything `serve.ts` or `plugin-auth` decide, + * and independent of whatever `NODE_ENV` the caller sets on the child. The + * dangerous direction is not a red test: it is a security-posture assertion + * that can never go red for the reason it exists, which reads as coverage. + * + * MEASURED on a real boot through this helper's own spawn recipe — same + * fixture, same code, the five variables below the only difference. Probe: + * `POST /api/v1/auth/sign-in/email` with `Origin: https://evil.example.com` + * (untrusted under every branch of `serve.ts`'s trusted-origin assembly, + * including the `isDev` `http://localhost:*` convenience that `run-dev.js` + * always turns on): + * + * | child env | answer | + * |---|---| + * | `{ ...process.env }` (this helper, before #11267) | `401 INVALID_EMAIL_OR_PASSWORD` — origin ACCEPTED, validation never ran | + * | family below stripped | `403 INVALID_ORIGIN` — validation ran and rejected | + * | only `TEST` stripped | `403 INVALID_ORIGIN` | + * + * The third row is the isolation: **`TEST` alone is load-bearing.** The + * `VITEST*` entries are stripped as hygiene — nothing in `os serve` reads them + * today, and a child that believes it is a vitest worker is a lie regardless. + * + * ⛔ `NODE_ENV` is deliberately NOT in this family. The vitest worker exports + * `NODE_ENV=test` too, but every caller here already pins the child's + * `NODE_ENV` explicitly (`bin/run-dev.js` sets `development` before argv is + * even parsed; the `bin/run.js` spawners pass it in `env`), so stripping it + * would change which entrypoint those tests resolve through rather than remove + * a leak. That is a different defect with its own card (#11317) — ⛔ do not + * fold it in here. + */ +export const VITEST_WORKER_ENV_KEYS = [ + 'TEST', + 'VITEST', + 'VITEST_WORKER_ID', + 'VITEST_POOL_ID', + 'VITEST_MODE', +] as const; + +/** `TEST` exactly, or any `VITEST`-prefixed variable — see `childEnv()`. */ +function isVitestWorkerKey(key: string): boolean { + return key === 'TEST' || key === 'VITEST' || key.startsWith('VITEST_'); +} + +/** + * Build the environment for a spawned CLI child: this process's environment + * minus the vitest worker family above, plus `overrides`. + * + * The strip is a **class**, not the fixed list: `TEST` exactly, plus anything + * matching `VITEST`/`VITEST_*`. `VITEST_WORKER_ENV_KEYS` names the five that + * vitest 4 exports today (and is what the pin asserts against), but a future + * runner variable in that namespace is caught without anyone having to + * rediscover this trap first. + * + * `overrides` is applied AFTER the strip, so a test that genuinely wants one of + * these set in its child can still say so explicitly — the point is that + * nothing arrives by accident. An `undefined` value UNSETS a variable for the + * child: Node's `spawn()` omits `undefined`-valued entries rather than + * stringifying them, which `''` would not do. + */ +export function childEnv( + overrides: Record = {}, +): Record { + const env: Record = {}; + for (const [key, value] of Object.entries(process.env)) { + if (isVitestWorkerKey(key)) continue; + env[key] = value; + } + return { ...env, ...overrides }; +} + export interface ServeRun { stdout: string; stderr: string; @@ -56,8 +146,9 @@ export function runServe( return new Promise((resolveRun, rejectRun) => { const child = spawn(TSX, [CLI, 'serve', opts.config ?? 'objectstack.config.ts', ...args], { cwd, - env: { - ...process.env, + // `childEnv`, never a bare `...process.env` — see its header for the + // measured reason (#11267). + env: childEnv({ NO_COLOR: '1', // Keep the fixture self-contained: no file written, no port conflict // with another agent's dev server, no inherited log level. @@ -65,7 +156,7 @@ export function runServe( OS_LOG_LEVEL: '', OS_DISABLE_CONSOLE: '1', ...(opts.env ?? {}), - }, + }), }); let stdout = ''; diff --git a/packages/cli/test/serve-mcp-capability-collision.e2e.test.ts b/packages/cli/test/serve-mcp-capability-collision.e2e.test.ts index 017b87609f..85dc3f7921 100644 --- a/packages/cli/test/serve-mcp-capability-collision.e2e.test.ts +++ b/packages/cli/test/serve-mcp-capability-collision.e2e.test.ts @@ -48,7 +48,7 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { randomPort } from './helpers/serve-process.js'; +import { childEnv, randomPort } from './helpers/serve-process.js'; const HERE = resolve(fileURLToPath(import.meta.url), '..'); /** `bin/run.js` — the SHIPPED entrypoint, i.e. the one the card's repro names. */ @@ -109,8 +109,12 @@ function boot(env: Record, waitFor: RegExp): Promise const child = spawn(process.execPath, [CLI, 'serve', '-p', port, '--dev'], { cwd: dir, stdio: ['pipe', 'pipe', 'pipe'], - env: { - ...process.env, + // `childEnv`, not a bare `...process.env`: the vitest worker exports + // `TEST=true`, which better-auth 1.7.1 reads directly and answers by + // switching its own origin/CSRF validation OFF in the child — see + // `helpers/serve-process.ts` for the measurement (#11267). This file + // signs in for real, so it is a child that actually reaches that code. + env: childEnv({ NO_COLOR: '1', OS_LOG_LEVEL: 'info', OS_DISABLE_CONSOLE: '1', @@ -118,7 +122,7 @@ function boot(env: Record, waitFor: RegExp): Promise // vitest exports `test`. NODE_ENV: 'development', ...env, - }, + }), }) as ChildProcessWithoutNullStreams; children.push(child); diff --git a/packages/cli/test/serve-mcp-stdio-answers.e2e.test.ts b/packages/cli/test/serve-mcp-stdio-answers.e2e.test.ts index 7d3de7ff75..3a3badeaee 100644 --- a/packages/cli/test/serve-mcp-stdio-answers.e2e.test.ts +++ b/packages/cli/test/serve-mcp-stdio-answers.e2e.test.ts @@ -47,7 +47,7 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { randomPort } from './helpers/serve-process.js'; +import { childEnv, randomPort } from './helpers/serve-process.js'; const HERE = resolve(fileURLToPath(import.meta.url), '..'); /** `bin/run.js` — the SHIPPED entrypoint, i.e. the one the card's repro names. */ @@ -104,8 +104,12 @@ function boot(env: Record, waitFor: RegExp): Promise const child = spawn(process.execPath, [CLI, 'serve', '-p', port, '--dev'], { cwd: dir, stdio: ['pipe', 'pipe', 'pipe'], - env: { - ...process.env, + // `childEnv`, not a bare `...process.env`: the vitest worker exports + // `TEST=true`, which better-auth 1.7.1 reads directly and answers by + // switching its own origin/CSRF validation OFF in the child — see + // `helpers/serve-process.ts` for the measurement (#11267). This file + // signs in for real, so it is a child that actually reaches that code. + env: childEnv({ NO_COLOR: '1', OS_LOG_LEVEL: 'info', OS_DISABLE_CONSOLE: '1', @@ -114,7 +118,7 @@ function boot(env: Record, waitFor: RegExp): Promise // `test`, which would leave the DB user-less and the mint unauthorized. NODE_ENV: 'development', ...env, - }, + }), }) as ChildProcessWithoutNullStreams; children.push(child); diff --git a/packages/cli/test/serve-node-env-production-default.e2e.test.ts b/packages/cli/test/serve-node-env-production-default.e2e.test.ts index bfd3e838ae..b0011d1e5b 100644 --- a/packages/cli/test/serve-node-env-production-default.e2e.test.ts +++ b/packages/cli/test/serve-node-env-production-default.e2e.test.ts @@ -130,6 +130,7 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { join, resolve } from 'node:path'; import type { Readable } from 'node:stream'; import { fileURLToPath } from 'node:url'; +import { childEnv } from './helpers/serve-process.js'; /** What `spawn(..., { stdio: ['ignore', 'pipe', 'pipe'] })` actually returns — no `stdin`. */ type ProbeChild = ChildProcessByStdio; @@ -210,8 +211,7 @@ async function probeOriginCheck(env: Record): Promis const child = spawn(process.execPath, [CLI, 'serve', '-p', String(port)], { cwd: dir, stdio: ['ignore', 'pipe', 'pipe'], - env: { - ...process.env, + env: childEnv({ NO_COLOR: '1', OS_LOG_LEVEL: 'warn', OS_DISABLE_CONSOLE: '1', @@ -226,22 +226,25 @@ async function probeOriginCheck(env: Record): Promis // than inheriting whatever this test RUNNER's own process (vitest sets // NODE_ENV=test) happened to have. NODE_ENV: undefined, - // MEASURED TRAP, worth stating explicitly: `...process.env` above is - // THIS FILE's own process env — the vitest WORKER's — and vitest's - // worker carries `TEST=true` (and `VITEST=true`) regardless of - // `NODE_ENV`. better-auth 1.7.1 reads `TEST` directly, independent of - // `NODE_ENV`: `create-context.mjs` defaults + // MEASURED TRAP, and the reason the base above is `childEnv()` rather + // than `...process.env`: this file's own process env is the vitest + // WORKER's, and that worker carries `TEST=true` (and `VITEST=true`) + // regardless of `NODE_ENV`. better-auth 1.7.1 reads `TEST` directly, + // independent of `NODE_ENV`: `create-context.mjs` defaults // `skipOriginCheck: … isTest() ? true : false`, and // `isTest = () => nodeENV === 'test' || toBoolean(env.TEST)`. Left // alone, that inherited `TEST=true` makes better-auth skip origin // validation ENTIRELY — a false GREEN that has nothing to do with // `serve.ts`'s own gate and stays green with the fix reverted, which is // exactly the vacuity this card's anti-vacuity section warns against, - // one layer further down than the one it names. Unset it the same way - // `NODE_ENV` is unset above, for the same reason. - TEST: undefined, + // one layer further down than the one it names. This file used to unset + // `TEST` by hand right here; #11267 moved that into `childEnv()` so + // every spawner in this directory gets it without having to know, and + // widened it to the whole `VITEST*` family. The behaviour of this + // fixture is unchanged — `childEnv()` removes a superset of what the + // hand-written `TEST: undefined` removed. ...env, - }, + }), }) as ProbeChild; children.push(child); diff --git a/packages/cli/test/serve-process-child-env.e2e.test.ts b/packages/cli/test/serve-process-child-env.e2e.test.ts new file mode 100644 index 0000000000..ca402486e5 --- /dev/null +++ b/packages/cli/test/serve-process-child-env.e2e.test.ts @@ -0,0 +1,249 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #11267 — a spawned `os serve` child must not inherit the vitest worker's + * `TEST=true`, because better-auth switches its own origin/CSRF validation OFF + * when it sees it. + * + * ## What this file pins, and why it takes a real boot to pin it + * + * The fix itself is four lines in `helpers/serve-process.ts` (`childEnv()` + * drops `TEST` and the `VITEST*` family before the overrides go on). A pin that + * only asserted "the returned object has no `TEST` key" would be true and + * almost worthless: it says nothing about whether that key ever mattered, and + * this card exists precisely because that distinction was invisible. So the + * structural assertions below are followed by two REAL boots that differ in + * nothing but those variables. + * + * ## The measurement, which is what the two boots re-run on every CI pass + * + * Probe: `POST /api/v1/auth/sign-in/email` carrying + * `Origin: https://evil.example.com`, no cookie, no `Sec-Fetch-*`. That is the + * shape `validateFormCsrf` forces an origin check for (`origin-check.mjs`: no + * cookie and no Fetch-Metadata, but an `origin` header present ⇒ + * `validateOrigin(ctx, true)`). The origin is not localhost, so it is untrusted + * under EVERY branch of `serve.ts`'s trusted-origin assembly — including the + * `isDev` `http://localhost:*` convenience, which `bin/run-dev.js` always turns + * on by setting `NODE_ENV=development` before argv is parsed. A pass therefore + * cannot be explained by the deployment trusting its own origin. + * + * | child env | answer | + * |---|---| + * | `{ ...process.env, … }` — this directory's shape before #11267 | `401 INVALID_EMAIL_OR_PASSWORD` — origin ACCEPTED, validation never ran | + * | `childEnv({ … })` | `403 INVALID_ORIGIN` — validation ran and rejected | + * + * Isolated when this was measured: stripping ONLY `TEST` (leaving `VITEST`, + * `VITEST_WORKER_ID`, `VITEST_POOL_ID`, `VITEST_MODE` in place) also answers + * `403 INVALID_ORIGIN`. `TEST` alone is load-bearing; the `VITEST*` entries are + * stripped as hygiene, not because anything reads them today. + * + * ## ⚠️ The first boot deliberately builds the env the WRONG way + * + * `leakedEnv()` below is a bare `...process.env` spread on purpose — it is the + * pre-#11267 recipe, kept executable so the repair stays distinguishable from a + * no-op. ⛔ Do not "clean it up" to `childEnv()`: that would delete the only + * evidence in the repo that the leak does anything, and leave a green suite + * behind. It is also the canary on the dependency — if better-auth stops + * reading `TEST`, that leg goes red, and the answer is to re-read this header + * and re-measure, not to silence it. + * + * Cost: two real `os serve` boots, ~18s together when measured on this + * container. Both children are killed in `afterAll` regardless of outcome. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { spawn, type ChildProcessByStdio } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import type { Readable } from 'node:stream'; +import { CLI, TSX, childEnv, randomPort, VITEST_WORKER_ENV_KEYS } from './helpers/serve-process.js'; + +/** What `spawn(…, { stdio: ['ignore', 'pipe', 'pipe'] })` actually returns — no `stdin`. */ +type ProbeChild = ChildProcessByStdio; + +/** + * No `plugins` key: this is the shape that leaves `serve.ts` to auto-inject + * `AuthPlugin` (`!hasAuthPlugin && tierEnabled('auth')`), which is what mounts + * better-auth at `/api/v1/auth/*`. The object exists only so the stack is a + * valid app; nothing below reads it. + */ +const CONFIG = ` +export default { + manifest: { + id: 'com.example.childenv', + namespace: 'childenv', + version: '1.0.0', + type: 'app', + name: 'childEnv origin-validation probe', + }, + objects: [{ + name: 'childenv_task', + label: 'Task', + sharingModel: 'public', + fields: { title: { type: 'text', label: 'Title' } }, + }], +}; +`; + +/** The overrides both legs share, so the ONLY difference between them is the base. */ +const OVERRIDES = { + NO_COLOR: '1', + OS_DATABASE_URL: ':memory:', + OS_LOG_LEVEL: 'warn', + OS_DISABLE_CONSOLE: '1', + // Explicit rather than leaning on serve.ts's isDev fallback secret, so a + // change to that fallback can never turn this into a boot-failure test. + OS_AUTH_SECRET: 'e2e-child-env-probe-secret-not-for-real-use', +}; + +/** + * ⚠️ The PRE-#11267 recipe, on purpose. See this file's header before touching + * it — it is the leg that proves the leak does something. + */ +function leakedEnv(): Record { + return { ...process.env, ...OVERRIDES }; +} + +let dir: string; +const children: ProbeChild[] = []; + +async function stop(child: ProbeChild): Promise { + if (child.exitCode !== null || child.signalCode !== null) return; + await new Promise((done) => { + const give = setTimeout(() => { + try { child.kill('SIGKILL'); } catch { /* already gone */ } + done(); + }, 10_000); + child.once('exit', () => { clearTimeout(give); done(); }); + try { + child.kill('SIGTERM'); + } catch { + clearTimeout(give); + done(); + } + }); +} + +/** + * Boot the real `os serve` through the same entrypoint `runServe()` uses, probe + * the origin check while it is UP, then stop it. `runServe()` itself cannot + * stand in here: it kills the child the moment `waitFor` matches, so there is + * no window in which to send a request. + */ +async function probeOrigin(env: Record): Promise<{ status: number; code: unknown }> { + const port = randomPort(); + const child = spawn(TSX, [CLI, 'serve', 'objectstack.config.ts', '--port', port], { + cwd: dir, + stdio: ['ignore', 'pipe', 'pipe'], + env, + }) as ProbeChild; + children.push(child); + + let out = ''; + let err = ''; + await new Promise((ready, fail) => { + const timer = setTimeout( + () => fail(new Error(`serve never became ready\n--- stdout ---\n${out}\n--- stderr ---\n${err}`)), + 150_000, + ); + const onData = () => { + if (/Press Ctrl\+C to stop|Server is ready/.test(out + err)) { + clearTimeout(timer); + ready(); + } + }; + child.stdout.on('data', (d) => { out += String(d); onData(); }); + child.stderr.on('data', (d) => { err += String(d); onData(); }); + child.on('exit', (code) => { + clearTimeout(timer); + fail(new Error(`serve exited ${code} before it was ready\n--- stdout ---\n${out}\n--- stderr ---\n${err}`)); + }); + }); + + try { + const res = await fetch(`http://localhost:${port}/api/v1/auth/sign-in/email`, { + method: 'POST', + headers: { + 'content-type': 'application/json', + // Not localhost, so untrusted under every branch of serve.ts's + // trusted-origin assembly. No cookie and no Sec-Fetch-* header: that is + // the shape validateFormCsrf forces an origin check for. + origin: 'https://evil.example.com', + }, + body: JSON.stringify({ email: 'nobody@example.com', password: 'definitely-wrong-password' }), + }); + let body: any = null; + try { body = await res.json(); } catch { /* non-JSON body, fall through */ } + return { status: res.status, code: body?.code }; + } finally { + await stop(child); + } +} + +describe('#11267: childEnv() keeps the vitest worker out of a spawned os serve', () => { + it('drops every variable in the vitest worker family', () => { + const env = childEnv(); + for (const key of VITEST_WORKER_ENV_KEYS) { + expect(Object.hasOwn(env, key), `childEnv() still carries ${key}`).toBe(false); + } + }); + + it('drops the whole VITEST namespace, not just the five names known today', () => { + const env = childEnv(); + const leaked = Object.keys(env).filter((k) => k === 'TEST' || k.startsWith('VITEST')); + expect(leaked).toEqual([]); + }); + + it('still carries the rest of the environment, and lets an override win', () => { + const env = childEnv({ TEST: 'deliberate', OS_LOG_LEVEL: 'warn' }); + // PATH is the one variable a spawned child cannot do without. + expect(env.PATH).toBe(process.env.PATH); + // Overrides are applied AFTER the strip: a test that genuinely wants one of + // these can still say so — the point is that nothing arrives by accident. + expect(env.TEST).toBe('deliberate'); + expect(env.OS_LOG_LEVEL).toBe('warn'); + }); + + it('an `undefined` override survives as an own key, so spawn() unsets it', () => { + const env = childEnv({ NODE_ENV: undefined }); + expect(Object.hasOwn(env, 'NODE_ENV')).toBe(true); + expect(env.NODE_ENV).toBeUndefined(); + }); + + describe('and that is not cosmetic — the same boot, the same probe, two answers', () => { + beforeAll(() => { + dir = mkdtempSync(join(tmpdir(), 'os-child-env-probe-')); + writeFileSync(join(dir, 'objectstack.config.ts'), CONFIG, 'utf8'); + }); + + afterAll(async () => { + for (const child of children) await stop(child); + if (dir) rmSync(dir, { recursive: true, force: true }); + }, 60_000); + + it( + 'inheriting the worker env (the pre-#11267 shape): the untrusted origin is ACCEPTED', + async () => { + // ⛔ Do NOT convert this leg to childEnv() — see the file header. + const { status, code } = await probeOrigin(leakedEnv()); + expect(status).not.toBe(403); + expect(code).not.toBe('INVALID_ORIGIN'); + // Positive control: the request really did reach the sign-in handler, + // rather than failing for some unrelated reason that also is not a 403. + expect(code).toBe('INVALID_EMAIL_OR_PASSWORD'); + }, + 180_000, + ); + + it( + 'through childEnv(): the untrusted origin is REJECTED', + async () => { + const { status, code } = await probeOrigin(childEnv(OVERRIDES)); + expect(status).toBe(403); + expect(code).toBe('INVALID_ORIGIN'); + }, + 180_000, + ); + }); +}); diff --git a/packages/cli/test/serve-stdio-stdout-purity.e2e.test.ts b/packages/cli/test/serve-stdio-stdout-purity.e2e.test.ts index 183935def1..97ae863ba7 100644 --- a/packages/cli/test/serve-stdio-stdout-purity.e2e.test.ts +++ b/packages/cli/test/serve-stdio-stdout-purity.e2e.test.ts @@ -44,7 +44,7 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { randomPort } from './helpers/serve-process.js'; +import { childEnv, randomPort } from './helpers/serve-process.js'; const HERE = resolve(fileURLToPath(import.meta.url), '..'); /** `bin/run.js` — the SHIPPED entrypoint, i.e. the one the card's repro names. */ @@ -96,8 +96,12 @@ function boot(env: Record, waitFor: RegExp): Promise const child = spawn(process.execPath, [CLI, 'serve', '-p', port, '--dev'], { cwd: dir, stdio: ['pipe', 'pipe', 'pipe'], - env: { - ...process.env, + // `childEnv`, not a bare `...process.env`: the vitest worker exports + // `TEST=true`, which better-auth 1.7.1 reads directly and answers by + // switching its own origin/CSRF validation OFF in the child — see + // `helpers/serve-process.ts` for the measurement (#11267). This file + // signs in for real, so it is a child that actually reaches that code. + env: childEnv({ NO_COLOR: '1', // The card's own capture level. `warn` (the default) still puts the // banner and the WARN records on the channel, so `info` is the same @@ -108,7 +112,7 @@ function boot(env: Record, waitFor: RegExp): Promise // hard-gated on `NODE_ENV === 'development'`, and vitest exports `test`. NODE_ENV: 'development', ...env, - }, + }), }) as ChildProcessWithoutNullStreams; children.push(child); From d0a619c458a2af44f00bf740b4b564bd6f8f5fe9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 13:19:48 +0000 Subject: [PATCH 2/2] test(cli): give spawned children an explicit crypto key, now that VITEST no longer leaks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught a claim in the previous commit that was simply false. Its header said the `VITEST*` entries were stripped "as hygiene — nothing in `os serve` reads them today". Something does: packages/services/service-settings/src/local-crypto-provider.ts:133 const detectMode = (env: EnvMap): CryptoMode => { if (env.VITEST || env.NODE_ENV === 'test') return 'test'; if (env.NODE_ENV === 'production') return 'production'; return 'development'; }; While a spawned child still inherited `VITEST=true`, its crypto layer sat in `test` mode — ephemeral key, never touches disk, never refuses — whatever posture the rest of the boot was in. So the `serve-node-env-production-default` pin, whose entire subject is that an unset `NODE_ENV` means PRODUCTION, was production for auth and test for crypto. Stopping the leak made it production for both, and it refused to boot without a stable key. That red is the gate working. Same defect class as the `TEST` leak one gate over: a security-relevant gate softened by a variable inherited from the test runner rather than by anything the code under test decided. So the fix is to supply what production posture demands, exactly as that fixture already supplies `OS_AUTH_SECRET` for the sibling gate — never to put `VITEST` back. Measured, single variable, same tree and same build, `HOME` pointed at an empty directory to match a clean runner: previous commit 1 failed | 2 passed with only that file's edit reverted 3 passed and the failure was ORDER-DEPENDENT, which is worse than red: with `$HOME/.objectstack/dev-crypto-key` absent the production boot refuses, and with it present — written by any earlier dev-mode boot in the same run, since development mode persists a minted key — the same boot succeeds. Under vitest's parallel workers that ordering is not deterministic. `E2E_SECRET_KEY` is now a `runServe()` default and is set explicitly by the spawners that roll their own env, which removes both halves: no child writes a key file, and no child depends on one. Verified with a pristine `HOME`: 11 files, 36 tests, green, and no key file created. Part of #11267 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR --- packages/cli/test/helpers/serve-process.ts | 54 +++++++++++++++++-- ...serve-mcp-capability-collision.e2e.test.ts | 9 +++- .../test/serve-mcp-stdio-answers.e2e.test.ts | 9 +++- ...ve-node-env-production-default.e2e.test.ts | 19 ++++++- .../test/serve-process-child-env.e2e.test.ts | 15 ++++-- .../serve-stdio-stdout-purity.e2e.test.ts | 9 +++- 6 files changed, 105 insertions(+), 10 deletions(-) diff --git a/packages/cli/test/helpers/serve-process.ts b/packages/cli/test/helpers/serve-process.ts index 7b212c0160..b23b38d5ea 100644 --- a/packages/cli/test/helpers/serve-process.ts +++ b/packages/cli/test/helpers/serve-process.ts @@ -63,9 +63,44 @@ export function randomPort(): string { * | family below stripped | `403 INVALID_ORIGIN` — validation ran and rejected | * | only `TEST` stripped | `403 INVALID_ORIGIN` | * - * The third row is the isolation: **`TEST` alone is load-bearing.** The - * `VITEST*` entries are stripped as hygiene — nothing in `os serve` reads them - * today, and a child that believes it is a vitest worker is a lie regardless. + * The third row is the isolation for THAT probe: `TEST` alone is what + * better-auth reads. + * + * ## ⚠️ `VITEST` is NOT cosmetic either — a claim this file got wrong once + * + * The first revision of this header said the `VITEST*` entries were stripped + * as hygiene, "nothing in `os serve` reads them today". That was **false**, and + * CI found the counterexample: + * + * ```ts + * // packages/services/service-settings/src/local-crypto-provider.ts:133 + * const detectMode = (env: EnvMap): CryptoMode => { + * if (env.VITEST || env.NODE_ENV === 'test') return 'test'; + * if (env.NODE_ENV === 'production') return 'production'; + * return 'development'; + * }; + * ``` + * + * So an inherited `VITEST=true` put every spawned child's crypto layer in + * `test` mode — ephemeral key, never touches disk, never refuses — no matter + * what posture the rest of the boot was in. That is the SAME defect class as + * the `TEST` leak one layer over: a security-relevant gate (here, stable + * encryption-key enforcement) softened by a variable the child inherited from + * the test runner rather than by anything the code under test decided. + * Stripping `VITEST` is therefore load-bearing in its own right, and the + * `serve-node-env-production-default` pin going red the moment it stopped + * leaking is the gate working, not the gate misfiring: that fixture's + * "production posture" had been genuine for auth and fake for crypto. + * + * The consequence is why `OS_SECRET_KEY` is a default below. Once the child + * stops claiming to be a vitest worker, `detectMode` answers `development` + * for the ordinary boots here, and development mode **persists** a minted key + * to `$HOME/.objectstack/dev-crypto-key`. Measured: with that file absent a + * production-posture boot refuses to start, and with it present — put there by + * any earlier dev-mode boot in the same run — the same boot succeeds. That is + * a cross-test ordering coupling through the runner's home directory, and + * under vitest's parallel workers it is nondeterministic. An explicit key + * removes both halves: nothing is written, and nothing is depended on. * * ⛔ `NODE_ENV` is deliberately NOT in this family. The vitest worker exports * `NODE_ENV=test` too, but every caller here already pins the child's @@ -75,6 +110,14 @@ export function randomPort(): string { * a leak. That is a different defect with its own card (#11317) — ⛔ do not * fold it in here. */ +/** + * A fixed, obviously-synthetic 32-byte key (64 hex chars) for spawned children, + * so no test boot has to mint one — see `runServe()` and the header above. + * ⛔ Test fixtures only; it is in the repo in plaintext and encrypts nothing + * anyone keeps. + */ +export const E2E_SECRET_KEY = '0e2e'.repeat(16); + export const VITEST_WORKER_ENV_KEYS = [ 'TEST', 'VITEST', @@ -155,6 +198,11 @@ export function runServe( OS_DATABASE_URL: ':memory:', OS_LOG_LEVEL: '', OS_DISABLE_CONSOLE: '1', + // Same "no file written" rule, extended to the crypto key — see the + // header. Without this the child mints one and PERSISTS it to + // `$HOME/.objectstack/dev-crypto-key`, which both litters the runner's + // home directory and couples unrelated tests to each other through it. + OS_SECRET_KEY: E2E_SECRET_KEY, ...(opts.env ?? {}), }), }); diff --git a/packages/cli/test/serve-mcp-capability-collision.e2e.test.ts b/packages/cli/test/serve-mcp-capability-collision.e2e.test.ts index 85dc3f7921..311d6f4b39 100644 --- a/packages/cli/test/serve-mcp-capability-collision.e2e.test.ts +++ b/packages/cli/test/serve-mcp-capability-collision.e2e.test.ts @@ -48,7 +48,7 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { childEnv, randomPort } from './helpers/serve-process.js'; +import { E2E_SECRET_KEY, childEnv, randomPort } from './helpers/serve-process.js'; const HERE = resolve(fileURLToPath(import.meta.url), '..'); /** `bin/run.js` — the SHIPPED entrypoint, i.e. the one the card's repro names. */ @@ -118,6 +118,13 @@ function boot(env: Record, waitFor: RegExp): Promise NO_COLOR: '1', OS_LOG_LEVEL: 'info', OS_DISABLE_CONSOLE: '1', + // Explicit, not minted: with `VITEST` no longer inherited (#11267), + // `local-crypto-provider.ts`'s detectMode answers `development` for + // this child instead of `test`, and development mode PERSISTS a minted + // key to `$HOME/.objectstack/dev-crypto-key`. Supplying one keeps this + // boot from writing to the runner's home directory and from coupling + // itself to whatever other test got there first. + OS_SECRET_KEY: E2E_SECRET_KEY, // The dev-admin seed the key mint signs in as is gated on this, and // vitest exports `test`. NODE_ENV: 'development', diff --git a/packages/cli/test/serve-mcp-stdio-answers.e2e.test.ts b/packages/cli/test/serve-mcp-stdio-answers.e2e.test.ts index 3a3badeaee..eb3bd9f5bf 100644 --- a/packages/cli/test/serve-mcp-stdio-answers.e2e.test.ts +++ b/packages/cli/test/serve-mcp-stdio-answers.e2e.test.ts @@ -47,7 +47,7 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { childEnv, randomPort } from './helpers/serve-process.js'; +import { E2E_SECRET_KEY, childEnv, randomPort } from './helpers/serve-process.js'; const HERE = resolve(fileURLToPath(import.meta.url), '..'); /** `bin/run.js` — the SHIPPED entrypoint, i.e. the one the card's repro names. */ @@ -113,6 +113,13 @@ function boot(env: Record, waitFor: RegExp): Promise NO_COLOR: '1', OS_LOG_LEVEL: 'info', OS_DISABLE_CONSOLE: '1', + // Explicit, not minted: with `VITEST` no longer inherited (#11267), + // `local-crypto-provider.ts`'s detectMode answers `development` for + // this child instead of `test`, and development mode PERSISTS a minted + // key to `$HOME/.objectstack/dev-crypto-key`. Supplying one keeps this + // boot from writing to the runner's home directory and from coupling + // itself to whatever other test got there first. + OS_SECRET_KEY: E2E_SECRET_KEY, // Explicit, not inherited: the dev-admin seed this fixture signs in as // is hard-gated on `NODE_ENV === 'development'`, and vitest exports // `test`, which would leave the DB user-less and the mint unauthorized. diff --git a/packages/cli/test/serve-node-env-production-default.e2e.test.ts b/packages/cli/test/serve-node-env-production-default.e2e.test.ts index b0011d1e5b..87a8e819f3 100644 --- a/packages/cli/test/serve-node-env-production-default.e2e.test.ts +++ b/packages/cli/test/serve-node-env-production-default.e2e.test.ts @@ -130,7 +130,7 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { join, resolve } from 'node:path'; import type { Readable } from 'node:stream'; import { fileURLToPath } from 'node:url'; -import { childEnv } from './helpers/serve-process.js'; +import { childEnv, E2E_SECRET_KEY } from './helpers/serve-process.js'; /** What `spawn(..., { stdio: ['ignore', 'pipe', 'pipe'] })` actually returns — no `stdin`. */ type ProbeChild = ChildProcessByStdio; @@ -221,6 +221,23 @@ async function probeOriginCheck(env: Record): Promis // "AuthPlugin.init() throws: secret is required" path regardless of // which NODE_ENV state this call is probing. OS_AUTH_SECRET: 'e2e-node-env-default-probe-secret-not-for-real-use', + // EXACTLY the argument the line above makes, for the sibling gate that + // #11267 exposed. The unset-`NODE_ENV` leg is — by this file's whole + // design — a PRODUCTION boot, and `LocalCryptoProvider` refuses to start + // in production without a stable key rather than mint one that would + // make every `sys_secret` value undecryptable after a restart. That + // refusal is a boot failure, not a signal about the origin gate this + // file measures, so the key is supplied explicitly. + // + // ⚠️ It was NOT needed before #11267 — and that is the finding, not an + // inconvenience. `local-crypto-provider.ts:133` reads + // `if (env.VITEST || env.NODE_ENV === 'test') return 'test'`, so while + // this fixture still inherited the vitest worker's `VITEST=true`, its + // crypto layer sat in TEST mode (ephemeral key, no disk, no refusal) + // while the rest of the boot was in production posture. The production + // posture this file exists to pin was genuine for auth and fake for + // crypto. Supplying the key is what makes it genuine for both. + OS_SECRET_KEY: E2E_SECRET_KEY, // The base default for every call: truly unset, unless overridden by // `env` below. Node's spawn omits an `undefined`-valued entry rather // than inheriting whatever this test RUNNER's own process (vitest sets diff --git a/packages/cli/test/serve-process-child-env.e2e.test.ts b/packages/cli/test/serve-process-child-env.e2e.test.ts index ca402486e5..4b3b63ec29 100644 --- a/packages/cli/test/serve-process-child-env.e2e.test.ts +++ b/packages/cli/test/serve-process-child-env.e2e.test.ts @@ -34,8 +34,13 @@ * * Isolated when this was measured: stripping ONLY `TEST` (leaving `VITEST`, * `VITEST_WORKER_ID`, `VITEST_POOL_ID`, `VITEST_MODE` in place) also answers - * `403 INVALID_ORIGIN`. `TEST` alone is load-bearing; the `VITEST*` entries are - * stripped as hygiene, not because anything reads them today. + * `403 INVALID_ORIGIN`, so `TEST` alone is what better-auth reads. + * + * ⚠️ `VITEST` is not merely hygiene either, though this file said so in its + * first revision and was wrong: `local-crypto-provider.ts:133` reads it + * (`if (env.VITEST || env.NODE_ENV === 'test') return 'test'`) and an + * inherited one silently put a spawned child's crypto layer in test mode. Same + * class, different gate. `helpers/serve-process.ts` carries the measurement. * * ## ⚠️ The first boot deliberately builds the env the WRONG way * @@ -57,7 +62,7 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { Readable } from 'node:stream'; -import { CLI, TSX, childEnv, randomPort, VITEST_WORKER_ENV_KEYS } from './helpers/serve-process.js'; +import { CLI, TSX, E2E_SECRET_KEY, childEnv, randomPort, VITEST_WORKER_ENV_KEYS } from './helpers/serve-process.js'; /** What `spawn(…, { stdio: ['ignore', 'pipe', 'pipe'] })` actually returns — no `stdin`. */ type ProbeChild = ChildProcessByStdio; @@ -95,6 +100,10 @@ const OVERRIDES = { // Explicit rather than leaning on serve.ts's isDev fallback secret, so a // change to that fallback can never turn this into a boot-failure test. OS_AUTH_SECRET: 'e2e-child-env-probe-secret-not-for-real-use', + // Explicit for the same reason, and it stays in the SHARED overrides so the + // two legs still differ in nothing but the vitest env family — which is the + // property the whole comparison rests on. + OS_SECRET_KEY: E2E_SECRET_KEY, }; /** diff --git a/packages/cli/test/serve-stdio-stdout-purity.e2e.test.ts b/packages/cli/test/serve-stdio-stdout-purity.e2e.test.ts index 97ae863ba7..b557e0a81b 100644 --- a/packages/cli/test/serve-stdio-stdout-purity.e2e.test.ts +++ b/packages/cli/test/serve-stdio-stdout-purity.e2e.test.ts @@ -44,7 +44,7 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { childEnv, randomPort } from './helpers/serve-process.js'; +import { E2E_SECRET_KEY, childEnv, randomPort } from './helpers/serve-process.js'; const HERE = resolve(fileURLToPath(import.meta.url), '..'); /** `bin/run.js` — the SHIPPED entrypoint, i.e. the one the card's repro names. */ @@ -108,6 +108,13 @@ function boot(env: Record, waitFor: RegExp): Promise // defect with more of it — the strictest setting this pin can run at. OS_LOG_LEVEL: 'info', OS_DISABLE_CONSOLE: '1', + // Explicit, not minted: with `VITEST` no longer inherited (#11267), + // `local-crypto-provider.ts`'s detectMode answers `development` for + // this child instead of `test`, and development mode PERSISTS a minted + // key to `$HOME/.objectstack/dev-crypto-key`. Supplying one keeps this + // boot from writing to the runner's home directory and from coupling + // itself to whatever other test got there first. + OS_SECRET_KEY: E2E_SECRET_KEY, // Explicit, not inherited: the dev-admin seed the mint signs in as is // hard-gated on `NODE_ENV === 'development'`, and vitest exports `test`. NODE_ENV: 'development',