From f24ae6deb8580b760f811efff4045fb7bff569e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Mon, 3 Aug 2026 10:13:55 +0200 Subject: [PATCH 1/2] Read the acting-account carrier from one place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rule that decides what a request says it is acting as — session stamp on the cookie transport, selector header on Bearer, a header on a cookie request naming nothing — sat inside the resolver that acts on the answer. A second caller needs the same question answered before any auth context exists, and two implementations of "whose record is this" drift; the copy that drifts is the one nobody reads. Move the decision, the header name and the two predicates into `auth/acting-carrier`, as a pure function of what the transport carried. The resolver keeps what only it may do: turning a claim into a data scope once a live grant says so. No behaviour changes — the resolver's arms are the same arms, and its suite passes untouched. The header-name guard follows the constant to its new file. --- .../acting-account-boundary-guard.test.ts | 2 +- src/lib/api-handler.ts | 108 ++++-------------- src/lib/auth/acting-carrier.ts | 105 +++++++++++++++++ 3 files changed, 130 insertions(+), 85 deletions(-) create mode 100644 src/lib/auth/acting-carrier.ts diff --git a/src/__tests__/acting-account-boundary-guard.test.ts b/src/__tests__/acting-account-boundary-guard.test.ts index 1b01ae55f..c842443b8 100644 --- a/src/__tests__/acting-account-boundary-guard.test.ts +++ b/src/__tests__/acting-account-boundary-guard.test.ts @@ -113,7 +113,7 @@ describe("the acting-account carrier is read in one place", () => { const namers = filesMatching(/x-healthlog-account/i); expect(namers.length).toBeGreaterThan(0); expect(namers).toEqual([ - "lib/api-handler.ts", + "lib/auth/acting-carrier.ts", "lib/openapi/routes/account-sharing.ts", ]); }); diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index e4899cbcb..187b362f5 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -19,6 +19,13 @@ import { STEP_UP_ELEVATION_TTL_SECONDS, } from "./auth/step-up"; import { hashToken } from "./auth/hmac"; +import { + carrierTarget, + decideActingCarrier, + readSelectorHeader, + selectorNamesAnAccount, + type ActingCarrier, +} from "./auth/acting-carrier"; import { AssistantDisabledError } from "./feature-flags"; import { ConsentRequiredError } from "./ai/consent-guard"; import { SCOPE_HEALTH_READ, SCOPE_HEALTH_WRITE } from "./mcp/oauth/config"; @@ -597,69 +604,23 @@ async function authenticateBearer( // // One function decides, for every transport, whether this request acts on // somebody else's record — the same argument that put `resolveBearerToken` in -// one file for two wires. Two carriers converge here because they are two -// answers to one question, and two answers that live apart drift. +// one file for two wires. Two carriers converge on one decision because they +// are two answers to one question, and two answers that live apart drift. // -// Neither carrier authorises anything. Both merely NAME an account; the grant -// table decides, on this request, with no cached verdict anywhere. The -// difference between them is who can set them: -// -// * The cookie transport carries it on the session ROW (`actingAsUserId`). -// A browser cannot address that row — the cookie holds a secret whose hash -// is the lookup key — so the value is server state, written only by the -// switch endpoint after it has validated the grant. -// * The Bearer transport carries it in a per-request HEADER. A long-lived -// token must not accumulate switch state: stamping it on the ApiToken row -// would make the credential itself ambient authority, and the token has to -// keep meaning "this person" for as long as it exists. -// -// The header is a Bearer-only carrier. On a cookie request it is refused, not -// ignored — a client that sends a selector over a transport that does not read -// one believes it is selecting an account and is not, and the failure of that -// belief is a support case about data that looks wrong. +// That decision now lives in `./auth/acting-carrier`, because a second caller +// needs it: the idempotency wrapper files its replay cell under the record a +// request claims, and it runs before any auth context exists. What stays here +// is what only this file may do — turning a claim into a data scope after a +// live grant says so. /** - * The per-request account selector, on the Bearer transport only. + * The selector header's name, re-exported. * - * Cross-origin JavaScript cannot set it: a custom request header forces a CORS - * preflight and this app sends no CORS headers at all, so the preflight is - * never answered. It is a selector regardless — the grant table is what - * authorises, and the header is treated as hostile input everywhere below. + * It is declared beside the carrier reader it belongs to; every route-facing + * helper is imported from this module, and the OpenAPI table and the suites + * that drive a switched request were written against that path. */ -export const ACCOUNT_SELECTOR_HEADER = "x-healthlog-account"; - -/** - * Longest selector value worth a database round trip. Account ids are 25-char - * cuids; anything past this names no account that could exist and is refused - * as such, on the same path and with the same bytes as an unknown account. - */ -const MAX_SELECTOR_LENGTH = 64; - -/** - * What, if anything, this request says it is acting as. - * - * `misplaced-header` is its own case rather than an error thrown at read time - * so that every mode below decides for itself — the modes disagree about the - * session carrier and must not disagree about this one by accident. - */ -type ActingCarrier = - | { kind: "none" } - | { kind: "session"; accountId: string } - | { kind: "header"; accountId: string } - | { kind: "misplaced-header" }; - -/** The selector header, or null when the request did not send one. */ -async function readSelectorHeader(): Promise { - try { - const headerList = await headers(); - return headerList.get(ACCOUNT_SELECTOR_HEADER); - } catch { - // Outside a Next.js request scope (direct unit invocation of a legacy - // route) there is no header to read. Same posture as the Bearer branch of - // `authenticateCaller`. - return null; - } -} +export { ACCOUNT_SELECTOR_HEADER } from "./auth/acting-carrier"; /** * The acting-account carrier for this request. @@ -677,32 +638,11 @@ async function readSelectorHeader(): Promise { * next request rather than from the next login. */ async function readActingCarrier(auth: AuthContext): Promise { - const header = await readSelectorHeader(); - - if (auth.authMethod === "bearer") { - // No session row exists on this transport, so the header is the only - // carrier there could be. - return header === null - ? { kind: "none" } - : { kind: "header", accountId: header }; - } - - if (header !== null) return { kind: "misplaced-header" }; - - const stamped = auth.session.actingAsUserId ?? null; - return stamped === null - ? { kind: "none" } - : { kind: "session", accountId: stamped }; -} - -/** Could this value name an account at all? */ -function selectorNamesAnAccount(value: string): boolean { - return value.length > 0 && value.length <= MAX_SELECTOR_LENGTH; -} - -/** The account a carrier names, or null when it names nothing. */ -function carrierTarget(carrier: ActingCarrier): string | null { - return "accountId" in carrier ? carrier.accountId : null; + return decideActingCarrier({ + transport: auth.authMethod === "bearer" ? "bearer" : "cookie", + stamped: auth.session.actingAsUserId ?? null, + header: await readSelectorHeader(), + }); } /** diff --git a/src/lib/auth/acting-carrier.ts b/src/lib/auth/acting-carrier.ts new file mode 100644 index 000000000..9be1e116d --- /dev/null +++ b/src/lib/auth/acting-carrier.ts @@ -0,0 +1,105 @@ +/** + * What a request says it is acting as. + * + * It lives apart from `requireRecordAuth`, which acts on the answer, because + * asking the question and deciding what it permits are different jobs and the + * asking is about to have more than one caller. + * + * The claim is a SELECTOR and never an authorisation. Nothing in this file + * looks at a grant, and no caller may treat a carrier as permission: the + * account named here is an id to be checked. What it decides is only which + * account is being named, and by which transport — the question `grants.ts` + * warns must have exactly one answer, because a second implementation of it + * drifts and the copy that drifts is the one nobody reads. + * + * The transport rules, unchanged from where they were first written: + * + * * The cookie transport carries the claim on the session ROW. A browser + * cannot address that row, so the value is server state written only by + * the switch endpoint after it validated a grant. + * * The Bearer transport carries it in a per-request HEADER. A long-lived + * token must not accumulate switch state. + * * The header on a cookie request is `misplaced`, not ignored — a client + * that believes it is selecting an account and is not produces a support + * case about data that looks wrong. + */ +import { headers } from "next/headers"; + +/** + * The per-request account selector, on the Bearer transport only. + * + * Cross-origin JavaScript cannot set it: a custom request header forces a CORS + * preflight and this app sends no CORS headers at all, so the preflight is + * never answered. It is a selector regardless — the grant table is what + * authorises, and the header is treated as hostile input everywhere it is read. + */ +export const ACCOUNT_SELECTOR_HEADER = "x-healthlog-account"; + +/** + * Longest selector value worth a database round trip. Account ids are 25-char + * cuids; anything past this names no account that could exist and is refused + * as such, on the same path and with the same bytes as an unknown account. + */ +export const MAX_SELECTOR_LENGTH = 64; + +/** + * What, if anything, this request says it is acting as. + * + * `misplaced-header` is its own case rather than an error thrown at read time + * so that every caller decides for itself — the callers disagree about what to + * do with a misplaced claim and must not disagree about detecting one. + */ +export type ActingCarrier = + | { kind: "none" } + | { kind: "session"; accountId: string } + | { kind: "header"; accountId: string } + | { kind: "misplaced-header" }; + +/** The selector header, or null when the request did not send one. */ +export async function readSelectorHeader(): Promise { + try { + const headerList = await headers(); + return headerList.get(ACCOUNT_SELECTOR_HEADER); + } catch { + // Outside a Next.js request scope (direct unit invocation of a legacy + // route) there is no header to read. Same posture as the Bearer branch of + // `authenticateCaller`. + return null; + } +} + +/** + * The carrier decision itself, as a pure function of what the transport + * carried. Pure on purpose: it is the one statement of the rule, and a rule + * that needs a request context to be read is a rule that gets re-implemented. + */ +export function decideActingCarrier(input: { + transport: "cookie" | "bearer"; + /** The session row's stamp. Ignored on the Bearer transport, which has none. */ + stamped: string | null; + header: string | null; +}): ActingCarrier { + if (input.transport === "bearer") { + // No session row exists on this transport, so the header is the only + // carrier there could be. + return input.header === null + ? { kind: "none" } + : { kind: "header", accountId: input.header }; + } + + if (input.header !== null) return { kind: "misplaced-header" }; + + return input.stamped === null + ? { kind: "none" } + : { kind: "session", accountId: input.stamped }; +} + +/** The account a carrier names, or null when it names nothing. */ +export function carrierTarget(carrier: ActingCarrier): string | null { + return "accountId" in carrier ? carrier.accountId : null; +} + +/** Could this value name an account at all? */ +export function selectorNamesAnAccount(value: string): boolean { + return value.length > 0 && value.length <= MAX_SELECTOR_LENGTH; +} From c7960d726d964e119b8b4da7d6cd9b36a52de5cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Mon, 3 Aug 2026 10:18:27 +0200 Subject: [PATCH 2/2] Scope the idempotency replay cell to the record a request claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A delegate retrying with one `Idempotency-Key` across two people's records met itself in one cell. The cache tuple was `(caller, key, method, path)`, so the second record's request replayed the first record's cached 201: the handler never ran, no row was written for the second person, and the caller was told it had been — with the other person's id in the body. Nothing errored and nothing logged a conflict, and five of the write verbs about to become delegable are wrapped here. Fold the claimed record into the `key` component, centrally, once. Not into `userId`: that column is a foreign key to `users` with a cascade behind it and cannot hold a composite. It keeps naming the caller, which is what makes two delegates on one record two cells for free. The separator is a byte the key validator refuses, so a caller cannot hand-craft a key into a cell belonging to a request they are not making. A request with no acting account keys byte-for-byte as it did before, so a client that never heard of sharing cannot be affected. Accepted and pinned by test: the fold uses the CLAIM, before the grant check, so within the 24h window a delegate whose access has ended can still replay a key they had already completed. They receive bytes they already had, nothing new is written, and a fresh key from them is refused because that request reaches the handler. Both frozen surfaces move in this diff. The claim reader gains a guard leg freezing its consumers, since an unchecked claim is exactly the value a future caller must not mistake for permission. --- .../acting-account-boundary-guard.test.ts | 25 ++ .../bearer-scope-enforcement-guard.test.ts | 10 +- .../__tests__/idempotency-conflict.test.ts | 9 + src/lib/__tests__/idempotency.test.ts | 139 ++++++++- src/lib/auth/acting-carrier.ts | 55 +++- src/lib/idempotency.ts | 66 ++++- .../idempotency-record-scoped-cell.test.ts | 267 ++++++++++++++++++ 7 files changed, 563 insertions(+), 8 deletions(-) create mode 100644 tests/integration/idempotency-record-scoped-cell.test.ts diff --git a/src/__tests__/acting-account-boundary-guard.test.ts b/src/__tests__/acting-account-boundary-guard.test.ts index c842443b8..bf83b9a2b 100644 --- a/src/__tests__/acting-account-boundary-guard.test.ts +++ b/src/__tests__/acting-account-boundary-guard.test.ts @@ -74,6 +74,9 @@ describe("the acting-account carrier is read in one place", () => { const CARRIER_ALLOWLIST = [ // Projects it off the session row it already loaded. "lib/auth/session.ts", + // Reads it. The one statement of "what does this request say it is acting + // as", for both transports and for every caller that has to ask. + "lib/auth/acting-carrier.ts", // The resolver. The only thing that acts on the value. "lib/api-handler.ts", // v1.36.0 — the only thing that WRITES it: the switch endpoint sets it @@ -117,6 +120,28 @@ describe("the acting-account carrier is read in one place", () => { "lib/openapi/routes/account-sharing.ts", ]); }); + + /** + * Who may ask "which record does this request claim" without an auth + * context in hand. + * + * `readClaimedActingAccount()` answers before any grant check has run, so a + * caller that treats its answer as permission has skipped the only thing + * that grants any. Exactly one caller exists, beside the file that declares + * it: the idempotency wrapper, which uses the claim to pick a cache cell and + * leaves the check to the handler it wraps. Everything else that needs the + * acting account has an auth context by then and goes through + * `requireRecordAuth`, which checks a live grant before returning. A second + * caller here is a design question, not a patch. + */ + it("the unchecked claim has exactly two consumers", () => { + const consumers = filesMatching(/readClaimedActingAccount/); + expect(consumers.length).toBeGreaterThan(0); + expect(consumers).toEqual([ + "lib/auth/acting-carrier.ts", + "lib/idempotency.ts", + ]); + }); }); describe("the cookie-only helpers cannot see the acting account", () => { diff --git a/src/__tests__/bearer-scope-enforcement-guard.test.ts b/src/__tests__/bearer-scope-enforcement-guard.test.ts index ffd5175ee..669ed5020 100644 --- a/src/__tests__/bearer-scope-enforcement-guard.test.ts +++ b/src/__tests__/bearer-scope-enforcement-guard.test.ts @@ -65,9 +65,13 @@ describe("T1 — the Bearer resolution set is frozen", () => { // `defaultUserIdResolver` — resolves the Bearer token to a user id so an // idempotency key files under its owner. It authorises nothing: the // handler still runs its own `requireAuth()`, and this resolver only - // decides which user's cache cell a replay lands in. Narrow in reach and - // deliberate, but it is a raw Bearer value becoming a user id, so it - // belongs on this list rather than beside it. + // decides which cache cell a replay lands in. Since delegated writes it + // decides that from two values — the caller resolved here, and the record + // the request claims, read through the one acting-account carrier reader + // and folded into the key. Both are unchecked selectors; neither widens + // what the handler will do. Narrow in reach and deliberate, but it is a + // raw Bearer value becoming a user id, so it belongs on this list rather + // than beside it. "lib/idempotency.ts", ].sort(); diff --git a/src/app/api/anamnesis/facts/__tests__/idempotency-conflict.test.ts b/src/app/api/anamnesis/facts/__tests__/idempotency-conflict.test.ts index 435f35ef2..adb31bbdc 100644 --- a/src/app/api/anamnesis/facts/__tests__/idempotency-conflict.test.ts +++ b/src/app/api/anamnesis/facts/__tests__/idempotency-conflict.test.ts @@ -75,7 +75,16 @@ function request(): NextRequest { beforeEach(() => { vi.clearAllMocks(); + // The whole shape `getSession()` returns. The session half carries the + // acting-account carrier, which the idempotency wrapper reads to decide + // whose record a cell belongs to — a fake that omitted it made the wrapper + // fail on a field production always has. mocks.getSession.mockResolvedValue({ + session: { + id: "session-1", + expiresAt: new Date(Date.now() + 60_000), + actingAsUserId: null, + }, user: { id: "user-1", role: "USER" }, }); mocks.findUnique.mockResolvedValue(null); diff --git a/src/lib/__tests__/idempotency.test.ts b/src/lib/__tests__/idempotency.test.ts index 5d15ba7db..f5cce86e6 100644 --- a/src/lib/__tests__/idempotency.test.ts +++ b/src/lib/__tests__/idempotency.test.ts @@ -219,7 +219,11 @@ describe("withIdempotency", () => { ); const wrapped = withIdempotency<[NextRequest]>(handler); await wrapped(makeRequest("POST", { "idempotency-key": "abc-12345678" })); - expect(getSession).toHaveBeenCalledTimes(1); + // Twice: once to resolve who is calling, once to read which record they + // say they are acting on. The second read is what a delegated write costs + // here, and it is a single indexed lookup on a request that is about to + // write anyway. + expect(getSession).toHaveBeenCalledTimes(2); expect(prisma.idempotencyKey.create).toHaveBeenCalledTimes(1); const persisted = vi.mocked(prisma.idempotencyKey.create).mock .calls[0][0] as { data: { userId: string } }; @@ -536,3 +540,136 @@ describe("isCachableStatus do-not-cache rules (V3 audit)", () => { expect(stored.length).toBeGreaterThan(0); }); }); + +/** + * The cell is scoped to the record the request claims, not only to the caller. + * + * `tests/integration/idempotency-record-scoped-cell.test.ts` proves the rows. + * These prove the key the wrapper composes, which is what decides whether two + * requests meet in one cell — including the two cases a row-level test cannot + * show, because both end in no row at all. + */ +describe("withIdempotency — record-scoped cells", () => { + const CLIENT_KEY = "abc-12345678"; + + /** Present these headers to the wrapper for the next call. */ + function present(values: Record): void { + vi.mocked(headers).mockResolvedValue({ + get: (name: string) => values[name.toLowerCase()] ?? null, + } as unknown as ReturnType extends Promise + ? T + : never); + } + + /** Sign the caller in over the cookie transport, optionally switched. */ + function cookieSession(actingAsUserId: string | null): void { + vi.mocked(getSession).mockResolvedValue({ + session: { + id: "s-1", + expiresAt: new Date(Date.now() + 60_000), + actingAsUserId, + }, + user: { id: "u-1", role: "USER" }, + } as Awaited>); + } + + async function run(key = CLIENT_KEY): Promise { + const handler = vi.fn(async () => + NextResponse.json({ data: { ok: true }, error: null }, { status: 201 }), + ); + const wrapped = withIdempotency<[NextRequest]>(handler, async () => "u-1"); + await wrapped(makeRequest("POST", { "idempotency-key": key })); + } + + /** The key the wrapper looked the cell up under. */ + function lookedUpKey(): string { + const call = vi.mocked(prisma.idempotencyKey.findUnique).mock + .calls[0][0] as { + where: { userId_key_method_path: { userId: string; key: string } }; + }; + return call.where.userId_key_method_path.key; + } + + /** The row the wrapper claimed. */ + function claimed(): { userId: string; key: string } { + return ( + vi.mocked(prisma.idempotencyKey.create).mock.calls[0][0] as { + data: { userId: string; key: string }; + } + ).data; + } + + it("folds the account named by the Bearer selector into the key", async () => { + vi.mocked(getSession).mockResolvedValue(null); + present({ "x-healthlog-account": "owner-9" }); + + await run(); + + expect(lookedUpKey()).toBe(`owner-9|${CLIENT_KEY}`); + // The owner column stays the ACTOR. It is a foreign key to `users` with a + // cascade behind it, so it can only ever hold a real account id — which is + // why the record moved into the key and not into here. + expect(claimed()).toMatchObject({ + userId: "u-1", + key: `owner-9|${CLIENT_KEY}`, + }); + }); + + it("folds the account the cookie session is switched to", async () => { + cookieSession("owner-7"); + present({}); + + await run(); + + expect(claimed().key).toBe(`owner-7|${CLIENT_KEY}`); + }); + + it("keys a request with no acting account byte-for-byte as the client sent it", async () => { + cookieSession(null); + present({}); + + await run(); + + expect(lookedUpKey()).toBe(CLIENT_KEY); + expect(claimed()).toMatchObject({ userId: "u-1", key: CLIENT_KEY }); + }); + + it("ignores a selector sent over the cookie transport", async () => { + // The resolver refuses that request outright (`misplaced_selector`). The + // cell must not pretend it named a record either — a claim the request was + // never going to be served must not move where its answer is filed. + cookieSession(null); + present({ "x-healthlog-account": "owner-9" }); + + await run(); + + expect(claimed().key).toBe(CLIENT_KEY); + }); + + it("refuses a client key that carries the separator", async () => { + // The one way a caller could aim at somebody else's cell: send + // `owner-9|abc-12345678` as the key from their own un-switched session. The + // key validator does not admit the byte, so the header is malformed and the + // wrapper does not cache at all. + vi.mocked(getSession).mockResolvedValue(null); + present({}); + + await run(`owner-9|${CLIENT_KEY}`); + + expect(prisma.idempotencyKey.findUnique).not.toHaveBeenCalled(); + expect(prisma.idempotencyKey.create).not.toHaveBeenCalled(); + }); + + it("skips the cache when the claimed account names nothing that could exist", async () => { + // Longer than any account id. The request is refused downstream; caching it + // would let a caller write an arbitrarily long key into the table on the + // way to a 403. + vi.mocked(getSession).mockResolvedValue(null); + present({ "x-healthlog-account": "x".repeat(65) }); + + await run(); + + expect(prisma.idempotencyKey.findUnique).not.toHaveBeenCalled(); + expect(prisma.idempotencyKey.create).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/auth/acting-carrier.ts b/src/lib/auth/acting-carrier.ts index 9be1e116d..1d958b0b8 100644 --- a/src/lib/auth/acting-carrier.ts +++ b/src/lib/auth/acting-carrier.ts @@ -1,9 +1,14 @@ /** * What a request says it is acting as. * - * It lives apart from `requireRecordAuth`, which acts on the answer, because - * asking the question and deciding what it permits are different jobs and the - * asking is about to have more than one caller. + * Two things in the app have to answer that question, and they have to answer + * it the same way: + * + * * `requireRecordAuth` (`src/lib/api-handler.ts`), which turns the claim + * into a data scope after checking a live grant. + * * `withIdempotency` (`src/lib/idempotency.ts`), which files the replay cell + * under the record the request claims — before any handler runs, so it + * cannot wait for the grant check the handler performs. * * The claim is a SELECTOR and never an authorisation. Nothing in this file * looks at a grant, and no caller may treat a carrier as permission: the @@ -25,6 +30,8 @@ */ import { headers } from "next/headers"; +import { getSession } from "@/lib/auth/session"; + /** * The per-request account selector, on the Bearer transport only. * @@ -103,3 +110,45 @@ export function carrierTarget(carrier: ActingCarrier): string | null { export function selectorNamesAnAccount(value: string): boolean { return value.length > 0 && value.length <= MAX_SELECTOR_LENGTH; } + +/** + * The account this request CLAIMS, resolved without an auth context in hand. + * + * For callers that run BEFORE authentication has been resolved into a context — + * today that is the idempotency wrapper, which has to know which record a + * request is aimed at before it decides whether the answer is already cached. + * It resolves the session itself for exactly one reason: the transport decides + * which carrier counts, and "there is a valid session" is what the transport + * is. Guessing it from the presence of a cookie or an `Authorization` header + * would be a second, weaker answer to a question this file exists to answer + * once — and the case it gets wrong (a stale session cookie beside a live + * Bearer token) is precisely a delegated write landing in the wrong cell. + * + * The cost is one indexed session lookup on a request that is already about to + * write. It is not shared with the caller's own auth resolution because the + * resolver a route passes to `withIdempotency` is pluggable, and threading a + * session through that seam would make every custom resolver responsible for a + * question none of them asks. + * + * Returns the CLAIMED account, unverified and unchecked. The grant check + * happens later, inside the handler, on every request. + */ +export async function readClaimedActingAccount(): Promise { + const header = await readSelectorHeader(); + let session: Awaited> = null; + try { + session = await getSession(); + } catch { + // A session that cannot be resolved is not a switched one. The request + // still has to be authenticated by whatever wraps this call, and an + // unauthenticated request claims nothing. + session = null; + } + return carrierTarget( + decideActingCarrier({ + transport: session ? "cookie" : "bearer", + stamped: session?.session.actingAsUserId ?? null, + header, + }), + ); +} diff --git a/src/lib/idempotency.ts b/src/lib/idempotency.ts index 04d92f8d6..42374bc13 100644 --- a/src/lib/idempotency.ts +++ b/src/lib/idempotency.ts @@ -6,12 +6,21 @@ * retry within the TTL (24h) for the same `(userId, key, method, path)` * tuple returns the cached envelope as a 200 (carrying the original * status inside the JSON if needed) — no second side-effect. + * + * `userId` is the CALLER. Since a caller can act on somebody else's record, + * the record the request claims is folded into `key` (see `cellKey`), so the + * tuple identifies a request to one person's record rather than a request by + * one person. A caller acting only as themselves is unaffected, byte for byte. */ import type { NextRequest } from "next/server"; import { headers } from "next/headers"; import { NextResponse } from "next/server"; import { prisma } from "@/lib/db"; import { getSession } from "@/lib/auth/session"; +import { + readClaimedActingAccount, + selectorNamesAnAccount, +} from "@/lib/auth/acting-carrier"; import { hashToken } from "@/lib/auth/hmac"; import { annotate } from "@/lib/logging/context"; import { isP2002 } from "@/lib/prisma-errors"; @@ -37,6 +46,34 @@ const PENDING_STATUS = 0; const KEY_REGEX = /^[A-Za-z0-9_\-:.]{8,128}$/; +/** + * Separates the record from the client's key inside a cell's `key` column. + * + * `KEY_REGEX` above does not admit it, which is the whole reason it was + * chosen: a caller cannot hand-craft a key that lands in the cell of a request + * they are not making. Changing either one without the other reopens that. + */ +const RECORD_SEPARATOR = "|"; + +/** + * The `key` column for a request, given the record it claims to act on. + * + * Why the key and not the owner column: `IdempotencyKey.userId` is a foreign + * key to `users` with a cascade behind it (`prisma/schema.prisma`), so it can + * hold one real account id and nothing composite. It keeps holding the ACTOR — + * which is what makes "two delegates, one owner, one key" two cells without + * anything extra — and the record joins the other half of the composite unique + * index instead. + * + * No carrier means the key the client sent, byte for byte. Every request that + * is nobody's delegate therefore files exactly where it filed before, and the + * fold cannot change the behaviour of a client that never heard of sharing. + */ +function cellKey(clientKey: string, actingAccountId: string | null): string { + if (actingAccountId === null) return clientKey; + return `${actingAccountId}${RECORD_SEPARATOR}${clientKey}`; +} + export interface IdempotencyContext { userId: string; key: string; @@ -288,6 +325,11 @@ export function isCachableStatus(status: number): boolean { * /external clients — without it, every Bearer-authed retry was running * the handler again and creating duplicate measurements (audit C-4). * + * It resolves the CALLER and only the caller — which record they are aiming at + * is a separate question, answered by `readClaimedActingAccount()` above and + * folded into the key. Neither answer authorises anything: the handler runs + * its own `requireAuth()` / `requireRecordAuth()` either way. + * * Exported for unit testing; production callers should let * `withIdempotency()` pick this up automatically via its default arg. */ @@ -374,10 +416,32 @@ export function withIdempotency< const userId = await userIdResolver(...args); if (!userId) return handler(...args); + // Which RECORD this request is aimed at, as claimed by the transport and + // not yet checked against a grant — the check belongs to the handler, and + // this runs before it. One delegate posting the same key first for one + // person and then for another must not meet themselves in a single cell: + // that replays the first person's response, writes nothing to the second + // person's record, and reports success. Nothing errors and nothing logs a + // conflict, which is why it is folded here rather than left to call sites. + // + // The consequence of using the CLAIM: inside the 24h window a delegate + // whose grant has since ended can still replay a key they already + // completed, without the grant check running. They receive bytes they + // already had and nothing new is written — a fresh key from them is + // refused, because that request reaches the handler. + const claimedRecord = await readClaimedActingAccount(); + if (claimedRecord !== null && !selectorNamesAnAccount(claimedRecord)) { + // A claim that names no account that could exist. The request is refused + // downstream; skipping the cache keeps a caller from writing an + // arbitrarily long key into the table on the way to that refusal, and + // never lets one fall back onto the un-delegated cell. + return handler(...args); + } + const url = new URL(request.url); const ctx: IdempotencyContext = { userId, - key, + key: cellKey(key, claimedRecord), method: request.method, path: url.pathname, }; diff --git a/tests/integration/idempotency-record-scoped-cell.test.ts b/tests/integration/idempotency-record-scoped-cell.test.ts new file mode 100644 index 000000000..bdb4ec1fc --- /dev/null +++ b/tests/integration/idempotency-record-scoped-cell.test.ts @@ -0,0 +1,267 @@ +/** + * The replay cache is scoped to the RECORD, not only to the caller. + * + * One delegate, two people's records, one `Idempotency-Key`. Before this was + * fixed the cell was `(caller, key, method, path)`, so the second record's + * request replayed the first record's cached 201: no handler, no row, and a + * success response carrying the other person's id. Nothing errored and nothing + * logged a conflict, which is why it needed a test rather than a bug report. + * + * Everything under the test is real — the shipped wrapper, the shipped + * resolver, the shipped grant module's tables, the composite unique index and + * the `user_id` foreign key. The handler is assembled here rather than imported + * because what is under test is the cell, not any one domain's write path; a + * shipped route would bind this file to that route's schema and its migration + * to a delegable mode. + */ +import { NextRequest, NextResponse } from "next/server"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { cookieJar, headerJar } from "./mock-next-headers"; +import { getPrismaClient, truncateAllTables } from "./setup"; + +vi.mock("next/headers", async () => { + const { cookieJar, headerJar } = await import("./mock-next-headers"); + return { + headers: vi.fn(async () => ({ + get: (name: string) => headerJar.get(name.toLowerCase()) ?? null, + })), + cookies: vi.fn(async () => ({ + get: (name: string) => { + const value = cookieJar.get(name); + return value ? { name, value } : undefined; + }, + set: (name: string, value: string) => { + cookieJar.set(name, value); + }, + delete: (name: string) => { + cookieJar.delete(name); + }, + })), + }; +}); + +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); + +import { + ACCOUNT_SELECTOR_HEADER, + apiHandler, + requireRecordAuth, +} from "@/lib/api-handler"; +import { hashToken } from "@/lib/auth/hmac"; +import { prisma } from "@/lib/db"; +import { withIdempotency } from "@/lib/idempotency"; + +const KEY = "retry-0f3a1c9d"; + +let counter = 0; + +async function makeUser(label: string) { + const suffix = `${label}-${counter++}`; + return getPrismaClient().user.create({ + data: { + username: `cell-${suffix}`, + email: `cell-${suffix}@example.test`, + role: "USER", + }, + }); +} + +async function mintToken(userId: string): Promise { + const raw = `hlk_${userId}-${counter++}`.padEnd(20, "0"); + await getPrismaClient().apiToken.create({ + data: { + userId, + name: "idempotency-cell-test", + tokenHash: hashToken(raw), + permissions: ["*"], + }, + }); + return raw; +} + +/** + * An accepted WRITE grant, written directly. + * + * `inviteGrant()` mints READ today; the level is Chunk B's to open. What this + * file needs is a row in the state the resolver reads, and that state is the + * two columns below. + */ +async function writeGrant(grantorId: string, granteeId: string) { + return getPrismaClient().accountGrant.create({ + data: { + grantorId, + granteeId, + access: "WRITE", + acceptedAt: new Date(Date.now() - 60_000), + }, + }); +} + +let handlerRuns = 0; + +/** + * A delegated write, wrapped exactly as the shipped routes wrap one: + * `apiHandler(withIdempotency(handler))`. + */ +const delegableWrite: (request: NextRequest) => Promise = apiHandler( + withIdempotency<[NextRequest]>(async () => { + handlerRuns += 1; + const { user } = await requireRecordAuth("write"); + const row = await prisma.measurement.create({ + data: { + userId: user.id, + type: "WEIGHT", + value: 80 + handlerRuns, + unit: "kg", + measuredAt: new Date(), + source: "MANUAL", + }, + }); + return NextResponse.json( + { data: { id: row.id, userId: row.userId }, error: null }, + { status: 201 }, + ); + }), +); + +function post(key: string | null = KEY): Promise { + const headers: Record = { + "content-type": "application/json", + }; + if (key) headers["idempotency-key"] = key; + return delegableWrite( + new NextRequest("http://localhost/api/test/delegable-write", { + method: "POST", + headers, + body: JSON.stringify({ ok: true }), + }), + ); +} + +/** Point the next request at a record. */ +function actAs(ownerId: string | null): void { + if (ownerId === null) headerJar.delete(ACCOUNT_SELECTOR_HEADER); + else headerJar.set(ACCOUNT_SELECTOR_HEADER, ownerId); +} + +async function measurementsOf(userId: string) { + return getPrismaClient().measurement.findMany({ where: { userId } }); +} + +beforeEach(async () => { + await truncateAllTables(getPrismaClient()); + cookieJar.clear(); + headerJar.clear(); + handlerRuns = 0; +}); + +describe("the replay cell is per record", () => { + it("writes to BOTH records when one key is reused across two owners", async () => { + const delegate = await makeUser("delegate"); + const ownerA = await makeUser("owner-a"); + const ownerB = await makeUser("owner-b"); + await writeGrant(ownerA.id, delegate.id); + await writeGrant(ownerB.id, delegate.id); + headerJar.set("authorization", `Bearer ${await mintToken(delegate.id)}`); + + actAs(ownerA.id); + const first = await post(); + expect(first.status).toBe(201); + expect((await first.json()).data.userId).toBe(ownerA.id); + + // Same caller, same key, same path — a different person's record. The + // handler must run again, because nothing about this request was answered + // by the previous one. + actAs(ownerB.id); + const second = await post(); + expect(second.status).toBe(201); + const secondBody = await second.json(); + + // Asserted first because it is the damage: without a per-record cell this + // is an empty list, and the caller was told 201. + expect(await measurementsOf(ownerB.id)).toHaveLength(1); + expect(secondBody.data.userId).toBe(ownerB.id); + expect(second.headers.get("X-Idempotent-Replay")).not.toBe("true"); + expect(handlerRuns).toBe(2); + expect(await measurementsOf(ownerA.id)).toHaveLength(1); + // Two cells under the one caller: the actor's id still owns both rows, so + // the foreign key and its cascade are untouched by the fold. + const cells = await getPrismaClient().idempotencyKey.findMany({ + where: { userId: delegate.id }, + }); + expect(cells).toHaveLength(2); + }); + + it("still replays a genuine retry on the SAME record", async () => { + const delegate = await makeUser("delegate"); + const owner = await makeUser("owner"); + await writeGrant(owner.id, delegate.id); + headerJar.set("authorization", `Bearer ${await mintToken(delegate.id)}`); + actAs(owner.id); + + const first = await post(); + const firstBody = await first.json(); + const replay = await post(); + + expect(replay.status).toBe(201); + expect(replay.headers.get("X-Idempotent-Replay")).toBe("true"); + expect(await replay.json()).toEqual(firstBody); + expect(handlerRuns).toBe(1); + expect(await measurementsOf(owner.id)).toHaveLength(1); + }); + + it("keys a request with no acting account exactly as the client sent it", async () => { + const caller = await makeUser("self"); + headerJar.set("authorization", `Bearer ${await mintToken(caller.id)}`); + + const response = await post(); + expect(response.status).toBe(201); + + // The self path is the pre-existing path and must stay byte-identical: + // same owner column, same key column, no separator, no prefix. + const cells = await getPrismaClient().idempotencyKey.findMany(); + expect(cells).toHaveLength(1); + expect(cells[0]).toMatchObject({ userId: caller.id, key: KEY }); + expect(await measurementsOf(caller.id)).toHaveLength(1); + }); + + it("replays inside the TTL after the grant is revoked, and writes nothing", async () => { + // The accepted consequence of folding the CLAIMED account rather than a + // verified one: within the 24h window a revoked delegate's retry is + // answered from the cache without the grant check running again. Pinned + // here so it stays a decision. What matters is the second half — the + // response is one they already received, and no new row appears. + const delegate = await makeUser("delegate"); + const owner = await makeUser("owner"); + const grant = await writeGrant(owner.id, delegate.id); + headerJar.set("authorization", `Bearer ${await mintToken(delegate.id)}`); + actAs(owner.id); + + const first = await post(); + const firstBody = await first.json(); + expect(first.status).toBe(201); + + // The owner withdraws access. Both revocation columns move together — + // the database refuses one without the other. + await getPrismaClient().accountGrant.update({ + where: { id: grant.id }, + data: { revokedAt: new Date(), revokedBy: "GRANTOR" }, + }); + + const afterRevoke = await post(); + expect(afterRevoke.status).toBe(201); + expect(afterRevoke.headers.get("X-Idempotent-Replay")).toBe("true"); + expect(await afterRevoke.json()).toEqual(firstBody); + expect(handlerRuns).toBe(1); + expect(await measurementsOf(owner.id)).toHaveLength(1); + + // A request the cache does NOT answer is refused, which is what keeps the + // window above bounded to keys the delegate had already completed. + const fresh = await post("retry-11119999"); + expect(fresh.status).toBe(403); + expect(await measurementsOf(owner.id)).toHaveLength(1); + }); +});