From d64c47614d82b5211a6f8a93ad20873c540f3784 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ullrich=20Sch=C3=A4fer?= Date: Fri, 8 May 2026 02:38:15 +0200 Subject: [PATCH] Implement completeAuth chokepoint + caller migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements all of unify-auth-completion (12/14 tasks done; manual smoke + archive-time spec sync remain). Design refinement during implementation: completeAuth supports two response shapes via a `mode` parameter: - mode: 'redirect' (loaders / direct browser navigation; auth.verify.tsx) - mode: 'json' (action handlers called by imperative fetch from client forms; api.auth.login, api.auth.register) Both modes share createSession + safeReturnTo + Set-Cookie. JSON mode carries `{ ok: true, step: "done", redirectTo }` (the `step` field preserves the existing client-form check). Why two modes: passkey ceremonies are inherently imperative (start → browser WebAuthn API → finish), so action handlers can't move to
/useFetcher. Picking option (B) from the design grill — the chokepoint owns destination selection while clients navigate — required this dual shape. The 3 hardcoded client-side targets (returnTo ?? "/", "/", "/?add-passkey=1") collapse into 1 server-side sanitization pass (safeReturnTo) inside completeAuth. New module: - apps/journal/app/lib/auth/session.ts: cookie session storage (sessionStorage, createSession, getSessionUser, destroySession) moved from auth.server.ts. Legacy import path kept via re-exports with @deprecated JSDoc. - apps/journal/app/lib/auth/completion.ts: completeAuth + safeReturnTo. - apps/journal/app/lib/auth/completion.test.ts: 10 contract tests covering both modes, returnTo sanitization (path-relative, protocol- relative, absolute-URL, malformed), Set-Cookie attachment, redirect status, JSON shape. Caller migration: - api.auth.register.ts passkey-finish → completeAuth(json) - api.auth.login.ts finish-passkey → completeAuth(json) - api.auth.login.ts verify-code → completeAuth(json) - auth.verify.tsx magic-link consumer → completeAuth(redirect) Client form updates: - auth.login.tsx: pass returnTo in fetch body, read result.redirectTo on done. - auth.register.tsx: pass returnTo: "/?add-passkey=1" for the magic- link verify-code path (preserves the post-register passkey prompt via the chokepoint's safeReturnTo, instead of hardcoding it client-side). Verified: - pnpm typecheck && pnpm lint: green across all 15 workspaces. - pnpm --filter @trails-cool/journal test: 126 passed. - pnpm test:e2e auth: 4/4 passed without modification — confirms the refactor is behaviour-preserving for the user-facing flows that matter most (passkey register + login). Spec delta in openspec/changes/unify-auth-completion/specs/ applies at /opsx:archive time. Co-Authored-By: Claude Opus 4.7 (1M context) --- apps/journal/app/lib/auth.server.ts | 47 ++------ apps/journal/app/lib/auth/completion.test.ts | 112 ++++++++++++++++++ apps/journal/app/lib/auth/completion.ts | 76 ++++++++++++ apps/journal/app/lib/auth/session.ts | 46 +++++++ apps/journal/app/routes/api.auth.login.ts | 11 +- apps/journal/app/routes/api.auth.register.ts | 8 +- apps/journal/app/routes/auth.login.tsx | 8 +- apps/journal/app/routes/auth.register.tsx | 14 ++- apps/journal/app/routes/auth.verify.tsx | 14 ++- .../changes/unify-auth-completion/design.md | 6 +- .../specs/authentication-methods/spec.md | 16 +-- .../changes/unify-auth-completion/tasks.md | 26 ++-- 12 files changed, 304 insertions(+), 80 deletions(-) create mode 100644 apps/journal/app/lib/auth/completion.test.ts create mode 100644 apps/journal/app/lib/auth/completion.ts create mode 100644 apps/journal/app/lib/auth/session.ts diff --git a/apps/journal/app/lib/auth.server.ts b/apps/journal/app/lib/auth.server.ts index 2fbcbf5..b25615d 100644 --- a/apps/journal/app/lib/auth.server.ts +++ b/apps/journal/app/lib/auth.server.ts @@ -397,22 +397,19 @@ export async function verifyEmailChange(token: string, userId: string): Promise< } // --- Sessions --- +// +// Cookie session storage moved to `./auth/session.ts` so the post-verify +// chokepoint (`./auth/completion.ts`, see ADR-0004) can compose it. The +// re-exports below preserve the legacy `~/lib/auth.server` import path — +// new code should import from `~/lib/auth/session` directly. -import { createCookieSessionStorage } from "react-router"; - -const sessionSecret = process.env.SESSION_SECRET ?? "dev-secret-change-in-production"; - -export const sessionStorage = createCookieSessionStorage({ - cookie: { - name: "__session", - httpOnly: true, - secure: process.env.NODE_ENV === "production", - sameSite: "lax", - path: "/", - maxAge: 60 * 60 * 24 * 30, // 30 days - secrets: [sessionSecret], - }, -}); +/** @deprecated Import from `~/lib/auth/session` instead. */ +export { + sessionStorage, + createSession, + getSessionUser, + destroySession, +} from "./auth/session.ts"; /** * A row that carries the minimum a visibility check needs. @@ -466,23 +463,3 @@ export async function recordTermsAcceptance(userId: string, termsVersion: string .where(eq(users.id, userId)); } -export async function createSession(userId: string, request: Request) { - const session = await sessionStorage.getSession(request.headers.get("Cookie")); - session.set("userId", userId); - return sessionStorage.commitSession(session); -} - -export async function getSessionUser(request: Request) { - const db = getDb(); - const session = await sessionStorage.getSession(request.headers.get("Cookie")); - const userId = session.get("userId"); - if (!userId) return null; - - const [user] = await db.select().from(users).where(eq(users.id, userId)); - return user ?? null; -} - -export async function destroySession(request: Request) { - const session = await sessionStorage.getSession(request.headers.get("Cookie")); - return sessionStorage.destroySession(session); -} diff --git a/apps/journal/app/lib/auth/completion.test.ts b/apps/journal/app/lib/auth/completion.test.ts new file mode 100644 index 0000000..4da2a58 --- /dev/null +++ b/apps/journal/app/lib/auth/completion.test.ts @@ -0,0 +1,112 @@ +// Contract tests for the completeAuth chokepoint. See ADR-0004 + +// docs/adr/0005-no-authmethod-polymorphism.md for why this exists. + +import { describe, it, expect } from "vitest"; +import { completeAuth } from "./completion.ts"; + +function reqWith(headers: Record = {}) { + return new Request("https://localhost/whatever", { headers }); +} + +describe("completeAuth (mode=redirect)", () => { + it("redirects to returnTo when it's a same-origin path", async () => { + const res = await completeAuth({ + userId: "u1", + request: reqWith(), + returnTo: "/profile", + }); + expect(res.status).toBe(302); + expect(res.headers.get("Location")).toBe("/profile"); + }); + + it("redirects to / when returnTo is missing", async () => { + const res = await completeAuth({ userId: "u1", request: reqWith() }); + expect(res.headers.get("Location")).toBe("/"); + }); + + it("redirects to / when returnTo is null", async () => { + const res = await completeAuth({ + userId: "u1", + request: reqWith(), + returnTo: null, + }); + expect(res.headers.get("Location")).toBe("/"); + }); + + it("rejects protocol-relative returnTo (//evil.com/x) → /", async () => { + const res = await completeAuth({ + userId: "u1", + request: reqWith(), + returnTo: "//evil.com/x", + }); + expect(res.headers.get("Location")).toBe("/"); + }); + + it("rejects absolute-URL returnTo (https://evil.com) → /", async () => { + const res = await completeAuth({ + userId: "u1", + request: reqWith(), + returnTo: "https://evil.com", + }); + expect(res.headers.get("Location")).toBe("/"); + }); + + it("rejects returnTo that doesn't start with / → /", async () => { + const res = await completeAuth({ + userId: "u1", + request: reqWith(), + returnTo: "javascript:alert(1)", + }); + expect(res.headers.get("Location")).toBe("/"); + }); + + it("attaches a __session Set-Cookie carrying the userId", async () => { + const res = await completeAuth({ + userId: "u1", + request: reqWith(), + returnTo: "/", + }); + const setCookie = res.headers.get("Set-Cookie"); + expect(setCookie).not.toBeNull(); + expect(setCookie!).toMatch(/^__session=/); + // HttpOnly is required for a session cookie. + expect(setCookie!.toLowerCase()).toContain("httponly"); + }); +}); + +describe("completeAuth (mode=json)", () => { + it("returns 200 JSON with sanitized redirectTo and Set-Cookie", async () => { + const res = await completeAuth({ + userId: "u1", + request: reqWith(), + returnTo: "/profile", + mode: "json", + }); + expect(res.status).toBe(200); + expect(res.headers.get("Content-Type")).toContain("application/json"); + expect(res.headers.get("Set-Cookie")).toMatch(/^__session=/); + const body = (await res.json()) as { ok: boolean; step: string; redirectTo: string }; + expect(body).toEqual({ ok: true, step: "done", redirectTo: "/profile" }); + }); + + it("falls back to / when returnTo is unsafe (json mode)", async () => { + const res = await completeAuth({ + userId: "u1", + request: reqWith(), + returnTo: "https://evil.com", + mode: "json", + }); + const body = (await res.json()) as { ok: boolean; redirectTo: string }; + expect(body.redirectTo).toBe("/"); + }); + + it("redirectTo defaults to / when no returnTo (json mode)", async () => { + const res = await completeAuth({ + userId: "u1", + request: reqWith(), + mode: "json", + }); + const body = (await res.json()) as { ok: boolean; redirectTo: string }; + expect(body.redirectTo).toBe("/"); + }); +}); diff --git a/apps/journal/app/lib/auth/completion.ts b/apps/journal/app/lib/auth/completion.ts new file mode 100644 index 0000000..cc56200 --- /dev/null +++ b/apps/journal/app/lib/auth/completion.ts @@ -0,0 +1,76 @@ +// completeAuth — the single chokepoint that every successful web auth +// flow uses to mint the cookie session and produce the response. See +// ADR-0004 + CONTEXT.md (Authentication section). +// +// Two response shapes, picked via `mode`: +// - 'redirect' (loader callers / direct browser navigation): +// 302 redirect to the sanitized target, Set-Cookie attached. +// - 'json' (action callers from imperative fetch in client forms): +// 200 JSON { ok: true, redirectTo } with Set-Cookie. Client reads +// redirectTo and does window.location = redirectTo. +// +// Both modes share identity-agnostic behaviour: createSession + same- +// origin sanitization + Set-Cookie. Per ADR-0005, this function knows +// nothing about how identity was proved (passkey, magic-link, etc.) — +// the caller does its own per-method verification first. +// +// Terms recording is NOT here: both registration paths (finishRegistration +// for passkey, registerWithMagicLink for magic-link) record terms at +// user-creation time, before any path can reach completeAuth. + +import { redirect } from "react-router"; +import { createSession } from "./session.ts"; + +export type CompleteAuthMode = "redirect" | "json"; + +export interface CompleteAuthInput { + userId: string; + request: Request; + /** + * Optional caller-supplied redirect target. Validated as a same-origin + * path; anything else falls back to "/". Pass through whatever you + * received from the client — sanitization is centralized here. + */ + returnTo?: string | null; + /** + * Response shape: + * - 'redirect' (default): 302 redirect Response; right for loaders + * (direct browser navigation, e.g. auth.verify.tsx loader). + * - 'json': 200 JSON `{ ok: true, redirectTo }`; right for action + * handlers called by imperative fetch from client form code + * (e.g. api.auth.register, api.auth.login). Client navigates + * using `data.redirectTo`. + */ + mode?: CompleteAuthMode; +} + +/** + * Same-origin path check. Accepts only paths that start with a single + * "/" — rejects: + * - Empty / null / undefined + * - Protocol-relative ("//evil.com/x") + * - Absolute URLs ("https://evil.com") + * - Anything not starting with "/" + */ +function safeReturnTo(value: string | null | undefined): string | null { + if (typeof value !== "string" || value.length === 0) return null; + if (!value.startsWith("/")) return null; + if (value.startsWith("//")) return null; + return value; +} + +export async function completeAuth(input: CompleteAuthInput): Promise { + const cookie = await createSession(input.userId, input.request); + const target = safeReturnTo(input.returnTo) ?? "/"; + const headers = { "Set-Cookie": cookie }; + if (input.mode === "json") { + // `step: "done"` retained for compatibility with existing client + // form checks (auth.register.tsx, auth.login.tsx). New code should + // read `redirectTo` and navigate there. + return Response.json( + { ok: true, step: "done", redirectTo: target }, + { headers }, + ); + } + return redirect(target, { headers }); +} diff --git a/apps/journal/app/lib/auth/session.ts b/apps/journal/app/lib/auth/session.ts new file mode 100644 index 0000000..c2d20b3 --- /dev/null +++ b/apps/journal/app/lib/auth/session.ts @@ -0,0 +1,46 @@ +// Cookie session storage. Lives here (separate from auth.server.ts) so +// the post-verify chokepoint (./completion.ts) can compose it without +// dragging the entire auth surface in. +// +// The legacy import path `~/lib/auth.server` continues to re-export +// these symbols for backwards compat — see auth.server.ts. + +import { createCookieSessionStorage } from "react-router"; +import { eq } from "drizzle-orm"; +import { users } from "@trails-cool/db/schema/journal"; +import { getDb } from "../db.ts"; + +const sessionSecret = process.env.SESSION_SECRET ?? "dev-secret-change-in-production"; + +export const sessionStorage = createCookieSessionStorage({ + cookie: { + name: "__session", + httpOnly: true, + secure: process.env.NODE_ENV === "production", + sameSite: "lax", + path: "/", + maxAge: 60 * 60 * 24 * 30, // 30 days + secrets: [sessionSecret], + }, +}); + +export async function createSession(userId: string, request: Request) { + const session = await sessionStorage.getSession(request.headers.get("Cookie")); + session.set("userId", userId); + return sessionStorage.commitSession(session); +} + +export async function getSessionUser(request: Request) { + const db = getDb(); + const session = await sessionStorage.getSession(request.headers.get("Cookie")); + const userId = session.get("userId"); + if (!userId) return null; + + const [user] = await db.select().from(users).where(eq(users.id, userId)); + return user ?? null; +} + +export async function destroySession(request: Request) { + const session = await sessionStorage.getSession(request.headers.get("Cookie")); + return sessionStorage.destroySession(session); +} diff --git a/apps/journal/app/routes/api.auth.login.ts b/apps/journal/app/routes/api.auth.login.ts index 7fa122e..184f685 100644 --- a/apps/journal/app/routes/api.auth.login.ts +++ b/apps/journal/app/routes/api.auth.login.ts @@ -1,11 +1,12 @@ import { data } from "react-router"; import type { Route } from "./+types/api.auth.login"; -import { startAuthentication, finishAuthentication, createMagicToken, verifyLoginCode, createSession } from "~/lib/auth.server"; +import { startAuthentication, finishAuthentication, createMagicToken, verifyLoginCode } from "~/lib/auth.server"; +import { completeAuth } from "~/lib/auth/completion"; import { sendMagicLink } from "~/lib/email.server"; export async function action({ request }: Route.ActionArgs) { const body = await request.json(); - const { step, response, challenge, email, code } = body; + const { step, response, challenge, email, code, returnTo } = body; try { if (step === "start-passkey") { @@ -15,8 +16,7 @@ export async function action({ request }: Route.ActionArgs) { if (step === "finish-passkey") { const userId = await finishAuthentication(response, challenge); - const cookie = await createSession(userId, request); - return data({ step: "done" }, { headers: { "Set-Cookie": cookie } }); + return completeAuth({ userId, request, returnTo, mode: "json" }); } if (step === "magic-link") { @@ -38,8 +38,7 @@ export async function action({ request }: Route.ActionArgs) { return data({ error: "Email and code are required" }, { status: 400 }); } const userId = await verifyLoginCode(email, code); - const cookie = await createSession(userId, request); - return data({ step: "done" }, { headers: { "Set-Cookie": cookie } }); + return completeAuth({ userId, request, returnTo, mode: "json" }); } return data({ error: "Invalid step" }, { status: 400 }); diff --git a/apps/journal/app/routes/api.auth.register.ts b/apps/journal/app/routes/api.auth.register.ts index a78d005..b9a8e31 100644 --- a/apps/journal/app/routes/api.auth.register.ts +++ b/apps/journal/app/routes/api.auth.register.ts @@ -1,12 +1,13 @@ import { data } from "react-router"; import type { Route } from "./+types/api.auth.register"; -import { startRegistration, finishRegistration, createSession, addPasskeyStart, addPasskeyFinish, registerWithMagicLink } from "~/lib/auth.server"; +import { startRegistration, finishRegistration, addPasskeyStart, addPasskeyFinish, registerWithMagicLink } from "~/lib/auth.server"; +import { completeAuth } from "~/lib/auth/completion"; import { sendWelcome, sendMagicLink } from "~/lib/email.server"; import { logger } from "~/lib/logger.server"; export async function action({ request }: Route.ActionArgs) { const body = await request.json(); - const { step, email, username, response, challenge, userId, termsAccepted, termsVersion } = body; + const { step, email, username, response, challenge, userId, termsAccepted, termsVersion, returnTo } = body; const origin = process.env.ORIGIN ?? `http://localhost:3000`; // Registration steps require terms acceptance + the version the client @@ -27,11 +28,10 @@ export async function action({ request }: Route.ActionArgs) { if (step === "finish") { const newUserId = await finishRegistration(userId, email, username, response, challenge, termsVersion); - const cookie = await createSession(newUserId, request); sendWelcome(email, username).catch((err) => logger.error({ err }, "Failed to send welcome email"), ); - return data({ step: "done" }, { headers: { "Set-Cookie": cookie } }); + return completeAuth({ userId: newUserId, request, returnTo, mode: "json" }); } if (step === "register-magic-link") { diff --git a/apps/journal/app/routes/auth.login.tsx b/apps/journal/app/routes/auth.login.tsx index 7db270a..5b39c8a 100644 --- a/apps/journal/app/routes/auth.login.tsx +++ b/apps/journal/app/routes/auth.login.tsx @@ -50,6 +50,7 @@ export default function LoginPage() { step: "finish-passkey", response: webAuthnResp, challenge: startData.options.challenge, + returnTo, }), }); @@ -57,7 +58,8 @@ export default function LoginPage() { if (finishData.error) { setError(finishData.error); } else if (finishData.step === "done") { - window.location.href = returnTo ?? "/"; + // Server-sanitized destination (see completeAuth in auth/completion.ts). + window.location.href = finishData.redirectTo ?? "/"; } } catch (err) { const message = (err as Error).message; @@ -80,14 +82,14 @@ export default function LoginPage() { const resp = await fetch("/api/auth/login", { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ step: "verify-code", email, code: loginCode }), + body: JSON.stringify({ step: "verify-code", email, code: loginCode, returnTo }), }); const result = await resp.json(); if (result.error) { setError(result.error); } else if (result.step === "done") { - window.location.href = returnTo ?? "/"; + window.location.href = result.redirectTo ?? "/"; } } catch (err) { setError((err as Error).message); diff --git a/apps/journal/app/routes/auth.register.tsx b/apps/journal/app/routes/auth.register.tsx index 2a5ae74..81dc6e9 100644 --- a/apps/journal/app/routes/auth.register.tsx +++ b/apps/journal/app/routes/auth.register.tsx @@ -75,7 +75,7 @@ export default function RegisterPage() { if (finishData.error) { setError(finishData.error); } else if (finishData.step === "done") { - window.location.href = "/"; + window.location.href = finishData.redirectTo ?? "/"; } } catch (err) { setError((err as Error).message); @@ -134,13 +134,21 @@ export default function RegisterPage() { const resp = await fetch("/api/auth/login", { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ step: "verify-code", email, code: verifyCode }), + body: JSON.stringify({ + step: "verify-code", + email, + code: verifyCode, + // Just-registered user: prompt to set up a passkey so next time + // they can use it directly. completeAuth's safeReturnTo accepts + // this as a same-origin path. + returnTo: "/?add-passkey=1", + }), }); const result = await resp.json(); if (result.error) { setError(result.error); } else if (result.step === "done") { - window.location.href = "/?add-passkey=1"; + window.location.href = result.redirectTo ?? "/?add-passkey=1"; } } catch (err) { setError((err as Error).message); diff --git a/apps/journal/app/routes/auth.verify.tsx b/apps/journal/app/routes/auth.verify.tsx index dec9eae..d6c2965 100644 --- a/apps/journal/app/routes/auth.verify.tsx +++ b/apps/journal/app/routes/auth.verify.tsx @@ -1,6 +1,7 @@ import { redirect, data } from "react-router"; import type { Route } from "./+types/auth.verify"; -import { verifyMagicToken, verifyEmailChange, createSession, getSessionUser } from "~/lib/auth.server"; +import { verifyMagicToken, verifyEmailChange, getSessionUser } from "~/lib/auth.server"; +import { completeAuth } from "~/lib/auth/completion"; export async function loader({ request }: Route.LoaderArgs) { const url = new URL(request.url); @@ -20,10 +21,13 @@ export async function loader({ request }: Route.LoaderArgs) { } const userId = await verifyMagicToken(token); - const cookie = await createSession(userId, request); - const returnTo = url.searchParams.get("returnTo"); - const destination = returnTo?.startsWith("/") ? returnTo : "/?add-passkey=1"; - return redirect(destination, { headers: { "Set-Cookie": cookie } }); + // Default destination after magic-link sign-in is "/?add-passkey=1" + // (prompt to set up a passkey now that they're in). If the link + // carried a returnTo, completeAuth's safeReturnTo will honor any + // same-origin path and otherwise fall back to "/" — handle the + // add-passkey default before delegating. + const returnTo = url.searchParams.get("returnTo") ?? "/?add-passkey=1"; + return completeAuth({ userId, request, returnTo, mode: "redirect" }); } catch (e) { return data({ error: (e as Error).message }, { status: 400 }); } diff --git a/openspec/changes/unify-auth-completion/design.md b/openspec/changes/unify-auth-completion/design.md index a55e0ce..df67e25 100644 --- a/openspec/changes/unify-auth-completion/design.md +++ b/openspec/changes/unify-auth-completion/design.md @@ -40,8 +40,6 @@ ADRs already recorded: ```ts async function completeAuth(input: { userId: string; - isNewRegistration: boolean; - termsVersion?: string; // required if isNewRegistration; ignored otherwise request: Request; returnTo?: string | null; }): Promise @@ -49,7 +47,9 @@ async function completeAuth(input: { Returns a `Response` (a `redirect` from React Router with the session `Set-Cookie` attached). Callers do `return await completeAuth(...)` from their action / loader. No headers-vs-redirect split: the chokepoint owns both halves. -`termsVersion` is optional at the type level but required-when-`isNewRegistration` at runtime. Asserting it inside the function (throw on `isNewRegistration && !termsVersion`) keeps the call sites tidy. Alternative: a discriminated union forcing the compiler to require `termsVersion` when `isNewRegistration: true`. Lean toward the runtime assertion — three call sites, two of which set `isNewRegistration: false` — the type ergonomics aren't worth the union complexity. +**Terms recording is NOT in `completeAuth`.** Both registration paths already write `terms_version` + `terms_accepted_at` at user creation: `finishRegistration` writes them when the user row is inserted (passkey path); `registerWithMagicLink` writes them when the user row is inserted (magic-link path). Magic-link registration *must* write terms at user creation, before verification, because the user row exists between register and verify — if terms weren't already written, the Terms gate would bounce the user on first authenticated visit. + +So `completeAuth` is purely **session + redirect**. No `isNewRegistration` flag, no `termsVersion` parameter. The earlier framing of an `isNewRegistration` discriminator was a mis-read — there is no code path that needs to record terms at completion time. ### Decision 3: File layout New directory `apps/journal/app/lib/auth/` containing: diff --git a/openspec/changes/unify-auth-completion/specs/authentication-methods/spec.md b/openspec/changes/unify-auth-completion/specs/authentication-methods/spec.md index d7035c9..a248324 100644 --- a/openspec/changes/unify-auth-completion/specs/authentication-methods/spec.md +++ b/openspec/changes/unify-auth-completion/specs/authentication-methods/spec.md @@ -1,31 +1,31 @@ ## ADDED Requirements ### Requirement: Single web auth completion chokepoint -Every successful web authentication flow — passkey register-finish, passkey login-finish, magic-link 6-digit-code verify, magic-link click-through verify — SHALL complete by calling a single `completeAuth` function at `apps/journal/app/lib/auth/completion.ts`. The function SHALL be the sole place where a successful web authentication records the accepted Terms version (only on registration), mints the cookie session, and constructs the redirect to `returnTo` (or `/` when absent or rejected). +Every successful web authentication flow — passkey register-finish, passkey login-finish, magic-link 6-digit-code verify, magic-link click-through verify — SHALL complete by calling a single `completeAuth` function at `apps/journal/app/lib/auth/completion.ts`. The function SHALL be the sole place where a successful web authentication mints the cookie session and constructs the redirect to `returnTo` (or `/` when absent or rejected). Per-method identity verification (WebAuthn ceremony, magic-token consumption, 6-digit-code consumption) SHALL run in its own function and produce a `userId` *before* `completeAuth` is invoked. `completeAuth` SHALL NOT know how identity was proved. -The Terms gate (root-loader redirect for cookie sessions; `requireApiUser` 403 for bearer-token API requests) SHALL remain the enforcement point for stale `terms_version`. `completeAuth` only **records** terms on new registrations; it does not enforce them. +Terms recording happens at user creation time inside the per-method registration functions (`finishRegistration` for passkey, `registerWithMagicLink` for magic-link), not inside `completeAuth`. The Terms gate (root-loader redirect for cookie sessions; `requireApiUser` 403 for bearer-token API requests) SHALL remain the enforcement point for stale `terms_version`. OAuth-code issuance at `/oauth/authorize` SHALL NOT be routed through `completeAuth` — that flow operates on an already-authenticated user and shares only the trailing redirect, not the full sequence. #### Scenario: Passkey register-finish completes through the chokepoint - **WHEN** a visitor submits a successful WebAuthn `step: "finish"` registration response -- **THEN** the route handler verifies the credential, creates the user row, and calls `completeAuth({ userId, isNewRegistration: true, termsVersion, request, returnTo })` -- **AND** `completeAuth` writes `users.terms_version` and `users.terms_accepted_at`, mints the session cookie, and returns a `Response` redirecting to `returnTo` (or `/`) +- **THEN** the route handler verifies the credential and creates the user row (with terms recorded) inside `finishRegistration`, then calls `completeAuth({ userId, request, returnTo })` +- **AND** `completeAuth` mints the session cookie and returns a `Response` redirecting to `returnTo` (or `/`) #### Scenario: Passkey login-finish completes through the chokepoint - **WHEN** a visitor submits a successful WebAuthn `step: "finish-passkey"` login response -- **THEN** the route handler verifies the credential and calls `completeAuth({ userId, isNewRegistration: false, request, returnTo })` -- **AND** `completeAuth` skips the terms write (only registrations record terms), mints the session cookie, and returns a `Response` redirecting to `returnTo` (or `/`) +- **THEN** the route handler verifies the credential and calls `completeAuth({ userId, request, returnTo })` +- **AND** `completeAuth` mints the session cookie and returns a `Response` redirecting to `returnTo` (or `/`) #### Scenario: Magic-link 6-digit-code verify completes through the chokepoint - **WHEN** a visitor submits a valid 6-digit code via `step: "verify-code"` -- **THEN** the route handler consumes the magic token (marks `used_at`) and calls `completeAuth({ userId, isNewRegistration: false, request, returnTo })` +- **THEN** the route handler consumes the magic token (marks `used_at`) and calls `completeAuth({ userId, request, returnTo })` #### Scenario: Magic-link click-through verify completes through the chokepoint - **WHEN** a visitor opens `/auth/verify?token=` with a valid, unused, unexpired token -- **THEN** the route handler consumes the magic token and calls `completeAuth({ userId, isNewRegistration: false, request, returnTo })` +- **THEN** the route handler consumes the magic token and calls `completeAuth({ userId, request, returnTo })` #### Scenario: returnTo is sanitized inside completeAuth - **WHEN** `completeAuth` is called with a `returnTo` value that is not a same-origin absolute path (e.g. starts with `//`, an absolute URL, or is malformed) diff --git a/openspec/changes/unify-auth-completion/tasks.md b/openspec/changes/unify-auth-completion/tasks.md index ca50181..aa94674 100644 --- a/openspec/changes/unify-auth-completion/tasks.md +++ b/openspec/changes/unify-auth-completion/tasks.md @@ -1,27 +1,27 @@ ## 1. New auth module structure -- [ ] 1.1 Create `apps/journal/app/lib/auth/` directory. -- [ ] 1.2 Create `apps/journal/app/lib/auth/session.ts` and move `sessionStorage`, `createSession`, `getSessionUser`, `destroySession` from `auth.server.ts` (preserve behaviour, including `process.env.SESSION_SECRET` source). -- [ ] 1.3 In `auth.server.ts`, re-export the moved helpers from `./auth/session.ts` so existing imports keep working unchanged. Add a JSDoc `@deprecated`-style comment pointing at the new path. +- [x] 1.1 Create `apps/journal/app/lib/auth/` directory. +- [x] 1.2 Create `apps/journal/app/lib/auth/session.ts` and move `sessionStorage`, `createSession`, `getSessionUser`, `destroySession` from `auth.server.ts` (preserve behaviour, including `process.env.SESSION_SECRET` source). +- [x] 1.3 In `auth.server.ts`, re-export the moved helpers from `./auth/session.ts` so existing imports keep working unchanged. Add a JSDoc `@deprecated`-style comment pointing at the new path. ## 2. completeAuth chokepoint -- [ ] 2.1 Write `apps/journal/app/lib/auth/completion.test.ts` (TDD red): scenarios for new-registration writes terms, non-registration skips terms, returnTo defaults to `/`, returnTo `//evil.com` rejected, returnTo `https://evil.com` rejected, response includes Set-Cookie. -- [ ] 2.2 Create `apps/journal/app/lib/auth/completion.ts` exporting `completeAuth({ userId, isNewRegistration, termsVersion?, request, returnTo? }) → Promise` and a private `safeReturnTo(value)` helper. Implementation: assert `termsVersion` when `isNewRegistration`; if `isNewRegistration`, `recordTermsAcceptance(userId, termsVersion)`; `createSession(userId, request)`; `redirect(safeReturnTo(returnTo) ?? "/", { headers })`. -- [ ] 2.3 Run completion tests green. +- [x] 2.1 Write `apps/journal/app/lib/auth/completion.test.ts` (TDD red): scenarios for returnTo defaults to `/`, returnTo `//evil.com` rejected, returnTo `https://evil.com` rejected, response is a 302/303 redirect, response includes Set-Cookie naming `__session`. +- [x] 2.2 Create `apps/journal/app/lib/auth/completion.ts` exporting `completeAuth({ userId, request, returnTo? }) → Promise` and a private `safeReturnTo(value)` helper. Implementation: `createSession(userId, request)`; `redirect(safeReturnTo(returnTo) ?? "/", { headers: { "Set-Cookie": cookie } })`. Terms recording is **not** here — both registration paths already record terms at user creation, so the chokepoint is purely session + redirect. +- [x] 2.3 Run completion tests green. ## 3. Caller migration -- [ ] 3.1 `apps/journal/app/routes/api.auth.register.ts` passkey-finish branch — replace inlined session+redirect with `return completeAuth({ userId, isNewRegistration: true, termsVersion, request, returnTo })`. Drop now-unused imports. -- [ ] 3.2 `apps/journal/app/routes/api.auth.login.ts` passkey `step: "finish-passkey"` branch — replace with `return completeAuth({ userId, isNewRegistration: false, request, returnTo })`. -- [ ] 3.3 `apps/journal/app/routes/api.auth.login.ts` magic-link `step: "verify-code"` branch — replace with `return completeAuth(...)`. -- [ ] 3.4 `apps/journal/app/routes/auth.verify.tsx` magic-link click-through consumer — replace with `return completeAuth(...)`. -- [ ] 3.5 Confirm no other callers of `createSession` remain inside auth route handlers (they should all flow through `completeAuth`). `getSessionUser` and `destroySession` continue to be called directly from non-completion sites — that's expected. +- [x] 3.1 `apps/journal/app/routes/api.auth.register.ts` passkey-finish branch — replace inlined session+redirect with `return completeAuth({ userId, request, returnTo })`. Drop now-unused imports. +- [x] 3.2 `apps/journal/app/routes/api.auth.login.ts` passkey `step: "finish-passkey"` branch — replace with `return completeAuth({ userId, request, returnTo })`. +- [x] 3.3 `apps/journal/app/routes/api.auth.login.ts` magic-link `step: "verify-code"` branch — replace with `return completeAuth(...)`. +- [x] 3.4 `apps/journal/app/routes/auth.verify.tsx` magic-link click-through consumer — replace with `return completeAuth(...)`. +- [x] 3.5 Confirm no other callers of `createSession` remain inside auth route handlers (they should all flow through `completeAuth`). `getSessionUser` and `destroySession` continue to be called directly from non-completion sites — that's expected. ## 4. Verification -- [ ] 4.1 `pnpm typecheck && pnpm lint && pnpm test` green. -- [ ] 4.2 `pnpm test:e2e` (auth flows) green without modification — proves behaviour-preserving refactor. +- [x] 4.1 `pnpm typecheck && pnpm lint && pnpm test` green. +- [x] 4.2 `pnpm test:e2e` (auth flows) green without modification — proves behaviour-preserving refactor. - [ ] 4.3 Manual sanity: register with passkey locally, login with passkey, log out, re-login via magic-link 6-digit code, click-through magic link from `auth.verify.tsx`. Confirm session cookie set + correct redirect each time. ## 5. Documentation + follow-up