diff --git a/src/app.ts b/src/app.ts index 8ef4547..2707bbe 100644 --- a/src/app.ts +++ b/src/app.ts @@ -1,7 +1,7 @@ import { createHmac, randomUUID } from "node:crypto"; import cookie from "@fastify/cookie"; import cors from "@fastify/cors"; -import Fastify, { type FastifyError, type FastifyInstance, type FastifyReply } from "fastify"; +import Fastify, { type FastifyError, type FastifyInstance, type FastifyReply, type FastifyRequest } from "fastify"; import { type AppCaller, isAppCaller, resolveAppCaller } from "./app-caller.js"; import { deliverBackendHandoff, type BackendHandoffPayload } from "./backend-handoff.js"; import { @@ -1094,6 +1094,38 @@ export async function buildApp(options: BuildAppOptions = {}): Promise { + reply.code(400); + const profile = getAppProfile(state.app_slug); + if (prefersHtml(request.headers.accept) && profile && isAllowedReturnOrigin(profile, state.return_to)) { + reply.type("text/html; charset=utf-8"); + return reply.send( + renderBrowserHandoffPage({ + handoffKind: (state.handoff ?? { kind: "browser_completion" }).kind, + status: "error", + provider: state.provider, + appSlug: state.app_slug, + mode: state.mode, + returnTo: state.return_to, + error, + }) + ); + } + return { error }; + }; + app.get<{ Params: { provider: Provider }; Querystring: { state: string }; @@ -1140,8 +1172,7 @@ export async function buildApp(options: BuildAppOptions = {}): Promise => { + if (handoff.kind !== "browser_completion" || !handoff.attemptId) return; + await store.updateAuthAttempt(statePayload.app_slug, handoff.attemptId, { + status: "denied", + at: new Date().toISOString(), + denialCode, + }); + }; // The cookie is checked before the claim: a stranger without it must not be // able to burn the state that the browser that launched it still needs. + // The attempt is denied, though: a launcher whose cookie lapsed can never + // complete this state, and its page polls an attempt that would otherwise + // sit pending until it expires. if (!oauthBindingHolds(config, keys, request.cookies[oauthBindingCookieName(statePayload.jti)], statePayload)) { - reply.code(400); - return { error: "state_not_bound" }; + await denyBrowserAttempt("state_not_bound"); + return refuseVerifiedState(request, reply, statePayload, "state_not_bound"); } setOAuthBindingCookie(reply, statePayload, "", 0); const firstCallback = await store.claimOAuthStateUse({ @@ -1216,18 +1259,8 @@ export async function buildApp(options: BuildAppOptions = {}): Promise => { - if (handoff.kind !== "browser_completion" || !handoff.attemptId) return; - await store.updateAuthAttempt(statePayload.app_slug, handoff.attemptId, { - status: "denied", - at: new Date().toISOString(), - denialCode, - }); - }; if (request.query.error) { const providerError = relayedProviderText(request.query.error, 64); diff --git a/src/store/in-memory.ts b/src/store/in-memory.ts index 2943e99..eafaea5 100644 --- a/src/store/in-memory.ts +++ b/src/store/in-memory.ts @@ -24,6 +24,9 @@ import { type UpsertLinkedIdentityInput, } from "./types.js"; +/** The skew a signed OAuth state is still accepted for past its exp (verifyOAuthState). */ +const OAUTH_STATE_SKEW_MS = 30_000; + function clone(value: T): T { return JSON.parse(JSON.stringify(value)) as T; } @@ -452,8 +455,10 @@ export class InMemoryStore implements HeimdallStore { } async claimOAuthStateUse(input: ClaimOAuthStateUseInput): Promise { + // A state is still accepted until its exp + 30 s skew, so a use record must outlive that. + const usedAtMs = Date.parse(input.usedAt); for (const [key, expiresAt] of this.oauthStateUses) { - if (expiresAt <= input.usedAt) { + if (Date.parse(expiresAt) + OAUTH_STATE_SKEW_MS <= usedAtMs) { this.oauthStateUses.delete(key); } } diff --git a/src/store/schema.ts b/src/store/schema.ts index 54f1fca..607445c 100644 --- a/src/store/schema.ts +++ b/src/store/schema.ts @@ -138,9 +138,9 @@ CREATE UNIQUE INDEX IF NOT EXISTS auth_completions_attempt_unconsumed_unique_idx -- One use per (state jti, phase): a signed OAuth state launches once and -- completes once. The first INSERT for a pair wins; ON CONFLICT DO NOTHING --- reports every later claim as no row. The store deletes nothing, so rows stay --- like consumed auth_completions; after expires_at a state is refused before --- any claim, so a row is never consulted again. +-- reports every later claim as no row. The Postgres store deletes nothing, so rows +-- stay like consumed auth_completions; a state is accepted until expires_at + 30 s +-- of skew, so a row may be pruned only after that and is never consulted again. CREATE TABLE IF NOT EXISTS oauth_state_uses ( jti TEXT NOT NULL, phase TEXT NOT NULL, diff --git a/tests/oauth-state-binding.test.ts b/tests/oauth-state-binding.test.ts index 3f8f0a5..9f576e8 100644 --- a/tests/oauth-state-binding.test.ts +++ b/tests/oauth-state-binding.test.ts @@ -176,13 +176,129 @@ describe("OAuth state binding", () => { expect(stranger.statusCode, query).toBe(400); expect(stranger.json(), query).toEqual({ error: "state_not_bound" }); } - expect((await ctx.store.findAuthAttempt("repixelizer", attempt.handle))?.status).toBe("pending"); - // The victim's own callback is unharmed. + // The state is still unclaimed, so the launcher's cookie still completes it... const response = await callback(app, victim.state, victim.cookie, "code=victim-code"); expect(response.statusCode).toBe(201); }); + it("a callback with no binding denies the attempt its state names, once, and never a completed one", async () => { + const app = await newApp(); + const ctx = getHeimdallRuntimeContext(app); + const make = async (handle: string) => { + await ctx.store.createAuthAttempt({ + handle, + appSlug: "repixelizer", + provider: "discord", + mode: "sign_in", + returnTo: "https://repixelizer.gamecult.org/app/", + createdAt: new Date().toISOString(), + expiresAt: new Date(Date.now() + 600_000).toISOString(), + }); + const caller = resolveAppCaller(ctx.config, "repixelizer", { "x-heimdall-app-secret": "repixelizer-secret" }); + const started = await startOAuthFlow( + ctx, + "discord", + caller, + { appSlug: "repixelizer", mode: "sign_in", returnTo: "https://repixelizer.gamecult.org/app/", handoff: { kind: "browser_completion" } }, + { trustedBrowserAttemptId: handle } + ); + return started.body as unknown as { authorizationUrl: string }; + }; + + // Lapsed cookie: the launcher launched, then its cookie is gone at the callback. + const lapsed = await make("lapsed"); + await launchFlow(app, lapsed); + const refused = await callback(app, stateOf(lapsed), undefined, "code=c"); + expect(refused.json()).toEqual({ error: "state_not_bound" }); + const denied = await ctx.store.findAuthAttempt("repixelizer", "lapsed"); + expect(denied?.status).toBe("denied"); + expect(denied?.denialCode).toBe("state_not_bound"); + + // A completed attempt stays completed under a later cookieless callback. + const done = await make("done"); + const launched = await launchFlow(app, done); + expect((await callback(app, launched.state, launched.cookie, "code=c")).statusCode).toBe(201); + await callback(app, launched.state, undefined, "code=c"); + expect(["completed", "consumed"]).toContain((await ctx.store.findAuthAttempt("repixelizer", "done"))?.status); + }); + + it("a state with no jti is refused as invalid and names no attempt to deny", async () => { + const app = await newApp(); + const ctx = getHeimdallRuntimeContext(app); + const original = decodeJwt(stateOf(await bifrostStart(app))).payload; + const { jti: _jti, ...rest } = original; + const token = signJwt(rest, ctx.keys); + const response = await callback(app, token, undefined, "code=c"); + expect(response.statusCode).toBe(400); + expect(response.json()).toEqual({ error: "invalid_state" }); + }); + + describe("a browser refused for its binding gets a page back to the app, not bare JSON", () => { + const html = { accept: "text/html" }; + const get = (app: FastifyInstance, url: string, cookie?: string) => + app.inject({ method: "GET", url, headers: { ...html, ...(cookie ? { cookie } : {}) } }); + + it("on a cookieless callback, a lapsed cookie and a reloaded launch", async () => { + const app = await newApp(); + const start = await bifrostStart(app); + const state = stateOf(start); + + const reloadBeforeLaunch = await get(app, `/v1/oauth/discord/callback?code=c&state=${encodeURIComponent(state)}`); + expect(reloadBeforeLaunch.statusCode).toBe(400); + expect(reloadBeforeLaunch.headers["content-type"]).toContain("text/html"); + expect(reloadBeforeLaunch.body).toContain("state_not_bound"); + expect(reloadBeforeLaunch.body).toContain("https://bifrost.gamecult.org/auth/heimdall/complete"); + + const victim = await launchFlow(app, start); + const reloaded = await get(app, launchUrl(start)); + expect(reloaded.statusCode).toBe(400); + expect(reloaded.headers["content-type"]).toContain("text/html"); + expect(reloaded.body).toContain("state_already_launched"); + expect(setCookieOf(reloaded)).toEqual([]); + + // Handoff reload: the page after a completed callback, loaded again. + expect((await get(app, `/v1/oauth/discord/callback?code=c&state=${encodeURIComponent(victim.state)}`, victim.cookie)).statusCode).toBeLessThan(300); + const reload = await get(app, `/v1/oauth/discord/callback?code=c&state=${encodeURIComponent(victim.state)}`, victim.cookie); + expect(reload.statusCode).toBe(400); + expect(reload.headers["content-type"]).toContain("text/html"); + expect(reload.body).toContain("state_already_used"); + }); + + it("never links to an origin the app does not own, and JSON stays JSON without Accept", async () => { + const app = await newApp(); + const ctx = getHeimdallRuntimeContext(app); + const original = decodeJwt(stateOf(await bifrostStart(app))).payload; + const token = signJwt({ ...original, return_to: "https://evil.example/steal" }, ctx.keys); + const evil = await get(app, `/v1/oauth/discord/callback?code=c&state=${encodeURIComponent(token)}`); + expect(evil.statusCode).toBe(400); + expect(evil.json()).toEqual({ error: "state_not_bound" }); + expect(evil.body).not.toContain("evil.example"); + + const plain = await callback(app, stateOf(await bifrostStart(app))); + expect(plain.json()).toEqual({ error: "state_not_bound" }); + }); + }); + + it("the memory store keeps a use record until the state's exp + 30 s skew", async () => { + const app = await newApp(); + const ctx = getHeimdallRuntimeContext(app); + const claim = (jti: string, usedAt: number) => + ctx.store.claimOAuthStateUse({ + jti, + phase: "launch", + appSlug: "bifrost", + usedAt: new Date(usedAt).toISOString(), + expiresAt: new Date(1_000_000).toISOString(), + }); + expect(await claim("a", 900_000)).toBe(true); + // Past exp but inside the skew a state still verifies, so its record must still refuse a replay. + expect(await claim("b", 1_000_000 + 29_999)).toBe(true); + expect(await claim("a", 1_000_000 + 29_999)).toBe(false); + expect(await claim("c", 1_000_000 + 30_000)).toBe(true); + expect(await claim("a", 1_000_000 + 30_000)).toBe(true); + }); + it("a provider error from the launching browser still denies the attempt, once", async () => { const app = await newApp(); const start = await bifrostStart(app); diff --git a/tests/store.test.ts b/tests/store.test.ts index 5474e0e..11614c4 100644 --- a/tests/store.test.ts +++ b/tests/store.test.ts @@ -216,14 +216,17 @@ describe("InMemoryStore.claimOAuthStateUse", () => { expect(await store.claimOAuthStateUse({ ...use, jti: "state-2" })).toBe(true); }); - it("drops a record once its state has expired, and keeps it until then", async () => { + it("drops a record once its state's exp + 30 s skew has passed, and keeps it until then", async () => { const store = new InMemoryStore(); await store.claimOAuthStateUse(use); // Before expiry a later claim of another state leaves the record in force. await store.claimOAuthStateUse({ ...use, jti: "state-2", usedAt: "2026-04-26T12:09:59.000Z" }); expect(await store.claimOAuthStateUse(use)).toBe(false); - // At and after expiry the record is dead weight: the state is refused before any claim. - await store.claimOAuthStateUse({ ...use, jti: "state-3", usedAt: "2026-04-26T12:10:00.000Z" }); + // Inside the 30 s skew past expiry the state still verifies, so the record still refuses a replay. + await store.claimOAuthStateUse({ ...use, jti: "state-3", usedAt: "2026-04-26T12:10:29.999Z" }); + expect(await store.claimOAuthStateUse(use)).toBe(false); + // At exp + 30 s the state is refused before any claim and the record is dead weight. + await store.claimOAuthStateUse({ ...use, jti: "state-4", usedAt: "2026-04-26T12:10:30.000Z" }); expect(await store.claimOAuthStateUse(use)).toBe(true); }); });