diff --git a/.claude/rules/role-system.md b/.claude/rules/role-system.md index e83ee14c..5df5119e 100644 --- a/.claude/rules/role-system.md +++ b/.claude/rules/role-system.md @@ -21,6 +21,57 @@ System roles are used for **runtime checks only** and must **NEVER** be stored i | `S_EVERYONE` | Public access | Always true | | `S_NO_ONE` | Locked access | Always false | +### `object` means the PERSISTED object — never the request payload + +For `S_SELF` and `S_CREATOR`, "object" is the record loaded from the database +(`serviceOptions.dbObject`), **not** the input DTO. This distinction is security-critical. + +On the **input** path the DTO is fully attacker-controlled. Deciding `S_SELF` from it would let an +authenticated attacker unlock an owner-restricted field on someone else's record just by putting +their own id in the body — the service writes to the target it was called with, not to the id in the +payload: + +``` +PATCH /users/ { "id": "", "iban": "DE...attacker" } +``` + +On the **output** path there is no attacker-controlled input: the object being checked *is* the +persisted record (and a list is checked per item), so it is the correct comparison target there. + +Both `check()` (`input.helper.ts`) and `checkRestricted()` (`restricted.decorator.ts`) implement this. +If you write your own rights check, compare against `dbObject` on input — never against the DTO. + +### ⚠️ `S_CREATOR` is the CREATOR of the record — which is often an admin, not the user + +`createdBy` is set by the audit plugin to whoever **created the record**. On a self-signup that is the +user themselves. But in an **invite or admin-provisioning flow it is the inviting admin** — and it +stays that way forever. + +So `@Restricted(S_CREATOR)` on a **User input field** does not mean "the user may edit their own +field". It means **"whoever created this account may edit it"** — granting the inviter permanent +write access to the invited user's record. + +That is almost never the intent, and it is dangerous on exactly the fields people reach for it on: + +```typescript +// DANGEROUS on an invite-based system: the inviting workspace admin IS the creator, so this +// lets them rewrite the invited member's email — and then trigger a password reset. +@UnifiedField({ roles: [RoleEnum.ADMIN, RoleEnum.S_CREATOR] }) +email?: string; + +// SAFE: only a system admin, and the user changes their own email through the verification-gated +// BetterAuth changeEmail flow, never through a generic update DTO. +@UnifiedField({ roles: [RoleEnum.ADMIN] }) +email?: string; +``` + +**Upgrade note:** before v11.28.x, `S_SELF`/`S_CREATOR` on an *input* field never actually fired — +the check read the claim off the DTO, and `MapAndValidatePipe` strips `id`/`createdBy` from payloads +(they are not `@UnifiedField`s). Such fields were therefore effectively **admin-only-or-denied**. +Now that the check reads the persisted object, they start working — and a field that looked +owner-restricted may suddenly become writable by an admin who provisioned the record. **Audit every +`S_SELF`/`S_CREATOR` on an input type before upgrading.** + ## Critical Rule ```typescript @@ -79,6 +130,45 @@ The role system is evaluated in: - `CheckResponseInterceptor` - Filters fields based on `@Restricted()` decorators - `CheckSecurityInterceptor` - Processes `securityCheck()` methods +## Status Codes: 401 vs 403 (v11.28.0+) + +**All five permission layers answer with the same policy.** Getting this wrong has a concrete +consequence: SPA auth layers treat 401 as "session expired" and log the user out — so a mere +permission error returned as 401 kicks a logged-in user out of the whole app. + +| Situation | Status | Message | +|-----------|--------|---------| +| Requester is **not authenticated** | **401** | `ErrorCode.UNAUTHORIZED` | +| Requester **is authenticated** but lacks a right | **403** | `ErrorCode.ACCESS_DENIED` | +| `S_NO_ONE` (locked for everyone) | **403 always**, even for anonymous requesters | `ErrorCode.ACCESS_DENIED` | + +`S_NO_ONE` is 403 even without a session because authenticating can *never* unlock it — a 401 would +tell the client to log in and retry, which is a lie. + +Exception: `ErrorCode.EMAIL_VERIFICATION_REQUIRED` is a legitimate **401** (thrown at sign-in, where +no session exists yet). Frontends must branch on the ErrorCode, not on the status alone, so this one +does not trigger the logout flow. + +### Never hand-roll the decision + +```typescript +import { accessDeniedException } from '@lenne.tech/nest-server'; + +// CORRECT — one policy, native exceptions (instanceof / @Catch keep working) +throw accessDeniedException(currentUser); +throw accessDeniedException(currentUser, 'Custom message'); + +// WRONG — drifts from the framework policy and mishandles falsy-but-present ids (0, '') +throw currentUser?.id ? new ForbiddenException() : new UnauthorizedException(); + +// WRONG — a permission error must never be a 401 for an authenticated user +throw new UnauthorizedException('Missing rights'); +``` + +**This applies to `securityCheck()` in your models too.** `CoreTenantMemberModel` is the reference +implementation. A model that throws `UnauthorizedException` from `securityCheck()` reintroduces the +auto-logout bug in your own project. + ## @Roles vs @UseGuards **IMPORTANT: `@Roles()` already handles JWT authentication internally.** diff --git a/.claude/rules/versioning.md b/.claude/rules/versioning.md index 2dd7053e..42de02ac 100644 --- a/.claude/rules/versioning.md +++ b/.claude/rules/versioning.md @@ -23,12 +23,27 @@ ## Release Process 1. Make changes and ensure all tests pass (`pnpm test`) -2. Update version in `package.json` -3. Build the package (`pnpm run build`) +2. **Bump the version in BOTH version-carrying files — they must never drift:** + - `package.json` → `version` + - `spectaql.yml` → `info.version` (feeds the published GraphQL API docs) + + `spectaql.yml` is derived from `package.json` by `extras/update-spectaql-version.mjs`, but that + script only runs via `pnpm run docs` — it is **not** part of `pnpm run build`. A release commit + that bumps only `package.json` therefore ships API docs advertising the previous version. Run + `node extras/update-spectaql-version.mjs` (or edit the file) and commit both together. +3. Build the package (`pnpm run build`) — this also regenerates `FRAMEWORK-API.md` with the new version 4. Publish to npm 5. Update and test in [nest-server-starter](https://github.com/lenneTech/nest-server-starter) 6. Commit changes to starter with migration notes +### Version Consistency Check + +```bash +# Both must print the same version +grep -m1 '"version"' package.json +grep -m1 '^ version:' spectaql.yml +``` + ## Package Distribution - **NPM Package**: `@lenne.tech/nest-server` diff --git a/FRAMEWORK-API.md b/FRAMEWORK-API.md index 9898af7f..e102cae9 100644 --- a/FRAMEWORK-API.md +++ b/FRAMEWORK-API.md @@ -1,6 +1,6 @@ # @lenne.tech/nest-server — Framework API Reference -> Auto-generated from source code on 2026-07-15 (v11.27.7) +> Auto-generated from source code on 2026-07-15 (v11.28.0) > File: `FRAMEWORK-API.md` — compact, machine-readable API surface for Claude Code ## CoreModule.forRoot() @@ -283,6 +283,31 @@ Generic: `CrudService` | `tus` | README, CHECKLIST | `src/core/modules/tus/` | | `user` | — | `src/core/modules/user/` | +## Errors & Status Codes + +One 401/403 policy across all permission layers (role guards, tenant guard, `check()`, +`checkRestricted()`, model `securityCheck()`): + +| Situation | Status | Message | +|-----------|--------|---------| +| Requester is **not authenticated** | **401** | `ErrorCode.UNAUTHORIZED` | +| Requester **is authenticated** but lacks a right | **403** | `ErrorCode.ACCESS_DENIED` | +| `S_NO_ONE` (locked for everyone, even admins) | **403 always** | `ErrorCode.ACCESS_DENIED` | + +Never hand-roll the decision — use `accessDeniedException(user)`. It returns the **native** +`ForbiddenException` / `UnauthorizedException`, so `instanceof` checks and `@Catch(...)` filters +in consuming projects keep working. + +### Exported error helpers + +| Export | Purpose | Path | +|--------|---------|------| +| `ExpiredRefreshTokenException` | Exception for expired refresh token | `src/core/modules/auth/exceptions/expired-refresh-token.exception.ts` | +| `ExpiredTokenException` | Exception for expired token | `src/core/modules/auth/exceptions/expired-token.exception.ts` | +| `InvalidTokenException` | Exception for invalid token | `src/core/modules/auth/exceptions/invalid-token.exception.ts` | +| `LegacyAuthDisabledException` | Exception thrown when Legacy Auth endpoints are accessed but disabled | `src/core/modules/auth/exceptions/legacy-auth-disabled.exception.ts` | +| `accessDeniedException()` | Creates the access error that matches the requester's auth state (RFC 9110, mirrors RolesGuard): | `src/core/common/exceptions/access-denied.exception.ts` | + ## Key Source Files | File | Purpose | @@ -293,6 +318,7 @@ Generic: `CrudService` | `src/core/common/services/crud.service.ts` | CrudService base class | | `src/core/common/services/config.service.ts` | ConfigService (global) | | `src/core/common/decorators/` | @Restricted, @Roles, @CurrentUser, @UnifiedField | +| `src/core/common/exceptions/` | accessDeniedException — the 401/403 policy | | `src/core/common/interceptors/` | CheckResponse, CheckSecurity, ResponseModel | | `docs/REQUEST-LIFECYCLE.md` | Complete request lifecycle | | `.claude/rules/` | Detailed rules for architecture, security, testing | diff --git a/docs/REQUEST-LIFECYCLE.md b/docs/REQUEST-LIFECYCLE.md index 607a6611..628c564c 100644 --- a/docs/REQUEST-LIFECYCLE.md +++ b/docs/REQUEST-LIFECYCLE.md @@ -949,6 +949,41 @@ The `process()` method in `ModuleService` is the **primary** way to handle CRUD +---------------------------------------------------------------+ ``` +### Status Codes: 401 vs 403 (v11.28.0+) + +One policy across **all five** permission layers — the role guards, the tenant guard, `check()` / +`checkRights`, `checkRestricted()` (object and field level), and a model's `securityCheck()`: + +| Situation | Status | Thrown by | +|-----------|--------|-----------| +| Requester is **not authenticated** | **401** `ErrorCode.UNAUTHORIZED` | guards, `accessDeniedException(undefined)` | +| Requester **is authenticated** but lacks a right | **403** `ErrorCode.ACCESS_DENIED` | guards, `accessDeniedException(user)` | +| Resource is locked via `S_NO_ONE` | **403**, always — even for anonymous requesters | guards, `check()` | + +`S_NO_ONE` is 403 for everyone because authenticating can never unlock it; a 401 would tell the +client to retry after logging in, which is a lie. + +**Why this matters:** SPA auth layers commonly treat 401 as "session expired" and clear the session +(the `@lenne.tech/nuxt-extensions` auth interceptor patches `$fetch`/`fetch` globally and does exactly +this). A permission error answered with 401 therefore logs the user out of the whole app. With this +policy a frontend may treat 401 as "session invalid" — with one exception: +`ErrorCode.EMAIL_VERIFICATION_REQUIRED` is a legitimate 401 (no session exists yet at sign-in) that +must **not** trigger a logout. Branch on the ErrorCode, not on the status alone. + +**Writing new denial code:** use the exported factory rather than hand-rolling the decision. It +returns the **native** `ForbiddenException` / `UnauthorizedException`, so `instanceof` checks and +`@Catch(...)` filters in consuming projects keep working: + +```typescript +import { accessDeniedException } from '@lenne.tech/nest-server'; + +// In a service, a custom guard, or a model's securityCheck(): +throw accessDeniedException(currentUser); +``` + +See `src/core/common/exceptions/access-denied.exception.ts` and +`migration-guides/11.27.7-to-11.28.0.md`. + ### Depth-Based Optimization (v11.23.0+) When `process()` is called from within another `process()` call (service cascades like A.create → B.create → C.create), steps 4–6 are **conditionally skipped** on inner calls to avoid redundant work: @@ -1095,6 +1130,10 @@ Controls who can access a resolver/controller method. Evaluated by the RolesGuar Controls who can see or modify specific properties. Evaluated by `CheckResponseInterceptor` (output) and `checkRights()` (input). +On **output** a denied field is silently removed (no exception). On **input** the request is +rejected: **403** for an authenticated requester, **401** for an anonymous one, and **403 always** +for `S_NO_ONE` — see [Status Codes: 401 vs 403](#status-codes-401-vs-403-v11280) above. + ```typescript export class User extends CorePersistenceModel { // Only admins or the user themselves can see the email diff --git a/migration-guides/11.27.7-to-11.28.0.md b/migration-guides/11.27.7-to-11.28.0.md new file mode 100644 index 00000000..66d230c7 --- /dev/null +++ b/migration-guides/11.27.7-to-11.28.0.md @@ -0,0 +1,331 @@ +# Migration Guide: 11.27.7 → 11.28.0 + +> Coming from 11.27.6 or earlier? Read [11.27.6 → 11.27.7](./11.27.6-to-11.27.7.md) first — it is a +> separate startup-crash + cookie-security bugfix release with no overlap with the changes here. + +## Overview + +| Category | Details | +|----------|---------| +| **Breaking Changes** | **(1)** Permission errors for **authenticated** users now return **403 Forbidden** instead of 401 Unauthorized. **(2)** `@Roles(S_NO_ONE)` / `@Restricted(S_NO_ONE)` now return **403 for every requester**, including unauthenticated ones (was 401). **(3)** The **error messages** of these denials changed from raw English strings to the translatable `ErrorCode.ACCESS_DENIED` / `ErrorCode.UNAUTHORIZED`. **(4)** `CoreTenantGuard` now returns **401** (was 403) when a request has no authenticated user. **(5)** `@Restricted(S_SELF)` / `@Restricted(S_CREATOR)` on **input** fields now actually enforce ownership (they were silently inert before) — a field that looked owner-restricted may become writable, so **audit them before upgrading**. | +| **New Features** | `accessDeniedException(user, message?)` — an exported factory that derives 401 or 403 from the requester's auth state. Use it in your own services, models and guards instead of hand-rolling the decision. | +| **Bugfixes** | **(a)** Status-code semantics per RFC 9110 across **all five** permission layers (role guards, tenant guard, `check()`, `checkRestricted()`, model `securityCheck()`), which previously contradicted each other. **(b)** `checkRestricted()` decided `S_SELF`/`S_CREATOR` ownership from the request **payload** instead of the persisted object — an authenticated attacker could unlock an owner-restricted input field on someone else's record. Now aligned with `check()`, which always read the persisted object. | +| **Migration Effort** | ~5–20 minutes. `pnpm update`, then **(a)** update any assertion or handler that matches on **status 401** for an authenticated permission error or on the **exact wording** of a permission-error message, and **(b)** audit every `@Restricted(S_SELF)` / `@Restricted(S_CREATOR)` on an **input** type (Breaking Change 5). | + +### Why + +- **RFC 9110:** 401 means "authenticate yourself", 403 means "you are authenticated, but not + allowed". A permission error answered with 401 tells clients to re-authenticate, which cannot fix + the problem. +- **Frontend auto-logout:** SPA auth layers commonly treat 401 as "session expired" and clear the + session — the `@lenne.tech/nuxt-extensions` auth interceptor patches `$fetch`/`fetch` globally and + does exactly this. With the old behavior, an authenticated user who merely lacked a right was + logged out of the whole app. +- **Debuggability:** 401 logs no longer mix real authentication failures with permission failures. + +--- + +## Quick Migration + +```bash +pnpm add @lenne.tech/nest-server@11.28.0 +pnpm run build && pnpm test +``` + +Then work through the four breaking changes below. If your project never asserts 401 on an +authenticated request and never matches on permission-error message text, there is nothing to do. + +--- + +## Breaking Change 1: 403 instead of 401 for authenticated permission errors + +| Path | Before | After (authenticated) | After (unauthenticated) | +|------|--------|-----------------------|-------------------------| +| `check()` / `checkRights` (`input.helper.ts`) | 401 | **403** | 401 | +| `checkRestricted()`, object level (`restricted.decorator.ts`) | 401 | **403** | 401 | +| `checkRestricted()`, field level (`restricted.decorator.ts`) | 401 | **403** | 401 | +| Roles processing in `prepareInput` (`service.helper.ts`) | 401 | **403** | 401 | +| `CoreTenantMemberModel.securityCheck()` (`core-tenant-member.model.ts`) | 401 | **403** | 401 | + +The decision key is `user.id`: a requester whose user object carries an id is authenticated and gets +403; everyone else gets 401. A **falsy but present** id (`0`, `''`) counts as authenticated — relevant +if your project uses numeric user ids. + +**Before:** +```typescript +// e2e: an authenticated customer patches an admin-only field +await testHelper.rest(`/users/${customer.id}`, { + cookies: customer.token, + method: 'PATCH', + payload: { customerNumber: 'B-HACKED' }, + statusCode: 401, // ← old: permission error surfaced as 401 +}); +``` + +**After:** +```typescript +await testHelper.rest(`/users/${customer.id}`, { + cookies: customer.token, + method: 'PATCH', + payload: { customerNumber: 'B-HACKED' }, + statusCode: 403, // ← new: authenticated, but lacking rights +}); +``` + +## Breaking Change 2: `S_NO_ONE` is now 403 for everyone + +`S_NO_ONE` marks a resource as locked for **all** requesters, administrators included. +Authenticating can therefore never grant access, which makes 401 ("authenticate and retry") a lie +even for an anonymous requester. All four paths now agree on **403**: + +| Path | Before | After | +|------|--------|-------| +| `RolesGuard` (`@Roles(S_NO_ONE)`) | 401 always | **403 always** | +| `BetterAuthRolesGuard` (`@Roles(S_NO_ONE)`) | 401 always | **403 always** | +| `CoreTenantGuard` (`@Roles(S_NO_ONE)`) | 403 always | 403 always (unchanged) | +| `check()` (`@Restricted(S_NO_ONE)`, e.g. on `password`) | 401 always | **403 always** | + +Update any test asserting `statusCode: 401` on a locked endpoint or a `S_NO_ONE`-restricted field. + +## Breaking Change 3: error messages are now translatable `ErrorCode`s + +The paths above previously returned raw English strings (`'Missing rights'`, `'No access'`, +`'The current user has no access rights for of '`, +`'Current user not allowed setting roles: '`). They now return the same `ErrorCode`s the role +guards have always thrown: + +| Requester | Message | +|-----------|---------| +| authenticated | `ErrorCode.ACCESS_DENIED` → `#LTNS_0101: Access denied - Insufficient permissions` | +| unauthenticated | `ErrorCode.UNAUTHORIZED` → `#LTNS_0100: Unauthorized - User is not logged in` | + +Two reasons: + +1. **They are translatable now.** The frontend error-translation layer keys off the `#LTNS_xxxx:` + marker. The old raw strings carried no marker, so users saw untranslated English — while the + *same* 403 coming from a role guard was translated. The framework no longer emits two + incompatible 403 formats. +2. **They no longer leak internals.** The old field-level message embedded the Input/Model class name + and the property name in the HTTP response. That detail is still available — it goes to the debug + log (`config.debug`) instead of to the client. + +**Migration:** replace message assertions with `ErrorCode` comparisons. + +```typescript +// Before +expect(res.errors[0].message).toEqual('The current user has no access rights for roles of UserInput'); + +// After +import { ErrorCode } from '@lenne.tech/nest-server'; +expect(res.errors[0].message).toEqual(ErrorCode.ACCESS_DENIED); +``` + +If you relied on the class/field detail while debugging, enable `config.debug` and read the server log. + +## Breaking Change 4: `CoreTenantGuard` returns 401 when unauthenticated + +`CoreTenantGuard` previously threw `ForbiddenException('Authentication required')` — a **403** for a +request with **no user at all**. That inverts the policy above: re-authenticating *is* the remedy +here, so the client must be told. All five sites now throw **401** (`ErrorCode.UNAUTHORIZED`). + +This only surfaces when `CoreTenantGuard` runs without a role guard ahead of it; in the standard +chain `RolesGuard` / `BetterAuthRolesGuard` already answered `!user` with 401. + +## Breaking Change 5: `S_SELF` / `S_CREATOR` on input fields now actually enforce ownership + +This is the one that needs a real audit before you upgrade. + +`checkRestricted()` used to decide `S_SELF` / `S_CREATOR` from the **request payload** (`data`) rather +than from the persisted object. On the input path `data` is the caller-supplied DTO, so an +authenticated attacker could unlock an owner-restricted field on **someone else's** record just by +asserting ownership in the body — the service applies the input to the target it was called with, not +to the ids in the payload: + +``` +PATCH /users/ { "id": "", "createdBy": "", "": ... } +``` + +Because `MapAndValidatePipe` strips `id`/`createdBy` from payloads (they are not `@UnifiedField`s), +the branch usually could not fire, and such fields were **effectively admin-only-or-denied**. The fix +reads ownership from `serviceOptions.dbObject` (as `check()` always did) — so `S_SELF`/`S_CREATOR` on +an input field **start working**. A field that *looked* owner-restricted may suddenly become writable. + +### The audit + +Search for ownership roles on input types: + +```bash +grep -rn "S_SELF\|S_CREATOR" src/server/**/inputs/ +``` + +For each hit, decide what the field really means: + +| You want… | Use | +|-----------|-----| +| "the user may edit their own record's field" | `S_SELF` — correct, and now enforced | +| "only an admin" (the common real case for `email`, `status`, `roles`) | `RoleEnum.ADMIN` — remove `S_CREATOR` | +| "the field is open to any logged-in user" | `S_USER` | + +**Two traps:** + +1. **`S_CREATOR` is not "the user themselves".** `createdBy` is stamped by the audit plugin onto + whoever **created the record**. On a self-signup that is the user; in an **invite / admin-provisioning + flow it is the inviting admin, permanently**. `@Restricted(S_CREATOR)` on a user input therefore + grants the *inviter* write access to the invited user's fields — rarely the intent, and dangerous + on `email` (→ password reset). +2. **A broader class-level role hides the field-level one.** `@Restricted(S_USER)` on the class + OR-merges with a field-level `S_SELF` (`mergeRoles` defaults to true), so `S_USER` alone already + grants the field — it was never owner-gated, and this change does not alter it. Such fields are + safe but misleading; consider dropping the redundant `S_SELF`. + +### Not affected + +- `check()` (`input.helper.ts`) already read from `dbObject` — no behavior change there. +- **Output** filtering is unchanged: the object being checked *is* the persisted record. +- `create()` has no `dbObject`, so ownership cannot be established there — `S_SELF`/`S_CREATOR` deny, + exactly as `check()` already did. (A normal create DTO carries neither `id` nor `createdBy`, so this + matches the old behavior for honest callers.) + +--- + +## What is NOT affected + +- **All "requires auth" cases** (no token/session) — still 401. Only the `S_NO_ONE` case changed + (Breaking Change 2), because there authentication can never help. +- **Token errors** (`Invalid token`, `Token expired`, refresh-token flows) — still 401. +- **Sign-in with wrong credentials** — still 401. +- **Email verification at sign-in** (`ErrorCode.EMAIL_VERIFICATION_REQUIRED`) — still **401**, and + correctly so: the requester has no session yet, so they are not authenticated. Note for frontends: + this is a 401 that must **not** trigger the auto-logout flow — branch on the ErrorCode, not on the + status alone. +- **`instanceof` checks and `@Catch(...)` filters** — see below. This is deliberate. + +## `instanceof` and exception filters keep working + +The framework throws the **native** `ForbiddenException` / `UnauthorizedException`, never a custom +subclass. Existing filters keep firing, and the REST error body (including the `name` field that +`HttpExceptionLogFilter` emits) is unchanged: + +```typescript +@Catch(ForbiddenException) // fires for authenticated permission errors +@Catch(UnauthorizedException) // fires for unauthenticated ones +``` + +A `@Catch(UnauthorizedException)` filter that used to catch an authenticated permission error will no +longer see it — it is a `ForbiddenException` now. That is Breaking Change 1, and the fix is to handle +`ForbiddenException` as well. No filter silently stops matching because of a changed exception class. + +### Use the factory in your own code + +```typescript +import { accessDeniedException } from '@lenne.tech/nest-server'; + +// In a service, a model's securityCheck(), or a custom guard: +throw accessDeniedException(currentUser); // 403 if authenticated, else 401 +throw accessDeniedException(currentUser, 'Custom message'); // same decision, custom message +``` + +Prefer this over hand-rolling `user?.id ? new ForbiddenException() : new UnauthorizedException()` — +it keeps the codebase on one policy and handles falsy-but-present ids correctly. + +--- + +## Detailed Migration Steps + +### Step 1: Update the package + +```bash +pnpm add @lenne.tech/nest-server@11.28.0 +``` + +### Step 2: Find status-code expectations to update + +The candidates are assertions that send a token/cookie **and** expect 401: + +```bash +# Backend tests +grep -rn "statusCode: 401" tests/ +grep -rn "toEqual(401)\|toBe(401)" tests/ + +# Locked endpoints / fields (S_NO_ONE) — every one of these is 403 now +grep -rn "S_NO_ONE" src/ tests/ +``` + +### Step 3: Find message assertions to update + +```bash +grep -rn "Missing rights\|No access\|no access rights for\|not allowed setting roles" src/ tests/ +``` + +Replace them with `ErrorCode.ACCESS_DENIED` / `ErrorCode.UNAUTHORIZED`. + +### Step 4: Find frontend 401 handlers + +The auto-logout interceptor usually does **not** live in your own `src/`/`app/` — in lt projects it +ships with `@lenne.tech/nuxt-extensions` (`node_modules/`). Check your own branches too: + +```bash +grep -rn "=== 401\|status === 401\|statusCode === 401\|case 401\|\[401" src/ app/ +grep -rn "onResponseError\|onError" app/plugins/ app/composables/ +``` + +A 403 must show an error message; only a 401 may clear the session. Remember that +`EMAIL_VERIFICATION_REQUIRED` is a 401 that must **not** log the user out — branch on the ErrorCode. + +For GraphQL clients the status sits at `errors[0].extensions.originalError.statusCode`, and +`originalError.error` is `"Forbidden"` instead of `"Unauthorized"` for these cases. + +### Step 5: Verify + +```bash +pnpm run build && pnpm test +``` + +--- + +## Compatibility Notes + +- Consumers of `@lenne.tech/nuxt-extensions`: version ≥ 1.8.4 additionally hardens the client auth + interceptor to verify the session before logging out on 401, so even backends still returning 401 + for permission errors no longer cause wrongful logouts. Both changes are independent and + complementary. +- Projects with a **vendored core** adopt this change via the regular core sync + (`/lt-dev:backend:update-nest-server-core`). +- **Custom models that throw from `securityCheck()`** should switch to `accessDeniedException(user)`. + Otherwise they keep returning 401 to authenticated users and reintroduce the auto-logout bug in + your own code. `CoreTenantMemberModel` is the reference implementation. + +--- + +## Troubleshooting + +### A test now fails with "expected 401, received 403" + +The request is authenticated and the user lacks rights — or the resource is `S_NO_ONE`-locked. 403 is +the new, correct status. Update the assertion (Step 2). + +### A test now fails on the error message + +Permission errors return `ErrorCode.ACCESS_DENIED` / `ErrorCode.UNAUTHORIZED` instead of raw English +(Breaking Change 3). Compare against the `ErrorCode` constant (Step 3). + +### My frontend no longer logs users out on permission errors + +That is the intended behavior: a permission error does not invalidate the session. Show an error +message instead — the new messages carry the `#LTNS_xxxx:` marker, so `useLtErrorTranslation()` +resolves them. Real session expiry still yields 401 and still triggers the logout flow. + +### I lost the "no access rights for `` of ``" detail + +It is written to the debug log now rather than returned to the client (it exposed internal class and +property names in the HTTP response). Enable `config.debug` to see it server-side. + +--- + +## References + +- [RFC 9110 §15.5.2 (401 Unauthorized) / §15.5.4 (403 Forbidden)](https://www.rfc-editor.org/rfc/rfc9110) +- `src/core/common/exceptions/access-denied.exception.ts` — the factory and the policy it encodes +- `src/core/modules/auth/guards/roles.guard.ts` — the 401/403 pattern the service layer now mirrors +- `.claude/rules/role-system.md` — role system and status-code rules +- [nest-server-starter](https://github.com/lenneTech/nest-server-starter) — reference implementation diff --git a/package.json b/package.json index 9554dd06..4b11791a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@lenne.tech/nest-server", - "version": "11.27.7", + "version": "11.28.0", "description": "Modern, fast, powerful Node.js web framework in TypeScript based on Nest with a GraphQL API and a connection to MongoDB (or other databases).", "keywords": [ "node", diff --git a/scripts/generate-framework-api.ts b/scripts/generate-framework-api.ts index 7e94b112..4c04413d 100644 --- a/scripts/generate-framework-api.ts +++ b/scripts/generate-framework-api.ts @@ -141,6 +141,39 @@ function extractForRootSignatures(): string[] { // ─── Module overview ──────────────────────────────────────────── +// ─── Exception extraction ──────────────────────────────────────── + +/** + * Collect the exported error helpers (classes and factories) from the exception directories. + * + * Extracted from source rather than hard-coded: a new exception must show up here automatically, + * otherwise a public export that changes HTTP semantics stays invisible in the very file CLAUDE.md + * advertises as the framework's machine-readable API surface. + */ +function extractExceptions(): string[] { + const rows: string[] = []; + + for (const sf of project.getSourceFiles(['src/core/**/exceptions/*.ts'])) { + const file = sf.getFilePath().replace(`${ROOT}/`, ''); + + for (const [name, decls] of sf.getExportedDeclarations()) { + const decl = decls[0]; + if (!decl) continue; + + const kind = decl.getKindName(); + if (kind !== 'ClassDeclaration' && kind !== 'FunctionDeclaration') continue; + + const doc = (decl as any).getJsDocs?.()[0]?.getDescription?.() ?? ''; + const summary = truncateDescription(doc.trim().split('\n')[0] ?? ''); + const signature = kind === 'FunctionDeclaration' ? `${name}()` : name; + + rows.push(`| \`${signature}\` | ${summary || '—'} | \`${file}\` |`); + } + } + + return rows.sort(); +} + function extractCoreModules(): string[] { const lines: string[] = []; const modulesDir = resolve(ROOT, 'src/core/modules'); @@ -248,6 +281,9 @@ function main() { // Core modules overview const coreModules = extractCoreModules(); + // Exported error helpers + const exceptions = extractExceptions(); + // ─── Assemble document ───────────────────────────────────────── const version = JSON.parse(readFileSync(resolve(ROOT, 'package.json'), 'utf-8')).version; @@ -285,6 +321,27 @@ ${crudMethods.join('\n')} |--------|------|------| ${coreModules.join('\n')} +## Errors & Status Codes + +One 401/403 policy across all permission layers (role guards, tenant guard, \`check()\`, +\`checkRestricted()\`, model \`securityCheck()\`): + +| Situation | Status | Message | +|-----------|--------|---------| +| Requester is **not authenticated** | **401** | \`ErrorCode.UNAUTHORIZED\` | +| Requester **is authenticated** but lacks a right | **403** | \`ErrorCode.ACCESS_DENIED\` | +| \`S_NO_ONE\` (locked for everyone, even admins) | **403 always** | \`ErrorCode.ACCESS_DENIED\` | + +Never hand-roll the decision — use \`accessDeniedException(user)\`. It returns the **native** +\`ForbiddenException\` / \`UnauthorizedException\`, so \`instanceof\` checks and \`@Catch(...)\` filters +in consuming projects keep working. + +### Exported error helpers + +| Export | Purpose | Path | +|--------|---------|------| +${exceptions.join('\n')} + ## Key Source Files | File | Purpose | @@ -295,6 +352,7 @@ ${coreModules.join('\n')} | \`src/core/common/services/crud.service.ts\` | CrudService base class | | \`src/core/common/services/config.service.ts\` | ConfigService (global) | | \`src/core/common/decorators/\` | @Restricted, @Roles, @CurrentUser, @UnifiedField | +| \`src/core/common/exceptions/\` | accessDeniedException — the 401/403 policy | | \`src/core/common/interceptors/\` | CheckResponse, CheckSecurity, ResponseModel | | \`docs/REQUEST-LIFECYCLE.md\` | Complete request lifecycle | | \`.claude/rules/\` | Detailed rules for architecture, security, testing | diff --git a/spectaql.yml b/spectaql.yml index b3c1b6cf..aa364358 100644 --- a/spectaql.yml +++ b/spectaql.yml @@ -19,7 +19,7 @@ servers: info: title: lT Nest Server description: Modern, fast, powerful Node.js web framework in TypeScript based on Nest with a GraphQL API and a connection to MongoDB (or other databases). - version: 11.27.7 + version: 11.28.0 contact: name: lenne.Tech GmbH url: https://lenne.tech diff --git a/src/core/common/decorators/restricted.decorator.ts b/src/core/common/decorators/restricted.decorator.ts index 6580ce66..ec90fe27 100644 --- a/src/core/common/decorators/restricted.decorator.ts +++ b/src/core/common/decorators/restricted.decorator.ts @@ -21,12 +21,12 @@ * that is worth doing. Until then, `pnpm run check:swc-tdz` is the mechanical guard. * See .claude/rules/architecture.md → "DI Token Placement (SWC-Safe)". */ -import { UnauthorizedException } from '@nestjs/common'; import 'reflect-metadata'; import _ = require('lodash'); import { ProcessType } from '../enums/process-type.enum'; import { RoleEnum } from '../enums/role.enum'; +import { accessDeniedException } from '../exceptions/access-denied.exception'; // Import from the id.helper LEAF, never from db.helper: db.helper imports input.helper, which // imports this file back — that cycle is what the extraction removed. See id.helper's docblock. import { equalIds, getIncludedIds } from '../helpers/id.helper'; @@ -293,15 +293,32 @@ export function checkRestricted( return false; } + // Ownership (S_SELF, S_CREATOR) must be decided from the PERSISTED object, never from `data`. + // + // On the INPUT path `data` is the caller-supplied DTO, so every ownership claim in it is + // attacker-controlled. An authenticated attacker could otherwise unlock an owner-restricted + // field on someone ELSE's record just by asserting ownership in the payload — the service + // applies the input to the target it was called with, not to the ids in the body: + // + // PATCH /users/ { "id": "", "createdBy": "", "": ... } + // + // `check()` (input.helper.ts) already reads both from `config.dbObject`; this is the same + // check and must agree with it. Where no dbObject exists (e.g. create), ownership cannot be + // established — exactly what check() does too. + // + // On the OUTPUT path there is no attacker-controlled input: `data` IS the persisted object + // (and a list yields one `data` per item), so it stays the source of truth there. + const owner = config.processType === ProcessType.INPUT ? config.dbObject : data; + // Check access rights if ( roles.includes(RoleEnum.S_EVERYONE) || user?.hasRole?.(roles) || (user?.id && roles.includes(RoleEnum.S_USER)) || - (roles.includes(RoleEnum.S_SELF) && equalIds(data, user)) || + (roles.includes(RoleEnum.S_SELF) && equalIds(owner, user)) || (roles.includes(RoleEnum.S_CREATOR) && - (('createdBy' in data && equalIds(data.createdBy, user)) || - (config.allowCreatorOfParent && !('createdBy' in data) && config.isCreatorOfParent))) || + ((owner && 'createdBy' in owner && equalIds(owner.createdBy, user)) || + (config.allowCreatorOfParent && owner && !('createdBy' in owner) && config.isCreatorOfParent))) || (roles.includes(RoleEnum.S_VERIFIED) && (user?.verified || user?.verifiedAt || user?.emailVerified)) || (user?.id && checkRoleAccess(roles, user?.roles, RequestContext.get()?.tenantRole)) ) { @@ -365,9 +382,10 @@ export function checkRestricted( if (config.debug) { console.debug(`The current user has no access rights for ${data.constructor?.name}`); } - // Throw error + // 403 when authenticated, 401 otherwise (see accessDeniedException). The class name stays in + // the debug log above — the client gets the translatable ErrorCode the role guards also use. if (config.throwError) { - throw new UnauthorizedException(`The current user has no access rights for ${data.constructor?.name}`); + throw accessDeniedException(user); } return null; } @@ -393,9 +411,14 @@ export function checkRestricted( // Check rights if (valid) { - // Check if data is user or user is creator of data (for nested plain objects) + // Check if the parent is the user, or the user created it (for nested plain objects). + // Same rule as above: on INPUT the ownership claim must come from the persisted object, not + // from the DTO — otherwise a forged `id`/`createdBy` in the payload would propagate a faked + // "creator of parent" trust down into every nested object. + const parent = config.processType === ProcessType.INPUT ? config.dbObject : data; config.isCreatorOfParent = - equalIds(data, user) || ('createdBy' in data ? equalIds(data.createdBy, user) : config.isCreatorOfParent); + equalIds(parent, user) || + (parent && 'createdBy' in parent ? equalIds(parent.createdBy, user) : config.isCreatorOfParent); // Check deep data[propertyKey] = checkRestricted(data[propertyKey], user, config, processedObjects); @@ -405,11 +428,10 @@ export function checkRestricted( `The current user has no access rights for ${propertyKey}${data.constructor?.name ? ` of ${data.constructor.name}` : ''}`, ); } - // Throw error + // 403 when authenticated, 401 otherwise (see accessDeniedException). The field and class name + // stay in the debug log above — the client gets the translatable ErrorCode the guards also use. if (config.throwError) { - throw new UnauthorizedException( - `The current user has no access rights for ${propertyKey}${data.constructor?.name ? ` of ${data.constructor.name}` : ''}`, - ); + throw accessDeniedException(user); } // Remove property diff --git a/src/core/common/exceptions/access-denied.exception.ts b/src/core/common/exceptions/access-denied.exception.ts new file mode 100644 index 00000000..6158b536 --- /dev/null +++ b/src/core/common/exceptions/access-denied.exception.ts @@ -0,0 +1,49 @@ +import { ForbiddenException, UnauthorizedException } from '@nestjs/common'; + +import { ErrorCode } from '../../modules/error-code/error-codes'; + +/** + * Creates the access error that matches the requester's auth state (RFC 9110, mirrors RolesGuard): + * **403 Forbidden** for authenticated requesters (a permission problem) and **401 Unauthorized** + * only when the requester is not authenticated. + * + * Frontends commonly treat 401 as "session expired" and auto-logout (the `@lenne.tech/nuxt-extensions` + * auth interceptor patches `$fetch`/`fetch` globally and does exactly this), so a mere permission + * error must never surface as 401 — it would kick a logged-in user out of the whole app. + * + * This is a **factory, not a class**, on purpose: it returns the *native* Nest exceptions, so + * `instanceof ForbiddenException` / `instanceof UnauthorizedException` and `@Catch(...)` filters in + * consuming projects keep working, and the REST wire body (which `HttpExceptionLogFilter` builds via + * `{ ...exception }`, including `name`) stays identical to what it was before. A custom + * `HttpException` subclass would satisfy neither. + * + * The default messages are the same translatable `ErrorCode`s the role guards throw (`#LTNS_xxxx:` + * marker → resolvable by the frontend error-translation layer). Pass an explicit `message` only + * where a raw string is genuinely required; prefer logging request-specific detail (class names, + * field names) over returning it to the client. + * + * @param user The **requesting** user (never the target object). The decision key is `user.id`: an + * id that is present — even a falsy one like `0` or `''` — counts as authenticated, while + * `undefined`, `null` or no user at all does not. This mirrors how `check()` defines "logged in" + * (`S_USER` requires `user?.id`). + * @param message Overrides the default `ErrorCode`. Omit it to stay consistent with `RolesGuard`. + * + * @example + * // 403 for an authenticated user who lacks a right, 401 for an anonymous requester: + * throw accessDeniedException(currentUser); + * + * @see src/core/modules/auth/guards/roles.guard.ts — the pre-existing 401/403 pattern this mirrors + * @see migration-guides/11.27.7-to-11.28.0.md + */ +export function accessDeniedException( + user: { id?: unknown } | null | undefined, + message?: string, +): ForbiddenException | UnauthorizedException { + // A present id means authenticated. `!!user?.id` would misjudge falsy-but-real ids (0, '') as + // anonymous and hand an authenticated user the very 401 this mechanism exists to avoid. + const authenticated = user?.id !== undefined && user?.id !== null; + + return authenticated + ? new ForbiddenException(message ?? ErrorCode.ACCESS_DENIED) + : new UnauthorizedException(message ?? ErrorCode.UNAUTHORIZED); +} diff --git a/src/core/common/helpers/input.helper.ts b/src/core/common/helpers/input.helper.ts index 62b6d309..6629e5d6 100644 --- a/src/core/common/helpers/input.helper.ts +++ b/src/core/common/helpers/input.helper.ts @@ -1,4 +1,4 @@ -import { BadRequestException, UnauthorizedException } from '@nestjs/common'; +import { BadRequestException, ForbiddenException } from '@nestjs/common'; import { plainToInstance } from 'class-transformer'; import { validate } from 'class-validator'; import { ValidatorOptions } from 'class-validator/types/validation/ValidatorOptions'; @@ -7,6 +7,8 @@ import { Kind } from 'graphql/index'; import { checkRestricted } from '../decorators/restricted.decorator'; import { ProcessType } from '../enums/process-type.enum'; import { RoleEnum } from '../enums/role.enum'; +import { accessDeniedException } from '../exceptions/access-denied.exception'; +import { ErrorCode } from '../../modules/error-code/error-codes'; import { clone } from './clone.helper'; import { merge } from './config.helper'; import { equalIds } from './id.helper'; @@ -248,9 +250,11 @@ export async function check( } let valid = false; - // Prevent access for everyone, including administrators + // Prevent access for everyone, including administrators. Always 403, never 401: the resource is + // locked permanently, so authenticating can never grant access and telling an anonymous + // requester to "authenticate and retry" (401) would be a lie. Both role guards do the same. if (roles.includes(RoleEnum.S_NO_ONE)) { - throw new UnauthorizedException('No access'); + throw new ForbiddenException(ErrorCode.ACCESS_DENIED); } // Check access @@ -276,7 +280,8 @@ export async function check( valid = true; } if (!valid) { - throw new UnauthorizedException('Missing rights'); + // 403 when authenticated, 401 otherwise — policy documented in accessDeniedException + throw accessDeniedException(user); } } diff --git a/src/core/common/helpers/service.helper.ts b/src/core/common/helpers/service.helper.ts index 6d9dd0b6..e59714f2 100644 --- a/src/core/common/helpers/service.helper.ts +++ b/src/core/common/helpers/service.helper.ts @@ -1,10 +1,10 @@ -import { UnauthorizedException } from '@nestjs/common'; import bcrypt = require('bcrypt'); import { sha256 } from 'js-sha256'; import _ = require('lodash'); import { Types } from 'mongoose'; import { RoleEnum } from '../enums/role.enum'; +import { accessDeniedException } from '../exceptions/access-denied.exception'; import { PrepareInputOptions } from '../interfaces/prepare-input-options.interface'; import { PrepareOutputOptions } from '../interfaces/prepare-output-options.interface'; import { ResolveSelector } from '../interfaces/resolve-selector.interface'; @@ -151,15 +151,17 @@ export async function prepareInput( value === undefined && delete input[key]; } - // Process roles + // Process roles — 403 when authenticated, 401 otherwise (see accessDeniedException). The rejected + // roles are logged rather than returned: the client gets the translatable ErrorCode the guards use. if (config.checkRoles && (input as Record).roles && !currentUser?.hasRole?.(RoleEnum.ADMIN)) { if (!(currentUser as any)?.roles) { - throw new UnauthorizedException('Missing roles of current user'); + throw accessDeniedException(currentUser); } else { const allowedRoles = _.intersection((input as Record).roles, (currentUser as any).roles); if (allowedRoles.length !== (input as Record).roles.length) { const missingRoles = _.difference((input as Record).roles, (currentUser as any).roles); - throw new UnauthorizedException(`Current user not allowed setting roles: ${missingRoles}`); + console.debug(`Current user not allowed setting roles: ${missingRoles}`); + throw accessDeniedException(currentUser); } (input as Record).roles = allowedRoles; } diff --git a/src/core/modules/auth/guards/roles.guard.ts b/src/core/modules/auth/guards/roles.guard.ts index 53ed23a3..9567a7ff 100644 --- a/src/core/modules/auth/guards/roles.guard.ts +++ b/src/core/modules/auth/guards/roles.guard.ts @@ -137,9 +137,10 @@ export class RolesGuard extends AuthGuard(AuthGuardStrategy.JWT) { ]); const roles = mergeRolesMetadata(reflectorRoles); - // Check if locked - always deny + // Check if locked - always deny. 403, never 401: the endpoint is locked permanently, so + // authenticating can never grant access and a 401 ("authenticate and retry") would be a lie. if (roles && roles.includes(RoleEnum.S_NO_ONE)) { - throw new UnauthorizedException(ErrorCode.UNAUTHORIZED); + throw new ForbiddenException(ErrorCode.ACCESS_DENIED); } // If no roles required, or S_EVERYONE is set, allow access without authentication @@ -293,9 +294,9 @@ export class RolesGuard extends AuthGuard(AuthGuardStrategy.JWT) { ]); const roles = mergeRolesMetadata(reflectorRoles); - // Check if locked + // Check if locked — 403, never 401 (see canActivate: authenticating can never unlock it) if (roles && roles.includes(RoleEnum.S_NO_ONE)) { - throw new UnauthorizedException(ErrorCode.UNAUTHORIZED); + throw new ForbiddenException(ErrorCode.ACCESS_DENIED); } // Check roles diff --git a/src/core/modules/better-auth/better-auth-roles.guard.ts b/src/core/modules/better-auth/better-auth-roles.guard.ts index 83812eeb..28e31ced 100644 --- a/src/core/modules/better-auth/better-auth-roles.guard.ts +++ b/src/core/modules/better-auth/better-auth-roles.guard.ts @@ -88,9 +88,11 @@ export class BetterAuthRolesGuard implements CanActivate { // Combine handler and class roles (handler takes precedence, like Reflector.getAll) const roles = mergeRolesMetadata([handlerRoles, classRoles]); - // Check if locked - always deny + // Check if locked - always deny. 403, never 401: the endpoint is locked permanently, so + // authenticating can never grant access and a 401 ("authenticate and retry") would be a lie. + // Kept in sync with RolesGuard (see .claude/rules/better-auth.md). if (roles && roles.includes(RoleEnum.S_NO_ONE)) { - throw new UnauthorizedException(ErrorCode.UNAUTHORIZED); + throw new ForbiddenException(ErrorCode.ACCESS_DENIED); } // If no roles required, or S_EVERYONE is set, allow access without authentication diff --git a/src/core/modules/better-auth/core-better-auth.controller.ts b/src/core/modules/better-auth/core-better-auth.controller.ts index c17bdca8..921153f3 100644 --- a/src/core/modules/better-auth/core-better-auth.controller.ts +++ b/src/core/modules/better-auth/core-better-auth.controller.ts @@ -5,6 +5,7 @@ import { Controller, Get, HttpCode, + HttpException, HttpStatus, InternalServerErrorException, Logger, @@ -932,12 +933,10 @@ export class CoreBetterAuthController { } catch (error) { this.logger.error(`Better Auth handler error: ${error instanceof Error ? error.message : 'Unknown error'}`); - // Re-throw NestJS exceptions - if ( - error instanceof BadRequestException || - error instanceof UnauthorizedException || - error instanceof InternalServerErrorException - ) { + // Re-throw NestJS exceptions. Matching HttpException (not an enumeration of subclasses) keeps + // any deliberately thrown status intact — an enumeration silently masks everything it forgets + // (e.g. a 403 from the rights checks) as a 500. + if (error instanceof HttpException) { throw error; } diff --git a/src/core/modules/tenant/core-tenant-member.model.ts b/src/core/modules/tenant/core-tenant-member.model.ts index e5b61a81..67d32450 100644 --- a/src/core/modules/tenant/core-tenant-member.model.ts +++ b/src/core/modules/tenant/core-tenant-member.model.ts @@ -1,10 +1,10 @@ -import { UnauthorizedException } from '@nestjs/common'; import { ObjectType } from '@nestjs/graphql'; import { Schema } from '@nestjs/mongoose'; import { Restricted } from '../../common/decorators/restricted.decorator'; import { UnifiedField } from '../../common/decorators/unified-field.decorator'; import { RoleEnum } from '../../common/enums/role.enum'; +import { accessDeniedException } from '../../common/exceptions/access-denied.exception'; import { CorePersistenceModel } from '../../common/models/core-persistence.model'; import { RequestContext } from '../../common/services/request-context.service'; import { DefaultHR, TenantMemberStatus } from './core-tenant.enums'; @@ -98,7 +98,11 @@ export class CoreTenantMemberModel extends CorePersistenceModel { */ override securityCheck(user: any, force?: boolean): this { if (force) return this; - if (!user) throw new UnauthorizedException('Access to tenant membership denied'); + // accessDeniedException picks the status from the requester's auth state: 401 here (no user), + // 403 below (authenticated, but not owner / admin / tenant manager). Model-level denials must + // use this helper too — a permission error must never reach the client as a 401, or frontends + // treat it as an expired session and log the user out. + if (!user) throw accessDeniedException(user); // Own membership or system admin if (user.id === this.user || user.hasRole?.(RoleEnum.ADMIN)) return this; @@ -116,6 +120,7 @@ export class CoreTenantMemberModel extends CorePersistenceModel { return this; } - throw new UnauthorizedException('Access to tenant membership denied'); + // Reached only when `user` is set (the `!user` case returned above), so this is a 403. + throw accessDeniedException(user); } } diff --git a/src/core/modules/tenant/core-tenant.guard.ts b/src/core/modules/tenant/core-tenant.guard.ts index 90c442c1..16927fd5 100644 --- a/src/core/modules/tenant/core-tenant.guard.ts +++ b/src/core/modules/tenant/core-tenant.guard.ts @@ -1,4 +1,12 @@ -import { CanActivate, ExecutionContext, ForbiddenException, Injectable, Logger, OnModuleDestroy } from '@nestjs/common'; +import { + CanActivate, + ExecutionContext, + ForbiddenException, + Injectable, + Logger, + OnModuleDestroy, + UnauthorizedException, +} from '@nestjs/common'; import { Reflector } from '@nestjs/core'; import { GqlContextType, GqlExecutionContext } from '@nestjs/graphql'; import { InjectModel } from '@nestjs/mongoose'; @@ -6,6 +14,7 @@ import { Model } from 'mongoose'; import { RoleEnum } from '../../common/enums/role.enum'; import { ConfigService } from '../../common/services/config.service'; +import { ErrorCode } from '../error-code/error-codes'; import { CoreTenantMemberModel } from './core-tenant-member.model'; import { SKIP_TENANT_CHECK_KEY } from './core-tenant.decorators'; import { TENANT_MEMBER_MODEL_TOKEN, TenantMemberStatus } from './core-tenant.enums'; @@ -73,9 +82,14 @@ interface CachedTenantIds { * 5. @SkipTenantCheck → role check against user.roles, no tenant context * 6. BetterAuth auto-skip (betterAuth.skipTenantCheck config + no header) → skip, no tenant context * + * Status codes (RFC 9110, aligned with RolesGuard since v11.28.0): + * - Missing authentication → 401 (re-authenticating IS the remedy, so the client must be told to) + * - Authenticated but lacking a right / membership → 403 + * - S_NO_ONE → always 403 (authenticating can never unlock it) + * * HEADER PRESENT: * - System ADMIN (adminBypass: true) → set req.tenantId + isAdminBypass - * - No user → 403 "Authentication required for tenant access" + * - No user → 401 (tenant access requires authentication) * - Authenticated non-admin user: * - Active member → checkRoleAccess against membership.role → set req.tenantId + tenantRole * - Not active member → ALWAYS 403 @@ -86,7 +100,7 @@ interface CachedTenantIds { * → resolveUserTenantIds with minLevel filter * - Authenticated + no checkable roles → resolveUserTenantIds (all memberships) * - No user + no checkable roles → pass (plugin safety net catches tenantId-schema access) - * - No user + checkable roles → 403 "Authentication required" + * - No user + checkable roles → 401 (authentication required) */ @Injectable() export class CoreTenantGuard implements CanActivate, OnModuleDestroy { @@ -216,8 +230,9 @@ export class CoreTenantGuard implements CanActivate, OnModuleDestroy { // Defense-in-depth: S_NO_ONE is normally caught by RolesGuard/BetterAuthRolesGuard upstream, // but guard it here too in case CoreTenantGuard runs standalone (e.g., custom guard chains). + // Always 403 — the same code the role guards return since v11.28.0. if (roles.includes(RoleEnum.S_NO_ONE)) { - throw new ForbiddenException('Access denied'); + throw new ForbiddenException(ErrorCode.ACCESS_DENIED); } const sEveryoneGrantsAccess: boolean = systemCheckRoles.includes(RoleEnum.S_EVERYONE); @@ -258,7 +273,7 @@ export class CoreTenantGuard implements CanActivate, OnModuleDestroy { const sUserGrantsAccess: boolean = systemCheckRoles.includes(RoleEnum.S_USER); if (sUserGrantsAccess) { if (!user) { - throw new ForbiddenException('Authentication required'); + throw new UnauthorizedException(ErrorCode.UNAUTHORIZED); } if (headerTenantId && !hasSkipDecorator) { return this.handleSystemRoleWithTenantHeader(user, headerTenantId, request, isAdmin); @@ -276,7 +291,7 @@ export class CoreTenantGuard implements CanActivate, OnModuleDestroy { const sVerifiedGrantsAccess: boolean = systemCheckRoles.includes(RoleEnum.S_VERIFIED); if (sVerifiedGrantsAccess) { if (!user) { - throw new ForbiddenException('Authentication required'); + throw new UnauthorizedException(ErrorCode.UNAUTHORIZED); } const isVerified = !!(user.verified || user.verifiedAt || user.emailVerified); if (!isVerified) { @@ -337,9 +352,9 @@ export class CoreTenantGuard implements CanActivate, OnModuleDestroy { return true; } - // No user + header → 403 (tenant access requires authentication) + // No user + header → 401 (tenant access requires authentication; re-auth is the remedy) if (!user) { - throw new ForbiddenException('Authentication required for tenant access'); + throw new UnauthorizedException(ErrorCode.UNAUTHORIZED); } // Authenticated non-admin user: MUST be active member @@ -375,9 +390,9 @@ export class CoreTenantGuard implements CanActivate, OnModuleDestroy { // Checkable roles present if (checkableRoles.length > 0) { - // No user + roles required → 403 + // No user + roles required → 401 (re-auth is the remedy, so it must not be a 403) if (!user) { - throw new ForbiddenException('Authentication required'); + throw new UnauthorizedException(ErrorCode.UNAUTHORIZED); } // Check role access against user.roles (hierarchy: level comparison, normal: exact match) @@ -604,7 +619,7 @@ export class CoreTenantGuard implements CanActivate, OnModuleDestroy { if (checkableRoles.length > 0) { // Defense-in-depth: reject unauthenticated access even if RolesGuard is absent if (!user) { - throw new ForbiddenException('Authentication required'); + throw new UnauthorizedException(ErrorCode.UNAUTHORIZED); } if (!isAdmin && !checkRoleAccess(checkableRoles, user.roles, undefined)) { throw new ForbiddenException('Insufficient role'); diff --git a/src/index.ts b/src/index.ts index 108cf003..2e482fbb 100644 --- a/src/index.ts +++ b/src/index.ts @@ -26,6 +26,7 @@ export * from './core/common/enums/logical-operator.enum'; export * from './core/common/enums/process-type.enum'; export * from './core/common/enums/role.enum'; export * from './core/common/enums/sort-order.emum'; +export * from './core/common/exceptions/access-denied.exception'; export * from './core/common/filters/http-exception-log.filter'; export * from './core/common/helpers/common.helper'; export * from './core/common/helpers/config.helper'; diff --git a/src/server/modules/user/user.service.ts b/src/server/modules/user/user.service.ts index 0297af2c..9dce3772 100644 --- a/src/server/modules/user/user.service.ts +++ b/src/server/modules/user/user.service.ts @@ -98,9 +98,10 @@ export class UserService extends CoreUserService { const dbUser = await this.mainDbModel.findOne({ id: user.id }).exec(); - // Check user + // Check user: the token is valid but the account no longer exists, so the session really is + // invalid — 401 is right here. (A permission error would have to be 403, see accessDeniedException.) if (!dbUser) { - throw new UnauthorizedException('User is not allowed to set the avatar'); + throw new UnauthorizedException('User of the current session no longer exists'); } // Check file diff --git a/tests/ai.e2e-spec.ts b/tests/ai.e2e-spec.ts index d7a55895..82fc4c0e 100644 --- a/tests/ai.e2e-spec.ts +++ b/tests/ai.e2e-spec.ts @@ -86,8 +86,12 @@ describe('AI module (e2e)', () => { * `iam.session_token` Set-Cookie. We forward that cookie to * `betterAuthService.getToken({ headers: { cookie } })` (the JWT plugin's * /iam/token endpoint underneath), which deterministically returns a JWT — - * regardless of whether the sign-in body contained one. This eliminates the - * pre-existing flaky 401s in this file. + * regardless of whether the sign-in body contained one. + * + * Note: this makes token ACQUISITION deterministic. The sporadic 401s this file used to see had a + * second, unrelated cause — another e2e file wiped the shared `jwks` keyset mid-run, which + * invalidated tokens that had already been issued here. That wipe has been removed; see + * tests/stories/better-auth-integration.story.test.ts. */ async function createHttpUser(email: string, roles: string[]): Promise { await testHelper.rest('/iam/sign-up/email', { @@ -537,14 +541,15 @@ describe('AI module (e2e)', () => { const own = await getUserTool!.execute({ id: ownerId.toString() }, context); expect((own as any).success).not.toBe(false); - // A regular user must NOT read another user's record — and the denial must be an - // authorization error (401/403), not just any thrown error. + // A regular user must NOT read another user's record. The requester IS authenticated and merely + // lacks the right, so checkRestricted() answers with a deterministic 403 — pinning it (instead of + // accepting [401, 403]) is what makes this test able to detect a regression to the old 401. const denial = await getUserTool!.execute({ id: otherId.toString() }, context).then( () => null, (e) => e, ); expect(denial).toBeTruthy(); - expect([401, 403]).toContain(denial.status); + expect(denial.status).toBe(403); await db.collection('users').deleteMany({ _id: { $in: [ownerId, otherId] } }); }); @@ -922,9 +927,10 @@ describe('AI module (e2e)', () => { }); it('exposes prompt endpoints to authenticated users (REST list + GraphQL) and rejects anonymous calls', async () => { - // Anonymous call → 401/403 on REST and an error on GraphQL. + // Anonymous call → a deterministic 401 on REST (no session at all; 403 is reserved for an + // authenticated requester who lacks a right) and an error on GraphQL. const anon = await request(app.getHttpServer()).get('/ai/prompts'); - expect([401, 403]).toContain(anon.status); + expect(anon.status).toBe(401); // Authenticated REST list works (default scope filters apply; at minimum returns an array). const restList = await request(app.getHttpServer()) diff --git a/tests/server.e2e-spec.ts b/tests/server.e2e-spec.ts index e05afc5e..cc816315 100644 --- a/tests/server.e2e-spec.ts +++ b/tests/server.e2e-spec.ts @@ -5,6 +5,7 @@ import { MongoClient, ObjectId } from 'mongodb'; import { ComparisonOperatorEnum, ConfigService, + ErrorCode, getPlain, HttpExceptionLogFilter, scimToMongo, @@ -487,9 +488,35 @@ describe('ServerModule (e2e)', () => { { token: gToken }, ); + expect(res.errors.length).toBeGreaterThanOrEqual(1); + // 403, not 401: the user is authenticated and merely lacks rights — 401 is reserved for + // unauthenticated requesters so frontends don't treat permission errors as expired sessions + expect(res.errors[0].extensions.originalError.statusCode).toEqual(403); + // The client gets the translatable ErrorCode the role guards also throw; the field and class + // name that used to be in this message are logged instead of returned + expect(res.errors[0].message).toEqual(ErrorCode.ACCESS_DENIED); + expect(res.errors[0].extensions.originalError.error).toEqual('Forbidden'); + expect(res.data).toBe(null); + }); + + /** + * The 401 branch of the rights checks is defense-in-depth, not an end-to-end path: every endpoint + * whose input carries a @Restricted field also requires authentication, so the role guards answer + * an anonymous requester with 401 long before checkRestricted()/check() run. The branch is covered + * exhaustively in tests/unit/access-denied-exception.spec.ts; here we only pin that an anonymous + * request really is rejected by the guard with 401 and the translatable ErrorCode. + */ + it('anonymous request is rejected by the guard with 401, not 403', async () => { + const res: any = await testHelper.graphQl({ + arguments: { id: gId }, + fields: ['id', 'email'], + name: 'getUser', + type: TestGraphQLType.QUERY, + }); + expect(res.errors.length).toBeGreaterThanOrEqual(1); expect(res.errors[0].extensions.originalError.statusCode).toEqual(401); - expect(res.errors[0].message).toEqual('The current user has no access rights for roles of UserInput'); + expect(res.errors[0].extensions.originalError.error).toEqual('Unauthorized'); expect(res.data).toBe(null); }); diff --git a/tests/stories/better-auth-autoregister-false.e2e-spec.ts b/tests/stories/better-auth-autoregister-false.e2e-spec.ts index b70390bf..4e3f65d9 100644 --- a/tests/stories/better-auth-autoregister-false.e2e-spec.ts +++ b/tests/stories/better-auth-autoregister-false.e2e-spec.ts @@ -216,11 +216,12 @@ describe('Story: BetterAuth autoRegister: false (Pattern 3)', () => { }); }); - it('should enforce @Roles(S_NO_ONE) — 401 always', async () => { - // S_NO_ONE throws UnauthorizedException (401) in RolesGuard, not ForbiddenException (403) + it('should enforce @Roles(S_NO_ONE) — 403 always', async () => { + // S_NO_ONE is locked permanently — authenticating can never unlock it, so 403 is the correct + // code for EVERY requester. A 401 would tell the client to retry after logging in. await testHelper.rest('/security-test/no-one', { method: 'GET', - statusCode: 401, + statusCode: 403, token: userToken, }); }); diff --git a/tests/stories/better-auth-integration.story.test.ts b/tests/stories/better-auth-integration.story.test.ts index 8deabbce..3e895600 100644 --- a/tests/stories/better-auth-integration.story.test.ts +++ b/tests/stories/better-auth-integration.story.test.ts @@ -955,14 +955,12 @@ describe('Story: BetterAuth Integration', () => { // =================================================================================================================== describe('JWT Token Resolution (Bug #4)', () => { - beforeAll(async () => { - // Clear stale JWKS keys that may have been encrypted with a different secret - // This prevents "Failed to decrypt private key" errors during JWT generation - if (db) { - await db.collection('jwks').deleteMany({}); - } - }); - + // NOTE: this block must NOT wipe the `jwks` collection. All e2e files share one database per + // run and execute in parallel forks, so dropping the BetterAuth signing keyset invalidates + // every JWT that other files already minted (they cache their tokens in beforeAll) — which + // surfaced as sporadic 401s elsewhere, e.g. in tests/ai.e2e-spec.ts. 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 (tests/global-setup.ts), so that state can no longer exist. it('should return a proper JWT (not session token) from REST sign-in when cookies are disabled', async () => { const cookiesDisabled = configService.getFastButReadOnly('cookies') === false; if (!cookiesDisabled) { diff --git a/tests/stories/better-auth-module-registration.e2e-spec.ts b/tests/stories/better-auth-module-registration.e2e-spec.ts index f8f7bead..c2ed39d7 100644 --- a/tests/stories/better-auth-module-registration.e2e-spec.ts +++ b/tests/stories/better-auth-module-registration.e2e-spec.ts @@ -331,11 +331,12 @@ describe('Story: BetterAuth Module Registration', () => { }); }); - it('should enforce @Roles(S_NO_ONE) — 401 always', async () => { - // S_NO_ONE throws UnauthorizedException (401) in RolesGuard, not ForbiddenException (403) + it('should enforce @Roles(S_NO_ONE) — 403 always', async () => { + // S_NO_ONE is locked permanently — authenticating can never unlock it, so 403 is the correct + // code for EVERY requester. A 401 would tell the client to retry after logging in. await testHelper.rest('/security-test/no-one', { method: 'GET', - statusCode: 401, + statusCode: 403, token: userToken, }); }); diff --git a/tests/stories/better-auth-rest-security.e2e-spec.ts b/tests/stories/better-auth-rest-security.e2e-spec.ts index e0c2f089..8b6495c6 100644 --- a/tests/stories/better-auth-rest-security.e2e-spec.ts +++ b/tests/stories/better-auth-rest-security.e2e-spec.ts @@ -101,7 +101,7 @@ class SecurityTestController { /** * Locked endpoint - S_NO_ONE should always deny access * @Roles(S_NO_ONE) means NO ONE can access, even admins - * RolesGuard will reject ALL requests with 401 + * RolesGuard will reject ALL requests with 403 (authenticating can never unlock it) */ @Get('locked') @Roles(RoleEnum.S_NO_ONE) @@ -718,9 +718,11 @@ describe('Story: BetterAuth REST Security', () => { describe('Locked Endpoints (S_NO_ONE)', () => { it('SECURITY: should REJECT unauthenticated requests', async () => { + // 403, not 401, even without a token: S_NO_ONE is locked permanently, so 'authenticate and + // retry' (401) would be a lie — logging in can never grant access to this endpoint. const result = await testHelper.rest('/security-test/locked', { method: 'GET', - statusCode: 401, + statusCode: 403, }); expect(result.success).not.toBe(true); @@ -735,7 +737,7 @@ describe('Story: BetterAuth REST Security', () => { // S_NO_ONE should deny access to everyone const result = await testHelper.rest('/security-test/locked', { method: 'GET', - statusCode: 401, // S_NO_ONE returns 401 Unauthorized + statusCode: 403, // S_NO_ONE is locked permanently → 403 for everyone, even admins token: adminUserToken, }); @@ -1061,7 +1063,7 @@ describe('Story: BetterAuth REST Security', () => { const result = await testHelper.rest('/security-test/locked', { cookies: adminUserSessionToken, method: 'GET', - statusCode: 401, + statusCode: 403, }); expect(result.success).not.toBe(true); diff --git a/tests/tenant-guard.e2e-spec.ts b/tests/tenant-guard.e2e-spec.ts index 0ad2930e..ec79d58b 100644 --- a/tests/tenant-guard.e2e-spec.ts +++ b/tests/tenant-guard.e2e-spec.ts @@ -1,4 +1,4 @@ -import { Controller, Get, Injectable, Module } from '@nestjs/common'; +import { Controller, ForbiddenException, Get, Injectable, Module, UnauthorizedException } from '@nestjs/common'; import { APP_GUARD } from '@nestjs/core'; import { MongooseModule, Prop, Schema, SchemaFactory, getModelToken } from '@nestjs/mongoose'; import { Test, TestingModule } from '@nestjs/testing'; @@ -10,6 +10,7 @@ import { Roles } from '../src/core/common/decorators/roles.decorator'; import { RoleEnum } from '../src/core/common/enums/role.enum'; import { mongooseTenantPlugin } from '../src/core/common/plugins/mongoose-tenant.plugin'; import { ConfigService } from '../src/core/common/services/config.service'; +import { ErrorCode } from '../src/core/modules/error-code/error-codes'; import { IRequestContext, RequestContext } from '../src/core/common/services/request-context.service'; import { CoreTenantMemberModel } from '../src/core/modules/tenant/core-tenant-member.model'; import { CoreTenantGuard } from '../src/core/modules/tenant/core-tenant.guard'; @@ -488,13 +489,13 @@ describe('CoreTenantGuard (e2e)', () => { // B3) Header + no user → 403 // ========================================================================= - it('should deny unauthenticated access when header is present', async () => { + it('should deny unauthenticated access when header is present (401)', async () => { const res = await request(app.getHttpServer()) .get('/test/tenant-member') .set('X-Tenant-Id', TENANT_A) - .expect(403); + .expect(401); - expect(res.body.message).toContain('Authentication required'); + expect(res.body.message).toContain(ErrorCode.UNAUTHORIZED); }); // ========================================================================= @@ -590,12 +591,12 @@ describe('CoreTenantGuard (e2e)', () => { expect(res.body.message).toContain('Insufficient role'); }); - it('should deny unauthenticated user with hierarchy roles and no header', async () => { + it('should deny unauthenticated user with hierarchy roles and no header (401)', async () => { const res = await request(app.getHttpServer()) .get('/test/tenant-member') - .expect(403); + .expect(401); - expect(res.body.message).toContain('Authentication required'); + expect(res.body.message).toContain(ErrorCode.UNAUTHORIZED); }); it('should resolve filtered tenantIds when no header + hierarchy role', async () => { @@ -806,12 +807,12 @@ describe('CoreTenantGuard (e2e)', () => { it('should deny @SkipTenantCheck + non-system role when user is unauthenticated (defense-in-depth)', async () => { // No X-Test-User-Id → req.user is undefined - // skipWithUserRoleCheck: checkableRoles.length > 0 && !user → 403 'Authentication required' + // skipWithUserRoleCheck: checkableRoles.length > 0 && !user → 401 (re-auth is the remedy) const res = await request(app.getHttpServer()) .get('/test/skip-tenant-with-normal-role') - .expect(403); + .expect(401); - expect(res.body.message).toContain('Authentication required'); + expect(res.body.message).toContain(ErrorCode.UNAUTHORIZED); }); // ========================================================================= @@ -1037,14 +1038,15 @@ describe('CoreTenantGuard (e2e)', () => { expect(res.body.ok).toBe(true); }); - it('[regression] S_USER on method with ADMIN on class: unauthenticated user should get 403', async () => { + it('[regression] S_USER on method with ADMIN on class: unauthenticated user should get 401', async () => { // S_USER means "must be logged in" — unauthenticated access must be blocked. // With the corrected OR-semantics: S_USER is checked as an alternative BEFORE real roles. - // When S_USER is in the active role set and no user is present, the guard throws 403. + // When S_USER is in the active role set and no user is present, the guard throws 401: + // authenticating IS the remedy, so the client must be told to (RFC 9110). // (In production, RolesGuard would block unauthenticated requests earlier with 401.) await request(app.getHttpServer()) .get('/admin-fallback/user-only-no-auth') - .expect(403); + .expect(401); }); it('[regression] no @Roles on method with ADMIN on class: non-admin user should get 403', async () => { @@ -1161,12 +1163,12 @@ describe('CoreTenantGuard (e2e)', () => { expect(res.body.tenantId).toBeUndefined(); }); - // J4: S_USER on method + ADMIN on class → 403 for unauthenticated - it('S_USER on method + ADMIN on class: unauthenticated user gets 403', async () => { - // S_USER present, no user → throws ForbiddenException('Authentication required') + // J4: S_USER on method + ADMIN on class → 401 for unauthenticated + it('S_USER on method + ADMIN on class: unauthenticated user gets 401', async () => { + // S_USER present, no user → 401 (authenticating is the remedy, so the client must be told) await request(app.getHttpServer()) .get('/admin-fallback/user-only') - .expect(403); + .expect(401); }); // J5: S_USER on method + X-Tenant-Id header → 200 for tenant member @@ -1211,11 +1213,11 @@ describe('CoreTenantGuard (e2e)', () => { .expect(403); }); - // J9: S_VERIFIED on method → 403 for unauthenticated user - it('S_VERIFIED on method: unauthenticated user gets 403', async () => { + // J9: S_VERIFIED on method → 401 for unauthenticated user + it('S_VERIFIED on method: unauthenticated user gets 401', async () => { await request(app.getHttpServer()) .get('/test/verified-only') - .expect(403); + .expect(401); }); // J10: S_VERIFIED + X-Tenant-Id header → 200 for verified tenant member @@ -2634,10 +2636,13 @@ describe('CoreTenantMemberModel.securityCheck()', () => { expect(() => member.securityCheck(null, true)).not.toThrow(); }); - it('should deny access when no user is provided', () => { + it('should deny access when no user is provided (401 Unauthorized)', () => { const member = createMember(); - expect(() => member.securityCheck(null)).toThrow('Access to tenant membership denied'); - expect(() => member.securityCheck(undefined)).toThrow('Access to tenant membership denied'); + // No user at all → 401: authenticating IS the remedy, so the client must be told to. + for (const user of [null, undefined]) { + expect(() => member.securityCheck(user)).toThrow(UnauthorizedException); + expect(() => member.securityCheck(user)).toThrow(ErrorCode.UNAUTHORIZED); + } }); it('should allow access for the membership owner (same user)', () => { @@ -2686,7 +2691,10 @@ describe('CoreTenantMemberModel.securityCheck()', () => { ConfigService.setConfig({ multiTenancy: { roleHierarchy: DEFAULT_ROLE_HIERARCHY } } as any); try { RequestContext.run(context, () => { - expect(() => member.securityCheck(memberUser)).toThrow('Access to tenant membership denied'); + // Authenticated, but below manager level → 403, never 401 (a 401 would make the frontend + // treat a mere permission error as an expired session and log the user out). + expect(() => member.securityCheck(memberUser)).toThrow(ForbiddenException); + expect(() => member.securityCheck(memberUser)).toThrow(ErrorCode.ACCESS_DENIED); }); } finally { ConfigService.setConfig({ multiTenancy: {} } as any); @@ -2700,16 +2708,21 @@ describe('CoreTenantMemberModel.securityCheck()', () => { ConfigService.setConfig({ multiTenancy: { roleHierarchy: DEFAULT_ROLE_HIERARCHY } } as any); try { RequestContext.run(context, () => { - expect(() => member.securityCheck(otherUser)).toThrow('Access to tenant membership denied'); + // Authenticated manager, but of a DIFFERENT tenant → 403, not 401 + expect(() => member.securityCheck(otherUser)).toThrow(ForbiddenException); + expect(() => member.securityCheck(otherUser)).toThrow(ErrorCode.ACCESS_DENIED); }); } finally { ConfigService.setConfig({ multiTenancy: {} } as any); } }); - it('should deny access for unauthenticated user without tenant context', () => { + it('should deny an authenticated non-member without tenant context (403 Forbidden)', () => { const member = createMember(); - expect(() => member.securityCheck(memberUser)).toThrow('Access to tenant membership denied'); + // memberUser IS authenticated (it has an id) — it just is not the owner, not an admin and has + // no tenant context. That is a permission problem → 403. + expect(() => member.securityCheck(memberUser)).toThrow(ForbiddenException); + expect(() => member.securityCheck(memberUser)).toThrow(ErrorCode.ACCESS_DENIED); }); }); diff --git a/tests/unit/access-denied-exception.spec.ts b/tests/unit/access-denied-exception.spec.ts new file mode 100644 index 00000000..15c44818 --- /dev/null +++ b/tests/unit/access-denied-exception.spec.ts @@ -0,0 +1,84 @@ +import { ForbiddenException, HttpException, UnauthorizedException } from '@nestjs/common'; + +import { accessDeniedException } from '../../src/core/common/exceptions/access-denied.exception'; +import { ErrorCode } from '../../src/core/modules/error-code/error-codes'; + +/** + * accessDeniedException centralizes the 401/403 decision of the rights checks + * (check() / checkRestricted() / roles processing in prepareInput): 403 for + * authenticated requesters (permission problem), 401 only when unauthenticated. + * + * Two contracts must hold, and a previous implementation (a custom `HttpException` + * subclass) silently broke both: + * + * 1. TYPE IDENTITY — the result must BE a native ForbiddenException / + * UnauthorizedException, so `instanceof` checks and `@Catch(...)` filters in + * consuming projects keep firing. Asserting on status and body alone cannot + * detect a broken type — which is exactly how that regression slipped through. + * 2. WIRE FORMAT — `HttpExceptionLogFilter` serializes `{ ...exception }`, so the + * exception's own `name` is client-visible on every REST error. It must stay + * 'ForbiddenException' / 'UnauthorizedException'. + */ +describe('accessDeniedException', () => { + describe('authenticated requester → 403 Forbidden', () => { + it('is a native ForbiddenException (instanceof must hold for consumer @Catch filters)', () => { + const exception = accessDeniedException({ id: 'user-1' }); + + expect(exception).toBeInstanceOf(ForbiddenException); + expect(exception).toBeInstanceOf(HttpException); + expect(exception).not.toBeInstanceOf(UnauthorizedException); + expect(exception.getStatus()).toEqual(403); + }); + + it('defaults to the translatable ErrorCode the role guards throw', () => { + expect(accessDeniedException({ id: 'user-1' }).getResponse()).toEqual( + new ForbiddenException(ErrorCode.ACCESS_DENIED).getResponse(), + ); + }); + + it('keeps the REST wire format identical to the native exception', () => { + // HttpExceptionLogFilter spreads the exception, so `name` reaches the client + expect({ ...accessDeniedException({ id: 'user-1' }) }).toEqual({ + ...new ForbiddenException(ErrorCode.ACCESS_DENIED), + }); + }); + + it('treats falsy-but-present ids (0, empty string) as authenticated', () => { + // Consumer projects may use numeric ids; `!!user?.id` would hand them a 401 + for (const id of [0, '', false]) { + expect(accessDeniedException({ id }).getStatus()).toEqual(403); + } + }); + }); + + describe('unauthenticated requester → 401 Unauthorized', () => { + it('is a native UnauthorizedException for undefined, null and a user without id', () => { + for (const user of [undefined, null, {}, { id: undefined }, { id: null }]) { + const exception = accessDeniedException(user); + + expect(exception).toBeInstanceOf(UnauthorizedException); + expect(exception).not.toBeInstanceOf(ForbiddenException); + expect(exception.getStatus()).toEqual(401); + } + }); + + it('defaults to the translatable ErrorCode the role guards throw', () => { + expect(accessDeniedException(undefined).getResponse()).toEqual( + new UnauthorizedException(ErrorCode.UNAUTHORIZED).getResponse(), + ); + }); + + it('keeps the REST wire format identical to the native exception', () => { + expect({ ...accessDeniedException(undefined) }).toEqual({ ...new UnauthorizedException(ErrorCode.UNAUTHORIZED) }); + }); + }); + + it('accepts an explicit message override on both branches', () => { + expect(accessDeniedException({ id: 'user-1' }, 'Custom').getResponse()).toEqual( + new ForbiddenException('Custom').getResponse(), + ); + expect(accessDeniedException(undefined, 'Custom').getResponse()).toEqual( + new UnauthorizedException('Custom').getResponse(), + ); + }); +}); diff --git a/tests/unit/restricted-s-self.spec.ts b/tests/unit/restricted-s-self.spec.ts new file mode 100644 index 00000000..57039697 --- /dev/null +++ b/tests/unit/restricted-s-self.spec.ts @@ -0,0 +1,260 @@ +import { ForbiddenException } from '@nestjs/common'; + +import { Restricted, checkRestricted } from '../../src/core/common/decorators/restricted.decorator'; +import { ProcessType } from '../../src/core/common/enums/process-type.enum'; +import { RoleEnum } from '../../src/core/common/enums/role.enum'; + +/** + * Ownership roles (S_SELF, S_CREATOR) must be decided from the PERSISTED object, never from the + * request payload. + * + * The bug this file guards against: `checkRestricted()` read the ownership claim off `data` — and on + * the input path `data` IS the caller-supplied DTO. An authenticated attacker could unlock an + * owner-restricted field on someone ELSE's record just by asserting ownership in the body: + * + * PATCH /users/ { "id": "", "createdBy": "", "iban": "DE...attacker" } + * + * The service applies the input to the target it was called with (``), not to the ids in the + * body — so the guard passed while the write landed on the victim. Three reads were affected: + * S_SELF (`equalIds(data, user)`), S_CREATOR (`data.createdBy`), and the `isCreatorOfParent` flag, + * which propagated the forged trust down into nested objects. + * + * `check()` (input.helper.ts) always read both from `config.dbObject` and got this right. + * + * Every case below is asserted in BOTH directions: the attacker must be rejected AND the legitimate + * owner must still get through — a fix that simply denies everything would be no fix at all. + * + * The rejection is a `ForbiddenException` (403), not a 401: the attacker IS authenticated (they have + * an id) and merely lacks the right — see accessDeniedException. 401 is reserved for anonymous + * requesters, which never reach these ownership branches end-to-end (the guards stop them first). + */ + +/** Stands in for a consumer model: `iban` is owner-only, `note` is creator-only. */ +class Account { + createdBy?: string = undefined; + + id: string = undefined; + + @Restricted(RoleEnum.S_SELF) + iban?: string = undefined; + + @Restricted(RoleEnum.S_CREATOR) + note?: string = undefined; +} + +/** A nested object without its own `createdBy` — reaches S_CREATOR via `allowCreatorOfParent`. */ +class Settings { + @Restricted(RoleEnum.S_CREATOR) + secret?: string = undefined; +} + +class AccountWithSettings { + createdBy?: string = undefined; + + id: string = undefined; + + settings?: Settings = undefined; +} + +/** + * The shape consumers actually ship: a class-level role plus a field-level ownership role. + * `mergeRoles` (default: true) OR-merges them, so S_USER alone already grants the field — the + * ownership check never gates. Every real usage in our customer projects looks like this. + */ +@Restricted(RoleEnum.S_USER) +class SharedInput { + @Restricted(RoleEnum.S_SELF) + colour?: string = undefined; +} + +const ATTACKER = { hasRole: () => false, id: 'attacker-1' }; + +/** The victim's record, as it exists in the database. */ +function victimRecord(): Account { + return Object.assign(new Account(), { + createdBy: 'victim-1', + iban: 'DE00 VICTIM', + id: 'victim-1', + note: 'victim note', + }); +} + +/** The attacker's own record, as it exists in the database. */ +function ownRecord(): Account { + return Object.assign(new Account(), { + createdBy: 'attacker-1', + iban: 'DE00 OWN', + id: 'attacker-1', + note: 'own note', + }); +} + +const INPUT = { processType: ProcessType.INPUT, throwError: true } as const; +const OUTPUT = { processType: ProcessType.OUTPUT, throwError: false } as const; + +describe('checkRestricted() — ownership is read from the persisted object', () => { + describe('INPUT: S_SELF', () => { + it('rejects a forged id in the payload (attack)', () => { + const forged = Object.assign(new Account(), { iban: 'DE00 ATTACKER', id: 'attacker-1' }); + + expect(() => checkRestricted(forged, ATTACKER, { ...INPUT, dbObject: victimRecord() })).toThrow( + ForbiddenException, + ); + }); + + it('still lets the owner set the field on their own record (legitimate)', () => { + const input = Object.assign(new Account(), { iban: 'DE00 NEW' }); + + const result = checkRestricted(input, ATTACKER, { ...INPUT, dbObject: ownRecord() }); + + expect(result.iban).toEqual('DE00 NEW'); + }); + + it('does not need an id in the DTO — the persisted object decides', () => { + // The old implementation only ever passed when the DTO carried an id, which is why S_SELF was + // ALSO broken for honest callers: a normal update DTO has none. + const input = Object.assign(new Account(), { iban: 'DE00 NEW' }); + + expect(input.id).toBeUndefined(); + expect(checkRestricted(input, ATTACKER, { ...INPUT, dbObject: ownRecord() }).iban).toEqual('DE00 NEW'); + }); + + it('denies the field when no persisted object exists (create)', () => { + const input = Object.assign(new Account(), { iban: 'DE00 NEW', id: 'attacker-1' }); + + expect(() => checkRestricted(input, ATTACKER, { ...INPUT })).toThrow(ForbiddenException); + }); + }); + + describe('INPUT: S_CREATOR', () => { + it('rejects a forged createdBy in the payload (attack)', () => { + const forged = Object.assign(new Account(), { createdBy: 'attacker-1', note: 'hijacked' }); + + expect(() => checkRestricted(forged, ATTACKER, { ...INPUT, dbObject: victimRecord() })).toThrow( + ForbiddenException, + ); + }); + + it('still lets the creator set the field on the record they created (legitimate)', () => { + const input = Object.assign(new Account(), { note: 'my note' }); + + const result = checkRestricted(input, ATTACKER, { ...INPUT, dbObject: ownRecord() }); + + expect(result.note).toEqual('my note'); + }); + + it('denies the field when no persisted object exists (create)', () => { + const input = Object.assign(new Account(), { createdBy: 'attacker-1', note: 'x' }); + + expect(() => checkRestricted(input, ATTACKER, { ...INPUT })).toThrow(ForbiddenException); + }); + }); + + describe('INPUT: nested objects (isCreatorOfParent)', () => { + it('does not let a forged parent ownership claim unlock a nested field (attack)', () => { + // The nested Settings has no createdBy of its own, so it relies on the parent's ownership. + // A forged id/createdBy on the parent DTO must not grant that trust. + const forged = Object.assign(new AccountWithSettings(), { + createdBy: 'attacker-1', + id: 'attacker-1', + settings: Object.assign(new Settings(), { secret: 'hijacked' }), + }); + + expect(() => + checkRestricted(forged, ATTACKER, { + ...INPUT, + allowCreatorOfParent: true, + dbObject: victimRecord(), // the write really targets the victim + }), + ).toThrow(ForbiddenException); + }); + + it('still lets the parent’s creator write the nested field (legitimate)', () => { + const input = Object.assign(new AccountWithSettings(), { + settings: Object.assign(new Settings(), { secret: 'mine' }), + }); + + const result = checkRestricted(input, ATTACKER, { + ...INPUT, + allowCreatorOfParent: true, + dbObject: ownRecord(), + }); + + expect(result.settings.secret).toEqual('mine'); + }); + }); + + describe('INPUT: a class-level restriction is OR-merged and must keep working', () => { + // mergeRoles defaults to true and the roles are OR-ed, so a class-level @Restricted WIDENS the + // field-level one rather than narrowing it. Every real consumer usage of S_SELF/S_CREATOR sits on + // a class that also carries S_USER or ADMIN, which is why they pass without a dbObject at all. + // Pinned here so the ownership fix provably does not tighten those paths. + it('lets any authenticated user through when the class adds S_USER — even without a dbObject', () => { + const input = Object.assign(new SharedInput(), { colour: 'red' }); + + const result = checkRestricted(input, ATTACKER, { ...INPUT }); + + expect(result.colour).toEqual('red'); + }); + + it('and still lets them through on an update targeting a foreign record', () => { + const input = Object.assign(new SharedInput(), { colour: 'red' }); + + const result = checkRestricted(input, ATTACKER, { ...INPUT, dbObject: victimRecord() }); + + // Not a hole this fix opens: S_USER alone already grants the field. Consumers who mean + // "only the owner" must NOT put a broader role on the class. + expect(result.colour).toEqual('red'); + }); + }); + + describe('OUTPUT: semantics must not change (data IS the persisted object here)', () => { + it('keeps owner- and creator-restricted fields on the requester’s own record', () => { + const result = checkRestricted(ownRecord(), ATTACKER, { ...OUTPUT }); + + expect(result.iban).toEqual('DE00 OWN'); + expect(result.note).toEqual('own note'); + }); + + it('strips owner- and creator-restricted fields from a foreign record', () => { + const result = checkRestricted(victimRecord(), ATTACKER, { ...OUTPUT }); + + expect(result.iban).toBeUndefined(); + expect(result.note).toBeUndefined(); + expect(result.id).toEqual('victim-1'); // unrestricted fields survive + }); + + it('decides per item in a list, not once for the whole response', () => { + const result = checkRestricted([ownRecord(), victimRecord()], ATTACKER, { ...OUTPUT }); + + expect(result[0].iban).toEqual('DE00 OWN'); + expect(result[1].iban).toBeUndefined(); + }); + + it('is not overridden by a dbObject that happens to be set', () => { + // On output the per-item object must win — a dbObject must never widen the response. + const result = checkRestricted(victimRecord(), ATTACKER, { ...OUTPUT, dbObject: ownRecord() }); + + expect(result.iban).toBeUndefined(); + expect(result.note).toBeUndefined(); + }); + + it('resolves a nested creator-restricted field via the parent (allowCreatorOfParent)', () => { + const own = Object.assign(new AccountWithSettings(), { + createdBy: 'attacker-1', + id: 'attacker-1', + settings: Object.assign(new Settings(), { secret: 'mine' }), + }); + const foreign = Object.assign(new AccountWithSettings(), { + createdBy: 'victim-1', + id: 'victim-1', + settings: Object.assign(new Settings(), { secret: 'not yours' }), + }); + + expect(checkRestricted(own, ATTACKER, { ...OUTPUT, allowCreatorOfParent: true }).settings.secret).toEqual('mine'); + expect( + checkRestricted(foreign, ATTACKER, { ...OUTPUT, allowCreatorOfParent: true }).settings.secret, + ).toBeUndefined(); + }); + }); +});