fix: eliminate SWC temporal-dead-zone import cycles (one already crashing), stop a silent Secure-cookie strip, and add the CI guard that would have caught all of it#560
Merged
Conversation
…import (SWC TDZ crash) The better-auth DI tokens were split across two files that imported each other: BETTER_AUTH_INSTANCE was declared in core-better-auth.module.ts, while BETTER_AUTH_CONFIG and BETTER_AUTH_COOKIE_DOMAIN were declared in core-better-auth.service.ts. The service imported BETTER_AUTH_INSTANCE from the module, and the module imported the other two tokens (plus the service class) back from the service — a genuine import cycle. Under tsc/CommonJS this happened to work by evaluation-order luck. Under SWC (`nest start -b swc`) it crashes at startup: ReferenceError: Cannot access 'BETTER_AUTH_INSTANCE' before initialization The tokens are dereferenced at module-evaluation time (`@Inject(...)` in the CoreBetterAuthService constructor is evaluated when the class is defined), so the cycle lands in the temporal dead zone. Fix: move all three tokens into a dedicated leaf module, core-better-auth.constants.ts, which imports nothing. Module and service both import the tokens from there, making the graph acyclic and the initialization order deterministic for every compiler. Backward compatible: both core-better-auth.module.ts and core-better-auth.service.ts re-export the tokens, so existing deep imports (`from '.../core-better-auth.module'` / `'.../core-better-auth.service'`) and the package barrel keep working unchanged. No public API change. Verified by compiling src/ with SWC (legacy decorators + decoratorMetadata, matching the Nest CLI's SWC builder) and requiring the module: the ReferenceError reproduces before this change and is gone after it. madge reports the better-auth cycles dropping from 9 to 1; the remaining guard -> module cycle is benign because it only dereferences CoreBetterAuthModule lazily inside a method body, never at eval time. Build, lint, format, 691 unit tests and 1380 e2e tests all pass.
kaihaase
changed the title
The two classes are mutually recursive and lived in two files that imported
each other. FilterInput referenced CombinedFilterInput EAGERLY — in a decorator
argument (`type: CombinedFilterInput`) and in the `design:type` metadata that
emitDecoratorMetadata emits for `combinedFilter?: CombinedFilterInput`. Both are
evaluated at class-definition time, i.e. while the module is still initializing,
so on the cycle they read a class binding still in its temporal dead zone.
This was not latent. It crashed, today, in shipped code:
require('dist/core/common/inputs/combined-filter.input.js')
-> ReferenceError: Cannot access 'CombinedFilterInput' before initialization
It stayed hidden only because entering through the package barrel pulls
filter.input in first via other importers (filter.args, filter.helper). A
vendor-mode deep import, a unit test importing the input directly, or a
reordering of src/index.ts would all have hit it.
A lazy thunk does NOT fix this: `type: () => CombinedFilterInput` still leaves
the eager design:type metadata behind, and SWC's `typeof` guard does not protect
the member expression it compiles to. The only fix is to remove the import edge,
so both classes now live in filter.input.ts with CombinedFilterInput declared
FIRST (that ordering is load-bearing). combined-filter.input.ts stays as a
re-export, so every existing import path still resolves.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
restricted.decorator sat on TWO runtime cycles at once:
restricted.decorator -> db.helper -> input.helper -> restricted.decorator
restricted.decorator -> tenant/core-tenant.helpers -> config.service
-> input.helper -> restricted.decorator
input.helper therefore evaluated while restricted.decorator was still
initializing. Nothing crashed, but only because every cross-cycle dereference
happened to sit inside a function body. A single top-level line in input.helper
— a module-level alias of checkRestricted, an @Restricted-decorated class,
design:type metadata — would have thrown under SWC -> CommonJS:
ReferenceError: Cannot access 'checkRestricted' before initialization
…in the file that drives field-level access control, on a compiler the default
build never runs, with a fully green test suite.
Two leaf extractions remove both edges:
- id.helper.ts <- the ID cluster (equalIds, getIncludedIds, getStringIds,
getObjectIds) out of db.helper. Depends only on mongoose
Types, lodash and a type.
- clone.helper.ts <- clone / deepFreeze out of input.helper, so config.service
can reach them without importing input.helper.
Both are true leaves (no framework imports), and db.helper / input.helper
re-export their former members, so every existing import path keeps working.
Removing one edge is not the same as removing the cycle: the id.helper
extraction alone left the config.service path fully intact. restricted.decorator
now appears in ZERO cycles; repo-wide cycles drop from 10 to 6, and most of the
remainder are type-only imports that madge reports but both compilers erase.
Its three exports are additionally converted from `const` arrows to hoisted
function declarations. With the cycles gone this is defense in depth: hoisted
declarations are initialized before any module body runs, so they stay TDZ-immune
whatever the import graph does next.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The DI-token extraction broke module <-> service, but a structurally identical cycle survived in the same module: better-auth-roles.guard imported CoreBetterAuthModule for the static getTokenServiceInstance() accessor, while the module imported the guard to register it as APP_GUARD. It was benign only because both sides dereference each other lazily inside method bodies — one static field initializer or one typed constructor parameter away from the very crash this branch fixes. core-better-auth.registry.ts now holds the BetterAuthTokenService reference. It uses `import type` only, which both tsc and SWC erase, so it emits no require() at all and is a true leaf. The guard reads the registry; the module writes it; CoreBetterAuthModule.getTokenServiceInstance() stays as a thin delegate, so the public API is unchanged (core-ai-mcp.controller keeps working). The better-auth module now has zero cycles. Also fixes a test-isolation leak found on the way: CoreBetterAuthModule.reset() cleared serviceInstance and userMapperInstance but never the token service, so a token service from a previous testing module survived into the next one and BetterAuthRolesGuard verified tokens against a stale instance. Additionally: - The constants docblock claimed the change "makes the dependency graph acyclic". That was false while the guard cycle survived, and a maintainer reading it would reasonably have hoisted the guard's lazy lookup into a static field — recreating the crash in good faith. Corrected, and the real rule is spelled out: a cycle is only fatal when dereferenced at module-evaluation time. - The backward-compat re-exports are narrowed to exactly the symbols each file exported before, instead of widening the surface to three valid import paths per token — which is precisely how the original cycle was born. - Token JSDoc now states the injected type; they are untyped string tokens, so the docblock is the only type signal a consumer gets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
createBetterAuthInstance pins `advanced.useSecureCookies: false` so BetterAuth's
native handlers read the same unprefixed session-cookie name the cookie helper
writes, and restores the Secure attribute via `advanced.defaultCookieAttributes`
on an https baseURL. But a consumer's `options.advanced` was merged SHALLOWLY —
so a project setting defaultCookieAttributes for an entirely unrelated reason
wholesale-replaced the framework's `{ secure: true }`:
options.advanced.defaultCookieAttributes = { partitioned: true }
-> Set-Cookie: iam.session_token=…; HttpOnly; SameSite=Lax (Secure GONE)
On an https deployment that ships session cookies in the clear on every flow
BetterAuth handles natively — 2FA verify, social callback, magic link, passkey —
all of which ESTABLISH a session. No error, no warning. `sameSite: 'none'` would
self-detect (browsers reject it without Secure), but `{ partitioned: true }` or
`{ domain: … }` strip it silently.
That one key is now deep-merged: `secure: true` is the base and the consumer's
keys spread over it, so an EXPLICIT `secure: false` still wins while an unrelated
key can no longer clobber it.
Tests: the existing spec only asserted the CONFIG shape. It would have stayed
green through this bug, and through a BetterAuth upgrade that reordered the
spread in createCookieGetter (where our Secure flag rests entirely on
defaultCookieAttributes being spread AFTER `secure: !!secureCookiePrefix`). The
new tests call BetterAuth's own getCookies() on a constructed instance and assert
the DERIVED attributes, plus sendWebResponse() — the helper a prior review accused
of dropping Secure — forwards it to Express verbatim. Derivation -> forwarding ->
wire is now pinned end to end.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This bug class had the worst possible failure profile: it passed a fully green
CI and crashed only for consumers. Nothing in the pipeline could see it.
- `nest build` / `pnpm run build` use tsc, whose emit tolerates these cycles.
- vitest DOES run SWC — but through Vite's module runner, whose getter-based
live bindings resolve a cycle lazily, so the temporal dead zone never opens.
2000+ green tests proved nothing here.
- oxlint has no `import/no-cycle` rule (checked; it is not implemented).
Only SWC -> CommonJS -> Node's require() fails, and no job ran that.
`pnpm run check:swc-tdz` compiles with SWC and loads the output under the CJS
loader. It is wired into check:raw (so `pnpm run check` picks it up) and into the
build workflow, which today runs neither `check` nor a server boot and is
therefore weaker than the local gate.
It loads EVERY compiled module as its own entry point, not just the barrel:
whether such a cycle throws depends on which module the graph is entered through,
and a barrel-only load reported the (real, crashing) combined-filter.input bug as
green. Sharded across 4 forked children — capped at 4 because each child pays a
full node_modules cold load, and 12 shards measured slower than running
single-threaded. It deliberately does NOT narrow the entry set to "files in a
cycle": that would make the guard's correctness depend on a static cycle analysis,
and a blind spot there would not produce a wrong answer but a GREEN one, on the
one check that exists because everything else is already blind.
Two structural specs pin what the guard cannot see. check:swc-tdz catches the
CRASH; it does not catch the DISARMING of a safety property. Each invariant below
can be reverted by a well-meaning refactor — an "organize imports", a "modernize
to arrow functions" — with nothing turning red, because the cycle would be
present-but-disarmed again:
- the DI-token and registry files stay import-free leaves
- the guard reaches the token service without importing the module
- the token STRING VALUES (public API; consumers may write
@Inject('BETTER_AUTH_INSTANCE') as a literal — a typo would be invisible
in-repo, since provide/inject/@Inject all read the same imported symbol)
- restricted.decorator takes its helpers from the leaves, and its exports stay
hoisted function declarations
- id.helper / clone.helper import no framework code
- the AI services' load-bearing `import type` stays type-only
Every one of these was mutation-tested: the bug was reintroduced and the test
watched go red.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The global setup drops whatever MONGODB_URI points at, and that variable is not always a test database: a running `lt dev` session exports it pointing at the project's DEVELOPMENT database. Running the suite from that shell would silently wipe the developer's data. Both the setup and the stale-run cleanup now require the database name to look like a test database before anything is dropped. CI does not set MONGODB_URI, so the guard never triggers there, and the run databases the suite creates (nest-server-e2e-run-*, nest-server-ci-*) satisfy it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The constraint this branch establishes existed nowhere except the source files it
fixed — and the footgun path stayed open: importing a token from
core-better-auth.module inside the module still compiles, still passes every test,
and only crashes under SWC. The same shape is still latent in tus.module.ts and in
nine AI service files.
- .claude/rules/better-auth.md gains a §6 on DI-token placement: the rule, the
exact error, the mistakes table, and — most importantly — a table of which
tools can and cannot see a regression here. A green `pnpm test` is NO evidence.
- .claude/rules/architecture.md generalises it to all core modules, lists which
modules are already safe and which are one back-import away, and records the
lesson that cost the most time: removing one edge is not removing the cycle.
Re-run madge and check the file appears in ZERO cycles.
- migration-guides/11.27.6-to-11.27.7.md covers the SWC startup crash (with the
error string, so an affected consumer searching for it lands here), the
CombinedFilterInput deep-import crash, the cookie Secure fix, and the
vendor-mode note that the BetterAuth change is an atomic file set — a partial
core sync that omits the new leaf leaves an unresolvable import.
- CLAUDE.md: document check:swc-tdz, and correct the stale claim that this repo
lints with ESLint and formats with Prettier (it uses oxlint / oxfmt).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kaihaase
changed the title
…able Three defects in the guard as first committed — one of which made it capable of reporting GREEN while checking nothing at all. 1. It built into the REAL dist/. `nest-cli.json` sets `deleteOutDir: true`, so `pnpm run check:swc-tdz` wiped the tsc-built artifact and left a partial SWC build in its place — the directory `pnpm start:prod` runs. Inside the check chain the next step rebuilt it, which is why it went unnoticed; standalone it silently destroyed the user's build. It now builds into `dist-swc-tdz/` via its own tsconfig: removed on success, KEPT on failure, because the emitted JS is the evidence (it shows whether the cyclic binding is dereferenced at top level or inside a function body). 2. It could pass having loaded zero modules. An empty or mis-emitted output directory meant `files = []`, no shard had anything to do, no failure was reported, and it printed "0 modules load clean" and exited 0. The one check that exists because tsc, vitest and oxlint are all blind to this bug class could itself go quietly blind. There is now a floor: if fewer than 200 loadable modules are found it fails instead of reporting success. 3. Nothing bounded a forked shard. A module that blocks inside require() — a socket, stdin, or a server boot if an exclusion ever stops matching — would have left the parent in Promise.all forever: a hang with no output and no error. Each shard now has a hard timeout, and `main.js` (whose last line calls `bootstrap()`) is excluded by BASENAME rather than by an absolute path, which would silently stop matching if the emit layout ever shifted. `process.send()` also waits for the flush before exiting, so a healthy shard cannot be misread as a dead one. Being side-effect free also removes the ordering constraint, so the guard moves AHEAD of the test step: a cycle now fails in ~10s instead of after the full suite. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The registry was re-exported through the module barrel, which put `setBetterAuthTokenService()` on the package's public surface — letting a consumer swap or null the very token service `BetterAuthRolesGuard` makes its authorization decisions with. In a PATCH release, no less. It is module-internal plumbing: it exists only so the guard can reach BetterAuthTokenService without importing CoreBetterAuthModule (the cycle). Nothing outside the module needs it, and consumers that want the instance already have the public, read-only `CoreBetterAuthModule.getTokenServiceInstance()`. With it dropped, the package root exports exactly what it did on develop — 472 symbols, zero added, zero removed. Verified by building both refs and diffing. Also pins the backward-compat re-exports. Every symbol moved to a leaf in this branch stays reachable only because its old home re-exports it, and this package has no `exports` map — consumers deep-import these files directly. Those re-export lines carry a public contract while looking exactly like dead code to anyone tidying up imports, and deleting one would break every deep importer with nothing turning red: the symbol is still exported from the barrel, so the package-root tests stay green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…the guide - .claude/rules/better-auth.md §5 still showed `CoreBetterAuthModule.getTokenServiceInstance()` inside the BetterAuthRolesGuard sample, while §6 — added in this same branch — lists importing the module into the guard as the mistake that re-creates the cycle. An agent copying §5 would have re-armed the bug the branch just fixed. The sample now reads the registry, with a note on when the static accessor is still fine (everywhere except the guard). - The migration guide documented ONE atomic file set for vendor-mode consumers. There are three: the BetterAuth tokens, the core helpers (id.helper / clone.helper — a sync that takes db.helper without id.helper gives an unresolvable import, exactly the hazard the guide already warned about elsewhere), and the filter inputs. All three are now listed, along with an explicit warning not to "clean up" the re-export lines that hold backward compatibility together. - The guide called the new leaves "internal". Now that the registry is genuinely internal (not barrel-exported) and the others are not, it says which is which — and records that the public API is unchanged, verified by diffing the export surface of both versions. FRAMEWORK-API.md regenerated for the version bump. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The leaf rule this branch introduced shipped with ten known violations of itself:
`TUS_CONFIG` in `tus.module.ts`, and 22 `AI_*` tokens spread across eleven
`ai/services/*.service.ts` files. None crashed — their graphs happened to be
acyclic — but "happens to be acyclic" is not a safety property, it is luck that
nothing is watching:
- `@Inject(TOKEN)` is a constructor-parameter decorator, evaluated at
class-definition time. A token declared in the file that also gets imported
back is one back-import away from being read inside its own temporal dead zone.
- `tsc`, `pnpm test` and `oxlint` are ALL blind to that. Only
`pnpm run check:swc-tdz` sees it, and only after the fact.
TUS_CONFIG now lives in `tus.constants.ts`, the AI tokens in `core-ai.constants.ts`.
Both files import nothing, so they can never be mid-evaluation when someone imports
them, under any compiler.
Also removes the last real cycle in the AI module. `core-ai.service` imports
`core-ai-interaction.service` (it needs the class), and the interaction service
needed the `AiInteractionRecord` type back — a cycle held apart by ONE keyword: the
`import type`, which both compilers erase. `core-ai.service` has a constructor
`@Inject`, so `decoratorMetadata` emits `design:paramtypes` at module-evaluation
time: an IDE "organize imports" widening that import would have armed the crash
silently. The type moved to its own interface leaf, so the edge is gone rather than
merely erased.
Every old location re-exports what it lost. Public API is byte-identical: 472
exports on develop, 472 here, zero added, zero removed — verified by building both
refs and diffing.
Repo-wide cycles: 6 -> 5. The five that remain are type-only imports madge reports
but both compilers erase (empty emits) — not runtime cycles at all.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…loaded The e2e suites intermittently failed with `socket hang up`, 401s on auth queries, and tests hanging past the 30s timeout — all of which read like genuine product bugs while being pure resource starvation. The trigger is two full e2e runs overlapping: several `lt dev` environments, a second project's check, or simply a forgotten background run. The escape hatch already existed — `CHECK_LOW_RESOURCE=1` caps parallel forks and raises timeouts — but it was OPT-IN, which is the wrong default. It only helps the developer who already knows the env var exists, and that is precisely the developer who does not need it. Whoever gets bitten is the one who has never heard of it, staring at a "flaky" auth test and re-running it until it passes. It now auto-enables from the 1-minute load average normalised per core. Explicit settings still win in both directions: CHECK_LOW_RESOURCE=1 -> force on (CI, or a machine you know is busy) CHECK_LOW_RESOURCE=0 -> force off (benchmarking; never auto-throttle) unset -> auto (on iff normalised load >= 0.7) Verified all three: under 12 CPU burners the unset case throttles (maxForks=4, timeouts raised) and prints why; `=0` overrides it back off. Load average is unavailable on Windows, where auto-detection stays off and the flag remains. Throttling costs some wall-clock. A green suite that has to be re-run costs more, and a red one that gets dismissed as "flaky" costs the most. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
.claude/rules/architecture.md introduced "DI tokens belong in an import-free leaf"
and then listed ten live violations of itself. A rule with known violations is a
suggestion, and a suggestion does not survive a refactor.
`import-cycle-invariants.spec.ts` now fails if:
- any DI token is declared in a *.module.ts or a *.service.ts (re-exports are
fine — those are the backward-compat shims; only declarations re-create the edge)
- any constants leaf grows an import, an export-from, or a require
- `core-ai-interaction.service` imports from `core-ai.service` at all
This matters because `check:swc-tdz` catches the CRASH, not the DISARMING of a
safety property — and only the second one is silent. A "modernize to arrow
functions", an "organize imports", a "split this file up" can each re-arm the bug
with everything staying green.
Every assertion was mutation-tested: the violation reintroduced, the test watched
go red. That includes a false positive in the first version of the spec, which
flagged the leaves' own docblocks — they legitimately contain the words `import`
and `require()` while explaining the bug they prevent. Only the code is evidence.
Rules updated to describe what is actually true: zero known violations, cycles
10 -> 5, and the lesson that cost the most time — removing one edge is not removing
the cycle.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Auto-updated by extras/update-spectaql-version.mjs to follow the package.json bump. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The guide listed three atomic file sets; the AI and TUS token extractions landed
afterwards and were missing. There are five, and six new files — a partial core
sync that omits any one of them leaves an unresolvable import.
Also adds what the guide did not say and should have: vendor-mode projects with
local edits in db.helper, input.helper, filter.input, restricted.decorator or any
ai/services/*.service.ts WILL get merge conflicts, because this release moves code
out of exactly those files. The resolution is always the same — keep the local
logic, keep the new import/export-from lines pointing at the leaf, do not restore
the declarations that moved out.
And the warning that matters most: the `export { … } from './…'` lines now scattered
across seventeen files look exactly like dead code and are the ENTIRE backward-
compatibility layer. This package has no `exports` map, so consumers deep-import
those files directly; deleting one line breaks all of them with nothing turning red,
because the symbol is still exported from the package root.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The prose said six; the list under it had seven, and the diff has seven. A vendor-mode consumer verifying "all six exist" after an interrupted sync would have stopped one file short — of a file whose absence is an unresolvable import. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
npm retired the legacy `/-/npm/v1/security/audits/quick` endpoint (HTTP 410), which every pnpm 10.x calls — so `pnpm audit`, and therefore `pnpm run check`, failed for a reason unrelated to the code and unfixable by any pnpm 10 version. Blocking check on that is wrong twice: it is not a finding, and a permanently-red audit trains everyone to ignore a red audit (the real hazard). check.mjs now degrades ONLY the retired-endpoint signature (ERR_PNPM_AUDIT_BAD_RESPONSE / "audit ... retired") to a loud non-blocking warning — never a green ✓, since that would claim "no vulnerabilities", which was not verified. A real vulnerability (parseable counts) and any other non-zero exit stay fatal. On pnpm 11 (next commit) audit works again via the bulk endpoint and this degrade never triggers; it stays as a safety net. npm audit is NOT used as a fallback: it resolves the tree from package.json alone, ignoring pnpm-lock.yaml and the security overrides, so it reports phantom findings for CVEs the overrides already fix. A misleading red is worse than an honest "could not run". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…yaml Motivation: `pnpm audit` is broken on every pnpm 10.x — npm retired the legacy audit endpoint (HTTP 410) and only pnpm 11 uses the working bulk-advisory endpoint. Verified empirically across 10.33.2, 10.34.5 and 11.13.0 (the repo's packageManager pin forces pnpm to re-exec the pinned version, so the fix only lands with the packageManager bump). pnpm 11 is a breaking config change, migrated canonically per https://pnpm.io/blog/releases/11.0: - The `pnpm` field in package.json is no longer read → moved to pnpm-workspace.yaml: overrides, allowBuilds, nodeLinker, autoInstallPeers, strictPeerDependencies, peerDependencyRules. .npmrc is now auth/registry only. - `allowBuilds` replaces `onlyBuiltDependencies` — a map that must classify EVERY install-script package as true/false. bcrypt/@swc/core/esbuild/… = true; @scarf/scarf (telemetry) = false. An unclassified one makes `pnpm install` exit non-zero and appends a broken stub (that stub was the pre-existing mess in the repo). Security overrides pruned 36 → 9. Each survivor is load-bearing: removing it lets the package resolve back INTO its vulnerable range (verified by a with/without lockfile diff, and confirmed by `pnpm audit` — clean before and after the prune). The 27 removed were no-ops (parents have since shipped fixed versions). Kept: ajv, @babel/core, picomatch, ws, uuid, vite, nodemailer, multer, js-yaml. Verified working on pnpm 11: - `pnpm install` (frozen + non-frozen), native builds (bcrypt), 0 ignored-builds errors - `pnpm audit` runs for real via the bulk endpoint → 0 vulnerabilities - every pnpm invocation in scripts/Dockerfile/CI: pnpm pack, store prune, install --prod --ignore-scripts, rebuild bcrypt — all exit 0; none use commands removed in v11 (install -g, link, server) - `pnpm run check` fully green (2107 tests) - CI (pnpm/action-setup@v6) and Docker (corepack) follow the packageManager field, so they use pnpm 11 with no workflow/Dockerfile edits Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The e2e config runs spec files in parallel forks (fileParallelism: true), and global-setup gives the whole RUN one database. So every parallel file shared one DB — and a file that mutates a GLOBAL collection corrupted the others mid-run. Concrete failure: better-auth-integration.story.test.ts clears the `jwks` collection (BetterAuth's JWT signing keys, used implicitly by every BetterAuth instance) in a beforeAll. Run in parallel with better-auth-autoregister-false, it wiped the keys that file's token was signed with → its authenticated request got a spurious 401 (expected 200). Isolated, both pass; together under parallel load, the 401 appeared intermittently. Root cause, not flakiness. tests/setup.ts runs in every fork BEFORE the test file (and config.env.ts) is imported, so it appends the vitest pool id to MONGODB_URI there: each concurrent fork gets its own database (…-run-<ts>-p<pid>-w<N>). Files sharing a fork run sequentially (safe); files in different forks can no longer collide. This is the right layer — better-auth-integration uses ServerModule, which reads the DB from the already-imported config, so per-file overrides in the spec don't work. No reporter change needed: db-lifecycle.reporter drops every DB whose name starts with the run DB name, which already matches the -w<N> forks — verified no orphaned databases remain after a run. Verified: the two colliding files pass together 3x; full `pnpm run check` green 3x in a row (2107 tests); zero orphaned -w databases afterwards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per-worker DB isolation put the actual test data in `${runDb}-w<N>` fork
databases, but the "kept for debugging" message still named the base `${runDb}`
— which the main process holds but no fork writes to. A developer debugging a
failed run would connect to an empty database.
The failure branch now lists the real `${runDb}-w*` fork databases (best-effort;
falls back to the base name if the listing fails). Verified end to end: a failing
run keeps the fork DB with its data and names it correctly; the next passing run
drops it (no orphaned databases).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kaihaase
marked this pull request as ready for review
July 15, 2026 07:40
kaihaase
added a commit
that referenced
this pull request
Jul 15, 2026
… the permission layers Rebased onto 11.27.7 (#560). Two related security/consistency fixes to the permission checks. ## 401/403 policy — one rule across all five layers Permission errors for AUTHENTICATED users were thrown as UnauthorizedException (401). Frontends treat 401 as "session expired" and auto-logout (the @lenne.tech/nuxt-extensions interceptor patches $fetch/fetch globally), so a mere permission error kicked logged-in users out of the whole app. `accessDeniedException(user, message?)` — a FACTORY, not a class — now derives the status from the requester's auth state and returns the NATIVE ForbiddenException / UnauthorizedException, so `instanceof` and `@Catch(...)` filters in consumer projects keep working and the REST wire body is unchanged (a custom HttpException subclass would break both). The five layers now agree: - check() / checkRestricted() / prepareInput(): 403 when authenticated, 401 otherwise - CoreTenantMemberModel.securityCheck(): was 401 for an authenticated non-member — the exact bug — now 403 - S_NO_ONE: was 401 (guards) / 403 (tenant guard) / mixed (check) — now 403 everywhere, for everyone (authenticating can never unlock it, so 401 would be a lie) - CoreTenantGuard: threw 403 at UNAUTHENTICATED requesters ("Authentication required") — now 401 Messages are the translatable ErrorCode.ACCESS_DENIED / ErrorCode.UNAUTHORIZED the guards already throw (the old raw strings had no #LTNS_ marker, so the frontend could not translate them, and they leaked internal class/field names into the response — those go to the debug log now). ## Ownership (S_SELF / S_CREATOR) read from the persisted object, not the payload checkRestricted() decided S_SELF/S_CREATOR from `data`. On the input path `data` IS the caller DTO, so an authenticated attacker could unlock an owner-restricted field on someone else's record by asserting ownership in the body — the service writes to the target it was called with, not to the ids in the payload. Three reads were affected (S_SELF, S_CREATOR, and the isCreatorOfParent trust that propagates into nested objects); all now read from config.dbObject on input, matching check(), which always did. Output is unchanged (there `data` IS the persisted object). Where no dbObject exists (create), ownership cannot be established — matching check() and the old behavior for honest callers. The fields were effectively inert before (MapAndValidatePipe strips id/createdBy), so this is a latent hardening, not a live-exploit fix — but it MUST be audited before adopting, because a field that looked owner-restricted may become writable. An instance-wide scan of all consumer projects found none currently exploitable; the migration guide has the audit steps and the S_CREATOR trap (the inviter, not the user, is the "creator" in invite flows). BREAKING: (1) authenticated permission errors → 403, (2) S_NO_ONE → 403 for everyone, (3) messages → ErrorCodes, (4) CoreTenantGuard → 401 when unauthenticated, (5) S_SELF/S_CREATOR on input fields now enforce ownership. See migration-guides/11.27.7-to-11.28.0.md. Also hardened: core-better-auth.controller re-throws `instanceof HttpException` instead of an enumeration that masked everything it forgot as a 500. FRAMEWORK-API.md gained an Errors & Status Codes section (extracted from source). versioning.md now documents the spectaql.yml version lockstep. Tests: factory contract (instanceof + wire format), all five layers, S_NO_ONE across paths, and the ownership fix in both directions (attack rejected, owner allowed) — red-green verified.
kaihaase
added a commit
that referenced
this pull request
Jul 15, 2026
…across the permission layers (#559) * 11.28.0: unify the 401/403 policy and fix ownership evaluation across the permission layers Rebased onto 11.27.7 (#560). Two related security/consistency fixes to the permission checks. ## 401/403 policy — one rule across all five layers Permission errors for AUTHENTICATED users were thrown as UnauthorizedException (401). Frontends treat 401 as "session expired" and auto-logout (the @lenne.tech/nuxt-extensions interceptor patches $fetch/fetch globally), so a mere permission error kicked logged-in users out of the whole app. `accessDeniedException(user, message?)` — a FACTORY, not a class — now derives the status from the requester's auth state and returns the NATIVE ForbiddenException / UnauthorizedException, so `instanceof` and `@Catch(...)` filters in consumer projects keep working and the REST wire body is unchanged (a custom HttpException subclass would break both). The five layers now agree: - check() / checkRestricted() / prepareInput(): 403 when authenticated, 401 otherwise - CoreTenantMemberModel.securityCheck(): was 401 for an authenticated non-member — the exact bug — now 403 - S_NO_ONE: was 401 (guards) / 403 (tenant guard) / mixed (check) — now 403 everywhere, for everyone (authenticating can never unlock it, so 401 would be a lie) - CoreTenantGuard: threw 403 at UNAUTHENTICATED requesters ("Authentication required") — now 401 Messages are the translatable ErrorCode.ACCESS_DENIED / ErrorCode.UNAUTHORIZED the guards already throw (the old raw strings had no #LTNS_ marker, so the frontend could not translate them, and they leaked internal class/field names into the response — those go to the debug log now). ## Ownership (S_SELF / S_CREATOR) read from the persisted object, not the payload checkRestricted() decided S_SELF/S_CREATOR from `data`. On the input path `data` IS the caller DTO, so an authenticated attacker could unlock an owner-restricted field on someone else's record by asserting ownership in the body — the service writes to the target it was called with, not to the ids in the payload. Three reads were affected (S_SELF, S_CREATOR, and the isCreatorOfParent trust that propagates into nested objects); all now read from config.dbObject on input, matching check(), which always did. Output is unchanged (there `data` IS the persisted object). Where no dbObject exists (create), ownership cannot be established — matching check() and the old behavior for honest callers. The fields were effectively inert before (MapAndValidatePipe strips id/createdBy), so this is a latent hardening, not a live-exploit fix — but it MUST be audited before adopting, because a field that looked owner-restricted may become writable. An instance-wide scan of all consumer projects found none currently exploitable; the migration guide has the audit steps and the S_CREATOR trap (the inviter, not the user, is the "creator" in invite flows). BREAKING: (1) authenticated permission errors → 403, (2) S_NO_ONE → 403 for everyone, (3) messages → ErrorCodes, (4) CoreTenantGuard → 401 when unauthenticated, (5) S_SELF/S_CREATOR on input fields now enforce ownership. See migration-guides/11.27.7-to-11.28.0.md. Also hardened: core-better-auth.controller re-throws `instanceof HttpException` instead of an enumeration that masked everything it forgot as a 500. FRAMEWORK-API.md gained an Errors & Status Codes section (extracted from source). versioning.md now documents the spectaql.yml version lockstep. Tests: factory contract (instanceof + wire format), all five layers, S_NO_ONE across paths, and the ownership fix in both directions (attack rejected, owner allowed) — red-green verified. * fix(tests): stop wiping the shared jwks keyset, which stranded other files' JWTs better-auth-integration.story.test.ts dropped the whole BetterAuth signing keyset in a beforeAll. All e2e files share ONE database per run and execute in parallel forks, so the wipe invalidated every JWT that other files had already minted — ai.e2e-spec.ts caches its bearer tokens in beforeAll and reuses them across ~40 tests, which then failed with 401 en bloc. retry could not help: the token was durably dead. Reproduced deterministically by pairing the two files, and confirmed on develop — predates the 401/403 work. The wipe guarded against keys left over from an earlier run under a different secret; since 11.24.4 every run gets a fresh, unique database, so that state can no longer exist. Separable from the 11.28.0 change — kept on this branch only because the pipeline cannot go green without it. --------- Co-authored-by: Kai Haase <kai.haase@lenne.tech>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
The better-auth DI tokens lived in two files that imported each other.
@Inject(BETTER_AUTH_INSTANCE)is a constructor-parameter decorator, and decorator arguments evaluate at class-definition time — while the module is still initializing. On a cycle that reads aconststill in its temporal dead zone. Under tsc it worked by evaluation-order luck; under SWC (nest start -b swc) it died at startup:Reviewing that fix turned up more instances of the same bug class — one already crashing in shipped code — and a cookie-security bug next door.
Why nothing caught it
tsc/pnpm run buildpnpm test(vitest)oxlintimport/no-cyclerequire()A green test suite was no evidence at all here. That is why this PR ships a guard, not just a fix.
Fixes
🔴
CombinedFilterInput— a live crash in shipped code.require('dist/core/common/inputs/combined-filter.input.js')threw. Masked only because entering via the barrel pullsfilter.inputin first — a vendor-mode deep import or a unit test importing it directly hit it. A lazy thunk does not help:emitDecoratorMetadatastill emits an eagerdesign:type. The import edge had to go.🟠
restricted.decorator— on two cycles, in the file that drives field-level access control. Saved only by the accident that every cross-cycle dereference sat inside a function body. Two leaf extractions (id.helper,clone.helper) remove both edges. Removing one edge is not removing the cycle: the first extraction left the second path fully intact.🟠 better-auth — the last module cycle + a
reset()leak.better-auth-roles.guard↔core-better-auth.modulesurvived the original fix; now brokered through animport type-only registry leaf.reset()never cleared the token service, so a stale instance leaked between testing modules.🟠 Every remaining DI token.
TUS_CONFIGand 22AI_*tokens sat in modules and services. None crashed — their graphs happened to be acyclic — but "happens to be acyclic" is not a safety property. All moved to import-free leaves. The AI module's last real cycle (core-ai.service↔core-ai-interaction.service) was held apart by a singleimport type; the type moved to its own leaf, so the edge is gone rather than merely erased.🔒 Session cookies could ship without
Secure. A consumer settingadvanced.defaultCookieAttributesfor an unrelated reason ({ partitioned: true },{ domain: … }) wholesale-replaced the framework's{ secure: true }— the merge was shallow. On https: session cookies in the clear on every natively-handled flow (2FA verify, social callback, magic link), all of which establish a session. Silent. Deep-merged now; an explicitsecure: falsestill wins.Guards
pnpm run check:swc-tdzcompiles with SWC and loads every module as its own entry point — a barrel-only load called theCombinedFilterInputcrash green. Runs before the tests, so a cycle fails in ~10s rather than after the full suite. ~3s, 4 shards, no new dependencies, side-effect free (own throwaway outDir).It deliberately does not narrow the entry set to "files in a cycle": that would make the guard depend on a static analysis whose blind spot would produce not a wrong answer but a green one, on the single check that exists because everything else is blind.
tests/unit/import-cycle-invariants.spec.tsenforces the rule instead of documenting it. The guard catches the crash; it does not catch the disarming of a safety property — and only the second is silent. An "organize imports", a "modernize to arrow functions", a "split this file up" can each re-arm the bug with everything staying green. The spec fails if a DI token reappears in a module/service, if a leaf grows an import, or if the AI services import each other again.vitest-e2e.config.tsauto-throttles. The e2e flakes (socket hang up, 401s, hangs past the timeout) were pure resource starvation when two full runs overlap. The escape hatch existed (CHECK_LOW_RESOURCE=1) but was opt-in — it only helped the developer who already knew the env var existed, which is precisely the one who did not need it. It now auto-enables from the load average; explicit settings still win in both directions.Verification
Every claim was reproduced, not reasoned about:
require('dist/index.js')ondevelopstill throws today.tsc --noEmitreported zero errors on the same code.getCookies()and the realSet-Cookieheader — not against our config shape.dist/.Cycles 10 → 5. The five that remain are type-only imports madge reports but both compilers erase — not runtime cycles at all.
pnpm run check: ✅ 2107 tests, 0 vulnerabilities, all steps green.Compatibility — public API is byte-identical
Built both refs and diffed the export surface: 472 symbols on
develop, 472 here. Zero added, zero removed. Every deep-import path keeps all its exports. Token string values unchanged, and pinned by a test — consumers may write@Inject('BETTER_AUTH_INSTANCE')as a literal.This is what makes 11.27.7 a PATCH despite the size of the diff: the criterion is breaking vs. non-breaking, not volume.
Vendor mode: three atomic file sets (BetterAuth tokens, core helpers, filter inputs) plus the new AI/TUS leaves. A partial core sync that omits a new leaf leaves an unresolvable import. See
migration-guides/11.27.6-to-11.27.7.md.Also in here
tests/global-setup.tsnow refuses to drop a database that is not recognizably a test DB. It drops whateverMONGODB_URIpoints at — and a runninglt devsession exports that pointing at the development database.CLAUDE.mdclaimed this repo lints with ESLint and formats with Prettier. It uses oxlint/oxfmt.🤖 Generated with Claude Code