Uh oh!
There was an error while loading. Please reload this page.
test(cli): stop spawned os serve children inheriting the vitest worker's TEST - #11340
Conversation
…ker's TEST
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR📓 Docs Drift CheckThis PR changes 1 package(s): 4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 8d1d74c476da30dedce01cce13a4fcacfc5bf755 && git checkout 8d1d74c476da30dedce01cce13a4fcacfc5bf755
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin d39569239b46f6ef68e8af438a6e78ccb62d6c92 d0a619c458a2af44f00bf740b4b564bd6f8f5fe9 && git checkout -B drift-repro d39569239b46f6ef68e8af438a6e78ccb62d6c92 && git merge --no-ff d0a619c458a2af44f00bf740b4b564bd6f8f5fe9
node scripts/docs-audit/affected-docs.mjs --json d39569239b46f6ef68e8af438a6e78ccb62d6c92
|
…EST no longer leaks
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqRUh oh!
There was an error while loading. Please reload this page.
Fixes#11267
What leaked, and what it switched off
Any
packages/clie2e test that spawns a realos serveand builds the child'senvironment with
{ ...process.env, … }carries the vitest worker's ownenvironment into the child. Vitest sets
TEST=trueon that workerunconditionally, independent of
NODE_ENV— measured here, in this repo'srunner:
TEST="true" VITEST="true" VITEST_WORKER_ID="0" VITEST_POOL_ID="1" VITEST_MODE="RUN" NODE_ENV="test".better-auth 1.7.1 reads
TESTdirectly, at the installed version:So the spawned child had better-auth's own origin/CSRF validation switched off
entirely — one layer below anything
serve.tsorplugin-authdecide, andindependent of whatever
NODE_ENVthe test set on the child.Proving it mattered — the part that is actually the work
Unsetting a variable is trivial; a fix nobody can distinguish from a no-op is
worthless. So the claim is measured on a real boot, same fixture, same code,
the env family the only difference. Probe:
POST /api/v1/auth/sign-in/emailcarrying
Origin: https://evil.example.com, no cookie, noSec-Fetch-*— theshape
validateFormCsrfforces an origin check for. The origin is notlocalhost, so it is untrusted under every branch of
serve.ts'strusted-origin assembly, including the
isDevhttp://localhost:*conveniencethat
bin/run-dev.jsalways turns on.{ ...process.env, … }— the shape in this directory today401 INVALID_EMAIL_OR_PASSWORD— origin ACCEPTED, validation never ran403 INVALID_ORIGIN— validation ran and rejectedTESTstripped,VITEST*left in place403 INVALID_ORIGINThe third row is the isolation for that probe:
TESTalone is whatbetter-auth reads.
VITESTis not hygiene either, and CI proved itThe first revision of this PR claimed the
VITEST*entries were stripped ashygiene because "nothing in
os servereads them". That was false. CI foundthe counterexample, and it is recorded here rather than quietly patched over:
While a spawned child still inherited
VITEST=true, its crypto layer sat intestmode — ephemeral key, never touches disk, never refuses — whateverposture the rest of the boot was in. So
serve-node-env-production-default,whose entire subject is that an unset
NODE_ENVmeans production, wasproduction for auth and test for crypto. The moment it stopped leaking, it
refused to boot without a stable key. That red is the gate working, and it
is the same defect class as the
TESTleak one gate over: a security-relevantgate — here, stable encryption-key enforcement — softened by a variable
inherited from the test runner rather than by anything the code under test
decided.
The fix is to supply what production posture demands, exactly as that fixture
already supplies
OS_AUTH_SECRETfor the sibling gate — never to putVITESTback.
E2E_SECRET_KEYis now arunServe()default and is set explicitly bythe spawners that roll their own env.
The failure was also order-dependent, which is worse than red. Development
mode persists a minted key to
$HOME/.objectstack/dev-crypto-key. Measured:with that file absent the production boot refuses; with it present — written by
any earlier dev-mode boot in the same run — the same boot succeeds. Under
vitest's parallel workers that ordering is not deterministic. The explicit key
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, no key filecreated.
Both of the first two rows are now committed as real boots in
serve-process-child-env.e2e.test.ts, so the repair stays distinguishable froma no-op on every CI pass rather than only in this description. The leg that
builds the env the old way is deliberately a bare
...process.envspread and ismarked ⛔ do-not-clean-up in the file header; it doubles as the canary on the
dependency.
The fix
helpers/serve-process.tsexportschildEnv(overrides): this process'senvironment minus the vitest worker family, plus the overrides. The strip is a
class, not a list —
TESTexactly, plus anything matchingVITEST/VITEST_*— so a runner variable added to that namespace tomorrow is caught without anyone
rediscovering the trap first.
VITEST_WORKER_ENV_KEYSnames the five vitest 4exports today and is what the pin asserts against. Overrides are applied after
the strip, so a test that genuinely wants one of these set can still say so.
runServe()builds through it, and so do the four spawners in this directorythat roll their own child env — including #11268's pin, which had to unset
TESTby hand right at the spawn site. That hand-written line is gone; thecomment explaining why stayed.
⛔
NODE_ENVis deliberately not in the family. The worker exportsNODE_ENV=testtoo, but every spawner here already pins the child'sNODE_ENVexplicitly, so stripping it would change which entrypoint those tests resolve
through rather than remove a leak. That is #11317, and it is not touched here.
Per-file: which spawners were exposed, and which were not
runServe()covers 8 importers; 3 moreos servespawners in the samedirectory rolled their own env. All 11 are listed — an honest "unaffected" is
worth more than an inflated count.
helpers/serve-process.ts...process.envchildEnv()serve-process-child-env.e2e.test.tsserve-mcp-capability-collision.e2e.test.tschildEnv(); still greenserve-mcp-stdio-answers.e2e.test.tschildEnv(); still greenserve-stdio-stdout-purity.e2e.test.tschildEnv(); still greenserve-node-env-production-default.e2e.test.tsTESTby handchildEnv(); hand-written line removedserve-app-runtime-hooks.e2e.test.tsonEnableranserve-boot-diagnostics.e2e.test.tsserve-no-artifact.e2e.test.tsserve-organizations-host-resolution.e2e.test.tsserve-organizations-mount-failure.e2e.test.tsartifact-pinned-boot.e2e.test.tsOS_MIGRATE_AND_EXIT, never serveschildEnv(); hygiene onlyThe three sign-in spawners are the interesting row, and they stay green —
not by luck, and worth stating because the naive reading says enabling origin
validation should have broken them.
origin-check.mjs'svalidateFormCsrfreturns without validating anything when the request has no cookie, no
Sec-Fetch-*, and noorigin/refererheader — which is exactly the shape ofa bare Node
fetch(). So those three exercise better-auth's sign-in handler forreal while never reaching the origin branch. They were exposed to the leak and
are fixed by this PR; they simply never made an assertion the leak could
falsify.
Reverse-verified
The strip line in
childEnv()was ablated (if (isVitestWorkerKey(key)) continue;replaced by a marker; both directions confirmed on disk bygrep -c, restored under anEXITtrap). Predicted direction before running:3 red / 3 green. Observed exactly that — the two structural assertions, and the
behaviour leg flipping
403 → 401, i.e. the untrusted origin accepted again:No rebuild step is involved in that ablation and none is owed: the helper is a
relative intra-package import (
./helpers/serve-process.js), resolved fromsource by vitest, never through a package
exportsfield intodist/. Restorewas confirmed on disk and re-run green (6/6).
Verification
Run at
d0a619c4, the final commit.pnpm --filter @objectstack/cli typecheck— OKHOMEpointed at an empty directory so a clean runner's missing~/.objectstack/dev-crypto-keyis reproduced rather than papered over by this container's ownpnpm lint(repo-wideeslint . --no-inline-config) — exit 0, cleannode scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack; all green, includingcheck:type-check-debt --re-measureagainst a fully built closure: "33 ledger entries re-measured in 240.2s, 1897 raw tsc errors total, none above its recorded number."No changeset —
skip-changeset@objectstack/cliis publishable, but itsfileswhitelist is["dist","README.md","CHANGELOG.md"]and this diff is 100% underpackages/cli/test/. Not one published byte changes, so the PR releasesnothing and has nothing to describe in release notes.
Generated by Claude Code
Generated by Claude Code