From e5990754caee60056e210a79e6de2ac86c50c888 Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Thu, 9 Jul 2026 11:36:22 -0400 Subject: [PATCH 1/2] fix(auth): keep userGuid additionalField as input:true (better-auth 1.6 regression) better-auth 1.6 changed how additional user fields are parsed from the OAuth provider profile: a user additionalField declared with input:false no longer flows through when a value is supplied (in 1.6.11 the parser throws "userGuid is not allowed to be set"). Our userGuid field (MP User_GUID / OAuth sub) is populated server-side via mapProfileToUser, so input:false silently broke it: session.user.userGuid became undefined, breaking every MP profile lookup - blank avatar, dead user menu. - Flip userGuid to input:true; extract to exported userAdditionalFields const. - Add a regression guard test running the real better-auth parse function (parseAdditionalUserInput from better-auth/db) against the real field config. - Correct auth reference docs (user-identity.md, sessions.md), add a Better Auth upgrade checklist and elevate the in-memory DB-adapter warning. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/references/auth/sessions.md | 15 +++---- .claude/references/auth/user-identity.md | 54 +++++++++++++++++++----- src/auth.test.ts | 30 +++++++++++++ src/lib/auth.ts | 30 ++++++++++--- 4 files changed, 102 insertions(+), 27 deletions(-) diff --git a/.claude/references/auth/sessions.md b/.claude/references/auth/sessions.md index e15a39f..633d39c 100644 --- a/.claude/references/auth/sessions.md +++ b/.claude/references/auth/sessions.md @@ -5,7 +5,7 @@ type: reference applies_to: [src/lib/auth.ts, src/lib/auth-client.ts, src/contexts/session-context.tsx] symbols: [auth, authClient, useAppSession, Session, SessionData] related: [oauth-flow.md, user-identity.md, route-protection.md] -last_verified: 2026-04-17 +last_verified: 2026-07-09 --- ## Purpose @@ -22,7 +22,7 @@ Session strategy: stateless JWT cookie cache (no DB), a lightweight `customSessi - **Cookie cache**: 1-hour TTL (`maxAge: 60 * 60`), `strategy: "jwt"`. Subsequent `getSession()` calls within the TTL decode the cookie, bypassing any DB (there is none anyway). - **OAuth tokens in cookie**: `account.storeAccountCookie: true` + `storeStateStrategy: "cookie"` — there's no DB to persist them in. - **`customSession` runs every `getSession()` call** when the cache expires. It **must stay cheap**: no MP API calls here. Profile enrichment happens in `UserProvider` (see `../contexts/user-provider.md`). -- **`additionalFields.userGuid`** is declared with `input: false` — clients cannot set it; it's populated only by `mapProfileToUser`. See `user-identity.md`. +- **`additionalFields.userGuid`** is declared with `input: true` (extracted to the exported `userAdditionalFields` const) and populated only by `mapProfileToUser`. It **must not** be `input: false`: as of better-auth 1.6 that flag blocks the field from the OAuth profile → `userGuid` never persists → broken avatar/menu. See `user-identity.md` and the guard in `src/auth.test.ts`. - **Type export:** `export type Session = typeof auth.$Infer.Session;` — server-side consumers import this (`src/lib/auth.ts:117`). - Client-side mirror: `SessionData = typeof authClient.$Infer.Session` (`src/contexts/session-context.tsx:5`). @@ -42,13 +42,8 @@ account: { storeAccountCookie: true, }, user: { - additionalFields: { - userGuid: { - type: "string" as const, - required: false, - input: false, - }, - }, + // extracted to an exported `userAdditionalFields` const (see user-identity.md) + additionalFields: userAdditionalFields, // { userGuid: { type: "string", required: false, input: true } } }, ``` @@ -155,7 +150,7 @@ export function useAppSession() { Returns `data` directly (no `isPending` exposure). ## Gotchas -- **No DB = no cross-restart persistence.** A server restart invalidates all sessions; users must re-log in. Known limitation. +- **⚠️ No DB adapter = in-memory session store (production risk).** No `database` key is configured in `src/lib/auth.ts`, so better-auth falls back to its in-memory adapter. Session state lives only in-memory + the signed cookie. A process restart drops the store, and on serverless/Vercel **every cold start begins with an empty store** — so once the 1-hour cookie cache expires, a request that lands on a fresh instance returns `null` and the user intermittently appears logged out. **Recommended follow-up: configure a persistent adapter before production.** This is the top auth refactor, independent of the userGuid fix. - **`userGuid` requires a cast** on both server and client because `customSessionClient` and `customSession`'s inferred output don't expose `additionalFields`. This is a Better Auth type limitation. See `user-identity.md#type-casts`. - **1-hour `cookieCache` staleness**: Changes to `customSession` logic (e.g., adding a field) won't appear in returning users' sessions until the cookie cache expires or they re-auth. - **Refresh tokens**: `offline_access` scope is requested and `storeAccountCookie: true` persists tokens, but automatic refresh behavior in stateless cookie mode is **not explicitly implemented or tested** in the codebase. diff --git a/.claude/references/auth/user-identity.md b/.claude/references/auth/user-identity.md index 6aef72d..bb69320 100644 --- a/.claude/references/auth/user-identity.md +++ b/.claude/references/auth/user-identity.md @@ -5,7 +5,7 @@ type: reference applies_to: [src/lib/auth.ts, src/contexts/user-context.tsx] symbols: [userGuid, user.id, additionalFields] related: [sessions.md, oauth-flow.md, ../services/user-service.md] -last_verified: 2026-04-17 +last_verified: 2026-07-09 --- ## Purpose @@ -32,21 +32,55 @@ Better Auth explicitly strips the incoming `id` when creating the user row, usin ## `additionalFields.userGuid` declaration (verbatim) ```typescript -// src/lib/auth.ts:22-30 -user: { - additionalFields: { - userGuid: { - type: "string" as const, - required: false, - input: false, - }, +// src/lib/auth.ts — extracted to an exported const so a test can assert against it +export const userAdditionalFields = { + userGuid: { + type: "string" as const, + required: false, + input: true, }, +}; + +// ...referenced in the options: +user: { + additionalFields: userAdditionalFields, }, ``` -- `input: false` — users cannot set it during signup; only server plugins (like `mapProfileToUser`) populate it. +> ⚠️ **`userGuid` MUST keep `input: true`.** It is populated server-side from the +> OAuth profile via `mapProfileToUser`, not by user input. As of better-auth +> 1.6, the provider-profile field parser (`parseAdditionalUserInput` in +> `node_modules/better-auth/dist/db/schema.mjs`) no longer lets an +> `input: false` additionalField through when a value is supplied — in 1.6.11 it +> throws `"userGuid is not allowed to be set"`, so the OAuth-created user never +> gets a `userGuid`. Result: `session.user.userGuid` is `undefined` → blank +> avatar, dead user menu, broken MP profile lookups. There is no user-facing +> form that sets this field (genericOAuth only; no email/password signup or +> update-user endpoint), so `input: true` carries no practical risk here. +> Guarded by `src/auth.test.ts` (runs the real better-auth parser against the +> real `userAdditionalFields` config). + +- `input: true` — see the warning above. The field is only ever set server-side by `mapProfileToUser`. - `required: false` — the field is optional in the TS type, so consumers must null-check. +## Better Auth upgrade checklist + +`npm audit fix` / `npm update` can bump better-auth across minor versions (this +bug shipped via a bump from 1.4.18 → 1.6.x). CI (build + lint + unit tests) does +**not** exercise a real OAuth login, so session/OAuth regressions ship silently. +After **any** better-auth version change: + +1. Read the changelog for `genericOAuth`, `customSession`, `additionalFields`, + cookie-cache / session serialization, and `mapProfileToUser`. +2. `npm run test:run src/auth.test.ts` — runs the 1.6 field-persistence guard. +3. **Manual smoke test (required — nothing else catches this):** `npm run dev`, + **sign out and sign in fresh** (stale sessions predate any user-record change), + open `/api/auth/get-session`, and confirm `user.userGuid` is present. Confirm + the header avatar renders and the user menu opens with a working sign-out. +4. If `userGuid` is missing, inspect `parseAdditionalUserInput` (formerly + `parseAdditionalUserInputFromProviderProfile`) in + `node_modules/better-auth/dist/db/schema.mjs`. + ## `mapProfileToUser` (verbatim) ```typescript diff --git a/src/auth.test.ts b/src/auth.test.ts index 793d39e..a788ec2 100644 --- a/src/auth.test.ts +++ b/src/auth.test.ts @@ -1,4 +1,6 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { parseAdditionalUserInput } from 'better-auth/db'; +import { userAdditionalFields } from '@/lib/auth'; /** * Auth Tests @@ -186,6 +188,34 @@ describe('Auth - OAuth Configuration', () => { expect(mappedFields.userGuid).toBe('ab12cd34-ef56-7890-abcd-ef1234567890'); }); + /** + * Regression guard for the better-auth 1.6 upgrade incident. + * + * 1.6 changed how additional user fields are parsed from the OAuth provider + * profile: a user additionalField declared with `input: false` no longer + * flows through when a value is supplied. `userGuid` is populated + * server-side via mapProfileToUser (never by user input), so `input: false` + * broke it — leaving session.user.userGuid undefined and breaking every MP + * profile lookup (blank avatar, dead user menu). + * + * In 1.6.11 the exact behavior is: parseAdditionalUserInput THROWS + * "userGuid is not allowed to be set" when the field is `input: false` and a + * value is present; with `input: true` it returns { userGuid }. + * + * This runs the REAL better-auth parse function against our REAL field + * config, so it fails if (a) someone flips userGuid back to input:false, or + * (b) a future better-auth upgrade changes how provider-profile fields are + * parsed. + */ + it('persists userGuid from the OAuth provider profile (better-auth 1.6 guard)', () => { + const guid = 'ab12cd34-ef56-7890-abcd-ef1234567890'; + const options = { user: { additionalFields: userAdditionalFields } }; + + const parsed = parseAdditionalUserInput(options, { userGuid: guid }); + + expect(parsed).toHaveProperty('userGuid', guid); + }); + it('should distinguish user.id (Better Auth internal) from userGuid (MP User_GUID)', () => { // Better Auth generates its own user.id (random nanoid-style) // The OAuth sub claim is stored as userGuid via additionalFields diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 2596305..ba11c69 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -5,6 +5,28 @@ import { nextCookies } from "better-auth/next-js"; const mpBaseUrl = process.env.MINISTRY_PLATFORM_BASE_URL!; +/** + * Custom fields added to the Better Auth `user` record. + * + * `userGuid` (the MP User_GUID / OAuth `sub`) MUST keep `input: true`. It is + * populated server-side from the OAuth profile via `mapProfileToUser`. As of + * better-auth 1.6, `parseAdditionalUserInputFromProviderProfile` strips any + * additional field declared with `input: false` BEFORE the user record is + * created — so `input: false` silently drops `userGuid`, which breaks every MP + * profile lookup (avatar, user menu, User_ID resolution). There is no + * user-facing form that sets this field (the app uses genericOAuth only, with + * no email/password signup or update-user endpoint), so allowing input carries + * no practical risk here. `src/auth.test.ts` guards this against future + * regressions. + */ +export const userAdditionalFields = { + userGuid: { + type: "string" as const, + required: false, + input: true, + }, +}; + const options = { baseURL: process.env.BETTER_AUTH_URL || process.env.NEXTAUTH_URL, secret: process.env.BETTER_AUTH_SECRET || process.env.NEXTAUTH_SECRET, @@ -20,13 +42,7 @@ const options = { storeAccountCookie: true, }, user: { - additionalFields: { - userGuid: { - type: "string" as const, - required: false, - input: false, - }, - }, + additionalFields: userAdditionalFields, }, plugins: [ genericOAuth({ From d107bd20e358be57e8b43f604a235ed1879f7a4a Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Thu, 9 Jul 2026 11:36:58 -0400 Subject: [PATCH 2/2] feat(auth): recover from sessions missing userGuid instead of trapping the user A session without a userGuid rendered a dead header - no avatar/menu, no sign-out control, user stuck. Add a defensive guard so an unusable session always has an exit, independent of root cause. - AuthWrapper: session with no userGuid -> redirect to /session-error. - New /session-error page: recovery screen with a handleSignOut form button, outside the (web) route group so it can't redirect-loop. - auth-wrapper.test.tsx covers all guard branches (no session -> /signin, no userGuid -> /session-error, null userGuid -> /session-error, healthy). - Document the recovery flow in route-protection.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/references/auth/route-protection.md | 46 +++++++++++++++++++-- src/app/session-error/page.tsx | 35 ++++++++++++++++ src/components/layout/auth-wrapper.test.tsx | 39 ++++++++++++++++- src/components/layout/auth-wrapper.tsx | 11 +++++ 4 files changed, 126 insertions(+), 5 deletions(-) create mode 100644 src/app/session-error/page.tsx diff --git a/.claude/references/auth/route-protection.md b/.claude/references/auth/route-protection.md index 646f045..ee7d94f 100644 --- a/.claude/references/auth/route-protection.md +++ b/.claude/references/auth/route-protection.md @@ -5,7 +5,7 @@ type: reference applies_to: [src/proxy.ts, src/components/layout/auth-wrapper.tsx, src/app/(web)/layout.tsx] symbols: [proxy, config, AuthWrapper] related: [sessions.md, oauth-flow.md] -last_verified: 2026-04-17 +last_verified: 2026-07-09 --- ## Purpose @@ -17,11 +17,12 @@ Dual-layer route protection: edge-level cookie presence check in `src/proxy.ts` - `src/components/layout/auth-wrapper.tsx` — server-component guard used by `(web)` layout - `src/components/layout/auth-wrapper.test.tsx` — `AuthWrapper` redirect/callbackUrl tests - `src/app/(web)/layout.tsx` — mounts `` around authenticated pages +- `src/app/session-error/page.tsx` — recovery page for broken (no-`userGuid`) sessions; **outside** `(web)` ## Key concepts - **Next.js 16 renamed `middleware.ts` → `proxy.ts`** and `middleware()` → `proxy()`. The matcher is still exported as `config`. - **Layer 1 (proxy):** `getSessionCookie()` from `better-auth/cookies` — **cookie-presence check only**, no JWT decode, no DB (there's no DB). Fast path gate at the edge. -- **Layer 2 (`AuthWrapper`):** `auth.api.getSession({ headers })` in a server component — decodes the cookie, validates session, redirects to `/signin` with `callbackUrl` if invalid/missing. +- **Layer 2 (`AuthWrapper`):** `auth.api.getSession({ headers })` in a server component — decodes the cookie, validates session, redirects to `/signin` with `callbackUrl` if invalid/missing. It **also** redirects a session that exists but has no `userGuid` to `/session-error` (see "Broken-session recovery" below). - **`x-pathname` forwarding:** proxy copies `pathname + search` into a request header. `AuthWrapper` reads it via `headers()` to build an accurate `callbackUrl` even when the proxy itself lets the request through (valid cookie, decoded session invalid). - **Public paths (proxy bypass):** `/api/*` (Better Auth endpoints) and `/signin`. Static (`_next/static`, `_next/image`, `favicon.ico`, `assets/`) are excluded via the matcher. @@ -79,7 +80,7 @@ export const config = { ## AuthWrapper (verbatim, `src/components/layout/auth-wrapper.tsx`) ```typescript -// src/components/layout/auth-wrapper.tsx:5-20 +// src/components/layout/auth-wrapper.tsx export async function AuthWrapper({ children }: { children: React.ReactNode }) { const hdrs = await headers(); const session = await auth.api.getSession({ headers: hdrs }); @@ -94,6 +95,14 @@ export async function AuthWrapper({ children }: { children: React.ReactNode }) { redirect(`${signinUrl.pathname}${signinUrl.search}`); } + // A session without a userGuid is unusable: every MP lookup keys off userGuid, + // and without it the header avatar/menu never renders — leaving the user with + // no way to sign out. Route these broken sessions to a recovery page instead. + const userGuid = (session.user as { userGuid?: string | null }).userGuid; + if (!userGuid) { + redirect("/session-error"); + } + return <>{children}; } ``` @@ -119,8 +128,9 @@ Mounted via: |---|---|---| | `/api/*` | pass through (public) | not mounted (no `(web)` layout) | | `/signin` | pass through (public) | not mounted | +| `/session-error` | pass through if cookie present; else → `/signin` | not mounted (outside `(web)`) — no redirect loop | | `/_next/static/*`, `/_next/image/*`, `/favicon.ico`, `/assets/*` | excluded by matcher | not mounted | -| `/tools/...`, `/home`, etc. | redirect to `/signin?callbackUrl=` if no cookie | redirect if decoded session is missing | +| `/tools/...`, `/home`, etc. | redirect to `/signin?callbackUrl=` if no cookie | `/signin` if session missing; `/session-error` if session present but no `userGuid` | ## `callbackUrl` preservation end-to-end @@ -135,6 +145,34 @@ Tested: - `src/proxy.test.ts:98-107` — proxy attaches `callbackUrl` with query string preserved - `src/components/layout/auth-wrapper.test.tsx:51-94` — `AuthWrapper` reads `x-pathname` and falls back to `/` +## Broken-session recovery (`/session-error`) + +A session can be authenticated (cookie present, `getSession()` returns a user) +yet have **no `userGuid`** — e.g. after a better-auth upgrade drops the field +(see `user-identity.md`), or a user row created before the field existed. Such a +session is a trap: every MP lookup keys off `userGuid`, so the header +avatar/menu never render — and the normal sign-out control lives *in* that menu, +so the user is stuck with no way out. + +`AuthWrapper` defends against this independent of root cause: a session with no +`userGuid` → `redirect("/session-error")`. The page (`src/app/session-error/page.tsx`) +renders a recovery screen with a `handleSignOut` form button, giving the user an +unconditional exit. + +Why this placement: +- **Not the proxy:** it does a cookie-*presence* check and never decodes the + JWT, so it can't cheaply see `userGuid`. +- **Not cookie deletion in the wrapper:** Next.js forbids cookie mutation during + a Server Component render (only actions/route handlers) — which is exactly why + recovery uses a server-action form button, not an auto-clear. +- **`/session-error` is outside the `(web)` route group,** so it is not wrapped + by `AuthWrapper` and cannot redirect-loop. The proxy lets it through whenever a + (broken but present) session cookie exists. + +Tested: `src/components/layout/auth-wrapper.test.tsx` — no session → `/signin`, +session without `userGuid` → `/session-error`, `userGuid: null` → `/session-error`, +healthy session → renders children. + ## Gotchas - **Next.js 16 rename:** anything that refers to `middleware.ts` or `middleware()` is stale. This project uses `proxy.ts` / `proxy()`. - **Proxy is a cookie-presence check only.** A forged/expired/invalid cookie will pass the proxy and only be caught by `AuthWrapper`. This is by design (edge speed), but it means route guards **must** rely on `AuthWrapper` (or equivalent `auth.api.getSession()` check) for correctness — never the proxy alone. diff --git a/src/app/session-error/page.tsx b/src/app/session-error/page.tsx new file mode 100644 index 0000000..5aadf91 --- /dev/null +++ b/src/app/session-error/page.tsx @@ -0,0 +1,35 @@ +import { handleSignOut } from "@/components/user-menu/actions"; + +/** + * Recovery page for authenticated-but-unusable sessions. + * + * AuthWrapper redirects here when a session exists but has no `userGuid`. Such a + * session can't load an MP profile, so the header avatar/menu — and therefore + * the normal sign-out control — never render. This page gives the user an + * unconditional way out via `handleSignOut`. It lives outside the (web) route + * group, so it is NOT wrapped by AuthWrapper and cannot cause a redirect loop. + */ +export default function SessionErrorPage() { + return ( +
+
+

+ We couldn't load your account +

+

+ Your sign-in completed, but it didn't include the Ministry Platform + user link we need to load your profile. Please sign out and sign in + again. If this keeps happening, contact your administrator. +

+
+ +
+
+
+ ); +} diff --git a/src/components/layout/auth-wrapper.test.tsx b/src/components/layout/auth-wrapper.test.tsx index 3498137..b0c3c8e 100644 --- a/src/components/layout/auth-wrapper.test.tsx +++ b/src/components/layout/auth-wrapper.test.tsx @@ -94,10 +94,47 @@ describe('AuthWrapper', () => { }); }); + describe('when the session is missing userGuid', () => { + // A session that exists but has no userGuid is unusable: every MP lookup + // keys off userGuid, so the header avatar/menu — and the sign-out control — + // never render. AuthWrapper routes these to /session-error, which can still + // sign the user out. This guards the better-auth 1.6 regression. + it('redirects to /session-error when userGuid is absent', async () => { + setHeaders({ 'x-pathname': '/tools/template' }); + mockGetSession.mockResolvedValueOnce({ + user: { id: 'ba-id', name: 'No Guid' }, + session: {}, + }); + + await expect( + AuthWrapper({ children: 'content' } as never) + ).rejects.toThrow('NEXT_REDIRECT:/session-error'); + + expect(mockRedirect).toHaveBeenCalledWith('/session-error'); + }); + + it('redirects to /session-error when userGuid is null', async () => { + setHeaders({ 'x-pathname': '/tools/template' }); + mockGetSession.mockResolvedValueOnce({ + user: { id: 'ba-id', userGuid: null }, + session: {}, + }); + + await expect( + AuthWrapper({ children: 'content' } as never) + ).rejects.toThrow('NEXT_REDIRECT:/session-error'); + + expect(mockRedirect).toHaveBeenCalledWith('/session-error'); + }); + }); + describe('when authenticated', () => { it('does not redirect and returns children', async () => { setHeaders({ 'x-pathname': '/tools/template' }); - mockGetSession.mockResolvedValueOnce({ user: { id: '1' }, session: {} }); + mockGetSession.mockResolvedValueOnce({ + user: { id: '1', userGuid: 'guid-1' }, + session: {}, + }); const result = await AuthWrapper({ children: 'content' } as never); diff --git a/src/components/layout/auth-wrapper.tsx b/src/components/layout/auth-wrapper.tsx index 501e6df..91ed6de 100644 --- a/src/components/layout/auth-wrapper.tsx +++ b/src/components/layout/auth-wrapper.tsx @@ -16,5 +16,16 @@ export async function AuthWrapper({ children }: { children: React.ReactNode }) { redirect(`${signinUrl.pathname}${signinUrl.search}`); } + // A session without a userGuid is unusable: every MP lookup keys off userGuid, + // and without it the header avatar/menu never renders — which leaves the user + // with no way to even sign out (the trap behind the better-auth 1.6 regression). + // Route these broken sessions to a recovery page that CAN sign them out, + // rather than rendering a dead app. /session-error lives outside the (web) + // route group, so it is not wrapped by AuthWrapper and cannot redirect-loop. + const userGuid = (session.user as { userGuid?: string | null }).userGuid; + if (!userGuid) { + redirect("/session-error"); + } + return <>{children}; }