trails/openspec/changes/unify-auth-completion/design.md

Context

apps/journal/app/lib/auth.server.ts is 489 lines covering passkey ceremonies, magic-link token lifecycle, session cookie storage, terms recording, and email-change tokens. Four route handlers (api.auth.register.ts, api.auth.login.ts × 2 branches, auth.verify.tsx) each repeat the same trailing sequence after their per-method identity verification:

if (isNewRegistration) await recordTermsAcceptance(userId, termsVersion);
const headers = await createSession(userId, request);
return redirect(safeReturnTo(returnTo) ?? "/", { headers });

The repetition is the entire asymmetry between methods at the post-verify layer. WebAuthn vs. magic-link diverge before this point (challenge persistence, token consumption, etc.) but converge here — every flow does the same thing.

ADRs already recorded:

• ADR-0004: centralize this orchestration in a single completeAuth function.
• ADR-0005: do not extract an AuthMethod interface; passkey + magic-link is the entire surface and OAuth2 is transport, not a peer.

Goals / Non-Goals

Goals:

• One function (completeAuth) is the only place where a successful web auth records terms + creates session + redirects. Bug fixes apply once.
• File reorganization: apps/journal/app/lib/auth/ directory containing completion.ts and session.ts. The existing auth.server.ts shrinks but stays for the per-method functions (passkey ceremony, magic-token lifecycle).
• returnTo sanitization centralized inside completeAuth so no caller can ship an open-redirect.
• Refactor is behaviour-preserving: existing e2e auth tests pass without modification.

Non-Goals:

• No AuthMethod interface (per ADR-0005).
• The OAuth-code issuance flow at /oauth/authorize is NOT migrated. It operates on already-authenticated users; only the trailing redirect overlaps, not enough leverage to fold in.
• The add-passkey post-login ceremony does NOT need completeAuth (no session creation — user is already logged in).
• The magic-link request-token step does NOT need completeAuth (only emits a token; verification happens later).
• Session transport (cookie vs. bearer) — orthogonal; not touched.
• Connected-devices UX — separate spec.
• Any change to the Terms gate enforcement points (root loader for web, requireApiUser for API).

Decisions

Decision 1: Single function, no mode flag

completeAuth covers exactly the four web post-verify call sites listed in the proposal. There is no redirectMode: 'cookie' | 'oauth-code' flag because OAuth-code issuance is genuinely a different flow (already-authenticated user, no session creation, only a code + redirect). Folding both into one function would force a discriminated union and obscure that they don't share much. The OAuth-code path stays in oauth.authorize.tsx unchanged.

Decision 2: Function signature

async function completeAuth(input: {
  userId: string;
  request: Request;
  returnTo?: string | null;
}): Promise<Response>

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.

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:

• completion.ts — completeAuth + safeReturnTo helper (open-redirect rejection).
• session.ts — sessionStorage, createSession, getSessionUser, destroySession. Moved from auth.server.ts.

auth.server.ts keeps the per-method functions (passkey ceremony, magic-token lifecycle, terms recording, email-change). It re-exports the moved session helpers from their new home so existing imports keep working without a touch:

// auth.server.ts
export { sessionStorage, createSession, getSessionUser, destroySession } from "./auth/session.ts";

That keeps the migration small. A future change can rip the re-exports and update callers across the app — out of scope here.

Decision 4: returnTo sanitization centralizes inside completeAuth

Today, several callers do their own returnTo checks of varying rigor (some check startsWith("/") && !startsWith("//"), others don't). Move the check into safeReturnTo inside completion.ts; every caller passes returnTo raw and gets the safe version applied uniformly. Reject anything that isn't a same-origin path → fall back to /.

Decision 5: Tests

Unit tests for completeAuth in completion.test.ts:

• new-registration writes users.terms_version + users.terms_accepted_at (mock the DB or use an in-memory test DB — match existing test patterns).
• non-registration skips the terms write.
• response includes a Set-Cookie header from sessionStorage.commitSession.
• returnTo of /foo redirects to /foo; returnTo of //evil.com/x redirects to /; returnTo of https://evil.com redirects to /; missing returnTo redirects to /.

Existing e2e/auth.test.ts (passkey register + login + logout) MUST pass without modification — that's the behaviour-preservation proof.

Risks / Trade-offs

• [Risk] Re-exporting from auth.server.ts to maintain backwards compatibility means there are temporarily two paths to the same symbols. → Mitigation: comment marks the re-exports as transitional. A follow-up PR (small) can update import paths app-wide and remove the re-exports.

• [Risk] The runtime assertion (isNewRegistration && !termsVersion → throw) is a programmer error, not a user-visible failure. If a future caller forgets to pass termsVersion, tests should catch it; if they don't, registration breaks loudly. → Accepted: the type-level alternative (discriminated union) costs more than it pays back at three internal call sites.

• [Trade-off] Centralizing returnTo sanitization changes behaviour at any call sites that don't currently sanitize. The change is strictly safer (rejecting more inputs as /) but worth confirming there are no integration tests that pass a deliberately-unsafe returnTo and expect it to work.

Migration Plan

Single PR ("spec apply") containing:

1. Create apps/journal/app/lib/auth/session.ts with the moved helpers.
2. Add re-exports in auth.server.ts.
3. Create apps/journal/app/lib/auth/completion.ts with completeAuth + safeReturnTo.
4. Write completion.test.ts (unit tests).
5. Migrate the 4 callers to completeAuth.
6. Verify pnpm typecheck && pnpm lint && pnpm test && pnpm test:e2e green.

Rollback: revert the PR. No DB or schema changes; the file moves are pure code-organization, the call-site changes are mechanical.

Open Questions

• Q1: Does sessionStorage get its secret from process.env.SESSION_SECRET directly, or from a config helper? Move-as-is and revisit only if it complicates the session.ts file. → Decide during implementation.
• Q2: Should the auth.server.ts re-exports be marked @deprecated JSDoc-style so editors warn on use? Light touch — yes, with a comment pointing at ./auth/session.ts. Decide during implementation.