mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-04-03 14:27:20 +02:00
🛡️ fix: Add Origin Binding to Admin OAuth Exchange Codes (#12469)
* fix(auth): add origin binding to admin OAuth exchange codes Bind admin OAuth exchange codes to the admin panel's origin at generation time and validate the origin on redemption. This prevents an intercepted code (via referrer leakage, logs, or network capture) from being redeemed by a different origin within the 30-second TTL. - Store the admin panel origin alongside the exchange code in cache - Extract the request origin (from Origin/Referer headers) on the exchange endpoint and pass it for validation - Reject code redemption when the request origin does not match the stored origin (code is still consumed to prevent replay) - Backward compatible: codes without a stored origin are accepted * fix(auth): add PKCE proof-of-possession to admin OAuth exchange codes Add a PKCE-like code_challenge/code_verifier flow to the admin OAuth exchange so that intercepting the exchange code alone is insufficient to redeem it. The admin panel generates a code_verifier (stored in its HttpOnly session cookie) and sends sha256(verifier) as code_challenge through the OAuth initiation URL. LibreChat stores the challenge keyed by OAuth state and attaches it to the exchange code. On redemption, the admin panel sends the verifier and LibreChat verifies the hash match. - Add verifyCodeChallenge() helper using SHA-256 - Store code_challenge in ADMIN_OAUTH_EXCHANGE cache (pkce: prefix, 5min TTL) - Capture OAuth state in callback middleware before passport processes it - Accept code_verifier in exchange endpoint body - Backward compatible: no challenge stored → PKCE check skipped * fix(auth): harden PKCE and origin binding in admin OAuth exchange - Await cache.set for PKCE challenge storage with error handling - Use crypto.timingSafeEqual for PKCE hash comparison - Drop case-insensitive flag from hex validation regexes - Add code_verifier length validation (max 512 chars) - Normalize Origin header via URL parsing in resolveRequestOrigin - Add test for undefined requestOrigin rejection - Clarify JSDoc: hex-encoded SHA-256, not RFC 7636 S256 * fix(auth): fail closed on PKCE callback cache errors, clean up origin/buffer handling - Callback middleware now redirects to error URL on cache.get failure instead of silently continuing without PKCE challenge - resolveRequestOrigin returns undefined (not raw header) on parse failure - Remove dead try/catch around Buffer.from which never throws for string input * chore(auth): remove narration comments, scope eslint-disable to lines * chore(auth): narrow query.state to string, remove narration comments in exchange.ts * fix(auth): address review findings — warn on missing PKCE challenge, validate verifier length, deduplicate URL parse - Log warning when OAuth state is present but no PKCE challenge found - Add minimum length check (>= 1) on code_verifier input validation - Update POST /oauth/exchange JSDoc to document code_verifier param - Deduplicate new URL(redirectUri) parse in createOAuthHandler - Restore intent comment on pre-delete pattern in exchangeAdminCode * test(auth): replace mock cache with real Keyv, remove all as-any casts - Use real Keyv in-memory store instead of hand-rolled Map mock - Replace jest.fn mocks with jest.spyOn on real Keyv instance - Remove redundant store.has() assertion, use cache.get() instead - Eliminate all eslint-disable and as-any suppressions - User fixture no longer needs any cast (Keyv accepts plain objects) * fix(auth): add IUser type cast for test fixture to satisfy tsc
This commit is contained in:
parent
1455f15b7b
commit
2bf0f892d6
4 changed files with 353 additions and 9 deletions
217
packages/api/src/auth/exchange.spec.ts
Normal file
217
packages/api/src/auth/exchange.spec.ts
Normal file
|
|
@ -0,0 +1,217 @@
|
|||
import crypto from 'crypto';
|
||||
import { Keyv } from 'keyv';
|
||||
import type { IUser } from '@librechat/data-schemas';
|
||||
|
||||
jest.mock(
|
||||
'@librechat/data-schemas',
|
||||
() => ({
|
||||
logger: {
|
||||
info: jest.fn(),
|
||||
warn: jest.fn(),
|
||||
},
|
||||
}),
|
||||
{ virtual: true },
|
||||
);
|
||||
|
||||
import { exchangeAdminCode, generateAdminExchangeCode, verifyCodeChallenge } from './exchange';
|
||||
|
||||
describe('admin OAuth code exchange', () => {
|
||||
const user = {
|
||||
_id: 'user123',
|
||||
email: 'admin@example.com',
|
||||
name: 'Admin User',
|
||||
username: 'admin',
|
||||
role: 'ADMIN',
|
||||
provider: 'openid',
|
||||
} as unknown as IUser;
|
||||
|
||||
const createCache = () => {
|
||||
const cache = new Keyv();
|
||||
const deleteSpy = jest.spyOn(cache, 'delete');
|
||||
return { cache, deleteSpy };
|
||||
};
|
||||
|
||||
describe('origin binding', () => {
|
||||
it('exchanges code when request origin matches generated origin', async () => {
|
||||
const { cache } = createCache();
|
||||
const exchangeCode = await generateAdminExchangeCode(
|
||||
cache,
|
||||
user,
|
||||
'jwt-token',
|
||||
'refresh-token',
|
||||
'https://admin.example.com',
|
||||
);
|
||||
|
||||
const result = await exchangeAdminCode(cache, exchangeCode, 'https://admin.example.com');
|
||||
|
||||
expect(result).not.toBeNull();
|
||||
expect(result!.token).toBe('jwt-token');
|
||||
expect(result!.refreshToken).toBe('refresh-token');
|
||||
expect(result!.user.email).toBe('admin@example.com');
|
||||
});
|
||||
|
||||
it('rejects code exchange when request origin does not match generated origin', async () => {
|
||||
const { cache, deleteSpy } = createCache();
|
||||
const exchangeCode = await generateAdminExchangeCode(
|
||||
cache,
|
||||
user,
|
||||
'jwt-token',
|
||||
'refresh-token',
|
||||
'https://admin.example.com',
|
||||
);
|
||||
|
||||
const result = await exchangeAdminCode(cache, exchangeCode, 'https://evil.example.com');
|
||||
|
||||
expect(result).toBeNull();
|
||||
expect(deleteSpy).toHaveBeenCalledWith(exchangeCode);
|
||||
await expect(cache.get(exchangeCode)).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
it('rejects code exchange when origin is stored but request has no origin', async () => {
|
||||
const { cache } = createCache();
|
||||
const exchangeCode = await generateAdminExchangeCode(
|
||||
cache,
|
||||
user,
|
||||
'jwt-token',
|
||||
'refresh-token',
|
||||
'https://admin.example.com',
|
||||
);
|
||||
|
||||
const result = await exchangeAdminCode(cache, exchangeCode, undefined);
|
||||
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it('allows exchange when no origin was stored (backward compat)', async () => {
|
||||
const { cache } = createCache();
|
||||
const exchangeCode = await generateAdminExchangeCode(
|
||||
cache,
|
||||
user,
|
||||
'jwt-token',
|
||||
'refresh-token',
|
||||
);
|
||||
|
||||
const result = await exchangeAdminCode(cache, exchangeCode, 'https://any-origin.com');
|
||||
|
||||
expect(result).not.toBeNull();
|
||||
expect(result!.token).toBe('jwt-token');
|
||||
});
|
||||
});
|
||||
|
||||
describe('one-time use', () => {
|
||||
it('rejects code that has already been used', async () => {
|
||||
const { cache } = createCache();
|
||||
const exchangeCode = await generateAdminExchangeCode(
|
||||
cache,
|
||||
user,
|
||||
'jwt-token',
|
||||
'refresh-token',
|
||||
'https://admin.example.com',
|
||||
);
|
||||
|
||||
await exchangeAdminCode(cache, exchangeCode, 'https://admin.example.com');
|
||||
const secondAttempt = await exchangeAdminCode(
|
||||
cache,
|
||||
exchangeCode,
|
||||
'https://admin.example.com',
|
||||
);
|
||||
|
||||
expect(secondAttempt).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('PKCE verification', () => {
|
||||
const codeVerifier = crypto.randomBytes(32).toString('hex');
|
||||
const codeChallenge = crypto.createHash('sha256').update(codeVerifier).digest('hex');
|
||||
|
||||
it('verifyCodeChallenge returns true for matching verifier', () => {
|
||||
expect(verifyCodeChallenge(codeVerifier, codeChallenge)).toBe(true);
|
||||
});
|
||||
|
||||
it('verifyCodeChallenge returns false for wrong verifier', () => {
|
||||
expect(verifyCodeChallenge('wrong-verifier', codeChallenge)).toBe(false);
|
||||
});
|
||||
|
||||
it('verifyCodeChallenge handles hex case insensitively (input gate rejects uppercase)', () => {
|
||||
const uppercaseChallenge = codeChallenge.toUpperCase();
|
||||
// Buffer.from(hex) is case-insensitive, so verification passes at this layer.
|
||||
// Uppercase challenges are rejected earlier by PKCE_CHALLENGE_PATTERN (no /i flag).
|
||||
expect(verifyCodeChallenge(codeVerifier, uppercaseChallenge)).toBe(true);
|
||||
});
|
||||
|
||||
it('exchanges code when valid code_verifier is provided', async () => {
|
||||
const { cache } = createCache();
|
||||
const exchangeCode = await generateAdminExchangeCode(
|
||||
cache,
|
||||
user,
|
||||
'jwt-token',
|
||||
'refresh-token',
|
||||
'https://admin.example.com',
|
||||
codeChallenge,
|
||||
);
|
||||
|
||||
const result = await exchangeAdminCode(
|
||||
cache,
|
||||
exchangeCode,
|
||||
'https://admin.example.com',
|
||||
codeVerifier,
|
||||
);
|
||||
|
||||
expect(result).not.toBeNull();
|
||||
expect(result!.token).toBe('jwt-token');
|
||||
});
|
||||
|
||||
it('rejects exchange when code_verifier does not match challenge', async () => {
|
||||
const { cache } = createCache();
|
||||
const exchangeCode = await generateAdminExchangeCode(
|
||||
cache,
|
||||
user,
|
||||
'jwt-token',
|
||||
'refresh-token',
|
||||
'https://admin.example.com',
|
||||
codeChallenge,
|
||||
);
|
||||
|
||||
const result = await exchangeAdminCode(
|
||||
cache,
|
||||
exchangeCode,
|
||||
'https://admin.example.com',
|
||||
'wrong-verifier',
|
||||
);
|
||||
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it('rejects exchange when challenge stored but no verifier provided', async () => {
|
||||
const { cache } = createCache();
|
||||
const exchangeCode = await generateAdminExchangeCode(
|
||||
cache,
|
||||
user,
|
||||
'jwt-token',
|
||||
'refresh-token',
|
||||
'https://admin.example.com',
|
||||
codeChallenge,
|
||||
);
|
||||
|
||||
const result = await exchangeAdminCode(cache, exchangeCode, 'https://admin.example.com');
|
||||
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it('allows exchange when no challenge stored and no verifier sent (backward compat)', async () => {
|
||||
const { cache } = createCache();
|
||||
const exchangeCode = await generateAdminExchangeCode(
|
||||
cache,
|
||||
user,
|
||||
'jwt-token',
|
||||
'refresh-token',
|
||||
'https://admin.example.com',
|
||||
);
|
||||
|
||||
const result = await exchangeAdminCode(cache, exchangeCode, 'https://admin.example.com');
|
||||
|
||||
expect(result).not.toBeNull();
|
||||
expect(result!.token).toBe('jwt-token');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -37,6 +37,8 @@ export interface AdminExchangeData {
|
|||
user: AdminExchangeUser;
|
||||
token: string;
|
||||
refreshToken?: string;
|
||||
origin?: string;
|
||||
codeChallenge?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -68,12 +70,30 @@ export function serializeUserForExchange(user: IUser): AdminExchangeUser {
|
|||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Verifies a PKCE code_verifier against a stored code_challenge.
|
||||
* Uses hex-encoded SHA-256 comparison (not RFC 7636 S256 which uses base64url).
|
||||
* @param verifier - The code_verifier provided during exchange
|
||||
* @param challenge - The hex-encoded SHA-256 code_challenge stored during code generation
|
||||
* @returns True if the verifier matches the challenge
|
||||
*/
|
||||
export function verifyCodeChallenge(verifier: string, challenge: string): boolean {
|
||||
const computed = crypto.createHash('sha256').update(verifier).digest();
|
||||
const storedBuf = Buffer.from(challenge, 'hex');
|
||||
if (computed.length !== storedBuf.length) {
|
||||
return false;
|
||||
}
|
||||
return crypto.timingSafeEqual(computed, storedBuf);
|
||||
}
|
||||
|
||||
/**
|
||||
* Generates an exchange code and stores user data for admin panel OAuth flow.
|
||||
* @param cache - The Keyv cache instance for storing exchange data
|
||||
* @param user - The authenticated user object
|
||||
* @param token - The JWT access token
|
||||
* @param refreshToken - Optional refresh token for OpenID users
|
||||
* @param origin - The admin panel origin (scheme://host:port) for origin binding
|
||||
* @param codeChallenge - PKCE code_challenge (hex-encoded SHA-256 of code_verifier)
|
||||
* @returns The generated exchange code
|
||||
*/
|
||||
export async function generateAdminExchangeCode(
|
||||
|
|
@ -81,6 +101,8 @@ export async function generateAdminExchangeCode(
|
|||
user: IUser,
|
||||
token: string,
|
||||
refreshToken?: string,
|
||||
origin?: string,
|
||||
codeChallenge?: string,
|
||||
): Promise<string> {
|
||||
const exchangeCode = crypto.randomBytes(32).toString('hex');
|
||||
|
||||
|
|
@ -89,6 +111,8 @@ export async function generateAdminExchangeCode(
|
|||
user: serializeUserForExchange(user),
|
||||
token,
|
||||
refreshToken,
|
||||
origin,
|
||||
codeChallenge,
|
||||
};
|
||||
|
||||
await cache.set(exchangeCode, data);
|
||||
|
|
@ -103,15 +127,19 @@ export async function generateAdminExchangeCode(
|
|||
* The code is deleted immediately after retrieval (one-time use).
|
||||
* @param cache - The Keyv cache instance for retrieving exchange data
|
||||
* @param code - The authorization code to exchange
|
||||
* @param requestOrigin - The origin of the requesting client for origin binding
|
||||
* @param codeVerifier - PKCE code_verifier to verify against the stored code_challenge
|
||||
* @returns The exchange response with token, refreshToken, and user data, or null if invalid/expired
|
||||
*/
|
||||
export async function exchangeAdminCode(
|
||||
cache: Keyv,
|
||||
code: string,
|
||||
requestOrigin?: string,
|
||||
codeVerifier?: string,
|
||||
): Promise<AdminExchangeResponse | null> {
|
||||
const data = (await cache.get(code)) as AdminExchangeData | undefined;
|
||||
|
||||
/** Delete immediately - one-time use */
|
||||
/** Delete before validation — ensures one-time use even if subsequent checks throw */
|
||||
await cache.delete(code);
|
||||
|
||||
if (!data) {
|
||||
|
|
@ -119,6 +147,18 @@ export async function exchangeAdminCode(
|
|||
return null;
|
||||
}
|
||||
|
||||
if (data.origin && data.origin !== requestOrigin) {
|
||||
logger.warn('[adminExchange] Authorization code origin mismatch');
|
||||
return null;
|
||||
}
|
||||
|
||||
if (data.codeChallenge) {
|
||||
if (!codeVerifier || !verifyCodeChallenge(codeVerifier, data.codeChallenge)) {
|
||||
logger.warn('[adminExchange] PKCE code_verifier mismatch or missing');
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
logger.info(`[adminExchange] Exchanged code for user: ${data.user?.email}`);
|
||||
|
||||
return {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue