diff --git a/app/api/auth/sso/complete/route.ts b/app/api/auth/sso/complete/route.ts index 588f74b63..b26c73501 100644 --- a/app/api/auth/sso/complete/route.ts +++ b/app/api/auth/sso/complete/route.ts @@ -95,7 +95,7 @@ export async function POST(request: NextRequest) { } else { cookieStore.delete(serverCookieName); } - storeIdToken(cookieStore, slot, tokens.id_token, request.nextUrl.basePath); + storeIdToken(cookieStore, slot, tokens.id_token, request.nextUrl.basePath, [tokens.refresh_token, pendingServerId]); } // Delete pending cookie diff --git a/app/api/auth/token/route.ts b/app/api/auth/token/route.ts index 4eda69f4c..0a55ef72f 100644 --- a/app/api/auth/token/route.ts +++ b/app/api/auth/token/route.ts @@ -90,21 +90,23 @@ type CookieStore = Awaited>; * * Scoped to the token's own lifetime - once it expires the cookie is worthless * and should not linger. A token too large to store is simply not cached. + * Returns the cookie value written, or null. */ function cacheAccessToken( cookieStore: CookieStore, slot: number, accessToken: string, expiresIn: number, -): void { +): string | null { const name = accessTokenCookieName(slot); const value = encodeCachedAccessToken(accessToken, expiresIn); if (!value) { // Oversized token: drop any stale entry rather than leaving a mismatch. cookieStore.delete(name); - return; + return null; } cookieStore.set(name, value, { ...getCookieOptions(), maxAge: expiresIn }); + return value; } export async function POST(request: NextRequest) { @@ -134,8 +136,8 @@ export async function POST(request: NextRequest) { const cookieName = refreshTokenCookieName(slot); cookieStore.set(cookieName, tokens.refresh_token, getCookieOptions()); } - cacheAccessToken(cookieStore, slot, tokens.access_token, tokens.expires_in || 3600); - storeIdToken(cookieStore, slot, tokens.id_token, request.nextUrl.basePath); + const cachedAccessToken = cacheAccessToken(cookieStore, slot, tokens.access_token, tokens.expires_in || 3600); + storeIdToken(cookieStore, slot, tokens.id_token, request.nextUrl.basePath, [tokens.refresh_token, cachedAccessToken, serverId]); // Persist which server entry minted this refresh token so the PUT/DELETE // handlers can route the refresh/revocation calls to the right token // endpoint without the client having to track it across page loads. @@ -234,11 +236,11 @@ export async function PUT(request: NextRequest) { } const expiresIn = tokens.expires_in || 3600; - cacheAccessToken(cookieStore, slot, tokens.access_token, expiresIn); + const cachedAccessToken = cacheAccessToken(cookieStore, slot, tokens.access_token, expiresIn); // Providers may reissue the id token on refresh. Only a slot signed in // through the provider keeps one; a password login's slot gains none here. if (typeof tokens.id_token === 'string' && cookieStore.get(idTokenCookieName(slot))) { - storeIdToken(cookieStore, slot, tokens.id_token, request.nextUrl.basePath); + storeIdToken(cookieStore, slot, tokens.id_token, request.nextUrl.basePath, [tokens.refresh_token, cachedAccessToken]); } return NextResponse.json({ diff --git a/lib/__tests__/oauth-token-route.test.ts b/lib/__tests__/oauth-token-route.test.ts index 59af7dc20..384702840 100644 --- a/lib/__tests__/oauth-token-route.test.ts +++ b/lib/__tests__/oauth-token-route.test.ts @@ -326,6 +326,34 @@ describe('oauth token route - sign-in keeps the id token (#905)', () => { expect(cookieStore.get('jmap_idt')).toBeUndefined(); }); + + // Sizes of a stock Keycloak realm's tokens. + it('stores the id token beside tokens of a typical size', async () => { + exchangeCodeForTokens.mockResolvedValue({ + access_token: 'a'.repeat(1130), expires_in: 300, refresh_token: 'r'.repeat(650), id_token: 'i'.repeat(1080), + }); + + await callRoute('POST', {}, { code: 'c', code_verifier: 'v', redirect_uri: 'https://mail.example.com/en/auth/callback' }); + + expect(cookieStore.get('jmap_idt')?.value).toBe('i'.repeat(1080)); + }); + + it('leaves the id token out when the response would outgrow a proxy header buffer (#1096)', async () => { + // A Keycloak realm with roles and a groups claim: together the three + // cookies exceed nginx's default 4 KB proxy_buffer_size, and the + // sign-in failed with 502. + exchangeCodeForTokens.mockResolvedValue({ + access_token: 'a'.repeat(1700), expires_in: 300, refresh_token: 'r'.repeat(650), id_token: 'i'.repeat(1300), + }); + cookieStore.set('jmap_idt', 'stale'); + + const { status } = await callRoute('POST', {}, { code: 'c', code_verifier: 'v', redirect_uri: 'https://mail.example.com/en/auth/callback' }); + + expect(status).toBe(200); + expect(cookieStore.get('jmap_idt')).toBeUndefined(); + expect(cookieStore.get('jmap_rt')?.value).toBe('r'.repeat(650)); + expect(cookieStore.get('jmap_at')?.value).toMatch(/^\d+\.a+$/); + }); }); describe('oauth token route - refresh keeps the id token current (#905)', () => { @@ -352,6 +380,23 @@ describe('oauth token route - refresh keeps the id token current (#905)', () => expect(cookieStore.get('jmap_idt')?.value).toBe(ID_TOKEN); }); + it('forgets the id token when a reissued one would outgrow a proxy header buffer (#1096)', async () => { + cookieStore.set('jmap_rt', 'rt'); + cookieStore.set('jmap_idt', ID_TOKEN); + fetchMock.mockResolvedValue({ + ok: true, + json: async () => ({ + access_token: 'a'.repeat(1700), expires_in: 300, refresh_token: 'r'.repeat(650), id_token: 'i'.repeat(1300), + }), + }); + + const { status } = await callPut({ force: 'true' }); + + expect(status).toBe(200); + expect(cookieStore.get('jmap_idt')).toBeUndefined(); + expect(cookieStore.get('jmap_rt')?.value).toBe('r'.repeat(650)); + }); + it('does not start keeping an id token for a password sign-in', async () => { cookieStore.set('jmap_rt', 'rt'); fetchMock.mockResolvedValue({ ok: true, json: async () => ({ access_token: 'at', expires_in: 300, id_token: ID_TOKEN }) }); diff --git a/lib/oauth/end-session.ts b/lib/oauth/end-session.ts index e6a0467f0..984ce2040 100644 --- a/lib/oauth/end-session.ts +++ b/lib/oauth/end-session.ts @@ -16,11 +16,25 @@ import { getCookieOptions } from '@/lib/oauth/cookie-config'; export const ID_TOKEN_COOKIE = 'jmap_idt'; /** - * Upper bound for a stored id token. Browsers cap a cookie at ~4096 bytes - * including name and attributes; a larger token is not stored, and sign-out - * falls back to identifying the client by `client_id` alone. + * Most bytes the Set-Cookie headers of a response may take for it to also + * carry the id token. nginx holds an upstream's response headers in + * `proxy_buffer_size`, one memory page (4 KB) unless raised, as does + * ingress-nginx, and answers 502 "upstream sent too big header" when they + * do not fit. The route's other headers take about 300 bytes; the refresh + * and access-token cookies beside the id token take 2-3 KB with a provider + * that issues large JWTs such as Keycloak. Adding the id token pushed such + * sign-ins over the limit (#1096), so it is only stored when it fits. + * Otherwise sign-out identifies the client by `client_id` alone. This also + * keeps the id-token cookie under the browsers' ~4096-byte cookie cap. */ -const MAX_ID_TOKEN_COOKIE_BYTES = 3500; +const MAX_SET_COOKIE_BYTES = 3500; + +/** + * A Set-Cookie header's size beyond the cookie value: header and cookie + * name, attributes (path, expiry, flags) and line break. About 120-135 bytes + * for these cookies, rounded up. + */ +const SET_COOKIE_OVERHEAD_BYTES = 150; /** Get the id-token cookie name for a given account slot. Slot 0 uses the bare name. */ export function idTokenCookieName(slot: number): string { @@ -42,16 +56,26 @@ interface CookieWriter { delete(options: { name: string; path: string }): unknown; } -/** Remember the slot's id token for sign-out, or forget a stale one. */ +function setCookieBytes(values: ReadonlyArray): number { + return values.reduce((sum, value) => (value ? sum + value.length + SET_COOKIE_OVERHEAD_BYTES : sum), 0); +} + +/** + * Remember the slot's id token for sign-out, or forget a stale one. + * `otherCookieValues` are the values of the other cookies the same response + * sets; the id token is only stored when all of them fit the response's + * header budget. + */ export function storeIdToken( cookieStore: CookieWriter, slot: number, idToken: unknown, basePath: string | undefined, + otherCookieValues: ReadonlyArray = [], ): void { const name = idTokenCookieName(slot); const path = idTokenCookiePath(basePath); - if (typeof idToken !== 'string' || !idToken || idToken.length > MAX_ID_TOKEN_COOKIE_BYTES) { + if (typeof idToken !== 'string' || !idToken || setCookieBytes([...otherCookieValues, idToken]) > MAX_SET_COOKIE_BYTES) { cookieStore.delete({ name, path }); return; }