From 9a6cc23876b3401194eddbb2962a8fb6c48e42fd Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 20 Aug 2026 13:59:57 +0200 Subject: [PATCH] fix: refresh TOTP-minted tokens with the default client id when no OAuth client is configured #873 --- app/api/auth/token/route.ts | 17 +++--- app/api/auth/totp-token-exchange/route.ts | 7 +-- lib/__tests__/oauth-token-exchange.test.ts | 69 ++++++++++++++++++++++ lib/__tests__/oauth-token-route.test.ts | 22 +++++++ lib/oauth/token-exchange.ts | 33 ++++++++--- 5 files changed, 127 insertions(+), 21 deletions(-) create mode 100644 lib/__tests__/oauth-token-exchange.test.ts diff --git a/app/api/auth/token/route.ts b/app/api/auth/token/route.ts index 0f4c1b9cf..f173c0b3a 100644 --- a/app/api/auth/token/route.ts +++ b/app/api/auth/token/route.ts @@ -8,7 +8,7 @@ import { encodeCachedAccessToken, decodeCachedAccessToken, } from '@/lib/oauth/tokens'; -import { exchangeCodeForTokens, buildOAuthParams, getMetadata, getTokenEndpoint } from '@/lib/oauth/token-exchange'; +import { exchangeCodeForTokens, buildOAuthParams, getMetadata, getTokenEndpoint, DEFAULT_CLIENT_ID } from '@/lib/oauth/token-exchange'; import { getCookieOptions } from '@/lib/oauth/cookie-config'; import { MAX_ACCOUNT_SLOTS } from '@/lib/account-utils'; @@ -117,12 +117,15 @@ export async function PUT(request: NextRequest) { } } - const tokenEndpoint = await getTokenEndpoint(serverId); + // The refresh token may have been minted by the password+TOTP login route, + // which works without a configured OAuth client by falling back to the + // default client id - refreshing must fall back the same way (#873). + const tokenEndpoint = await getTokenEndpoint(serverId, { fallbackClientId: DEFAULT_CLIENT_ID }); const params = buildOAuthParams({ grant_type: 'refresh_token', refresh_token: refreshToken, - }, serverId); + }, serverId, { fallbackClientId: DEFAULT_CLIENT_ID }); const tokenResponse = await fetch(tokenEndpoint, { method: 'POST', @@ -185,9 +188,9 @@ export async function DELETE(request: NextRequest) { if (token) { // Best-effort revocation try { - const metadata = await getMetadata(slotServerId).catch(() => null); + const metadata = await getMetadata(slotServerId, { fallbackClientId: DEFAULT_CLIENT_ID }).catch(() => null); if (metadata?.revocation_endpoint) { - const params = buildOAuthParams({ token, token_type_hint: 'refresh_token' }, slotServerId); + const params = buildOAuthParams({ token, token_type_hint: 'refresh_token' }, slotServerId, { fallbackClientId: DEFAULT_CLIENT_ID }); await fetch(metadata.revocation_endpoint, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, @@ -208,7 +211,7 @@ export async function DELETE(request: NextRequest) { const cookieStore = await cookies(); const refreshToken = cookieStore.get(cookieName)?.value; const slotServerId = cookieStore.get(refreshTokenServerCookieName(slot))?.value || null; - const metadata = await getMetadata(slotServerId).catch((err) => { + const metadata = await getMetadata(slotServerId, { fallbackClientId: DEFAULT_CLIENT_ID }).catch((err) => { logger.warn('Failed to discover OAuth metadata during logout', { error: err instanceof Error ? err.message : 'Unknown error', }); @@ -220,7 +223,7 @@ export async function DELETE(request: NextRequest) { const params = buildOAuthParams({ token: refreshToken, token_type_hint: 'refresh_token', - }, slotServerId); + }, slotServerId, { fallbackClientId: DEFAULT_CLIENT_ID }); try { const revocationResponse = await fetch(metadata.revocation_endpoint, { diff --git a/app/api/auth/totp-token-exchange/route.ts b/app/api/auth/totp-token-exchange/route.ts index 6e5b87c41..8c41f14cc 100644 --- a/app/api/auth/totp-token-exchange/route.ts +++ b/app/api/auth/totp-token-exchange/route.ts @@ -10,6 +10,7 @@ import { recordLogin } from '@/lib/telemetry/login-tracker'; import { parseJmapServers, findServerByUrl, findServerById } from '@/lib/admin/jmap-servers'; import { MAX_ACCOUNT_SLOTS } from '@/lib/account-utils'; import { generateCodeVerifier, generateCodeChallenge } from '@/lib/oauth/pkce'; +import { DEFAULT_CLIENT_ID } from '@/lib/oauth/token-exchange'; /** * Exchange a password + (optional) TOTP code for OAuth tokens. @@ -31,12 +32,6 @@ import { generateCodeVerifier, generateCodeChallenge } from '@/lib/oauth/pkce'; * the (≈30s) code in every request. */ -// Fallback OAuth client id used when no client is configured. Stalwart accepts -// any client id unless `require_client_registration` is enabled (default off); -// when it is enabled the admin must configure `oauthClientId` with this -// redirect URI registered. -const DEFAULT_CLIENT_ID = 'bulwark-webmail'; - interface LoginResult { type?: string; // The response keeps snake_case: only the LoginResponse variant *tags* are diff --git a/lib/__tests__/oauth-token-exchange.test.ts b/lib/__tests__/oauth-token-exchange.test.ts new file mode 100644 index 000000000..2c5635fac --- /dev/null +++ b/lib/__tests__/oauth-token-exchange.test.ts @@ -0,0 +1,69 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +vi.mock('@/lib/logger', () => ({ + logger: { warn: vi.fn(), error: vi.fn(), debug: vi.fn(), info: vi.fn() }, +})); + +vi.mock('@/lib/oauth/discovery', () => ({ + discoverOAuth: vi.fn(), +})); + +vi.mock('@/lib/security/url-guard', () => ({ + isPublicHttpUrl: vi.fn(), +})); + +vi.mock('@/lib/read-file-env', () => ({ + readFileEnv: () => '', +})); + +// No admin config set - every lookup falls through to its default, so the +// env vars stubbed per-test are the only configuration source. +vi.mock('@/lib/admin/config-manager', () => ({ + configManager: { + get: (_key: string, def: unknown) => def, + ensureLoaded: async () => {}, + }, +})); + +import { getRequiredConfig, buildOAuthParams, DEFAULT_CLIENT_ID } from '@/lib/oauth/token-exchange'; + +describe('token-exchange client id fallback (#873)', () => { + beforeEach(() => { + vi.stubEnv('JMAP_SERVER_URL', 'https://mail.example.com'); + vi.stubEnv('OAUTH_CLIENT_ID', ''); + vi.stubEnv('OAUTH_ISSUER_URL', ''); + vi.stubEnv('OAUTH_CLIENT_SECRET', ''); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it('still fails loudly without a fallback so OAuth-initiating flows surface misconfiguration', () => { + expect(() => getRequiredConfig(null)).toThrow(/OAUTH_CLIENT_ID/); + }); + + it('uses the fallback client id when no client is configured', () => { + const config = getRequiredConfig(null, { fallbackClientId: DEFAULT_CLIENT_ID }); + expect(config.clientId).toBe('bulwark-webmail'); + // Discovery must target the mail server itself - that is where the TOTP + // login route minted the token. + expect(config.discoveryUrl).toBe('https://mail.example.com'); + }); + + it('prefers a configured client id over the fallback', () => { + vi.stubEnv('OAUTH_CLIENT_ID', 'configured-client'); + const config = getRequiredConfig(null, { fallbackClientId: DEFAULT_CLIENT_ID }); + expect(config.clientId).toBe('configured-client'); + }); + + it('builds refresh params with the fallback client id', () => { + const params = buildOAuthParams( + { grant_type: 'refresh_token', refresh_token: 'rt' }, + null, + { fallbackClientId: DEFAULT_CLIENT_ID }, + ); + expect(params.get('client_id')).toBe('bulwark-webmail'); + expect(params.get('grant_type')).toBe('refresh_token'); + }); +}); diff --git a/lib/__tests__/oauth-token-route.test.ts b/lib/__tests__/oauth-token-route.test.ts index 43a9ca748..5af602b7f 100644 --- a/lib/__tests__/oauth-token-route.test.ts +++ b/lib/__tests__/oauth-token-route.test.ts @@ -26,6 +26,7 @@ vi.mock('@/lib/oauth/token-exchange', () => ({ buildOAuthParams: (...args: unknown[]) => buildOAuthParams(...args), getMetadata: vi.fn().mockResolvedValue(null), getTokenEndpoint: (...args: unknown[]) => getTokenEndpoint(...args), + DEFAULT_CLIENT_ID: 'bulwark-webmail', })); /** Minimal in-memory stand-in for the Next.js cookie store. */ @@ -218,6 +219,27 @@ describe('oauth token route - access token cache (#552)', () => { expect(cookieStore.deleted).not.toContain('jmap_rt'); }); + it('refreshes with the default client id fallback so TOTP sessions survive reloads (#873)', async () => { + cookieStore.set('jmap_rt', 'totp-minted-refresh-token'); + fetchMock.mockResolvedValue({ + ok: true, + json: async () => ({ access_token: 'fresh-token', expires_in: 1800 }), + }); + + const { status } = await callPut({ force: 'true' }); + + expect(status).toBe(200); + // Tokens minted by the TOTP login route exist even when no OAuth client is + // configured; the refresh must offer the same default client id instead of + // failing on the missing OAUTH_CLIENT_ID. + expect(getTokenEndpoint).toHaveBeenCalledWith(null, { fallbackClientId: 'bulwark-webmail' }); + expect(buildOAuthParams).toHaveBeenCalledWith( + { grant_type: 'refresh_token', refresh_token: 'totp-minted-refresh-token' }, + null, + { fallbackClientId: 'bulwark-webmail' }, + ); + }); + it('does not serve a cached token once the refresh token is gone', async () => { seedCachedToken('orphan-token', 1500); diff --git a/lib/oauth/token-exchange.ts b/lib/oauth/token-exchange.ts index 51fe2d213..ba3bdcb93 100644 --- a/lib/oauth/token-exchange.ts +++ b/lib/oauth/token-exchange.ts @@ -6,6 +6,23 @@ import { readFileEnv } from '@/lib/read-file-env'; import { configManager } from '@/lib/admin/config-manager'; import { parseJmapServers, findServerById } from '@/lib/admin/jmap-servers'; +// Fallback OAuth client id used when no client is configured. The password+TOTP +// login route mints tokens against the mail server's built-in OAuth with this +// id (Stalwart accepts any client id unless `require_client_registration` is +// enabled), so refreshing/revoking those tokens must fall back to the same id +// instead of failing on the missing OAUTH_CLIENT_ID (#873). +export const DEFAULT_CLIENT_ID = 'bulwark-webmail'; + +export interface ClientConfigOptions { + /** + * Client id to use when none is configured. Only pass this for operations on + * tokens that may have been minted by the TOTP login fallback; flows that + * initiate OAuth (authorize URL, code exchange, SSO) must keep failing loudly + * so a misconfiguration surfaces at login rather than as a broken session. + */ + fallbackClientId?: string; +} + // SSRF guard for OAuth discovery. When `oauthAllowPrivateEndpoints` is set, // the admin opts in to discovery resolving to RFC-1918 / loopback hosts — // required for split-DNS deployments where the JMAP server's public hostname @@ -34,14 +51,14 @@ function getServerEntry(serverId?: string | null) { return findServerById(servers, serverId); } -export function getRequiredConfig(serverId?: string | null) { +export function getRequiredConfig(serverId?: string | null, options?: ClientConfigOptions) { const entry = getServerEntry(serverId); const globalClientId = configManager.get('oauthClientId', '') || process.env.OAUTH_CLIENT_ID; const globalServerUrl = configManager.get('jmapServerUrl', '') || process.env.JMAP_SERVER_URL || process.env.NEXT_PUBLIC_JMAP_SERVER_URL; const globalIssuerUrl = configManager.get('oauthIssuerUrl', '') || process.env.OAUTH_ISSUER_URL; - const clientId = entry?.oauth?.clientId || globalClientId; + const clientId = entry?.oauth?.clientId || globalClientId || options?.fallbackClientId; const serverUrl = entry?.url || globalServerUrl; const issuerUrl = entry?.oauth?.issuerUrl || globalIssuerUrl; @@ -61,8 +78,8 @@ function getClientSecret(serverId?: string | null): string { return getGlobalClientSecret(); } -export async function getTokenEndpoint(serverId?: string | null): Promise { - const { discoveryUrl } = getRequiredConfig(serverId); +export async function getTokenEndpoint(serverId?: string | null, options?: ClientConfigOptions): Promise { + const { discoveryUrl } = getRequiredConfig(serverId, options); const metadata = await discoverOAuth(discoveryUrl, { validateEndpoint: getDiscoveryValidator() }); if (!metadata?.token_endpoint) { throw new Error('OAuth token endpoint not found'); @@ -70,13 +87,13 @@ export async function getTokenEndpoint(serverId?: string | null): Promise { - const { discoveryUrl } = getRequiredConfig(serverId); +export async function getMetadata(serverId?: string | null, options?: ClientConfigOptions): Promise { + const { discoveryUrl } = getRequiredConfig(serverId, options); return discoverOAuth(discoveryUrl, { validateEndpoint: getDiscoveryValidator() }); } -export function buildOAuthParams(base: Record, serverId?: string | null): URLSearchParams { - const { clientId } = getRequiredConfig(serverId); +export function buildOAuthParams(base: Record, serverId?: string | null, options?: ClientConfigOptions): URLSearchParams { + const { clientId } = getRequiredConfig(serverId, options); const params = new URLSearchParams({ ...base, client_id: clientId }); const secret = getClientSecret(serverId); if (secret) {