diff --git a/.env.example b/.env.example index dee0316..9ba378f 100644 --- a/.env.example +++ b/.env.example @@ -69,6 +69,12 @@ NODE_ENV=production # # WEBAUTHN_RP_ID=bills.example.com +# openid-client v6 requires HTTPS for the OIDC issuer and all discovered +# endpoints. If your identity provider is reached over an internal http:// +# address (e.g. http://authentik:9000 on a Docker network), opt back in: +# +# OIDC_ALLOW_INSECURE_HTTP=true + # ── Bank Sync (SimpleFIN) ───────────────────────────────────────────────────── # Enable/disable bank sync from the Admin panel. Users connect their own # SimpleFIN Bridge from the Data page. No environment config required. diff --git a/client/api.sessionExpiry.test.ts b/client/api.sessionExpiry.test.ts index 981136d..71142fe 100644 --- a/client/api.sessionExpiry.test.ts +++ b/client/api.sessionExpiry.test.ts @@ -1,22 +1,21 @@ // @vitest-environment jsdom -// 1e (QA follow-up) — a 401 outside /auth/* means the session died mid-use: -// api.ts must dispatch 'auth:expired' (AuthProvider clears the user and -// RequireAuth redirects). A 401 under /auth/* is "wrong credential input" -// (e.g. change-password with a bad current password) and must NOT dispatch. +// Session-expiry contract: the auth middleware is the only source of the +// AUTH_REQUIRED code (no valid session) — api.ts dispatches 'auth:expired' on +// it so AuthProvider clears the user and RequireAuth redirects. Any other 401 +// (code AUTH_ERROR — wrong credential input, e.g. change-password with a bad +// current password, on ANY path) must NOT dispatch. A path prefix cannot make +// that distinction — /profile/change-password proved it in review. import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { api } from '@/api'; -function mock401(): typeof fetch { +function mock401(code: string): typeof fetch { return vi.fn( async () => - new Response( - JSON.stringify({ error: 'AUTH_ERROR', message: 'Unauthorized', code: 'AUTH_ERROR' }), - { - status: 401, - headers: { 'Content-Type': 'application/json' }, - }, - ), + new Response(JSON.stringify({ error: code, message: 'Unauthorized', code }), { + status: 401, + headers: { 'Content-Type': 'application/json' }, + }), ) as unknown as typeof fetch; } @@ -29,7 +28,6 @@ describe('session-expiry dispatch on 401', () => { beforeEach(() => { expiredEvents = 0; window.addEventListener('auth:expired', onExpired); - vi.stubGlobal('fetch', mock401()); }); afterEach(() => { @@ -37,12 +35,17 @@ describe('session-expiry dispatch on 401', () => { vi.unstubAllGlobals(); }); - it('dispatches auth:expired for a 401 on a data endpoint', async () => { + it('dispatches auth:expired for AUTH_REQUIRED (dead session) on a data endpoint', async () => { + vi.stubGlobal('fetch', mock401('AUTH_REQUIRED')); await expect(api.bills()).rejects.toMatchObject({ status: 401 }); expect(expiredEvents).toBe(1); }); - it('does NOT dispatch for a 401 under /auth/* (wrong credential input)', async () => { + it('does NOT dispatch for AUTH_ERROR (wrong credential) — even outside /auth/*', async () => { + vi.stubGlobal('fetch', mock401('AUTH_ERROR')); + await expect( + api.changeProfilePassword({ current_password: 'wrong', new_password: 'longenough1' }), + ).rejects.toMatchObject({ status: 401 }); await expect( api.changePassword({ current_password: 'wrong', new_password: 'longenough1' }), ).rejects.toMatchObject({ status: 401 }); diff --git a/client/api.ts b/client/api.ts index 53900a5..d664499 100644 --- a/client/api.ts +++ b/client/api.ts @@ -147,11 +147,13 @@ async function _fetch( _csrfFetch = null; return _fetch(method, path, body, true); } - // Session expired mid-use: a 401 outside /auth/* means the session cookie - // died (under /auth/* a 401 is "wrong credential input" — e.g. bad current - // password — and must NOT log the user out). AuthProvider listens and - // clears the user, so RequireAuth redirects to /login preserving state.from. - if (res.status === 401 && !path.startsWith('/auth/')) { + // Session expired mid-use: the auth middleware is the only source of + // AUTH_REQUIRED (no valid session). Wrong-credential 401s (change-password, + // TOTP disable, …) carry AUTH_ERROR and must NOT log the user out — a path + // prefix can't make that distinction (/profile/change-password proved it). + // AuthProvider listens and clears the user; RequireAuth then redirects to + // /login preserving state.from. + if (res.status === 401 && data?.code === 'AUTH_REQUIRED') { window.dispatchEvent(new Event('auth:expired')); } throw toApiError(res, data); diff --git a/client/pages/LoginPage.tsx b/client/pages/LoginPage.tsx index fcb109d..fe3d493 100644 --- a/client/pages/LoginPage.tsx +++ b/client/pages/LoginPage.tsx @@ -2,7 +2,6 @@ import { useState, useEffect, type ComponentType, type ReactNode } from 'react'; import { Link, useNavigate, useLocation } from 'react-router-dom'; import { Gauge, KeyRound, LockKeyhole, ShieldCheck } from 'lucide-react'; import { toast } from 'sonner'; -import { startAuthentication } from '@simplewebauthn/browser'; import { api } from '@/api'; import { errMessage } from '@/lib/utils'; import { useAuth, type User } from '@/hooks/useAuth'; @@ -186,19 +185,29 @@ export default function LoginPage() { setWebauthnError(''); setWebauthnLoading(true); try { + // Lazy: keep the WebAuthn lib out of the login-critical entry bundle + // (only key-protected accounts ever reach this; ProfilePage does the same). + const { startAuthentication } = await import('@simplewebauthn/browser'); const response = await startAuthentication({ optionsJSON: options as Parameters[0]['optionsJSON'], }); const data = await api.webauthnChallenge({ challenge_token: challengeToken, response }); handlePostLogin((data as { user?: User }).user!); } catch (err) { - // NotAllowedError = user cancelled / timed out at the browser prompt. + // NotAllowedError = user cancelled / timed out at the browser prompt — + // the endpoint was never called, so the single-use challenge token is + // still valid and retry works. Any SERVER failure has already consumed + // the token (anti-replay), so a retry can only ever return "Challenge + // expired" — send the user back to sign in instead of a dead button. const cancelled = (err as Error)?.name === 'NotAllowedError'; - setWebauthnError( - cancelled - ? 'Security key prompt was cancelled. Try again, or go back and sign in again.' - : errMessage(err, 'Security key verification failed.'), - ); + if (cancelled) { + setWebauthnError('Security key prompt was cancelled. Try again.'); + } else { + setWebauthnChallenge(null); + setWebauthnOptions(null); + setWebauthnError(''); + setError(errMessage(err, 'Security key verification failed.') + ' Please sign in again.'); + } } finally { setWebauthnLoading(false); } @@ -217,8 +226,14 @@ export default function LoginPage() { const data = await api.totpChallenge(payload); handlePostLogin(data.user!); } catch (err) { - setTotpError(errMessage(err, 'Invalid code.')); + // The server consumes the single-use challenge token BEFORE verifying the + // code (anti-replay), so after any server rejection a retry with the same + // token can only return "Challenge expired" — restart the sign-in instead + // of leaving a form that can never succeed. (Same semantics as WebAuthn.) + setTotpChallenge(null); setTotpCode(''); + setTotpError(''); + setError(errMessage(err, 'Invalid code.') + ' Please sign in again.'); } finally { setTotpLoading(false); } diff --git a/middleware/requireAuth.cts b/middleware/requireAuth.cts index 7eb0d12..395c506 100644 --- a/middleware/requireAuth.cts +++ b/middleware/requireAuth.cts @@ -64,7 +64,11 @@ function requireAuth(req: Req, res: Res, next: Next): any { } const user = getSessionUser(req.cookies?.[COOKIE_NAME]); - if (!user) return res.status(401).json(standardizeError('Not authenticated', 'AUTH_ERROR')); + // AUTH_REQUIRED (not AUTH_ERROR): the one code that means "no valid session". + // The client keys its session-expired redirect off this code — wrong-credential + // 401s elsewhere (change-password, TOTP disable, …) use AUTH_ERROR and must + // NOT log the user out. + if (!user) return res.status(401).json(standardizeError('Not authenticated', 'AUTH_REQUIRED')); req.user = user; next(); } diff --git a/routes/authOidc.cts b/routes/authOidc.cts index 58aa924..3f4738f 100644 --- a/routes/authOidc.cts +++ b/routes/authOidc.cts @@ -94,10 +94,12 @@ router.get('/callback', async (req: Req, res: Res) => { } try { - // Full token exchange + cryptographic ID token verification via openid-client@5. - // Verifies: state, PKCE, JWT signature (JWKS), issuer, audience, expiry, nonce. - // Throws on any validation failure. Tokens are never logged. - const claims = await exchangeAndVerifyTokens(config, code, stateId, savedState); + // Full token exchange + cryptographic ID token verification via openid-client@6. + // Verifies: state, PKCE, JWT signature (JWKS), issuer, audience, expiry, nonce, + // and the RFC 9207 `iss` response parameter — which is why the FULL callback + // query is forwarded, not just code/state. Throws on any validation failure. + // Tokens are never logged. + const claims = await exchangeAndVerifyTokens(config, code, stateId, savedState, req.query); // Map verified claims to a local user account const user = await findOrProvisionUser(claims, config); diff --git a/services/authService.cts b/services/authService.cts index 7ed27f0..22ce843 100644 --- a/services/authService.cts +++ b/services/authService.cts @@ -94,10 +94,14 @@ async function login(username: any, password: any) { const loginToken = createLoginChallenge(getDb(), user.id, challengeId); return { requires_webauthn: true, challenge_token: loginToken, webauthn_options: options }; } catch (err: any) { - // Stale flag (enabled but no usable credentials): a hard failure here would - // lock the account out entirely — fall through to password-only login instead. + // ONLY the stale-flag case (enabled but no usable credentials) may fall + // through to password-only login — a hard failure there would lock the + // account out entirely. Any other error (DB/crypto hiccup) rethrows: + // silently downgrading 2FA to password-only on a transient fault would + // let an attacker with the password ride out the second factor. + if (err.message !== 'No registered WebAuthn credentials') throw err; log.warn( - `[auth] WebAuthn enabled for user ${user.id} but challenge creation failed (${err.message}); proceeding with password login`, + `[auth] WebAuthn enabled for user ${user.id} but no usable credentials; proceeding with password login`, ); } } diff --git a/services/oidcService.cts b/services/oidcService.cts index a26efd3..98d8fcc 100644 --- a/services/oidcService.cts +++ b/services/oidcService.cts @@ -493,11 +493,17 @@ async function getOidcClient(config) { config.tokenEndpointAuthMethod === 'client_secret_post' ? oidc.ClientSecretPost(config.clientSecret) : oidc.ClientSecretBasic(config.clientSecret); + // openid-client v6 enforces HTTPS on the issuer and every discovered + // endpoint (v5 allowed http). Self-hosters commonly reach the provider over + // an internal http address (e.g. http://authentik:9000 on a Docker network), + // so OIDC_ALLOW_INSECURE_HTTP=true opts back in — deliberate and explicit. + const allowHttp = process.env.OIDC_ALLOW_INSECURE_HTTP === 'true'; _cachedClient = await oidc.discovery( new URL(config.issuerUrl), config.clientId, { redirect_uris: [config.redirectUri], response_types: ['code'] }, clientAuth, + allowHttp ? { execute: [oidc.allowInsecureRequests] } : undefined, ); _cachedClientKey = clientKey; _cacheTs = now; @@ -649,14 +655,19 @@ async function buildAuthorizationUrl(config, state) { * * Tokens are never logged. Returns verified claims on success. */ -async function exchangeAndVerifyTokens(config, code, stateId, savedState) { +async function exchangeAndVerifyTokens(config, code, stateId, savedState, callbackQuery = {}) { const client = await getOidcClient(config); // authorizationCodeGrant() handles token exchange + full validation in one // step (signature via discovered JWKS, iss/aud/exp/nbf, PKCE, state, nonce). - // It throws on any problem. The "current URL" is the callback URL as the - // provider redirected to it. + // It throws on any problem. The "current URL" must carry EVERY parameter the + // provider actually returned — v6 validates the RFC 9207 `iss` parameter when + // the provider advertises authorization_response_iss_parameter_supported + // (authentik does), so rebuilding with only code/state breaks the exchange. const currentUrl = new URL(config.redirectUri); + for (const [key, value] of Object.entries(callbackQuery)) { + if (typeof value === 'string') currentUrl.searchParams.set(key, value); + } currentUrl.searchParams.set('code', code); currentUrl.searchParams.set('state', stateId);