diff --git a/README.md b/README.md index c6e183c..d1dad0d 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,7 @@ A complete magic-link email auth flow: users enter their email, receive a login ### Security -- **CSRF protection** using the synchronizer token pattern with timing-safe comparison and origin validation +- **CSRF protection** using the synchronizer token pattern with timing-safe comparison and origin validation. A token that has aged out but still verifies against the session secret is recognised as stale rather than forged, so the form is re-rendered with a fresh token and the user's input intact instead of a dead-end 403 - **Rate limiting** middleware with configurable sliding-window limits per IP - **Signup spam defense** on the login form — an always-on honeypot and per-IP rate limit, plus an optional first-party proof-of-work captcha (no third party, account, or extra secret; off by default, enable with `CAPTCHA_ENABLED`) - **Session fixation prevention** — sessions are regenerated on login diff --git a/src/client/components/flash.css b/src/client/components/flash.css index 6686982..9773c11 100644 --- a/src/client/components/flash.css +++ b/src/client/components/flash.css @@ -13,3 +13,11 @@ border-radius: var(--radius); margin-bottom: 1rem; } + +.flash-warning { + border: 1px solid rgba(251, 191, 36, 0.25); + background: rgba(251, 191, 36, 0.08); + padding: 12px 16px; + border-radius: var(--radius); + margin-bottom: 1rem; +} diff --git a/src/server/components/flash.tsx b/src/server/components/flash.tsx index eb64ee5..9e8ee47 100644 --- a/src/server/components/flash.tsx +++ b/src/server/components/flash.tsx @@ -1,14 +1,21 @@ interface FlashProps { - type: "success" | "error"; + type: "success" | "error" | "warning"; children: React.ReactNode; } +const CLASS_NAMES = { + success: "flash-success", + error: "flash-error", + warning: "flash-warning", +} as const; + export const Flash = ({ type, children }: FlashProps) => ( - // role="alert" (assertive) for errors so screen readers interrupt and announce - // them; role="status" (polite) for success so it's announced without cutting - // off the user. Both are announced when injected after a post-redirect-get. + // role="alert" (assertive) for errors and warnings so screen readers + // interrupt and announce them - both require the user to act; role="status" + // (polite) for success so it's announced without cutting off the user. Both + // are announced when injected after a post-redirect-get.
{children} diff --git a/src/server/controllers/app/forms.test.ts b/src/server/controllers/app/forms.test.ts index 30252f1..00e7108 100644 --- a/src/server/controllers/app/forms.test.ts +++ b/src/server/controllers/app/forms.test.ts @@ -1,7 +1,16 @@ -import { afterAll, beforeEach, describe, expect, mock, test } from "bun:test"; +import { + afterAll, + afterEach, + beforeEach, + describe, + expect, + mock, + setSystemTime, + test, +} from "bun:test"; import { SQL } from "bun"; import { findOrCreateUser } from "../../services/auth"; -import { createCsrfToken } from "../../services/csrf"; +import { createCsrfToken, TIME_WINDOW_MINUTES } from "../../services/csrf"; import { createAuthenticatedSession, createGuestSession, @@ -30,11 +39,24 @@ describe("Forms Controller", () => { await cleanupTestData(db); }); + afterEach(() => { + setSystemTime(); + }); + afterAll(async () => { await connection.end(); mock.restore(); }); + // Mint a token as if the page had rendered several windows ago: authentic, + // but too stale to act on. + const mintStaleToken = async (sessionId: string): Promise => { + setSystemTime(new Date(Date.now() - TIME_WINDOW_MINUTES * 3 * 60 * 1000)); + const token = await createCsrfToken(sessionId, "POST", "/forms"); + setSystemTime(); + return token; + }; + const createTestSession = async () => { const user = await findOrCreateUser(randomEmail()); return createAuthenticatedSession(user.id); @@ -150,7 +172,7 @@ describe("Forms Controller", () => { expect(setCookie).toContain("Alex"); }); - test("redirects without flash when name is missing", async () => { + test("redirects with a validation flash when name is missing", async () => { const sessionId = await createGuestSession(); const cookieHeader = `session_id=${sessionId}`; const csrfToken = await createCsrfToken(sessionId, "POST", "/forms"); @@ -174,16 +196,18 @@ describe("Forms Controller", () => { expect(response.headers.get("location")).toBe("/forms"); const setCookie = findSetCookie(request, "flash_state"); - expect(setCookie).toBeUndefined(); + expect(setCookie).toBeDefined(); + expect(setCookie).toContain("validation-error"); }); - test("redirects without flash when name is too short", async () => { + test("preserves submitted values when name is too short", async () => { const sessionId = await createGuestSession(); const cookieHeader = `session_id=${sessionId}`; const csrfToken = await createCsrfToken(sessionId, "POST", "/forms"); const mockFormData = new FormData(); mockFormData.append("name", "ab"); + mockFormData.append("email", "alex@example.com"); mockFormData.append("_csrf", csrfToken); const request = createBunRequest("http://localhost:3000/forms", { @@ -201,7 +225,10 @@ describe("Forms Controller", () => { expect(response.headers.get("location")).toBe("/forms"); const setCookie = findSetCookie(request, "flash_state"); - expect(setCookie).toBeUndefined(); + expect(setCookie).toBeDefined(); + expect(setCookie).toContain("validation-error"); + expect(setCookie).toContain("ab"); + expect(setCookie).toContain("alex@example.com"); }); test("rejects request without CSRF token", async () => { @@ -301,5 +328,107 @@ describe("Forms Controller", () => { expect(setCookie).toBeDefined(); expect(setCookie).toContain("submission-success"); }); + + test("preserves submitted values when the CSRF token has expired", async () => { + const sessionId = await createGuestSession(); + const staleToken = await mintStaleToken(sessionId); + + const mockFormData = new FormData(); + mockFormData.append("name", "Alex"); + mockFormData.append("email", "alex@example.com"); + mockFormData.append("message", "Hello world"); + mockFormData.append("_csrf", staleToken); + + const request = createBunRequest("http://localhost:3000/forms", { + method: "POST", + headers: { + Origin: "http://localhost:3000", + Cookie: `session_id=${sessionId}`, + }, + body: mockFormData, + }); + + const response = await forms.create(request); + + expect(response.status).toBe(303); + expect(response.headers.get("location")).toBe("/forms"); + + const setCookie = findSetCookie(request, "flash_state"); + expect(setCookie).toBeDefined(); + expect(setCookie).toContain("csrf-expired"); + expect(setCookie).toContain("Alex"); + expect(setCookie).toContain("alex@example.com"); + expect(setCookie).toContain("Hello world"); + // The stale submission must never be recorded as a success. + expect(setCookie).not.toContain("submission-success"); + }); + + test("still hard-fails an expired token from a foreign origin", async () => { + const sessionId = await createGuestSession(); + const staleToken = await mintStaleToken(sessionId); + + const mockFormData = new FormData(); + mockFormData.append("name", "Alex"); + mockFormData.append("_csrf", staleToken); + + const request = createBunRequest("http://localhost:3000/forms", { + method: "POST", + headers: { + Origin: "http://evil.example", + Cookie: `session_id=${sessionId}`, + }, + body: mockFormData, + }); + + const response = await forms.create(request); + + expect(response.status).toBe(403); + expect(await response.text()).toBe("Invalid request origin"); + expect(findSetCookie(request, "flash_state")).toBeUndefined(); + }); + }); + + describe("CSRF expiry recovery round-trip", () => { + test("re-renders the form pre-filled with a fresh token", async () => { + const sessionId = await createGuestSession(); + + const request = createBunRequest("http://localhost:3000/forms", { + headers: { Cookie: `session_id=${sessionId}` }, + }); + + const { setFlash } = stateHelpers(); + setFlash(request, { + state: "csrf-expired", + name: "Alex", + email: "alex@example.com", + message: "Hello world", + }); + + const response = await forms.index(request); + const html = await response.text(); + + expect(html).toContain("Your session timed out"); + expect(html).toContain('value="Alex"'); + expect(html).toContain('value="alex@example.com"'); + expect(html).toContain("Hello world"); + expect(html).toContain('name="_csrf"'); + }); + + test("does not pre-fill the form after a successful submission", async () => { + const sessionId = await createGuestSession(); + + const request = createBunRequest("http://localhost:3000/forms", { + headers: { Cookie: `session_id=${sessionId}` }, + }); + + const { setFlash } = stateHelpers(); + setFlash(request, { state: "submission-success", name: "Alex" }); + + const response = await forms.index(request); + const html = await response.text(); + + expect(html).toContain("Submitted successfully"); + expect(html).not.toContain('value="Alex"'); + }); }); }); diff --git a/src/server/controllers/app/forms.tsx b/src/server/controllers/app/forms.tsx index 8152466..8c62318 100644 --- a/src/server/controllers/app/forms.tsx +++ b/src/server/controllers/app/forms.tsx @@ -1,15 +1,22 @@ import type { BunRequest } from "bun"; import { getSessionContext } from "../../middleware/auth"; -import { csrfProtection } from "../../middleware/csrf"; +import { checkCsrf, isRecoverableCsrfFailure } from "../../middleware/csrf"; import { createCsrfToken } from "../../services/csrf"; import { setSessionCookie } from "../../services/sessions"; import type { FormsState } from "../../templates/forms"; import { Forms } from "../../templates/forms"; +import { readFormValues } from "../../utils/form-data"; import { redirect, render } from "../../utils/response"; -import { stateHelpers } from "../../utils/state"; +import { fitFlashState, stateHelpers } from "../../utils/state"; const { getFlash, setFlash } = stateHelpers(); +const FORM_FIELDS = ["name", "email", "message"] as const; + +// Ordered longest-first: sacrifice the message before the short fields when +// the preserved values don't fit the flash cookie. +const TRIMMABLE_FIELDS = ["message", "name", "email"] as const; + export const forms = { async index(req: BunRequest): Promise { const ctx = await getSessionContext(req); @@ -51,29 +58,51 @@ export const forms = { return redirect("/forms"); } - const csrfResponse = await csrfProtection(req, { - method: "POST", - path: "/forms", - }); - if (csrfResponse) { - return csrfResponse; + const csrf = await checkCsrf(req, { method: "POST", path: "/forms" }); + if (!csrf.ok) { + // Forged, missing or cross-origin: fail hard, exactly as before. + if (!isRecoverableCsrfFailure(csrf)) { + return csrf.response; + } + + // Stale but authentic. Don't run the action - hand the work back with a + // fresh token so the user can resubmit instead of losing what they typed. + const stale = await readFormValues(req, FORM_FIELDS); + setFlash( + req, + fitFlashState( + { + state: "csrf-expired", + name: stale.name, + email: stale.email, + message: stale.message, + }, + TRIMMABLE_FIELDS, + ), + ); + return redirect("/forms"); } - const formData = await req.formData(); - const name = formData.get("name") as string; - const email = formData.get("email") as string; - const message = formData.get("message") as string; + const { name, email, message } = await readFormValues(req, FORM_FIELDS); - if (!name || name.trim().length < 3) { + if (!name || name.length < 3) { + setFlash( + req, + fitFlashState( + { state: "validation-error", name, email, message }, + TRIMMABLE_FIELDS, + ), + ); return redirect("/forms"); } - setFlash(req, { - state: "submission-success", - name: name.trim(), - email: email?.trim() || undefined, - message: message?.trim() || undefined, - }); + setFlash( + req, + fitFlashState( + { state: "submission-success", name, email, message }, + TRIMMABLE_FIELDS, + ), + ); return redirect("/forms"); }, }; diff --git a/src/server/controllers/app/projects.test.ts b/src/server/controllers/app/projects.test.ts index ce1ad3a..b4bc4f2 100644 --- a/src/server/controllers/app/projects.test.ts +++ b/src/server/controllers/app/projects.test.ts @@ -1,7 +1,16 @@ -import { afterAll, beforeEach, describe, expect, mock, test } from "bun:test"; +import { + afterAll, + afterEach, + beforeEach, + describe, + expect, + mock, + setSystemTime, + test, +} from "bun:test"; import { SQL } from "bun"; import { findOrCreateUser } from "../../services/auth"; -import { createCsrfToken } from "../../services/csrf"; +import { createCsrfToken, TIME_WINDOW_MINUTES } from "../../services/csrf"; import type { Project } from "../../services/project"; import { createAuthenticatedSession, @@ -47,6 +56,10 @@ describe("Projects Controller", () => { mockDeleteProject.mockClear(); }); + afterEach(() => { + setSystemTime(); + }); + afterAll(async () => { await connection.end(); mock.restore(); @@ -57,6 +70,18 @@ describe("Projects Controller", () => { return createAuthenticatedSession(user.id); }; + // Mint a token as if the page had rendered several windows ago: authentic, + // but too stale to act on. + const mintStaleToken = async ( + sessionId: string, + path: string, + ): Promise => { + setSystemTime(new Date(Date.now() - TIME_WINDOW_MINUTES * 3 * 60 * 1000)); + const token = await createCsrfToken(sessionId, "POST", path); + setSystemTime(); + return token; + }; + describe("GET /projects", () => { test("renders projects page with create form for guests", async () => { const guestSessionId = await createGuestSession(); @@ -619,4 +644,84 @@ describe("Projects Controller", () => { expect(response.headers.get("location")).toBe("/projects"); }); }); + + describe("CSRF expiry recovery", () => { + test("create preserves the title and does not create the project", async () => { + const sessionId = await createTestSession(); + const staleToken = await mintStaleToken(sessionId, "/projects"); + + const mockFormData = new FormData(); + mockFormData.append("title", "My New Project"); + mockFormData.append("_csrf", staleToken); + + const request = createBunRequest("http://localhost:3000/projects", { + method: "POST", + headers: { + Origin: "http://localhost:3000", + Cookie: `session_id=${sessionId}`, + }, + body: mockFormData, + }); + + const response = await projects.create(request); + + expect(mockCreateProject).not.toHaveBeenCalled(); + expect(response.status).toBe(303); + expect(response.headers.get("location")).toBe("/projects"); + + const setCookie = findSetCookie(request, "flash_state"); + expect(setCookie).toContain("csrf-expired"); + expect(setCookie).toContain("My New Project"); + }); + + test("destroy never deletes on a stale token", async () => { + const sessionId = await createTestSession(); + const staleToken = await mintStaleToken(sessionId, "/projects/1/delete"); + + const mockFormData = new FormData(); + mockFormData.append("_csrf", staleToken); + + const request = createBunRequest( + "http://localhost:3000/projects/1/delete", + { + method: "POST", + headers: { + Origin: "http://localhost:3000", + Cookie: `session_id=${sessionId}`, + }, + body: mockFormData, + }, + { id: "1" }, + ); + + const response = await projects.destroy(request); + + // The load-bearing assertion: a destructive action must never be + // replayed off a stale token. + expect(mockDeleteProject).not.toHaveBeenCalled(); + expect(response.status).toBe(303); + expect(response.headers.get("location")).toBe("/projects"); + expect(findSetCookie(request, "flash_state")).toContain( + "delete-csrf-expired", + ); + }); + + test("renders the recovery warning and pre-fills the title", async () => { + const sessionId = await createTestSession(); + mockGetProjects.mockResolvedValueOnce([]); + + const request = createBunRequest("http://localhost:3000/projects", { + headers: { Cookie: `session_id=${sessionId}` }, + }); + + const { setFlash } = stateHelpers(); + setFlash(request, { state: "csrf-expired", title: "My New Project" }); + + const response = await projects.index(request); + const html = await response.text(); + + expect(html).toContain("Your session timed out"); + expect(html).toContain('value="My New Project"'); + }); + }); }); diff --git a/src/server/controllers/app/projects.tsx b/src/server/controllers/app/projects.tsx index 2fbe754..0c600ab 100644 --- a/src/server/controllers/app/projects.tsx +++ b/src/server/controllers/app/projects.tsx @@ -1,6 +1,6 @@ import type { BunRequest } from "bun"; import { getSessionContext, requireAuth } from "../../middleware/auth"; -import { csrfProtection } from "../../middleware/csrf"; +import { checkCsrf, isRecoverableCsrfFailure } from "../../middleware/csrf"; import { createCsrfToken } from "../../services/csrf"; import { createProject, @@ -10,8 +10,9 @@ import { import { setSessionCookie } from "../../services/sessions"; import type { ProjectsState } from "../../templates/projects"; import { Projects } from "../../templates/projects"; +import { readFormValues } from "../../utils/form-data"; import { redirect, render } from "../../utils/response"; -import { stateHelpers } from "../../utils/state"; +import { fitFlashState, stateHelpers } from "../../utils/state"; const { getFlash, setFlash } = stateHelpers(); @@ -76,23 +77,40 @@ export const projects = { return redirect("/projects"); } - const csrfResponse = await csrfProtection(req, { - method: "POST", - path: "/projects", - }); - if (csrfResponse) { - return csrfResponse; + const csrf = await checkCsrf(req, { method: "POST", path: "/projects" }); + if (!csrf.ok) { + // Forged, missing or cross-origin: fail hard, exactly as before. + if (!isRecoverableCsrfFailure(csrf)) { + return csrf.response; + } + + // Stale but authentic. Don't create - hand the title back with a fresh + // token so the user can resubmit. + const stale = await readFormValues(req, ["title"]); + setFlash( + req, + fitFlashState( + { state: "csrf-expired", title: stale.title }, + ["title"], + ), + ); + return redirect("/projects"); } - const formData = await req.formData(); - const title = formData.get("title") as string; + const { title } = await readFormValues(req, ["title"]); - if (!title || title.trim().length < 2) { + if (!title || title.length < 2) { + setFlash( + req, + fitFlashState({ state: "validation-error", title }, [ + "title", + ]), + ); return redirect("/projects"); } const createdBy = ctx.user?.email ?? null; - await createProject(title.trim(), createdBy); + await createProject(title, createdBy); setFlash(req, { state: "submission-success" }); return redirect("/projects"); }, @@ -105,12 +123,20 @@ export const projects = { return authRedirect; } - const csrfResponse = await csrfProtection(req, { + const csrf = await checkCsrf(req, { method: "POST", - path: req.url, + path: new URL(req.url).pathname, }); - if (csrfResponse) { - return csrfResponse; + if (!csrf.ok) { + if (!isRecoverableCsrfFailure(csrf)) { + return csrf.response; + } + + // Stale but authentic. Nothing to preserve, and a delete must never be + // replayed silently - bounce back so the row re-renders with a fresh + // token and the user confirms with a deliberate second click. + setFlash(req, { state: "delete-csrf-expired" }); + return redirect("/projects"); } const idParam = req.params.id; diff --git a/src/server/controllers/auth/logout.test.ts b/src/server/controllers/auth/logout.test.ts index 87e0655..b924bc3 100644 --- a/src/server/controllers/auth/logout.test.ts +++ b/src/server/controllers/auth/logout.test.ts @@ -1,4 +1,13 @@ -import { afterAll, beforeEach, describe, expect, mock, test } from "bun:test"; +import { + afterAll, + afterEach, + beforeEach, + describe, + expect, + mock, + setSystemTime, + test, +} from "bun:test"; import { SQL } from "bun"; import { cleanupTestData } from "../../test-utils/helpers"; @@ -50,7 +59,7 @@ mock.module("../../middleware/auth", () => { }); import { findOrCreateUser } from "../../services/auth"; -import { createCsrfToken } from "../../services/csrf"; +import { createCsrfToken, TIME_WINDOW_MINUTES } from "../../services/csrf"; import { db } from "../../services/database"; import { createAuthenticatedSession } from "../../services/sessions"; import { createBunRequest, findSetCookie } from "../../test-utils/bun-request"; @@ -61,6 +70,10 @@ describe("Logout Controller", () => { await cleanupTestData(db); }); + afterEach(() => { + setSystemTime(); + }); + afterAll(async () => { await connection.end(); mock.restore(); @@ -306,5 +319,71 @@ describe("Logout Controller", () => { expect(setCookie).toContain("session_id="); expect(setCookie).toContain("Max-Age=0"); }); + + test("honours a stale but authentic token so sign-out never appears broken", async () => { + const user = await findOrCreateUser("csrf-stale@example.com"); + const sessionId = await createAuthenticatedSession(user.id); + + setSystemTime(new Date(Date.now() - TIME_WINDOW_MINUTES * 3 * 60 * 1000)); + const staleToken = await createCsrfToken( + sessionId, + "POST", + "/auth/logout", + ); + setSystemTime(); + + const formData = new FormData(); + formData.append("_csrf", staleToken); + + const request = createBunRequest("http://localhost:3000/auth/logout", { + method: "POST", + headers: { + Origin: "http://localhost:3000", + Cookie: `session_id=${sessionId}`, + }, + body: formData, + }); + + const response = await logout.create(request); + + expect(response.status).toBe(303); + expect(response.headers.get("location")).toBe("/login"); + expect(response.headers.get("Clear-Site-Data")).toBe( + '"cookies", "storage"', + ); + + const remaining = await db`SELECT id_hash FROM sessions`; + expect(remaining.length).toBe(0); + }); + + test("still rejects a stale token from a foreign origin", async () => { + const user = await findOrCreateUser("csrf-stale-origin@example.com"); + const sessionId = await createAuthenticatedSession(user.id); + + setSystemTime(new Date(Date.now() - TIME_WINDOW_MINUTES * 3 * 60 * 1000)); + const staleToken = await createCsrfToken( + sessionId, + "POST", + "/auth/logout", + ); + setSystemTime(); + + const formData = new FormData(); + formData.append("_csrf", staleToken); + + const request = createBunRequest("http://localhost:3000/auth/logout", { + method: "POST", + headers: { + Origin: "http://evil.example", + Cookie: `session_id=${sessionId}`, + }, + body: formData, + }); + + const response = await logout.create(request); + + expect(response.status).toBe(403); + expect(await response.text()).toBe("Invalid request origin"); + }); }); }); diff --git a/src/server/controllers/auth/logout.tsx b/src/server/controllers/auth/logout.tsx index 599b557..d17aa27 100644 --- a/src/server/controllers/auth/logout.tsx +++ b/src/server/controllers/auth/logout.tsx @@ -1,6 +1,6 @@ import type { BunRequest } from "bun"; import { getSessionContext } from "../../middleware/auth"; -import { csrfProtection } from "../../middleware/csrf"; +import { checkCsrf } from "../../middleware/csrf"; import { clearSessionCookie, deleteSession } from "../../services/sessions"; export const logout = { @@ -8,12 +8,20 @@ export const logout = { const ctx = await getSessionContext(req); if (ctx.isAuthenticated && ctx.sessionId) { - const csrfResponse = await csrfProtection(req, { + const csrf = await checkCsrf(req, { method: "POST", path: "/auth/logout", }); - if (csrfResponse) { - return csrfResponse; + + // Sign-out is the one action that honours a stale token rather than + // bouncing. The nav button's token is minted on every page render, so an + // old tab hits this constantly, and unlike the create/delete flows there + // is nothing to preserve and no form to return the user to. The request + // still passed the origin check and still proved possession of the + // session's CSRF secret; sign-out is idempotent and reversible, and a + // sign-out button that appears broken is its own security problem. + if (!csrf.ok && csrf.reason !== "expired-token") { + return csrf.response; } try { diff --git a/src/server/middleware/csrf.test.ts b/src/server/middleware/csrf.test.ts index 7388e71..4d658e4 100644 --- a/src/server/middleware/csrf.test.ts +++ b/src/server/middleware/csrf.test.ts @@ -1,12 +1,21 @@ -import { afterAll, beforeEach, describe, expect, mock, test } from "bun:test"; +import { + afterAll, + afterEach, + beforeEach, + describe, + expect, + mock, + setSystemTime, + test, +} from "bun:test"; import { SQL } from "bun"; import { findOrCreateUser } from "../services/auth"; -import { createCsrfToken } from "../services/csrf"; +import { createCsrfToken, TIME_WINDOW_MINUTES } from "../services/csrf"; import { db } from "../services/database"; import { createAuthenticatedSession } from "../services/sessions"; import { createBunRequest } from "../test-utils/bun-request"; import { cleanupTestData } from "../test-utils/helpers"; -import { csrfProtection } from "./csrf"; +import { checkCsrf, csrfProtection, isRecoverableCsrfFailure } from "./csrf"; if (!process.env.DATABASE_URL) { throw new Error("DATABASE_URL is required for tests"); @@ -26,6 +35,10 @@ describe("CSRF Middleware", () => { await cleanupTestData(db); }); + afterEach(() => { + setSystemTime(); + }); + afterAll(async () => { await connection.end(); mock.restore(); @@ -395,4 +408,143 @@ describe("CSRF Middleware", () => { expect(await response?.text()).toBe("Invalid CSRF token"); }); }); + + describe("checkCsrf", () => { + const postRequest = ( + sessionId: string | null, + token: string | null, + origin: string | null = ORIGIN, + ) => { + const body = new FormData(); + body.append("title", "Something"); + if (token) { + body.append("_csrf", token); + } + + const headers: Record = {}; + if (origin) { + headers.Origin = origin; + } + if (sessionId) { + headers.Cookie = `session_id=${sessionId}`; + } + + return createBunRequest("http://localhost:3000/forms", { + method: "POST", + headers, + body, + }); + }; + + const mintAged = async ( + sessionId: string, + minutesAgo: number, + ): Promise => { + setSystemTime(new Date(Date.now() - minutesAgo * 60 * 1000)); + const token = await createCsrfToken(sessionId, "POST", "/forms"); + setSystemTime(); + return token; + }; + + test("returns ok for a valid token", async () => { + const sessionId = await createTestSession(); + const token = await createCsrfToken(sessionId, "POST", "/forms"); + + const result = await checkCsrf(postRequest(sessionId, token), { + method: "POST", + path: "/forms", + }); + + expect(result.ok).toBe(true); + }); + + test("reports expired-token for a stale but authentic token", async () => { + const sessionId = await createTestSession(); + const token = await mintAged(sessionId, TIME_WINDOW_MINUTES * 3); + + const result = await checkCsrf(postRequest(sessionId, token), { + method: "POST", + path: "/forms", + }); + + expect(result.ok).toBe(false); + expect(result.ok === false && result.reason).toBe("expired-token"); + expect(isRecoverableCsrfFailure(result)).toBe(true); + }); + + test("an origin failure is never recoverable", async () => { + const sessionId = await createTestSession(); + const token = await createCsrfToken(sessionId, "POST", "/forms"); + + const result = await checkCsrf(postRequest(sessionId, token, null), { + method: "POST", + path: "/forms", + }); + + expect(result.ok).toBe(false); + expect(result.ok === false && result.reason).toBe("invalid-origin"); + expect(isRecoverableCsrfFailure(result)).toBe(false); + expect(result.ok === false && result.response.status).toBe(403); + }); + + test("a cross-origin request with a stale token is not recoverable", async () => { + const sessionId = await createTestSession(); + const token = await mintAged(sessionId, TIME_WINDOW_MINUTES * 3); + + const result = await checkCsrf( + postRequest(sessionId, token, "http://evil.example"), + { method: "POST", path: "/forms" }, + ); + + expect(result.ok === false && result.reason).toBe("invalid-origin"); + expect(isRecoverableCsrfFailure(result)).toBe(false); + }); + + test("reports missing-token and is not recoverable", async () => { + const sessionId = await createTestSession(); + + const result = await checkCsrf(postRequest(sessionId, null), { + method: "POST", + path: "/forms", + }); + + expect(result.ok === false && result.reason).toBe("missing-token"); + expect(isRecoverableCsrfFailure(result)).toBe(false); + }); + + test("reports invalid-token for a forged token", async () => { + const sessionId = await createTestSession(); + await createCsrfToken(sessionId, "POST", "/forms"); + + const result = await checkCsrf(postRequest(sessionId, "forged.token"), { + method: "POST", + path: "/forms", + }); + + expect(result.ok === false && result.reason).toBe("invalid-token"); + expect(isRecoverableCsrfFailure(result)).toBe(false); + }); + + test("reports expired-session and is not recoverable without a secret", async () => { + const sessionId = await createTestSession(); + + const result = await checkCsrf(postRequest(sessionId, "nonce.token"), { + method: "POST", + path: "/forms", + }); + + expect(result.ok === false && result.reason).toBe("expired-session"); + expect(isRecoverableCsrfFailure(result)).toBe(false); + }); + + test("reports missing-session when no cookie is present", async () => { + const result = await checkCsrf(postRequest(null, "nonce.token"), { + method: "POST", + path: "/forms", + }); + + expect(result.ok === false && result.reason).toBe("missing-session"); + expect(isRecoverableCsrfFailure(result)).toBe(false); + }); + }); }); diff --git a/src/server/middleware/csrf.ts b/src/server/middleware/csrf.ts index 4ddea56..a27d5f2 100644 --- a/src/server/middleware/csrf.ts +++ b/src/server/middleware/csrf.ts @@ -2,8 +2,8 @@ import type { BunRequest } from "bun"; import { CSRF_FIELD_NAME, CSRF_HEADER_NAME, + inspectCsrfToken, validateOrigin, - verifyCsrfToken, } from "../services/csrf"; import { log } from "../services/logger"; import { getSessionIdFromRequest } from "../services/sessions"; @@ -15,40 +15,85 @@ export interface CsrfOptions { } /** - * CSRF protection middleware - * Validates CSRF token and Origin/Referer headers for state-changing requests + * Why a CSRF check failed. + * + * Everything except the two "expired-*" reasons is a hard failure. In + * particular "invalid-origin" must never be treated as recoverable: it is the + * actual cross-origin attack path, and re-issuing a token there would turn the + * app into a token vending machine for attacker-initiated POSTs. */ -export const csrfProtection = async ( +export type CsrfFailureReason = + | "method-mismatch" + | "invalid-origin" + | "missing-session" + | "missing-token" + | "invalid-token" + | "rate-limited" + | "expired-token" + | "expired-session"; + +export type CsrfCheckResult = + | { ok: true } + | { ok: false; reason: CsrfFailureReason; response: Response }; + +/** + * A stale token still proves possession of the session's CSRF secret, so the + * caller may re-render the form with a fresh token instead of failing hard. + * The action itself must not be performed either way. + * + * Only "expired-token" qualifies. "expired-session" is deliberately excluded: + * with no secret to verify against there is no proof of authenticity, so a + * forged token against a dead session is indistinguishable from a real one. + */ +export const isRecoverableCsrfFailure = (result: CsrfCheckResult): boolean => + !result.ok && result.reason === "expired-token"; + +/** + * CSRF protection check + * Validates CSRF token and Origin/Referer headers for state-changing requests, + * reporting why the check failed so callers can offer recovery where it's safe + */ +export const checkCsrf = async ( req: BunRequest, options: CsrfOptions, -): Promise => { +): Promise => { const { method: expectedMethod, expectedOrigin } = options; const actualMethod = req.method.toUpperCase(); + const fail = ( + reason: CsrfFailureReason, + body: string, + status: number, + ): CsrfCheckResult => ({ + ok: false, + reason, + response: new Response(body, { status }), + }); + // Assert method matches if provided (catch misconfigurations) if (expectedMethod && expectedMethod.toUpperCase() !== actualMethod) { log.error( "csrf", `Method mismatch - expected ${expectedMethod}, got ${actualMethod}`, ); - return new Response("Invalid request configuration", { status: 500 }); + return fail("method-mismatch", "Invalid request configuration", 500); } // Only protect state-changing methods (use actual request method) const protectedMethods = ["POST", "PUT", "PATCH", "DELETE"]; if (!protectedMethods.includes(actualMethod)) { - return null; // Allow non-state-changing methods + return { ok: true }; // Allow non-state-changing methods } // Validate Origin/Referer first (defense in depth) if (!validateOrigin(req, expectedOrigin)) { - return new Response("Invalid request origin", { status: 403 }); + return fail("invalid-origin", "Invalid request origin", 403); } const sessionId = getSessionIdFromRequest(req); if (!sessionId) { - return new Response("Invalid CSRF token", { status: 403 }); + return fail("missing-session", "Invalid CSRF token", 403); } // Extract CSRF token from header or form data @@ -76,24 +121,43 @@ export const csrfProtection = async ( } if (!csrfToken) { - return new Response("Invalid CSRF token", { status: 403 }); + return fail("missing-token", "Invalid CSRF token", 403); } // Use normalized path from request URL for verification const requestUrl = new URL(req.url); const normalizedPath = requestUrl.pathname; - // Verify the CSRF token (use actual request method) - const isValid = await verifyCsrfToken( + // Inspect the CSRF token (use actual request method) + const status = await inspectCsrfToken( sessionId, actualMethod, normalizedPath, csrfToken, ); - if (!isValid) { - return new Response("Invalid CSRF token", { status: 403 }); + switch (status) { + case "valid": + return { ok: true }; + case "expired": + return fail("expired-token", "Invalid CSRF token", 403); + case "session-expired": + return fail("expired-session", "Invalid CSRF token", 403); + case "rate-limited": + return fail("rate-limited", "Invalid CSRF token", 403); + default: + return fail("invalid-token", "Invalid CSRF token", 403); } +}; - return null; // Token is valid, allow request to continue +/** + * CSRF protection middleware + * Returns null when the request may proceed, or the failure Response to return + */ +export const csrfProtection = async ( + req: BunRequest, + options: CsrfOptions, +): Promise => { + const result = await checkCsrf(req, options); + return result.ok ? null : result.response; }; diff --git a/src/server/services/csrf.test.ts b/src/server/services/csrf.test.ts index 1b731fc..14c4a15 100644 --- a/src/server/services/csrf.test.ts +++ b/src/server/services/csrf.test.ts @@ -1,10 +1,22 @@ -import { afterAll, beforeEach, describe, expect, mock, test } from "bun:test"; +import { + afterAll, + afterEach, + beforeEach, + describe, + expect, + mock, + setSystemTime, + test, +} from "bun:test"; import { SQL } from "bun"; import { cleanupTestData } from "../test-utils/helpers"; import { findOrCreateUser } from "./auth"; import { + CSRF_GRACE_WINDOWS, createCsrfToken, ensureCsrfSecret, + inspectCsrfToken, + TIME_WINDOW_MINUTES, validateOrigin, verifyCsrfToken, } from "./csrf"; @@ -27,6 +39,10 @@ describe("CSRF Service", () => { await cleanupTestData(db); }); + afterEach(() => { + setSystemTime(); + }); + afterAll(async () => { await connection.end(); mock.restore(); @@ -351,4 +367,123 @@ describe("CSRF Service", () => { process.env.APP_URL = originalAppUrl; }); }); + + describe("inspectCsrfToken", () => { + const WINDOW_MS = TIME_WINDOW_MINUTES * 60 * 1000; + + // Mint a token as if the page had rendered `minutesAgo` in the past. + const mintAged = async ( + sessionId: string, + minutesAgo: number, + ): Promise => { + setSystemTime(new Date(Date.now() - minutesAgo * 60 * 1000)); + const token = await createCsrfToken(sessionId, "POST", "/forms"); + setSystemTime(); + return token; + }; + + test("returns valid for a freshly minted token", async () => { + const sessionId = await createTestSession(); + const token = await createCsrfToken(sessionId, "POST", "/forms"); + + expect(await inspectCsrfToken(sessionId, "POST", "/forms", token)).toBe( + "valid", + ); + }); + + test("returns expired for a token a few windows old", async () => { + const sessionId = await createTestSession(); + // 3 windows back is beyond the current/previous pair but inside grace. + const token = await mintAged(sessionId, TIME_WINDOW_MINUTES * 3); + + expect(await inspectCsrfToken(sessionId, "POST", "/forms", token)).toBe( + "expired", + ); + }); + + test("returns invalid once a token falls outside the grace range", async () => { + const sessionId = await createTestSession(); + const token = await mintAged( + sessionId, + TIME_WINDOW_MINUTES * (CSRF_GRACE_WINDOWS + 4), + ); + + expect(await inspectCsrfToken(sessionId, "POST", "/forms", token)).toBe( + "invalid", + ); + }); + + test("returns session-expired when the session has no secret", async () => { + const sessionId = await createTestSession(); + + expect( + await inspectCsrfToken(sessionId, "POST", "/forms", "nonce.token"), + ).toBe("session-expired"); + }); + + test("returns invalid for a malformed token", async () => { + const sessionId = await createTestSession(); + await ensureCsrfSecret(sessionId); + + expect( + await inspectCsrfToken(sessionId, "POST", "/forms", "no-dot-here"), + ).toBe("invalid"); + }); + + test("returns rate-limited after repeated forged attempts", async () => { + const sessionId = await createTestSession(); + await ensureCsrfSecret(sessionId); + + for (let attempt = 0; attempt < 10; attempt++) { + await inspectCsrfToken(sessionId, "POST", "/forms", "bad.token"); + } + + expect( + await inspectCsrfToken(sessionId, "POST", "/forms", "bad.token"), + ).toBe("rate-limited"); + }); + + test("expired tokens never trip the failure brake", async () => { + const sessionId = await createTestSession(); + const token = await mintAged(sessionId, TIME_WINDOW_MINUTES * 3); + + // Well past MAX_FAILURES_PER_WINDOW: a user with several stale tabs must + // not be able to lock themselves out of the recovery path. + for (let attempt = 0; attempt < 11; attempt++) { + expect(await inspectCsrfToken(sessionId, "POST", "/forms", token)).toBe( + "expired", + ); + } + }); + + test("expired tokens are still rejected by verifyCsrfToken", async () => { + const sessionId = await createTestSession(); + const token = await mintAged(sessionId, TIME_WINDOW_MINUTES * 3); + + expect(await verifyCsrfToken(sessionId, "POST", "/forms", token)).toBe( + false, + ); + }); + + test("a token from the previous window is still valid", async () => { + const sessionId = await createTestSession(); + setSystemTime(new Date(Date.now() - WINDOW_MS)); + const token = await createCsrfToken(sessionId, "POST", "/forms"); + setSystemTime(); + + const status = await inspectCsrfToken(sessionId, "POST", "/forms", token); + // Depending on where "now" sits in its bucket this is either the previous + // bucket (valid) or one older (expired) - never a hard failure. + expect(["valid", "expired"]).toContain(status); + }); + + test("an expired token for the wrong path stays invalid", async () => { + const sessionId = await createTestSession(); + const token = await mintAged(sessionId, TIME_WINDOW_MINUTES * 3); + + expect( + await inspectCsrfToken(sessionId, "POST", "/projects", token), + ).toBe("invalid"); + }); + }); }); diff --git a/src/server/services/csrf.ts b/src/server/services/csrf.ts index c39462f..906f4f4 100644 --- a/src/server/services/csrf.ts +++ b/src/server/services/csrf.ts @@ -11,13 +11,40 @@ export const CSRF_HEADER_NAME = "X-CSRF-Token"; export const CSRF_FIELD_NAME = "_csrf"; export const CSRF_SECRET_LENGTH = 32; export const CSRF_NONCE_LENGTH = 16; + +// Tokens are bucketed by time. Verification accepts the current and previous +// bucket, so a token's effective lifetime is TIME_WINDOW_MINUTES..2x depending +// on where in the window the page happened to render. export const TIME_WINDOW_MINUTES = 15; +// Older buckets accepted for *recovery only* - never to perform the action. +// A token matching one of these proves possession of the session's secret, so +// the caller can safely re-render the form with a fresh token instead of a 403. +export const CSRF_GRACE_WINDOWS = 8; + +/** + * Outcome of inspecting a token. Only "valid" may perform the action, and only + * "expired" - which proves possession of the session secret - is safe to + * recover from by re-issuing a token. + */ +export type CsrfTokenStatus = + | "valid" + | "expired" + | "session-expired" + | "invalid" + | "rate-limited"; + // Rate limiting - simple in-memory counter for failed attempts const failureCounters = new Map(); const MAX_FAILURES_PER_WINDOW = 10; const FAILURE_WINDOW_MS = 60 * 1000; // 1 minute +// Expired-but-authentic tokens don't count toward the failure brake (see +// inspectCsrfToken), so they get their own far looser ceiling to cap replay of +// a captured old token. No human submits 60 stale forms in a minute. +const EXPIRED_COUNTER_PREFIX = "expired:"; +const MAX_EXPIRED_PER_WINDOW = 60; + /** * Ensure a CSRF secret exists for the given session * Generates and stores a new secret if none exists @@ -115,44 +142,48 @@ export const createCsrfToken = async ( }; /** - * Verify a CSRF token against the session, method, and path - * Returns true if valid, false otherwise + * Inspect a CSRF token against the session, method, and path + * + * Distinguishes a stale-but-authentic token from a forged one. A token whose + * HMAC verifies against an older bucket proves the holder has this session's + * secret - it is only stale, not untrusted - so callers can offer the user a + * fresh token rather than a dead end. */ -export const verifyCsrfToken = async ( +export const inspectCsrfToken = async ( sessionId: string, method: string, path: string, providedToken: string, -): Promise => { +): Promise => { try { const sessionIdHash = computeHMAC(sessionId); // Check rate limiting if (isRateLimited(sessionIdHash)) { - return false; + return "rate-limited"; } // Parse token format: nonce.token const parts = providedToken.split("."); if (parts.length !== 2) { recordFailure(sessionIdHash); - return false; + return "invalid"; } const [nonce, token] = parts; // Get session's CSRF secret const result = await db` - SELECT csrf_secret - FROM sessions - WHERE id_hash = ${sessionIdHash} + SELECT csrf_secret + FROM sessions + WHERE id_hash = ${sessionIdHash} AND expires_at > CURRENT_TIMESTAMP AND csrf_secret IS NOT NULL `; if (result.length === 0 || !result[0].csrf_secret) { recordFailure(sessionIdHash); - return false; + return "session-expired"; } const csrfSecret = result[0].csrf_secret as string; @@ -161,35 +192,65 @@ export const verifyCsrfToken = async ( const normalizedPath = path.startsWith("/") ? path : `/${path}`; const pathOnly = normalizedPath.split("?")[0].split("#")[0]; - // Check current and previous time buckets (allow small clock skew) const now = Math.floor(Date.now() / 1000); const currentBucket = Math.floor(now / (TIME_WINDOW_MINUTES * 60)); - const previousBucket = currentBucket - 1; - for (const timeBucket of [currentBucket, previousBucket]) { + const matchesBucket = (timeBucket: number): boolean => { const payload = `${nonce}${method.toUpperCase()}${pathOnly}${timeBucket}`; const expectedToken = createHmac("sha256", csrfSecret) .update(payload) .digest("base64url"); // Timing-safe comparison - if ( + return ( token.length === expectedToken.length && timingSafeEqual(Buffer.from(token), Buffer.from(expectedToken)) - ) { + ); + }; + + // Check current and previous time buckets (allow small clock skew) + for (const timeBucket of [currentBucket, currentBucket - 1]) { + if (matchesBucket(timeBucket)) { clearFailures(sessionIdHash); - return true; + return "valid"; + } + } + + // Older buckets within the grace range: authentic, just stale. + for (let offset = 2; offset <= 1 + CSRF_GRACE_WINDOWS; offset++) { + if (matchesBucket(currentBucket - offset)) { + // Deliberately neither recordFailure nor clearFailures. The brake + // exists to stop guessing, and there is nothing to guess once the HMAC + // verifies - counting these would let a user with several stale tabs + // lock themselves out. Clearing them would let one captured old token + // reset an attacker's counter indefinitely. + if (isExpiredFlooding(sessionIdHash)) { + return "invalid"; + } + return "expired"; } } recordFailure(sessionIdHash); - return false; + return "invalid"; } catch { recordFailure(computeHMAC(sessionId)); - return false; + return "invalid"; } }; +/** + * Verify a CSRF token against the session, method, and path + * Returns true only for a token fresh enough to perform the action + */ +export const verifyCsrfToken = async ( + sessionId: string, + method: string, + path: string, + providedToken: string, +): Promise => + (await inspectCsrfToken(sessionId, method, path, providedToken)) === "valid"; + /** * Validate request Origin/Referer header against expected origin */ @@ -256,3 +317,22 @@ const recordFailure = (key: string): void => { const clearFailures = (key: string): void => { failureCounters.delete(key); }; + +/** + * Count an expired-token hit and report whether the session is over the (much + * looser) expired ceiling. Keeps replay of a captured stale token bounded + * without letting ordinary retries trip the main failure brake. + */ +const isExpiredFlooding = (sessionIdHash: string): boolean => { + const key = `${EXPIRED_COUNTER_PREFIX}${sessionIdHash}`; + const now = Date.now(); + const counter = failureCounters.get(key); + + if (!counter || now > counter.resetAt) { + failureCounters.set(key, { count: 1, resetAt: now + FAILURE_WINDOW_MS }); + return false; + } + + counter.count++; + return counter.count > MAX_EXPIRED_PER_WINDOW; +}; diff --git a/src/server/templates/forms.tsx b/src/server/templates/forms.tsx index 1682f29..3fe2d89 100644 --- a/src/server/templates/forms.tsx +++ b/src/server/templates/forms.tsx @@ -5,12 +5,19 @@ import { Layout } from "@server/components/layouts"; import type { User } from "@server/services/users"; export interface FormsState { - state?: "submission-success"; + state?: "submission-success" | "csrf-expired" | "validation-error"; name?: string; email?: string; message?: string; } +// Only re-fill the form when the submit failed. After a success the fields +// should be empty and ready for the next entry. +const RESTORING_STATES: ReadonlySet = new Set([ + "csrf-expired", + "validation-error", +]); + interface FormsProps { user: User | null; csrfToken?: string; @@ -23,109 +30,129 @@ export const Forms = ({ csrfToken, formCsrfToken, state, -}: FormsProps) => ( - -

Form Patterns

-

- Interactive forms with validation, CSRF protection, and flash messages - baked in. -

+}: FormsProps) => { + const restored = RESTORING_STATES.has(state?.state) ? state : {}; + + return ( + +

Form Patterns

+

+ Interactive forms with validation, CSRF protection, and flash messages + baked in. +

+ + {state?.state === "submission-success" && ( + + Submitted successfully + {state.name && <> — {state.name}} + {state.email && <> ({state.email})} + {state.message && <>: “{state.message}”} + + )} - {state?.state === "submission-success" && ( - - Submitted successfully - {state.name && <> — {state.name}} - {state.email && <> ({state.email})} - {state.message && <>: “{state.message}”} - - )} + {state?.state === "csrf-expired" && ( + + Your session timed out — nothing was saved. Check your details and + submit again. + + )} -
-
-

Try it out

-

- Submit the form to see a real POST → flash cookie → redirect cycle. - Try submitting with fewer than 3 characters in the name field. -

-
- - - - - - - - -