fix(openid): trim secret gates and add PKCE client metadata tests

This commit is contained in:
CMF\e-leite 2026-04-02 16:39:20 +01:00
parent c9fb9c4a67
commit 3b7d8ac247
5 changed files with 77 additions and 7 deletions

View file

@ -41,7 +41,7 @@ router.get('/', async function (req, res) {
const isOpenIdEnabled =
!!process.env.OPENID_CLIENT_ID &&
(isEnabled(process.env.OPENID_USE_PKCE) || !!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;

View file

@ -73,7 +73,7 @@ const configureSocialLogins = async (app) => {
}
if (
process.env.OPENID_CLIENT_ID &&
(isEnabled(process.env.OPENID_USE_PKCE) || 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

View file

@ -805,10 +805,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(
@ -817,7 +817,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(),
);

View file

@ -140,6 +140,8 @@ describe('setupOpenId', () => {
beforeEach(async () => {
// Clear previous mock calls and reset implementations
jest.clearAllMocks();
const { isEnabled } = require('@librechat/api');
isEnabled.mockImplementation(jest.requireActual('@librechat/api').isEnabled);
// Reset environment variables needed by the strategy
process.env.OPENID_ISSUER = 'https://fake-issuer.com';
@ -159,6 +161,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({
@ -190,6 +193,72 @@ describe('setupOpenId', () => {
verifyCallback = require('openid-client/passport').__getVerifyCallbackByName('openid');
});
describe('clientMetadata construction in setupOpenId', () => {
it('sets token_endpoint_auth_method to none for PKCE without a client secret', async () => {
const openidClient = require('openid-client');
openidClient.discovery.mockClear();
process.env.OPENID_USE_PKCE = 'true';
delete 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('leaves token_endpoint_auth_method unset for secret-based clients without nonce', async () => {
const openidClient = require('openid-client');
openidClient.discovery.mockClear();
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_post when nonce generation is enabled for secret-based clients', async () => {
const openidClient = require('openid-client');
openidClient.discovery.mockClear();
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 a whitespace-only client secret as absent', async () => {
const openidClient = require('openid-client');
openidClient.discovery.mockClear();
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 a public-client auth method when PKCE and a client secret are both configured', async () => {
const openidClient = require('openid-client');
openidClient.discovery.mockClear();
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();