From 7b6b8fc132016a7d7731f7118206b694c470df94 Mon Sep 17 00:00:00 2001 From: Matthieu MALVACHE Date: Thu, 26 Feb 2026 00:08:57 +0100 Subject: [PATCH] feat(auth): add RP-initiated logout and OAuth unit tests OAuth logout now terminates the IdP session via end_session_endpoint with HTTPS-only URL validation. Adds 14 unit tests for PKCE and OAuth discovery. --- README.md | 2 +- ROADMAP.md | 4 +- app/api/auth/token/route.ts | 32 ++++++-- lib/__tests__/oauth-discovery.test.ts | 108 ++++++++++++++++++++++++++ lib/__tests__/oauth-pkce.test.ts | 61 +++++++++++++++ stores/auth-store.ts | 27 +++++-- 6 files changed, 218 insertions(+), 16 deletions(-) create mode 100644 lib/__tests__/oauth-discovery.test.ts create mode 100644 lib/__tests__/oauth-pkce.test.ts diff --git a/README.md b/README.md index 987e1b19..4c6dcb4c 100644 --- a/README.md +++ b/README.md @@ -117,7 +117,7 @@ This webmail client is designed to work seamlessly with [**Stalwart Mail Server* - SPF/DKIM/DMARC status indicators - No password storage (session-based auth) - TOTP two-factor authentication support -- OAuth2/OIDC with PKCE for SSO login (opt-in, Basic Auth remains default) +- OAuth2/OIDC with PKCE for SSO login (opt-in, RP-initiated logout, Basic Auth remains default) - External IdP support (Keycloak, Authentik) via configurable issuer URL - Session persistence via httpOnly refresh token cookies - CORS misconfiguration detection with actionable error messages diff --git a/ROADMAP.md b/ROADMAP.md index cac206fd..e172c8d9 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -18,7 +18,7 @@ This document tracks the development status and planned features for JMAP Webmai - [x] Authentication error handling - [x] JMAP identities for sender address - [x] TOTP two-factor authentication (Stalwart-compatible) -- [x] OAuth2/OIDC with PKCE (opt-in SSO, session persistence via httpOnly refresh tokens) +- [x] OAuth2/OIDC with PKCE (opt-in SSO, session persistence, RP-initiated logout) - [x] External IdP support via explicit issuer URL (Keycloak, Authentik, etc.) ### JMAP Server Connection @@ -220,6 +220,7 @@ This document tracks the development status and planned features for JMAP Webmai - [x] Unit tests for calendar invitation parsing (25 tests) - [x] Unit tests for calendar participants (26 tests) - [x] Unit tests for template utilities (48 tests) +- [x] Unit tests for OAuth PKCE and discovery (14 tests) - [x] XSS attack vector testing - [x] Playwright E2E framework setup @@ -237,7 +238,6 @@ This document tracks the development status and planned features for JMAP Webmai - [ ] Free/busy queries (Principal/getAvailability) - [ ] Calendar sharing UI (JMAP Sharing RFC 9670) - [ ] Email encryption (PGP/GPG) -- [ ] OAuth2 token introspection and userinfo endpoint support ### Performance Optimizations - [ ] Email content caching diff --git a/app/api/auth/token/route.ts b/app/api/auth/token/route.ts index ed00f569..f67a536a 100644 --- a/app/api/auth/token/route.ts +++ b/app/api/auth/token/route.ts @@ -37,10 +37,9 @@ async function getTokenEndpoint(): Promise { return metadata.token_endpoint; } -async function getRevocationEndpoint(): Promise { +async function getMetadata(): Promise { const { discoveryUrl } = getRequiredConfig(); - const metadata = await discoverOAuth(discoveryUrl); - return metadata?.revocation_endpoint || null; + return discoverOAuth(discoveryUrl); } function buildOAuthParams(base: Record): URLSearchParams { @@ -159,17 +158,22 @@ export async function DELETE() { try { const cookieStore = await cookies(); const refreshToken = cookieStore.get(REFRESH_TOKEN_COOKIE)?.value; + const metadata = await getMetadata().catch((err) => { + logger.warn('Failed to discover OAuth metadata during logout', { + error: err instanceof Error ? err.message : 'Unknown error', + }); + return null; + }); if (refreshToken) { - const revocationEndpoint = await getRevocationEndpoint(); - if (revocationEndpoint) { + if (metadata?.revocation_endpoint) { const params = buildOAuthParams({ token: refreshToken, token_type_hint: 'refresh_token', }); try { - const revocationResponse = await fetch(revocationEndpoint, { + const revocationResponse = await fetch(metadata.revocation_endpoint, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, body: params.toString(), @@ -185,7 +189,21 @@ export async function DELETE() { cookieStore.delete(REFRESH_TOKEN_COOKIE); } - return NextResponse.json({ ok: true }); + let end_session_url: string | undefined; + if (metadata?.end_session_endpoint) { + try { + const parsed = new URL(metadata.end_session_endpoint); + if (parsed.protocol === 'https:') { + end_session_url = metadata.end_session_endpoint; + } else { + logger.warn('Ignoring non-HTTPS end_session_endpoint', { url: metadata.end_session_endpoint }); + } + } catch { + logger.warn('Invalid end_session_endpoint URL', { url: metadata.end_session_endpoint }); + } + } + + return NextResponse.json({ ok: true, ...(end_session_url && { end_session_url }) }); } catch (error) { logger.error('Token revocation error', { error: error instanceof Error ? error.message : 'Unknown error' }); return NextResponse.json({ error: 'Internal server error' }, { status: 500 }); diff --git a/lib/__tests__/oauth-discovery.test.ts b/lib/__tests__/oauth-discovery.test.ts new file mode 100644 index 00000000..0d2f6d61 --- /dev/null +++ b/lib/__tests__/oauth-discovery.test.ts @@ -0,0 +1,108 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { OAuthMetadata } from '../oauth/discovery'; + +const VALID_METADATA: OAuthMetadata = { + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/authorize', + token_endpoint: 'https://auth.example.com/token', + revocation_endpoint: 'https://auth.example.com/revoke', + end_session_endpoint: 'https://auth.example.com/logout', +}; + +describe('oauth/discovery', () => { + let discoverOAuth: typeof import('../oauth/discovery').discoverOAuth; + + beforeEach(async () => { + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + vi.resetModules(); + const mod = await import('../oauth/discovery'); + discoverOAuth = mod.discoverOAuth; + }); + + it('discovers metadata from oauth-authorization-server', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve(VALID_METADATA), + })); + + const result = await discoverOAuth('https://mail.example.com'); + + expect(result).toEqual(VALID_METADATA); + expect(fetch).toHaveBeenCalledTimes(1); + expect(fetch).toHaveBeenCalledWith( + 'https://mail.example.com/.well-known/oauth-authorization-server' + ); + }); + + it('falls back to openid-configuration when first returns 404', async () => { + vi.stubGlobal('fetch', vi.fn() + .mockResolvedValueOnce({ ok: false, status: 404 }) + .mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve(VALID_METADATA), + })); + + const result = await discoverOAuth('https://fallback.example.com'); + + expect(result).toEqual(VALID_METADATA); + expect(fetch).toHaveBeenCalledTimes(2); + expect(fetch).toHaveBeenNthCalledWith( + 2, + 'https://fallback.example.com/.well-known/openid-configuration' + ); + }); + + it('returns null when both endpoints fail', async () => { + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + vi.stubGlobal('fetch', vi.fn() + .mockResolvedValueOnce({ ok: false, status: 404 }) + .mockResolvedValueOnce({ ok: false, status: 404 })); + + const result = await discoverOAuth('https://fail.example.com'); + + expect(result).toBeNull(); + expect(consoleSpy).toHaveBeenCalled(); + }); + + it('parses optional fields (revocation_endpoint, end_session_endpoint)', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve(VALID_METADATA), + })); + + const result = await discoverOAuth('https://optional.example.com'); + + expect(result?.revocation_endpoint).toBe('https://auth.example.com/revoke'); + expect(result?.end_session_endpoint).toBe('https://auth.example.com/logout'); + }); + + it('returns null when required fields (authorization_endpoint, token_endpoint) are missing', async () => { + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + vi.stubGlobal('fetch', vi.fn() + .mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve({ issuer: 'https://auth.example.com' }), + }) + .mockResolvedValueOnce({ ok: false, status: 404 })); + + const result = await discoverOAuth('https://incomplete.example.com'); + + expect(result).toBeNull(); + expect(consoleSpy).toHaveBeenCalled(); + }); + + it('caches results — second call for same server URL does not re-fetch', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve(VALID_METADATA), + })); + + const first = await discoverOAuth('https://cached.example.com'); + const second = await discoverOAuth('https://cached.example.com'); + + expect(first).toEqual(VALID_METADATA); + expect(second).toEqual(VALID_METADATA); + expect(fetch).toHaveBeenCalledTimes(1); + }); +}); diff --git a/lib/__tests__/oauth-pkce.test.ts b/lib/__tests__/oauth-pkce.test.ts new file mode 100644 index 00000000..263565e5 --- /dev/null +++ b/lib/__tests__/oauth-pkce.test.ts @@ -0,0 +1,61 @@ +import { describe, it, expect } from 'vitest'; +import { generateCodeVerifier, generateCodeChallenge, generateState } from '../oauth/pkce'; + +describe('oauth/pkce', () => { + describe('generateCodeVerifier', () => { + it('returns a 43-character base64url string', () => { + const verifier = generateCodeVerifier(); + expect(verifier).toHaveLength(43); + expect(verifier).toMatch(/^[A-Za-z0-9_-]+$/); + }); + + it('contains no base64 padding or unsafe characters', () => { + for (let i = 0; i < 20; i++) { + const verifier = generateCodeVerifier(); + expect(verifier).not.toMatch(/[+/=]/); + } + }); + + it('generates unique values', () => { + const a = generateCodeVerifier(); + const b = generateCodeVerifier(); + expect(a).not.toBe(b); + }); + }); + + describe('generateCodeChallenge', () => { + it('returns a base64url string different from the verifier', async () => { + const verifier = generateCodeVerifier(); + const challenge = await generateCodeChallenge(verifier); + expect(challenge).toMatch(/^[A-Za-z0-9_-]+$/); + expect(challenge).not.toBe(verifier); + }); + + it('produces the RFC 7636 Appendix B test vector', async () => { + const verifier = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'; + const challenge = await generateCodeChallenge(verifier); + expect(challenge).toBe('E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM'); + }); + + it('is deterministic for the same verifier', async () => { + const verifier = generateCodeVerifier(); + const a = await generateCodeChallenge(verifier); + const b = await generateCodeChallenge(verifier); + expect(a).toBe(b); + }); + }); + + describe('generateState', () => { + it('returns a 43-character base64url string', () => { + const state = generateState(); + expect(state).toHaveLength(43); + expect(state).toMatch(/^[A-Za-z0-9_-]+$/); + }); + + it('generates unique values per call', () => { + const a = generateState(); + const b = generateState(); + expect(a).not.toBe(b); + }); + }); +}); diff --git a/stores/auth-store.ts b/stores/auth-store.ts index b1a35d00..3e45f22f 100644 --- a/stores/auth-store.ts +++ b/stores/auth-store.ts @@ -259,12 +259,7 @@ export const useAuthStore = create()( logout: () => { const state = get(); - - if (state.authMode === 'oauth') { - fetch('/api/auth/token', { method: 'DELETE' }).catch((err) => { - debug.error('Token revocation failed:', err); - }); - } + const wasOAuth = state.authMode === 'oauth'; clearRefreshTimer(); state.client?.disconnect(); @@ -300,6 +295,26 @@ export const useAuthStore = create()( useVacationStore.getState().clearState(); useCalendarStore.getState().clearState(); useFilterStore.getState().clearState(); + + if (wasOAuth) { + fetch('/api/auth/token', { method: 'DELETE' }) + .then((res) => { + if (!res.ok) throw new Error(`Revocation failed: ${res.status}`); + return res.json(); + }) + .then((data) => { + if (data.end_session_url) { + const locale = window.location.pathname.split('/')[1] || 'en'; + const redirectUri = `${window.location.origin}/${locale}/login`; + const url = new URL(data.end_session_url); + url.searchParams.set('post_logout_redirect_uri', redirectUri); + window.location.href = url.toString(); + } + }) + .catch((err) => { + debug.error('OAuth logout cleanup failed:', err); + }); + } }, checkAuth: async () => {