From 72dc154185ed3fef5e0c3aeef0e944b35b09c5f6 Mon Sep 17 00:00:00 2001 From: Shkumbin Sherifi Date: Tue, 14 Jul 2026 13:39:05 +0200 Subject: [PATCH] Fix OAuth state across Render restarts --- SECURITY.md | 2 +- scripts/session.test.mjs | 15 ++++++++++++ src/pages/api/auth/callback.ts | 6 ++--- src/pages/api/auth/login.ts | 4 ++-- src/server/session.ts | 43 ++++++++++++++++++++++------------ 5 files changed, 49 insertions(+), 21 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index def2d93..e3c1b1b 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -21,7 +21,7 @@ GitHub profile to request a private channel before sharing exploit details. - Never requests repository, organization, code, issue, workflow, administration, or write permissions. - Stores expiring user and refresh tokens only on the server, keyed by an opaque session id. Tokens never reach the browser, never enter `localStorage`, and are never logged. - Requires user-to-server token expiration and rotates a token if it approaches expiry. -- Protects the authorization-code flow with both a one-time `state` value and PKCE (`S256`). OAuth state expires after 10 minutes. +- Protects the authorization-code flow with a short-lived signed `state` cookie and PKCE (`S256`). The verifier stays in the HttpOnly cookie so an authorization in progress survives a server restart; it expires after 10 minutes. - Uses the configured `APP_ORIGIN` for the callback URL instead of trusting the incoming request host in production. - Keeps sessions in memory for one hour and caches fetched stars for 60 seconds. Both disappear when the server restarts. diff --git a/scripts/session.test.mjs b/scripts/session.test.mjs index f7af7a3..3c156b8 100644 --- a/scripts/session.test.mjs +++ b/scripts/session.test.mjs @@ -9,8 +9,10 @@ const { destroySession, getCachedStars, getSession, + newOAuthRequest, reserveStarsFetch, signSessionId, + consumeOAuthState, verifySignedSessionId, } = await import("../src/server/session.ts"); @@ -36,6 +38,19 @@ test("signed session ids reject tampering", () => { destroySession(sessionId); }); +test("OAuth request cookies carry a signed PKCE verifier", () => { + const request = newOAuthRequest(); + const verifier = consumeOAuthState(request.state, request.cookieValue); + + assert.ok(verifier); + assert.ok(verifier.length >= 43); + assert.equal(consumeOAuthState(`${request.state}x`, request.cookieValue), null); + + const last = request.cookieValue.at(-1); + const tampered = `${request.cookieValue.slice(0, -1)}${last === "0" ? "1" : "0"}`; + assert.equal(consumeOAuthState(request.state, tampered), null); +}); + test("star caches remain isolated between user sessions", () => { const aliceId = createSession(token("alice"), "alice"); const bobId = createSession(token("bob"), "bob"); diff --git a/src/pages/api/auth/callback.ts b/src/pages/api/auth/callback.ts index 184c869..da49391 100644 --- a/src/pages/api/auth/callback.ts +++ b/src/pages/api/auth/callback.ts @@ -24,11 +24,11 @@ export const GET: APIRoute = async ({ request, url, cookies, redirect }) => { return new Response("Missing code or state", { status: 400 }); } - const cookieState = cookies.get(OAUTH_STATE_COOKIE)?.value; + const oauthRequest = cookies.get(OAUTH_STATE_COOKIE)?.value; cookies.delete(OAUTH_STATE_COOKIE, { path: "/" }); - const codeVerifier = cookieState === state ? consumeOAuthState(state) : null; - if (!cookieState || cookieState !== state || !codeVerifier) { + const codeVerifier = oauthRequest ? consumeOAuthState(state, oauthRequest) : null; + if (!codeVerifier) { return new Response("Invalid OAuth state. Please try signing in again.", { status: 400, }); diff --git a/src/pages/api/auth/login.ts b/src/pages/api/auth/login.ts index a3d43b9..fac6560 100644 --- a/src/pages/api/auth/login.ts +++ b/src/pages/api/auth/login.ts @@ -16,8 +16,8 @@ export const GET: APIRoute = async ({ request, cookies, redirect }) => { { status: 503, headers: { "content-type": "text/plain" } } ); } - const { state, codeChallenge } = newOAuthRequest(); - cookies.set(OAUTH_STATE_COOKIE, state, { + const { state, codeChallenge, cookieValue } = newOAuthRequest(); + cookies.set(OAUTH_STATE_COOKIE, cookieValue, { httpOnly: true, secure: useSecureCookies(request), sameSite: "lax", diff --git a/src/server/session.ts b/src/server/session.ts index 1e24f36..4d48c28 100644 --- a/src/server/session.ts +++ b/src/server/session.ts @@ -1,9 +1,9 @@ /** * In-memory session store. GitHub App user and refresh tokens never leave the server. * - * v1 limitation: this is a single-process Map. Fine for self-hosting and - * local dev. For multi-instance production deploys, swap this file's body - * for a Redis/KV client. The exported interface must stay the same. + * User sessions remain single-process in v1. OAuth requests are instead kept + * in a signed, short-lived cookie so the GitHub callback survives a process + * restart or a different instance. */ import { randomBytes, createHash, createHmac, timingSafeEqual } from "node:crypto"; import type { StarredData } from "../scripts/treemap"; @@ -26,7 +26,6 @@ const COOKIE_NAME = "gitvis_session"; const STATE_COOKIE = "gitvis_oauth_state"; const sessions = new Map(); -const pendingStates = new Map(); export function signSessionId(sessionId: string): string { const h = createHmac("sha256", getSessionSecret()).update(sessionId).digest("hex"); @@ -103,23 +102,37 @@ function pruneExpired(): void { export function newOAuthRequest(): { state: string; codeChallenge: string; + cookieValue: string; } { const state = randomBytes(16).toString("hex"); const codeVerifier = randomBytes(32).toString("base64url"); const codeChallenge = createHash("sha256").update(codeVerifier).digest("base64url"); - pendingStates.set(state, { createdAt: Date.now(), codeVerifier }); - for (const [key, pending] of pendingStates) { - if (Date.now() - pending.createdAt > OAUTH_STATE_TTL_MS) pendingStates.delete(key); - } - return { state, codeChallenge }; + const payload = `${state}.${Date.now()}.${codeVerifier}`; + const signature = createHmac("sha256", getSessionSecret()).update(payload).digest("hex"); + return { state, codeChallenge, cookieValue: `${payload}.${signature}` }; } -export function consumeOAuthState(state: string): string | null { - const pending = pendingStates.get(state); - if (!pending) return null; - pendingStates.delete(state); - if (Date.now() - pending.createdAt > OAUTH_STATE_TTL_MS) return null; - return pending.codeVerifier; +export function consumeOAuthState(state: string, cookieValue: string): string | null { + const parts = cookieValue.split("."); + if (parts.length !== 4) return null; + const [cookieState, createdAtValue, codeVerifier, signature] = parts; + if (cookieState !== state) return null; + + const createdAt = Number(createdAtValue); + const age = Date.now() - createdAt; + if (!Number.isFinite(createdAt) || age < 0 || age > OAUTH_STATE_TTL_MS) return null; + + const payload = `${cookieState}.${createdAtValue}.${codeVerifier}`; + const expected = createHmac("sha256", getSessionSecret()).update(payload).digest("hex"); + try { + const actualBytes = Buffer.from(signature, "hex"); + const expectedBytes = Buffer.from(expected, "hex"); + if (actualBytes.length !== expectedBytes.length) return null; + if (!timingSafeEqual(actualBytes, expectedBytes)) return null; + } catch { + return null; + } + return codeVerifier; } export const SESSION_COOKIE_NAME = COOKIE_NAME;