From 77c523ea35e794ef647d00aac8d40f3ed57f3d02 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 18 May 2026 15:27:19 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=BD=20fix:=20Strip=20Admin=20OAuth=20R?= =?UTF-8?q?edirect=20Params=20(#13181)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/api/src/auth/adminPkce.spec.ts | 83 +++++++++++++++++++++---- packages/api/src/auth/exchange.ts | 53 ++++++++++++---- 2 files changed, 113 insertions(+), 23 deletions(-) diff --git a/packages/api/src/auth/adminPkce.spec.ts b/packages/api/src/auth/adminPkce.spec.ts index 906b7d400b..57d643a61a 100644 --- a/packages/api/src/auth/adminPkce.spec.ts +++ b/packages/api/src/auth/adminPkce.spec.ts @@ -21,6 +21,8 @@ function makeReq(overrides: Partial = {}): PkceStrippable describe('stripCodeChallenge', () => { const challenge = 'a'.repeat(64); + const callbackUrl = 'https://admin.example.com/auth/openid/callback'; + const encodedCallbackUrl = 'https%3A%2F%2Fadmin.example.com%2Fauth%2Fopenid%2Fcallback'; it('removes code_challenge from req.query and both URL strings (sole param)', () => { const req = makeReq({ @@ -36,6 +38,48 @@ describe('stripCodeChallenge', () => { expect(req.url).toBe('/oauth/openid'); }); + it('removes admin-panel-only params before the request reaches Passport', () => { + const originalUrl = + `/api/admin/oauth/openid?code_challenge=${challenge}` + + `&redirect_uri=${encodedCallbackUrl}&redirectTo=%2Fsettings&foo=bar`; + const url = + `/oauth/openid?code_challenge=${challenge}` + + `&redirect_uri=${encodedCallbackUrl}&redirectTo=%2Fsettings&foo=bar`; + const req = makeReq({ + query: { + code_challenge: challenge, + redirect_uri: callbackUrl, + redirectTo: '/settings', + foo: 'bar', + }, + originalUrl, + url, + }); + + stripCodeChallenge(req); + + expect(req.query.code_challenge).toBeUndefined(); + expect(req.query.redirect_uri).toBeUndefined(); + expect(req.query.redirectTo).toBeUndefined(); + expect(req.query.foo).toBe('bar'); + expect(req.originalUrl).toBe('/api/admin/oauth/openid?foo=bar'); + expect(req.url).toBe('/oauth/openid?foo=bar'); + }); + + it('removes redirect_uri when it is the only admin-panel-only param', () => { + const req = makeReq({ + query: { redirect_uri: callbackUrl }, + originalUrl: `/oauth/openid?redirect_uri=${encodedCallbackUrl}`, + url: `/oauth/openid?redirect_uri=${encodedCallbackUrl}`, + }); + + stripCodeChallenge(req); + + expect(req.query.redirect_uri).toBeUndefined(); + expect(req.originalUrl).toBe('/oauth/openid'); + expect(req.url).toBe('/oauth/openid'); + }); + it('preserves other params when code_challenge is last', () => { const req = makeReq({ query: { foo: 'bar', code_challenge: challenge }, @@ -123,14 +167,20 @@ describe('stripCodeChallenge', () => { describe('storeAndStripChallenge', () => { const challenge = 'a'.repeat(64); + const callbackUrl = 'https://admin.example.com/auth/openid/callback'; + const encodedCallbackUrl = 'https%3A%2F%2Fadmin.example.com%2Fauth%2Fopenid%2Fcallback'; it('stores valid challenge in cache and strips from request', async () => { const cache = new Keyv(); const setSpy = jest.spyOn(cache, 'set'); + const url = `/oauth/openid?code_challenge=${challenge}&redirect_uri=${encodedCallbackUrl}`; const req = makeReq({ - query: { code_challenge: challenge }, - originalUrl: `/oauth/openid?code_challenge=${challenge}`, - url: `/oauth/openid?code_challenge=${challenge}`, + query: { + code_challenge: challenge, + redirect_uri: callbackUrl, + }, + originalUrl: url, + url, }); const result = await storeAndStripChallenge(cache, req, 'test-state', 'openid'); @@ -138,6 +188,7 @@ describe('storeAndStripChallenge', () => { expect(result).toBe(true); expect(setSpy).toHaveBeenCalledWith(`pkce:test-state`, challenge, expect.any(Number)); expect(req.query.code_challenge).toBeUndefined(); + expect(req.query.redirect_uri).toBeUndefined(); expect(req.originalUrl).toBe('/oauth/openid'); expect(req.url).toBe('/oauth/openid'); }); @@ -162,10 +213,14 @@ describe('storeAndStripChallenge', () => { it('strips and returns true when code_challenge is invalid (not 64 hex)', async () => { const cache = new Keyv(); const setSpy = jest.spyOn(cache, 'set'); + const url = `/oauth/openid?code_challenge=too-short&redirect_uri=${encodedCallbackUrl}`; const req = makeReq({ - query: { code_challenge: 'too-short' }, - originalUrl: '/oauth/openid?code_challenge=too-short', - url: '/oauth/openid?code_challenge=too-short', + query: { + code_challenge: 'too-short', + redirect_uri: callbackUrl, + }, + originalUrl: url, + url, }); const result = await storeAndStripChallenge(cache, req, 'test-state', 'openid'); @@ -173,6 +228,7 @@ describe('storeAndStripChallenge', () => { expect(result).toBe(true); expect(setSpy).not.toHaveBeenCalled(); expect(req.query.code_challenge).toBeUndefined(); + expect(req.query.redirect_uri).toBeUndefined(); expect(req.originalUrl).toBe('/oauth/openid'); expect(req.url).toBe('/oauth/openid'); }); @@ -180,18 +236,23 @@ describe('storeAndStripChallenge', () => { it('returns false and does not strip on cache failure', async () => { const cache = new Keyv(); jest.spyOn(cache, 'set').mockRejectedValueOnce(new Error('cache down')); + const url = `/oauth/openid?code_challenge=${challenge}&redirect_uri=${encodedCallbackUrl}`; const req = makeReq({ - query: { code_challenge: challenge }, - originalUrl: `/oauth/openid?code_challenge=${challenge}`, - url: `/oauth/openid?code_challenge=${challenge}`, + query: { + code_challenge: challenge, + redirect_uri: callbackUrl, + }, + originalUrl: url, + url, }); const result = await storeAndStripChallenge(cache, req, 'test-state', 'openid'); expect(result).toBe(false); expect(req.query.code_challenge).toBe(challenge); - expect(req.originalUrl).toBe(`/oauth/openid?code_challenge=${challenge}`); - expect(req.url).toBe(`/oauth/openid?code_challenge=${challenge}`); + expect(req.query.redirect_uri).toBe(callbackUrl); + expect(req.originalUrl).toBe(url); + expect(req.url).toBe(url); }); it('reads code_challenge before stripping (ordering guarantee)', async () => { diff --git a/packages/api/src/auth/exchange.ts b/packages/api/src/auth/exchange.ts index 650b006e38..74d882bb09 100644 --- a/packages/api/src/auth/exchange.ts +++ b/packages/api/src/auth/exchange.ts @@ -182,9 +182,36 @@ export const PKCE_CHALLENGE_TTL = 5 * 60 * 1000; /** Regex pattern for valid PKCE challenges: 64 hex characters (SHA-256 hex digest) */ export const PKCE_CHALLENGE_PATTERN = /^[a-f0-9]{64}$/; -/** Removes `code_challenge` from a single URL string, preserving other query params. */ -const stripChallengeFromUrl = (url: string): string => - url.replace(/\?code_challenge=[^&]*&/, '?').replace(/[?&]code_challenge=[^&]*/, ''); +const ADMIN_OAUTH_STRIPPED_QUERY_PARAMS = new Set(['code_challenge', 'redirect_uri', 'redirectTo']); + +const getQueryParamName = (param: string): string => { + const separatorIndex = param.indexOf('='); + return separatorIndex === -1 ? param : param.slice(0, separatorIndex); +}; + +/** Removes admin-panel-only query params from a single URL string. */ +const stripAdminOAuthParamsFromUrl = (url: string): string => { + const hashIndex = url.indexOf('#'); + const urlWithoutHash = hashIndex === -1 ? url : url.slice(0, hashIndex); + const hash = hashIndex === -1 ? '' : url.slice(hashIndex); + const queryIndex = urlWithoutHash.indexOf('?'); + + if (queryIndex === -1) { + return url; + } + + const path = urlWithoutHash.slice(0, queryIndex); + const query = urlWithoutHash.slice(queryIndex + 1); + const params = query.split('&').filter((param) => { + if (!param) { + return false; + } + + return !ADMIN_OAUTH_STRIPPED_QUERY_PARAMS.has(getQueryParamName(param)); + }); + + return params.length > 0 ? `${path}?${params.join('&')}${hash}` : `${path}${hash}`; +}; /** Minimal request shape needed by {@link stripCodeChallenge}. */ export interface PkceStrippableRequest { @@ -194,29 +221,31 @@ export interface PkceStrippableRequest { } /** - * Strips `code_challenge` from the request query and URL strings. + * Strips admin-panel-only params from the request query and URL strings. * * openid-client v6's Passport Strategy uses `currentUrl.searchParams.size === 0` * to distinguish an initial authorization request from an OAuth callback. - * The admin-panel-specific `code_challenge` query parameter would cause the - * strategy to misclassify the request as a callback and return 401. + * Admin-panel-specific query params would cause the strategy to misclassify the + * request as a callback and return 401. * * Applied defensively to all providers to ensure the admin-panel-private - * `code_challenge` parameter never reaches any Passport strategy. + * parameters never reach any Passport strategy. */ export function stripCodeChallenge(req: PkceStrippableRequest): void { delete req.query.code_challenge; - req.originalUrl = stripChallengeFromUrl(req.originalUrl); - req.url = stripChallengeFromUrl(req.url); + delete req.query.redirect_uri; + delete req.query.redirectTo; + req.originalUrl = stripAdminOAuthParamsFromUrl(req.originalUrl); + req.url = stripAdminOAuthParamsFromUrl(req.url); } /** - * Stores the admin-panel PKCE challenge in cache, then strips `code_challenge` - * from the request so it doesn't interfere with the Passport strategy. + * Stores the admin-panel PKCE challenge in cache, then strips admin-panel-only + * params from the request so they don't interfere with the Passport strategy. * * Must be called before `passport.authenticate()` — the two operations are * logically atomic: read the challenge from the query, persist it, then remove - * the parameter from the request URL. + * those parameters from the request URL. * @param cache - The Keyv cache instance for storing PKCE challenges. * @param req - The Express request to read and mutate. * @param state - The OAuth state value (cache key).