mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-04-05 15:27:20 +02:00
🔐 fix: Strip code_challenge from Admin OAuth requests before Passport (#12534)
* 🔐 fix: Strip code_challenge from admin OAuth requests before Passport openid-client v6's Passport Strategy uses `currentUrl.searchParams.size === 0` to distinguish initial authorization requests from OAuth callbacks. The admin-panel-specific `code_challenge` query parameter caused the strategy to misclassify the request as a callback and return 401 Unauthorized. * 🔐 fix: Strip code_challenge from admin OAuth requests before Passport openid-client v6's Passport Strategy uses `currentUrl.searchParams.size === 0` to distinguish initial authorization requests from OAuth callbacks. The admin-panel-specific `code_challenge` query parameter caused the strategy to misclassify the request as a callback and return 401 Unauthorized. - Fix regex to handle `code_challenge` in any query position without producing malformed URLs, and handle empty `code_challenge=` values (`[^&]*` vs `[^&]+`) - Combine `storePkceChallenge` + `stripCodeChallenge` into a single `storeAndStripChallenge` helper to enforce read-store-strip ordering - Apply defensively to all 7 admin OAuth providers - Add 12 unit tests covering stripCodeChallenge and storeAndStripChallenge * refactor: Extract PKCE helpers to utility file, harden tests - Move stripCodeChallenge and storeAndStripChallenge to api/server/utils/adminPkce.js — eliminates _test production export and avoids loading the full auth.js module tree in tests - Add missing req.originalUrl/req.url assertions to invalid-challenge and no-challenge test branches (regression blind spots) - Hoist cache reference to module scope in tests (was redundantly re-acquired from mock factory on every beforeEach) * chore: Address review NITs — imports, exports, naming, assertions - Fix import order in auth.js (longest-to-shortest per CLAUDE.md) - Remove unused PKCE_CHALLENGE_TTL/PKCE_CHALLENGE_PATTERN exports - Hoist strip arrow to module-scope stripChallengeFromUrl - Rename auth.test.js → auth.spec.js (project convention) - Tighten cache-failure test: toBe instead of toContain, add req.url * refactor: Move PKCE helpers to packages/api with dependency injection Move stripCodeChallenge and storeAndStripChallenge from api/server/utils into packages/api/src/auth/exchange.ts alongside the existing PKCE verification logic. Cache is now injected as a Keyv parameter, matching the dependency-injection pattern used throughout packages/api/. - Add PkceStrippableRequest interface for minimal req typing - auth.js imports storeAndStripChallenge from @librechat/api - Delete api/server/utils/adminPkce.js - Move tests to packages/api/src/auth/adminPkce.spec.ts (TypeScript, real Keyv instances, no getLogStores mock needed)
This commit is contained in:
Danny Avila
GitHub
parent
ed02fe40e0
commit
fa4a43da21
3 changed files with 298 additions and 34 deletions
|
|
@ -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<boolean>} 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`,
|
||||
|
|
|
|||
211
packages/api/src/auth/adminPkce.spec.ts
Normal file
211
packages/api/src/auth/adminPkce.spec.ts
Normal file
|
|
@ -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> = {}): 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);
|
||||
});
|
||||
});
|
||||
|
|
@ -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<string, unknown>;
|
||||
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<boolean> {
|
||||
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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue