diff --git a/.claude/agent-memory/lt-dev-backend-reviewer/MEMORY.md b/.claude/agent-memory/lt-dev-backend-reviewer/MEMORY.md index 9997b89..02a9476 100644 --- a/.claude/agent-memory/lt-dev-backend-reviewer/MEMORY.md +++ b/.claude/agent-memory/lt-dev-backend-reviewer/MEMORY.md @@ -1,3 +1,4 @@ - [Core ErrorCode in framework repo](project_core-errorcode.md) — src/core has its own ErrorCode registry; modern core modules use it, but the baseline is mixed (core-user still raw strings). - [AI module prompt/slot service raw exceptions](project_ai-module-prompt-service-errors.md) — most of the AI module routes through ErrorCode, but CoreAiPromptService + CoreAiSlotService still throw raw-string ForbiddenExceptions; consistency cleanup, not a security gap. - [SWC TDZ import cycles](project_swc-tdz-import-cycles.md) — cycles crash under `-b swc` only when deref'd at module-eval time; full 9-cycle audit: only 3 are real, `filter.input ↔ combined-filter.input` already throws on deep import. +- [401/403 permission-denial surface](project_401-403-denial-surface.md) — 5 layers deny permission with inconsistent status codes (S_NO_ONE alone has 3); map all five before grading any auth-status change. diff --git a/.claude/agent-memory/lt-dev-backend-reviewer/project_401-403-denial-surface.md b/.claude/agent-memory/lt-dev-backend-reviewer/project_401-403-denial-surface.md new file mode 100644 index 0000000..390aa49 --- /dev/null +++ b/.claude/agent-memory/lt-dev-backend-reviewer/project_401-403-denial-surface.md @@ -0,0 +1,43 @@ +--- +name: 401-403-denial-surface +description: The framework denies permission from 5+ layers with inconsistent 401/403 semantics — S_NO_ONE alone has 3 different status codes; map before reviewing any auth-status change +metadata: + type: project +--- + +`@lenne.tech/nest-server` denies a request for permission reasons from **five** layers, +and their 401/403 semantics do **not** agree. Anyone reviewing an auth-status change must +map all five, not just the ones in the diff. + +| Layer | File | S_NO_ONE | authenticated + lacks right | unauthenticated | +|---|---|---|---|---| +| RolesGuard | `src/core/modules/auth/guards/roles.guard.ts` (:141, :297, :332) | 401 always | 403 | 401 | +| BetterAuthRolesGuard | `src/core/modules/better-auth/better-auth-roles.guard.ts` (:86, :146, :165) | 401 always | 403 | 401 | +| CoreTenantGuard | `src/core/modules/tenant/core-tenant.guard.ts` (:219, :261, :279, :342, :380) | **403 always** | 403 | **403** ("Authentication required" — inverse of RFC 9110) | +| service-layer rights checks | `check()` / `checkRestricted()` / `prepareInput()` | 403 (auth) / 401 (anon) — **since 11.28.0** | 403 | 401 | +| model `securityCheck()` | `src/core/modules/tenant/core-tenant-member.model.ts` (:101, :119) — the only throwing securityCheck in core | n/a | **401** (still) | 401 | + +**Why:** PR #559 (11.28.0, `AccessDeniedException`) fixed only the service-layer row. Reviews of +that PR — and of any follow-up — keep having to re-derive this table, which costs a full +cross-file audit each time. Two traps it exposes: + +1. **S_NO_ONE has three different status codes** across the layers. Three e2e tests lock the + guard's 401 in place and must be updated together with any change: + `tests/stories/better-auth-rest-security.e2e-spec.ts:738`, + `better-auth-autoregister-false.e2e-spec.ts:219`, + `better-auth-module-registration.e2e-spec.ts:334`. +2. **`.claude/rules/better-auth.md` only requires RolesGuard ↔ BetterAuthRolesGuard to stay in + sync with *each other*.** They do. The divergences are guard-layer ↔ service-layer ↔ + securityCheck, which no rule covers — so "both guards agree" is not evidence of coherence. + +Also inconsistent: email-verification denial is **403** via `BetterAuthRolesGuard` (S_VERIFIED) +and `CoreTenantGuard` ("Verification required"), but **401** via +`CoreBetterAuthController`/`Resolver` `checkEmailVerification()` (`ErrorCode.EMAIL_VERIFICATION_REQUIRED`). +The 401 is defensible at sign-in (no session yet) but is a landmine for frontends that +auto-logout on 401. + +**How to apply:** Before grading any 401/403 change, re-verify this table against current source +(line numbers drift). Judge completeness against *all five* layers — a PR that "centralizes the +401/403 decision" in one layer while leaving the others is an incomplete fix, not a done one. +Related: [[core-errorcode]] — the guards throw translatable `ErrorCode.ACCESS_DENIED`, the +service layer throws raw English strings, for the same logical denial. diff --git a/.claude/agent-memory/lt-dev-docs-reviewer/MEMORY.md b/.claude/agent-memory/lt-dev-docs-reviewer/MEMORY.md index 7500978..6b731d8 100644 --- a/.claude/agent-memory/lt-dev-docs-reviewer/MEMORY.md +++ b/.claude/agent-memory/lt-dev-docs-reviewer/MEMORY.md @@ -5,3 +5,6 @@ - [Migration-guide behavior-change count trap](migration-guide-behavior-change-count-trap.md) — derive behavior changes from better-auth.config.ts/cookies.helper.ts diffs; guide Overview counts have under-reported before - [Review committed state vs working tree](review-committed-vs-working-tree.md) — `git status` FIRST; the author's uncommitted delta can be a better revision than the commits, and untracked keystone files ship breaks - [Vendor-mode atomic file-set check](vendor-mode-atomic-file-set-check.md) — every new file under src/core/ that an existing core file imports must be an enumerated atomic set in the guide +- [FRAMEWORK-API generator blind spots](framework-api-generator-allowlist.md) — FRAMEWORK-API.md only documents allowlisted config interfaces and has NO exceptions/classes concept; new public exports are silently omitted +- [Release version artifacts](release-version-artifacts.md) — a version bump must touch package.json AND spectaql.yml; the lockstep is a git-history convention, written in no rule doc +- [Migration-guide self-description traps](migration-guide-behavior-change-count-trap.md) — never trust a guide's Overview counts or its "aligns with existing pattern X" claims; derive both from the source diff diff --git a/.claude/agent-memory/lt-dev-docs-reviewer/migration-guide-behavior-change-count-trap.md b/.claude/agent-memory/lt-dev-docs-reviewer/migration-guide-behavior-change-count-trap.md index fb2527a..54d3b7e 100644 --- a/.claude/agent-memory/lt-dev-docs-reviewer/migration-guide-behavior-change-count-trap.md +++ b/.claude/agent-memory/lt-dev-docs-reviewer/migration-guide-behavior-change-count-trap.md @@ -1,12 +1,16 @@ --- name: migration-guide-behavior-change-count-trap -description: When reviewing a nest-server migration guide, enumerate behavior changes from the better-auth.config.ts / cookies.helper.ts diff directly — the guide's Overview counts have under-reported before +description: Never trust a nest-server migration guide's own Overview counts or its "this aligns with existing pattern X" claims — derive both from the source diff and from reading every branch of X metadata: type: feedback --- -When assessing whether a `migration-guides/X-to-Y.md` is COMPLETE, do NOT trust the guide's own Overview "Bugfixes"/"Behavior Changes" counts. Derive the true list of consumer-visible behavior changes straight from the framework-source diff — especially `src/core/modules/better-auth/better-auth.config.ts` and `src/core/common/helpers/cookies.helper.ts`, which are large files where multiple independent behavior changes land in one release and are easy to miss. +When assessing whether a `migration-guides/X-to-Y.md` is COMPLETE and ACCURATE, do not trust the guide's self-description. Two traps, both confirmed by real reviews: -**Why:** In the 11.27.5→11.27.6 review, the guide documented 3 behavior changes and 4 bugfixes but the code had ≥5: it omitted the `advanced.useSecureCookies: false` pin entirely (a BetterAuth 2FA/passkey/`/token` 401 split-brain fix WITH an opt-out via `betterAuth.options.advanced.useSecureCookies`) and only mentioned the `deriveCookieDomainFromUrls()` `api.dev` bare-TLD guard as an export aside, not as a behavior change. Each of these had its own regression test (`tests/unit/better-auth-secure-cookies.spec.ts`) — so a fast completeness cross-check is: every new `tests/unit/*secure*`/behavior spec should map to a documented guide entry. +**Trap 1 — Overview counts under-report.** Derive the true list of consumer-visible behavior changes straight from the framework-source diff, especially `src/core/modules/better-auth/better-auth.config.ts` and `src/core/common/helpers/cookies.helper.ts`, where several independent behavior changes land in one release. In the 11.27.5→11.27.6 review the guide claimed 3 behavior changes / 4 bugfixes but the code had ≥5: it omitted the `advanced.useSecureCookies: false` pin entirely (a 2FA/passkey/`/token` 401 split-brain fix WITH an opt-out) and demoted the `deriveCookieDomainFromUrls()` bare-TLD guard to an export aside. Fast cross-check: every new `tests/unit/*` behavior spec should map to a documented guide entry. -**How to apply:** For each `+`-added behavior in the config/helper diffs, grep the guide for a matching keyword (e.g. `useSecureCookies`, `__Secure`, the changed function name). A behavior change present in code but absent from the guide's "Behavior changes" section AND its opt-out is a High-priority finding. Also re-check the guide's numeric counts ("Three consumer-visible shifts") against the actual count. See [[doc-surfaces-for-config-features]] for the full doc-surface set. +**Trap 2 — "aligns with existing pattern X" claims are only spot-checked.** When a guide justifies a breaking change as *"we are just aligning with what X already did"*, read EVERY branch of X, not the one the guide quotes. In the 11.27.6→11.28.0 review the guide claimed the new service-layer 401/403 split "aligns with the existing `RolesGuard` pattern" and listed RolesGuard under "What is NOT affected". True for the role-mismatch branch (`!user` → 401, user-lacks-role → 403) — but **false for the `S_NO_ONE` branch**: `roles.guard.ts` and `better-auth-roles.guard.ts` throw `UnauthorizedException` (401) for `S_NO_ONE` even when authenticated, while `core-tenant.guard.ts` throws `ForbiddenException` (403) and the newly changed `check()` now also throws 403. So the release *created* a three-way split while the guide asserted alignment. + +Related sub-trap: check whether the aligned-with pattern also carries an **ErrorCode** that the new code drops. RolesGuard throws `ForbiddenException(ErrorCode.ACCESS_DENIED)` (`LTNS_0101`, has a German translation); the new `AccessDeniedException` sites throw raw strings with no `#LTNS_xxxx:` marker, so `useLtErrorTranslation()` cannot translate them — while the guide simultaneously says "ErrorCodes — unchanged" and recommends the error-translation layer as the remedy. + +**How to apply:** For each `+`-added behavior in the diff, grep the guide for a matching keyword. A behavior change present in code but absent from the guide's "Behavior changes" section AND its opt-out is a High-priority finding. For every "already behaves like X" / "unchanged" claim, open X and enumerate its branches. See [[doc-surfaces-for-config-features]]. diff --git a/.claude/agent-memory/lt-dev-docs-reviewer/release-version-artifacts.md b/.claude/agent-memory/lt-dev-docs-reviewer/release-version-artifacts.md new file mode 100644 index 0000000..df521ff --- /dev/null +++ b/.claude/agent-memory/lt-dev-docs-reviewer/release-version-artifacts.md @@ -0,0 +1,17 @@ +--- +name: release-version-artifacts +description: A release bump must update BOTH package.json and spectaql.yml (info.version) — the lockstep is a git-history convention only, it is written in no rule doc +metadata: + type: project +--- + +Two files carry the framework version and must be bumped together in a release commit: + +1. `package.json` → `"version"` +2. `spectaql.yml` → `info.version` (feeds the published GraphQL API docs site) + +**Why:** `.claude/rules/versioning.md` "Release Process" lists only *"Update version in `package.json`"* — `spectaql.yml` is never mentioned, and neither is it in CLAUDE.md's Living-Documentation table. The lockstep exists purely as a git-history convention: 7 of the last 8 release commits before 11.28.0 (83b59e1, 912896a, e17fdd1, beda830, 48e58f3, c9a6477, 8d7a7be) bumped `spectaql.yml` in the same commit as `package.json`. Because no doc states it, it is reliably forgotten — the 11.28.0 PR shipped `package.json` at 11.28.0 while `spectaql.yml` still said 11.27.6, which would publish API docs advertising the wrong version. + +**How to apply:** On any branch that changes `package.json`'s version, immediately run +`grep -n version spectaql.yml` and compare. If they disagree, that is a High-priority finding. +`FRAMEWORK-API.md`'s version line is NOT a third artifact to bump by hand — it is regenerated by `pnpm run build` (see [[framework-api-generator-allowlist]]). There is no CHANGELOG.md in this repo, and no `migration-guides/README.md` index, so those are never part of a version bump. diff --git a/.claude/agent-memory/lt-dev-security-reviewer/MEMORY.md b/.claude/agent-memory/lt-dev-security-reviewer/MEMORY.md index 39eea57..06d640b 100644 --- a/.claude/agent-memory/lt-dev-security-reviewer/MEMORY.md +++ b/.claude/agent-memory/lt-dev-security-reviewer/MEMORY.md @@ -12,3 +12,5 @@ ## Review Methodology - [project-e2e-node-env-trap.md](project-e2e-node-env-trap.md) — e2e without NODE_ENV=e2e fabricates 5 bogus BetterAuth "Invalid credentials" failures; reproduces on base branch too, so a control-diff won't catch it +- [project-betterauth-native-cookie-forwarding.md](project-betterauth-native-cookie-forwarding.md) — BetterAuth native-handler paths forward Set-Cookie verbatim, bypass the cookie helper's Secure flag; useSecureCookies:false (11.27.6) drops Secure on 2FA/social/magic-link session cookies +- [project-exception-wire-format.md](project-exception-wire-format.md) — HttpExceptionLogFilter sends `{...exception}` (class `name` is client-visible); `extends HttpException` breaks instanceof vs native Forbidden/Unauthorized diff --git a/.claude/agent-memory/lt-dev-security-reviewer/project-exception-wire-format.md b/.claude/agent-memory/lt-dev-security-reviewer/project-exception-wire-format.md new file mode 100644 index 0000000..ae3c2bf --- /dev/null +++ b/.claude/agent-memory/lt-dev-security-reviewer/project-exception-wire-format.md @@ -0,0 +1,40 @@ +--- +name: project-exception-wire-format +description: HttpExceptionLogFilter serializes {...exception} for REST, so a custom HttpException subclass changes the wire body (name/options) and breaks instanceof against native Nest exceptions +metadata: + type: project +--- + +Two non-obvious traps when reviewing any new/changed exception class in nest-server. + +**1. The REST wire body is `{ ...exception }`, not `getResponse()`.** +`src/core/common/filters/http-exception-log.filter.ts` does +`res.status(status).json({ ...exception })`, and `src/main.ts` registers it via +`useGlobalFilters()` (nest-server-starter mirrors this). The spread yields +`{ response, status, options, message, name }` — so **`name` (the exception class +name) is client-visible on every REST error**. A new subclass silently changes the +wire format: `name: "AccessDeniedException"` instead of `"ForbiddenException"`, and +`options` disappears when the 3rd `super()` arg is omitted. GraphQL is unaffected — +it surfaces `getResponse()` under `extensions.originalError`. + +**2. `extends HttpException` is `instanceof` NEITHER `ForbiddenException` NOR +`UnauthorizedException`.** +Verified empirically. A subclass that picks its status at runtime (e.g. +`AccessDeniedException(user)` → 403/401) therefore breaks every downstream +`instanceof UnauthorizedException` / `instanceof ForbiddenException` check and every +`@Catch(ForbiddenException)` filter — including authz-denial audit logging (OWASP +A09) in consumer projects. Framework-internal catch sites that key on this: +`core-better-auth.controller.ts:456` + `:938`, `core-system-setup.service.ts:112/217`, +`core-ai-prompt.service.ts:158`. + +**Why:** found on `fix/403-for-permission-errors` (PR #559, 11.28.0). The PR's own +unit spec asserted `getResponse()` body parity with the native exceptions — which +passes — so the `instanceof` divergence went undetected, and the shipped migration +guide told consumers to "match `ForbiddenException` instead", which does not work. + +**How to apply:** when a diff adds or changes an exception class, always check +(a) `instanceof` against the native Nest exception it replaces, and (b) the +`{ ...exception }` spread shape, not just `getResponse()`. Prefer a factory returning +the **native** exceptions over a new `HttpException` subclass when the only goal is +picking between 401 and 403. See also [[project-betterauth-native-cookie-forwarding]] +for another "framework serializes something verbatim" trap in this repo. diff --git a/.claude/agent-memory/lt-dev-test-reviewer/MEMORY.md b/.claude/agent-memory/lt-dev-test-reviewer/MEMORY.md index 7256ae8..c9106bc 100644 --- a/.claude/agent-memory/lt-dev-test-reviewer/MEMORY.md +++ b/.claude/agent-memory/lt-dev-test-reviewer/MEMORY.md @@ -1,4 +1,4 @@ -- [E2E Test Isolation Model](e2e-isolation-model.md) — how the Vitest e2e suite shares one MongoDB across parallel forks and what that means for collection-wide deleteMany in tests. +- [E2E Test Isolation Model](e2e-isolation-model.md) — per-RUN unique DB but ONE DB shared across files; the `jwks` deleteMany landmine that makes "flaky 401s" a deterministic cross-file bug (pair-run to reproduce, don't blame load). - [ConfigService Singleton in Tests](configservice-singleton-in-tests.md) — per-file fork isolation makes static ConfigService safe between files; within a file mergeConfig (lodash merge) means arrays don't clear via [] — check test ORDER, not "reset". - [AI Module Test Coverage](ai-module-test-coverage.md) — coverage gaps observed in the AI module review (OAuth 2.1 stores, MCP HTTP session lifecycle, sessionStart/stop hooks, search_tools/ask_user_question execute bodies); brittle regex-on-message assertions caused by mixed raw-string vs ErrorCode throws. - [Vitest Is Blind to SWC/CJS TDZ](vitest-blind-to-swc-cjs-tdz.md) — vitest (even with unplugin-swc) can NEVER catch import-cycle TDZ crashes; only an SWC→CJS build+require does. Plus: 9 pre-existing cycles, so "zero cycles" tests are a non-starter. diff --git a/.claude/agent-memory/lt-dev-test-reviewer/e2e-isolation-model.md b/.claude/agent-memory/lt-dev-test-reviewer/e2e-isolation-model.md index 640e10e..9e1064e 100644 --- a/.claude/agent-memory/lt-dev-test-reviewer/e2e-isolation-model.md +++ b/.claude/agent-memory/lt-dev-test-reviewer/e2e-isolation-model.md @@ -1,16 +1,26 @@ --- name: e2e-isolation-model -description: How the nest-server Vitest e2e suite shares one MongoDB across parallel forks — implications for collection-wide deleteMany() in test files +description: How the nest-server Vitest e2e suite shares ONE database across parallel forks within a run — and the jwks landmine that makes "flaky 401s" a deterministic cross-file bug metadata: type: project --- -The e2e suite (`vitest-e2e.config.ts`) runs with `pool: 'forks'`, `fileParallelism: true`, `isolate: true`. Despite `isolate: true` (which isolates module state per fork, NOT the database), every test file connects to the SAME MongoDB database (one of `nest-server-{ci,e2e,local,dev}` from `src/config.env.ts`, selected by `NODE_ENV`). +The e2e suite (`vitest-e2e.config.ts`) runs `pool: 'forks'`, `fileParallelism: true`, `isolate: true`. -**Why:** There is no per-fork DB namespacing. The DB name is fixed per environment, not per worker. +**DB model (corrected 2026-07-13 — earlier note here was stale):** `tests/global-setup.ts` gives every RUN its own database (`-run--p`), so two concurrent runs no longer clobber each other. But **within a single run, ALL test files still share that one database** — `isolate: true` isolates module state per fork, never the DB. So cross-RUN interference is fixed; cross-FILE interference is very much alive. -**How to apply when reviewing test isolation:** -- A test file that does `db.collection('X').deleteMany({})` is only safe if NO OTHER test file touches collection `X`. Verify with `grep -rln "" tests/`. -- Within a single test file, `it()` blocks run sequentially (Vitest default), so a mid-file `deleteMany` is safe against that file's own later tests as long as they recreate their fixtures. -- The shared `users` collection IS touched by many files — tests that depend on user counts MUST scope to isolated users (unique `ObjectId`/`@test.com` emails), never assume a clean `users` collection. -- Cleanup filters keyed on `@test.com` emails are the suite convention; collection-wide `deleteMany({})` is acceptable ONLY for collections owned exclusively by one file (e.g. the AI module's `ai*` collections). +**Escape hatch:** `deriveTestDbUri(suffix)` (`tests/db-lifecycle.reporter.ts:44`) gives a file its OWN database. Already used by `error-code-scenarios`, `mongoose-plugins`, `multi-tenancy`, `tenant-guard`. Any file that mutates GLOBAL state (not just its own rows) belongs in this list. + +## The jwks landmine (root cause of the recurring "flaky 401s") + +`tests/stories/better-auth-integration.story.test.ts:962` runs `db.collection('jwks').deleteMany({})` in a `beforeAll` — an unscoped wipe of the **BetterAuth JWT signing keyset** — and that file does NOT use `deriveTestDbUri`. 19 e2e files boot BetterAuth-JWT apps against the same shared run DB. + +Victim pattern: `tests/ai.e2e-spec.ts` mints Bearer tokens ONCE in `beforeAll` (:156-157) and reuses them across ~40 later tests. When the jwks wipe lands mid-file, BetterAuth regenerates the keyset and every previously-signed JWT becomes unverifiable → `expected 200 "OK", got 401 "Unauthorized"` on ~10 tests at once. `retry: 5` cannot save it — the token is *durably* dead, and every retry replays the same stale `beforeAll` token. + +**Why it masquerades as an environment flake:** it is 100% deterministic *given the fork scheduling that pairs the two files*, and invisible otherwise. Verified 2026-07-13: 6/6 green across isolated runs, a full 51-file suite, and even two deliberately-concurrent full suites — yet `vitest run tests/ai.e2e-spec.ts tests/stories/better-auth-integration.story.test.ts` fails **4/4 on the branch and 2/2 on develop**. Do NOT accept "flaky under load / CPU contention" for this signature; that hypothesis is disproven. + +**How to apply:** +- Pair-run the suspect file with the jwks-wiping file to reproduce a "flaky 401" on demand — that is the fastest discriminator, far better than re-running in isolation (which always passes). +- Treat unscoped `deleteMany({})` on shared auth collections (`jwks`, `session`, `account`, `users`) as a cross-file bug, not a local cleanup. Other files' cleanups here are correctly scoped (`$in` / email regex) — jwks is the outlier. +- Tokens minted once in `beforeAll` and reused across a long file are the vulnerable shape; re-minting per test (`beforeEach`) is the hardening. +- `tests/ai.e2e-spec.ts:90` claims a JWT-fallback fix "eliminates the pre-existing flaky 401s in this file" — it made token *acquisition* deterministic but does nothing about *invalidation* by a foreign jwks wipe. The comment is misleading; don't trust it as evidence the file is fixed. diff --git a/.gitignore b/.gitignore index f202ecd..ae31ec1 100644 --- a/.gitignore +++ b/.gitignore @@ -112,3 +112,4 @@ permissions.md # Throwaway SWC build used by check:swc-tdz (kept only on failure, for inspecting the emit) dist-swc-tdz +.lt-dev/