Conversation
….28.0 review (#562) Two unrelated bits of housekeeping that were sitting in the working tree. **.gitignore** — `.lt-dev/` is created per worktree by the `lt dev` CLI and has no business being tracked. **.claude/agent-memory/** — written by the review agents during the 11.28.0 (401/403) review. These are the findings worth carrying forward, not the review prose: - `security-reviewer/project-exception-wire-format.md` — `HttpExceptionLogFilter` serializes `{ ...exception }` for REST, so a custom `HttpException` subclass silently changes the wire body (`name`, `options`) AND breaks `instanceof` against the native Nest exceptions. This is the finding that would otherwise have shipped a silent breaking change in #559. - `backend-reviewer/project_401-403-denial-surface.md` — the framework has five permission-denial layers, and they used to contradict each other. A change to one is a change to none. - `docs-reviewer/release-version-artifacts.md` — `spectaql.yml` carries the version too and is derived from `package.json`, but only by `pnpm run docs`, never by `build`. It is therefore reliably forgotten in release commits. - `test-reviewer/e2e-isolation-model.md` — e2e files share ONE database per run and execute in parallel forks, so any unscoped write to global state (a `jwks` wipe, a config mutation) strands the other files. Pair-run to reproduce; an isolated re-run proves nothing. Kept separate from #559 and #561 on purpose: neither is part of a code change.
… was hiding (#561) `tsc --noEmit -p tsconfig.json` reported 8 errors on develop. Nothing in the check chain runs it (`nest build` uses tsconfig.build.json, which already excludes the two problem files), so the errors only surfaced as red squiggles in the editor -- and were ignored long enough that a genuine bug hid among them. The real bug: vite.config.ts sets `tsCompiler: 'esbuild'`, but vite-plugin-node@8 narrowed SupportedTSCompiler to 'vite' | 'swc'. The config would not work if anyone ran it. Switched to 'swc', which the project already uses elsewhere (`nest start -b swc`). The rest were configuration mismatches, each fixed at the root rather than silenced: - scripts/generate-framework-api.ts shimmed __dirname from `import.meta.url`, which cannot typecheck under `module: commonjs`. The package IS CommonJS (no "type": "module"), so tsx runs the script as CJS and __dirname is native -- the shim was never needed. Removed. (A separate ESM tsconfig for scripts/ is not an option: scripts/init-server.ts imports src/, which would drag the whole `import x = require(...)` tree into ESM mode.) - vite.config.ts and the migrate template are excluded from the root tsconfig, mirroring tsconfig.build.json. vite@8 is ESM-only and cannot be resolved under `moduleResolution: node`, which `module: commonjs` forces; the template deliberately imports '@lenne.tech/nest-server' because it is copied into consumer projects. - performance-caches.spec.ts marked a method `override` against a base class whose compile-time type is `any` (it comes from a dynamic import), which TS rejects outright (TS4113). - unified-field-whitelist.spec.ts redeclared `email` without an initializer (TS2612). Added `= undefined`, matching every Input class in the codebase. Verification: `tsc --noEmit` now reports 0 errors; the generator still runs; full check green.
…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>
…t step Two gaps in the 11.27.7→11.28.0 guide, found while reviewing it against the merged 11.28.0 state: - "work through the four breaking changes" → five (the S_SELF/S_CREATOR ownership change is BC5). - The numbered Detailed Migration Steps covered BC1–4 but not BC5 — so a consumer working the steps top-to-bottom would skip the one change with security impact (input-field ownership now enforced). Added it as Step 5 (audit S_SELF/S_CREATOR on input types), Verify becomes Step 6. Docs only; no code change.
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
2 participants
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.
Unified 401/403 permission semantics and hardened S_SELF/S_CREATOR ownership checks. Permission errors now follow RFC 9110 consistently across all five permission layers, so authenticated users no longer get logged out by a mere permission error.
Breaking changes (see migration guide):
New: accessDeniedException(user, message?) — an exported factory that derives 401/403 from the requester's auth state and returns the native Nest exceptions, so instanceof and @catch(...) filters keep working.
Effort: pnpm update, then update any test/handler that asserts 401 on an authenticated request or matches permission-error message text, and run the ownership audit (guide Step 5).