From 7a9b626fb3d76b0cc7ebdc9e2412d4a50fce7692 Mon Sep 17 00:00:00 2001 From: dormouse-bot <287024035+dormouse-bot@users.noreply.github.com> Date: Fri, 2 Oct 2026 13:58:42 +0000 Subject: [PATCH 1/2] fix: compare expiries after the advisory lock wait in Hosted's locked transactions `now()` is the transaction's start, read at BEGIN before pg_advisory_xact_lock waits, so a setup token that expired while its restore waited was reinserted and could evict a live one. Compare against statement_timestamp() (LOCKED_NOW) instead. Closes #911 --- docs/specs/hosted.md | 1 + hosted/server/relay-api.ts | 11 ++++++----- hosted/server/relay-auth.ts | 7 +++++++ hosted/server/tests/relay.test.ts | 29 +++++++++++++++++++++++++++++ scripts/spec-word-budgets.json | 2 +- 5 files changed, 44 insertions(+), 6 deletions(-) diff --git a/docs/specs/hosted.md b/docs/specs/hosted.md index 39c46374a..ee8981c45 100644 --- a/docs/specs/hosted.md +++ b/docs/specs/hosted.md @@ -118,6 +118,7 @@ A session-gated route answers a session of an account no longer entitled with th - **Must sweep every Relay table's expired rows from the relay's hourly Cron Trigger** (rationale); sign-in challenges, minted unauthenticated and flat, are bounded by it and the per-address limit. - **Must answer 429 with `Retry-After` past the per-address limit on `signin/*` (`RELAY_SIGNIN_LIMIT`) and `setup/begin`/`finish` (`RELAY_SETUP_LIMIT`)**, before the body limit and any database read: 30 a minute, a ceremony's two routes sharing one budget (rationale). - **Must restore a token a refused `finish` spent on its original expiry, within the Burrow's cap, and never once that expiry has passed.** +- **Must compare expiries under an advisory lock against `LOCKED_NOW` in `hosted/server/relay-auth.ts`**, never `now()`, which predates the lock wait. **Push.** The push routes keep `docs/specs/relay.md` -> "Web Push" and its "State files" upsert rules; a send is HTTPS from the Burrow to the relay Worker, independent of terminal transport. diff --git a/hosted/server/relay-api.ts b/hosted/server/relay-api.ts index 3db77c690..9543d08a8 100644 --- a/hosted/server/relay-api.ts +++ b/hosted/server/relay-api.ts @@ -63,6 +63,7 @@ import { relayPushRoutes } from "./relay-push"; import { OWNER_COLUMNS, database, + LOCKED_NOW, locked, ownerOf, requireBurrow, @@ -522,7 +523,7 @@ export function relayApiRoutes(app: Hono<{ Bindings: RelayEnv }>) { `WITH spent AS ( UPDATE dormouse_relay_enrollment_approvals SET "redeemedBurrowId" = $3, "redeemedAt" = now() - WHERE "userCode" = $1 AND "userId" = $2 AND "expiresAt" > now() + WHERE "userCode" = $1 AND "userId" = $2 AND "expiresAt" > ${LOCKED_NOW} AND "redeemedAt" IS NULL RETURNING "userId" ) @@ -537,7 +538,7 @@ export function relayApiRoutes(app: Hono<{ Bindings: RelayEnv }>) { rows: [spent], } = await db.query<{ redeemedBurrowId: string }>( `SELECT "redeemedBurrowId" FROM dormouse_relay_enrollment_approvals - WHERE "userCode" = $1 AND "expiresAt" > now() AND "redeemedBurrowId" IS NOT NULL`, + WHERE "userCode" = $1 AND "expiresAt" > ${LOCKED_NOW} AND "redeemedBurrowId" IS NOT NULL`, [userCode], ); return spent ? { redeemed: spent.redeemedBurrowId } : "expired"; @@ -660,16 +661,16 @@ export async function admit( return locked(db, `${table}:${ownerValue}`, async () => { const { rows } = await db.query<{ expiresAt: number }>( `WITH pruned AS ( - DELETE FROM ${table} WHERE ${owner} = $1 AND "expiresAt" <= now() + DELETE FROM ${table} WHERE ${owner} = $1 AND "expiresAt" <= ${LOCKED_NOW} ), trimmed AS ( DELETE FROM ${table} WHERE ${pk} IN ( - SELECT ${pk} FROM ${table} WHERE ${owner} = $1 AND "expiresAt" > now() + SELECT ${pk} FROM ${table} WHERE ${owner} = $1 AND "expiresAt" > ${LOCKED_NOW} ORDER BY "expiresAt" DESC, ${pk} OFFSET $2 ) ) INSERT INTO ${table} (${columns.map((column) => `"${column}"`).join(", ")}, "expiresAt") SELECT ${columns.map((_, i) => `$${i + 3}`).join(", ")}, ${expiresAt} - WHERE ${expiresAt} > now() + WHERE ${expiresAt} > ${LOCKED_NOW} ON CONFLICT (${pk}) DO NOTHING RETURNING ${epochMs('"expiresAt"')} AS "expiresAt"`, [ownerValue, cap - 1, ...values, expiry], diff --git a/hosted/server/relay-auth.ts b/hosted/server/relay-auth.ts index 2102c25fd..e44988172 100644 --- a/hosted/server/relay-auth.ts +++ b/hosted/server/relay-auth.ts @@ -110,6 +110,13 @@ export function database( return withClient(c.env.HYPERDRIVE.connectionString, action); } +/** + * "Now" for a liveness check inside `locked`'s action: the statement's start, + * after the lock wait. Not `now()`, the transaction's start before that wait, + * so a row that expired while its writer waited would still read as live. + */ +export const LOCKED_NOW = "statement_timestamp()"; + /** Runs `action` in a transaction holding `key`'s advisory lock, so a cap check and its insert cannot interleave. */ export async function locked(db: Client, key: string, action: () => Promise) { await db.query("BEGIN"); diff --git a/hosted/server/tests/relay.test.ts b/hosted/server/tests/relay.test.ts index 3f24d1f47..facec4ab6 100644 --- a/hosted/server/tests/relay.test.ts +++ b/hosted/server/tests/relay.test.ts @@ -765,6 +765,35 @@ test("restoring a setup token that has since expired never evicts a live one", a expect(after).toContain(digest(back)); }); +test("a setup token that expires while its restore waits on the lock is not restored", async ({ + onTestFinished, +}) => { + const f = await fixture(); + onTestFinished(f.close); + const owner = await f.account(ADMIN_EMAIL); + const laptop = await f.burrow(owner); + const live: string[] = []; + for (let i = 0; i < MAX_TOKENS_PER_BURROW; i++) live.push(digest(await f.setupToken(laptop.token))); + const expiresAt = new Date(Date.now() + 1000); + // Another write holds the Burrow's token lock across the restored token's expiry. + await withClient(f.url, async (holder) => { + await holder.query("BEGIN"); + await holder.query("SELECT pg_advisory_xact_lock(hashtextextended($1, 0))", [ + `dormouse-relay:dormouse_relay_setup_tokens:${laptop.burrowId}`, + ]); + const restoring = withClient(f.url, (db) => + restoreSetupToken(db, randomSecret(), { burrowId: laptop.burrowId, userId: owner, expiresAt }), + ); + await new Promise((resolve) => setTimeout(resolve, expiresAt.getTime() - Date.now() + 500)); + await holder.query("COMMIT"); + await restoring; + }); + const hashes = (await f.sql<{ tokenHash: string }>(`SELECT "tokenHash" FROM dormouse_relay_setup_tokens`)) + .map((row) => row.tokenHash) + .sort(); + expect(hashes).toEqual([...live].sort()); +}); + test("a de-entitled account signs in to nothing, and its sessions answer as expired", async ({ onTestFinished, }) => { diff --git a/scripts/spec-word-budgets.json b/scripts/spec-word-budgets.json index 238186ff8..40f3ab145 100644 --- a/scripts/spec-word-budgets.json +++ b/scripts/spec-word-budgets.json @@ -12,7 +12,7 @@ "docs/specs/dor-tools-builtin.md": 1100, "docs/specs/dor-tools-lib.md": 400, "docs/specs/glossary.md": 3000, - "docs/specs/hosted.md": 4800, + "docs/specs/hosted.md": 4850, "docs/specs/layout.md": 12050, "docs/specs/mobile-terminal-ui.md": 2300, "docs/specs/mouse-and-clipboard.md": 5250, From 6f37695b7223c8cc663dea9fc338012ecd5f81f2 Mon Sep 17 00:00:00 2001 From: dormouse-bot <287024035+dormouse-bot@users.noreply.github.com> Date: Fri, 2 Oct 2026 14:10:01 +0000 Subject: [PATCH 2/2] fix: an expired row admit refuses no longer trims a live one The trimmed CTE ran whether or not the insert's guard admitted the row, so a restore refused as expired still evicted the Burrow's oldest live token at the cap. --- hosted/server/relay-api.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hosted/server/relay-api.ts b/hosted/server/relay-api.ts index 9543d08a8..38bef5bd5 100644 --- a/hosted/server/relay-api.ts +++ b/hosted/server/relay-api.ts @@ -643,8 +643,8 @@ async function consumeChallenge(db: Client, challenge: string, burrowId: string * in one statement: its own expired rows pruned, its live rows trimmed to * leave one slot under the cap (latest expiry kept), then the row. No other * owner's rows are read. `expiry` is a TTL in milliseconds or an instant; a - * row already expired is not inserted. Answers its expiry in epoch - * milliseconds, or undefined when nothing was inserted. + * row already expired is not inserted and trims nothing. Answers its expiry + * in epoch milliseconds, or undefined when nothing was inserted. */ export async function admit( db: Client, @@ -663,7 +663,7 @@ export async function admit( `WITH pruned AS ( DELETE FROM ${table} WHERE ${owner} = $1 AND "expiresAt" <= ${LOCKED_NOW} ), trimmed AS ( - DELETE FROM ${table} WHERE ${pk} IN ( + DELETE FROM ${table} WHERE ${expiresAt} > ${LOCKED_NOW} AND ${pk} IN ( SELECT ${pk} FROM ${table} WHERE ${owner} = $1 AND "expiresAt" > ${LOCKED_NOW} ORDER BY "expiresAt" DESC, ${pk} OFFSET $2 )