From 60b842f6142260a458a680fa47aa3afd053b6a9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 6 Aug 2026 01:29:07 +0200 Subject: [PATCH] fix: reauthorize delegated replay - Recheck the live grant before returning cached delegated responses - Refuse cached replay when the acting record cannot be resolved safely --- src/__tests__/sharing-surface-guard.test.ts | 25 +++++++++ src/lib/auth/acting-carrier.ts | 26 +++++---- src/lib/idempotency.ts | 56 +++++++++++++++++-- .../idempotency-record-scoped-cell.test.ts | 37 ------------ .../sharing-idempotency-revocation.test.ts | 33 ++++++++++- 5 files changed, 123 insertions(+), 54 deletions(-) diff --git a/src/__tests__/sharing-surface-guard.test.ts b/src/__tests__/sharing-surface-guard.test.ts index 8cc24b41d..8f1ecd56d 100644 --- a/src/__tests__/sharing-surface-guard.test.ts +++ b/src/__tests__/sharing-surface-guard.test.ts @@ -2848,6 +2848,31 @@ describe("(h) the guardian route set is frozen", () => { }); }); +/* -------------------------------------------------------------------------- */ +/* (i) the delegated replay boundary */ +/* -------------------------------------------------------------------------- */ + +describe("(i) delegated replay is re-authorized", () => { + it("checks the current grant before returning a cached delegated body", () => { + const replay = functionSource( + "lib/idempotency.ts", + "canReplayDelegatedResponse", + ); + expect(replay.length).toBeGreaterThan(0); + expect(replay).toContain("findActiveGrant"); + expect(replay).toContain("grantorId: recordUserId"); + expect(replay).toContain("granteeId: actorUserId"); + + const wrapper = functionSource("lib/idempotency.ts", "withIdempotency"); + expect(wrapper.length).toBeGreaterThan(0); + expect(wrapper).toContain("claimedRecord === undefined"); + const authorization = wrapper.indexOf("canReplayDelegatedResponse"); + const replayReturn = wrapper.indexOf("return cached.response"); + expect(authorization).toBeGreaterThan(-1); + expect(replayReturn).toBeGreaterThan(authorization); + }); +}); + /* -------------------------------------------------------------------------- */ /* The counter-test: quiet on the innocent tree */ /* -------------------------------------------------------------------------- */ diff --git a/src/lib/auth/acting-carrier.ts b/src/lib/auth/acting-carrier.ts index b7f25f52c..055a708ec 100644 --- a/src/lib/auth/acting-carrier.ts +++ b/src/lib/auth/acting-carrier.ts @@ -184,10 +184,16 @@ export async function getUnswitchedSession(): Promise { +export async function readClaimedActingAccount(): Promise< + string | null | undefined +> { const header = await readSelectorHeader(); let session: Awaited> = null; try { @@ -198,11 +204,11 @@ export async function readClaimedActingAccount(): Promise { // unauthenticated request claims nothing. session = null; } - return carrierTarget( - decideActingCarrier({ - transport: session ? "cookie" : "bearer", - stamped: session?.session.actingAsUserId ?? null, - header, - }), - ); + const carrier = decideActingCarrier({ + transport: session ? "cookie" : "bearer", + stamped: session?.session.actingAsUserId ?? null, + header, + }); + if (carrier.kind === "misplaced-header") return undefined; + return carrierTarget(carrier); } diff --git a/src/lib/idempotency.ts b/src/lib/idempotency.ts index 42374bc13..28fb170a1 100644 --- a/src/lib/idempotency.ts +++ b/src/lib/idempotency.ts @@ -24,6 +24,7 @@ import { import { hashToken } from "@/lib/auth/hmac"; import { annotate } from "@/lib/logging/context"; import { isP2002 } from "@/lib/prisma-errors"; +import { findActiveGrant } from "@/lib/sharing/grants"; import { decrypt, encrypt } from "@/lib/crypto"; const TTL_MS = 24 * 60 * 60 * 1000; @@ -396,6 +397,34 @@ function inflightConflictResponse(): Response { ); } +/** + * Whether a cached delegated response may still be returned. + * + * The wrapper runs before a route can call `requireRecordAuth`, but it must + * not become an alternate way around that fresh grant check. A completed cell + * carries the caller and claimed record; the grant row is the only authority + * that decides whether that pair is live now. A missing, expired, revoked, or + * unreadable grant deliberately falls through to the handler, whose normal + * refusal is the only response the caller receives. + */ +async function canReplayDelegatedResponse( + actorUserId: string, + recordUserId: string, +): Promise { + try { + return ( + (await findActiveGrant({ + grantorId: recordUserId, + granteeId: actorUserId, + })) !== null + ); + } catch { + // The cache is an accelerator. A failed lookup must never return a body + // that the route would refuse once the database is available again. + return false; + } +} + export function withIdempotency< Args extends [Request | NextRequest, ...unknown[]], >( @@ -424,12 +453,17 @@ export function withIdempotency< // 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. + // A claim only chooses the cache cell. It never carries authority: a + // completed delegated cell is checked against the current grant again + // immediately before its body can be returned below. const claimedRecord = await readClaimedActingAccount(); + if (claimedRecord === undefined) { + // The carrier cannot safely name a record. In particular, a selector on + // a cookie request is a misplaced claim, not an own-record request. + // Let the route return its normal refusal instead of exposing a cached + // body from the own-record cell. + return handler(...args); + } 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 @@ -447,7 +481,17 @@ export function withIdempotency< }; const cached = await findCached(ctx); - if (cached?.kind === "replay") return cached.response; + if (cached?.kind === "replay") { + if ( + claimedRecord !== null && + !(await canReplayDelegatedResponse(userId, claimedRecord)) + ) { + // Do not replace the route's refusal with a cache-specific response: + // it owns the stable 403 envelope and the associated audit trail. + return handler(...args); + } + return cached.response; + } if (cached?.kind === "pending") { // A concurrent request is mid-flight on this exact key. Refuse // rather than run the side-effect a second time. The client should diff --git a/tests/integration/idempotency-record-scoped-cell.test.ts b/tests/integration/idempotency-record-scoped-cell.test.ts index 97684954a..9fc440c9b 100644 --- a/tests/integration/idempotency-record-scoped-cell.test.ts +++ b/tests/integration/idempotency-record-scoped-cell.test.ts @@ -227,41 +227,4 @@ describe("the replay cell is per record", () => { 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); - }); }); diff --git a/tests/integration/sharing-idempotency-revocation.test.ts b/tests/integration/sharing-idempotency-revocation.test.ts index 577a683c2..50cd3114f 100644 --- a/tests/integration/sharing-idempotency-revocation.test.ts +++ b/tests/integration/sharing-idempotency-revocation.test.ts @@ -75,6 +75,16 @@ async function mintToken(userId: string): Promise { return raw; } +async function mintSession(userId: string): Promise { + const session = await getPrismaClient().session.create({ + data: { + userId, + expiresAt: new Date(Date.now() + 60 * 60 * 1000), + }, + }); + cookieJar.set("healthlog_session", session.id); +} + async function createWriteGrant(input: { id: string; grantorId: string; @@ -93,8 +103,8 @@ async function createWriteGrant(input: { const delegatedWrite: (request: NextRequest) => Promise = apiHandler( withIdempotency<[NextRequest]>(async () => { - handlerRuns += 1; const { user } = await requireRecordAuth("write", "measurements"); + handlerRuns += 1; const row = await prisma.measurement.create({ data: { userId: user.id, @@ -230,4 +240,25 @@ describe("delegated idempotency replay", () => { }), ).toBe(1); }); + + it("does not replay when the cookie transport carries a misplaced selector", async () => { + const principal = SECURITY_PRINCIPALS.ownerIdempotencyPositiveControl; + await createUser(principal.identities.actorUserId); + await mintSession(principal.identities.actorUserId); + + const first = await post(); + expect(first.status).toBe(201); + + headerJar.set(ACCOUNT_SELECTOR_HEADER, "untrusted-record-selector"); + const replay = await post(); + + expect(replay.status).toBe(403); + expect(replay.headers.get("X-Idempotent-Replay")).not.toBe("true"); + expect(handlerRuns).toBe(1); + expect( + await getPrismaClient().measurement.count({ + where: { userId: principal.identities.recordUserId }, + }), + ).toBe(1); + }); });