From 076c2f9c5dc0ae7204f9dae021f61c49361a7468 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Tue, 8 Sep 2026 20:23:30 +0200 Subject: [PATCH] fix(auth): sign out the other devices, lose a refresh to a revocation, key the replay cache by authority "Sign out everywhere" revoked every other refresh token and nothing else. A native login is two rows, and the Bearer check consults only the paired access token, so the other phones kept working until that token expired, up to a day on the default policy. The route also handed destroyOtherSessions a session-kind credential for a Bearer caller, whose session.id is the ApiToken row id; the exception matched nothing and the calling phone revoked its own refresh token. destroyOtherSessions now finds the paired access tokens through the refresh rows and revokes them inside the same transaction, reporting the count, and the route names a Bearer caller by its access-token hash. Programmatic tokens are untouched. rotateRefreshToken marked the consumed row under usedAt: null alone, so a revocation landing between the read and the write was overtaken and a pair minted after the family ended stayed live. The guard now includes revokedAt: null; on a lost race the minted pair is retired and the reason distinguishes a revoked family from a double consume. withIdempotency consulted the grant only at replay time, and only for its existence; a narrow-scoped token was checked only on the delegated arm. Both move into the cell key: a delegated cell carries the grant's id, level and scope fingerprint, a narrow token's cells carry the token, and a request with no live grant neither reads nor claims a cell. Cookie and wildcard own-record cells are keyed byte-for-byte as before. Tests: integration for the paired access tokens (cookie and Bearer caller), a deterministic revoke-mid-rotation race against Postgres, unit coverage for the new cell keys, and the structural guard moved to the new invariant. --- src/__tests__/sharing-surface-guard.test.ts | 32 ++-- .../auth/me/sessions/__tests__/route.test.ts | 51 +++++- src/app/api/auth/me/sessions/route.ts | 55 ++++++- src/lib/__tests__/idempotency.test.ts | 100 ++++++++++- src/lib/auth/refresh-token.ts | 21 ++- src/lib/auth/session.ts | 72 ++++++-- src/lib/idempotency.ts | 155 +++++++++++------- .../refresh-rotation-revoke-race.test.ts | 154 +++++++++++++++++ .../sessions-and-known-devices.test.ts | 124 ++++++++++++++ 9 files changed, 659 insertions(+), 105 deletions(-) create mode 100644 tests/integration/refresh-rotation-revoke-race.test.ts diff --git a/src/__tests__/sharing-surface-guard.test.ts b/src/__tests__/sharing-surface-guard.test.ts index a0b1a029a..fc7fc4016 100644 --- a/src/__tests__/sharing-surface-guard.test.ts +++ b/src/__tests__/sharing-surface-guard.test.ts @@ -3108,23 +3108,33 @@ describe("(h) the guardian route set is frozen", () => { /* -------------------------------------------------------------------------- */ 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"); + // v1.38.11 — the grant is no longer consulted at replay time; it is folded + // into the cell key BEFORE any cell is read or claimed (`delegatedGrantFacet`), + // so a replaced or narrowed grant cannot reach a cell filled under the wider + // one. The invariant this pins moved accordingly: the grant lookup names the + // right pair, it happens before `findCached`, and a missing grant leaves the + // wrapper without touching a cell. + it("resolves the current grant into the cell key before any cell is read", () => { + const facet = functionSource("lib/idempotency.ts", "delegatedGrantFacet"); + expect(facet.length).toBeGreaterThan(0); + expect(facet).toContain("findActiveGrant"); + expect(facet).toContain("grantorId: recordUserId"); + expect(facet).toContain("granteeId: actorUserId"); + expect(facet).toContain("grant.id"); + expect(facet).toContain("grant.access"); + expect(facet).toContain("scopeJson"); const wrapper = functionSource("lib/idempotency.ts", "withIdempotency"); expect(wrapper.length).toBeGreaterThan(0); expect(wrapper).toContain("claimedRecord === undefined"); - const authorization = wrapper.indexOf("canReplayDelegatedResponse"); + const authorization = wrapper.indexOf("delegatedGrantFacet("); + const noGrant = wrapper.indexOf("grantFacet === null"); + const cacheRead = wrapper.indexOf("findCached(ctx)"); const replayReturn = wrapper.indexOf("return cached.response"); expect(authorization).toBeGreaterThan(-1); - expect(replayReturn).toBeGreaterThan(authorization); + expect(noGrant).toBeGreaterThan(authorization); + expect(cacheRead).toBeGreaterThan(noGrant); + expect(replayReturn).toBeGreaterThan(cacheRead); }); }); diff --git a/src/app/api/auth/me/sessions/__tests__/route.test.ts b/src/app/api/auth/me/sessions/__tests__/route.test.ts index 17bf18a59..471e38b04 100644 --- a/src/app/api/auth/me/sessions/__tests__/route.test.ts +++ b/src/app/api/auth/me/sessions/__tests__/route.test.ts @@ -5,7 +5,14 @@ import { NextRequest } from "next/server"; process.env.API_TOKEN_HMAC_KEY ??= "a".repeat(64); vi.mock("@/lib/db", () => ({ - prisma: { session: { findMany: vi.fn() } }, + prisma: { + session: { findMany: vi.fn() }, + apiToken: { + findUnique: vi.fn(), + update: vi.fn(() => ({ catch: () => undefined })), + }, + user: { findUnique: vi.fn() }, + }, })); // `sessionHandle` stays REAL: the point of the handle test is that the route @@ -43,6 +50,8 @@ vi.mock("next/headers", () => ({ })); import { GET, DELETE } from "../route"; +import { headers } from "next/headers"; +import { hashToken } from "@/lib/auth/hmac"; import { DELETE as DELETE_ONE } from "../[id]/route"; import { prisma } from "@/lib/db"; import { @@ -151,7 +160,10 @@ describe("GET /api/auth/me/sessions", () => { describe("DELETE /api/auth/me/sessions (sign out everywhere)", () => { it("revokes other sessions and reports the count", async () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); - vi.mocked(destroyOtherSessions).mockResolvedValue({ sessionsRevoked: 3 }); + vi.mocked(destroyOtherSessions).mockResolvedValue({ + sessionsRevoked: 3, + accessTokensRevoked: 0, + }); const res = await DELETE(del()); expect(res.status).toBe(200); @@ -162,6 +174,41 @@ describe("DELETE /api/auth/me/sessions (sign out everywhere)", () => { sessionId: "sess-current", }); }); + + it("names a Bearer caller by its access token so its own device login is spared", async () => { + // No cookie session: the caller is a phone presenting its login token. + vi.mocked(getSession).mockResolvedValue(null); + const raw = `hlk_${"d".repeat(64)}`; + vi.mocked(headers).mockResolvedValue({ + get: (name: string) => + name.toLowerCase() === "authorization" ? `Bearer ${raw}` : null, + } as never); + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "tok-phone", + userId: "user-1", + permissions: ["*"], + revoked: false, + expiresAt: null, + } as never); + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + id: "user-1", + username: "u", + role: "USER", + } as never); + vi.mocked(destroyOtherSessions).mockResolvedValue({ + sessionsRevoked: 1, + accessTokensRevoked: 1, + }); + + const res = await DELETE(del()); + + expect(res.status).toBe(200); + expect(destroyOtherSessions).toHaveBeenCalledWith("user-1", { + kind: "accessToken", + accessTokenHash: hashToken(raw), + }); + vi.mocked(headers).mockResolvedValue({ get: () => null } as never); + }); }); describe("DELETE /api/auth/me/sessions/[id]", () => { diff --git a/src/app/api/auth/me/sessions/route.ts b/src/app/api/auth/me/sessions/route.ts index 24f41a095..3d447e3d9 100644 --- a/src/app/api/auth/me/sessions/route.ts +++ b/src/app/api/auth/me/sessions/route.ts @@ -12,13 +12,19 @@ * caller's own row is flagged `isCurrent` so the UI can mark "this device". */ import { NextRequest } from "next/server"; -import { apiHandler, requireAuth } from "@/lib/api-handler"; +import { headers } from "next/headers"; +import { apiHandler, HttpError, requireAuth } from "@/lib/api-handler"; import { apiSuccess } from "@/lib/api-response"; import { annotate } from "@/lib/logging/context"; import { auditLog } from "@/lib/auth/audit"; import { getClientIp } from "@/lib/api-response"; import { prisma } from "@/lib/db"; -import { destroyOtherSessions, sessionHandle } from "@/lib/auth/session"; +import { hashToken } from "@/lib/auth/hmac"; +import { + destroyOtherSessions, + sessionHandle, + type CurrentCredential, +} from "@/lib/auth/session"; import { lookupIpLocation } from "@/lib/geo"; import { coarseDeviceLabel, maskIp } from "@/lib/auth/device-fingerprint"; @@ -86,23 +92,54 @@ export const GET = apiHandler(async () => { }); export const DELETE = apiHandler(async (request: NextRequest) => { - const { user, session } = await requireAuth(); + const auth = await requireAuth(); + const { user } = auth; - const { sessionsRevoked } = await destroyOtherSessions(user.id, { - kind: "session", - sessionId: session.id, - }); + // Name the caller by the transport it actually used. On the Bearer path + // `session.id` is the `ApiToken` row id, not a session id; handing that to + // the session-kind spared nothing, so the phone that pressed "sign out + // everywhere" was the one signed out at its next rotation, while the other + // devices kept their access tokens. The access-token kind spares the + // caller's own device login by its `accessTokenHash`. + const current: CurrentCredential = + auth.authMethod === "bearer" + ? { + kind: "accessToken", + accessTokenHash: hashToken(await presentedBearerToken()), + } + : { kind: "session", sessionId: auth.session.id }; + + const { sessionsRevoked, accessTokensRevoked } = await destroyOtherSessions( + user.id, + current, + ); await auditLog("auth.session.revoke_others", { userId: user.id, ipAddress: getClientIp(request), - details: { sessionsRevoked }, + details: { sessionsRevoked, accessTokensRevoked }, }); annotate({ action: { name: "auth.session.revoke_others" }, - meta: { sessions_revoked: sessionsRevoked }, + meta: { + sessions_revoked: sessionsRevoked, + access_tokens_revoked: accessTokensRevoked, + }, }); return apiSuccess({ sessionsRevoked }); }); + +/** + * The raw Bearer token this request presented. `requireAuth` has already + * accepted it, so a missing header here is a programming error, not a client + * one; it is refused the same way rather than reasoned about. + */ +async function presentedBearerToken(): Promise { + const authHeader = (await headers()).get("authorization"); + if (!authHeader?.startsWith("Bearer ")) { + throw new HttpError(401, "Not authenticated"); + } + return authHeader.slice(7); +} diff --git a/src/lib/__tests__/idempotency.test.ts b/src/lib/__tests__/idempotency.test.ts index 0476131c5..61494372e 100644 --- a/src/lib/__tests__/idempotency.test.ts +++ b/src/lib/__tests__/idempotency.test.ts @@ -52,7 +52,23 @@ import { } from "../idempotency"; import { prisma } from "@/lib/db"; import { getSession } from "@/lib/auth/session"; +vi.mock("@/lib/sharing/grants", () => ({ + findActiveGrant: vi.fn(), +})); + +import { createHash } from "node:crypto"; import { headers } from "next/headers"; +import { findActiveGrant } from "@/lib/sharing/grants"; + +/** A live WRITE grant, whole record, as `delegatedGrantFacet` reads it. */ +const GRANT = { id: "grant-1", access: "WRITE", scopeJson: null }; +function grantFacet(grant: { id: string; access: string; scopeJson: unknown }) { + const fp = createHash("sha256") + .update(JSON.stringify(grant.scopeJson ?? null)) + .digest("hex") + .slice(0, 16); + return `g:${grant.id}:${grant.access}:${fp}`; +} function makeRequest( method: string, @@ -68,6 +84,7 @@ function makeRequest( beforeEach(() => { vi.resetAllMocks(); vi.mocked(prisma.idempotencyKey.findUnique).mockResolvedValue(null); + vi.mocked(findActiveGrant).mockResolvedValue(GRANT as never); vi.mocked(prisma.idempotencyKey.create).mockResolvedValue({} as never); vi.mocked(prisma.idempotencyKey.updateMany).mockResolvedValue({ count: 1, @@ -365,7 +382,9 @@ describe("defaultUserIdResolver (audit C-4)", () => { vi.mocked(getSession).mockResolvedValue(null); mockHeader("Bearer hlk_abcdef"); vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "tok-device", userId: "u-bearer", + permissions: ["*"], revoked: false, expiresAt: null, } as never); @@ -610,13 +629,13 @@ describe("withIdempotency — record-scoped cells", () => { await run(); - expect(lookedUpKey()).toBe(`owner-9|${CLIENT_KEY}`); + expect(lookedUpKey()).toBe(`owner-9|${grantFacet(GRANT)}|${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}`, + key: `owner-9|${grantFacet(GRANT)}|${CLIENT_KEY}`, }); }); @@ -626,7 +645,80 @@ describe("withIdempotency — record-scoped cells", () => { await run(); - expect(claimed().key).toBe(`owner-7|${CLIENT_KEY}`); + expect(claimed().key).toBe(`owner-7|${grantFacet(GRANT)}|${CLIENT_KEY}`); + }); + + it("files a delegated request under the grant it acts through, so a replaced grant is a fresh cell", async () => { + vi.mocked(getSession).mockResolvedValue(null); + present({ "x-healthlog-account": "owner-9" }); + await run(); + const underFirstGrant = claimed().key; + + vi.clearAllMocks(); + vi.mocked(prisma.idempotencyKey.findUnique).mockResolvedValue(null); + vi.mocked(prisma.idempotencyKey.create).mockResolvedValue({} as never); + vi.mocked(findActiveGrant).mockResolvedValue({ + ...GRANT, + id: "grant-2", + access: "READ", + } as never); + present({ "x-healthlog-account": "owner-9" }); + await run(); + + expect(claimed().key).not.toBe(underFirstGrant); + expect(claimed().key).toBe( + `owner-9|${grantFacet({ ...GRANT, id: "grant-2", access: "READ" })}|${CLIENT_KEY}`, + ); + }); + + it("files a scope edited in place under a fresh cell too", async () => { + vi.mocked(getSession).mockResolvedValue(null); + vi.mocked(findActiveGrant).mockResolvedValue({ + ...GRANT, + scopeJson: { areas: ["vitals"] }, + } as never); + present({ "x-healthlog-account": "owner-9" }); + await run(); + expect(claimed().key).not.toBe( + `owner-9|${grantFacet(GRANT)}|${CLIENT_KEY}`, + ); + }); + + it("neither reads nor claims a cell when no live grant exists", async () => { + vi.mocked(getSession).mockResolvedValue(null); + vi.mocked(findActiveGrant).mockResolvedValue(null); + present({ "x-healthlog-account": "owner-9" }); + await run(); + expect(prisma.idempotencyKey.findUnique).not.toHaveBeenCalled(); + expect(prisma.idempotencyKey.create).not.toHaveBeenCalled(); + }); + + it("keys a narrow-scoped token's own-record cell by the token, and a wildcard token's byte-for-byte", async () => { + vi.mocked(getSession).mockResolvedValue(null); + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "tok-narrow", + userId: "u-1", + permissions: ["measurements:write"], + revoked: false, + expiresAt: null, + } as never); + present({ authorization: `Bearer hlk_${"b".repeat(64)}` }); + await run(); + expect(claimed().key).toBe(`t:tok-narrow|${CLIENT_KEY}`); + + vi.clearAllMocks(); + vi.mocked(prisma.idempotencyKey.findUnique).mockResolvedValue(null); + vi.mocked(prisma.idempotencyKey.create).mockResolvedValue({} as never); + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "tok-wild", + userId: "u-1", + permissions: ["*"], + revoked: false, + expiresAt: null, + } as never); + present({ authorization: `Bearer hlk_${"c".repeat(64)}` }); + await run(); + expect(claimed().key).toBe(CLIENT_KEY); }); it("keys a request with no acting account byte-for-byte as the client sent it", async () => { @@ -807,7 +899,9 @@ describe("the record-session fence sits above the replay cache", () => { // header on this transport is neither read nor honoured. vi.mocked(getSession).mockResolvedValue(null); vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "tok-device", userId: "u-bearer", + permissions: ["*"], revoked: false, expiresAt: null, } as never); diff --git a/src/lib/auth/refresh-token.ts b/src/lib/auth/refresh-token.ts index bf597ee01..007c70ed7 100644 --- a/src/lib/auth/refresh-token.ts +++ b/src/lib/auth/refresh-token.ts @@ -224,8 +224,14 @@ export async function rotateRefreshToken(input: { select: { id: true }, }); + // `revokedAt: null` is part of the guard, not only `usedAt: null`. The row + // was read as live at the top of this function; a revocation landing between + // that read and this write — "sign out everywhere", a credential rotation, + // reuse detection tripped by a sibling — would otherwise be overtaken, and + // the pair minted a moment ago would be the one live login left on a family + // the user had just ended. const updated = await prisma.refreshToken.updateMany({ - where: { id: row.id, usedAt: null }, + where: { id: row.id, usedAt: null, revokedAt: null }, data: { usedAt: new Date(), replacedById: newRow?.id ?? null, @@ -233,8 +239,11 @@ export async function rotateRefreshToken(input: { }); if (updated.count === 0) { - // Lost the race — another concurrent refresh consumed this row. - // Revoke our just-issued tokens to avoid leaking an extra valid pair. + // Lost the race: another concurrent refresh consumed this row, or a + // revocation landed underneath it. Either way, retire what was just minted + // so no extra valid pair leaks, then say which it was — a revoked family + // must read as `revoked`, which the client answers with a fresh sign-in, + // not as `already_used`, which it would retry. await prisma.refreshToken.updateMany({ where: { tokenHash: newHash }, data: { revokedAt: new Date() }, @@ -243,7 +252,11 @@ export async function rotateRefreshToken(input: { where: { tokenHash: hashToken(bundle.accessToken) }, data: { revoked: true }, }); - return { ok: false, reason: "already_used" }; + const now = await prisma.refreshToken.findUnique({ + where: { id: row.id }, + select: { revokedAt: true }, + }); + return { ok: false, reason: now?.revokedAt ? "revoked" : "already_used" }; } // Best-effort: sunset the access token paired with the consumed refresh, so diff --git a/src/lib/auth/session.ts b/src/lib/auth/session.ts index c8899fadb..f484953d2 100644 --- a/src/lib/auth/session.ts +++ b/src/lib/auth/session.ts @@ -525,19 +525,30 @@ export type CurrentCredential = /** * v1.23 — "sign out everywhere" for the user-facing active-session surface * (issue #64). Distinct from `destroyAllSessions`: this keeps the caller's - * CURRENT cookie session alive so clicking the button doesn't log the user out - * of the device they pressed it on, and it does NOT revoke `ApiToken`s — those - * are long-lived programmatic credentials the user manages separately under - * /settings/api-tokens, not "sessions" in the device-list sense. It DOES revoke - * every native-client `RefreshToken` (each is a device login) so a signed-in - * phone/tablet is dropped too, matching the "everywhere" promise. + * CURRENT credential alive so pressing the button does not sign out the device + * it was pressed on, and it leaves the long-lived programmatic `ApiToken`s + * alone — those are credentials the user manages separately under + * /settings/api-tokens, not "sessions" in the device-list sense. * - * Returns the number of OTHER web sessions removed so the surface can confirm. + * It DOES revoke every other native-client `RefreshToken` (each is a device + * login), and, since v1.38.11, the access token paired with each of them. A + * native login is two rows: the `RefreshToken`, and the `ApiToken` its + * `accessTokenHash` points at, which is the only row the Bearer resolver + * consults. Revoking the refresh token alone left the other phone working + * until its access token expired — up to a day on the default policy — which + * is not what "everywhere" promised. The paired access tokens are found + * through the refresh rows inside the same transaction, so a rotation landing + * between the read and the write cannot slip a fresh pair past it: the + * rotation's own consume step refuses a row revoked underneath it and retires + * what it minted. + * + * Returns the number of OTHER web sessions removed and the number of access + * tokens revoked, so the surface and the audit row can say what happened. */ export async function destroyOtherSessions( userId: string, current: CurrentCredential, -): Promise<{ sessionsRevoked: number }> { +): Promise<{ sessionsRevoked: number; accessTokensRevoked: number }> { // Which rows describe the CALLER depends on how the caller authenticated, and // the two shapes are not interchangeable. A cookie caller is a `Session` row. // A Bearer caller is an `ApiToken` whose device login is the `RefreshToken` @@ -551,28 +562,53 @@ export async function destroyOtherSessions( current.kind === "accessToken" ? { accessTokenHash: { not: current.accessTokenHash } } : {}; + const revokedAt = new Date(); - const [deleted] = await prisma.$transaction([ - prisma.session.deleteMany({ + return prisma.$transaction(async (tx) => { + const families = await tx.refreshToken.findMany({ + where: { userId, revokedAt: null, ...keptRefreshWhere }, + select: { accessTokenHash: true }, + }); + const pairedAccessHashes = families + .map((row) => row.accessTokenHash) + .filter((hash): hash is string => hash !== null); + + const deleted = await tx.session.deleteMany({ where: { userId, ...(keptSessionId ? { id: { not: keptSessionId } } : {}), }, - }), - prisma.refreshToken.updateMany({ + }); + await tx.refreshToken.updateMany({ where: { userId, revokedAt: null, ...keptRefreshWhere }, - data: { revokedAt: new Date() }, - }), + data: { revokedAt }, + }); + // Only the access tokens the revoked refresh rows point at. A programmatic + // `hlk_` token has no refresh row and is never in this list. + const access = + pairedAccessHashes.length === 0 + ? { count: 0 } + : await tx.apiToken.updateMany({ + where: { + userId, + tokenHash: { in: pairedAccessHashes }, + revoked: false, + }, + data: { revoked: true }, + }); // v1.23 — "sign out everywhere" also drops every trusted device so a // remembered browser can no longer skip the second factor (§1.7: a device // cookie is killed on sign-out-everywhere). - prisma.trustedDevice.deleteMany({ where: { userId } }), + await tx.trustedDevice.deleteMany({ where: { userId } }); // v1.30.34 — and every step-up elevation. A user who signs out everywhere // expects nothing to remain that could act on the account; a live elevation // is exactly that. - prisma.stepUpElevation.deleteMany({ where: { userId } }), - ]); - return { sessionsRevoked: deleted.count }; + await tx.stepUpElevation.deleteMany({ where: { userId } }); + return { + sessionsRevoked: deleted.count, + accessTokensRevoked: access.count, + }; + }); } /** diff --git a/src/lib/idempotency.ts b/src/lib/idempotency.ts index d7e081f41..c9e2dbeb8 100644 --- a/src/lib/idempotency.ts +++ b/src/lib/idempotency.ts @@ -27,6 +27,7 @@ import { annotate } from "@/lib/logging/context"; import { isP2002 } from "@/lib/prisma-errors"; import { findActiveGrant } from "@/lib/sharing/grants"; import { decrypt, encrypt } from "@/lib/crypto"; +import { createHash } from "node:crypto"; const TTL_MS = 24 * 60 * 60 * 1000; /** @@ -70,10 +71,36 @@ const RECORD_SEPARATOR = "|"; * 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. + * + * The cell a request reads and claims, since v1.38.11: + * + * An own-record request under a cookie session or a wildcard token is keyed + * byte-for-byte as the client sent it — that contract is frozen by the unit + * tests and is what every existing client's retry depends on. Everything else + * folds the authority the request carries into the key, so a cell can only be + * hit again by a request the handler would answer the same way: + * + * - a delegated request carries the grant it acts under (`delegatedGrantFacet`), + * so a replaced or narrowed grant lands in a fresh cell and reaches the + * handler's own refusal instead of an earlier response; + * - a narrow-scoped Bearer token carries its own identity (`narrowTokenFacet`), + * so it replays only what it wrote itself and never a wildcard credential's + * response on a route outside its scope. + * + * The handler's `requireRecordAuth` remains the authority; the key only makes + * sure the cache cannot answer for a credential the handler has not seen. */ -function cellKey(clientKey: string, actingAccountId: string | null): string { - if (actingAccountId === null) return clientKey; - return `${actingAccountId}${RECORD_SEPARATOR}${clientKey}`; +function cellKey( + clientKey: string, + actingAccountId: string | null, + facets: readonly string[], +): string { + const parts = [ + ...(actingAccountId === null ? [] : [actingAccountId]), + ...facets, + clientKey, + ]; + return parts.join(RECORD_SEPARATOR); } export interface IdempotencyContext { @@ -399,70 +426,79 @@ function inflightConflictResponse(): Response { } /** - * Whether a cached delegated response may still be returned. + * The grant a delegated request acts under, as a key facet — or `null` when + * there is no live grant, in which case the request must not touch a cell at + * all and the handler issues its own refusal. * - * 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. + * The wrapper runs before a route can call `requireRecordAuth`, and it must + * not become an alternate way around that check. Until v1.38.11 the grant was + * consulted at replay time, but only for its existence: a delegate whose grant + * had been replaced by a narrower one, or whose scope had been edited in + * place, still matched the cell they filled under the wider grant and were + * handed that body back. Folding the grant's identity, level and scope into + * the key means a cell is reachable only under the exact authority it was + * written under; any change lands the retry in a fresh cell, where the handler + * decides. A missing, expired, revoked, or unreadable grant reads as `null` — + * the cache is an accelerator, and falling through to the handler is always + * the safe answer. */ -async function canReplayDelegatedResponse( +async function delegatedGrantFacet( actorUserId: string, recordUserId: string, -): Promise { +): Promise { try { - return ( - (await findActiveGrant({ - grantorId: recordUserId, - granteeId: actorUserId, - })) !== null - ); + const grant = await findActiveGrant({ + grantorId: recordUserId, + granteeId: actorUserId, + }); + if (!grant) return null; + const scopeFingerprint = createHash("sha256") + .update(JSON.stringify(grant.scopeJson ?? null)) + .digest("hex") + .slice(0, 16); + return `g:${grant.id}:${grant.access}:${scopeFingerprint}`; } 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; + return null; } } /** - * Is this request presenting a narrow, single-purpose Bearer token? + * The identity of a narrow, single-purpose Bearer token, as a key facet — or + * `null` for a cookie session, a wildcard token, or no Bearer header. * - * Asked only on the delegated-replay path, and it exists because a live grant - * is no longer the whole question there. A narrow scope is refused delegation - * outright by `requireRecordAuth` — before any grant is read — so a credential - * that would be refused by the handler must not be handed a cached delegated - * body instead. The actor may genuinely hold the grant and the row may be - * genuinely theirs; the point is that THIS credential is not one the grant was - * ever exercised through. + * A narrow token is refused delegation outright by `requireRecordAuth`, and + * refused any own-record route outside its scope by `requireAuth`. Neither + * refusal is cachable, so a narrow token can never fill a cell it should not + * have; what it could do, before v1.38.11, was READ one: the own-record cell + * is keyed by account, and a wildcard credential of the same account had + * filled it. Keying the narrow token's cells by the token itself keeps its own + * retries idempotent — an ingest client re-posting after a timeout still + * replays — while no cell written by another credential is reachable to it. * - * Authorises nothing, exactly like the resolver above: a true answer only - * declines to serve the cache, and the handler then issues its own refusal with - * the audit trail that belongs to it. One indexed single-row read, on a path - * that is already doing a grant lookup. + * Authorises nothing. An unreadable token reads as narrow, and then keys by + * the presented hash: falling into a private cell is always safe. One indexed + * single-row read, only when a Bearer header is present. */ -async function presentsNarrowScopedToken(): Promise { +async function narrowTokenFacet(): Promise { let authHeader: string | null = null; try { const headerList = await headers(); authHeader = headerList.get("authorization"); } catch { - return false; + return null; } - if (!authHeader?.startsWith("Bearer ")) return false; + if (!authHeader?.startsWith("Bearer ")) return null; + const presentedHash = hashToken(authHeader.slice(7)); const apiToken = await prisma.apiToken .findUnique({ - where: { tokenHash: hashToken(authHeader.slice(7)) }, - select: { permissions: true }, + where: { tokenHash: presentedHash }, + select: { id: true, permissions: true }, }) .catch(() => null); - // Unreadable reads as narrow: the cache is an accelerator, and falling - // through to the handler is always a safe answer. - if (!apiToken) return true; - return !apiToken.permissions.includes("*"); + if (!apiToken) return `t:${presentedHash.slice(0, 32)}`; + return apiToken.permissions.includes("*") ? null : `t:${apiToken.id}`; } export function withIdempotency< @@ -533,31 +569,34 @@ export function withIdempotency< return handler(...args); } + // The authority this request carries, folded into the cell key BEFORE any + // cell is read or claimed — see `cellKey`. Both facets are resolved here + // rather than at replay time so that a request the handler would refuse + // cannot read a cell at all, and a request the handler would accept lands + // in a cell only a request with the same authority can reach. + const facets: string[] = []; + if (claimedRecord !== null) { + const grantFacet = await delegatedGrantFacet(userId, claimedRecord); + if (grantFacet === null) { + // No live grant: neither read nor claim a cell. The route owns the + // stable 403 envelope and the audit trail that goes with it. + return handler(...args); + } + facets.push(grantFacet); + } + const tokenFacet = await narrowTokenFacet(); + if (tokenFacet !== null) facets.push(tokenFacet); + const url = new URL(request.url); const ctx: IdempotencyContext = { userId, - key: cellKey(key, claimedRecord), + key: cellKey(key, claimedRecord, facets), method: request.method, path: url.pathname, }; const cached = await findCached(ctx); if (cached?.kind === "replay") { - if ( - claimedRecord !== null && - (!(await canReplayDelegatedResponse(userId, claimedRecord)) || - // A narrow scope is refused delegation by the handler before any - // grant is read, so a live grant is not sufficient here. Without this - // arm, a cell filled earlier by the same person under a cookie - // session could be replayed by a single-purpose token naming the - // owner's record — no row written, but the owner's response body - // returned to a credential the handler would have turned away. - (await presentsNarrowScopedToken())) - ) { - // 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") { diff --git a/tests/integration/refresh-rotation-revoke-race.test.ts b/tests/integration/refresh-rotation-revoke-race.test.ts new file mode 100644 index 000000000..c79bb0892 --- /dev/null +++ b/tests/integration/refresh-rotation-revoke-race.test.ts @@ -0,0 +1,154 @@ +/** + * Refresh rotation versus a revocation that lands mid-flight. + * + * `rotateRefreshToken` reads the presented row as live, mints a new pair, and + * only then marks the old row consumed with a conditional update. Until + * v1.38.11 that condition checked `usedAt: null` alone, so a revocation + * landing between the read and the write — "sign out everywhere" from another + * device, a credential rotation — was overtaken: the update still matched, the + * caller walked away with a fresh pair minted a moment AFTER the family had + * been ended, and it was the one live login left. + * + * The race is made deterministic by hooking the lookup the rotation performs + * between minting and marking (the `findUnique` that fetches the new row's id) + * and revoking the family inside it. Real Postgres; the conditional update is + * the thing under test. + */ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { getPrismaClient, truncateAllTables } from "./setup"; + +const { issueAccessAndRefresh, rotateRefreshToken } = + await import("@/lib/auth/refresh-token"); +const { destroyOtherSessions } = await import("@/lib/auth/session"); +const { resolveBearerToken, BearerAuthError } = + await import("@/lib/auth/bearer"); +const { prisma } = await import("@/lib/db"); + +const NATIVE_POLICY = { + policy: "native" as const, + accessTokenDays: 1, + refreshTokenDays: 30, + tokenLabel: "native", +}; +const DEVICE_ID = "race-device-1"; + +async function seedUser(username: string) { + return getPrismaClient().user.create({ + data: { username, email: `${username}@example.test` }, + }); +} + +async function bearerVerdict(rawToken: string): Promise { + try { + await resolveBearerToken(rawToken, { kind: "wildcard-only" }); + } catch (error) { + if (error instanceof BearerAuthError) return error.reason; + throw error; + } + return "accepted"; +} + +beforeEach(async () => { + await truncateAllTables(getPrismaClient()); + vi.restoreAllMocks(); +}); + +describe("refresh rotation — revocation landing between read and consume", () => { + it("does not hand out a live pair on a family revoked mid-rotation", async () => { + const user = await seedUser("race-owner"); + const phone = await issueAccessAndRefresh({ + userId: user.id, + policy: NATIVE_POLICY, + deviceId: DEVICE_ID, + source: "test", + }); + // A second, web session is the one pressing "sign out everywhere". + const browser = await getPrismaClient().session.create({ + data: { userId: user.id, expiresAt: new Date(Date.now() + 1e6) }, + }); + + // Hook the lookup between "mint" and "mark consumed", and end the family + // from the browser session inside it — after the rotation has read the + // old row as live and after the new pair exists, before the old row is + // marked. Exactly the window the old `usedAt: null` guard did not cover. + const original = prisma.refreshToken.findUnique.bind(prisma.refreshToken); + let revokedInsideTheWindow = false; + // Prisma's delegate returns a lazy thenable rather than a Promise; the + // spy's implementation type is widened for that alone. + const spy = vi + .spyOn(prisma.refreshToken, "findUnique") + .mockImplementation((async (args: unknown) => { + if (!revokedInsideTheWindow) { + revokedInsideTheWindow = true; + await destroyOtherSessions(user.id, { + kind: "session", + sessionId: browser.id, + }); + } + return original(args as never); + }) as never); + + const result = await rotateRefreshToken({ + refreshToken: phone.refreshToken, + policy: NATIVE_POLICY, + deviceId: DEVICE_ID, + }); + spy.mockRestore(); + + expect(revokedInsideTheWindow).toBe(true); + // The rotation must lose, and must say why in the word the client acts on. + expect(result).toEqual({ ok: false, reason: "revoked" }); + + // No live refresh token on the family, and nothing minted survives. + const live = await getPrismaClient().refreshToken.findMany({ + where: { userId: user.id, revokedAt: null }, + }); + expect(live).toHaveLength(0); + const liveAccess = await getPrismaClient().apiToken.findMany({ + where: { userId: user.id, revoked: false }, + }); + expect(liveAccess).toHaveLength(0); + expect(await bearerVerdict(phone.accessToken)).toBe("revoked"); + }); + + it("POSITIVE CONTROL: the same rotation without a revocation succeeds", async () => { + const user = await seedUser("race-control"); + const phone = await issueAccessAndRefresh({ + userId: user.id, + policy: NATIVE_POLICY, + deviceId: DEVICE_ID, + source: "test", + }); + const result = await rotateRefreshToken({ + refreshToken: phone.refreshToken, + policy: NATIVE_POLICY, + deviceId: DEVICE_ID, + }); + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(await bearerVerdict(result.bundle.accessToken)).toBe("accepted"); + }); + + it("still reports a plain double-consume as already_used", async () => { + const user = await seedUser("race-double"); + const phone = await issueAccessAndRefresh({ + userId: user.id, + policy: NATIVE_POLICY, + deviceId: DEVICE_ID, + source: "test", + }); + const first = await rotateRefreshToken({ + refreshToken: phone.refreshToken, + policy: NATIVE_POLICY, + deviceId: DEVICE_ID, + }); + expect(first.ok).toBe(true); + const replay = await rotateRefreshToken({ + refreshToken: phone.refreshToken, + policy: NATIVE_POLICY, + deviceId: DEVICE_ID, + }); + expect(replay).toEqual({ ok: false, reason: "already_used" }); + }); +}); diff --git a/tests/integration/sessions-and-known-devices.test.ts b/tests/integration/sessions-and-known-devices.test.ts index c54e97f91..bd80a95f7 100644 --- a/tests/integration/sessions-and-known-devices.test.ts +++ b/tests/integration/sessions-and-known-devices.test.ts @@ -149,3 +149,127 @@ describe("UserKnownDevice unique index", () => { expect(rows).toHaveLength(2); }); }); + +describe("destroyOtherSessions — native device logins", () => { + it("revokes the access token paired with every other refresh token, and nothing else", async () => { + const prisma = getPrismaClient(); + const user = await makeUser("sess-native-owner"); + const current = await prisma.session.create({ + data: { userId: user.id, expiresAt: new Date(Date.now() + 1e6) }, + }); + // Two phones: each is a RefreshToken pointing at its login ApiToken. + for (const n of ["a", "b"]) { + await prisma.apiToken.create({ + data: { + userId: user.id, + name: `native-${n}`, + tokenHash: `access-${n}`, + permissions: ["*"], + }, + }); + await prisma.refreshToken.create({ + data: { + userId: user.id, + tokenHash: `refresh-${n}`, + accessTokenHash: `access-${n}`, + expiresAt: new Date(Date.now() + 1e6), + }, + }); + } + // A programmatic token: no refresh row, managed under /settings/api-tokens. + const automation = await prisma.apiToken.create({ + data: { + userId: user.id, + name: "automation", + tokenHash: "hlk-automation", + permissions: ["measurements:write"], + }, + }); + // Another account's phone must be untouched. + const other = await makeUser("sess-native-other"); + await prisma.apiToken.create({ + data: { + userId: other.id, + name: "native-other", + tokenHash: "access-other", + permissions: ["*"], + }, + }); + await prisma.refreshToken.create({ + data: { + userId: other.id, + tokenHash: "refresh-other", + accessTokenHash: "access-other", + expiresAt: new Date(Date.now() + 1e6), + }, + }); + + const result = await destroyOtherSessions(user.id, { + kind: "session", + sessionId: current.id, + }); + expect(result.accessTokensRevoked).toBe(2); + + const revoked = await prisma.apiToken.findMany({ + where: { tokenHash: { in: ["access-a", "access-b"] } }, + select: { revoked: true }, + }); + expect(revoked.map((t) => t.revoked)).toEqual([true, true]); + const kept = await prisma.apiToken.findMany({ + where: { tokenHash: { in: [automation.tokenHash, "access-other"] } }, + select: { tokenHash: true, revoked: true }, + }); + expect(kept.every((t) => t.revoked === false)).toBe(true); + const otherRefresh = await prisma.refreshToken.findUnique({ + where: { tokenHash: "refresh-other" }, + }); + expect(otherRefresh?.revokedAt).toBeNull(); + }); + + it("spares the calling phone when the caller is a Bearer device login", async () => { + const prisma = getPrismaClient(); + const user = await makeUser("sess-native-caller"); + for (const n of ["caller", "other"]) { + await prisma.apiToken.create({ + data: { + userId: user.id, + name: `native-${n}`, + tokenHash: `access-${n}`, + permissions: ["*"], + }, + }); + await prisma.refreshToken.create({ + data: { + userId: user.id, + tokenHash: `refresh-${n}`, + accessTokenHash: `access-${n}`, + expiresAt: new Date(Date.now() + 1e6), + }, + }); + } + await prisma.session.create({ + data: { userId: user.id, expiresAt: new Date(Date.now() + 1e6) }, + }); + + const result = await destroyOtherSessions(user.id, { + kind: "accessToken", + accessTokenHash: "access-caller", + }); + // A Bearer caller has no session row, so every browser session goes. + expect(result.sessionsRevoked).toBe(1); + expect(result.accessTokensRevoked).toBe(1); + + const caller = await prisma.refreshToken.findUnique({ + where: { tokenHash: "refresh-caller" }, + }); + expect(caller?.revokedAt).toBeNull(); + const callerAccess = await prisma.apiToken.findUnique({ + where: { tokenHash: "access-caller" }, + }); + expect(callerAccess?.revoked).toBe(false); + const otherAccess = await prisma.apiToken.findUnique({ + where: { tokenHash: "access-other" }, + }); + expect(otherAccess?.revoked).toBe(true); + }); +});