mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-22 07:36:33 +01:00
🪂 fix: Automatic logout_hint Fallback for Oversized OpenID Token URLs (#12326)
* fix: automatic logout_hint fallback for long OpenID tokens
Implements OIDC RP-Initiated Logout cascading strategy to prevent errors when id_token_hint makes logout URL too long.
Automatically detects URLs exceeding configurable length and falls back to logout_hint only when URL is too long, preserving previous behavior when token is missing. Adds OPENID_MAX_LOGOUT_URL_LENGTH environment variable. Comprehensive test coverage with 20 tests. Works with any OpenID provider.
* fix: address review findings for OIDC logout URL length fallback
- Replace two-boolean tri-state (useIdTokenHint/urlTooLong) with a single
string discriminant ('use_token'|'too_long'|'no_token') for clarity
- Fix misleading warning: differentiate 'url too long + no client_id' from
'no token + no client_id' so operators get actionable advice
- Strict env var parsing: reject partial numeric strings like '500abc' that
Number.parseInt silently accepted; use regex + Number() instead
- Pre-compute projected URL length from base URL + token length (JWT chars
are URL-safe), eliminating the set-then-delete mutation pattern
- Extract parseMaxLogoutUrlLength helper for validation and early return
- Add tests: invalid env values, url-too-long + missing OPENID_CLIENT_ID,
boundary condition (exact max vs max+1), cookie-sourced long token
- Remove redundant try/finally in 'respects custom limit' test
- Use empty value in .env.example to signal optional config (default: 2000)
---------
Co-authored-by: Airam Hernández Hernández <airam.hernandez@intelequia.com>
Co-authored-by: Danny Avila <danny@librechat.ai>
This commit is contained in:
parent
594d9470d5
commit
96f6976e00
3 changed files with 379 additions and 11 deletions
|
|
@ -540,6 +540,8 @@ OPENID_ON_BEHALF_FLOW_USERINFO_SCOPE="user.read" # example for Scope Needed for
|
|||
OPENID_USE_END_SESSION_ENDPOINT=
|
||||
# URL to redirect to after OpenID logout (defaults to ${DOMAIN_CLIENT}/login)
|
||||
OPENID_POST_LOGOUT_REDIRECT_URI=
|
||||
# Maximum logout URL length before using logout_hint instead of id_token_hint (default: 2000)
|
||||
OPENID_MAX_LOGOUT_URL_LENGTH=
|
||||
|
||||
#========================#
|
||||
# SharePoint Integration #
|
||||
|
|
|
|||
|
|
@ -4,11 +4,27 @@ const { logger } = require('@librechat/data-schemas');
|
|||
const { logoutUser } = require('~/server/services/AuthService');
|
||||
const { getOpenIdConfig } = require('~/strategies');
|
||||
|
||||
/** Parses and validates OPENID_MAX_LOGOUT_URL_LENGTH, returning defaultValue on invalid input */
|
||||
function parseMaxLogoutUrlLength(defaultValue = 2000) {
|
||||
const raw = process.env.OPENID_MAX_LOGOUT_URL_LENGTH;
|
||||
const trimmed = raw == null ? '' : raw.trim();
|
||||
if (trimmed === '') {
|
||||
return defaultValue;
|
||||
}
|
||||
const parsed = /^\d+$/.test(trimmed) ? Number(trimmed) : NaN;
|
||||
if (!Number.isFinite(parsed) || parsed <= 0) {
|
||||
logger.warn(
|
||||
`[logoutController] Invalid OPENID_MAX_LOGOUT_URL_LENGTH value "${raw}", using default ${defaultValue}`,
|
||||
);
|
||||
return defaultValue;
|
||||
}
|
||||
return parsed;
|
||||
}
|
||||
|
||||
const logoutController = async (req, res) => {
|
||||
const parsedCookies = req.headers.cookie ? cookies.parse(req.headers.cookie) : {};
|
||||
const isOpenIdUser = req.user?.openidId != null && req.user?.provider === 'openid';
|
||||
|
||||
/** For OpenID users, read tokens from session (with cookie fallback) */
|
||||
let refreshToken;
|
||||
let idToken;
|
||||
if (isOpenIdUser && req.session?.openidTokens) {
|
||||
|
|
@ -44,22 +60,64 @@ const logoutController = async (req, res) => {
|
|||
const endSessionEndpoint = openIdConfig.serverMetadata().end_session_endpoint;
|
||||
if (endSessionEndpoint) {
|
||||
const endSessionUrl = new URL(endSessionEndpoint);
|
||||
/** Redirect back to app's login page after IdP logout */
|
||||
const postLogoutRedirectUri =
|
||||
process.env.OPENID_POST_LOGOUT_REDIRECT_URI || `${process.env.DOMAIN_CLIENT}/login`;
|
||||
endSessionUrl.searchParams.set('post_logout_redirect_uri', postLogoutRedirectUri);
|
||||
|
||||
/** Add id_token_hint (preferred) or client_id for OIDC spec compliance */
|
||||
/**
|
||||
* OIDC RP-Initiated Logout cascading strategy:
|
||||
* 1. id_token_hint (most secure, identifies exact session)
|
||||
* 2. logout_hint + client_id (when URL would exceed safe length)
|
||||
* 3. client_id only (when no token available)
|
||||
*
|
||||
* JWT tokens from spec-compliant OIDC providers use base64url
|
||||
* encoding (RFC 7515), whose characters are all URL-safe, so
|
||||
* token length equals URL-encoded length for projection.
|
||||
* Non-compliant issuers using standard base64 (+/=) will cause
|
||||
* underestimation; increase OPENID_MAX_LOGOUT_URL_LENGTH if the
|
||||
* fallback does not trigger as expected.
|
||||
*/
|
||||
const maxLogoutUrlLength = parseMaxLogoutUrlLength();
|
||||
let strategy = 'no_token';
|
||||
if (idToken) {
|
||||
const baseLength = endSessionUrl.toString().length;
|
||||
const projectedLength = baseLength + '&id_token_hint='.length + idToken.length;
|
||||
if (projectedLength > maxLogoutUrlLength) {
|
||||
strategy = 'too_long';
|
||||
logger.debug(
|
||||
`[logoutController] Logout URL too long (${projectedLength} chars, max ${maxLogoutUrlLength}), ` +
|
||||
'switching to logout_hint strategy',
|
||||
);
|
||||
} else {
|
||||
strategy = 'use_token';
|
||||
}
|
||||
}
|
||||
|
||||
if (strategy === 'use_token') {
|
||||
endSessionUrl.searchParams.set('id_token_hint', idToken);
|
||||
} else if (process.env.OPENID_CLIENT_ID) {
|
||||
endSessionUrl.searchParams.set('client_id', process.env.OPENID_CLIENT_ID);
|
||||
} else {
|
||||
logger.warn(
|
||||
'[logoutController] Neither id_token_hint nor OPENID_CLIENT_ID is available. ' +
|
||||
'To enable id_token_hint, set OPENID_REUSE_TOKENS=true. ' +
|
||||
'The OIDC end-session request may be rejected by the identity provider.',
|
||||
);
|
||||
if (strategy === 'too_long') {
|
||||
const logoutHint = req.user?.email || req.user?.username || req.user?.openidId;
|
||||
if (logoutHint) {
|
||||
endSessionUrl.searchParams.set('logout_hint', logoutHint);
|
||||
}
|
||||
}
|
||||
|
||||
if (process.env.OPENID_CLIENT_ID) {
|
||||
endSessionUrl.searchParams.set('client_id', process.env.OPENID_CLIENT_ID);
|
||||
} else if (strategy === 'too_long') {
|
||||
logger.warn(
|
||||
'[logoutController] Logout URL exceeds max length and OPENID_CLIENT_ID is not set. ' +
|
||||
'The OIDC end-session request may be rejected. ' +
|
||||
'Consider setting OPENID_CLIENT_ID or increasing OPENID_MAX_LOGOUT_URL_LENGTH.',
|
||||
);
|
||||
} else {
|
||||
logger.warn(
|
||||
'[logoutController] Neither id_token_hint nor OPENID_CLIENT_ID is available. ' +
|
||||
'To enable id_token_hint, set OPENID_REUSE_TOKENS=true. ' +
|
||||
'The OIDC end-session request may be rejected by the identity provider.',
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
response.redirect = endSessionUrl.toString();
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
const cookies = require('cookie');
|
||||
|
||||
const mockLogoutUser = jest.fn();
|
||||
const mockLogger = { warn: jest.fn(), error: jest.fn() };
|
||||
const mockLogger = { warn: jest.fn(), error: jest.fn(), debug: jest.fn() };
|
||||
const mockIsEnabled = jest.fn();
|
||||
const mockGetOpenIdConfig = jest.fn();
|
||||
|
||||
|
|
@ -256,4 +256,312 @@ describe('LogoutController', () => {
|
|||
expect(res.clearCookie).toHaveBeenCalledWith('token_provider');
|
||||
});
|
||||
});
|
||||
|
||||
describe('URL length limit and logout_hint fallback', () => {
|
||||
it('uses logout_hint when id_token makes URL exceed default limit (2000 chars)', async () => {
|
||||
const longIdToken = 'a'.repeat(3000);
|
||||
const req = buildReq({
|
||||
user: { _id: 'user1', openidId: 'oid1', provider: 'openid', email: 'user@example.com' },
|
||||
session: {
|
||||
openidTokens: { refreshToken: 'srt', idToken: longIdToken },
|
||||
destroy: jest.fn(),
|
||||
},
|
||||
});
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).not.toContain('id_token_hint=');
|
||||
expect(body.redirect).toContain('logout_hint=user%40example.com');
|
||||
expect(body.redirect).toContain('client_id=my-client-id');
|
||||
expect(mockLogger.debug).toHaveBeenCalledWith(expect.stringContaining('Logout URL too long'));
|
||||
});
|
||||
|
||||
it('uses id_token_hint when URL is within default limit', async () => {
|
||||
const shortIdToken = 'short-token';
|
||||
const req = buildReq({
|
||||
session: {
|
||||
openidTokens: { refreshToken: 'srt', idToken: shortIdToken },
|
||||
destroy: jest.fn(),
|
||||
},
|
||||
});
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toContain('id_token_hint=short-token');
|
||||
expect(body.redirect).not.toContain('logout_hint=');
|
||||
expect(body.redirect).not.toContain('client_id=');
|
||||
});
|
||||
|
||||
it('respects custom OPENID_MAX_LOGOUT_URL_LENGTH', async () => {
|
||||
process.env.OPENID_MAX_LOGOUT_URL_LENGTH = '500';
|
||||
const mediumIdToken = 'a'.repeat(600);
|
||||
const req = buildReq({
|
||||
user: { _id: 'user1', openidId: 'oid1', provider: 'openid', email: 'user@example.com' },
|
||||
session: {
|
||||
openidTokens: { refreshToken: 'srt', idToken: mediumIdToken },
|
||||
destroy: jest.fn(),
|
||||
},
|
||||
});
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).not.toContain('id_token_hint=');
|
||||
expect(body.redirect).toContain('logout_hint=user%40example.com');
|
||||
});
|
||||
|
||||
it('uses username as logout_hint when email is not available', async () => {
|
||||
const longIdToken = 'a'.repeat(3000);
|
||||
const req = buildReq({
|
||||
user: {
|
||||
_id: 'user1',
|
||||
openidId: 'oid1',
|
||||
provider: 'openid',
|
||||
username: 'testuser',
|
||||
},
|
||||
session: {
|
||||
openidTokens: { refreshToken: 'srt', idToken: longIdToken },
|
||||
destroy: jest.fn(),
|
||||
},
|
||||
});
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toContain('logout_hint=testuser');
|
||||
});
|
||||
|
||||
it('uses openidId as logout_hint when email and username are not available', async () => {
|
||||
const longIdToken = 'a'.repeat(3000);
|
||||
const req = buildReq({
|
||||
user: { _id: 'user1', openidId: 'unique-oid-123', provider: 'openid' },
|
||||
session: {
|
||||
openidTokens: { refreshToken: 'srt', idToken: longIdToken },
|
||||
destroy: jest.fn(),
|
||||
},
|
||||
});
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toContain('logout_hint=unique-oid-123');
|
||||
});
|
||||
|
||||
it('uses openidId as logout_hint when email and username are explicitly null', async () => {
|
||||
const longIdToken = 'a'.repeat(3000);
|
||||
const req = buildReq({
|
||||
user: {
|
||||
_id: 'user1',
|
||||
openidId: 'oid-without-email',
|
||||
provider: 'openid',
|
||||
email: null,
|
||||
username: null,
|
||||
},
|
||||
session: {
|
||||
openidTokens: { refreshToken: 'srt', idToken: longIdToken },
|
||||
destroy: jest.fn(),
|
||||
},
|
||||
});
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).not.toContain('id_token_hint=');
|
||||
expect(body.redirect).toContain('logout_hint=oid-without-email');
|
||||
expect(body.redirect).toContain('client_id=my-client-id');
|
||||
});
|
||||
|
||||
it('uses only client_id when absolutely no hint is available', async () => {
|
||||
const longIdToken = 'a'.repeat(3000);
|
||||
const req = buildReq({
|
||||
user: {
|
||||
_id: 'user1',
|
||||
openidId: '',
|
||||
provider: 'openid',
|
||||
email: '',
|
||||
username: '',
|
||||
},
|
||||
session: {
|
||||
openidTokens: { refreshToken: 'srt', idToken: longIdToken },
|
||||
destroy: jest.fn(),
|
||||
},
|
||||
});
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).not.toContain('id_token_hint=');
|
||||
expect(body.redirect).not.toContain('logout_hint=');
|
||||
expect(body.redirect).toContain('client_id=my-client-id');
|
||||
});
|
||||
|
||||
it('warns about missing OPENID_CLIENT_ID when URL is too long', async () => {
|
||||
delete process.env.OPENID_CLIENT_ID;
|
||||
const longIdToken = 'a'.repeat(3000);
|
||||
const req = buildReq({
|
||||
user: { _id: 'user1', openidId: 'oid1', provider: 'openid', email: 'user@example.com' },
|
||||
session: {
|
||||
openidTokens: { refreshToken: 'srt', idToken: longIdToken },
|
||||
destroy: jest.fn(),
|
||||
},
|
||||
});
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).not.toContain('id_token_hint=');
|
||||
expect(body.redirect).toContain('logout_hint=');
|
||||
expect(body.redirect).not.toContain('client_id=');
|
||||
expect(mockLogger.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('OPENID_CLIENT_ID is not set'),
|
||||
);
|
||||
});
|
||||
|
||||
it('falls back to logout_hint for cookie-sourced long token', async () => {
|
||||
const longCookieToken = 'a'.repeat(3000);
|
||||
cookies.parse.mockReturnValue({
|
||||
refreshToken: 'cookie-rt',
|
||||
openid_id_token: longCookieToken,
|
||||
});
|
||||
const req = buildReq({
|
||||
user: { _id: 'user1', openidId: 'oid1', provider: 'openid', email: 'user@example.com' },
|
||||
session: { destroy: jest.fn() },
|
||||
});
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).not.toContain('id_token_hint=');
|
||||
expect(body.redirect).toContain('logout_hint=user%40example.com');
|
||||
expect(body.redirect).toContain('client_id=my-client-id');
|
||||
});
|
||||
|
||||
it('keeps id_token_hint when projected URL length equals the max', async () => {
|
||||
const baseUrl = new URL('https://idp.example.com/logout');
|
||||
baseUrl.searchParams.set('post_logout_redirect_uri', 'https://app.example.com/login');
|
||||
const baseLength = baseUrl.toString().length;
|
||||
const tokenLength = 2000 - baseLength - '&id_token_hint='.length;
|
||||
const exactToken = 'a'.repeat(tokenLength);
|
||||
|
||||
const req = buildReq({
|
||||
session: {
|
||||
openidTokens: { refreshToken: 'srt', idToken: exactToken },
|
||||
destroy: jest.fn(),
|
||||
},
|
||||
});
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toContain('id_token_hint=');
|
||||
expect(body.redirect).not.toContain('logout_hint=');
|
||||
});
|
||||
|
||||
it('falls back to logout_hint when projected URL is one char over the max', async () => {
|
||||
const baseUrl = new URL('https://idp.example.com/logout');
|
||||
baseUrl.searchParams.set('post_logout_redirect_uri', 'https://app.example.com/login');
|
||||
const baseLength = baseUrl.toString().length;
|
||||
const tokenLength = 2000 - baseLength - '&id_token_hint='.length + 1;
|
||||
const overToken = 'a'.repeat(tokenLength);
|
||||
|
||||
const req = buildReq({
|
||||
user: { _id: 'user1', openidId: 'oid1', provider: 'openid', email: 'user@example.com' },
|
||||
session: {
|
||||
openidTokens: { refreshToken: 'srt', idToken: overToken },
|
||||
destroy: jest.fn(),
|
||||
},
|
||||
});
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).not.toContain('id_token_hint=');
|
||||
expect(body.redirect).toContain('logout_hint=');
|
||||
});
|
||||
});
|
||||
|
||||
describe('invalid OPENID_MAX_LOGOUT_URL_LENGTH values', () => {
|
||||
it('silently uses default when value is empty', async () => {
|
||||
process.env.OPENID_MAX_LOGOUT_URL_LENGTH = '';
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
expect(mockLogger.warn).not.toHaveBeenCalledWith(
|
||||
expect.stringContaining('Invalid OPENID_MAX_LOGOUT_URL_LENGTH'),
|
||||
);
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toContain('id_token_hint=small-id-token');
|
||||
});
|
||||
|
||||
it('warns and uses default for partial numeric string', async () => {
|
||||
process.env.OPENID_MAX_LOGOUT_URL_LENGTH = '500abc';
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
expect(mockLogger.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Invalid OPENID_MAX_LOGOUT_URL_LENGTH'),
|
||||
);
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toContain('id_token_hint=small-id-token');
|
||||
});
|
||||
|
||||
it('warns and uses default for zero value', async () => {
|
||||
process.env.OPENID_MAX_LOGOUT_URL_LENGTH = '0';
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
expect(mockLogger.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Invalid OPENID_MAX_LOGOUT_URL_LENGTH'),
|
||||
);
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toContain('id_token_hint=small-id-token');
|
||||
});
|
||||
|
||||
it('warns and uses default for negative value', async () => {
|
||||
process.env.OPENID_MAX_LOGOUT_URL_LENGTH = '-1';
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
expect(mockLogger.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Invalid OPENID_MAX_LOGOUT_URL_LENGTH'),
|
||||
);
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toContain('id_token_hint=small-id-token');
|
||||
});
|
||||
|
||||
it('warns and uses default for non-numeric string', async () => {
|
||||
process.env.OPENID_MAX_LOGOUT_URL_LENGTH = 'abc';
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
expect(mockLogger.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Invalid OPENID_MAX_LOGOUT_URL_LENGTH'),
|
||||
);
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toContain('id_token_hint=small-id-token');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue