diff --git a/.env.example b/.env.example index db09bb471f..f8bc03310d 100644 --- a/.env.example +++ b/.env.example @@ -526,7 +526,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 a57e4bd958..e63812f5ba 100644 --- a/api/server/routes/config.js +++ b/api/server/routes/config.js @@ -27,7 +27,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 7314a84e15..b2d942618f 100644 --- a/api/strategies/openidStrategy.js +++ b/api/strategies/openidStrategy.js @@ -783,18 +783,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} */ @@ -809,10 +816,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( @@ -821,7 +828,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 6d824176f7..3a90dbd7d5 100644 --- a/api/strategies/openidStrategy.spec.js +++ b/api/strategies/openidStrategy.spec.js @@ -3,7 +3,7 @@ const fetch = require('node-fetch'); const jwtDecode = require('jsonwebtoken/decode'); const { ErrorTypes } = require('librechat-data-provider'); const { findUser, createUser, updateUser } = require('~/models'); -const { resolveAppConfigForUser } = require('@librechat/api'); +const { resolveAppConfigForUser, isEnabled } = require('@librechat/api'); const { getAppConfig } = require('~/server/services/Config'); const { setupOpenId } = require('./openidStrategy'); @@ -143,6 +143,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'; @@ -162,6 +163,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({ @@ -193,6 +195,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();