diff --git a/src/__tests__/api-errors-leaf-guard.test.ts b/src/__tests__/api-errors-leaf-guard.test.ts new file mode 100644 index 000000000..053e9c46d --- /dev/null +++ b/src/__tests__/api-errors-leaf-guard.test.ts @@ -0,0 +1,72 @@ +/** + * The error classes live in ONE leaf module — the cycle stays broken. + * + * `api-handler.ts` imports the record-session fence, and the fence throws + * `RecordSessionChangedError` / `SharingAccessDeniedError`. When those + * classes lived inside api-handler the two modules imported each other and + * the loop worked only by evaluation-order luck. This guard pins the split: + * + * 1. identity — the class an importer gets from `api-handler` IS the class + * the fence throws (from `api-errors`), so `instanceof` across the two + * import paths can never diverge; + * 2. structure — the fence carries no VALUE import from api-handler + * (a type-only import is erased and allowed), and the leaf imports + * nothing but the fence contract. + * + * Watched red: re-declaring `HttpError` inside api-handler (the pre-split + * shape) fails the identity leg; adding a value import of api-handler back + * into the fence fails the structural leg. + */ +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; + +import * as apiErrors from "@/lib/api-errors"; +import { + HttpError, + RecordSessionChangedError, + SharingAccessDeniedError, + SharingAuthError, + SharingNotPermittedError, + StepUpRequiredError, +} from "@/lib/api-handler"; + +const SRC = join(__dirname, ".."); + +describe("api-errors leaf module", () => { + it("api-handler re-exports the identical class objects", () => { + expect(HttpError).toBe(apiErrors.HttpError); + expect(SharingAuthError).toBe(apiErrors.SharingAuthError); + expect(SharingAccessDeniedError).toBe(apiErrors.SharingAccessDeniedError); + expect(SharingNotPermittedError).toBe(apiErrors.SharingNotPermittedError); + expect(RecordSessionChangedError).toBe(apiErrors.RecordSessionChangedError); + expect(StepUpRequiredError).toBe(apiErrors.StepUpRequiredError); + }); + + it("the record-session fence carries no value import from api-handler", () => { + const source = readFileSync( + join(SRC, "lib/sharing/record-session-fence.ts"), + "utf8", + ); + const imports = source.match(/^import[^;]+from "@\/lib\/api-handler";/gms); + for (const statement of imports ?? []) { + expect( + statement.startsWith("import type"), + `record-session-fence.ts imports a VALUE from api-handler — that is the cycle the api-errors leaf removed:\n${statement}`, + ).toBe(true); + } + }); + + it("the leaf imports nothing but the fence contract", () => { + const source = readFileSync(join(SRC, "lib/api-errors.ts"), "utf8"); + const imports = source.match(/^import[^;]+;/gms) ?? []; + // Self-check: the walker found the one expected import. + expect(imports.length).toBeGreaterThan(0); + for (const statement of imports) { + expect( + statement.includes("record-session-fence-contract"), + `api-errors.ts must stay a leaf; unexpected import:\n${statement}`, + ).toBe(true); + } + }); +}); diff --git a/src/lib/api-errors.ts b/src/lib/api-errors.ts new file mode 100644 index 000000000..58597c667 --- /dev/null +++ b/src/lib/api-errors.ts @@ -0,0 +1,131 @@ +/** + * Error classes for `apiHandler`-wrapped routes — a LEAF module. + * + * Moved out of `api-handler.ts` to break the one real import cycle in the + * tree: api-handler imports the record-session fence, and the fence threw + * `RecordSessionChangedError` / `SharingAccessDeniedError` imported back + * from api-handler — a loop that only worked by evaluation-order luck. + * This module imports nothing but the fence CONTRACT constants (itself a + * leaf), so anything may import it. `api-handler.ts` re-exports every + * class, so the ~500 existing importers read exactly as before. + */ +import { RECORD_FENCE_ERROR_CODE } from "./sharing/record-session-fence-contract"; + +/** + * Custom error class for HTTP errors with status codes. + * Throw inside apiHandler-wrapped routes to return a JSON error response. + */ +export class HttpError extends Error { + constructor( + public statusCode: number, + message: string, + ) { + super(message); + this.name = "HttpError"; + } +} + +/** + * v1.36.0 — the two refusals the acting-account resolver can raise. + * + * Both are 403 with a stable `meta.errorCode`, the same envelope shape the + * step-up and consent gates already use, so a client branches on the code + * rather than on prose. + */ +export abstract class SharingAuthError extends HttpError { + abstract readonly errorCode: string; +} + +/** + * "You may not act as that account." + * + * Deliberately parameterless. The message is a fixed literal and the + * constructor takes no arguments, so there is no way for a caller to vary the + * response — which is what makes a selector naming an account that does not + * exist and one naming an account that granted nothing produce the SAME BYTES. + * That is not a formatting nicety: a distinguishable refusal turns this + * endpoint into an account-enumeration oracle for anyone with a login. + * + * The indistinguishability is structural rather than careful, too. Neither + * branch ever looks an account up: the resolver asks the grant table for the + * (owner, delegate) pair and a missing account and a missing grant are the + * same empty result, on the same code path, after the same one query. + * + * Why the reason is not in the response at all: the caller already knows what + * they sent, and the only party who benefits from knowing WHICH condition + * failed is someone probing. The reason goes to the audit row and the wide + * event, where the owner and the operator can see it. + */ +export class SharingAccessDeniedError extends SharingAuthError { + readonly errorCode = "sharing.access.denied"; + constructor() { + super(403, "Account access denied"); + this.name = "SharingAccessDeniedError"; + } +} + +/** + * "This endpoint does not act on another account's record." + * + * A different fact from the one above and so a different code: nothing here is + * about whether a grant exists. The route simply never declared that it can be + * used under a switch, so it refuses rather than quietly serving the caller's + * OWN record — which is the reverse data-mixing failure, and the reason this + * class exists at all. A delegate who believes they are reading the owner's + * record must never be handed their own instead. + */ +export class SharingNotPermittedError extends SharingAuthError { + readonly errorCode = "sharing.not_permitted"; + constructor() { + super(403, "This endpoint cannot be used while acting on another account"); + this.name = "SharingNotPermittedError"; + } +} + +/** + * v1.37.0 — "the record this session is on is not the record you asserted." + * + * A third sibling rather than a branch, so it rides the existing + * `SharingAuthError` arm with no new serialisation code. The status is + * the one thing that differs and it differs on purpose: 409, not 403, because + * this is not a refusal of access. The caller may well be entitled to both + * records. What failed is agreement about WHICH one, and the instruction the + * code carries is "reconcile through `/api/auth/me` and try again", not "you + * have lost this record". + * + * That distinction is load-bearing for deploy compatibility: the currently + * shipped bundle reacts to `sharing.access.denied` by leaving the record and + * hard-navigating, which is exactly the wrong response to a transient + * disagreement and exactly the right one for a bundle too old to reconcile. See + * `src/lib/sharing/record-session-fence-contract.ts` for why the fence hands + * each bundle the code it can act on. + * + * The body carries `meta.errorCode` and nothing else — no current epoch, no + * current scope. A client may learn its context from exactly two responses + * (`GET /api/auth/me` and `POST /api/account/switch`), and putting the truth in + * this refusal would open a third path, one that arrives on the very requests + * whose context is in doubt. + */ +export class RecordSessionChangedError extends SharingAuthError { + readonly errorCode = RECORD_FENCE_ERROR_CODE; + constructor() { + super(409, "The record this session is on has changed"); + this.name = "RecordSessionChangedError"; + } +} + +/** + * Error thrown when a step-up gate is not satisfied. Carries `errorCode` so + * the route can surface a stable machine code (`auth.stepup.required`) the + * client branches on to launch a re-verification flow rather than parsing + * prose. + */ +export class StepUpRequiredError extends HttpError { + constructor( + public errorCode: string = "auth.stepup.required", + message = "Recent second-factor verification required", + ) { + super(401, message); + this.name = "StepUpRequiredError"; + } +} diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index 9d91d6d47..d319a36de 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -37,7 +37,6 @@ import { type GrantNeed, } from "./sharing/grants"; import type { ShareScope } from "./sharing/scope"; -import { RECORD_FENCE_ERROR_CODE } from "./sharing/record-session-fence-contract"; import { assertRecordSessionFence, attachRecordContextEcho, @@ -73,108 +72,26 @@ export function isMcpAudienceToken(permissions: readonly string[]): boolean { ); } -/** - * Custom error class for HTTP errors with status codes. - * Throw inside apiHandler-wrapped routes to return a JSON error response. - */ -export class HttpError extends Error { - constructor( - public statusCode: number, - message: string, - ) { - super(message); - this.name = "HttpError"; - } -} +// The error classes live in the LEAF module `api-errors.ts` (moved to break +// the api-handler ↔ record-session-fence import cycle) and are re-exported +// here so the hundreds of existing importers read exactly as before. +import { + HttpError, + SharingAuthError, + SharingAccessDeniedError, + SharingNotPermittedError, + RecordSessionChangedError, + StepUpRequiredError, +} from "./api-errors"; -/** - * v1.36.0 — the two refusals the acting-account resolver can raise. - * - * Both are 403 with a stable `meta.errorCode`, the same envelope shape the - * step-up and consent gates already use, so a client branches on the code - * rather than on prose. - */ -export abstract class SharingAuthError extends HttpError { - abstract readonly errorCode: string; -} - -/** - * "You may not act as that account." - * - * Deliberately parameterless. The message is a fixed literal and the - * constructor takes no arguments, so there is no way for a caller to vary the - * response — which is what makes a selector naming an account that does not - * exist and one naming an account that granted nothing produce the SAME BYTES. - * That is not a formatting nicety: a distinguishable refusal turns this - * endpoint into an account-enumeration oracle for anyone with a login. - * - * The indistinguishability is structural rather than careful, too. Neither - * branch ever looks an account up: the resolver asks the grant table for the - * (owner, delegate) pair and a missing account and a missing grant are the - * same empty result, on the same code path, after the same one query. - * - * Why the reason is not in the response at all: the caller already knows what - * they sent, and the only party who benefits from knowing WHICH condition - * failed is someone probing. The reason goes to the audit row and the wide - * event, where the owner and the operator can see it. - */ -export class SharingAccessDeniedError extends SharingAuthError { - readonly errorCode = "sharing.access.denied"; - constructor() { - super(403, "Account access denied"); - this.name = "SharingAccessDeniedError"; - } -} - -/** - * "This endpoint does not act on another account's record." - * - * A different fact from the one above and so a different code: nothing here is - * about whether a grant exists. The route simply never declared that it can be - * used under a switch, so it refuses rather than quietly serving the caller's - * OWN record — which is the reverse data-mixing failure, and the reason this - * class exists at all. A delegate who believes they are reading the owner's - * record must never be handed their own instead. - */ -export class SharingNotPermittedError extends SharingAuthError { - readonly errorCode = "sharing.not_permitted"; - constructor() { - super(403, "This endpoint cannot be used while acting on another account"); - this.name = "SharingNotPermittedError"; - } -} - -/** - * v1.37.0 — "the record this session is on is not the record you asserted." - * - * A third sibling rather than a branch, so it rides the existing - * `SharingAuthError` arm below with no new serialisation code. The status is - * the one thing that differs and it differs on purpose: 409, not 403, because - * this is not a refusal of access. The caller may well be entitled to both - * records. What failed is agreement about WHICH one, and the instruction the - * code carries is "reconcile through `/api/auth/me` and try again", not "you - * have lost this record". - * - * That distinction is load-bearing for deploy compatibility: the currently - * shipped bundle reacts to `sharing.access.denied` by leaving the record and - * hard-navigating, which is exactly the wrong response to a transient - * disagreement and exactly the right one for a bundle too old to reconcile. See - * `src/lib/sharing/record-session-fence-contract.ts` for why the fence hands - * each bundle the code it can act on. - * - * The body carries `meta.errorCode` and nothing else — no current epoch, no - * current scope. A client may learn its context from exactly two responses - * (`GET /api/auth/me` and `POST /api/account/switch`), and putting the truth in - * this refusal would open a third path, one that arrives on the very requests - * whose context is in doubt. - */ -export class RecordSessionChangedError extends SharingAuthError { - readonly errorCode = RECORD_FENCE_ERROR_CODE; - constructor() { - super(409, "The record this session is on has changed"); - this.name = "RecordSessionChangedError"; - } -} +export { + HttpError, + SharingAuthError, + SharingAccessDeniedError, + SharingNotPermittedError, + RecordSessionChangedError, + StepUpRequiredError, +}; /** * Wraps an API route handler with Wide Event logging. @@ -1119,22 +1036,6 @@ export async function requireCookieAuth(): Promise { return { ...sessionData, authMethod: "cookie" }; } -/** - * Error thrown when a step-up gate is not satisfied. Carries `errorCode` so - * the route can surface a stable machine code (`auth.stepup.required`) the - * client branches on to launch a re-verification flow rather than parsing - * prose. - */ -export class StepUpRequiredError extends HttpError { - constructor( - public errorCode: string = "auth.stepup.required", - message = "Recent second-factor verification required", - ) { - super(401, message); - this.name = "StepUpRequiredError"; - } -} - /** * Default step-up freshness window (5 minutes) for sensitive mutations. * Within the 5–15 min band OWASP recommends; tight end because the gated diff --git a/src/lib/sharing/record-session-fence.ts b/src/lib/sharing/record-session-fence.ts index 55a11c2a9..9fe513050 100644 --- a/src/lib/sharing/record-session-fence.ts +++ b/src/lib/sharing/record-session-fence.ts @@ -51,11 +51,15 @@ import { recordSessionFenceVerdict, type AssertedRecordContext, } from "@/lib/sharing/record-session-fence-contract"; +// Runtime error classes come from the LEAF module — importing them from +// api-handler recreated the import cycle this split removed. The +// `AuthContext` type import stays on api-handler: type-only, erased at +// runtime, no cycle. import { RecordSessionChangedError, SharingAccessDeniedError, - type AuthContext, -} from "@/lib/api-handler"; +} from "@/lib/api-errors"; +import type { AuthContext } from "@/lib/api-handler"; import { annotate, getEvent } from "@/lib/logging/context"; /**