From 6ae05df79e8efec5d41ccbcb98602d59aba8e7a6 Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Tue, 29 Sep 2026 19:32:53 -0400 Subject: [PATCH 1/2] fix(auth): verify the MP id_token lazily instead of fetching discovery at boot (#101) genericOAuth fetched MP's OIDC discovery document once, inside the auth context's init, with no timeout or retry, and every auth request and in-process auth.api call (every page's session check) awaited it. A failed fetch dropped the provider (404 PROVIDER_NOT_FOUND until selfHealingAuth rebuilt the instance after a 30 s cooldown); a hung one stalled the first sign-in and every session check for undici's ~300 s headers timeout, which the facade's wait cap did not cover. The provider now has explicit endpoints and no discoveryUrl (authorizationUrl, tokenUrl, endSessionEndpoint for the id_token_hint, accountSubject reading profile.sub), so building the instance, next build, cold starts and session checks make no MP call and the provider always registers. The id_token is verified in getUserInfo by verifyMpIdToken: jose jwtVerify with RS256 pinned, against the issuer and JWKS from discovery, loaded by lazyIdTokenVerifier at the first callback (5 s timeout, no redirects, single-flight, cached on success, never cached on failure; jose does not cache a failed key-set fetch either). The JWKS URL is not pinned: a live MP serves /oauth/.well-known/jwks, the IdentityServer default is /openid-configuration/jwks. A document without a usable issuer or jwks_uri fails closed. An MP blip now refuses only the sign-ins that land during it, and the next one works on the same process. Removed: selfHealingAuth and its facade, requireIdTokenVerification (throws without discovery), disableIdTokenNonceBinding (a no-op without an id_token config), the unverified hand-rolled decoder, and the now-unreachable "expired"/"undecodable_id_token" reasons (jose covers them). With no id_token config better-auth also refuses the /sign-in/social idToken branch itself; the route filter and hooks.before guard stay primary. No provider.issuer skips the RFC 9207 callback iss check, moot for MP, which does not advertise authorization_response_iss_parameter_supported. New log events (identifiers only): auth.oidc.discovery_failed and auth.userinfo.id_token_unverified. The auth-error copy for unable_to_get_user_info now invites a retry. jose becomes a direct dependency. scripts/check-lockfile.mjs --fix skipped writing when the tree shape was unchanged, so a dependency that is already hoisted (only the root entry changes) never reached package-lock.json; it now writes metadata-only differences too. Tests: new src/auth.oidc-discovery.test.ts (MP rejecting, 500ing or hanging at boot with zero MP calls; discovery or JWKS failing at the callback then recovering on the same instance; timeout, redirects, single-flight, cache; lazyIdTokenVerifier units) replaces auth.discovery-rebuild.test.ts; re-adding discoveryUrl fails 11 of its 23 tests. auth.test.ts now uses genuinely signed tokens (node environment, stubUserinfo) and covers alg confusion; code-flow, id-token-sign-in, oidc-hardening, secret-guard and route tests updated; mock-oidc gains discovery/jwks/mp modes and signIdToken. The route test's fetch now blocks every URL instead of falling through to the real network. Verified with the issue's repro on next build + next start (https fake MP): MP failing before start -> /sign-in/social 200 with zero discovery requests; MP hanging on a fresh process -> every request answers in 10-40 ms. Co-Authored-By: Claude Opus 5.5 (1M context) --- package-lock.json | 1 + package.json | 1 + scripts/check-lockfile.mjs | 20 +- src/app/api/auth/[...all]/route.test.ts | 48 +- src/app/auth-error/page.tsx | 6 +- src/auth.code-flow.test.ts | 60 ++- src/auth.discovery-rebuild.test.ts | 389 ---------------- src/auth.id-token-sign-in.test.ts | 55 ++- src/auth.oidc-discovery.test.ts | 331 +++++++++++++ src/auth.oidc-hardening.test.ts | 42 +- src/auth.secret-guard.test.ts | 20 +- src/auth.test.ts | 216 ++++++--- src/components/sign-in/sign-in.test.tsx | 5 +- src/components/sign-in/sign-in.tsx | 6 +- src/lib/auth.ts | 593 ++++++++++++------------ src/test-utils/mock-oidc.ts | 46 +- 16 files changed, 983 insertions(+), 856 deletions(-) delete mode 100644 src/auth.discovery-rebuild.test.ts create mode 100644 src/auth.oidc-discovery.test.ts diff --git a/package-lock.json b/package-lock.json index 59ac1cb5..3b6fa5d0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -26,6 +26,7 @@ "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", "dotenv": "^17.3.1", + "jose": "^6.2.12", "lucide-react": "^1.16.0", "next": "^16.3.7", "react": "^19.2.4", diff --git a/package.json b/package.json index 3a9baefd..4a03c91f 100644 --- a/package.json +++ b/package.json @@ -51,6 +51,7 @@ "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", "dotenv": "^17.3.1", + "jose": "^6.2.12", "lucide-react": "^1.16.0", "next": "^16.3.7", "react": "^19.2.4", diff --git a/scripts/check-lockfile.mjs b/scripts/check-lockfile.mjs index 055eda29..2b54174f 100644 --- a/scripts/check-lockfile.mjs +++ b/scripts/check-lockfile.mjs @@ -135,8 +135,22 @@ try { const drifted = missing.length || extra.length || changed.length; + // Write with the repo's existing newline style rather than forcing LF. + const writeCanonical = () => { + const usesCRLF = readFileSync(LOCKFILE, 'utf8').includes('\r\n'); + writeFileSync(LOCKFILE, usesCRLF ? canonical.replace(/\n/g, '\r\n') : canonical); + }; + if (!drifted) { - if (committed !== canonical && !fix) { + if (committed !== canonical && fix) { + // Same tree, different metadata. `--fix` still writes it: adding a + // dependency that is already installed transitively (hoisted) changes + // only the root entry's `dependencies`, which `treeShape` skips, so + // without this the new dependency never reached the lockfile. + writeCanonical(); + console.log('✓ package-lock.json tree already matched Linux resolution; npm metadata rewritten.'); + console.log(' Review `git diff package-lock.json`, then commit it.'); + } else if (committed !== canonical) { // Same tree, different metadata. Harmless for npm ci — say so and pass, // rather than crying wolf. `npm run deps:relock` normalizes it if desired. console.log('✓ package-lock.json tree matches Linux resolution — no drift.'); @@ -148,9 +162,7 @@ try { } if (fix) { - // Write with the repo's existing newline style rather than forcing LF. - const usesCRLF = readFileSync(LOCKFILE, 'utf8').includes('\r\n'); - writeFileSync(LOCKFILE, usesCRLF ? canonical.replace(/\n/g, '\r\n') : canonical); + writeCanonical(); console.log('✓ package-lock.json rewritten to match Linux resolution.'); console.log(' Review `git diff package-lock.json`, then commit it.'); process.exit(0); diff --git a/src/app/api/auth/[...all]/route.test.ts b/src/app/api/auth/[...all]/route.test.ts index 09cf1d4f..e7087afe 100644 --- a/src/app/api/auth/[...all]/route.test.ts +++ b/src/app/api/auth/[...all]/route.test.ts @@ -17,44 +17,18 @@ import { NextRequest } from "next/server"; * importing the real `@/lib/auth` module must never reach Ministry Platform. */ -// A minimal stub of MP's OIDC discovery document. genericOAuth's plugin -// `init` fetches this eagerly when `@/lib/auth` is constructed (at import -// time, before any test body runs), so the stub has to be installed inside -// `vi.hoisted()` — the only thing that runs before the `import "./route"` -// below actually executes and triggers that construction. Without it, -// discovery fails (no real network in tests, correctly per CLAUDE.md), the -// "ministry-platform" provider is never registered, and even an ALLOWED -// `POST /sign-in/social` would 404 for the wrong reason (no such provider), -// masking whether the allowlist wrapper itself delegates correctly. +// `fetch` THROWS for every URL. Importing `@/lib/auth` and starting sign-in +// make no MP call (issue #101: the provider has explicit endpoints, and +// discovery is only fetched when a callback needs to verify an id_token), so +// the "ministry-platform" provider is registered without any stub, and nothing +// here can reach a real Ministry Platform (CLAUDE.md). Installed in +// `vi.hoisted()` so it is in place before `import "./route"` builds the auth +// instance. const { mockGetTableRecords } = vi.hoisted(() => { - const realFetch = globalThis.fetch; - const stubbedFetch: typeof fetch = (input, init) => { - const url = - typeof input === "string" - ? input - : input instanceof URL - ? input.href - : input.url; - if (url.endsWith("/oauth/.well-known/openid-configuration")) { - return Promise.resolve( - new Response( - JSON.stringify({ - issuer: "https://test-mp.example.com", - authorization_endpoint: - "https://test-mp.example.com/oauth/connect/authorize", - token_endpoint: "https://test-mp.example.com/oauth/connect/token", - // Required: requireIdTokenVerification skips the provider - // without it. Never fetched by these tests. - jwks_uri: - "https://test-mp.example.com/oauth/.well-known/openid-configuration/jwks", - }), - { status: 200, headers: { "Content-Type": "application/json" } }, - ), - ); - } - return realFetch(input, init); - }; - globalThis.fetch = stubbedFetch; + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = input instanceof Request ? input.url : String(input); + throw new Error(`Blocked unexpected fetch in test: ${url}`); + }) as typeof fetch; return { mockGetTableRecords: vi.fn(), }; diff --git a/src/app/auth-error/page.tsx b/src/app/auth-error/page.tsx index a0c57b1b..53cfe00b 100644 --- a/src/app/auth-error/page.tsx +++ b/src/app/auth-error/page.tsx @@ -40,7 +40,11 @@ import Link from "next/link"; // observed; anything not listed here — including a code from a future // better-auth version — falls back to the generic message below. const KNOWN_ERROR_MESSAGES: Record = { - unable_to_get_user_info: "We couldn't read your Ministry Platform account.", + // Also what a Ministry Platform blip during the callback looks like (the + // id_token could not be verified, or userinfo did not answer), so the + // message invites a retry. + unable_to_get_user_info: + "We couldn't read your Ministry Platform account. Please try again in a moment.", account_not_linked: "This Ministry Platform account isn't linked to an existing sign-in here.", email_not_found: diff --git a/src/auth.code-flow.test.ts b/src/auth.code-flow.test.ts index 11e6b991..337857cf 100644 --- a/src/auth.code-flow.test.ts +++ b/src/auth.code-flow.test.ts @@ -12,11 +12,12 @@ import { genericOAuth, type GenericOAuthConfig, type GenericOAuthOptions } from * state is refused before the code is ever redeemed; * - the code exchange (no PKCE verifier, since MP does not support PKCE); * - id_token verification against the mock JWKS (foreign key, wrong iss/aud, - * expired) and the app's userinfo sub binding; + * expired) — done by the app's own `verifyMpIdToken` since issue #101 — + * and the app's userinfo sub binding; * - session minting (cookies set, `/get-session` resolves the MP identity); * - sign-out effectiveness on the same instance; - * - `pkce: false` and `disableIdTokenNonceBinding: true`, pinned in config - * and behaviourally. + * - `pkce: false` and no `discoveryUrl` (so no nonce), pinned in config and + * behaviourally. * * The mock OIDC server (src/test-utils/mock-oidc.ts) signs real RS256 * id_tokens with a key its JWKS publishes. `fetch` THROWS for any URL it does @@ -78,6 +79,17 @@ function loggedText(): string { .join('\n'); } +/** The structured (JSON) events written to console.error. */ +function loggedEvents(): Array> { + return vi.mocked(console.error).mock.calls.flatMap(([line]) => { + try { + return [JSON.parse(String(line)) as Record]; + } catch { + return []; + } + }); +} + /** Asserts a refused callback: redirected to our error page with `code`, no session. */ function expectRefused(response: Response, code: string) { expect(response.status).toBe(302); @@ -127,7 +139,8 @@ describe('sign-in start: the authorize request', () => { // pkce: false — MP does not support PKCE. expect(q.has('code_challenge')).toBe(false); expect(q.has('code_challenge_method')).toBe(false); - // disableIdTokenNonceBinding: true — MP does not echo `nonce`. + // No nonce: MP does not echo one, and with no `discoveryUrl` the provider + // has no id_token config, so better-auth sends none. expect(q.has('nonce')).toBe(false); // The state rides in an encrypted, HttpOnly, 10-minute cookie. @@ -266,19 +279,21 @@ describe('callback: state validation', () => { }); describe('callback: id_token verification (the verified path)', () => { - it.each<[string, () => void]>([ - ['signed by a key the JWKS does not publish', () => (oidc.state.foreignKey = true)], - ['issued by another issuer', () => (oidc.state.claims = { iss: 'https://evil.example/oauth' })], - ['issued for another client', () => (oidc.state.claims = { aud: 'another-client' })], - ['already expired', () => (oidc.state.claims = { exp: Math.floor(T0 / 1000) - 60 })], - ])('refuses an id_token %s, before userinfo is ever called', async (_label, arrange) => { + it.each<[string, () => void, Record]>([ + ['signed by a key the JWKS does not publish', () => (oidc.state.foreignKey = true), { code: 'ERR_JWS_SIGNATURE_VERIFICATION_FAILED' }], + ['issued by another issuer', () => (oidc.state.claims = { iss: 'https://evil.example/oauth' }), { code: 'ERR_JWT_CLAIM_VALIDATION_FAILED', claim: 'iss' }], + ['issued for another client', () => (oidc.state.claims = { aud: 'another-client' }), { code: 'ERR_JWT_CLAIM_VALIDATION_FAILED', claim: 'aud' }], + ['already expired', () => (oidc.state.claims = { exp: Math.floor(T0 / 1000) - 60 }), { code: 'ERR_JWT_EXPIRED' }], + ])('refuses an id_token %s, before userinfo is ever called', async (_label, arrange, logged) => { arrange(); const { response, jar } = await codeFlow(auth, ORIGIN); expectRefused(response, 'unable_to_get_user_info'); expect(hasSessionCookie(jar)).toBe(false); expect(userinfoCalls()).toEqual([]); - expect(loggedText()).toContain('id_token failed verification'); + expect(loggedEvents()).toContainEqual( + expect.objectContaining({ event: 'auth.userinfo.id_token_unverified', reason: 'verification_failed', ...logged }), + ); }); it('refuses a verified id_token whose sub differs from the userinfo sub (sub binding)', async () => { @@ -300,32 +315,35 @@ describe('callback: id_token verification (the verified path)', () => { }); describe('provider config pins (MP supports neither PKCE nor nonce echo)', () => { - it('pins pkce: false and disableIdTokenNonceBinding: true', () => { + it('pins pkce: false, and no discoveryUrl (issue #101)', () => { const config = getMpProviderConfig(); expect(config.pkce).toBe(false); - expect(config.disableIdTokenNonceBinding).toBe(true); + expect(config).not.toHaveProperty('discoveryUrl'); }); - it('the live provider verifies id_tokens (issuer + JWKS config) but does not require a nonce', async () => { + it('the live provider has no id_token config of its own (getUserInfo verifies) and requires no nonce', async () => { const providers = (await (await auth.$context).socialProviders) as Array<{ id: string; issuer?: string; - idToken?: { issuer: string; audience: string }; + idToken?: unknown; requiresIdTokenNonce: boolean; }>; const mp = providers.find((p) => p.id === 'ministry-platform'); - expect(mp?.issuer).toBe(MOCK_ISSUER); - expect(mp?.idToken).toMatchObject({ issuer: MOCK_ISSUER, audience: MOCK_CLIENT_ID }); + expect(mp).toBeDefined(); + expect(mp?.idToken).toBeUndefined(); + expect(mp?.issuer).toBeUndefined(); expect(mp?.requiresIdTokenNonce).toBe(false); }); - it('negative control: with nonce binding on, the same MP-shaped id_token (no nonce) cannot sign in', async () => { - const withNonce = withProviderConfig({ disableIdTokenNonceBinding: false }); - const { authorizeUrl } = await startSignIn(withNonce, ORIGIN); + it('negative control: re-adding discoveryUrl switches nonce binding on, and the MP-shaped id_token (no nonce) cannot sign in', async () => { + const withDiscovery = withProviderConfig({ discoveryUrl: `${MOCK_ISSUER}/.well-known/openid-configuration` }); + const { authorizeUrl } = await startSignIn(withDiscovery, ORIGIN); expect(authorizeUrl.searchParams.has('nonce')).toBe(true); - const { response } = await codeFlow(withNonce, ORIGIN); + const { response } = await codeFlow(withDiscovery, ORIGIN); expectRefused(response, 'unable_to_get_user_info'); + // better-auth's own nonce refusal, not ours. + expect(loggedText()).toContain('id_token failed verification'); }); it('negative control: with pkce on, a code_challenge is sent (which MP would reject)', async () => { diff --git a/src/auth.discovery-rebuild.test.ts b/src/auth.discovery-rebuild.test.ts deleted file mode 100644 index 04e43692..00000000 --- a/src/auth.discovery-rebuild.test.ts +++ /dev/null @@ -1,389 +0,0 @@ -// @vitest-environment node -// (node, not jsdom: better-auth verifies the id_token with `jose` over WebCrypto, -// which rejects jsdom-realm typed arrays.) -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { toNextJsHandler } from 'better-auth/next-js'; - -/** - * Review item security-discovery-failure-no-retry: genericOAuth fetches MP's OIDC - * discovery document once per auth instance, with no retry, so one failed - * fetch at cold start used to leave `/sign-in/social` answering - * `404 PROVIDER_NOT_FOUND` until the process restarted. `selfHealingAuth` in - * src/lib/auth.ts now rebuilds the instance (single-flight, 30 s cooldown) - * when a sign-in finds the provider missing. - * - * Drives the REAL `auth` export against a mock MP OIDC provider whose - * discovery endpoint can be made to fail. `fetch` THROWS for any URL the mock - * does not serve, so nothing here can reach a real Ministry Platform. - */ - -const mockOidc = await vi.hoisted(async () => { - const { generateKeyPairSync, sign } = await import('node:crypto'); - const base = 'https://test-mp.example.com'; - const issuer = `${base}/oauth`; - const discoveryUrl = `${issuer}/.well-known/openid-configuration`; - const { publicKey, privateKey } = generateKeyPairSync('rsa', { modulusLength: 2048 }); - const jwk = { ...publicKey.export({ format: 'jwk' }), kid: 'test-key-1', alg: 'RS256', use: 'sig' }; - const sub = 'dddddddd-dddd-4ddd-8ddd-dddddddddddd'; - const state = { - /** While true, the discovery endpoint rejects (a network error). */ - discoveryDown: false, - discoveryFetches: 0, - }; - - const b64 = (v: unknown) => Buffer.from(JSON.stringify(v)).toString('base64url'); - function signIdToken(): string { - const now = Math.floor(Date.now() / 1000); - const input = `${b64({ alg: 'RS256', kid: 'test-key-1', typ: 'JWT' })}.${b64({ - iss: issuer, - aud: 'test-client-id', - sub, - iat: now, - exp: now + 300, - })}`; - return `${input}.${sign('RSA-SHA256', Buffer.from(input), privateKey).toString('base64url')}`; - } - - const json = (body: unknown, status = 200) => - new Response(JSON.stringify(body), { status, headers: { 'Content-Type': 'application/json' } }); - - globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { - const request = new Request(input, init); - const url = request.url; - if (url === discoveryUrl) { - state.discoveryFetches += 1; - if (state.discoveryDown) throw new TypeError('fetch failed'); - return json({ - issuer, - authorization_endpoint: `${issuer}/connect/authorize`, - token_endpoint: `${issuer}/connect/token`, - userinfo_endpoint: `${issuer}/connect/userinfo`, - end_session_endpoint: `${issuer}/connect/endsession`, - jwks_uri: `${issuer}/.well-known/openid-configuration/jwks`, - id_token_signing_alg_values_supported: ['RS256'], - }); - } - if (url === `${issuer}/.well-known/openid-configuration/jwks`) return json({ keys: [jwk] }); - if (url === `${issuer}/connect/token` && request.method === 'POST') { - return json({ - access_token: 'access-token-user', - id_token: signIdToken(), - token_type: 'Bearer', - expires_in: 3600, - }); - } - if (url === `${issuer}/connect/userinfo`) { - return json({ sub, given_name: 'Rebuilt', family_name: 'Instance' }); - } - throw new Error(`Blocked unexpected fetch in test: ${url}`); - }) as typeof fetch; - - return { sub, state }; -}); - -vi.mock('@/lib/providers/ministry-platform', () => ({ - MPHelper: class { - getTableRecords = vi.fn().mockResolvedValue([]); - }, -})); - -type AuthModule = typeof import('@/lib/auth'); - -const ORIGIN = 'http://localhost:3000'; -const SECOND = 1000; -const T0 = new Date('2026-09-29T08:00:00Z').getTime(); -const savedVitest = process.env.VITEST; - -function cookiePairs(response: Response, into = new Map()) { - for (const line of response.headers.getSetCookie()) { - const pair = line.split(';')[0]; - const eq = pair.indexOf('='); - const name = pair.slice(0, eq).trim(); - const value = pair.slice(eq + 1).trim(); - if (value === '' || /max-age=0/i.test(line)) into.delete(name); - else into.set(name, value); - } - return into; -} -const header = (jar: Map) => [...jar].map(([k, v]) => `${k}=${v}`).join('; '); - -async function loadAuth(): Promise { - vi.resetModules(); - return import('@/lib/auth'); -} - -function startSignIn({ auth }: AuthModule) { - return auth.handler( - new Request(`${ORIGIN}/api/auth/sign-in/social`, { - method: 'POST', - headers: { 'Content-Type': 'application/json', Origin: ORIGIN }, - body: JSON.stringify({ provider: 'ministry-platform', callbackURL: '/' }), - }), - ); -} - -/** Full code flow; returns the session cookie jar. */ -async function signIn(mod: AuthModule) { - const start = await startSignIn(mod); - expect(start.status).toBe(200); - const jar = cookiePairs(start); - const { url } = (await start.json()) as { url: string }; - const state = new URL(url).searchParams.get('state')!; - const callback = await mod.auth.handler( - new Request(`${ORIGIN}/api/auth/callback/ministry-platform?code=test-code&state=${encodeURIComponent(state)}`, { - headers: { Cookie: header(jar) }, - }), - ); - expect(callback.status).toBe(302); - expect(callback.headers.get('location')).not.toMatch(/error/); - return cookiePairs(callback, new Map()); -} - -async function sessionGuid({ auth }: AuthModule, jar: Map) { - const res = await auth.handler(new Request(`${ORIGIN}/api/auth/get-session`, { headers: { Cookie: header(jar) } })); - const body = (await res.json()) as { user?: { userGuid?: string } } | null; - return body?.user?.userGuid ?? null; -} - -const rebuildLogs = (spy: { mock: { calls: unknown[][] } }) => - spy.mock.calls - .map(([line]) => String(line)) - .filter((line: string) => line.includes('"auth.discovery.rebuild"')) - .map((line: string) => JSON.parse(line) as Record); - -let errorSpy: ReturnType; -let warnSpy: ReturnType; - -beforeEach(() => { - mockOidc.state.discoveryDown = false; - mockOidc.state.discoveryFetches = 0; - delete (globalThis as Record)[Symbol.for('mpnext.auth')]; - vi.useFakeTimers({ toFake: ['Date'] }); - vi.setSystemTime(T0); - errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); -}); - -afterEach(() => { - process.env.VITEST = savedVitest; - delete (globalThis as Record)[Symbol.for('mpnext.auth')]; - vi.useRealTimers(); - vi.restoreAllMocks(); -}); - -describe('boot-time discovery failure', () => { - it('first discovery fails, next succeeds → after the cooldown sign-in works without a restart', async () => { - mockOidc.state.discoveryDown = true; - const mod = await loadAuth(); - - const refused = await startSignIn(mod); - expect(refused.status).toBe(404); - expect(await refused.json()).toMatchObject({ code: 'PROVIDER_NOT_FOUND' }); - expect(mockOidc.state.discoveryFetches).toBe(1); - - // MP is back, but the cooldown has not elapsed: still refused, no new fetch. - mockOidc.state.discoveryDown = false; - vi.setSystemTime(T0 + 29 * SECOND); - expect((await startSignIn(mod)).status).toBe(404); - expect(mockOidc.state.discoveryFetches).toBe(1); - - vi.setSystemTime(T0 + 31 * SECOND); - const jar = await signIn(mod); - expect(mockOidc.state.discoveryFetches).toBe(2); - expect(await sessionGuid(mod, jar)).toBe(mockOidc.sub); - expect(rebuildLogs(warnSpy)).toEqual([ - expect.objectContaining({ outcome: 'recovered', providerId: 'ministry-platform' }), - ]); - // The facade now serves the rebuilt instance everywhere, not just handler. - const ctx = await mod.auth.$context; - expect(ctx.socialProviders.map((p) => p.id)).toContain('ministry-platform'); - }); - - it('works through the real route adapter (toNextJsHandler checks "handler" in auth)', async () => { - mockOidc.state.discoveryDown = true; - const mod = await loadAuth(); - const { POST } = toNextJsHandler(mod.auth); - await mod.auth.$context; - mockOidc.state.discoveryDown = false; - vi.setSystemTime(T0 + 31 * SECOND); - - const res = await POST( - new Request(`${ORIGIN}/api/auth/sign-in/social`, { - method: 'POST', - headers: { 'Content-Type': 'application/json', Origin: ORIGIN }, - body: JSON.stringify({ provider: 'ministry-platform', callbackURL: '/' }), - }), - ); - expect(res.status).toBe(200); - expect('handler' in mod.auth && 'api' in mod.auth).toBe(true); - expect('noSuchProperty' in mod.auth).toBe(false); - }); - - it('a rebuild that still finds MP down keeps refusing, logs, and waits out another cooldown', async () => { - mockOidc.state.discoveryDown = true; - const mod = await loadAuth(); - await mod.auth.$context; - - vi.setSystemTime(T0 + 31 * SECOND); - expect((await startSignIn(mod)).status).toBe(404); - expect(mockOidc.state.discoveryFetches).toBe(2); - expect(rebuildLogs(errorSpy)).toEqual([ - expect.objectContaining({ outcome: 'provider_still_missing' }), - ]); - - // Next cooldown is measured from that rebuild. - vi.setSystemTime(T0 + 55 * SECOND); - expect((await startSignIn(mod)).status).toBe(404); - expect(mockOidc.state.discoveryFetches).toBe(2); - - mockOidc.state.discoveryDown = false; - vi.setSystemTime(T0 + 62 * SECOND); - expect((await startSignIn(mod)).status).toBe(200); - expect(mockOidc.state.discoveryFetches).toBe(3); - }); - - it('concurrent sign-ins share one rebuild (no rebuild storm)', async () => { - mockOidc.state.discoveryDown = true; - const mod = await loadAuth(); - await mod.auth.$context; - mockOidc.state.discoveryDown = false; - vi.setSystemTime(T0 + 31 * SECOND); - - const responses = await Promise.all(Array.from({ length: 20 }, () => startSignIn(mod))); - - expect(responses.map((r) => r.status)).toEqual(Array(20).fill(200)); - expect(mockOidc.state.discoveryFetches).toBe(2); - expect(rebuildLogs(warnSpy)).toHaveLength(1); - }); - - it('only sign-in and callback requests trigger a rebuild', async () => { - mockOidc.state.discoveryDown = true; - const mod = await loadAuth(); - await mod.auth.$context; - mockOidc.state.discoveryDown = false; - vi.setSystemTime(T0 + 31 * SECOND); - - await mod.auth.handler(new Request(`${ORIGIN}/api/auth/get-session`)); - await mod.auth.api.getSession({ headers: new Headers() }); - expect(mockOidc.state.discoveryFetches).toBe(1); - - // A callback does (it needs the provider to redeem the code). - const cb = await mod.auth.handler( - new Request(`${ORIGIN}/api/auth/callback/ministry-platform?code=x&state=y`), - ); - expect(cb.status).toBe(302); - expect(mockOidc.state.discoveryFetches).toBe(2); - }); -}); - -describe('a healthy instance is never rebuilt', () => { - it('keeps its instance, and its sessions, however long it runs', async () => { - const mod = await loadAuth(); - const jar = await signIn(mod); - const ctxBefore = await mod.auth.$context; - - for (const minutes of [1, 5, 30]) { - vi.setSystemTime(T0 + minutes * 60 * SECOND); - expect((await startSignIn(mod)).status).toBe(200); - } - - expect(mockOidc.state.discoveryFetches).toBe(1); - expect(await mod.auth.$context).toBe(ctxBefore); - expect(await sessionGuid(mod, jar)).toBe(mockOidc.sub); - expect(rebuildLogs(warnSpy)).toHaveLength(0); - expect(rebuildLogs(errorSpy)).toHaveLength(0); - }); -}); - -describe('one shared, self-healing instance per process', () => { - it('two module copies (two Next layers) share the facade and see the rebuild', async () => { - delete process.env.VITEST; - mockOidc.state.discoveryDown = true; - const routeLayer = await loadAuth(); - const actionLayer = await loadAuth(); - expect(actionLayer.auth).toBe(routeLayer.auth); - await routeLayer.auth.$context; - - mockOidc.state.discoveryDown = false; - vi.setSystemTime(T0 + 31 * SECOND); - const jar = await signIn(routeLayer); - - // The server-action layer reads the session from the rebuilt instance. - const session = await actionLayer.auth.api.getSession({ headers: new Headers({ Cookie: header(jar) }) }); - expect(session?.user.userGuid).toBe(mockOidc.sub); - }); -}); - -describe('selfHealingAuth (unit)', () => { - type Fake = { - handler: (r: Request) => Promise; - $context: Promise<{ socialProviders: { id: string }[] }>; - tag: string; - }; - const signInRequest = () => new Request(`${ORIGIN}/api/auth/sign-in/social`, { method: 'POST' }); - const fake = (tag: string, context: Fake['$context']): Fake => ({ - tag, - $context: context, - handler: async () => new Response(tag), - }); - const missing = () => Promise.resolve({ socialProviders: [] }); - const present = () => Promise.resolve({ socialProviders: [{ id: 'ministry-platform' }] }); - - it('logs create_failed (error name only) and keeps the current instance when a rebuild throws', async () => { - const { selfHealingAuth } = await loadAuth(); - let calls = 0; - const auth = selfHealingAuth(() => { - calls += 1; - if (calls === 1) return fake('first', missing()); - throw new RangeError('boom https://secret@example'); - }); - vi.setSystemTime(T0 + 31 * SECOND); - - expect(await (await auth.handler(signInRequest())).text()).toBe('first'); - const [log] = rebuildLogs(errorSpy); - expect(log).toMatchObject({ outcome: 'create_failed', errName: 'RangeError' }); - expect(JSON.stringify(log)).not.toContain('secret'); - }); - - it('treats a rejected $context as a missing provider', async () => { - const { selfHealingAuth } = await loadAuth(); - const rejected = Promise.reject(new Error('init failed')); - rejected.catch(() => {}); - const instances = [fake('broken', rejected), fake('healed', present())]; - const auth = selfHealingAuth(() => instances.shift()!); - vi.setSystemTime(T0 + 31 * SECOND); - - expect(await (await auth.handler(signInRequest())).text()).toBe('healed'); - }); - - it('a request waits at most waitMs for a hung rebuild, which then completes in the background', async () => { - const { selfHealingAuth } = await loadAuth(); - let finishDiscovery!: () => void; - const hung = new Promise<{ socialProviders: { id: string }[] }>((resolve) => { - finishDiscovery = () => resolve({ socialProviders: [{ id: 'ministry-platform' }] }); - }); - const instances = [fake('broken', missing()), fake('healed', hung)]; - const auth = selfHealingAuth(() => instances.shift()!, { waitMs: 5 }); - vi.setSystemTime(T0 + 31 * SECOND); - - expect(await (await auth.handler(signInRequest())).text()).toBe('broken'); - // A second request during the same rebuild waits on it rather than starting another. - expect(await (await auth.handler(signInRequest())).text()).toBe('broken'); - expect(instances).toHaveLength(0); - - finishDiscovery(); - await new Promise((resolve) => setTimeout(resolve, 0)); - expect(await (await auth.handler(signInRequest())).text()).toBe('healed'); - }); - - it('leaves own properties spy-able and delegates the rest to the current instance', async () => { - const { selfHealingAuth } = await loadAuth(); - const auth = selfHealingAuth(() => fake('only', present())); - expect(auth.tag).toBe('only'); - expect((auth as unknown as { fetch: unknown }).fetch).toBe(auth.handler); - const spy = vi.spyOn(auth, 'handler').mockResolvedValue(new Response('spied')); - expect(await (await auth.handler(signInRequest())).text()).toBe('spied'); - spy.mockRestore(); - expect(await (await auth.handler(signInRequest())).text()).toBe('only'); - }); -}); diff --git a/src/auth.id-token-sign-in.test.ts b/src/auth.id-token-sign-in.test.ts index 94348d3a..ed6e4489 100644 --- a/src/auth.id-token-sign-in.test.ts +++ b/src/auth.id-token-sign-in.test.ts @@ -3,6 +3,7 @@ // which rejects jsdom-realm typed arrays.) import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { betterAuth } from 'better-auth'; +import { genericOAuth, type GenericOAuthConfig, type GenericOAuthOptions } from 'better-auth/plugins'; /** * End-to-end guard for the `/sign-in/social` id_token token-substitution @@ -10,11 +11,12 @@ import { betterAuth } from 'better-auth'; * * better-auth's `/sign-in/social` has an id_token mode: given * `idToken: { token, accessToken }` it verifies the id_token (signature, iss, - * aud) and then calls our `getUserInfo` with the CALLER'S access token. Because - * `discoveryUrl` is set, that mode is live for the "ministry-platform" + * aud) and then calls our `getUserInfo` with the CALLER'S access token. While + * `discoveryUrl` was set, that mode was live for the "ministry-platform" * provider. Nothing bound the verified id_token's `sub` to the access token's * userinfo `sub`, so an attacker's own valid id_token plus a victim's access - * token minted a session as the victim. + * token minted a session as the victim. Since issue #101 there is no + * `discoveryUrl`, so better-auth refuses the mode itself as well. * * These tests drive the REAL `auth` instance from src/lib/auth.ts against a * mock MP OIDC provider (discovery, JWKS and userinfo), with id_tokens really @@ -24,12 +26,15 @@ import { betterAuth } from 'better-auth'; * THROW for any URL the mock doesn't serve: nothing here can reach a real * Ministry Platform. * - * Two layers are proven independently here: + * Three layers are proven independently here: * - `refuseIdTokenSignIn` (`hooks.before`) refuses the mode outright. - * - With that hook removed, `getUserInfo`'s sub binding still refuses the + * - With that hook removed, better-auth refuses it (`ID_TOKEN_NOT_SUPPORTED`) + * because the provider has no id_token config without `discoveryUrl`. + * - With the hook removed AND `discoveryUrl` re-added (the configuration that + * made the mode live), `getUserInfo`'s sub binding still refuses the * substituted token (while the victim's own matched pair still works, which * proves the mode really is live and the refusal is the binding). - * The third layer, the route's body filter, is covered in + * The fourth layer, the route's body filter, is covered in * src/app/api/auth/[...all]/route.test.ts. */ @@ -219,10 +224,46 @@ describe('refuseIdTokenSignIn (hooks.before) — primary control', () => { }); }); -describe('getUserInfo sub binding — defence in depth, with the hook removed', () => { +describe('with the hook removed, better-auth refuses the mode itself (no discoveryUrl, issue #101)', () => { // Same config, same plugins, same getUserInfo — only the user hook is gone. const authWithoutHook = betterAuth({ ...auth.options, hooks: {} }); + it.each([ + ['an attacker id_token paired with a victim access token', attackBody], + [ + 'even a matched id_token + access token pair (the mode is off, not just the attack)', + () => ({ + provider: 'ministry-platform', + idToken: { token: mockOidc.signIdToken(mockOidc.subs.victim), accessToken: 'access-token-victim' }, + }), + ], + ])('refuses %s with 404 ID_TOKEN_NOT_SUPPORTED', async (_label, body) => { + const response = await signInSocial(authWithoutHook, body()); + + expect(response.status).toBe(404); + expect(await response.json()).toMatchObject({ code: 'ID_TOKEN_NOT_SUPPORTED' }); + expect(sessionCookies(response)).toEqual([]); + expect(mockOidc.userinfoCalls).toEqual([]); + }); +}); + +describe('getUserInfo sub binding — defence in depth, with the hook removed AND discoveryUrl re-added', () => { + // The configuration that made the mode live: `discoveryUrl` gives the + // provider an id_token config (and, with it, nonce binding and a mandatory + // verifier, which the old config also had to set). + const plugins = (auth.options.plugins ?? []).map((plugin) => { + if (plugin.id !== 'generic-oauth') return plugin; + const [config] = (plugin as unknown as { options: GenericOAuthOptions }).options.config; + const withDiscovery: GenericOAuthConfig = { + ...config, + discoveryUrl: 'https://test-mp.example.com/oauth/.well-known/openid-configuration', + requireIdTokenVerification: true, + disableIdTokenNonceBinding: true, + }; + return genericOAuth({ config: [withDiscovery] }); + }); + const authWithoutHook = betterAuth({ ...auth.options, plugins, hooks: {} }); + it('refuses an attacker id_token paired with a victim access token', async () => { const response = await signInSocial(authWithoutHook, attackBody()); diff --git a/src/auth.oidc-discovery.test.ts b/src/auth.oidc-discovery.test.ts new file mode 100644 index 00000000..61c73034 --- /dev/null +++ b/src/auth.oidc-discovery.test.ts @@ -0,0 +1,331 @@ +// @vitest-environment node +// (node, not jsdom: jose verifies id_tokens over WebCrypto, which rejects +// jsdom-realm typed arrays.) +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { SignJWT, exportJWK, generateKeyPair } from 'jose'; + +/** + * Issue #101: an MP blip during OIDC discovery must not take sign-in down for + * longer than the blip itself. + * + * Before, genericOAuth fetched MP's discovery document ONCE, inside the auth + * context's `init`, with no timeout and no retry, and every auth request (and + * every page's session check) awaited that `init`. A failed fetch left + * `/sign-in/social` answering `404 PROVIDER_NOT_FOUND` until the instance was + * rebuilt; a hung one stalled every request for undici's ~300 s headers + * timeout. + * + * Now the provider is configured with explicit endpoints, so building the + * instance makes no MP call at all, and the id_token verifier (issuer + JWKS) + * is loaded from discovery lazily by `lazyIdTokenVerifier`: at the first + * callback, with a timeout, cached on success, never on failure. + * + * The integration tests drive the REAL `auth` export against + * src/test-utils/mock-oidc.ts; the unit tests stub `fetch` themselves. Either + * way `fetch` THROWS for any URL not served, so nothing here can reach a real + * Ministry Platform. + */ + +const oidc = await vi.hoisted(async () => + (await import('@/test-utils/mock-oidc')).installMockOidc(), +); + +vi.mock('@/lib/providers/ministry-platform', () => ({ + MPHelper: class { + getTableRecords = vi.fn().mockResolvedValue([{ User_ID: 1357 }]); + }, +})); + +import { + CookieJar, + MOCK_ISSUER, + callback, + codeFlow, + getSession, + postSignInSocial, + startSignIn, +} from '@/test-utils/mock-oidc'; + +type AuthModule = typeof import('@/lib/auth'); + +const ORIGIN = 'http://localhost:3000'; +const DISCOVERY_URL = `${MOCK_ISSUER}/.well-known/openid-configuration`; + +/** A fresh module: a fresh auth instance and a fresh (empty) verifier cache. */ +async function freshAuth(): Promise { + vi.resetModules(); + return import('@/lib/auth'); +} + +const discoveryFetches = () => oidc.calls.filter((c) => c.url === DISCOVERY_URL).length; +const userinfoCalls = () => oidc.calls.filter((c) => c.url === `${MOCK_ISSUER}/connect/userinfo`); + +/** The structured (JSON) events written to console.error. */ +function loggedEvents(): Array> { + return vi.mocked(console.error).mock.calls.flatMap(([line]) => { + try { + return [JSON.parse(String(line)) as Record]; + } catch { + return []; + } + }); +} + +function expectRefused(location: string) { + const url = new URL(location, ORIGIN); + expect(url.pathname).toBe('/auth-error'); + expect(url.searchParams.get('error')).toBe('unable_to_get_user_info'); +} + +beforeEach(() => { + oidc.reset(); + vi.spyOn(console, 'error').mockImplementation(() => {}); + vi.spyOn(console, 'warn').mockImplementation(() => {}); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe('MP down or hanging while the app starts', () => { + // The issue's first symptom: `/signin` spinning on a 404, or every page + // hanging ~300 s, because the auth instance fetched discovery at build. + it.each(['reject', 'http_500', 'hang'] as const)( + 'MP %s: the instance builds, sign-in starts and session checks answer, with no MP call', + async (mode) => { + oidc.state.mp = mode; + const { auth } = await freshAuth(); + + // In `hang` mode any MP call here would never settle and time the test out. + await auth.$context; + const start = await postSignInSocial(auth, ORIGIN, { provider: 'ministry-platform', callbackURL: '/' }); + expect(start.status).toBe(200); + const { url } = (await start.json()) as { url: string }; + expect(url).toMatch(/^https:\/\/test-mp\.example\.com\/oauth\/connect\/authorize\?/); + expect(await getSession(auth, ORIGIN, new CookieJar())).toBeNull(); + expect(await auth.api.getSession({ headers: new Headers() })).toBeNull(); + + expect(oidc.calls).toEqual([]); + }, + ); + + it('the ministry-platform provider is always registered', async () => { + oidc.state.mp = 'reject'; + const { auth } = await freshAuth(); + const ctx = await auth.$context; + expect(ctx.socialProviders.map((p) => p.id)).toContain('ministry-platform'); + }); +}); + +describe('discovery failing at the callback: that sign-in is refused, the next one works', () => { + it.each([ + ['rejects (a network error)', 'reject', { reason: 'request_failed', errName: 'TypeError' }], + ['answers 500', 'http_500', { reason: 'http_status', status: 500 }], + ] as const)('discovery %s → refused and logged; after MP recovers, the SAME instance signs in (no restart)', async (_label, mode, logged) => { + const { auth } = await freshAuth(); + oidc.state.discovery = mode; + + const failed = await codeFlow(auth, ORIGIN); + + expectRefused(failed.location); + expect(failed.jar.get('session_token')).toBeUndefined(); + expect(userinfoCalls()).toEqual([]); + expect(loggedEvents()).toContainEqual(expect.objectContaining({ event: 'auth.oidc.discovery_failed', ...logged })); + expect(loggedEvents()).toContainEqual( + expect.objectContaining({ event: 'auth.userinfo.id_token_unverified', reason: 'verifier_unavailable' }), + ); + // Identifiers only: the discovery URL never reaches the log. + expect(vi.mocked(console.error).mock.calls.flat().join(' ')).not.toContain('test-mp.example.com'); + + oidc.state.discovery = 'ok'; + const { jar, location } = await codeFlow(auth, ORIGIN); + + expect(location).toBe('/'); + expect((await getSession(auth, ORIGIN, jar))?.user).toMatchObject({ userGuid: oidc.sub, userId: 1357 }); + // The failure was not cached: discovery was simply asked again. + expect(discoveryFetches()).toBe(2); + }); + + // The same property one layer down: jose must not cache a failed key-set + // fetch either (it clears its in-flight fetch in a `finally`). + it.each([ + ['rejects (a network error)', 'reject', { errName: 'TypeError' }], + ['answers 500', 'http_500', { code: 'ERR_JOSE_GENERIC' }], + ] as const)('the JWKS %s → refused; after MP recovers, the SAME instance signs in', async (_label, mode, logged) => { + const { auth } = await freshAuth(); + oidc.state.jwks = mode; + + const failed = await codeFlow(auth, ORIGIN); + + expectRefused(failed.location); + expect(userinfoCalls()).toEqual([]); + expect(loggedEvents()).toContainEqual( + expect.objectContaining({ event: 'auth.userinfo.id_token_unverified', reason: 'verification_failed', ...logged }), + ); + + oidc.state.jwks = 'ok'; + const { location } = await codeFlow(auth, ORIGIN); + + expect(location).toBe('/'); + // Discovery itself succeeded the first time and stayed cached. + expect(discoveryFetches()).toBe(1); + }); + + it('the discovery request carries OIDC_DISCOVERY_TIMEOUT_MS and refuses redirects', async () => { + const mod = await freshAuth(); + const mpFetch = globalThis.fetch; + const discoveryInits: RequestInit[] = []; + vi.spyOn(globalThis, 'fetch').mockImplementation((input, init) => { + if ((input instanceof Request ? input.url : String(input)) === DISCOVERY_URL) discoveryInits.push(init ?? {}); + return mpFetch(input, init); + }); + const timeoutSpy = vi.spyOn(AbortSignal, 'timeout'); + + const { location } = await codeFlow(mod.auth, ORIGIN); + + expect(location).toBe('/'); + expect(mod.OIDC_DISCOVERY_TIMEOUT_MS).toBe(5_000); + expect(discoveryInits).toHaveLength(1); + expect(discoveryInits[0].redirect).toBe('error'); + const call = timeoutSpy.mock.calls.findIndex(([ms]) => ms === mod.OIDC_DISCOVERY_TIMEOUT_MS); + expect(discoveryInits[0].signal).toBe(timeoutSpy.mock.results[call].value); + }); + + it('concurrent callbacks share one discovery fetch, and later sign-ins reuse it', async () => { + const { auth } = await freshAuth(); + const starts = await Promise.all(Array.from({ length: 5 }, () => startSignIn(auth, ORIGIN))); + + const responses = await Promise.all( + starts.map(({ jar, authorizeUrl }) => + callback(auth, ORIGIN, { code: 'test-code', state: String(authorizeUrl.searchParams.get('state')) }, jar), + ), + ); + + expect(responses.map((r) => [r.status, r.headers.get('location')])).toEqual(Array(5).fill([302, '/'])); + expect(discoveryFetches()).toBe(1); + + await codeFlow(auth, ORIGIN); + expect(discoveryFetches()).toBe(1); + }); +}); + +describe('lazyIdTokenVerifier (unit)', () => { + const BASE = 'https://mp.test/ministryplatformapi/oauth'; + const DISCOVERY = `${BASE}/.well-known/openid-configuration`; + // The live MP's JWKS path — not the IdentityServer default the mock uses — + // given as a path-absolute reference, so it must be resolved, not pinned. + const JWKS = 'https://mp.test/ministryplatformapi/oauth/.well-known/jwks'; + + interface Stub { + discovery: () => Response | Promise; + jwks?: () => Response | Promise; + } + + /** Stubs `fetch` for one test; returns the URLs requested, and each request's init. */ + function stubFetch(stub: Stub) { + const calls: Array<{ url: string; init?: RequestInit }> = []; + vi.spyOn(globalThis, 'fetch').mockImplementation(async (input, init) => { + const url = input instanceof Request ? input.url : String(input); + calls.push({ url, init }); + if (url === DISCOVERY) return stub.discovery(); + if (url === JWKS && stub.jwks) return stub.jwks(); + throw new Error(`Blocked unexpected fetch in test: ${url}`); + }); + return calls; + } + + const json = (body: unknown) => + new Response(JSON.stringify(body), { status: 200, headers: { 'Content-Type': 'application/json' } }); + const document = (overrides: Record = {}) => + json({ issuer: BASE, jwks_uri: '/ministryplatformapi/oauth/.well-known/jwks', ...overrides }); + + it('fetches nothing until first used, then resolves jwks_uri and verifies against it', async () => { + const { lazyIdTokenVerifier } = await freshAuth(); + const { publicKey, privateKey } = await generateKeyPair('RS256'); + const jwk = { ...(await exportJWK(publicKey)), kid: 'k1', alg: 'RS256' }; + const calls = stubFetch({ discovery: () => document(), jwks: () => json({ keys: [jwk] }) }); + + const load = lazyIdTokenVerifier(DISCOVERY); + expect(calls).toEqual([]); + + const verifier = await load(); + expect(verifier.issuer).toBe(BASE); + const token = await new SignJWT({ sub: 'x' }) + .setProtectedHeader({ alg: 'RS256', kid: 'k1' }) + .setIssuer(BASE) + .setExpirationTime('5m') + .sign(privateKey); + const { jwtVerify } = await import('jose'); + await expect(jwtVerify(token, verifier.jwks, { issuer: verifier.issuer })).resolves.toBeDefined(); + expect(calls.map((c) => c.url)).toEqual([DISCOVERY, JWKS]); + }); + + it('caches a success: later calls, concurrent or not, share one fetch', async () => { + const { lazyIdTokenVerifier } = await freshAuth(); + const calls = stubFetch({ discovery: () => document() }); + const load = lazyIdTokenVerifier(DISCOVERY); + + const [a, b] = await Promise.all([load(), load()]); + const c = await load(); + + expect(a).toBe(b); + expect(c).toBe(a); + expect(calls).toHaveLength(1); + }); + + it('never caches a failure: the next call fetches again', async () => { + const { lazyIdTokenVerifier } = await freshAuth(); + let down = true; + const calls = stubFetch({ + discovery: () => (down ? Promise.reject(new TypeError('fetch failed')) : document()), + }); + const load = lazyIdTokenVerifier(DISCOVERY); + + await expect(load()).rejects.toThrow(/OIDC discovery failed: request_failed/); + down = false; + await expect(load()).resolves.toMatchObject({ issuer: BASE }); + expect(calls).toHaveLength(2); + }); + + it('gives up on a hung discovery request after timeoutMs, and logs TimeoutError', async () => { + const { lazyIdTokenVerifier } = await freshAuth(); + // Accepts the request and never answers; settles only when the caller aborts. + vi.spyOn(globalThis, 'fetch').mockImplementation( + (_input, init) => + new Promise((_, reject) => { + init?.signal?.addEventListener('abort', () => reject(init.signal!.reason), { once: true }); + }), + ); + const started = Date.now(); + + await expect(lazyIdTokenVerifier(DISCOVERY, { timeoutMs: 20 })()).rejects.toThrow(/request_failed/); + + expect(Date.now() - started).toBeLessThan(2_000); + expect(loggedEvents()).toContainEqual( + expect.objectContaining({ event: 'auth.oidc.discovery_failed', reason: 'request_failed', errName: 'TimeoutError' }), + ); + }); + + it.each<[string, () => Response, Record]>([ + ['a 404', () => new Response('nope', { status: 404 }), { reason: 'http_status', status: 404 }], + ['a non-JSON 200', () => new Response('proxy', { status: 200 }), { reason: 'invalid_json', errName: 'SyntaxError' }], + ['a JSON array', () => json([]), { reason: 'invalid_document', field: 'body' }], + ['JSON null', () => json(null), { reason: 'invalid_document', field: 'body' }], + ['no issuer', () => document({ issuer: undefined }), { reason: 'invalid_document', field: 'issuer' }], + ['an issuer that is not a URL', () => document({ issuer: 'mp' }), { reason: 'invalid_document', field: 'issuer' }], + ['no jwks_uri', () => document({ jwks_uri: undefined }), { reason: 'invalid_document', field: 'jwks_uri' }], + ['a non-string jwks_uri', () => document({ jwks_uri: 42 }), { reason: 'invalid_document', field: 'jwks_uri' }], + ['an unparseable jwks_uri', () => document({ jwks_uri: 'https://[' }), { reason: 'invalid_document', field: 'jwks_uri' }], + ])('fails closed on %s, logging identifiers only', async (_label, discovery, logged) => { + const { lazyIdTokenVerifier } = await freshAuth(); + stubFetch({ discovery }); + + await expect(lazyIdTokenVerifier(DISCOVERY)()).rejects.toThrow(/OIDC discovery failed/); + + const events = loggedEvents().filter((e) => e.event === 'auth.oidc.discovery_failed'); + expect(events).toEqual([expect.objectContaining(logged)]); + const text = vi.mocked(console.error).mock.calls.flat().join(' '); + expect(text).not.toContain('mp.test'); + expect(text).not.toContain('proxy'); + }); +}); diff --git a/src/auth.oidc-hardening.test.ts b/src/auth.oidc-hardening.test.ts index 8359a736..49dcd889 100644 --- a/src/auth.oidc-hardening.test.ts +++ b/src/auth.oidc-hardening.test.ts @@ -5,10 +5,10 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; /** * End-to-end guards for the 2026-09-28 OIDC hardening in src/lib/auth.ts: - * - `requireIdTokenVerification: true` (review item - * security-require-id-token-verification): a discovery document missing - * `jwks_uri` or `issuer` must take the provider down, not leave it live with - * id_token verification silently off. + * - Fail-closed verification (review item + * security-require-id-token-verification, now enforced by `verifyMpIdToken` + * since issue #101): a discovery document missing `jwks_uri` or `issuer` + * must refuse sign-in, never let an id_token through unverified. * - id_token `exp` / `azp` checks in `getUserInfo` (review item * security-id-token-claim-checks-weak), through the real code flow with * genuinely signed tokens that better-auth's own verifier ACCEPTS. @@ -19,8 +19,8 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; * security-client-data-overexposure). * * Each test re-imports `@/lib/auth` (Vitest builds a fresh instance per - * import, see `sharedInstance`) so discovery runs again against the mock's - * current document. `fetch` THROWS for any URL the mock does not serve, so + * import, see `sharedInstance`), so the lazily cached verifier is rebuilt and + * discovery is fetched again from the mock's current document. `fetch` THROWS for any URL the mock does not serve, so * nothing here can reach a real Ministry Platform. */ @@ -167,14 +167,6 @@ function loggedEvents(): Array> { }); } -function loggedText(): string { - return vi - .mocked(console.error) - .mock.calls.flat() - .map((a) => (typeof a === 'string' ? a : JSON.stringify(a))) - .join('\n'); -} - beforeEach(() => { mockOidc.state.omit = []; mockOidc.state.claims = {}; @@ -186,27 +178,31 @@ afterEach(() => { vi.restoreAllMocks(); }); -describe('requireIdTokenVerification: a partial discovery document takes the provider down', () => { +describe('a partial discovery document can never let an unverified id_token through', () => { it('control: full discovery — sign-in starts and completes', async () => { const auth = await freshAuth(); const { location } = await codeFlow(auth); expect(location).not.toMatch(/error/); }); - // If `requireIdTokenVerification` is removed, genericOAuth keeps the provider - // live with NO id_token verifier in both cases below, sign-in returns 200, - // and these tests fail. + // If `fetchIdTokenVerifier` accepted a document without these, there would be + // no key set or no issuer to check against, and these tests would sign in. it.each([['jwks_uri'], ['issuer']])( - 'discovery without %s → sign-in refused with 404 PROVIDER_NOT_FOUND and an error log', + 'discovery without %s → the callback refuses sign-in and logs which field was missing', async (missing) => { mockOidc.state.omit = [missing]; const auth = await freshAuth(); - const response = await startSignIn(auth); + const { location, jar } = await codeFlow(auth); - expect(response.status).toBe(404); - expect(await response.json()).toMatchObject({ code: 'PROVIDER_NOT_FOUND' }); - expect(loggedText()).toContain('requires verified ID tokens'); + expect(location).toMatch(/error=unable_to_get_user_info/); + expect([...jar.keys()].some((n) => n.endsWith('session_token'))).toBe(false); + expect(loggedEvents()).toContainEqual( + expect.objectContaining({ event: 'auth.oidc.discovery_failed', reason: 'invalid_document', field: missing }), + ); + expect(loggedEvents()).toContainEqual( + expect.objectContaining({ event: 'auth.userinfo.id_token_unverified', reason: 'verifier_unavailable' }), + ); }, ); }); diff --git a/src/auth.secret-guard.test.ts b/src/auth.secret-guard.test.ts index 0d6c426f..7c6c8d9b 100644 --- a/src/auth.secret-guard.test.ts +++ b/src/auth.secret-guard.test.ts @@ -14,8 +14,9 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; * IMPORTING `@/lib/auth` throws for each bad configuration — removing the * module-level call (or any single check) turns them red. * - * `fetch` is stubbed to throw so the genericOAuth discovery fetch at init can - * never leave the process. + * `fetch` is stubbed to throw so nothing can leave the process. (Building the + * instance makes no MP call anyway since issue #101 — see the normalized-values + * test below.) */ vi.mock('@/lib/providers/ministry-platform', () => ({ @@ -166,11 +167,18 @@ describe('auth-critical URLs at module load', () => { setEnv({ BETTER_AUTH_URL: 'https://app.example.org/', MINISTRY_PLATFORM_BASE_URL: 'https://mp.example.org/api/' }); const { auth } = await import('@/lib/auth'); expect(auth.options.baseURL).toBe('https://app.example.org'); - const fetchMock = vi.mocked(fetch); + const genericOAuth = (auth.options.plugins ?? []).find((p) => p.id === 'generic-oauth') as unknown as { + options: { config: Array> }; + }; + expect(genericOAuth.options.config[0]).toMatchObject({ + authorizationUrl: 'https://mp.example.org/api/oauth/connect/authorize', + tokenUrl: 'https://mp.example.org/api/oauth/connect/token', + endSessionEndpoint: 'https://mp.example.org/api/oauth/connect/endsession', + }); + // Building the instance makes no MP call (issue #101): discovery is only + // fetched when the first id_token needs verifying. await auth.$context; - expect(fetchMock.mock.calls.map(([input]) => String(input))).toContain( - 'https://mp.example.org/api/oauth/.well-known/openid-configuration', - ); + expect(vi.mocked(fetch)).not.toHaveBeenCalled(); }); it('pins Secure cookies in production, and leaves them to the baseURL scheme otherwise', async () => { diff --git a/src/auth.test.ts b/src/auth.test.ts index 9f31120b..30303178 100644 --- a/src/auth.test.ts +++ b/src/auth.test.ts @@ -1,4 +1,8 @@ +// @vitest-environment node +// (node, not jsdom: `getUserInfo` verifies the id_token with `jose` over +// WebCrypto, which rejects jsdom-realm typed arrays.) import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { createHmac } from 'node:crypto'; import { parseAdditionalUserInputFromProviderProfile } from 'better-auth/db'; import type { GenericOAuthConfig, @@ -13,9 +17,9 @@ const { mockGetTableRecords } = vi.hoisted(() => ({ })); // Mock MP OIDC server (discovery with `issuer` + `jwks_uri`, a local JWKS), so -// the instance under test is built on the VERIFIED id_token path, not with the -// provider skipped by a failed discovery fetch. Any other fetch throws. -await vi.hoisted(async () => (await import('@/test-utils/mock-oidc')).installMockOidc()); +// `getUserInfo`'s own id_token verification (issue #101) runs against real +// RS256 signatures. Any other fetch throws. +const oidc = await vi.hoisted(async () => (await import('@/test-utils/mock-oidc')).installMockOidc()); // MPHelper is mocked as a class (not vi.fn().mockImplementation) so `new MPHelper()` // inside resolveMpUserId picks up the stubbed method — see .claude/references/testing.md. @@ -36,19 +40,33 @@ import { } from '@/lib/auth'; /** - * An UNSIGNED compact JWT carrying the given payload. `getUserInfo` reads - * `sub`, `exp`, `aud` and `azp` from the id_token without verifying it - * (genericOAuth's wrapper verifies the signature before calling it — see - * `readIdTokenClaims` in src/lib/auth.ts), so calling the configured - * `getUserInfo` directly needs no real signature. `exp` defaults to five - * minutes from now, since `getUserInfo` refuses a token without one; pass - * `exp: undefined` to omit it. src/auth.id-token-sign-in.test.ts covers the - * signed, end-to-end path. + * A genuinely signed MP id_token (RS256, by the key the mock JWKS publishes), + * for calling the configured `getUserInfo` directly: since issue #101 it + * verifies the signature, `iss` and `aud` itself. Defaults to the mock issuer, + * our client id, the mock `sub`, `iat` now and `exp` five minutes out; the + * given claims override them, and an `undefined` value drops the claim. */ -function fakeIdToken(payload: Record): string { - const enc = (v: unknown) => Buffer.from(JSON.stringify(v)).toString('base64url'); - const claims = { exp: Math.floor(Date.now() / 1000) + 300, ...payload }; - return `${enc({ alg: 'RS256', typ: 'JWT' })}.${enc(claims)}.sig`; +function signedIdToken(claims: Record): string { + return oidc.signIdToken(claims); +} + +// Captured before any test spies on `fetch`, so `stubUserinfo` can always +// hand everything but userinfo to the mock MP. +const mockMpFetch = globalThis.fetch; +const USERINFO_URL = `${process.env.MINISTRY_PLATFORM_BASE_URL}/oauth/connect/userinfo`; + +/** + * Replaces ONLY the userinfo response; discovery and the JWKS still come from + * the mock MP, because `getUserInfo` verifies the id_token before it calls + * userinfo. Returns a mock of just the userinfo calls, as `(url, init)`. + */ +function stubUserinfo(respond: () => Response | Promise) { + const userinfo = vi.fn<(url: string, init?: RequestInit) => Promise>(async () => respond()); + vi.spyOn(globalThis, 'fetch').mockImplementation((input, init) => { + const url = input instanceof Request ? input.url : String(input); + return url === USERINFO_URL ? userinfo(url, init) : mockMpFetch(input, init); + }); + return userinfo; } /** @@ -443,12 +461,41 @@ describe('Auth - OAuth Configuration', () => { // 2.1), so this must stay explicitly false (accepted risk F8 — see the // nonce comment in src/lib/auth.ts). expect(config.pkce).toBe(false); - // A partial discovery document must not silently disable id_token - // verification. Behavioural guard: src/auth.oidc-hardening.test.ts. - expect(config.requireIdTokenVerification).toBe(true); expect(config.authorizationUrlParams).toEqual({ realm: 'realm' }); }); + /** + * Issue #101: explicit endpoints, no `discoveryUrl`. With discovery, + * genericOAuth fetched it once at boot with no timeout or retry, and one MP + * blip left sign-in 404ing (or every request hanging) until something rebuilt + * the instance. The id_token is verified in `getUserInfo` instead. + */ + it('configures explicit MP endpoints and no discoveryUrl (issue #101)', () => { + const config = getMpProviderConfig(); + const oauth = `${process.env.MINISTRY_PLATFORM_BASE_URL}/oauth`; + + expect(config).not.toHaveProperty('discoveryUrl'); + // Meaningless without discovery (genericOAuth throws for it at init). + expect(config).not.toHaveProperty('requireIdTokenVerification'); + expect(config.authorizationUrl).toBe(`${oauth}/connect/authorize`); + expect(config.tokenUrl).toBe(`${oauth}/connect/token`); + // Without it, sign-out loses the MP end-session URL and its id_token_hint. + expect(config.endSessionEndpoint).toBe(`${oauth}/connect/endsession`); + }); + + it('keys the account subject on the profile sub (the MP User_GUID)', async () => { + const config = getMpProviderConfig(); + const tokens = {} as OAuth2Tokens; + const guid = 'ab12cd34-ef56-7890-abcd-ef1234599001'; + + expect( + await config.accountSubject!({ tokens, profile: { sub: guid, id: 'not-this', emailVerified: false } }), + ).toBe(guid); + // Never `id`, which getUserInfo does not return (the default resolver's + // fallback for a provider not recognised as OIDC from discovery). + expect(await config.accountSubject!({ tokens, profile: { id: 'not-this', emailVerified: false } })).toBe(''); + }); + /** * Regression guard for the better-auth 1.7 account-identity churn. * @@ -483,18 +530,16 @@ describe('Auth - OAuth Configuration', () => { /** * Regression guard for the better-auth 1.7 generic-OAuth rewrite. * - * MP's discovery document advertises `id_token_signing_alg_values_supported`, - * so better-auth treats this provider as OIDC and its default - * `accountSubject` resolver reads `profile.sub` off the raw profile returned - * by `getUserInfo`. Before 1.7 the resolver fell back to `profile.id`; that - * fallback is gone, so returning only `id` (the pre-1.7 shape) resolves the - * account subject to "" and breaks account identity for every user. + * The configured `accountSubject` resolver reads `profile.sub` off the raw + * profile returned by `getUserInfo`. Returning only `id` (the pre-1.7 shape) + * resolves the account subject to "" and breaks account identity for every + * user. */ it('returns sub (not id) from getUserInfo (better-auth 1.7 guard)', async () => { const config = getMpProviderConfig(); const guid = 'ab12cd34-ef56-7890-abcd-ef1234567890'; - const fetchSpy = vi.spyOn(globalThis, 'fetch').mockResolvedValue( + const fetchSpy = stubUserinfo(() => new Response( JSON.stringify({ sub: guid, @@ -509,7 +554,7 @@ describe('Auth - OAuth Configuration', () => { const profile = await config.getUserInfo!({ accessToken: 'access-token', - idToken: fakeIdToken({ sub: guid }), + idToken: signedIdToken({ sub: guid }), } as OAuth2Tokens); expect(fetchSpy).toHaveBeenCalledWith( @@ -542,7 +587,7 @@ describe('Auth - OAuth Configuration', () => { const config = getMpProviderConfig(); const guid = 'ab12cd34-ef56-7890-abcd-ef1234598001'; - vi.spyOn(globalThis, 'fetch').mockResolvedValue( + stubUserinfo(() => new Response( JSON.stringify({ sub: guid, @@ -557,7 +602,7 @@ describe('Auth - OAuth Configuration', () => { const profile = await config.getUserInfo!({ accessToken: 'access-token', - idToken: fakeIdToken({ sub: guid }), + idToken: signedIdToken({ sub: guid }), } as OAuth2Tokens); expect(profile).toMatchObject({ emailVerified: false }); @@ -567,7 +612,7 @@ describe('Auth - OAuth Configuration', () => { const config = getMpProviderConfig(); const guid = 'ab12cd34-ef56-7890-abcd-ef1234598002'; - vi.spyOn(globalThis, 'fetch').mockResolvedValue( + stubUserinfo(() => new Response( JSON.stringify({ sub: guid, @@ -582,7 +627,7 @@ describe('Auth - OAuth Configuration', () => { const profile = await config.getUserInfo!({ accessToken: 'access-token', - idToken: fakeIdToken({ sub: guid }), + idToken: signedIdToken({ sub: guid }), } as OAuth2Tokens); expect(profile).toMatchObject({ emailVerified: true }); @@ -590,7 +635,7 @@ describe('Auth - OAuth Configuration', () => { it('returns null from getUserInfo when the userinfo request fails', async () => { const config = getMpProviderConfig(); - vi.spyOn(globalThis, 'fetch').mockResolvedValue( + stubUserinfo(() => new Response(null, { status: 401 }), ); const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); @@ -598,7 +643,7 @@ describe('Auth - OAuth Configuration', () => { await expect( config.getUserInfo!({ accessToken: 'bad-token', - idToken: fakeIdToken({ sub: 'ab12cd34-ef56-7890-abcd-ef1234598003' }), + idToken: signedIdToken({ sub: 'ab12cd34-ef56-7890-abcd-ef1234598003' }), } as OAuth2Tokens), ).resolves.toBeNull(); expect(errorSpy).toHaveBeenCalledTimes(1); @@ -619,7 +664,7 @@ describe('Auth - OAuth Configuration', () => { describe('userinfo request robustness', () => { const guid = 'ab12cd34-ef56-7890-abcd-ef1234596001'; const tokens = () => - ({ accessToken: 'secret-access-token', idToken: fakeIdToken({ sub: guid }) }) as OAuth2Tokens; + ({ accessToken: 'secret-access-token', idToken: signedIdToken({ sub: guid }) }) as OAuth2Tokens; function loggedEvent(spy: { mock: { calls: unknown[][] } }) { expect(spy.mock.calls).toHaveLength(1); @@ -632,7 +677,7 @@ describe('Auth - OAuth Configuration', () => { const config = getMpProviderConfig(); const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); const timeout = new DOMException('The operation was aborted due to timeout', 'TimeoutError'); - vi.spyOn(globalThis, 'fetch').mockRejectedValue(timeout); + stubUserinfo(() => Promise.reject(timeout)); await expect(config.getUserInfo!(tokens())).resolves.toBeNull(); expect(loggedEvent(errorSpy)).toEqual({ @@ -646,7 +691,7 @@ describe('Auth - OAuth Configuration', () => { it('returns null when fetch rejects with a non-Error value', async () => { const config = getMpProviderConfig(); const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - vi.spyOn(globalThis, 'fetch').mockRejectedValue('boom'); + stubUserinfo(() => Promise.reject('boom')); await expect(config.getUserInfo!(tokens())).resolves.toBeNull(); expect(loggedEvent(errorSpy)).toMatchObject({ reason: 'request_failed', errName: 'string' }); @@ -655,7 +700,7 @@ describe('Auth - OAuth Configuration', () => { it('returns null for a non-JSON 200 (a proxy HTML error page), without logging the body', async () => { const config = getMpProviderConfig(); const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - vi.spyOn(globalThis, 'fetch').mockResolvedValue( + stubUserinfo(() => new Response('Gateway says hi to member@example.com', { status: 200, headers: { 'Content-Type': 'text/html' }, @@ -675,7 +720,7 @@ describe('Auth - OAuth Configuration', () => { ])('returns null when the JSON body is %s (not an object)', async (_label, body) => { const config = getMpProviderConfig(); const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - vi.spyOn(globalThis, 'fetch').mockResolvedValue( + stubUserinfo(() => new Response(body, { status: 200, headers: { 'Content-Type': 'application/json' } }), ); @@ -687,16 +732,18 @@ describe('Auth - OAuth Configuration', () => { expect(USERINFO_TIMEOUT_MS).toBe(10_000); const config = getMpProviderConfig(); const timeoutSpy = vi.spyOn(AbortSignal, 'timeout'); - const fetchSpy = vi.spyOn(globalThis, 'fetch').mockResolvedValue( + const fetchSpy = stubUserinfo(() => new Response(JSON.stringify({ sub: guid }), { status: 200 }), ); await config.getUserInfo!(tokens()); - expect(timeoutSpy).toHaveBeenCalledWith(USERINFO_TIMEOUT_MS); + // (The discovery and JWKS requests set their own, shorter, timeouts.) + const call = timeoutSpy.mock.calls.findIndex(([ms]) => ms === USERINFO_TIMEOUT_MS); + expect(call).toBeGreaterThanOrEqual(0); const init = fetchSpy.mock.calls[0][1]!; expect(init.redirect).toBe('error'); - expect(init.signal).toBe(timeoutSpy.mock.results[0].value); + expect(init.signal).toBe(timeoutSpy.mock.results[call].value); }); it.each([ @@ -707,7 +754,7 @@ describe('Auth - OAuth Configuration', () => { ['whitespace parts', { given_name: ' Pat ', family_name: ' ' }, 'Pat'], ])('builds the display name from string claims only (%s)', async (_label, claims, expected) => { const config = getMpProviderConfig(); - vi.spyOn(globalThis, 'fetch').mockResolvedValue( + stubUserinfo(() => new Response(JSON.stringify({ sub: guid, ...claims }), { status: 200 }), ); @@ -721,14 +768,15 @@ describe('Auth - OAuth Configuration', () => { /** * Review item security-id-token-claim-checks-weak: jose checks `exp` only when it * is present, and nothing checks `azp` for a multi-audience token, so - * `getUserInfo` does both (before spending a userinfo call). + * `getUserInfo` does both (before spending a userinfo call). Every token here + * is genuinely signed, so the refusal is the claim check, not the signature. */ describe('id_token claim checks', () => { const guid = 'ab12cd34-ef56-7890-abcd-ef1234595001'; const clientId = process.env.OIDC_CLIENT_ID!; function mockUserinfo() { - return vi.spyOn(globalThis, 'fetch').mockResolvedValue( + return stubUserinfo(() => new Response(JSON.stringify({ sub: guid, given_name: 'Pat', family_name: 'Doe' }), { status: 200, headers: { 'Content-Type': 'application/json' }, @@ -736,10 +784,33 @@ describe('Auth - OAuth Configuration', () => { ); } + // jose (in `verifyMpIdToken`) refuses these itself, before the app's checks. + it.each([ + ['a non-numeric exp', { exp: 'tomorrow' }, { code: 'ERR_JWT_CLAIM_VALIDATION_FAILED', claim: 'exp' }], + ['an exp in the past', { exp: Math.floor(Date.now() / 1000) - 1 }, { code: 'ERR_JWT_EXPIRED' }], + ])('jose refuses an id_token with %s, before calling userinfo', async (_label, claims, logged) => { + const config = getMpProviderConfig(); + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const fetchSpy = mockUserinfo(); + + await expect( + config.getUserInfo!({ + accessToken: 'access-token', + idToken: signedIdToken({ sub: guid, ...claims }), + } as OAuth2Tokens), + ).resolves.toBeNull(); + + expect(fetchSpy).not.toHaveBeenCalled(); + expect(errorSpy).toHaveBeenCalledTimes(1); + expect(JSON.parse(String(errorSpy.mock.calls[0][0]))).toMatchObject({ + event: 'auth.userinfo.id_token_unverified', + reason: 'verification_failed', + ...logged, + }); + }); + it.each([ ['no exp', { exp: undefined }, 'missing_exp'], - ['a non-numeric exp', { exp: 'tomorrow' }, 'missing_exp'], - ['an exp in the past', { exp: Math.floor(Date.now() / 1000) - 1 }, 'expired'], ['aud: [other, ours] with azp: other', { aud: ['other-client', clientId], azp: 'other-client' }, 'azp_mismatch'], ['aud: [other, ours] with no azp', { aud: ['other-client', clientId] }, 'azp_mismatch'], ])('refuses an id_token with %s, before calling userinfo', async (_label, claims, reason) => { @@ -750,7 +821,7 @@ describe('Auth - OAuth Configuration', () => { await expect( config.getUserInfo!({ accessToken: 'access-token', - idToken: fakeIdToken({ sub: guid, ...claims }), + idToken: signedIdToken({ sub: guid, ...claims }), } as OAuth2Tokens), ).resolves.toBeNull(); @@ -773,7 +844,7 @@ describe('Auth - OAuth Configuration', () => { await expect( config.getUserInfo!({ accessToken: 'access-token', - idToken: fakeIdToken({ sub: guid, ...claims }), + idToken: signedIdToken({ sub: guid, ...claims }), } as OAuth2Tokens), ).resolves.toMatchObject({ sub: guid }); }); @@ -867,7 +938,7 @@ describe('Auth - OAuth Configuration', () => { ])('returns null from getUserInfo when sub is %s (refuses sign-in)', async (_label, subClaim) => { const config = getMpProviderConfig(); const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - vi.spyOn(globalThis, 'fetch').mockResolvedValue( + stubUserinfo(() => new Response( JSON.stringify({ ...subClaim, @@ -884,7 +955,7 @@ describe('Auth - OAuth Configuration', () => { accessToken: 'access-token', // A valid id_token, so the refusal is provably the userinfo `sub` // check and not the id_token binding check that runs before it. - idToken: fakeIdToken({ sub: 'ab12cd34-ef56-7890-abcd-ef1234598004' }), + idToken: signedIdToken({ sub: 'ab12cd34-ef56-7890-abcd-ef1234598004' }), } as OAuth2Tokens), ).resolves.toBeNull(); expect(errorSpy).toHaveBeenCalledWith( @@ -894,18 +965,19 @@ describe('Auth - OAuth Configuration', () => { /** * Token-substitution guard (defence in depth behind `refuseIdTokenSignIn`). - * `/sign-in/social`'s id_token mode calls `getUserInfo` with a + * `/sign-in/social`'s id_token mode called `getUserInfo` with a * CALLER-SUPPLIED access token; the userinfo `sub` it yields must match the * verified id_token's `sub`, or an attacker's id_token plus a victim's access * token signs in as the victim. Every refusal returns null (never throws) and - * logs `auth.userinfo.sub_mismatch` with a reason — never the GUIDs or token - * contents themselves. + * logs a reason (`auth.userinfo.sub_mismatch`, or + * `auth.userinfo.id_token_unverified` for a token that fails verification) — + * never the GUIDs or token contents themselves. */ describe('id_token sub binding', () => { const userinfoSub = 'ab12cd34-ef56-7890-abcd-ef1234597001'; function mockUserinfo(sub: string) { - return vi.spyOn(globalThis, 'fetch').mockResolvedValue( + return stubUserinfo(() => new Response( JSON.stringify({ sub, given_name: 'Pat', family_name: 'Doe' }), { status: 200, headers: { 'Content-Type': 'application/json' } }, @@ -930,7 +1002,7 @@ describe('Auth - OAuth Configuration', () => { const profile = await config.getUserInfo!({ accessToken: 'access-token', - idToken: fakeIdToken({ sub: userinfoSub }), + idToken: signedIdToken({ sub: userinfoSub }), } as OAuth2Tokens); expect(profile).toMatchObject({ sub: userinfoSub }); @@ -942,7 +1014,7 @@ describe('Auth - OAuth Configuration', () => { const profile = await config.getUserInfo!({ accessToken: 'access-token', - idToken: fakeIdToken({ sub: userinfoSub.toUpperCase() }), + idToken: signedIdToken({ sub: userinfoSub.toUpperCase() }), } as OAuth2Tokens); // The userinfo sub is what is returned, unchanged. @@ -958,7 +1030,7 @@ describe('Auth - OAuth Configuration', () => { await expect( config.getUserInfo!({ accessToken: 'victim-access-token', - idToken: fakeIdToken({ sub: attackerSub }), + idToken: signedIdToken({ sub: attackerSub }), } as OAuth2Tokens), ).resolves.toBeNull(); @@ -972,8 +1044,10 @@ describe('Auth - OAuth Configuration', () => { expect(logged).not.toContain('victim-access-token'); }); + // Signed and verified, so the refusal is the app's check: jose type-checks + // `sub` only when asked to match a given subject. it.each([ - ['missing', {}], + ['missing', { sub: undefined }], ['empty', { sub: '' }], ['non-string', { sub: 12345 }], ])('refuses an id_token whose sub is %s, before calling userinfo', async (_label, payload) => { @@ -984,7 +1058,7 @@ describe('Auth - OAuth Configuration', () => { await expect( config.getUserInfo!({ accessToken: 'access-token', - idToken: fakeIdToken(payload), + idToken: signedIdToken(payload), } as OAuth2Tokens), ).resolves.toBeNull(); @@ -994,16 +1068,26 @@ describe('Auth - OAuth Configuration', () => { ); }); + /** An HS256 token (HMAC of `payload` under `secret`) naming the mock JWKS key's `kid`. */ + const hs256 = (payload: string, secret: string) => { + const input = `${Buffer.from(JSON.stringify({ alg: 'HS256', kid: 'test-key-1', typ: 'JWT' })).toString('base64url')}.${Buffer.from(payload).toString('base64url')}`; + return `${input}.${createHmac('sha256', secret).update(input).digest('base64url')}`; + }; + const unsigned = (claims: Record) => + `${Buffer.from(JSON.stringify({ alg: 'none', typ: 'JWT' })).toString('base64url')}.${Buffer.from(JSON.stringify(claims)).toString('base64url')}.`; + it.each([ - ['not three segments', 'only.two'], - ['a five-segment JWE', 'a.b.c.d.e'], - ['a payload that is not JSON', `x.${Buffer.from('not json').toString('base64url')}.y`], - ['a payload that is a JSON array', `x.${Buffer.from('["sub"]').toString('base64url')}.y`], - ['a payload that is JSON null', `x.${Buffer.from('null').toString('base64url')}.y`], - ])('refuses an undecodable id_token (%s), before calling userinfo', async (_label, idToken) => { + ['not three segments', 'only.two', 'ERR_JWS_INVALID'], + ['a five-segment JWE', 'a.b.c.d.e', 'ERR_JWS_INVALID'], + ['a header that is not JSON', `x.${Buffer.from('{}').toString('base64url')}.y`, 'ERR_JWS_INVALID'], + // Algorithm confusion: the client secret is not a verification key here. + ['HS256 signed with the client secret', () => hs256(JSON.stringify({ iss: oidc.issuer, aud: process.env.OIDC_CLIENT_ID, sub: userinfoSub, exp: 4102444800 }), process.env.OIDC_CLIENT_SECRET ?? 'secret'), 'ERR_JOSE_ALG_NOT_ALLOWED'], + ['unsigned (alg: none)', () => unsigned({ iss: oidc.issuer, aud: process.env.OIDC_CLIENT_ID, sub: userinfoSub, exp: 4102444800 }), 'ERR_JOSE_ALG_NOT_ALLOWED'], + ])('refuses a malformed or unverifiable id_token (%s), before calling userinfo', async (_label, token, code) => { const config = getMpProviderConfig(); const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); const fetchSpy = mockUserinfo(userinfoSub); + const idToken = typeof token === 'function' ? token() : token; await expect( config.getUserInfo!({ accessToken: 'access-token', idToken } as OAuth2Tokens), @@ -1011,8 +1095,10 @@ describe('Auth - OAuth Configuration', () => { expect(fetchSpy).not.toHaveBeenCalled(); expect(loggedEvents(errorSpy.mock.calls)).toContainEqual( - expect.objectContaining({ event: 'auth.userinfo.sub_mismatch', reason: 'undecodable_id_token' }), + expect.objectContaining({ event: 'auth.userinfo.id_token_unverified', reason: 'verification_failed', code }), ); + // Identifiers only: the token never reaches the log. + expect(errorSpy.mock.calls.flat().join(' ')).not.toContain(idToken); }); /** diff --git a/src/components/sign-in/sign-in.test.tsx b/src/components/sign-in/sign-in.test.tsx index 843f71b4..8f4142d3 100644 --- a/src/components/sign-in/sign-in.test.tsx +++ b/src/components/sign-in/sign-in.test.tsx @@ -136,8 +136,9 @@ describe("sanitizeCallbackUrl", () => { * Error handling (security-signin-page-swallows-errors, 2026-09-28). * * Every failure used to be ignored: a `{ error }` from `signIn.social` (429 - * from the rate limiter, 404 PROVIDER_NOT_FOUND when OIDC discovery failed at - * boot) or a failed `getSession()` left a spinner that never resolved, and + * from the rate limiter, 404 PROVIDER_NOT_FOUND when the provider was not + * registered — before issue #101, whenever OIDC discovery failed at boot) or a + * failed `getSession()` left a spinner that never resolved, and * nothing capped how often the page restarted OAuth by itself. */ describe("SignIn error handling", () => { diff --git a/src/components/sign-in/sign-in.tsx b/src/components/sign-in/sign-in.tsx index 34765207..e123bb9a 100644 --- a/src/components/sign-in/sign-in.tsx +++ b/src/components/sign-in/sign-in.tsx @@ -104,8 +104,10 @@ function statusOf(error: unknown): number | undefined { /** * Maps a failed `signIn.social` to what the user is told. 429 is the rate - * limiter; 404 / `PROVIDER_NOT_FOUND` means the MP provider never registered, - * typically because OIDC discovery failed at boot. + * limiter; 404 / `PROVIDER_NOT_FOUND` means the MP provider is not + * registered. Since issue #101 that no longer follows from an MP outage (the + * provider is configured without boot-time discovery), so it now means a + * misconfiguration — but the page must still say so rather than spin. */ function problemForSignInError(error: unknown): SignInProblem { const status = statusOf(error); diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 3b54e175..859ec4d7 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -8,6 +8,7 @@ import { customSession } from "better-auth/plugins"; import { nextCookies } from "better-auth/next-js"; import { APIError, createAuthMiddleware } from "better-auth/api"; import { isIP } from "node:net"; +import { createRemoteJWKSet, jwtVerify, type JWTPayload, type JWTVerifyGetKey } from "jose"; import { MPHelper } from "@/lib/providers/ministry-platform"; import { sanitizeGuid } from "@/lib/providers/ministry-platform/utils/filter-sanitize"; import { getAuthBaseUrl, getMpBaseUrl } from "@/lib/env"; @@ -276,14 +277,20 @@ export const disabledAuthPaths = [ * `idToken: { token, accessToken }` it signs the caller in directly: it * verifies the id_token (signature, iss, aud), then calls our `getUserInfo` * with the CALLER-SUPPLIED `accessToken` (node_modules/better-auth/dist/api/ - * routes/sign-in.mjs). Because `discoveryUrl` is set, genericOAuth builds an - * id_token config for this provider, which switches that mode ON + * routes/sign-in.mjs). While `discoveryUrl` was set, genericOAuth built an + * id_token config for this provider, which switched that mode ON * (`supportsIdTokenSignIn`), and genericOAuth offers no option to turn it off. * Nothing in better-auth binds the verified id_token to the access token, so * an attacker's own valid id_token plus ANY other user's MP access token * minted a session as that other user. Reproduced end to end against a mock * OIDC server. * + * Since issue #101 the provider has no `discoveryUrl` and so no id_token + * config, and better-auth refuses the mode itself (`404 + * ID_TOKEN_NOT_SUPPORTED`). That is a side effect of configuration, not a + * control — re-adding `discoveryUrl` would switch the mode straight back on — + * so this hook stays the primary one and answers first. + * * `hooks.before` runs for every endpoint on both paths — HTTP requests and * in-process `auth.api.signInSocial` calls — so refusing here covers callers * the route filter in `src/app/api/auth/[...all]/route.ts` never sees. It @@ -314,70 +321,173 @@ const refuseIdTokenSignIn = createAuthMiddleware(async (ctx) => { }); /** - * Reads the claims from a compact JWS WITHOUT verifying its signature. - * - * Deliberately unverified, and safe to be: `getUserInfo` uses these only for - * a BINDING check between two tokens (`sub`) and for claim checks jose skips - * (`exp` presence, `azp`), never as a trust decision on their own. By the time - * `getUserInfo` runs, genericOAuth's wrapper has already verified the id_token - * against MP's JWKS, issuer and audience (plugins/generic-oauth/index.mjs, - * `getUserInfo`). That is GUARANTEED, not assumed, by - * `requireIdTokenVerification: true` on the provider below: genericOAuth only - * builds its id_token verifier when discovery yields both `issuer` and - * `jwks_uri`, and without the option a partial discovery document left the - * provider live with verification silently skipped. With it, such a provider - * is skipped (sign-in 404s `PROVIDER_NOT_FOUND`, with an error log). A failed - * discovery fetch leaves no authorization endpoint, so that provider is - * skipped too (until `selfHealingAuth` rebuilds the instance and discovery - * succeeds). `src/auth.oidc-hardening.test.ts` pins both. - * - * Hand-rolled rather than `jose`'s `decodeJwt`: `jose` is only a transitive - * dependency (via better-auth), and importing it directly would break silently - * the day better-auth stops depending on it. + * Timeout for the discovery request behind `lazyIdTokenVerifier`, and for the + * JWKS request jose makes from it. Both run inside the OAuth callback, so a + * hung MP holds that one callback for at most this long — never the process. + */ +export const OIDC_DISCOVERY_TIMEOUT_MS = 5_000; + +/** + * The only id_token signing algorithm accepted. MP advertises exactly this + * (`id_token_signing_alg_values_supported: ["RS256"]`, checked against a live + * MP 2026-09-29). Pinned rather than read from discovery, so nothing in a + * token header or a discovery document can widen what is accepted. If MP ever + * signs with another algorithm, sign-in fails loudly — `auth.userinfo. + * id_token_unverified` with `code: "ERR_JOSE_ALG_NOT_ALLOWED"` — and it has to + * be added here. */ -function readIdTokenClaims( - idToken: string, -): { decoded: true; claims: Record } | { decoded: false } { - const parts = idToken.split("."); - if (parts.length !== 3) return { decoded: false }; +export const ID_TOKEN_ALGORITHMS = ["RS256"]; + +/** What `getUserInfo` verifies MP's id_tokens against. */ +export interface IdTokenVerifier { + issuer: string; + jwks: JWTVerifyGetKey; +} + +type DiscoveryFailure = + | { reason: "http_status"; status: number } + | { reason: "request_failed" | "invalid_json"; errName: string } + | { reason: "invalid_document"; field: "body" | "issuer" | "jwks_uri" }; + +/** Identifiers only: the reason, a status or an error NAME, never the body or URL. */ +function logDiscoveryFailure(detail: DiscoveryFailure) { + console.error( + JSON.stringify({ + event: "auth.oidc.discovery_failed", + message: + "MP OIDC discovery document unavailable; sign-in fails until it loads (retried on the next sign-in)", + ...detail, + }), + ); +} + +async function fetchIdTokenVerifier( + discoveryUrl: string, + timeoutMs: number, +): Promise { + const fail = (detail: DiscoveryFailure): never => { + logDiscoveryFailure(detail); + throw new Error(`OIDC discovery failed: ${detail.reason}`); + }; + let response: Response; try { - const payload: unknown = JSON.parse(Buffer.from(parts[1], "base64url").toString("utf8")); - if (typeof payload !== "object" || payload === null || Array.isArray(payload)) { - return { decoded: false }; - } - return { decoded: true, claims: payload as Record }; - } catch { - return { decoded: false }; + // `redirect: "error"`: MP serves discovery directly; a redirect is either + // a misconfiguration or someone steering us to a different key set. + response = await fetch(discoveryUrl, { + signal: AbortSignal.timeout(timeoutMs), + redirect: "error", + }); + } catch (err) { + return fail({ reason: "request_failed", errName: errorName(err) }); + } + if (!response.ok) return fail({ reason: "http_status", status: response.status }); + let doc: unknown; + try { + doc = await response.json(); + } catch (err) { + return fail({ reason: "invalid_json", errName: errorName(err) }); + } + if (typeof doc !== "object" || doc === null || Array.isArray(doc)) { + return fail({ reason: "invalid_document", field: "body" }); + } + const { issuer, jwks_uri: jwksUri } = doc as Record; + if (typeof issuer !== "string" || !URL.canParse(issuer)) { + return fail({ reason: "invalid_document", field: "issuer" }); } + if (typeof jwksUri !== "string" || !URL.canParse(jwksUri, discoveryUrl)) { + return fail({ reason: "invalid_document", field: "jwks_uri" }); + } + return { + issuer, + // jose caches the key set, re-fetches it (at most once per 30 s) when a + // token names an unknown `kid` (key rotation), and refuses a redirected + // or non-200 response. + jwks: createRemoteJWKSet(new URL(jwksUri, discoveryUrl), { timeoutDuration: timeoutMs }), + }; +} + +/** + * Returns a loader for the id_token verifier (issuer + JWKS) described by the + * OIDC discovery document at `discoveryUrl`, fetched on first use rather than + * when the auth instance is built (issue #101). + * + * - Lazy: nothing is fetched until the first OAuth callback needs to verify an + * id_token, so building the auth instance, `next build`, cold starts and + * every session check make no MP call at all. + * - Cached on success for the life of the process — the trust model genericOAuth + * had when it fetched discovery at boot. Key rotation is jose's job (above). + * - Single-flight: concurrent callbacks share one in-flight fetch. + * - Never caches a failure: a failed fetch is logged (`auth.oidc.discovery_failed`), + * that sign-in is refused, and the next callback tries again. A blip lasts + * exactly as long as MP is actually down. + * - Fails closed: a document without a usable `issuer` or `jwks_uri` is a + * failure, never a verifier that skips a check. + * + * Exported so tests can build an isolated loader; the app uses the one + * module-level instance `loadMpIdTokenVerifier`. + */ +export function lazyIdTokenVerifier( + discoveryUrl: string, + { timeoutMs = OIDC_DISCOVERY_TIMEOUT_MS }: { timeoutMs?: number } = {}, +): () => Promise { + let pending: Promise | null = null; + return () => + (pending ??= fetchIdTokenVerifier(discoveryUrl, timeoutMs).catch((err: unknown) => { + pending = null; + throw err; + })); } /** - * OIDC Core §3.1.3.7 claim checks that better-auth's verifier does not make. - * `verifyIdToken` hands jose only `issuer` and `audience`, and jose checks - * `exp` only when it is PRESENT, so a token with no `exp` passed; and nothing - * checks `azp` when `aud` lists several audiences. Returns the refusal reason, - * or null when the claims are acceptable. Runs on an already-verified token - * (see `readIdTokenClaims`), so this narrows what a genuinely MP-signed token - * may look like; it is not the signature check. + * OIDC Core §3.1.3.7 claim checks jose's `jwtVerify` does not make on its + * own: it checks `exp` only when it is PRESENT (an expired one is already + * refused there, as `ERR_JWT_EXPIRED`), so a token with no `exp` would pass; + * and nothing checks `azp` when `aud` lists several audiences. Returns the + * refusal reason, or null when the claims are acceptable. Runs on an + * already-verified token (see `verifyMpIdToken`), so this narrows what a + * genuinely MP-signed token may look like; it is not the signature check. */ function idTokenClaimFailure( claims: Record, clientId: string, -): "missing_exp" | "expired" | "azp_mismatch" | null { +): "missing_exp" | "azp_mismatch" | null { const { exp, aud, azp } = claims; - if (typeof exp !== "number" || !Number.isFinite(exp)) return "missing_exp"; - if (exp * 1000 <= Date.now()) return "expired"; + // (jose has already refused a present-but-non-numeric `exp`.) + if (typeof exp !== "number") return "missing_exp"; if (Array.isArray(aud) && azp !== clientId) return "azp_mismatch"; return null; } +/** + * Logs why `getUserInfo` refused an id_token it could not verify. + * `verifier_unavailable`: discovery did not load (its own + * `auth.oidc.discovery_failed` line says why). `verification_failed`: jose + * refused the token or could not fetch the key set; `code` is jose's error + * code (e.g. `ERR_JWS_SIGNATURE_VERIFICATION_FAILED`, `ERR_JWT_EXPIRED`, + * `ERR_JWKS_TIMEOUT`) and `claim` the claim name for a claim failure. All + * identifiers: never the token, its claims, or `err.message`. + */ +function logIdTokenUnverified( + detail: + | { reason: "verifier_unavailable" } + | { reason: "verification_failed"; code?: string; claim?: string; errName: string }, +) { + console.error( + JSON.stringify({ + event: "auth.userinfo.id_token_unverified", + message: "MP id_token could not be verified; refusing sign-in", + ...detail, + }), + ); +} + /** * Logs why `getUserInfo` refused to bind the id_token to the userinfo profile. * Identifiers only, per CLAUDE.md rule 12: never token contents and never the * GUIDs themselves (same precedent as `auth.userinfo.invalid_sub`). */ function logSubBindingFailure( - reason: "missing_id_token" | "undecodable_id_token" | "missing_sub" | "mismatch", + reason: "missing_id_token" | "missing_sub" | "mismatch", ) { console.error( JSON.stringify({ @@ -390,7 +500,7 @@ function logSubBindingFailure( } /** Same logging rules as `logSubBindingFailure`: the reason, never the claims. */ -function logIdTokenClaimFailure(reason: "missing_exp" | "expired" | "azp_mismatch") { +function logIdTokenClaimFailure(reason: "missing_exp" | "azp_mismatch") { console.error( JSON.stringify({ event: "auth.userinfo.id_token_claims_invalid", @@ -538,6 +648,81 @@ if (!process.env.VITEST) { const mpBaseUrl = getMpBaseUrl(); const authBaseUrl = getAuthBaseUrl(); +/** The genericOAuth provider id this app signs in with. */ +export const MP_PROVIDER_ID = "ministry-platform"; + +/** + * Ministry Platform's OIDC endpoints, configured explicitly instead of read + * from the discovery document at boot (issue #101). + * + * With `discoveryUrl` set, genericOAuth fetched discovery ONCE, inside the auth + * context's `init`, with no timeout and no retry, and every auth request — and + * every in-process `auth.api.*` call, i.e. every page's session check — awaits + * that `init`. A failed fetch dropped the provider for the life of the + * instance (`/sign-in/social` 404'd `PROVIDER_NOT_FOUND` until something + * rebuilt it); a hung one stalled every request for undici's ~300 s headers + * timeout. With these set and no `discoveryUrl`, building the auth instance + * makes no network call, so the provider always registers. + * + * These are IdentityServer's fixed `connect/*` paths under MP's `/oauth` + * (checked against a live MP discovery document 2026-09-29). The JWKS URL is + * deliberately NOT pinned: it is not the same on every IdentityServer (a live + * MP serves `/oauth/.well-known/jwks`; the IdentityServer default is + * `/oauth/.well-known/openid-configuration/jwks`), so it and the issuer still + * come from discovery — fetched lazily, when the first id_token needs + * verifying (`loadMpIdTokenVerifier`). + */ +const mpOidc = { + discovery: `${mpBaseUrl}/oauth/.well-known/openid-configuration`, + authorization: `${mpBaseUrl}/oauth/connect/authorize`, + token: `${mpBaseUrl}/oauth/connect/token`, + userinfo: `${mpBaseUrl}/oauth/connect/userinfo`, + endSession: `${mpBaseUrl}/oauth/connect/endsession`, +}; + +const loadMpIdTokenVerifier = lazyIdTokenVerifier(mpOidc.discovery); + +/** + * Verifies an MP id_token — RS256 signature against MP's JWKS, `iss` against + * the discovered issuer, `aud` against our client id, plus `exp`/`nbf` when + * present — and returns its claims, or null (logged) when it cannot be + * verified. + * + * This is the check genericOAuth used to make itself when discovery ran at + * boot (`verifyProviderIdToken`, the same jose `jwtVerify` call), moved here + * because without `discoveryUrl` the provider has no id_token config and + * better-auth verifies nothing. `getUserInfo` is the one place every sign-in + * path reaches with the token response in hand, so this is where it runs. + * Neither a nonce nor PKCE is checked: MP supports neither (accepted risk F8, + * see `pkce` below). + */ +async function verifyMpIdToken(idToken: string): Promise { + let verifier: IdTokenVerifier; + try { + verifier = await loadMpIdTokenVerifier(); + } catch { + logIdTokenUnverified({ reason: "verifier_unavailable" }); + return null; + } + try { + const { payload } = await jwtVerify(idToken, verifier.jwks, { + issuer: verifier.issuer, + audience: process.env.OIDC_CLIENT_ID!, + algorithms: ID_TOKEN_ALGORITHMS, + }); + return payload; + } catch (err) { + const { code, claim } = (err ?? {}) as { code?: unknown; claim?: unknown }; + logIdTokenUnverified({ + reason: "verification_failed", + ...(typeof code === "string" && { code }), + ...(typeof claim === "string" && { claim }), + errName: errorName(err), + }); + return null; + } +} + /** * Client-IP resolution for better-auth's rate limiter (`/sign-in*` is 3 * requests per 10 s per IP in production). By default better-auth trusts only @@ -748,33 +933,36 @@ const options = { genericOAuth({ config: [ { - providerId: "ministry-platform", - discoveryUrl: `${mpBaseUrl}/oauth/.well-known/openid-configuration`, - // No issuer pinning here, deliberately. better-auth 1.7.0–1.7.2 keyed - // accounts on (issuer, accountId) and refused to initialize a discovery - // provider whose issuer it could not resolve, so this config carried an - // explicit `accountIssuer`. 1.7.3 reverted both halves: accounts are - // identified by (providerId, accountId) again as in 1.6 (#11153), and a - // discovery failure no longer takes down the auth API (#10978). The - // option was removed along with that revert, so setting it is now a - // type error. Discovery still supplies the endpoints and the JWKS used - // to verify ID tokens. - // - // Discovery runs ONCE per auth instance, with no retry. If that fetch - // fails, genericOAuth logs "Discovery fetch failed" and skips the - // provider, so sign-in fails CLOSED (`404 PROVIDER_NOT_FOUND`). (Before - // 1.7.3 the provider stayed live with no id_token verification; that - // fail-open mode is gone.) It no longer lasts until a restart: - // `selfHealingAuth` below rebuilds the instance — re-running - // discovery — on the next sign-in after a 30 s cooldown. + providerId: MP_PROVIDER_ID, + // NO `discoveryUrl`, deliberately (issue #101): see `mpOidc` above. + // Setting it would bring back the boot-time fetch that took sign-in + // down on an MP blip, AND give the provider an id_token config, which + // re-opens `/sign-in/social`'s id_token mode (see + // `refuseIdTokenSignIn`). The id_token is verified in `getUserInfo` + // instead (`verifyMpIdToken`). `src/auth.test.ts` and + // `src/auth.code-flow.test.ts` fail if it comes back, and + // `src/auth.oidc-discovery.test.ts` pins why. // - // Without this, a discovery document that returned the endpoints but - // omitted `jwks_uri` or `issuer` left the provider live with id_token - // verification silently OFF (genericOAuth builds its verifier only - // when both are present). With it, that provider is skipped with an - // error log instead. `readIdTokenClaims` relies on this guarantee; - // `src/auth.oidc-hardening.test.ts` fails if it is removed. - requireIdTokenVerification: true, + // No issuer pinning either. better-auth 1.7.0–1.7.2 keyed accounts on + // (issuer, accountId), so this config once carried an explicit + // `accountIssuer`; 1.7.3 reverted that (accounts are (providerId, + // accountId) again, #11153) and removed the option. Without discovery + // the provider has no `issuer`, so better-auth's RFC 9207 callback + // `iss` check is skipped — moot for MP, whose discovery document does + // not advertise `authorization_response_iss_parameter_supported` (it + // sends no `iss`), and a single-provider app has no mix-up to defend. + authorizationUrl: mpOidc.authorization, + tokenUrl: mpOidc.token, + // Required for RP-initiated logout: without it better-auth builds no + // MP end-session URL, and sign-out loses the `id_token_hint` that + // lets MP end its own session without a "log out?" prompt. + endSessionEndpoint: mpOidc.endSession, + // The account subject is the MP User_GUID. genericOAuth's default + // resolver reads `profile.sub` only for a provider it recognised as + // OIDC from a discovery document, and `profile.id` otherwise — which + // `getUserInfo` never returns, so the default would key every + // account on "". `getUserInfo` has already validated `sub`. + accountSubject: ({ profile }) => (typeof profile.sub === "string" ? profile.sub : ""), clientId: process.env.OIDC_CLIENT_ID!, clientSecret: process.env.OIDC_CLIENT_SECRET!, scopes: [ @@ -788,28 +976,17 @@ const options = { // `code_challenge_methods_supported`; that does not mean it works.) // The consequence is spelled out in the nonce comment below. pkce: false, - // Ministry Platform does not echo the `nonce` back in the id_token, - // so better-auth's nonce binding must be switched off or NO ONE CAN - // SIGN IN. - // - // better-auth 1.7 turns nonce binding on automatically for any - // provider whose discovery document yields an id_token config - // (`requiresIdTokenNonce`), sends a `nonce` on the authorize request, - // and then requires the claim to come back: - // `nonceMatches()` returns false when the claim is absent - // (`typeof claimNonce !== "string"`). MP omits it, so verification - // failed every time with `unable_to_get_user_info` and the log line - // "id_token failed verification against the discovery JWKS or - // expected nonce". Verified 2026-09-12 by decoding a real MP - // id_token: kid, alg, iss and aud all matched; only `nonce` was - // missing. - // - // (It looked intermittent at the time because, before better-auth - // 1.7.3, a FAILED boot-time discovery left the provider live with no - // id_token config, which skipped verification and so let sign-in - // succeed. In 1.7.4 a failed discovery skips the provider entirely — - // sign-in 404s until `selfHealingAuth` rebuilds the instance — and - // `requireIdTokenVerification` above refuses a partial one.) + // No nonce either: Ministry Platform does not echo the `nonce` back + // in the id_token. With no `discoveryUrl` the provider has no + // id_token config, so better-auth neither sends a `nonce` nor + // requires one (`requiresIdTokenNonce` is false), and + // `verifyMpIdToken` checks none. (While discovery was configured, + // better-auth 1.7 switched nonce binding on automatically and every + // MP sign-in failed with `unable_to_get_user_info` until + // `disableIdTokenNonceBinding: true` was set — verified 2026-09-12 + // by decoding a real MP id_token: kid, alg, iss and aud all matched; + // only `nonce` was missing. `src/auth.code-flow.test.ts` keeps that + // negative control.) // // What this does NOT give up: the id_token signature is still checked // against MP's JWKS, and the issuer and audience are still checked. @@ -826,49 +1003,45 @@ const options = { // defences keep codes out of an attacker's reach: a dedicated MP // OIDC client with exact redirect URIs, `Referrer-Policy`, and no // code-bearing URLs in logs. - disableIdTokenNonceBinding: true, authorizationUrlParams: { realm: "realm", }, getUserInfo: async (tokens) => { - // Bind the access token to the (already verified) id_token: the - // userinfo `sub` must equal the id_token `sub`. Defence in depth - // behind `refuseIdTokenSignIn` — `/sign-in/social`'s id_token mode - // calls this with a CALLER-SUPPLIED access token, so without this - // check an attacker's id_token plus a victim's access token signed - // in as the victim. Every refusal returns null, never throws (see - // the `sub` comment below for why). + // Verify the id_token (`verifyMpIdToken`: signature, iss, aud), + // then bind the access token to it: the userinfo `sub` must equal + // the verified id_token `sub`. The binding is defence in depth for + // any path that hands this function a CALLER-SUPPLIED access token + // — `/sign-in/social`'s id_token mode did, so an attacker's + // id_token plus a victim's access token signed in as the victim + // (see `refuseIdTokenSignIn`). Every refusal returns null, never + // throws (see the `sub` comment below for why). // // A MISSING id_token fails closed too, deliberately. Every // legitimate caller supplies one: the code-flow callback requests // the `openid` scope, for which OIDC Core §3.1.3.3 requires an // id_token in the token response (MP sends one — the nonce comment - // above records decoding a real one), and the id_token mode cannot - // run without one. So an absent id_token means an unexpected code - // path handing us an access token with nothing to bind it to; a - // future better-auth path of that shape should be refused, not - // trusted. The cost, if MP ever stopped sending id_tokens, is a - // loud sign-in outage logged as `reason: "missing_id_token"`, not a - // silent takeover. Checked before the userinfo fetch so a refused - // request never spends an MP call. + // above records decoding a real one). So an absent id_token means + // an unexpected code path handing us an access token with nothing + // to verify or bind; a future better-auth path of that shape should + // be refused, not trusted. The cost, if MP ever stopped sending + // id_tokens, is a loud sign-in outage logged as + // `reason: "missing_id_token"`, not a silent takeover. Checked + // before any fetch so a refused request never spends an MP call. if (typeof tokens.idToken !== "string" || tokens.idToken === "") { logSubBindingFailure("missing_id_token"); return null; } - const idToken = readIdTokenClaims(tokens.idToken); - if (!idToken.decoded) { - logSubBindingFailure("undecodable_id_token"); - return null; - } - const idTokenSub = idToken.claims.sub; + const claims = await verifyMpIdToken(tokens.idToken); + if (!claims) return null; + const idTokenSub = claims.sub; if (typeof idTokenSub !== "string" || idTokenSub === "") { logSubBindingFailure("missing_sub"); return null; } - // `exp` present and in the future; `azp` is us when `aud` is a - // list. See `idTokenClaimFailure`. + // `exp` present (jose has already refused an expired one); `azp` + // is us when `aud` is a list. See `idTokenClaimFailure`. const claimFailure = idTokenClaimFailure( - idToken.claims, + claims, process.env.OIDC_CLIENT_ID!, ); if (claimFailure) { @@ -886,7 +1059,7 @@ const options = { let profile: Record; try { const response = await fetch( - `${mpBaseUrl}/oauth/connect/userinfo`, + mpOidc.userinfo, { headers: { Authorization: `Bearer ${tokens.accessToken}`, @@ -952,13 +1125,11 @@ const options = { return null; } - // `sub` (not `id`) is what better-auth 1.7 reads for the account - // subject. MP's discovery document advertises - // `id_token_signing_alg_values_supported`, so the provider is - // treated as OIDC and the default `accountSubject` resolver reads - // `profile.sub` from this raw profile. Returning only `id` (the - // pre-1.7 shape) resolves the subject to "" and breaks account - // identity. `src/auth.test.ts` guards this. + // `sub` (not `id`) is what the account subject is keyed on: the + // `accountSubject` resolver above reads `profile.sub` from this raw + // profile. Returning only `id` (the pre-1.7 shape) resolves the + // subject to "" and breaks account identity. `src/auth.test.ts` + // guards this. // // `email` here is the REAL MP address, kept on the raw profile so // `mapProfileToUser` can move it into `mpEmail`. It does not reach @@ -1084,172 +1255,6 @@ export function sharedInstance( return (store[key] ??= create()); } -/** The genericOAuth provider id this app signs in with. */ -export const MP_PROVIDER_ID = "ministry-platform"; - -/** - * Minimum time between two builds of the auth instance (the first build - * counts). Bounds discovery traffic to one fetch per 30 s per process while MP - * is down, however many users retry sign-in. - */ -export const DISCOVERY_REBUILD_COOLDOWN_MS = 30 * 1000; - -/** - * How long a sign-in request waits for an in-flight rebuild before going ahead - * with the current (provider-less) instance. genericOAuth's discovery fetch has - * no timeout of its own, so without this a hung MP would hold every sign-in - * request open; the rebuild itself carries on in the background. - */ -export const DISCOVERY_REBUILD_WAIT_MS = 10 * 1000; - -interface RebuildableAuth { - handler: (request: Request) => Promise; - $context: Promise<{ socialProviders: ReadonlyArray<{ id: string }> }>; -} - -/** - * Only these requests need the MP provider; everything else (`/get-session`, - * `/sign-out`, ...) never triggers a rebuild, so an MP outage cannot slow page - * loads down. Paths are under better-auth's default `/api/auth` basePath. - */ -function needsProvider(request: Request): boolean { - // `Request.url` is always absolute, so this cannot throw. - const path = new URL(request.url).pathname.replace(/\/+$/, ""); - return path === "/api/auth/sign-in/social" || path.startsWith("/api/auth/callback/"); -} - -async function hasProvider(instance: RebuildableAuth): Promise { - try { - const ctx = await instance.$context; - return ctx.socialProviders.some((provider) => provider.id === MP_PROVIDER_ID); - } catch { - return false; - } -} - -/** Identifiers only: never the discovery URL or the error message. */ -function logDiscoveryRebuild( - outcome: "recovered" | "provider_still_missing" | "create_failed", - errName?: string, -) { - const log = outcome === "recovered" ? console.warn : console.error; - log( - JSON.stringify({ - event: "auth.discovery.rebuild", - message: - outcome === "recovered" - ? "Rebuilt the auth instance; the MP OIDC provider is available again" - : "Rebuilt the auth instance; the MP OIDC provider is still unavailable", - providerId: MP_PROVIDER_ID, - outcome, - ...(errName && { errName }), - }), - ); -} - -/** - * Wraps `create` so a boot-time OIDC discovery failure heals without a - * restart. - * - * genericOAuth fetches MP's discovery document ONCE, while the auth context - * initializes, with no retry. If that fetch fails (or the document lacks - * `issuer`/`jwks_uri` — see `requireIdTokenVerification`), the provider is - * skipped and `/sign-in/social` returns `404 PROVIDER_NOT_FOUND` for the life - * of the instance: one transient MP blip at cold start used to disable sign-in - * until the process restarted. Fail-closed, but an outage. - * - * Now a sign-in or callback request that finds the provider missing builds a - * fresh instance — which runs discovery again — and swaps it in if (and only - * if) the new one has the provider: - * - single-flight: concurrent requests share one rebuild; - * - at most one build per `DISCOVERY_REBUILD_COOLDOWN_MS`, counting the first; - * inside the cooldown the request just gets the 404, as before; - * - a request waits at most `DISCOVERY_REBUILD_WAIT_MS` for the rebuild; - * - an instance that HAS the provider is never rebuilt, so a healthy - * instance's in-memory sessions and account rows (the `id_token_hint` for - * sign-out) are never thrown away. Swapping out a provider-less instance - * loses nothing: without the provider no OAuth callback could have stored a - * session in it (and `session_data` cookies are keyed from the secret, not - * the instance). - * - * Transparent to callers: the returned object delegates every property to the - * CURRENT instance at access time (`auth.api.getSession(...)`, - * `auth.$context`, ...), and its `handler` — what `toNextJsHandler` in the - * `/api/auth` route calls per request — does the check first. So never cache - * `auth.api` in a module-level variable; it would pin the first instance. - * In-process `auth.api.signInSocial` calls do not trigger a rebuild (nothing - * in `src/` makes one). `src/auth.discovery-rebuild.test.ts` pins all of this. - */ -export function selfHealingAuth( - create: () => T, - { - cooldownMs = DISCOVERY_REBUILD_COOLDOWN_MS, - waitMs = DISCOVERY_REBUILD_WAIT_MS, - }: { cooldownMs?: number; waitMs?: number } = {}, -): T { - let current = create(); - let builtAt = Date.now(); - let rebuilding: Promise | null = null; - - async function rebuild(): Promise { - builtAt = Date.now(); - let candidate: T; - try { - candidate = create(); - } catch (err) { - logDiscoveryRebuild("create_failed", errorName(err)); - return; - } - if (await hasProvider(candidate)) { - current = candidate; - logDiscoveryRebuild("recovered"); - } else { - logDiscoveryRebuild("provider_still_missing"); - } - } - - async function ensureProvider(): Promise { - if (await hasProvider(current)) return; - if (!rebuilding) { - if (Date.now() - builtAt < cooldownMs) return; - rebuilding = rebuild().finally(() => { - rebuilding = null; - }); - } - let timer: ReturnType | undefined; - await Promise.race([ - rebuilding, - new Promise((resolve) => { - timer = setTimeout(resolve, waitMs); - }), - ]); - clearTimeout(timer); - } - - const handler = async (request: Request): Promise => { - if (needsProvider(request)) await ensureProvider(); - return current.handler(request); - }; - - // `handler`/`fetch` are real own properties of the target (so `vi.spyOn` - // and friends can redefine them); every other property reads through to the - // current instance. - const own: Pick & { fetch: RebuildableAuth["handler"] } = { - handler, - fetch: handler, - }; - return new Proxy(own, { - get(target, prop, receiver) { - if (Object.hasOwn(target, prop)) return Reflect.get(target, prop, receiver); - return Reflect.get(current, prop, current); - }, - // `toNextJsHandler` checks `"handler" in auth` before calling it. - has(target, prop) { - return prop in target || prop in current; - }, - }) as unknown as T; -} - -export const auth = sharedInstance(SHARED_AUTH_KEY, () => selfHealingAuth(createAuth)); +export const auth = sharedInstance(SHARED_AUTH_KEY, createAuth); export type Session = typeof auth.$Infer.Session; diff --git a/src/test-utils/mock-oidc.ts b/src/test-utils/mock-oidc.ts index dd803ce8..8ab103ee 100644 --- a/src/test-utils/mock-oidc.ts +++ b/src/test-utils/mock-oidc.ts @@ -6,8 +6,8 @@ * * `installMockOidc()` replaces `globalThis.fetch` with a stub that THROWS for * any URL it does not serve, so nothing that uses it can reach a real Ministry - * Platform. It must run before `@/lib/auth` is imported (genericOAuth fetches - * discovery when the auth instance is built), i.e. inside `vi.hoisted`: + * Platform. Run it before `@/lib/auth` is imported, i.e. inside `vi.hoisted`, + * so no module ever captures the real `fetch`: * * const oidc = await vi.hoisted(async () => * (await import('@/test-utils/mock-oidc')).installMockOidc()); @@ -25,8 +25,21 @@ export const MOCK_ISSUER = `${MOCK_MP_BASE}/oauth`; export const MOCK_CLIENT_ID = 'test-client-id'; export const MOCK_SUB = 'dddddddd-dddd-4ddd-8ddd-dddddddddddd'; +/** + * How an MP endpoint behaves: `ok`; `reject` (a network error, like a refused + * connection); `http_500`; `hang` (accepts the request, never answers — it + * settles only when the caller's `AbortSignal` fires). + */ +export type MockEndpointMode = 'ok' | 'reject' | 'http_500' | 'hang'; + /** Everything a test may change between requests. `reset()` restores it. */ export interface MockOidcState { + /** How the discovery endpoint behaves. */ + discovery: MockEndpointMode; + /** How the JWKS endpoint behaves. */ + jwks: MockEndpointMode; + /** When not `ok`, EVERY mock MP endpoint behaves this way (MP down). */ + mp: MockEndpointMode; /** Keys to drop from the discovery document. */ omitDiscovery: string[]; /** Claim overrides for the next id_tokens (an `undefined` value drops the claim). */ @@ -49,6 +62,12 @@ export interface MockOidc { state: MockOidcState; /** Every request the mock served, in order. */ calls: RecordedCall[]; + /** + * An RS256 id_token signed by the key the JWKS publishes (or the foreign key + * when `state.foreignKey`), with the given claim overrides on top of + * `state.claims` (an `undefined` value drops the claim). + */ + signIdToken(claims?: Record): string; reset(): void; } @@ -61,7 +80,16 @@ const json = (body: unknown) => }); function freshState(): MockOidcState { - return { omitDiscovery: [], claims: {}, foreignKey: false, userinfo: {} }; + return { discovery: 'ok', jwks: 'ok', mp: 'ok', omitDiscovery: [], claims: {}, foreignKey: false, userinfo: {} }; +} + +/** A non-`ok` mode's response: rejects, 500s, or waits for the caller's abort. */ +function misbehave(mode: Exclude, signal: AbortSignal | null | undefined) { + if (mode === 'reject') return Promise.reject(new TypeError('fetch failed')); + if (mode === 'http_500') return Promise.resolve(new Response('error', { status: 500 })); + return new Promise((_, reject) => { + signal?.addEventListener('abort', () => reject(signal.reason), { once: true }); + }); } export function installMockOidc(): MockOidc { @@ -75,13 +103,14 @@ export function installMockOidc(): MockOidc { issuer: MOCK_ISSUER, state: freshState(), calls: [], + signIdToken: (claims) => signIdToken(claims), reset() { mock.state = freshState(); mock.calls.length = 0; }, }; - function signIdToken(): string { + function signIdToken(overrides: Record = {}): string { const now = Math.floor(Date.now() / 1000); const claims = JSON.parse( JSON.stringify({ @@ -91,6 +120,7 @@ export function installMockOidc(): MockOidc { iat: now, exp: now + 300, ...mock.state.claims, + ...overrides, }), ) as Record; const input = `${b64({ alg: 'RS256', kid: 'test-key-1', typ: 'JWT' })}.${b64(claims)}`; @@ -103,7 +133,10 @@ export function installMockOidc(): MockOidc { const { url, method } = request; mock.calls.push({ url, method, body: await request.clone().text() }); + const servesUrl = url.startsWith(`${MOCK_ISSUER}/`); + if (servesUrl && mock.state.mp !== 'ok') return misbehave(mock.state.mp, request.signal); if (url === `${MOCK_ISSUER}/.well-known/openid-configuration`) { + if (mock.state.discovery !== 'ok') return misbehave(mock.state.discovery, request.signal); const doc: Record = { issuer: MOCK_ISSUER, authorization_endpoint: `${MOCK_ISSUER}/connect/authorize`, @@ -116,7 +149,10 @@ export function installMockOidc(): MockOidc { for (const key of mock.state.omitDiscovery) delete doc[key]; return json(doc); } - if (url === `${MOCK_ISSUER}/.well-known/openid-configuration/jwks`) return json({ keys: [jwk] }); + if (url === `${MOCK_ISSUER}/.well-known/openid-configuration/jwks`) { + if (mock.state.jwks !== 'ok') return misbehave(mock.state.jwks, request.signal); + return json({ keys: [jwk] }); + } if (url === `${MOCK_ISSUER}/connect/token` && method === 'POST') { return json({ access_token: 'mock-access-token', From 23660fde7dc2e49effff1c8881f92fa942fe3f23 Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Tue, 29 Sep 2026 19:33:00 -0400 Subject: [PATCH 2/2] docs: describe the #101 OIDC change across references, security notes and playbooks - auth.md: config table (no discoveryUrl; explicit endpoints, endSessionEndpoint, accountSubject), new "id_token verification (lazy discovery)" section, nonce section now "do not re-add discoveryUrl" (F8 kept), F12 layers, log events, PROVIDER_NOT_FOUND now means misconfiguration only, upgrade checklist. - testing.md: auth.oidc-discovery.test.ts replaces auth.discovery-rebuild; mock-oidc modes and signIdToken; counts regenerated from a full run. - deps-known-issues.md: deps:relock now writes metadata-only differences; the stale "PKCE can likely be enabled" item records that MP does not support it. - README: the build makes no MP call; an outage fails only the sign-in that hits it. - 2026-09-25 / 2026-09-28 security notes: dated "Update 2026-09-29 (issue #101)" notes; history unchanged. - Downstream playbooks: Phase 4 port rewritten for the #101 approach (code sketch, pitfalls, what to delete if selfHealingAuth was ported, what to keep on a discoveryUrl fork); self-check greps updated; deferred "drop discoveryUrl" and F12-residue items marked done. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../playbooks/port-downstream-hardening.md | 15 +- .../port-security-review-2026-09-28.md | 110 ++++++++-- .claude/references/auth.md | 203 ++++++++++++------ .claude/references/deps-known-issues.md | 11 +- .claude/references/testing.md | 24 +-- README.md | 2 +- docs/security/2026-09-25-signin-hardening.md | 6 + docs/security/2026-09-28-auth-review.md | 4 +- .../security/downstream-hardening-playbook.md | 109 +++++++--- 9 files changed, 348 insertions(+), 136 deletions(-) diff --git a/.claude/playbooks/port-downstream-hardening.md b/.claude/playbooks/port-downstream-hardening.md index e3210151..26f0142e 100644 --- a/.claude/playbooks/port-downstream-hardening.md +++ b/.claude/playbooks/port-downstream-hardening.md @@ -104,7 +104,8 @@ grep -rqF 'startsWith("/\\")' src/ && echo "✗ F3b (weak leading-/\\ check)" || grep -rqF '\u001f' src/components/sign-in/ src/app/signin/ 2>/dev/null && echo "✓ control chars refused" || echo "✗ F3b (no control-char rule)" # F12: is ID-token sign-in refused? (live whenever discoveryUrl is set on better-auth >= 1.7) -grep -q "discoveryUrl" src/lib/auth.ts && ! grep -q "ID_TOKEN_SIGN_IN_DISABLED" src/lib/auth.ts \ +# (`^\s*discoveryUrl:` is the config key; the second grep skips a function parameter of that name) +grep -E "^\s*discoveryUrl:" src/lib/auth.ts | grep -qv "discoveryUrl: string" && ! grep -q "ID_TOKEN_SIGN_IN_DISABLED" src/lib/auth.ts \ && echo "✗ F12 (no hooks.before idToken guard)" || echo "✓ F12 guard (or no discoveryUrl)" grep -q "allowedSignInSocialKeys" "src/app/api/auth/[...all]/route.ts" && echo "✓ F12 route filter" || echo "✗ F12 (route filter)" @@ -396,7 +397,7 @@ Reported privately on 2026-09-25 by Jonathon Huff (The Moody Church). Severity * ### Does this apply here? ```bash -grep -n "discoveryUrl" src/lib/auth.ts # set? the branch is live +grep -nE "^\s*discoveryUrl:" src/lib/auth.ts | grep -v "discoveryUrl: string" # set? the branch is live grep -n "ID_TOKEN_SIGN_IN_DISABLED" src/lib/auth.ts # absent? affected grep '"version"' node_modules/better-auth/package.json # >= 1.7? affected grep -n "allowedSignInSocialKeys" "src/app/api/auth/[...all]/route.ts" # absent? layer (a) missing @@ -416,7 +417,7 @@ Fixed: a plain **404** from the route before better-auth runs (and, with the rou ### What it is -better-auth 1.7's `POST /sign-in/social` has an `idToken` branch: body `{ provider, idToken: { token, accessToken } }` creates a session directly — no `state`, no authorization code, no exchange. It is enabled for a genericOAuth provider whenever that provider has an id-token verification config, which it gets automatically because `auth.ts` sets `discoveryUrl`. **genericOAuth has no option to turn it off.** +better-auth 1.7's `POST /sign-in/social` has an `idToken` branch: body `{ provider, idToken: { token, accessToken } }` creates a session directly — no `state`, no authorization code, no exchange. It is enabled for a genericOAuth provider whenever that provider has an id-token verification config, which it gets automatically when `auth.ts` sets `discoveryUrl`. **genericOAuth has no option to turn it off.** Upstream `main` stopped setting `discoveryUrl` in issue #101, so better-auth refuses the branch itself there (`ID_TOKEN_NOT_SUPPORTED`). The layers below are still ported as the primary control. better-auth verifies the id_token (signature, issuer, audience = `OIDC_CLIENT_ID`), then calls **our** `getUserInfo` with the **caller-supplied** `accessToken`. Identity — `sub` → `userGuid` — comes from MP's `/connect/userinfo` for that access token. **Nothing binds `id_token.sub` to `userinfo.sub`.** So the attacker's *own* valid id_token for this client, plus a victim's MP access token from **any** MP client that `/connect/userinfo` accepts, yields an app session **as the victim** — their roles on every authorization check, their `User_ID` on every write. Reproduced upstream against a mock. @@ -477,7 +478,7 @@ The subset rule also refuses keys the app never sends — `scopes`, `errorCallba Why all three: (a) is HTTP-only and in-process `auth.api` calls skip it; (b) depends on better-auth keeping the key named `idToken`; (c) is what still holds if a future better-auth path reaches `getUserInfo` with caller-supplied tokens. -**Do not drop `discoveryUrl` as a drive-by.** It would remove the id-token config and with it the branch — and the dependence on discovery at boot — but it loses JWKS verification of the normal flow's id_token (upstream now requires it: `requireIdTokenVerification: true`), and since 1.7 reads `profile.id` for non-OIDC providers it needs an `accountSubject` mapping. Upstream deferred it as a separate trade-off; raise it with the user rather than deciding. +**Do not drop `discoveryUrl` as a drive-by.** Dropped on its own, it loses genericOAuth's JWKS verification of the normal flow's id_token, and it breaks account identity without an `accountSubject` mapping (since 1.7 the default reads `profile.id` for a non-OIDC provider). Upstream **did** drop it in issue #101 (2026-09-29), as a complete change: explicit endpoints including `endSessionEndpoint`, `accountSubject` → `sub`, and the id_token verified in `getUserInfo` against a lazily loaded JWKS. Port that as a whole, from `port-security-review-2026-09-28.md` Phase 4, and only after asking the user. Keep the three layers above either way. ### Tests, and the mutations that must fail them @@ -678,13 +679,13 @@ Not security findings, but both cost hours upstream and both are inherited code. Symptom: `/auth-error?error=unable_to_get_user_info`, with `id_token failed verification against the discovery JWKS or expected nonce`. -better-auth 1.7 turns nonce binding on automatically for any provider whose discovery document yields an id_token config, sends a `nonce` on the authorize request, then requires the claim to come back — `nonceMatches` returns false when the claim is absent. **MP omits it.** So: +better-auth 1.7 turns nonce binding on automatically for any provider whose discovery document yields an id_token config, sends a `nonce` on the authorize request, then requires the claim to come back — `nonceMatches` returns false when the claim is absent. **MP omits it.** So, while this repo's config sets `discoveryUrl`: ```ts disableIdTokenNonceBinding: true, ``` -What made this look intermittent is inverted from the obvious reading: on better-auth before 1.7.3, **sign-in succeeded only when the boot-time discovery fetch had failed**, because that left the id_token config undefined and skipped verification altogether. A *working* discovery meant a *broken* sign-in. On 1.7.4 a failed discovery skips the provider instead (sign-in 404s `PROVIDER_NOT_FOUND`); upstream also sets `requireIdTokenVerification: true` so a discovery document missing `issuer` or `jwks_uri` is refused rather than silently skipping verification, and rebuilds the `auth` instance on the next sign-in after a 30 s cooldown so one failed discovery does not last for the life of the process. +What made this look intermittent is inverted from the obvious reading: on better-auth before 1.7.3, **sign-in succeeded only when the boot-time discovery fetch had failed**, because that left the id_token config undefined and skipped verification altogether. A *working* discovery meant a *broken* sign-in. On 1.7.4 a failed discovery skips the provider instead (sign-in 404s `PROVIDER_NOT_FOUND`). On a `discoveryUrl` config, keep `requireIdTokenVerification: true` as well, so that a discovery document missing `issuer` or `jwks_uri` is refused rather than silently skipping verification. Upstream has since removed boot-time discovery (issue #101): no `discoveryUrl`, so no nonce binding and neither option. See `port-security-review-2026-09-28.md` Phase 4. What you give up: binding the id_token to this particular authorization request. Signature, issuer and audience are still verified against MP's JWKS. The `state` cookie check and the client secret do **not** cover the gap, and MP offers no PKCE — see Phase 11: this is an accepted risk. @@ -835,7 +836,7 @@ Carry these forward as known risk; don't present them as closed. - **No CSP reporting endpoint** (decided upstream 2026-09-29). - `/_not-found` and `/_global-error` are prerendered and therefore nonce-less. Accepted upstream: neither can opt out; `/_global-error` recovers via a plain link. - An MP timeout during a role lookup fails after 20 s to the error boundary; no retry/backoff on that path. -- **F12 residue** — genericOAuth still has no usable switch for the `idToken` branch; the fix *refuses* it rather than removing it. better-auth's `disableIdTokenSignIn` is **not** an alternative: the code flow calls the same `verifyProviderIdToken`, which returns `false` when that flag is set, so it breaks every sign-in. Re-check the hook and the route's key list on every better-auth upgrade. Dropping `discoveryUrl` (which would remove the branch, at the cost of JWKS verification) is deferred as a separate trade-off. +- **F12 residue** — genericOAuth still has no usable switch for the `idToken` branch; the fix *refuses* it rather than removing it. better-auth's `disableIdTokenSignIn` is **not** an alternative: the code flow calls the same `verifyProviderIdToken`, which returns `false` when that flag is set, so it breaks every sign-in. Re-check the hook and the route's key list on every better-auth upgrade. **Upstream dropped `discoveryUrl` in issue #101** (2026-09-29) and kept JWKS verification by doing it in `getUserInfo`, so the branch is refused by better-auth there too. The hook stays, because re-adding `discoveryUrl` would switch the branch back on. - better-auth's own logger logs callback `error`/`state`/`iss`/`callbackURL` values verbatim (accepted upstream). Upstream's 2026-09-28/29 review follow-up — what changed, and what is **breaking for forks** (`firstName`/`lastName` gone from the session, trusted sender for communications, deny-all stored procedures, `CurrentUserProfile`, required https `BETTER_AUTH_URL`, `server-only`, `--conditions=react-server` for the generators, one-time `session_data` invalidation) — is summarised in `docs/security/downstream-hardening-playbook.md` § 2026-09-29 follow-up. Port it with **`.claude/playbooks/port-security-review-2026-09-28.md`** after the above, not mixed into them. diff --git a/.claude/playbooks/port-security-review-2026-09-28.md b/.claude/playbooks/port-security-review-2026-09-28.md index 8999c9da..7920e769 100644 --- a/.claude/playbooks/port-security-review-2026-09-28.md +++ b/.claude/playbooks/port-security-review-2026-09-28.md @@ -10,7 +10,7 @@ You are Claude Code running in a repo that was **forked or copied from MPNext** 1. The app **refuses to boot** on a forgeable auth configuration: no secret, better-auth's public default secret, a secret shorter than 32 characters, `BETTER_AUTH_SECRETS`, `TEST` set in production, an unset or non-https `BETTER_AUTH_URL`, or a non-https MP URL. 2. Sessions have a **hard 12 h ceiling**. A copied or forged cookie that no live server row backs dies within 1 h. `session_data` is encrypted (JWE). The user's MP OAuth tokens are held neither in a cookie nor in memory. -3. There is **one `auth` instance per process** across Next's bundle layers, so sign-out deletes the row that `/get-session` reads and the MP logout carries `id_token_hint`. A failed OIDC discovery at boot heals on the next sign-in instead of lasting until a restart. +3. There is **one `auth` instance per process** across Next's bundle layers, so sign-out deletes the row that `/get-session` reads and the MP logout carries `id_token_hint`. Building that instance makes **no MP call** (explicit OIDC endpoints; the id_token verifier loads discovery lazily at the first callback), so an MP blip at boot can neither take sign-in down nor hang every request. 4. **The role gate fails closed**: with `MP_SECURITY_ROLES` unset, nobody gets in. `*` is an explicit opt-in. 5. **Every MP fetch has a timeout and refuses redirects.** The service-account token is refreshed single-flight and validated. A 401 refreshes and retries once. No endpoint path can leave the API root. 6. **Provider services validate every path segment.** Stored procedures are deny-all unless allowlisted per instance. Communications take a server-built trusted sender. @@ -48,7 +48,7 @@ Each one changes a contract that fork-specific code may depend on. Grep for ever Answer these by reading the repo. Track the answers, and only proceed once each has an answer or has been raised with the user. 1. **Has the 2026-09-12/25 playbook been ported?** Run its triage block (the start of `port-downstream-hardening.md` Phase 1). Any `✗` there means stop and port that playbook first. -2. **better-auth version** (`npm ls better-auth`). Upstream is 1.7.4, and several items below cite 1.7.4 internals (`refreshCache` defaulting on in stateless mode, `requireIdTokenVerification`, `DEFAULT_SECRET`). On a different minor, re-verify each cited behaviour in `node_modules/better-auth/dist/` before you port it. Don't carry the claim over unchecked. +2. **better-auth version** (`npm ls better-auth`). Upstream is 1.7.4, and several items below cite 1.7.4 internals (`refreshCache` defaulting on in stateless mode, genericOAuth's one-shot discovery in `init`, `DEFAULT_SECRET`). On a different minor, re-verify each cited behaviour in `node_modules/better-auth/dist/` before you port it. Don't carry the claim over unchecked. 3. **Stateless or database-backed sessions?** Upstream has no database: an encrypted cookie cache backed by better-auth's per-process memory adapter. With a real adapter, Phases 3 and 4 change shape (revocation becomes possible, and the shared-instance fix matters less). **Ask the user** before porting those phases onto a database-backed fork. 4. **Host.** Vercel, Azure, Cloudflare in front, a bare VM? It decides the rate-limit IP header in Phase 6, and whether the per-process caveats in Phase 4 apply. 5. **Run the triage script.** Every `✗` is work. A `✓` means the grep passed; still read the phase for what a grep cannot see. @@ -68,13 +68,17 @@ grep -q 'strategy: "jwe"' src/lib/auth.ts && echo "✓ JWE cookie cache" || echo grep -q "storeAccountCookie: false" src/lib/auth.ts && echo "✓ no account cookie" || echo "✗ P3 user OAuth tokens in a cookie" grep -q "WITHHELD_SESSION_FIELDS\|ipAddress.*userAgent" src/lib/auth.ts && echo "✓ session fields withheld" || echo "✗ P3 token/ip/UA in /get-session" -# Phase 4: one instance, discovery rebuild, logout hint +# Phase 4: one instance, no boot-time discovery (issue #101), logout hint grep -q "globalThis" src/lib/auth.ts && echo "✓ shared instance" || echo "✗ P4 one auth per bundle layer" -grep -q "selfHealingAuth\|discovery.rebuild" src/lib/auth.ts && echo "✓ discovery rebuild" || echo "✗ P4 discovery failure lasts until restart" +# (`^\s*discoveryUrl:` is the config key; the second grep skips a function parameter of that name) +grep -E "^\s*discoveryUrl:" src/lib/auth.ts | grep -qv "discoveryUrl: string" && echo "✗ P4 discovery fetched at boot (one MP blip = sign-in down / requests hang)" || echo "✓ no boot-time discovery" +grep -q "selfHealingAuth" src/lib/auth.ts && echo "✗ P4 rebuild facade still present (delete it with the #101 port)" || echo "✓ no rebuild facade" +grep -q "endSessionEndpoint" src/lib/auth.ts && echo "✓ explicit end-session endpoint" || echo "✗ P4 endSessionEndpoint (needed once discoveryUrl is gone)" grep -rq "id_token_hint" src/components/ && echo "✓ id_token_hint on logout" || echo "✗ P4 logout hint" -# Phase 5: OIDC -grep -q "requireIdTokenVerification: true" src/lib/auth.ts && echo "✓ id_token verification required" || echo "✗ P5 requireIdTokenVerification" +# Phase 5: OIDC — the id_token must be verified by something +grep -q "verifyMpIdToken" src/lib/auth.ts && echo "✓ id_token verified in getUserInfo" \ + || { grep -q "requireIdTokenVerification: true" src/lib/auth.ts && echo "✓ id_token verified by genericOAuth (discoveryUrl fork)" || echo "✗ P5 id_token verification not guaranteed"; } grep -q "azp" src/lib/auth.ts && echo "✓ exp/azp checks" || echo "✗ P5 exp/azp" grep -q '"offline_access"' src/lib/auth.ts && echo "✗ P5 offline_access requested" || echo "✓ no offline_access" @@ -187,15 +191,19 @@ In `enrichSessionUser` (the `customSession` callback), return the user plus `use --- -## Phase 4: One `auth` instance per process, self-healing discovery, logout hint +## Phase 4: One `auth` instance per process, no boot-time discovery, logout hint **Why: the instance.** Next loads `src/lib/auth.ts` once **per bundle layer**. Upstream measured 4 copies under `next dev` and 2 in a production build. Each copy built its own `betterAuth()` with its own memory adapter. The OAuth callback wrote the session and account rows in the route-handler copy, while the sign-out server action ran in another copy where those rows did not exist. Two consequences followed: - Sign-out never deleted the row `/get-session` reads, so a copied cookie pair outlived sign-out up to the 12 h cap. - better-auth found no id_token, so the MP logout URL had no `id_token_hint`, and MP stopped at a "log out?" prompt. On a shared PC whose tab is then closed, the **MP SSO session stays alive**. -**Why: discovery.** genericOAuth fetches MP's discovery document **once**, with no retry. In 1.7.4 a failed fetch skips the provider, so `/sign-in/social` returns `404 PROVIDER_NOT_FOUND` **until the process restarts**. That fails closed, but it is an outage caused by one cold-start blip. +**Why: discovery (issue #101).** With `discoveryUrl` set, genericOAuth fetches MP's discovery document **once**, inside the auth context's `init`, with no timeout and no retry. Every auth request awaits that `init`, and so does every in-process `auth.api.*` call, which means every page's session check. The results: +- A **failed** fetch skips the provider, so `/sign-in/social` returns `404 PROVIDER_NOT_FOUND` until a restart. `/signin` spins with no error, and an uptime check stays green. +- A **hung** fetch stalls every request for undici's ~300 s headers timeout. -**Port.** +Upstream first shipped a self-healing rebuild facade (`selfHealingAuth`). It healed the failed case after a 30 s cooldown but not the hang. It was then replaced by removing boot-time discovery altogether. + +**Port: one instance.** ```ts export const SHARED_AUTH_KEY = Symbol.for("mpnext.auth"); @@ -204,16 +212,80 @@ export function sharedInstance(key: symbol, create: () => T, env = process.en const store = globalThis as unknown as Record; return (store[key] ??= create()); } -export const auth = sharedInstance(SHARED_AUTH_KEY, () => selfHealingAuth(createAuth)); +export const auth = sharedInstance(SHARED_AUTH_KEY, createAuth); +``` + +**Port: no boot-time discovery.** Configure the endpoints explicitly and verify the id_token yourself in `getUserInfo`, against an issuer and JWKS loaded from discovery **lazily**. `jose` becomes a direct dependency (it is already in the tree via better-auth; add it with `npm run deps:relock`). + +```ts +import { createRemoteJWKSet, jwtVerify, type JWTPayload, type JWTVerifyGetKey } from "jose"; + +const mpOidc = { + discovery: `${mpBaseUrl}/oauth/.well-known/openid-configuration`, + authorization: `${mpBaseUrl}/oauth/connect/authorize`, + token: `${mpBaseUrl}/oauth/connect/token`, + userinfo: `${mpBaseUrl}/oauth/connect/userinfo`, + endSession: `${mpBaseUrl}/oauth/connect/endsession`, +}; + +// Fetched on first use; single-flight; cached on success; never cached on failure. +export function lazyIdTokenVerifier(discoveryUrl: string, { timeoutMs = 5_000 } = {}) { + let pending: Promise<{ issuer: string; jwks: JWTVerifyGetKey }> | null = null; + return () => + (pending ??= fetchIdTokenVerifier(discoveryUrl, timeoutMs).catch((err) => { + pending = null; + throw err; + })); +} +// fetchIdTokenVerifier: fetch(discoveryUrl, { signal: AbortSignal.timeout(timeoutMs), redirect: "error" }); +// require a URL-parseable `issuer` and `jwks_uri` (fail closed otherwise, logging only a reason); +// return { issuer, jwks: createRemoteJWKSet(new URL(jwks_uri, discoveryUrl), { timeoutDuration: timeoutMs }) }. + +const loadMpIdTokenVerifier = lazyIdTokenVerifier(mpOidc.discovery); + +async function verifyMpIdToken(idToken: string): Promise { + // loader failure → log { reason: "verifier_unavailable" }, return null + // jwtVerify(idToken, jwks, { issuer, audience: OIDC_CLIENT_ID, algorithms: ["RS256"] }) + // failure → log { reason: "verification_failed", code, claim, errName }, return null +} + +genericOAuth({ config: [{ + providerId: "ministry-platform", + // NO discoveryUrl, NO requireIdTokenVerification, NO disableIdTokenNonceBinding + authorizationUrl: mpOidc.authorization, + tokenUrl: mpOidc.token, + endSessionEndpoint: mpOidc.endSession, + accountSubject: ({ profile }) => (typeof profile.sub === "string" ? profile.sub : ""), + // ...pkce: false, scopes, getUserInfo: missing id_token → null; claims = await verifyMpIdToken(...); + // then the existing sub / exp / azp / userinfo binding checks on `claims` +}] }); ``` -`selfHealingAuth(create)` returns a `Proxy` whose `handler` checks, **only for `/api/auth/sign-in/social` and `/api/auth/callback/*`**, whether the current instance has the MP provider (`(await instance.$context).socialProviders`). If it doesn't, the proxy rebuilds: single-flight, at most one build per 30 s counting the first, and a request waits at most 10 s. It swaps the new instance in **only if it has the provider**, and logs `auth.discovery.rebuild` with `outcome` and never the URL. An instance that has the provider is never rebuilt, so live sessions are never thrown away. Every other property reads through to the current instance at access time, so **never cache `auth.api` in a module-level variable**. +Pitfalls, each of which upstream hit or verified: +- **Do not pin the JWKS URL.** It differs between IdentityServers: a live MP serves `/oauth/.well-known/jwks`, while the IdentityServer default is `/oauth/.well-known/openid-configuration/jwks`. Take it from `jwks_uri`. +- **`endSessionEndpoint` is required.** Without discovery, better-auth has no MP logout URL, and sign-out loses the `id_token_hint` read below. +- **`accountSubject` must read `sub`.** Without discovery the provider is not recognised as OIDC, so the default resolver reads `profile.id`, and every account would be keyed on `""`. +- **Remove `requireIdTokenVerification`.** Without `discoveryUrl`, genericOAuth *throws* for it at init. The new verifier fails closed on a partial document itself. `disableIdTokenNonceBinding` becomes a no-op: with no id_token config, better-auth sends and requires no nonce. +- **RS256 only.** Pin the algorithm list; don't take it from the token header or from discovery. +- **Tests that call `getUserInfo` directly** with unsigned fake id_tokens now need genuinely signed ones, plus a `fetch` stub that replaces only the userinfo response, since discovery and the JWKS still have to answer. Suites that verify with jose need `// @vitest-environment node`. +- **The F12 `idToken` branch goes away at the source**, because the provider has no id_token config and better-auth returns `404 ID_TOKEN_NOT_SUPPORTED`. Keep the `refuseIdTokenSignIn` hook and the route filter anyway, since re-adding `discoveryUrl` would switch the branch back on. + +**If this repo already ported `selfHealingAuth`,** delete it together with everything that existed only for it: `DISCOVERY_REBUILD_COOLDOWN_MS`, `DISCOVERY_REBUILD_WAIT_MS`, `RebuildableAuth`, `needsProvider`, `hasProvider`, `logDiscoveryRebuild` and `src/auth.discovery-rebuild.test.ts`. Keep `MP_PROVIDER_ID` if it is used as the `providerId`. Then export `sharedInstance(SHARED_AUTH_KEY, createAuth)` directly. Tell the user that the `auth.discovery.rebuild` log event is replaced by `auth.oidc.discovery_failed` and `auth.userinfo.id_token_unverified`. + +**If this repo keeps `discoveryUrl`** (the user declines the port), keep `requireIdTokenVerification: true` and `disableIdTokenNonceBinding: true`. Know that a hung discovery at boot still stalls every request. **Sign-out** (`src/components/user-menu/actions.ts`): call `auth.api.signOut({ headers, body: { disableRedirect: true } })` first, so the app session is cleared even if the env below is broken. Take the `id_token_hint` out of the returned `url`, trusting it **only if its origin is the MP origin**. Rebuild the end-session URL yourself, with `post_logout_redirect_uri` set to exactly the registered value (`getAuthBaseUrl()`), `client_id` always, and `id_token_hint` when available. No localhost fallback. **Caveat to tell the user.** This is per *process*. On serverless, a sign-out that lands on another instance has no id_token, so MP prompts once. That is documented upstream as accepted. -**Tests.** `src/auth.shared-instance.test.ts` signs in through one module copy and signs out through another, with a negative control. It fails when the cache is removed. `src/auth.discovery-rebuild.test.ts` covers cooldown, single-flight, wait cap, swap-only-on-success and that non-sign-in paths never rebuild. +**Tests.** `src/auth.shared-instance.test.ts` signs in through one module copy and signs out through another, with a negative control. It fails when the cache is removed. `src/auth.oidc-discovery.test.ts` covers issue #101: +- MP rejecting, 500ing or hanging at boot, with no MP call before the callback; +- discovery or JWKS failing at the callback, refused, and then recovering on the same instance; +- the discovery timeout and `redirect: "error"`; +- single-flight and caching; +- `lazyIdTokenVerifier` unit cases. + +A mutation that re-adds `discoveryUrl` turns the boot and recovery cases red; the hang case fails by timeout. --- @@ -221,12 +293,12 @@ export const auth = sharedInstance(SHARED_AUTH_KEY, () => selfHealingAuth(create **Port** in the genericOAuth provider config and `getUserInfo`: -1. **`requireIdTokenVerification: true`.** genericOAuth builds its id_token verifier only when discovery yields both `issuer` and `jwks_uri`. Without this option, a partial discovery document left the provider live with **verification silently off**. With it, that provider is skipped and an error is logged. -2. **`exp` and `azp` checks.** better-auth hands jose only `issuer` and `audience`, and jose checks `exp` only when it is present. In `getUserInfo`, decode the already-verified id_token (hand-rolled base64url decode, because `jose` is only a transitive dependency) and refuse when `exp` is missing, not finite or in the past, or when `aud` is an array and `azp !== OIDC_CLIENT_ID`. Log `auth.userinfo.id_token_claims_invalid` with `reason` only. +1. **The id_token is always verified.** Upstream's current code does this with `verifyMpIdToken` in `getUserInfo`, as ported in Phase 4 (issue #101). Its loader fails closed on a discovery document without a usable `issuer` or `jwks_uri`. On a fork that keeps `discoveryUrl`, set **`requireIdTokenVerification: true`** instead. genericOAuth builds its verifier only when discovery yields both `issuer` and `jwks_uri`, and without the option a partial document left the provider live with **verification silently off**. +2. **`exp` and `azp` checks.** jose checks `exp` only when it is present (and refuses a past or non-numeric one itself), and nothing checks `azp`. In `getUserInfo`, on the **verified** payload, refuse when `exp` is missing, or when `aud` is an array and `azp !== OIDC_CLIENT_ID`. Log `auth.userinfo.id_token_claims_invalid` with `reason` only. (A fork still on `discoveryUrl` decodes the already-verified token instead, and must also refuse a past `exp` itself.) 3. **The userinfo fetch never throws.** Use `AbortSignal.timeout(10_000)` and `redirect: "error"`, because a followed redirect would re-send the user's bearer. A non-2xx status, a network error, invalid JSON or a non-object body each **return `null`** and log `auth.userinfo.fetch_failed` with a status or `errName`. Returning `null` is better-auth's contract for "unusable". A throw from `getUserInfo` is **not** equivalent: the callback route doesn't wrap it, so it surfaces as an unhandled error. 4. **Display name.** Build `name` only from `given_name`/`family_name`/`name` claims that are actually strings (`profileDisplayName`). Never interpolate a missing claim. 5. **Scopes.** Use `openid` plus the MP data scope, with **no `offline_access`**. The app never refreshes the user's token. (Upstream decided against narrowing further, because forks need the broad scope.) -6. Keep `pkce: false` and `disableIdTokenNonceBinding: true`. MP supports neither PKCE nor the nonce. F8 is an accepted risk; see `port-downstream-hardening.md` Phase 11. +6. Keep `pkce: false`. MP supports neither PKCE nor the nonce. Without `discoveryUrl`, nonce binding is off structurally. A fork that keeps `discoveryUrl` must keep `disableIdTokenNonceBinding: true` as well. F8 is an accepted risk; see `port-downstream-hardening.md` Phase 11. **Tests.** Upstream `src/test-utils/mock-oidc.ts` is a mock OIDC provider (discovery, JWKS, token, userinfo) that drives the **real** code flow end to end. `src/auth.code-flow.test.ts`, `src/auth.oidc-hardening.test.ts` and `src/auth.origin-check.test.ts` run on it. Mutants (origin check off, `trustedOrigins: ["*"]`, strategy, `expiresIn`, `refreshCache`, sub binding, nonce, PKCE, account cookie) each turn a test red. Port the harness first. It makes every other auth phase testable. @@ -390,7 +462,7 @@ Add tests with a **Flight-shaped** promise (`then` returning `undefined`) for bo ## Phase 15: CI and setup 1. **Pin every action by full commit SHA**, with a version comment. Resolve each SHA from the tag via the GitHub API; don't copy SHAs from here. Add `permissions: contents: read` at workflow level, `persist-credentials: false` on checkout, and a `.github/dependabot.yml` for `github-actions` (weekly, grouped). -2. **Jobs**: `lint` (`npm run lint` + `npx tsc --noEmit`); `build` (`npm run build` with dummy env, then a prerender check); `test` (with coverage); and `lockfile` if the fork has one. In the dummy build env, point the MP URL at the reserved, non-resolving `https://mp.invalid`, so OIDC discovery during "Collecting page data" fails harmlessly and **never reaches a real MP**. +2. **Jobs**: `lint` (`npm run lint` + `npx tsc --noEmit`); `build` (`npm run build` with dummy env, then a prerender check); `test` (with coverage); and `lockfile` if the fork has one. In the dummy build env, point the MP URL at the reserved, non-resolving `https://mp.invalid`, so nothing during "Collecting page data" can reach a real MP. (Upstream makes no MP call at build at all since issue #101; a fork that still sets `discoveryUrl` fetches discovery there, and it fails harmlessly.) 3. **Prerender check** (`scripts/check-prerender.mjs`). Read `.next/prerender-manifest.json` and fail if any route other than `/_not-found` and `/_global-error` was prerendered. A static page carries no CSP nonce and never hydrates under the enforced CSP. Every new page that doesn't read `headers()` needs `export const dynamic = "force-dynamic"`. 4. **Setup script** (if the fork kept `npm run setup`). Write `.env.local` values quoted, with `$` escaped as `\$`, so `@next/env`'s dotenv-expand returns them unchanged. Use a replacer *function* in line replacement, because `$'`, `` $` `` and `$&` in a value otherwise copy other parts of the file, secrets included. Write with mode `0600`. Enforce a hand-entered secret of ≥ 32 characters that isn't the public default, judged the way Next's `loadEnvConfig` will read it back. Use `npm ci`, never `npm install` or `npm update`. Recommend a **dedicated** MP OIDC client (Authorization Code only, exact redirect URIs) separate from the Client Credentials data client, and remove any "enable Implicit/Hybrid" guidance. 5. Run the `scripts/` tests in CI as a second Vitest project, outside the `src/` coverage denominator. @@ -450,8 +522,8 @@ The PR description must call out: - [ ] Boot guards for the secret, `TEST`, `BETTER_AUTH_SECRETS` and both URLs, each tested, with no value in any message. - [ ] `.env*` ignored and blocked in pre-commit. No tracked env or personal settings files. `SECURITY.md` points at private reporting. - [ ] 12 h / no-slide / 1 h / JWE / `refreshCache: false` / no account cookie / tokens stripped / session fields withheld / negative-cached `User_ID`. -- [ ] One `auth` per process, self-healing discovery, and logout with `client_id` + `id_token_hint`. -- [ ] `requireIdTokenVerification`, `exp`/`azp` checks, a userinfo fetch that never throws, no `offline_access`. +- [ ] One `auth` per process, no boot-time discovery (explicit endpoints, lazy id_token verifier, `endSessionEndpoint`), and logout with `client_id` + `id_token_hint`. +- [ ] The id_token is always verified (`verifyMpIdToken`, or `requireIdTokenVerification` on a `discoveryUrl` fork), `exp`/`azp` checks, a userinfo fetch that never throws, no `offline_access`. - [ ] `/sign-in/social` raw Content-Type check, 4 KB body cap, 2048-character `callbackURL` limit, `no-store` everywhere, validated rate-limit IP config. - [ ] Role gate fails closed, with `*` as an explicit opt-in. - [ ] MP client: timeouts, no redirects, path guard, validated single-flight token, 401 retry, name-only logs. @@ -489,7 +561,7 @@ git show 9e8ef87 6f1d6fc # P2 .env* ignore + pre-commit, SECURITY.md; settings git show 6616310 2227a80 # P2 env.ts URL validation; loopback http in production git show a424953 f2af96d # P3/P5 user OAuth tokens; OIDC provider, userinfo, JWE, session fields git show 0e2652e 10ef3df # P4 shared instance; logout client_id + id_token_hint -git show 5e1d2cc # P4 discovery rebuild +git show 5e1d2cc # P4 discovery rebuild (superseded: see the issue #101 PR, which removes it) git show 48a871b 96ab2f5 # P6 4 KB body cap; raw Content-Type check git show 721d6e5 0f61f54 # P6 no-store; rate-limit IP git show 2801d11 8370bcb # P7 fail-closed roles; private self-sanitizing role read diff --git a/.claude/references/auth.md b/.claude/references/auth.md index 01878378..e0a86b74 100644 --- a/.claude/references/auth.md +++ b/.claude/references/auth.md @@ -179,9 +179,10 @@ Origin/callbackURL check off either (left undefined, better-auth uses `src/auth.secret-guard.test.ts` clears it to prove the import throws. `next build` imports this module, so a build environment needs a real secret too — and a valid `MINISTRY_PLATFORM_BASE_URL` / `BETTER_AUTH_URL` (see -[Environment Variables](#environment-variables)). Building the instance also -runs OIDC discovery (a read-only GET, once per build worker), so a `next build` -with a real `.env.local` contacts the real MP discovery endpoint. +[Environment Variables](#environment-variables)). Building the instance makes +no network call (since issue #101 the provider has explicit endpoints and +discovery is fetched only when the first OAuth callback verifies an id_token), +so `next build` never contacts MP. The guard enforces length only. better-auth's own entropy estimate still only *warns* on a low-entropy secret, so generate it (`openssl rand -base64 32`; @@ -299,13 +300,14 @@ showing one client locking out another when unconfigured. | Setting | Value | Notes | |---------|-------|-------| | `providerId` | `"ministry-platform"` | Used in OAuth URLs and `signIn.social({ provider })` | -| `discoveryUrl` | `${MP_BASE_URL}/oauth/.well-known/openid-configuration` | OIDC auto-discovery | -| `requireIdTokenVerification` | `true` | Guarantees the code-flow id_token is verified: genericOAuth builds its verifier only when discovery yields both `issuer` and `jwks_uri`, and without this a partial discovery document left the provider live with verification silently off. With it, such a provider is skipped (sign-in 404s `PROVIDER_NOT_FOUND`). Pinned by `src/auth.oidc-hardening.test.ts` | +| `discoveryUrl` | **Not set — deliberately** (issue #101) | genericOAuth fetched it once, inside the auth context's `init`, with no timeout or retry, and every request awaited it: one MP blip dropped the provider (`404 PROVIDER_NOT_FOUND`) or hung every request ~300 s. Re-adding it also gives the provider an id_token config, which re-opens `/sign-in/social`'s id_token mode. Pinned by `src/auth.code-flow.test.ts` and `src/auth.oidc-discovery.test.ts` | +| `authorizationUrl` / `tokenUrl` | `${MP_BASE_URL}/oauth/connect/authorize` / `…/connect/token` | IdentityServer's fixed `connect/*` paths (checked against a live MP 2026-09-29). Building the instance makes no MP call | +| `endSessionEndpoint` | `${MP_BASE_URL}/oauth/connect/endsession` | **Required** for RP-initiated logout: without it better-auth builds no MP logout URL and sign-out loses `id_token_hint` | +| `accountSubject` | `({ profile }) => profile.sub` (`""` if not a string) | The account subject is the MP User_GUID. Without discovery the provider is not recognised as OIDC, and the default resolver would read `profile.id`, which `getUserInfo` never returns | | `scopes` | `openid`, `http://www.thinkministry.com/dataplatform/scopes/all` | The second scope is the literal URI MP expects, not a short name. No `offline_access`: no refresh token is ever used. **The broad scope is kept deliberately (decision 2026-09-29):** MPNext is a template, and forks build features that need it. The user's token is minimised instead — no `offline_access`, no account cookie, access/refresh tokens blanked in memory (see Session Strategy). Whether userinfo works with a narrower scope is unverified (needs a non-production MP) | | `pkce` | `false` | **Required** — MP does not support PKCE, and 1.7 defaults this to `true`. Not a pending follow-up (see [`nonce` binding is off](#nonce-binding-is-off-and-must-stay-off) for the consequence) | -| `disableIdTokenNonceBinding` | `true` | **Required.** MP does not echo `nonce` back in the `id_token`, and better-auth rejects a missing claim. See [`nonce` binding is off](#nonce-binding-is-off-and-must-stay-off) | | `authorizationUrlParams` | `{ realm: "realm" }` | Extra query parameter MP's authorize endpoint expects | -| `getUserInfo` | Custom callback | Requires an id_token and binds it: the userinfo `sub` must equal the id_token `sub` (case-insensitive), else `null` + `auth.userinfo.sub_mismatch` — see [id_token sign-in is disabled](#id_token-sign-in-is-disabled-sign-insocial). Fetches OIDC userinfo; validates `sub` with `sanitizeGuid` and returns `null` (sign-in refused) if unusable; returns the real `email` on the raw profile and `emailVerified: profile.email_verified === true` (not hardcoded — see [Account linking](#account-linking-disabled)) | +| `getUserInfo` | Custom callback | Requires an id_token and **verifies it** (`verifyMpIdToken`: jose `jwtVerify`, RS256 only, `iss`/`aud` against the lazily discovered issuer and our client id — see [id_token verification](#id_token-verification-lazy-discovery)), else `null` + `auth.userinfo.id_token_unverified`. Then binds it: the userinfo `sub` must equal the id_token `sub` (case-insensitive), else `null` + `auth.userinfo.sub_mismatch` — see [id_token sign-in is disabled](#id_token-sign-in-is-disabled-sign-insocial). Fetches OIDC userinfo; validates `sub` with `sanitizeGuid` and returns `null` (sign-in refused) if unusable; returns the real `email` on the raw profile and `emailVerified: profile.email_verified === true` (not hardcoded — see [Account linking](#account-linking-disabled)) | | `mapProfileToUser` | Custom callback | Returns `userGuid: sub`, `email: @mp.invalid` (synthetic — see [Email is never a key](#email-is-never-a-key-synthetic-email-real-address-in-mpemail)), `mpEmail: real address or null`; throws if `sub` is absent | ### Better Auth 1.7 migration notes @@ -322,15 +324,15 @@ way it does: | Account identity keyed on `(issuer, accountId)` | **Reverted in 1.7.3** — back to `(providerId, accountId)`, and `accountIssuer` was removed. See [Version Notes](#173--account-identity-reverted-breaking). | | Account subject no longer falls back to `id` | `getUserInfo` returns `sub` (see below) | | `pkce` defaults to `true` | Kept explicitly `false`: MP does not support PKCE (its discovery document lists `S256`, but that does not mean it works). Accepted risk F8 — see below | -| ID tokens verified against provider JWKS | Automatic — MP publishes `jwks_uri`. It also turns on `nonce` binding, which MP cannot satisfy, so `disableIdTokenNonceBinding: true` is set (below) | +| ID tokens verified against provider JWKS | Only for a provider with a discovered id_token config — which this one no longer has (issue #101). The app verifies the id_token itself in `getUserInfo` (below). While discovery was configured it also turned on `nonce` binding, which MP cannot satisfy | -> ⚠️ **`getUserInfo` must return `sub`, not `id`.** MP's discovery document -> advertises `id_token_signing_alg_values_supported`, so Better Auth treats the -> provider as OIDC and its default `accountSubject` resolver reads -> **`profile.sub`** off the raw profile. Pre-1.7 the resolver fell back to -> `profile.id`; that fallback is gone. Returning only `id` resolves the account -> subject to `""` and breaks account identity for every user. `src/auth.test.ts` -> guards this by calling the real configured `getUserInfo`. +> ⚠️ **`getUserInfo` must return `sub`, not `id`.** The configured +> `accountSubject` resolver reads **`profile.sub`** off the raw profile. +> (genericOAuth's default reads `sub` only for a provider it recognised as OIDC +> from a discovery document, and `profile.id` otherwise; pre-1.7 it fell back to +> `id` either way.) Returning only `id` resolves the account subject to `""` and +> breaks account identity for every user. `src/auth.test.ts` guards this by +> calling the real configured `getUserInfo` and `accountSubject`. > ⚠️ **`accountIssuer` is gone — do not re-add it.** The advice above held only > for 1.7.0–1.7.2. **1.7.3 reverted it** (#11153, #10978): accounts are keyed on @@ -341,7 +343,8 @@ way it does: > `>= 1.7.3` version floor: [Version Notes](#173--account-identity-reverted-breaking). > ℹ️ **RP-initiated logout: only the `id_token` is taken from better-auth.** -> MP's discovery document exposes `end_session_endpoint`, so 1.7 builds a +> The provider's `endSessionEndpoint` is configured explicitly (it came from +> discovery before issue #101), so 1.7 builds a > provider logout URL (with `id_token_hint`) and `auth.api.signOut()` returns it > as `url` when called with `disableRedirect: true`. `handleSignOut()` reads > `id_token_hint` from that URL (MP origin only) and builds the final URL @@ -351,28 +354,30 @@ way it does: #### `nonce` binding is off, and must stay off -> ⚠️ **`disableIdTokenNonceBinding: true` is load-bearing — do not remove it.** -> Because MP publishes a `jwks_uri`, better-auth 1.7 derives an id_token config -> from discovery, sets `requiresIdTokenNonce`, sends a server-generated `nonce` -> on the authorize request and then requires the claim back (OIDC Core -> §3.1.3.7). **Ministry Platform does not return it.** `nonceMatches()` treats -> an absent claim as a mismatch, so every sign-in failed with -> `/auth-error?error=unable_to_get_user_info` and the server log line -> `id_token failed verification against the discovery JWKS or expected nonce`. -> Verified 2026-09-12 by decoding a real MP id_token: `kid`, `alg`, `iss` and -> `aud` all matched; only `nonce` was missing. Fixed in `f88a9f1`. +> ⚠️ **Nonce binding is off structurally — do not re-add `discoveryUrl`.** +> With no `discoveryUrl` the provider has no id_token config, so better-auth +> neither sends a `nonce` nor requires one (`requiresIdTokenNonce` is false), +> and `verifyMpIdToken` checks none. Re-adding `discoveryUrl` makes better-auth +> 1.7 derive an id_token config from discovery, set `requiresIdTokenNonce`, +> send a server-generated `nonce` on the authorize request and then require the +> claim back (OIDC Core §3.1.3.7). **Ministry Platform does not return it.** +> `nonceMatches()` treats an absent claim as a mismatch, so every sign-in failed +> with `/auth-error?error=unable_to_get_user_info` and the server log line +> `id_token failed verification against the discovery JWKS or expected nonce` +> until `disableIdTokenNonceBinding: true` was set (`f88a9f1`). Verified +> 2026-09-12 by decoding a real MP id_token: `kid`, `alg`, `iss` and `aud` all +> matched; only `nonce` was missing. > > It looked intermittent at the time, and the reason was inverted from the > obvious one: before better-auth 1.7.3, sign-in **succeeded** only when the > boot-time discovery fetch had failed, because that left the id_token config -> undefined and skipped verification entirely. That fail-open mode is gone: a -> failed discovery now skips the provider (404 until a rebuild, see -> [Upgrade Checklist](#better-auth-upgrade-checklist) step 5), and -> `requireIdTokenVerification` refuses a partial one. +> undefined and skipped verification entirely. Neither mode exists now: there +> is no boot-time discovery, and the app's own verifier fails closed. > > **What is still checked:** the id_token signature against MP's JWKS, `iss` -> and `aud`, plus the app's own `exp`/`azp` checks in `getUserInfo`. **What is -> given up:** binding the id_token to this particular authorization request. +> and `aud` (by `verifyMpIdToken`), plus the app's own `exp`/`azp` checks in +> `getUserInfo`. **What is given up:** binding the id_token to this particular +> authorization request. > > **Accepted risk (F8), stated plainly:** with MP omitting `nonce` and not > supporting PKCE, **nothing binds an authorization code to the browser that @@ -391,8 +396,53 @@ way it does: > injection cheaper to repeat. Only a server-side state store > (`storeStateStrategy: "database"` + `secondaryStorage`) would fix that. > -> **Pinned:** `src/auth.code-flow.test.ts` asserts `disableIdTokenNonceBinding` -> and drives the real code flow against a mock provider that omits `nonce`. +> **Pinned:** `src/auth.code-flow.test.ts` asserts there is no `discoveryUrl`, +> that the live provider has no id_token config and requires no nonce, drives +> the real code flow against a mock provider that omits `nonce`, and keeps a +> negative control: with `discoveryUrl` re-added, the authorize request carries +> a `nonce` and the same MP-shaped id_token cannot sign in. + +#### id_token verification (lazy discovery) + +Since issue #101 the app verifies MP's id_token itself, in `getUserInfo` +(`verifyMpIdToken` in `src/lib/auth.ts`) — the same jose `jwtVerify` call +genericOAuth's `verifyProviderIdToken` made while discovery was configured +(`jose` is a direct dependency for this, no longer only a transitive one): + +- **Algorithm:** RS256 only (`ID_TOKEN_ALGORITHMS`). MP advertises exactly + that; pinned rather than read from discovery or the token header, so `none` + or an HS256 token signed with the client secret is refused + (`ERR_JOSE_ALG_NOT_ALLOWED`). If MP ever signs with another algorithm, + sign-in fails loudly and the list needs it. +- **Issuer and key set** come from MP's discovery document, loaded by + `lazyIdTokenVerifier` **at the first callback that needs them**, not at boot: + 5 s timeout (`OIDC_DISCOVERY_TIMEOUT_MS`, also used for the JWKS request), + `redirect: "error"`, single-flight, **cached on success** for the life of + the process (the trust model boot-time discovery had), **never cached on + failure** — the next sign-in simply tries again. jose caches the key set and + re-fetches it (at most once per 30 s) when a token names an unknown `kid`, + so key rotation needs no restart; it does not cache a failed key-set fetch + either, so a JWKS blip also heals on the next sign-in. +- **The JWKS URL is not pinned**, deliberately: it differs between + IdentityServers (a live MP serves `/oauth/.well-known/jwks`; the + IdentityServer default is `/oauth/.well-known/openid-configuration/jwks`). It + is resolved from `jwks_uri` against the discovery URL. +- **Fails closed:** a discovery document without a usable `issuer` or + `jwks_uri` is a failure (`auth.oidc.discovery_failed`, `reason: + "invalid_document"`, `field`), never a verifier that skips a check. +- **`aud`** must be `OIDC_CLIENT_ID`; `exp`/`nbf` are checked when present + (`exp` presence and `azp` by the app afterwards). +- **No `provider.issuer`:** better-auth's RFC 9207 callback `iss` check is + therefore skipped — moot, since MP's discovery document does not advertise + `authorization_response_iss_parameter_supported` (it sends no `iss`) and a + single-provider app has no mix-up to defend. + +What an MP outage looks like now: building the instance, `next build`, cold +starts and every session check make **no MP call**, so `/signin` always starts +sign-in. If MP is down at the callback, that one sign-in is refused +(`/auth-error?error=unable_to_get_user_info`, "Please try again in a moment") +and the next attempt after MP recovers succeeds on the same process — no +restart, no cooldown. `src/auth.oidc-discovery.test.ts` pins all of this. ### User Additional Fields @@ -509,21 +559,29 @@ redirect — the only mode this app uses. With `idToken: { token, accessToken }` it signs the caller in *directly*: it verifies the id_token (signature, `iss`, `aud`) and then calls our `getUserInfo` with the **caller-supplied** `accessToken` (`node_modules/better-auth/dist/api/routes/sign-in.mjs`). -Because `discoveryUrl` is set, genericOAuth builds an id_token config for the -provider, which switches that mode **on** (`supportsIdTokenSignIn`), and +While `discoveryUrl` was set, genericOAuth built an id_token config for the +provider, which switched that mode **on** (`supportsIdTokenSignIn`), and genericOAuth has no option to turn it off. Nothing bound the verified id_token to the access token, so an attacker's own valid id_token plus **any other user's** MP access token minted a session as that other user. Reproduced end to -end against a mock OIDC server. Dropping `discoveryUrl` would also close it but -is a separate trade-off (it removes JWKS verification of the code-flow id_token -too), so it stays. +end against a mock OIDC server. -Three independent layers now close it; keep all three: +**Since issue #101 there is no `discoveryUrl`**, so the provider has no id_token +config and better-auth refuses the mode itself (`404 ID_TOKEN_NOT_SUPPORTED`); +the id_token of the normal code flow is verified by `verifyMpIdToken` instead +(see [id_token verification](#id_token-verification-lazy-discovery)). That is a +side effect of configuration, not a control — re-adding `discoveryUrl` switches +the mode straight back on — so the layers below stay, and the hook still answers +first. `src/auth.id-token-sign-in.test.ts` proves both: with the hook removed, +better-auth returns `ID_TOKEN_NOT_SUPPORTED`; with the hook removed **and** +`discoveryUrl` re-added, the sub binding still refuses the substitution. + +Three independent layers close it; keep all three: | Layer | Where | What it does | |---|---|---| | **Primary** | `refuseIdTokenSignIn` — `hooks.before` in `src/lib/auth.ts` | If `ctx.path === "/sign-in/social"` and the body *has* an `idToken` key (presence, not truthiness), throws `APIError.from("NOT_FOUND", { code: "ID_TOKEN_SIGN_IN_DISABLED" })`. User hooks run for HTTP **and** in-process `auth.api.signInSocial`, so this covers callers the route never sees. 404, not 400, to match the route's deny posture and better-auth's own `ID_TOKEN_NOT_SUPPORTED` (also 404). The customSession/nextCookies plugins register hooks on their plugin objects, so the user `hooks` key does not displace them | -| **Defence in depth** | `getUserInfo` in `src/lib/auth.ts` | Decodes the id_token payload (hand-rolled base64url — `jose` is only a transitive dependency) and refuses (`null`, never a throw) unless its `sub` equals the userinfo `sub`, case-insensitively. Logs `auth.userinfo.sub_mismatch` with a `reason` (`missing_id_token`, `undecodable_id_token`, `missing_sub`, `mismatch`) and nothing else — no GUIDs, no token content. The decode is unverified on purpose: genericOAuth's wrapper has already verified the id_token before `getUserInfo` runs | +| **Defence in depth** | `getUserInfo` in `src/lib/auth.ts` | Verifies the id_token (`verifyMpIdToken`; a malformed or unverifiable one logs `auth.userinfo.id_token_unverified`), then refuses (`null`, never a throw) unless its `sub` equals the userinfo `sub`, case-insensitively. Logs `auth.userinfo.sub_mismatch` with a `reason` (`missing_id_token`, `missing_sub`, `mismatch`) and nothing else — no GUIDs, no token content | | **Route filter** | `isAllowedSignInSocialBody` in `src/app/api/auth/[...all]/route.ts` | For `POST /sign-in/social` only: Content-Type media type must be exactly `application/json` (params and case tolerated, any `,` refused); `request.clone().json()` must parse to a plain object whose keys are all in `allowedSignInSocialKeys` (`provider`, `callbackURL` — exactly what `authClient.signIn.social` sends) with `provider === "ministry-platform"`. Anything else gets the route's usual 404 without reaching better-auth. This also closes `scopes`, `loginHint`, `additionalParams`, `errorCallbackURL`, `newUserCallbackURL`, `additionalData`, `requestSignUp` and `disableRedirect` | **A missing id_token fails closed.** Every legitimate caller supplies one: the @@ -704,7 +762,8 @@ The guard must be a **ref, checked and set synchronously before the first fails against the old implementation. `/signin` also no longer fails silently: a 429 from the rate limiter, a missing -provider (`PROVIDER_NOT_FOUND`), a failed session check or a failed start each +provider (`PROVIDER_NOT_FOUND` — since issue #101 a misconfiguration only, never +an MP outage), a failed session check or a failed start each show a message with a retry button instead of a blank spinner, and automatic restarts are capped (`MAX_AUTOMATIC_SIGN_IN_ATTEMPTS` = 2 per 2 minutes, `src/components/sign-in/sign-in-attempts.ts`, cleared by `SessionGuard` once a @@ -1056,8 +1115,9 @@ and the thrown `Error`'s message keeps only `status`/`statusText`/`endpoint` — response body. The structured events above (`mp.read.unauthorized`, `mp.write.unauthorized`, `mp.write.non_user`, and in `src/lib/auth.ts` `auth.userinfo.invalid_sub`, `auth.userinfo.sub_mismatch`, -`auth.userinfo.id_token_claims_invalid`, `auth.userinfo.fetch_failed`, -`auth.session.user_id_unresolved` and `auth.discovery.rebuild`) are the greppable +`auth.userinfo.id_token_claims_invalid`, `auth.userinfo.id_token_unverified`, +`auth.userinfo.fetch_failed`, `auth.oidc.discovery_failed` and +`auth.session.user_id_unresolved`) are the greppable contract this policy exists alongside; they log a `reason`/status/error name only — never GUIDs, token contents or `err.message` — and are unaffected by it. Caller input is never echoed into our own log lines or thrown messages either (log injection). @@ -1330,14 +1390,18 @@ the `better-auth` version, do this before merging: `DEFAULT_SECRET` and the `disableOriginCheck` resolution. See [Session lifetime and revocation](#session-lifetime-and-revocation-stateless). - the mock-OIDC suites — `src/auth.code-flow.test.ts` (state binding, - `pkce`/nonce pins, the full callback), `src/auth.oidc-hardening.test.ts` - (`requireIdTokenVerification`, `exp`/`azp`, JWE strategy), - `src/auth.origin-check.test.ts`, `src/auth.session-config.test.ts`, - `src/auth.rate-limit.test.ts` and `src/auth.discovery-rebuild.test.ts`. Each + `pkce`/no-`discoveryUrl`/nonce pins, the full callback), + `src/auth.oidc-hardening.test.ts` (fail-closed partial discovery, + `exp`/`azp`, JWE strategy), `src/auth.oidc-discovery.test.ts` (issue #101: + no MP call at boot, recovery after a discovery/JWKS failure, + `lazyIdTokenVerifier`), `src/auth.origin-check.test.ts`, + `src/auth.session-config.test.ts` and `src/auth.rate-limit.test.ts`. Each has mutation controls (origin check off, `trustedOrigins: ["*"]`, cookie strategy, `expiresIn`, `refreshCache`, sub binding, nonce, PKCE, account cookie); a green run after an upgrade means those still bite. Simplest: - `npm run test:run -- src/auth`. + `npm run test:run -- src/auth`. Also re-check that genericOAuth still makes + no network call in `init` for a provider without `discoveryUrl`, and still + leaves such a provider with no id_token config. 4. **Manual smoke test (required — nothing else catches this):** - `npm run dev`, sign in through Ministry Platform. - Open `/api/auth/get-session` and confirm the session `user` object contains @@ -1346,26 +1410,29 @@ the `better-auth` version, do this before merging: - Sign out and confirm the MP end-session redirect completes. - Existing sessions predate the new user-record shape, so **sign out and log in fresh** — don't test against a stale session. -5. **If sign-in fails at the callback**, check the dev-server log for the - provider-level errors better-auth emits at init and callback time: +5. **If sign-in fails at the callback** (`/auth-error?error=unable_to_get_user_info`), + check the server log for the app's structured events: + - `auth.oidc.discovery_failed` → MP's discovery document did not load when + the callback needed it: `reason` is `request_failed` (with `errName`, e.g. + `TimeoutError` after `OIDC_DISCOVERY_TIMEOUT_MS`), `http_status` (with + `status`), `invalid_json`, or `invalid_document` (with `field`: `body`, + `issuer` or `jwks_uri`). Followed by `auth.userinfo.id_token_unverified` + with `reason: "verifier_unavailable"`. Nothing is cached, so the next + sign-in retries; no restart is needed. + - `auth.userinfo.id_token_unverified` with `reason: "verification_failed"` → + jose refused the token or could not fetch the key set; `code` says which + (`ERR_JWS_SIGNATURE_VERIFICATION_FAILED`, `ERR_JWT_CLAIM_VALIDATION_FAILED` + with `claim` `iss`/`aud`, `ERR_JWT_EXPIRED`, `ERR_JOSE_ALG_NOT_ALLOWED` — MP + signing with something other than `ID_TOKEN_ALGORITHMS` — or + `ERR_JWKS_TIMEOUT`). An `iss`/`aud` failure after an MP change means the + issuer or the OIDC client changed. - `id_token failed verification against the discovery JWKS or expected nonce` - → if `disableIdTokenNonceBinding: true` has gone missing from the config, - put it back: MP never sends the `nonce` claim (see - [`nonce` binding is off](#nonce-binding-is-off-and-must-stay-off)). With it - set, this line means JWKS/issuer/audience changed instead. - - `Discovery fetch failed for "ministry-platform"`, or `Provider - "ministry-platform": … Provider skipped.` → MP discovery was unreachable - (or, with `requireIdTokenVerification`, lacked a usable `issuer` or - `jwks_uri`) when the instance was built. Since 1.7.3 this no longer throws out - of `betterAuth()` (#10978); the provider is skipped and sign-in fails closed - with `404 PROVIDER_NOT_FOUND`. It heals without a restart: the exported `auth` - is a `selfHealingAuth` facade, and the next sign-in or callback request after - a 30 s cooldown (`DISCOVERY_REBUILD_COOLDOWN_MS`) rebuilds the instance — - single-flight, waiting at most 10 s — and swaps it in only if the provider is - now present. Look for `auth.discovery.rebuild` with `outcome` `recovered`, - `provider_still_missing` or `create_failed`. `/get-session` never triggers a - rebuild. Because the facade delegates to the *current* instance, never cache - `auth.api` in a module-level variable. + (better-auth's own line) → `discoveryUrl` has come back into the config, + which turns nonce binding on; MP never sends the `nonce` claim. Remove it + (see [`nonce` binding is off](#nonce-binding-is-off-and-must-stay-off)). + - `404 PROVIDER_NOT_FOUND` from `/sign-in/social` → the provider is not + registered. Without `discoveryUrl` genericOAuth makes no network call in + `init`, so this is a configuration error, never an MP outage. 6. If `userGuid` is missing, check `parseAdditionalUserInputFromProviderProfile` in `node_modules/better-auth/dist/db/schema.mjs` — the library may have changed how additional fields flow from the OAuth profile into the user record. diff --git a/.claude/references/deps-known-issues.md b/.claude/references/deps-known-issues.md index 69c2b059..1b33d9af 100644 --- a/.claude/references/deps-known-issues.md +++ b/.claude/references/deps-known-issues.md @@ -202,9 +202,12 @@ reaching `main` — by the guard, from Windows, as designed. `deps:relock` is `node scripts/check-lockfile.mjs --fix` — the same guard as `deps:verify`, which relocks a throwaway copy with `npm install --package-lock-only --os=linux --cpu=x64 --ignore-scripts --no-audit --no-fund` and writes the canonical result back over -`package-lock.json`, preserving the repo's CRLF line endings. It rewrites only when the tree -shape actually drifted; a lockfile that differs from canonical in npm metadata alone is left -untouched. Verified 2026-08-21: it restores every missing nested/bundled entry, prunes +`package-lock.json`, preserving the repo's CRLF line endings. It rewrites whenever the result +differs from the committed lockfile — tree drift **or** npm metadata alone. (Until 2026-09-29 +it wrote only on tree drift, so adding a dependency that was already installed transitively — +`jose`, hoisted via better-auth, for issue #101 — changed only the root entry's +`dependencies`, which the tree comparison skips, and the new dependency never reached the +lockfile. `deps:verify` still passes on metadata-only differences.) Verified 2026-08-21: it restores every missing nested/bundled entry, prunes nothing, and does not narrow the lockfile to one platform — platform entry counts were byte-identical before and after (win32 76, darwin 75, linux-x64 46, android 40). It is idempotent, and `--package-lock-only` never touches `node_modules`, so it is safe to run @@ -270,7 +273,7 @@ entanglement is inherent to those upstream packages, so the guard is the fix, no | Item | Detail | Raised | |---|---|---| -| `pkce: false` in `src/lib/auth.ts` | Not a dependency issue, but a live security posture item: MP discovery advertises `S256`, so PKCE can likely be enabled. Needs its own change + test. | 2026-08-21 | +| `pkce: false` in `src/lib/auth.ts` | Not a dependency issue, but a live security posture item: MP discovery advertises `S256`, so PKCE can likely be enabled. Needs its own change + test. *Update 2026-09-29: decided — MP does not actually support PKCE despite advertising it, so `pkce: false` is required (accepted risk F8, `.claude/references/auth.md`).* | 2026-08-21 | ### Resolved diff --git a/.claude/references/testing.md b/.claude/references/testing.md index 84101b55..736561b8 100644 --- a/.claude/references/testing.md +++ b/.claude/references/testing.md @@ -728,24 +728,24 @@ Things that will bite whoever edits these: Per-file counts below are from `npx vitest run --reporter=json` on 2026-09-29 (both projects; `scripts/` paths are repo-relative, the rest are under `src/`). -They sum to 2037 (2034 passing, 3 skipped) at that date; the total drifts as tests +They sum to 2065 (2062 passing, 3 skipped) at that date; the total drifts as tests are added, so treat it as a snapshot and rerun the command for a current figure. | Test File | Tests | What It Covers | |-----------|-------|----------------| -| `app/api/auth/[...all]/route.test.ts` | 118 | Route allowlist (deny-by-default), `/sign-in/social` body-key filter, strict raw-header Content-Type check (NBSP), body size cap (declared and streamed), `callbackURL` ≤ 2048 (F12), `Cache-Control: no-store` on every response, `toNextJsHandler` wiring | +| `app/api/auth/[...all]/route.test.ts` | 118 | Route allowlist (deny-by-default), `/sign-in/social` body-key filter, strict raw-header Content-Type check (NBSP), body size cap (declared and streamed), `callbackURL` ≤ 2048 (F12), `Cache-Control: no-store` on every response, `toNextJsHandler` wiring — with a `fetch` that throws for every URL (importing auth and starting sign-in make no MP call) | | `lib/providers/ministry-platform/utils/http-client.test.ts` | 100 | HTTP verbs, URL building, form data, error handling; per-request timeouts, `redirect: "error"`, JSON Content-Type check, `buildUrl` endpoint guard (`..`, `?`, `#`, `\`, encoded separators, control chars), GUID-redacted errors | -| `auth.test.ts` | 81 | `enrichSessionUser` (`userId` only; `token`/`ipAddress`/`userAgent` withheld), cached User_ID resolution, OAuth config guards, `getUserInfo` id_token↔userinfo `sub` binding (F12), disabled paths incl. `/link-social` — runs on the verified (`requireIdTokenVerification`) path | +| `auth.test.ts` | 83 | `enrichSessionUser` (`userId` only; `token`/`ipAddress`/`userAgent` withheld), cached User_ID resolution, OAuth config guards (explicit endpoints, no `discoveryUrl`, `endSessionEndpoint`, `accountSubject` reads `sub`), `getUserInfo`'s own id_token verification (malformed, `alg: none`, HS256 with the client secret, non-numeric/past `exp`), `exp`/`azp` checks, id_token↔userinfo `sub` binding (F12), disabled paths incl. `/link-social`. Node environment; calls `getUserInfo` with genuinely signed tokens (`oidc.signIdToken`) and `stubUserinfo`, which replaces only the userinfo response | | `services/contactLogService.test.ts` | 80 | Contact log CRUD, service-layer read/write gate, field allowlists, `Made_By` kept on update, date conversion, Zod validation, filter-injection regression guard | | `lib/providers/ministry-platform/services/file.service.test.ts` | 76 | All 8 file endpoints, multipart bodies, unauthenticated blob fetch (timeout, no redirects), ID/GUID validation, unique IDs kept out of errors and logs | | `lib/providers/ministry-platform/utils/filter-sanitize.test.ts` | 68 | Quote doubling, LIKE escaping, GUID rejection, numeric-ID validation | +| `lib/providers/ministry-platform/services/communication.service.test.ts` | 68 | Email/SMS JSON vs multipart paths, trusted sender stamped over payload `author`/`from`/`FromAddress` | | `app/signin/page.test.tsx` | 65 | `signIn.social({ provider: "ministry-platform" })`, `callbackUrl` fallbacks and open-redirect sanitizing on both sinks (F3/F3b: tab/CR/LF, backslash, control chars, encoded separators), already-signed-in bounce, `isRedirecting` latch | | `lib/providers/ministry-platform/helper.test.ts` | 65 | MPHelper CRUD, validation, procedures (deny-all default, `allowedProcedures`), communications with a required sender, files | | `scripts/setup-env.test.ts` | 64 | (`scripts` project) setup's `.env.local` writer: values quoted/escaped and round-tripped through `@next/env`, exact-key replace, invalid keys/values refused, file written 0600 | | `components/contact-logs/actions.test.ts` | 61 | Contact log create/update/delete actions, security-role gate on reads AND writes, argument guards, field allowlists, ownership policy, numeric-ID injection rejection | | `lib/security-headers.test.ts` | 57 | Static header values (framing, sniffing, referrer, permissions, COOP/CORP, HSTS in production only), `originOf` (non-http(s) schemes and unsafe hostnames → null), nonce generation, every CSP directive incl. `base-uri 'none'`, enforce vs report-only | | `services/authorizationService.test.ts` | 57 | MP security-role gate for reads and writes, `hasSecurityRole`, fail-closed default when no role policy is configured (`roles_not_configured`, `mp.authz.config` warn-once), explicit `*` = any role, `","` treated as unset, `MP_SECURITY_ROLES` + deprecated `MP_WRITE_SECURITY_ROLES` fallback, `mp.read.unauthorized` / `mp.write.unauthorized` denials, no cross-request caching | -| `lib/providers/ministry-platform/services/communication.service.test.ts` | 56 | Email/SMS JSON vs multipart paths, trusted sender stamped over payload `author`/`from`/`FromAddress` | | `proxy.test.ts` | 56 | Route protection (public paths incl. `/signed-out`, `/api` exact + anchored matcher, cookie presence only), the per-request CSP nonce forwarded on request headers, enforce vs report-only, MP origins in `img-src` / `form-action`, matcher pattern | | `components/sign-in/sign-in.test.tsx` | 49 | `sanitizeCallbackUrl` unit tests: hostile and benign lists, C1/lowercase-encoded cases, raw (not URL-normalized) return, origin/throw backstops | | `lib/providers/ministry-platform/services/table.service.test.ts` | 48 | TableService CRUD, table-name identifier validation, ID validation | @@ -758,11 +758,12 @@ are added, so treat it as a snapshot and rerun the command for a current figure. | `lib/providers/ministry-platform/services/guards.test.ts` | 30 | `sanitizeIdentifier` (plain identifiers, ≤ 128) and `errorName` (class name only, never the message) | | `lib/providers/ministry-platform/provider.test.ts` | 28 | Provider delegation to all six sub-services, sender threaded through to communications | | `components/contact-lookup-details/actions.test.ts` | 27 | Contact details + log type mapping, security-role read gate, numeric-ID injection rejection | -| `auth.code-flow.test.ts` | 25 | The real authorization-code flow against `mock-oidc`: authorize request (state, redirect URI, no nonce/PKCE), happy-path callback, state validation (missing, mismatched, cookie-less, tampered, expired — refused before the code is redeemed), id_token and sub-binding refusals | +| `auth.code-flow.test.ts` | 25 | The real authorization-code flow against `mock-oidc`: authorize request (state, redirect URI, no nonce/PKCE), happy-path callback, state validation (missing, mismatched, cookie-less, tampered, expired — refused before the code is redeemed), id_token refusals (foreign key, wrong `iss`/`aud`, expired — logged with jose's `code`) and sub-binding refusals; pins `pkce: false` and no `discoveryUrl`, with a negative control that re-adding it turns nonce binding on and breaks MP sign-in | | `app/auth-error/page.test.tsx` | 24 | OAuth-failure landing page, known codes mapped to fixed messages, unrecognized codes get the generic one, `error_description` never rendered | -| `auth.secret-guard.test.ts` | 24 | `assertAuthEnvironment`: missing, empty, default, short (< 32) or overridden secret refused at import; `TEST` flag; auth-critical URLs validated at module load; the secret never in the message | +| `auth.secret-guard.test.ts` | 24 | `assertAuthEnvironment`: missing, empty, default, short (< 32) or overridden secret refused at import; `TEST` flag; auth-critical URLs validated at module load (and the endpoints built from the normalized MP URL, with no MP call when the instance is built); the secret never in the message | | `lib/providers/ministry-platform/auth/client-credentials.test.ts` | 24 | Client-credentials token grant: timeout, `redirect: "error"`, Content-Type and token-shape checks, status in errors, no body in logs | | `components/layout/header.test.tsx` | 23 | App-title env fallback, profile-loading state, bar/hamburger/sidebar stay rendered while `useUser()` is suspended (only the avatar suspends), `HeaderSkeleton` matches the fixed h-16 bar, avatar vs icon fallback, the tooltip chain (incl. falling back to `mpEmail`, never the synthetic session email), sidebar open/close ownership | +| `auth.oidc-discovery.test.ts` | 23 | Issue #101 regression: MP rejecting, 500ing or hanging at boot → the instance builds, sign-in starts and session checks answer with no MP call; discovery or JWKS failing at the callback → that sign-in refused and logged, the next one after MP recovers succeeds on the same instance; discovery timeout (`OIDC_DISCOVERY_TIMEOUT_MS`) and `redirect: "error"`; single-flight and cached success; `lazyIdTokenVerifier` unit (relative `jwks_uri`, never-cached failures, hung request timed out, fail-closed invalid documents) | | `auth.ip-address.test.ts` | 21 | `AUTH_IP_ADDRESS_HEADERS` / `AUTH_TRUSTED_PROXIES` parsing and the client IP resolved per host (Cloudflare, Azure, Vercel, appending proxy); rate-limit buckets key on it | | `services/domainTimezoneService.test.ts` | 20 | Windows-to-IANA mapping, DST, round-tripping, cache | | `components/contact-lookup/contact-lookup-search.test.tsx` | 19 | Empty query clears results without calling the action, in-flight lock via the disabled button, Enter vs button submit, action rejection surfaced | @@ -779,18 +780,17 @@ are added, so treat it as a snapshot and rerun the command for a current figure. | `components/layout/sidebar.test.tsx` | 13 | Nav label+href pairs, entries are `next/link` (no document reload), drawer + Dashboard render while the profile is pending, `onClose` from X and from a nav link, panel stays mounted when closed, Contact Lookup hidden/shown by `canAccessContactFeatures` (fails closed) | | `components/shared-actions/user.test.ts` | 13 | `getCurrentUserProfile` maps to the six-field `CurrentUserProfile`, server-computed `canAccessContactFeatures`, role-less users keep their profile | | `scripts/check-prerender.test.ts` | 13 | (`scripts` project) CI prerender guard: only `/_not-found` and `/_global-error` may be static; exit codes | +| `auth.id-token-sign-in.test.ts` | 13 | F12 end to end: real `auth` against a mocked MP OIDC provider with RS256-signed id_tokens; the `hooks.before` refusal over HTTP and in-process; with the hook removed, better-auth's own `ID_TOKEN_NOT_SUPPORTED` (no id_token config without `discoveryUrl`); and with the hook removed and `discoveryUrl` re-added, the `sub` binding alone (node environment) | +| `contexts/user-context.test.tsx` | 13 | Server-started promise exposed without a client refetch, suspend/resolve/reject, server re-render swaps the promise, refresh stays inside a transition (no fallback mid-reload) | | `auth.origin-check.test.ts` | 12 | The real instance enforces the origin check under `NODE_ENV=test`; cross-site `callbackURL`/Origin refused; negative controls (`disableOriginCheck`, `trustedOrigins: ["*"]`) go green, proving the test can fail | | `components/contact-lookup/contact-lookup.test.tsx` | 12 | Search-to-results state wiring, error and empty propagation, emptying the box clears stale results | | `services/sessionContextService.test.ts` | 12 | Acting-user resolution, `mp.write.non_user` warning, Next control-flow errors rethrown | -| `auth.discovery-rebuild.test.ts` | 11 | Boot-time discovery failure self-heals: rebuild on sign-in/callback after the 30 s cooldown, single-flight, only sign-in/callback trigger it, `auth.discovery.rebuild` outcomes, a healthy instance is never rebuilt | -| `auth.id-token-sign-in.test.ts` | 11 | F12 end to end: real `auth` against a mocked MP OIDC provider with RS256-signed id_tokens; the `hooks.before` refusal over HTTP and in-process, and the `sub` binding alone with the hook removed (node environment) | -| `contexts/user-context.test.tsx` | 13 | Server-started promise exposed without a client refetch, suspend/resolve/reject, server re-render swaps the promise, refresh stays inside a transition (no fallback mid-reload) | | `app/(web)/contactlookup/layout.test.tsx` | 9 | Page-layer role gate: renders children for a role-holder, `redirect("/no-access")` for every denial reason, MP failure surfaces instead of redirecting | | `app/global-error.test.tsx` | 9 | Renders its own html/body (via `renderToStaticMarkup`), sets `document.title` with no metadata export, imports no app code | | `components/sign-in/sign-in-attempts.test.ts` | 9 | `/signin` automatic-restart cap: attempts per window, future timestamps ignored, never blocks sign-in when storage throws | | `components/user-menu/sign-out-button.test.tsx` | 9 | `SignOutButton` / `signOutEverywhere`: broadcasts before `handleSignOut`, rethrows the redirect signal, returns a real failure's message | | `app/(web)/error.test.tsx` | 8 | Shell boundary: retry is called, digest shown as a reference code, and the error message reaches neither the log nor the page | -| `auth.oidc-hardening.test.ts` | 8 | `requireIdTokenVerification` (partial discovery takes the provider down), id_token `exp`/`aud`/`azp` checks through the code flow, `session_data` is JWE, `/get-session` key sets (no token, IP, UA) | +| `auth.oidc-hardening.test.ts` | 8 | Fail-closed verification (a discovery document without `issuer` or `jwks_uri` refuses sign-in at the callback, logging `auth.oidc.discovery_failed` with the `field`), id_token `exp`/`aud`/`azp` checks through the code flow, `session_data` is JWE, `/get-session` key sets (no token, IP, UA) | | `auth.session-lifetime.test.ts` | 8 | Clock walk: 12 h absolute cap, no sliding, a cookie pair copied before sign-out dies within 1 h; negative control with the pre-fix config | | `components/layout/session-guard.test.tsx` | 8 | `SessionGuard`: leaves for `/signed-out` (not `/signin`) when the session ends, no redirect before first session or mid-refetch, clears the sign-in restart counter, cross-tab broadcast re-check | | `lib/providers/ministry-platform/helper.wiring.test.ts` | 8 | Real MPHelper → provider → services with mocked HTTP: trusted sender overrides spoofed payload fields, old call shape refused, procedure allowlist per instance (default deny), no `$ignorePermissions` forwarded | @@ -818,9 +818,9 @@ are added, so treat it as a snapshot and rerun the command for a current figure. | `auth.user-id-cache.test.ts` | 3 | 15-minute User_ID cache TTL; attribution dropped within one TTL once the `dp_Users` login is gone | | `app/(web)/home/page.test.tsx` | 2 | Unconditional redirect to `/`, never looping back to `/home` | | `contexts/session-context.test.tsx` | 2 | `useAppSession` wrapper | -| **Total** | **2037** | 85 files (2026-09-29) | +| **Total** | **2065** | 85 files (2026-09-29) | -Shared helper (not a test file): `src/test-utils/mock-oidc.ts` — a mock MP OIDC provider (discovery, JWKS, token, userinfo; RS256 id_tokens) behind a stub `fetch` that **throws on any URL it does not serve**, so no auth test can reach a real Ministry Platform. Install it inside `vi.hoisted` (better-auth runs discovery when the instance is built) and use it from `// @vitest-environment node` suites. The `src/auth.*.test.ts` suites that import it run on it (`auth.test.ts`, `auth.code-flow.test.ts`, `auth.ip-address.test.ts`, `auth.origin-check.test.ts`, `auth.rate-limit.test.ts`, `auth.session-config.test.ts`, `auth.user-id-cache.test.ts`), on the verified (`requireIdTokenVerification: true`) path. +Shared helper (not a test file): `src/test-utils/mock-oidc.ts` — a mock MP OIDC provider (discovery, JWKS, token, userinfo; RS256 id_tokens) behind a stub `fetch` that **throws on any URL it does not serve**, so no auth test can reach a real Ministry Platform. `state.discovery`, `state.jwks` and `state.mp` (every endpoint) take `ok | reject | http_500 | hang` (`hang` settles only when the caller's `AbortSignal` fires); `signIdToken(claims)` mints a genuinely signed id_token for calling `getUserInfo` directly. Install it inside `vi.hoisted`, before `@/lib/auth` is imported, and use it from `// @vitest-environment node` suites. The `src/auth.*.test.ts` suites that import it run on it (`auth.test.ts`, `auth.code-flow.test.ts`, `auth.ip-address.test.ts`, `auth.origin-check.test.ts`, `auth.rate-limit.test.ts`, `auth.session-config.test.ts`, `auth.user-id-cache.test.ts`, `auth.oidc-discovery.test.ts`), with `getUserInfo` verifying id_tokens itself (issue #101). ## Ministry Platform Safety in Tests diff --git a/README.md b/README.md index 2d489621..d2e00a77 100644 --- a/README.md +++ b/README.md @@ -399,7 +399,7 @@ When deploying to production: 5. Enable HTTPS/SSL certificates 6. Test the complete authentication flow in production environment -> `next build` loads the auth module while collecting page data, so better-auth makes its read-only OIDC discovery `GET` to `MINISTRY_PLATFORM_BASE_URL` during the build. With a real `.env.local` that contacts your real MP discovery endpoint; if it fails, the build logs "Discovery fetch failed" and carries on (CI builds against an unresolvable `mp.invalid` URL on purpose). +> `next build` loads the auth module while collecting page data, but building the auth instance makes no network call: the Ministry Platform provider has explicit endpoints, and OIDC discovery is fetched only when the first sign-in callback verifies an id_token (issue #101). So a build, a cold start or an MP outage never stops `/signin` from starting sign-in; if MP is down at the callback, that one sign-in fails with a "try again" message and the next one works. ## Project Structure diff --git a/docs/security/2026-09-25-signin-hardening.md b/docs/security/2026-09-25-signin-hardening.md index 47acbd2b..83652028 100644 --- a/docs/security/2026-09-25-signin-hardening.md +++ b/docs/security/2026-09-25-signin-hardening.md @@ -30,6 +30,12 @@ With `discoveryUrl` set, better-auth 1.7 enables an ID-token mode on exchange, and it took the identity from a caller-supplied access token without checking it against the ID token. +> **Update 2026-09-29 (issue #101):** current `main` no longer sets +> `discoveryUrl` (explicit endpoints; the id_token is verified in `getUserInfo`), +> so better-auth refuses this mode itself with `404 ID_TOKEN_NOT_SUPPORTED`. The +> F12 fix (`cf5a824`) is kept and still answers first. A fork that still sets +> `discoveryUrl` is affected exactly as described here. + **What exploiting it required:** 1. An ID token issued to this app's own OIDC client. MPNext exchanges codes diff --git a/docs/security/2026-09-28-auth-review.md b/docs/security/2026-09-28-auth-review.md index 9cc9372d..87926361 100644 --- a/docs/security/2026-09-28-auth-review.md +++ b/docs/security/2026-09-28-auth-review.md @@ -44,6 +44,8 @@ The per-finding working notes were deleted once fixed; their doc edits landed in - **CI build (wave 2):** `build` job + `scripts/check-prerender.mjs` (only `/_not-found`, `/_global-error` static); setup tests run as a second Vitest project in `test:run`; `SessionContextService` rethrows Next control-flow errors (no build-log noise) — `ci-missing-lint-and-build` - **Env validation + discovery rebuild (wave 2):** `src/lib/env.ts` — `MINISTRY_PLATFORM_BASE_URL` https-only (loopback http outside production), normalized; `BETTER_AUTH_URL` required, origin only, https for every real host (loopback http allowed even in production so `next build`/`next start` work locally and in CI); the exported `auth` is a self-healing facade that rebuilds the instance on sign-in/callback when the MP provider is missing (single-flight, 30 s cooldown, `auth.discovery.rebuild` log) — `auth-url-env-not-validated`, `discovery-failure-no-retry` + + > **Update 2026-09-29 (issue #101):** the self-healing facade is gone from `main`, superseded by removing boot-time discovery. It left a gap: a *hung* discovery still stalled every request ~300 s. The provider now has explicit endpoints (no `discoveryUrl`), so building the instance makes no MP call. `getUserInfo` verifies the id_token itself (`verifyMpIdToken`, RS256 only), against an issuer and JWKS loaded lazily from discovery at the first callback. That load has a 5 s timeout, is cached on success and is never cached on failure. `requireIdTokenVerification` from item (c) is therefore no longer set: without `discoveryUrl`, genericOAuth throws for it, and the verifier now fails closed on a partial discovery document by itself. The `auth.discovery.rebuild` log event is replaced by `auth.oidc.discovery_failed` and `auth.userinfo.id_token_unverified`. - **Auth test harness (wave 2):** `src/test-utils/mock-oidc.ts` + code-flow, origin-check, session-config and rate-limit suites; all discovery stubs on the verified path; mutants (origin check off, `trustedOrigins: ["*"]`, strategy, `expiresIn`, `refreshCache`, sub-binding, nonce, PKCE, account cookie) all caught — `auth-test-gaps` - **Dependencies (wave 3):** `next`/`eslint-config-next` 16.3.5 → 16.3.7 (GHSA-vcvr-r3jv-pc5j); `server-only` guards on `auth.ts`, the MP client and every service (a client import fails `next build` — verified); relocked with `deps:relock`, `deps:verify` clean — `next-image-optimizer-and-version`, `no-server-only-guard` @@ -58,7 +60,7 @@ The per-finding working notes were deleted once fixed; their doc edits landed in | `shared-device-session-persistence` remainder, sign-out revocation (closed) | **Leave as is** — 12 h cap + 1 h replay bound accepted; document | | CSP reporting | **None** — known gap | | `style-src 'unsafe-inline'` | **Accepted** — a nonce could cover react-remove-scroll (`get-nonce` `setNonce()`); kept for simplicity, `img-src`/`font-src` already block CSS exfiltration | -| Discovery failure | **Rebuild on failure** — shared `auth` rebuilt on the next sign-in/callback after a 30 s cooldown (wave 2) | +| Discovery failure | **Rebuild on failure** — shared `auth` rebuilt on the next sign-in/callback after a 30 s cooldown (wave 2). *Superseded 2026-09-29 (issue #101): no boot-time discovery at all — see the update note above* | | F8 authorization-code injection (MP supports neither PKCE nor `nonce`) | **Accepted** — defences are a dedicated OIDC client with exact redirect URIs, `Referrer-Policy`, no code-bearing URLs in logs | Where each decision is recorded: `CLAUDE.md` and `.claude/references/auth.md` (roles by name, scope, session lifetime, F8, OAuth state reuse, cookie residue, better-auth's verbatim logging), `README.md` and `.env.example` (`MP_SECURITY_ROLES`), `.claude/references/security-headers.md` (CSP reporting, `style-src`), `docs/security/Additional_Security_Hardening.md` §1/§3, and the playbook's known-open list. diff --git a/docs/security/downstream-hardening-playbook.md b/docs/security/downstream-hardening-playbook.md index 00647df5..3aa1de50 100644 --- a/docs/security/downstream-hardening-playbook.md +++ b/docs/security/downstream-hardening-playbook.md @@ -78,8 +78,13 @@ grep -rqF 'startsWith("/\\")' src/ && echo "✗ F3b (weak leading-/\\ check)" || grep -rqF '\u001f' src/components/sign-in/ src/app/signin/ 2>/dev/null && echo "✓ control chars refused" || echo "✗ F3b (no control-char rule)" # F12: is ID-token sign-in refused? (live whenever discoveryUrl is set on better-auth >= 1.7) -grep -q "discoveryUrl" src/lib/auth.ts && ! grep -q "ID_TOKEN_SIGN_IN_DISABLED" src/lib/auth.ts \ +# (`^\s*discoveryUrl:` is the config key; the second grep skips a function parameter of that name) +grep -E "^\s*discoveryUrl:" src/lib/auth.ts | grep -qv "discoveryUrl: string" && ! grep -q "ID_TOKEN_SIGN_IN_DISABLED" src/lib/auth.ts \ && echo "✗ F12 (no hooks.before idToken guard)" || echo "✓ F12 guard (or no discoveryUrl)" +# Issue #101: does one MP blip at boot take sign-in down? (see "Boot-time discovery" below) +grep -E "^\s*discoveryUrl:" src/lib/auth.ts | grep -qv "discoveryUrl: string" && echo "✗ #101 (discovery fetched at boot)" \ + || { grep -q "verifyMpIdToken" src/lib/auth.ts && echo "✓ #101 explicit endpoints + own id_token verification" \ + || echo "✗ no discoveryUrl AND no id_token verification — sign-in is unverified"; } grep -q "allowedSignInSocialKeys" "src/app/api/auth/[...all]/route.ts" && echo "✓ F12 route filter" || echo "✗ F12 (route filter)" # F4: can a caller smuggle attribution fields into a write? @@ -569,10 +574,11 @@ Keep structured events. Alerts grep on them; the main ones upstream: | `mp.write.non_user` | a write ran with no resolved acting user | | `auth.userinfo.invalid_sub` | MP userinfo returned no usable `sub` | | `auth.userinfo.sub_mismatch` | id_token `sub` missing or not equal to userinfo `sub` (F12) | -| `auth.userinfo.id_token_claims_invalid` | id_token `exp` missing or past, or `azp` wrong (2026-09-28) | +| `auth.userinfo.id_token_claims_invalid` | id_token `exp` missing, or `azp` wrong (2026-09-28; a past `exp` is now refused by jose — `id_token_unverified`) | +| `auth.userinfo.id_token_unverified` | id_token could not be verified: `reason` `verifier_unavailable` (discovery did not load) or `verification_failed` with jose's `code`/`claim` (issue #101, 2026-09-29) | | `auth.userinfo.fetch_failed` | userinfo request failed, timed out or redirected (2026-09-28) | +| `auth.oidc.discovery_failed` | MP's discovery document did not load at the callback: `reason` `http_status`, `request_failed`, `invalid_json` or `invalid_document` (+ `field`) (issue #101, 2026-09-29) | | `auth.session.user_id_unresolved` | `dp_Users` lookup for the session's `User_ID` failed (2026-09-28) | -| `auth.discovery.rebuild` | the `auth` instance was rebuilt after a failed discovery (2026-09-29) | ### Make it enforced, not advisory @@ -869,7 +875,7 @@ precondition cheaper) — as rated in the advisory. ### Does this apply to me? ```bash -grep -n "discoveryUrl" src/lib/auth.ts # set? the branch is live +grep -nE "^\s*discoveryUrl:" src/lib/auth.ts | grep -v "discoveryUrl: string" # set? the branch is live grep -n "ID_TOKEN_SIGN_IN_DISABLED" src/lib/auth.ts # absent? affected grep '"version"' node_modules/better-auth/package.json # >= 1.7? affected grep -n "allowedSignInSocialKeys" "src/app/api/auth/[...all]/route.ts" # absent? layer (a) missing @@ -898,8 +904,11 @@ better-auth 1.7's `POST /sign-in/social` has an `idToken` branch: body `{ provider, idToken: { token, accessToken } }` creates a session directly — no `state`, no authorization code, no exchange. It is enabled for a genericOAuth provider whenever that provider has an id-token verification -config, which it gets automatically because `auth.ts` sets `discoveryUrl`. -**genericOAuth has no option to turn it off.** +config, which it gets automatically when `auth.ts` sets `discoveryUrl`. +**genericOAuth has no option to turn it off.** (Upstream `main` stopped +setting `discoveryUrl` in issue #101, so the branch is off there as well — see +[Boot-time discovery](#boot-time-discovery-issue-101) — but the guard below is +kept as the primary control.) better-auth verifies the id_token (signature, issuer, audience = your `OIDC_CLIENT_ID`), then calls **our** `getUserInfo` with the @@ -987,12 +996,13 @@ Why all three: (a) is HTTP-only and in-process `auth.api` calls skip it; (b) depends on better-auth keeping the key named `idToken`; (c) is what still holds if a future better-auth path reaches `getUserInfo` with caller-supplied tokens. -Considered and **deferred**: dropping `discoveryUrl` would remove the id-token -config and with it the branch — and would also remove the dependence on -discovery at boot — but it loses JWKS verification of the normal flow's id_token -(upstream now *requires* it: `requireIdTokenVerification: true`), and -since 1.7 reads `profile.id` for non-OIDC providers it needs an -`accountSubject` mapping. A separate trade-off, not part of this fix. +Dropping `discoveryUrl` was **deferred** here, because it loses genericOAuth's +JWKS verification of the normal flow's id_token. It was **done in issue #101** +(2026-09-29): the app now verifies the id_token itself, with the same jose call, +and maps `accountSubject` to `sub`. That removes the boot-time dependency on +discovery, and the branch goes with it. See +[Boot-time discovery](#boot-time-discovery-issue-101). Keep (a)–(c) anyway, +since re-adding `discoveryUrl` would switch the branch straight back on. ### Tests, and the mutations that must fail them @@ -1051,20 +1061,26 @@ Symptom: `/auth-error?error=unable_to_get_user_info`, with better-auth 1.7 turns nonce binding on automatically for any provider whose discovery document yields an id_token config, sends a `nonce` on the authorize request, then requires the claim to come back — `nonceMatches` returns false when -the claim is absent. **MP omits it.** So: +the claim is absent. **MP omits it.** So, while your config sets `discoveryUrl`: ```ts disableIdTokenNonceBinding: true, ``` +Upstream no longer needs this. Since issue #101 it sets no `discoveryUrl`, so the +provider has no id_token config and nonce binding is off structurally (see +[Boot-time discovery](#boot-time-discovery-issue-101)). If you port that, drop +both options. If you keep `discoveryUrl`, keep both this and +`requireIdTokenVerification: true`. + What made this look intermittent is inverted from the obvious reading: on better-auth before 1.7.3, **sign-in succeeded only when the boot-time discovery fetch had failed**, because that left the id_token config undefined and skipped verification altogether. A *working* discovery meant a *broken* sign-in. On 1.7.4 a failed discovery skips the provider instead (sign-in 404s -`PROVIDER_NOT_FOUND`), and upstream sets `requireIdTokenVerification: true` so a -discovery document missing `issuer` or `jwks_uri` is refused rather than -silently skipping verification. +`PROVIDER_NOT_FOUND`), and `requireIdTokenVerification: true` refuses a +discovery document missing `issuer` or `jwks_uri` rather than silently skipping +verification. What you give up: binding the id_token to this particular authorization request. Signature, issuer and audience are still verified against MP's JWKS. The @@ -1244,6 +1260,9 @@ Before you call your fork done: - [ ] Every page that reads MP data gates itself — delete a layout's gate and a direct request for the page is still refused - [ ] `BETTER_AUTH_SECRET` rotated if you were ever exposed to F-UPDATE-USER +- [ ] With MP unreachable (or hanging) when the app starts, `POST /api/auth/sign-in/social` + still returns 200 immediately, and a sign-in after MP recovers succeeds without a + restart (issue #101) --- @@ -1260,8 +1279,8 @@ A second auth review (2026-09-28) found no Critical or High issues; the characters in endpoint paths; errors log names only. - **Provider services:** table/procedure names, IDs and GUIDs validated and each path segment encoded; `$ignorePermissions` removed; codegen escaping. -- **Auth core:** `requireIdTokenVerification: true`; id_token `exp`/`azp` - checks; userinfo never throws (timeout, no redirects); `session_data` is +- **Auth core:** `requireIdTokenVerification: true` (superseded by issue #101 + below); id_token `exp`/`azp` checks; userinfo never throws (timeout, no redirects); `session_data` is encrypted (JWE); `User_ID` lookup failures negative-cached (30 s / 5 min); `token`, `ipAddress` and `userAgent` stripped from `/get-session`; startup guard on the secret; `AUTH_IP_ADDRESS_HEADERS` / `AUTH_TRUSTED_PROXIES` for @@ -1282,11 +1301,13 @@ A second auth review (2026-09-28) found no Critical or High issues; the 0600; `npm ci`, no `npm update`. - **Env validation + discovery:** `src/lib/env.ts` validates both URLs at startup. A failed discovery no longer disables sign-in for the life of the - process: the next sign-in or callback after a 30 s cooldown rebuilds the - `auth` instance (single-flight; `auth.discovery.rebuild` log event). + process. This first landed as a self-healing rebuild of the `auth` instance, + and was then replaced by removing boot-time discovery altogether — see + [Boot-time discovery](#boot-time-discovery-issue-101). - **Tests:** a mock OIDC provider (`src/test-utils/mock-oidc.ts`) drives the - real code flow; origin check, session config, rate limit and discovery - rebuild each have a suite that a mutation turns red. + real code flow; origin check, session config, rate limit and the discovery + outage (`src/auth.oidc-discovery.test.ts`) each have a suite that a mutation + turns red. - **Dependencies:** Next 16.3.7 (GHSA-vcvr-r3jv-pc5j); `import "server-only"` in `auth.ts`, the MP client and every service. @@ -1316,6 +1337,43 @@ and a verification list, is in `.claude/playbooks/port-security-review-2026-09-2 `session_data` cookie once, on deploy. Where no in-memory session row backs the request (a new process or another instance), those users sign in again. +### Boot-time discovery (issue #101) + +Reported 2026-09-29 by Jonathon Huff (The Moody Church). With `discoveryUrl` +set, genericOAuth fetches MP's discovery document **once**, inside the auth +context's `init`, with no timeout and no retry, and every auth request awaits +that `init`. Every in-process `auth.api.*` call awaits it too, which means every +page's session check. + +- A **failed** fetch drops the provider: `/sign-in/social` returns `404 + PROVIDER_NOT_FOUND`, and `/signin` spins, until a restart. The rebuild facade + above healed this after a 30 s cooldown. +- A **hung** fetch stalls every request for undici's ~300 s headers timeout. + The facade did not cover this case. + +`/signin` still returns 200, so an uptime check stays green. + +**Does this apply to me?** It applies if `grep -nE "^\s*discoveryUrl:" +src/lib/auth.ts | grep -v "discoveryUrl: string"` prints (the second grep skips +a function parameter of that name). It is worse without the facade. + +**Upstream fix:** no `discoveryUrl`. Explicit `authorizationUrl`, `tokenUrl` and +`endSessionEndpoint`, plus `accountSubject: ({ profile }) => profile.sub`. +`getUserInfo` verifies the id_token itself (`verifyMpIdToken`: jose `jwtVerify`, +RS256 only, `iss`/`aud`) against an issuer and JWKS that `lazyIdTokenVerifier` +loads from discovery **at the first callback**: + +- 5 s timeout; +- cached on success, never on failure; +- single-flight. + +Building the instance, `next build`, cold starts and session checks make no MP +call. An MP outage now fails only the sign-in whose callback hits it, and the +next sign-in recovers. It also removes the F12 branch at the source, because +the provider has no id_token config, and nonce binding goes the same way. Step +by step, including what to delete if you ported the rebuild facade: +`.claude/playbooks/port-security-review-2026-09-28.md` Phase 4. + --- ## Known-open items upstream (not yet fixed anywhere) @@ -1345,8 +1403,11 @@ and a verification list, is in `.claude/playbooks/port-security-review-2026-09-2 `disableIdTokenSignIn` is not an alternative: the normal code flow calls the same `verifyProviderIdToken`, which returns `false` when that flag is set, so it would break every sign-in. Re-check the hook and the route's key list on - every better-auth upgrade. Dropping `discoveryUrl` (which would remove the - branch, at the cost of JWKS verification) is deferred as a separate trade-off. + every better-auth upgrade. **Largely closed by issue #101 (2026-09-29):** the + provider has no `discoveryUrl`, and so no id_token config, which means + better-auth refuses the branch itself (`ID_TOKEN_NOT_SUPPORTED`). JWKS + verification is kept, because the app now does it. The hook stays, since + re-adding `discoveryUrl` would switch the branch back on. - **better-auth's own logger** still logs callback `error`/`state`/`iss`/ `callbackURL` values verbatim (accepted; a custom `logger` would fix it).