From b26c1746289b9fa3debcb69f105fe3ef6c262cc6 Mon Sep 17 00:00:00 2001 From: Justin Willhite Date: Tue, 8 Sep 2026 19:11:31 +0000 Subject: [PATCH] fix(oauth): normalize the discovery base so a session JMAP_SERVER_URL refreshes (#971) OAuth authorization-server metadata lives at the origin root (RFC 8414), but getRequiredConfig() derived the discovery URL straight from JMAP_SERVER_URL. When that is the session URL (for example https://mail.example.com/jmap/session, a common setup so the client can skip the 307 redirect a preflighted CORS request cannot follow), discoverOAuth fetched https://mail.example.com/jmap/session/.well-known/oauth-authorization-server, got a 404, and token refresh failed with "OAuth token endpoint not found", so the session was dropped after one access-token lifetime. Strip a trailing session path (/jmap/session or /.well-known/jmap, plus any query or fragment) before discovery. A plain server root is unchanged, and only the session suffix is removed, so a real base path prefix is kept. Adds oauth-discovery-base.test.ts driving getRequiredConfig; the normalization cases fail without the fix. --- lib/__tests__/oauth-discovery-base.test.ts | 79 ++++++++++++++++++++++ lib/oauth/token-exchange.ts | 28 +++++++- 2 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 lib/__tests__/oauth-discovery-base.test.ts diff --git a/lib/__tests__/oauth-discovery-base.test.ts b/lib/__tests__/oauth-discovery-base.test.ts new file mode 100644 index 000000000..5d1c8937c --- /dev/null +++ b/lib/__tests__/oauth-discovery-base.test.ts @@ -0,0 +1,79 @@ +import { describe, it, expect, beforeEach, afterEach, vi } 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: () => '', +})); + +vi.mock('@/lib/admin/config-manager', () => ({ + configManager: { + get: (_key: string, def: unknown) => def, + ensureLoaded: async () => {}, + }, +})); + +import { getRequiredConfig } from '@/lib/oauth/token-exchange'; + +// OAuth discovery metadata lives at the origin root (RFC 8414), but JMAP_SERVER_URL +// is often the session URL. Discovery must normalize to the root, or +// discoverOAuth fetches `/.well-known/oauth-authorization-server` (404) +// and refresh fails with "OAuth token endpoint not found". (#971) +describe('OAuth discovery base normalization (#971)', () => { + beforeEach(() => { + vi.stubEnv('OAUTH_CLIENT_ID', 'client'); + vi.stubEnv('OAUTH_ISSUER_URL', ''); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + const discoveryFor = (serverUrl: string) => { + vi.stubEnv('JMAP_SERVER_URL', serverUrl); + return getRequiredConfig().discoveryUrl; + }; + + it('strips a trailing /jmap/session so discovery hits the server root', () => { + expect(discoveryFor('https://mail.example.com/jmap/session')).toBe('https://mail.example.com'); + }); + + it('strips a trailing /.well-known/jmap', () => { + expect(discoveryFor('https://mail.example.com/.well-known/jmap')).toBe('https://mail.example.com'); + }); + + it('strips a session path that has a trailing slash', () => { + expect(discoveryFor('https://mail.example.com/jmap/session/')).toBe('https://mail.example.com'); + }); + + it('drops query and fragment', () => { + expect(discoveryFor('https://mail.example.com/jmap/session?slot=0#x')).toBe('https://mail.example.com'); + }); + + it('leaves a plain server root unchanged', () => { + expect(discoveryFor('https://mail.example.com')).toBe('https://mail.example.com'); + }); + + it('only strips the session suffix, keeping any base path prefix', () => { + expect(discoveryFor('https://mail.example.com/api/jmap/session')).toBe('https://mail.example.com/api'); + expect(discoveryFor('https://mail.example.com/mail')).toBe('https://mail.example.com/mail'); + }); + + it('does not strip a path-based OAuth issuer (would break e.g. a Keycloak realm)', () => { + // Discovery also runs against OAUTH_ISSUER_URL, which takes precedence. A + // path-based issuer must keep its path, so this is not normalized to the origin. + vi.stubEnv('JMAP_SERVER_URL', 'https://mail.example.com/jmap/session'); + vi.stubEnv('OAUTH_ISSUER_URL', 'https://kc.example.com/realms/mail'); + expect(getRequiredConfig().discoveryUrl).toBe('https://kc.example.com/realms/mail'); + }); +}); diff --git a/lib/oauth/token-exchange.ts b/lib/oauth/token-exchange.ts index cbcbd4bf2..b66f4d1c5 100644 --- a/lib/oauth/token-exchange.ts +++ b/lib/oauth/token-exchange.ts @@ -51,6 +51,32 @@ function getServerEntry(serverId?: string | null) { return findServerById(servers, serverId); } +/** + * Strip a trailing JMAP session path so OAuth discovery hits the right base. + * + * `discoverOAuth` appends `/.well-known/...` to this base, so JMAP_SERVER_URL set to + * the session URL (`.../jmap/session` or `.../.well-known/jmap`, common so the client + * can skip the 307 redirect a preflighted CORS request cannot follow) makes discovery + * fetch `/.well-known/oauth-authorization-server`, get a 404, and refresh + * fail with "OAuth token endpoint not found". + * + * Only the session suffix (and any query or fragment) is removed, not the whole path: + * discovery also runs against OAUTH_ISSUER_URL, and a path-based issuer (for example a + * Keycloak realm at `/realms/`) must keep its path. So this is deliberately not + * `new URL(url).origin`. (#971) + */ +function discoveryBase(url: string): string { + try { + const u = new URL(url); + u.pathname = u.pathname.replace(/\/(?:jmap\/session|\.well-known\/jmap)\/*$/, ''); + u.search = ''; + u.hash = ''; + return u.toString().replace(/\/+$/, ''); + } catch { + return url; + } +} + export function getRequiredConfig(serverId?: string | null, options?: ClientConfigOptions) { const entry = getServerEntry(serverId); @@ -69,7 +95,7 @@ export function getRequiredConfig(serverId?: string | null, options?: ClientConf if (!clientId || !serverUrl) { throw new Error(`OAuth misconfigured: ${[!clientId && 'OAUTH_CLIENT_ID', !serverUrl && 'JMAP_SERVER_URL'].filter(Boolean).join(', ')} not set`); } - const discoveryUrl = issuerUrl?.trim() || serverUrl; + const discoveryUrl = discoveryBase(issuerUrl?.trim() || serverUrl); if (issuerUrl !== undefined && issuerUrl !== '' && !issuerUrl.trim()) { logger.warn('OAUTH_ISSUER_URL is set but empty, falling back to JMAP_SERVER_URL for discovery'); }