From 5d393ad79a036f74b3c7d412634e65ac33c47aab Mon Sep 17 00:00:00 2001 From: eleite93 <93412593+eleite93@users.noreply.github.com> Date: Sat, 23 May 2026 13:57:59 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=AA=20fix:=20Support=20OpenID=20PKCE?= =?UTF-8?q?=20Without=20Client=20Secret=20(#12364)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: allow OpenID PKCE authentication without client secret * Linting * Strategy fix * fix(openid): trim secret gates and add PKCE client metadata tests * chore(openid): normalize spec line endings * ⚡ perf: Short-Circuit Config Override Resolution for Empty Principals (#12549) Skip the getApplicableConfigs DB query when buildPrincipals returns an empty array, since there are no principals to match against. * ⚡ perf: Separate Error Handling for Principal Resolution vs Config Overrides (#12550) Distinguish between buildPrincipals and getApplicableConfigs failures so the uncached fallback to baseConfig is intentional and logged separately from config override errors. * Revert "⚡ perf: Separate Error Handling for Principal Resolution vs Config Overrides (#12550)" This reverts commit 1729378a652f97e33ceded17c1b6735888670702. * Revert "⚡ perf: Short-Circuit Config Override Resolution for Empty Principals (#12549)" This reverts commit a100aa57382518e9525d0f98e7540ea5e8d09545. --------- Co-authored-by: CMF\e-leite Co-authored-by: Danny Avila --- .env.example | 3 +- api/server/routes/config.js | 2 +- api/server/socialLogins.js | 2 +- api/strategies/openidStrategy.js | 25 +++-- api/strategies/openidStrategy.spec.js | 149 +++++++++++++++++++++++--- 5 files changed, 154 insertions(+), 27 deletions(-) diff --git a/.env.example b/.env.example index 35a2a84043..8129498982 100644 --- a/.env.example +++ b/.env.example @@ -570,7 +570,8 @@ OPENID_IMAGE_URL= # Set to true to automatically redirect to the OpenID provider when a user visits the login page # This will bypass the login form completely for users, only use this if OpenID is your only authentication method OPENID_AUTO_REDIRECT=false -# Set to true to use PKCE (Proof Key for Code Exchange) for OpenID authentication +# Set to true to use PKCE (Proof Key for Code Exchange) for OpenID authentication. +# For public clients (no client secret), leave OPENID_CLIENT_SECRET empty and set this to true. OPENID_USE_PKCE=false #Set to true to reuse openid tokens for authentication management instead of using the mongodb session and the custom refresh token. OPENID_REUSE_TOKENS= diff --git a/api/server/routes/config.js b/api/server/routes/config.js index 2b9fd6e7ad..33d1a8a325 100644 --- a/api/server/routes/config.js +++ b/api/server/routes/config.js @@ -33,7 +33,7 @@ function isBirthday() { function buildSharedPayload() { const isOpenIdEnabled = !!process.env.OPENID_CLIENT_ID && - !!process.env.OPENID_CLIENT_SECRET && + (isEnabled(process.env.OPENID_USE_PKCE) || !!process.env.OPENID_CLIENT_SECRET?.trim()) && !!process.env.OPENID_ISSUER && !!process.env.OPENID_SESSION_SECRET; diff --git a/api/server/socialLogins.js b/api/server/socialLogins.js index dfb03b4d37..78f0e82a32 100644 --- a/api/server/socialLogins.js +++ b/api/server/socialLogins.js @@ -83,7 +83,7 @@ const configureSocialLogins = async (app) => { } if ( process.env.OPENID_CLIENT_ID && - process.env.OPENID_CLIENT_SECRET && + (isEnabled(process.env.OPENID_USE_PKCE) || process.env.OPENID_CLIENT_SECRET?.trim()) && process.env.OPENID_ISSUER && process.env.OPENID_SCOPE && process.env.OPENID_SESSION_SECRET diff --git a/api/strategies/openidStrategy.js b/api/strategies/openidStrategy.js index 595c2b535a..7321eb59d2 100644 --- a/api/strategies/openidStrategy.js +++ b/api/strategies/openidStrategy.js @@ -767,18 +767,25 @@ const setupOpenIdAdmin = (openidConfig) => { */ async function setupOpenId() { try { + const usePKCE = isEnabled(process.env.OPENID_USE_PKCE); const shouldGenerateNonce = isEnabled(process.env.OPENID_GENERATE_NONCE); /** @type {ClientMetadata} */ const clientMetadata = { client_id: process.env.OPENID_CLIENT_ID, - client_secret: process.env.OPENID_CLIENT_SECRET, + response_types: ['code'], + grant_types: ['authorization_code'], }; - if (shouldGenerateNonce) { - clientMetadata.response_types = ['code']; - clientMetadata.grant_types = ['authorization_code']; - clientMetadata.token_endpoint_auth_method = 'client_secret_post'; + const clientSecret = process.env.OPENID_CLIENT_SECRET?.trim(); + + if (clientSecret) { + clientMetadata.client_secret = clientSecret; + if (shouldGenerateNonce) { + clientMetadata.token_endpoint_auth_method = 'client_secret_post'; + } + } else if (usePKCE) { + clientMetadata.token_endpoint_auth_method = 'none'; } /** @type {Configuration} */ @@ -793,10 +800,10 @@ async function setupOpenId() { ); logger.info(`[openidStrategy] OpenID authentication configuration`, { + usePKCE, + hasClientSecret: !!clientSecret, + tokenEndpointAuthMethod: clientMetadata.token_endpoint_auth_method ?? '(library default)', generateNonce: shouldGenerateNonce, - reason: shouldGenerateNonce - ? 'OPENID_GENERATE_NONCE=true - Will generate nonce and use explicit metadata for federated providers' - : 'OPENID_GENERATE_NONCE=false - Standard flow without explicit nonce or metadata', }); const openidLogin = new CustomOpenIDStrategy( @@ -805,7 +812,7 @@ async function setupOpenId() { scope: process.env.OPENID_SCOPE, callbackURL: process.env.DOMAIN_SERVER + process.env.OPENID_CALLBACK_URL, clockTolerance: process.env.OPENID_CLOCK_TOLERANCE || 300, - usePKCE: isEnabled(process.env.OPENID_USE_PKCE), + usePKCE, }, createOpenIDCallback(), ); diff --git a/api/strategies/openidStrategy.spec.js b/api/strategies/openidStrategy.spec.js index 15e507e9b5..56cc97a3af 100644 --- a/api/strategies/openidStrategy.spec.js +++ b/api/strategies/openidStrategy.spec.js @@ -3,10 +3,12 @@ const fetch = require('node-fetch'); const jwtDecode = require('jsonwebtoken/decode'); const { ErrorTypes, FileSources } = require('librechat-data-provider'); const { findUser, createUser, updateUser } = require('~/models'); -const { getOpenIdIssuer, resolveAppConfigForUser } = require('@librechat/api'); +const { getOpenIdIssuer, resolveAppConfigForUser, isEnabled } = require('@librechat/api'); const { getAppConfig } = require('~/server/services/Config'); const { setupOpenId } = require('./openidStrategy'); +const mockCloudfrontFileSource = FileSources.cloudfront ?? 'cloudfront'; + // --- Mocks --- jest.mock('node-fetch'); jest.mock('jsonwebtoken/decode'); @@ -22,18 +24,68 @@ jest.mock('~/server/services/Files/strategies', () => ({ jest.mock('~/server/services/Config', () => ({ getAppConfig: jest.fn().mockResolvedValue({}), })); -jest.mock('@librechat/api', () => ({ - ...jest.requireActual('@librechat/api'), - isEnabled: jest.fn(() => false), - isEmailDomainAllowed: jest.fn(() => true), - findOpenIDUser: jest.requireActual('@librechat/api').findOpenIDUser, - getOpenIdEmail: jest.requireActual('@librechat/api').getOpenIdEmail, - getBalanceConfig: jest.fn(() => ({ - enabled: false, - })), - getOpenIdIssuer: jest.fn(() => 'https://fake-issuer.com'), - resolveAppConfigForUser: jest.fn(async (_getAppConfig, _user) => ({})), -})); +jest.mock('@librechat/api', () => { + const actual = jest.requireActual('@librechat/api'); + const getStringClaim = (claims, claim) => { + const value = claims[claim]; + return typeof value === 'string' && value ? value : undefined; + }; + + return { + ...actual, + isEnabled: jest.fn(() => false), + isEmailDomainAllowed: jest.fn(() => true), + findOpenIDUser: actual.findOpenIDUser, + getOpenIdEmail: jest.fn((claims, strategyName = 'openidStrategy') => { + if (claims == null) { + return undefined; + } + + const claimKey = process.env.OPENID_EMAIL_CLAIM?.trim(); + if (claimKey) { + const value = claims[claimKey]; + if (typeof value === 'string' && value) { + return value; + } + + const { logger } = require('@librechat/data-schemas'); + if (value != null) { + logger.warn( + `[${strategyName}] OPENID_EMAIL_CLAIM="${claimKey}" resolved to a non-string value (type: ${typeof value}). Falling back to: email -> preferred_username -> upn.`, + ); + } else { + logger.warn( + `[${strategyName}] OPENID_EMAIL_CLAIM="${claimKey}" not present in userinfo. Falling back to: email -> preferred_username -> upn.`, + ); + } + } + + return ( + getStringClaim(claims, 'email') ?? + getStringClaim(claims, 'preferred_username') ?? + getStringClaim(claims, 'upn') + ); + }), + getBalanceConfig: jest.fn(() => ({ + enabled: false, + })), + getOpenIdIssuer: jest.fn(() => 'https://fake-issuer.com'), + getAvatarFileStrategy: jest.fn((config, fallbackStrategy) => { + const { FileSources } = jest.requireActual('librechat-data-provider'); + if (config?.fileStrategies) { + return config.fileStrategies.avatar ?? config.fileStrategies.default ?? config.fileStrategy; + } + return config?.fileStrategy ?? fallbackStrategy ?? FileSources.local; + }), + getAvatarSaveParams: jest.fn((strategy, params) => { + const { FileSources } = jest.requireActual('librechat-data-provider'); + return strategy === FileSources.s3 || strategy === mockCloudfrontFileSource + ? { ...params, basePath: 'avatars' } + : params; + }), + resolveAppConfigForUser: jest.fn(async (_getAppConfig, _user) => ({})), + }; +}); jest.mock('~/models', () => ({ findUser: jest.fn(), createUser: jest.fn(), @@ -145,6 +197,7 @@ describe('setupOpenId', () => { beforeEach(async () => { // Clear previous mock calls and reset implementations jest.clearAllMocks(); + isEnabled.mockImplementation(jest.requireActual('@librechat/api').isEnabled); // Reset environment variables needed by the strategy process.env.OPENID_ISSUER = 'https://fake-issuer.com'; @@ -164,6 +217,7 @@ describe('setupOpenId', () => { delete process.env.OPENID_EMAIL_CLAIM; delete process.env.PROXY; delete process.env.OPENID_USE_PKCE; + delete process.env.OPENID_GENERATE_NONCE; // Default jwtDecode mock returns a token that includes the required role. jwtDecode.mockReturnValue({ @@ -195,6 +249,71 @@ describe('setupOpenId', () => { verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid'); }); + describe('clientMetadata construction in setupOpenId', () => { + let openidClient; + + beforeEach(() => { + openidClient = require('openid-client'); + openidClient.discovery.mockClear(); + }); + + it('sets token_endpoint_auth_method to none for PKCE without a client secret', async () => { + process.env.OPENID_USE_PKCE = 'true'; + delete process.env.OPENID_CLIENT_SECRET; + + await setupOpenId(); + + const [, , metadata] = openidClient.discovery.mock.calls.at(-1); + expect(metadata.token_endpoint_auth_method).toBe('none'); + expect(metadata.client_secret).toBeUndefined(); + }); + + it('leaves token_endpoint_auth_method unset for secret-based clients without nonce', async () => { + process.env.OPENID_USE_PKCE = 'false'; + process.env.OPENID_CLIENT_SECRET = 'my-secret'; + + await setupOpenId(); + + const [, , metadata] = openidClient.discovery.mock.calls.at(-1); + expect(metadata.client_secret).toBe('my-secret'); + expect(metadata.token_endpoint_auth_method).toBeUndefined(); + }); + + it('sets client_secret and client_secret_post when nonce generation is enabled', async () => { + process.env.OPENID_USE_PKCE = 'false'; + process.env.OPENID_GENERATE_NONCE = 'true'; + process.env.OPENID_CLIENT_SECRET = 'my-secret'; + + await setupOpenId(); + + const [, , metadata] = openidClient.discovery.mock.calls.at(-1); + expect(metadata.client_secret).toBe('my-secret'); + expect(metadata.token_endpoint_auth_method).toBe('client_secret_post'); + }); + + it('treats whitespace-only secret as absent', async () => { + process.env.OPENID_USE_PKCE = 'true'; + process.env.OPENID_CLIENT_SECRET = ' '; + + await setupOpenId(); + + const [, , metadata] = openidClient.discovery.mock.calls.at(-1); + expect(metadata.client_secret).toBeUndefined(); + expect(metadata.token_endpoint_auth_method).toBe('none'); + }); + + it('does not force an auth method when PKCE and a client secret are both configured without nonce', async () => { + process.env.OPENID_USE_PKCE = 'true'; + process.env.OPENID_CLIENT_SECRET = 'my-secret'; + + await setupOpenId(); + + const [, , metadata] = openidClient.discovery.mock.calls.at(-1); + expect(metadata.client_secret).toBe('my-secret'); + expect(metadata.token_endpoint_auth_method).toBeUndefined(); + }); + }); + it('should create a new user with correct username when preferred_username claim exists', async () => { // Arrange – our userinfo already has preferred_username 'testusername' const userinfo = tokenset.claims(); @@ -1122,7 +1241,7 @@ describe('setupOpenId', () => { it('should save CloudFront IdP avatars under the shared avatar prefix', async () => { const { getStrategyFunctions } = require('~/server/services/Files/strategies'); - getAppConfig.mockResolvedValueOnce({ fileStrategy: FileSources.cloudfront }); + getAppConfig.mockResolvedValueOnce({ fileStrategy: mockCloudfrontFileSource }); const { user } = await validate(tokenset); const strategyResult = @@ -1130,7 +1249,7 @@ describe('setupOpenId', () => { const { saveBuffer } = strategyResult.value; const [saveParams] = saveBuffer.mock.calls[0]; - expect(getStrategyFunctions).toHaveBeenLastCalledWith(FileSources.cloudfront); + expect(getStrategyFunctions).toHaveBeenLastCalledWith(mockCloudfrontFileSource); expect(saveParams).toEqual( expect.objectContaining({ basePath: 'avatars',