diff --git a/api/server/routes/admin/auth.js b/api/server/routes/admin/auth.js index 72f23b7d52..07306ac4db 100644 --- a/api/server/routes/admin/auth.js +++ b/api/server/routes/admin/auth.js @@ -3,7 +3,12 @@ const passport = require('passport'); const crypto = require('node:crypto'); const { CacheKeys } = require('librechat-data-provider'); const { logger, SystemCapabilities } = require('@librechat/data-schemas'); -const { getAdminPanelUrl, exchangeAdminCode, createSetBalanceConfig } = require('@librechat/api'); +const { + getAdminPanelUrl, + exchangeAdminCode, + createSetBalanceConfig, + storeAndStripChallenge, +} = require('@librechat/api'); const { loginController } = require('~/server/controllers/auth/LoginController'); const { requireCapability } = require('~/server/middleware/roles/capabilities'); const { createOAuthHandler } = require('~/server/controllers/auth/oauth'); @@ -73,11 +78,6 @@ router.get('/oauth/openid/check', (req, res) => { res.status(200).json({ message: 'OpenID check successful' }); }); -/** PKCE challenge cache TTL: 5 minutes (enough for user to authenticate with IdP) */ -const PKCE_CHALLENGE_TTL = 5 * 60 * 1000; -/** Regex pattern for valid PKCE challenges: 64 hex characters (SHA-256 hex digest) */ -const PKCE_CHALLENGE_PATTERN = /^[a-f0-9]{64}$/; - /** * Generates a random hex state string for OAuth flows. * @returns {string} A 32-byte random hex string. @@ -86,27 +86,6 @@ function generateState() { return crypto.randomBytes(32).toString('hex'); } -/** - * Stores a PKCE challenge in cache keyed by state. - * @param {string} state - The OAuth state value. - * @param {string | undefined} codeChallenge - The PKCE code_challenge from query params. - * @param {string} provider - Provider name for logging. - * @returns {Promise} True if stored successfully or no challenge provided. - */ -async function storePkceChallenge(state, codeChallenge, provider) { - if (typeof codeChallenge !== 'string' || !PKCE_CHALLENGE_PATTERN.test(codeChallenge)) { - return true; - } - try { - const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); - await cache.set(`pkce:${state}`, codeChallenge, PKCE_CHALLENGE_TTL); - return true; - } catch (err) { - logger.error(`[admin/oauth/${provider}] Failed to store PKCE challenge:`, err); - return false; - } -} - /** * Middleware to retrieve PKCE challenge from cache using the OAuth state. * Reads state from req.oauthState (set by a preceding middleware). @@ -148,7 +127,8 @@ function retrievePkceChallenge(provider) { router.get('/oauth/openid', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'openid'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'openid'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/openid/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -185,7 +165,8 @@ router.get( router.get('/oauth/saml', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'saml'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'saml'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/saml/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -222,7 +203,8 @@ router.post( router.get('/oauth/google', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'google'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'google'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/google/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -260,7 +242,8 @@ router.get( router.get('/oauth/github', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'github'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'github'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/github/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -298,7 +281,8 @@ router.get( router.get('/oauth/discord', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'discord'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'discord'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/discord/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -336,7 +320,8 @@ router.get( router.get('/oauth/facebook', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'facebook'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'facebook'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/facebook/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -374,7 +359,8 @@ router.get( router.get('/oauth/apple', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'apple'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'apple'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/apple/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, diff --git a/packages/api/src/auth/adminPkce.spec.ts b/packages/api/src/auth/adminPkce.spec.ts new file mode 100644 index 0000000000..906b7d400b --- /dev/null +++ b/packages/api/src/auth/adminPkce.spec.ts @@ -0,0 +1,211 @@ +import { Keyv } from 'keyv'; + +jest.mock( + '@librechat/data-schemas', + () => ({ + logger: { + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }, + }), + { virtual: true }, +); + +import { stripCodeChallenge, storeAndStripChallenge } from './exchange'; +import type { PkceStrippableRequest } from './exchange'; + +function makeReq(overrides: Partial = {}): PkceStrippableRequest { + return { query: {}, originalUrl: '', url: '', ...overrides }; +} + +describe('stripCodeChallenge', () => { + const challenge = 'a'.repeat(64); + + it('removes code_challenge from req.query and both URL strings (sole param)', () => { + const req = makeReq({ + query: { code_challenge: challenge }, + originalUrl: `/api/admin/oauth/openid?code_challenge=${challenge}`, + url: `/oauth/openid?code_challenge=${challenge}`, + }); + + stripCodeChallenge(req); + + expect(req.query.code_challenge).toBeUndefined(); + expect(req.originalUrl).toBe('/api/admin/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 }, + originalUrl: `/oauth/openid?foo=bar&code_challenge=${challenge}`, + url: `/oauth/openid?foo=bar&code_challenge=${challenge}`, + }); + + stripCodeChallenge(req); + + expect(req.query.code_challenge).toBeUndefined(); + expect(req.query.foo).toBe('bar'); + expect(req.originalUrl).toBe('/oauth/openid?foo=bar'); + expect(req.url).toBe('/oauth/openid?foo=bar'); + }); + + it('preserves other params when code_challenge is first of multiple', () => { + const req = makeReq({ + query: { code_challenge: challenge, foo: 'bar' }, + originalUrl: `/oauth/openid?code_challenge=${challenge}&foo=bar`, + url: `/oauth/openid?code_challenge=${challenge}&foo=bar`, + }); + + stripCodeChallenge(req); + + expect(req.query.code_challenge).toBeUndefined(); + expect(req.originalUrl).toBe('/oauth/openid?foo=bar'); + expect(req.url).toBe('/oauth/openid?foo=bar'); + }); + + it('preserves other params when code_challenge is in the middle', () => { + const req = makeReq({ + query: { a: '1', code_challenge: challenge, b: '2' }, + originalUrl: `/oauth/openid?a=1&code_challenge=${challenge}&b=2`, + url: `/oauth/openid?a=1&code_challenge=${challenge}&b=2`, + }); + + stripCodeChallenge(req); + + expect(req.query.code_challenge).toBeUndefined(); + expect(req.originalUrl).toBe('/oauth/openid?a=1&b=2'); + expect(req.url).toBe('/oauth/openid?a=1&b=2'); + }); + + it('handles empty code_challenge= value', () => { + const req = makeReq({ + query: { code_challenge: '' }, + originalUrl: '/oauth/openid?code_challenge=', + url: '/oauth/openid?code_challenge=', + }); + + stripCodeChallenge(req); + + expect(req.query.code_challenge).toBeUndefined(); + expect(req.originalUrl).toBe('/oauth/openid'); + expect(req.url).toBe('/oauth/openid'); + }); + + it('is a no-op when no code_challenge is present', () => { + const req = makeReq({ + query: { foo: 'bar' }, + originalUrl: '/oauth/openid?foo=bar', + url: '/oauth/openid?foo=bar', + }); + + stripCodeChallenge(req); + + expect(req.query.foo).toBe('bar'); + expect(req.originalUrl).toBe('/oauth/openid?foo=bar'); + expect(req.url).toBe('/oauth/openid?foo=bar'); + }); + + it('is a no-op on a bare path with no query string', () => { + const req = makeReq({ + query: {}, + originalUrl: '/oauth/openid', + url: '/oauth/openid', + }); + + stripCodeChallenge(req); + + expect(req.originalUrl).toBe('/oauth/openid'); + expect(req.url).toBe('/oauth/openid'); + }); +}); + +describe('storeAndStripChallenge', () => { + const challenge = 'a'.repeat(64); + + it('stores valid challenge in cache and strips from request', async () => { + const cache = new Keyv(); + const setSpy = jest.spyOn(cache, 'set'); + const req = makeReq({ + query: { code_challenge: challenge }, + originalUrl: `/oauth/openid?code_challenge=${challenge}`, + url: `/oauth/openid?code_challenge=${challenge}`, + }); + + const result = await storeAndStripChallenge(cache, req, 'test-state', 'openid'); + + expect(result).toBe(true); + expect(setSpy).toHaveBeenCalledWith(`pkce:test-state`, challenge, expect.any(Number)); + expect(req.query.code_challenge).toBeUndefined(); + expect(req.originalUrl).toBe('/oauth/openid'); + expect(req.url).toBe('/oauth/openid'); + }); + + it('strips and returns true when no code_challenge is present', async () => { + const cache = new Keyv(); + const setSpy = jest.spyOn(cache, 'set'); + const req = makeReq({ + query: {}, + originalUrl: '/oauth/openid', + url: '/oauth/openid', + }); + + const result = await storeAndStripChallenge(cache, req, 'test-state', 'openid'); + + expect(result).toBe(true); + expect(setSpy).not.toHaveBeenCalled(); + expect(req.originalUrl).toBe('/oauth/openid'); + expect(req.url).toBe('/oauth/openid'); + }); + + 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 req = makeReq({ + query: { code_challenge: 'too-short' }, + originalUrl: '/oauth/openid?code_challenge=too-short', + url: '/oauth/openid?code_challenge=too-short', + }); + + const result = await storeAndStripChallenge(cache, req, 'test-state', 'openid'); + + expect(result).toBe(true); + expect(setSpy).not.toHaveBeenCalled(); + expect(req.query.code_challenge).toBeUndefined(); + expect(req.originalUrl).toBe('/oauth/openid'); + expect(req.url).toBe('/oauth/openid'); + }); + + 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 req = makeReq({ + query: { code_challenge: challenge }, + originalUrl: `/oauth/openid?code_challenge=${challenge}`, + url: `/oauth/openid?code_challenge=${challenge}`, + }); + + 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}`); + }); + + it('reads code_challenge before stripping (ordering guarantee)', async () => { + const cache = new Keyv(); + const setSpy = jest.spyOn(cache, 'set'); + const req = makeReq({ + query: { code_challenge: challenge }, + originalUrl: `/oauth/openid?code_challenge=${challenge}`, + url: `/oauth/openid?code_challenge=${challenge}`, + }); + + await storeAndStripChallenge(cache, req, 'test-state', 'openid'); + + const storedValue = setSpy.mock.calls[0][1]; + expect(storedValue).toBe(challenge); + }); +}); diff --git a/packages/api/src/auth/exchange.ts b/packages/api/src/auth/exchange.ts index f304ac6532..931c251d55 100644 --- a/packages/api/src/auth/exchange.ts +++ b/packages/api/src/auth/exchange.ts @@ -168,6 +168,73 @@ export async function exchangeAdminCode( }; } +/** PKCE challenge cache TTL: 5 minutes (enough for user to authenticate with IdP) */ +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=[^&]*/, ''); + +/** Minimal request shape needed by {@link stripCodeChallenge}. */ +export interface PkceStrippableRequest { + query: Record; + originalUrl: string; + url: string; +} + +/** + * Strips `code_challenge` 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. + * + * Applied defensively to all providers to ensure the admin-panel-private + * `code_challenge` parameter never reaches any Passport strategy. + */ +export function stripCodeChallenge(req: PkceStrippableRequest): void { + delete req.query.code_challenge; + req.originalUrl = stripChallengeFromUrl(req.originalUrl); + req.url = stripChallengeFromUrl(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. + * + * 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. + * @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). + * @param provider - Provider name for logging. + * @returns True if stored (or no challenge provided); false on cache failure. + */ +export async function storeAndStripChallenge( + cache: Keyv, + req: PkceStrippableRequest, + state: string, + provider: string, +): Promise { + const { code_challenge: codeChallenge } = req.query; + if (typeof codeChallenge !== 'string' || !PKCE_CHALLENGE_PATTERN.test(codeChallenge)) { + stripCodeChallenge(req); + return true; + } + try { + await cache.set(`pkce:${state}`, codeChallenge, PKCE_CHALLENGE_TTL); + stripCodeChallenge(req); + return true; + } catch (err) { + logger.error(`[admin/oauth/${provider}] Failed to store PKCE challenge:`, err); + return false; + } +} + /** * Checks if the redirect URI is for the admin panel (cross-origin). * Uses proper URL parsing to compare origins, handling edge cases where