🧽 fix: Strip Admin OAuth Redirect Params (#13181)

packages/api/src/auth/adminPkce.spec.ts

@@ -21,6 +21,8 @@ function makeReq(overrides: Partial<PkceStrippableRequest> = {}): 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 () => {

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).