mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-03 14:50:19 +01:00
🚪 fix: Complete OIDC RP-Initiated Logout With id_token_hint and Redirect Race Fix (#12024)
* fix: complete OIDC logout implementation The OIDC logout feature added in #5626 was incomplete: 1. Backend: Missing id_token_hint/client_id parameters required by the RP-Initiated Logout spec. Keycloak 18+ rejects logout without these. 2. Frontend: The logout redirect URL was passed through isSafeRedirect() which rejects all absolute URLs. The redirect was silently dropped. Backend: Add id_token_hint (preferred) or client_id (fallback) to the logout URL for OIDC spec compliance. Frontend: Use window.location.replace() for logout redirects from the backend, bypassing isSafeRedirect() which was designed for user-input validation. Fixes #5506 * fix: accept undefined in setTokenHeader to properly clear Authorization header When token is undefined, delete the Authorization header instead of setting it to "Bearer undefined". Removes the @ts-ignore workaround in AuthContext. * fix: skip axios 401 refresh when Authorization header is cleared When the Authorization header has been removed (e.g. during logout), the response interceptor now skips the token refresh flow. This prevents a successful refresh from canceling an in-progress OIDC external redirect via window.location.replace(). * fix: guard against undefined OPENID_CLIENT_ID in logout URL Prevent literal "client_id=undefined" in the OIDC end-session URL when OPENID_CLIENT_ID is not set. Log a warning when neither id_token_hint nor client_id is available. * fix: prevent race condition canceling OIDC logout redirect The logout mutation wrapper's cleanup (clearStates, removeQueries) triggers re-renders and 401s on in-flight requests. The axios interceptor would refresh the token successfully, firing dispatchTokenUpdatedEvent which cancels the window.location.replace() navigation to the IdP's end_session_endpoint. Fix: - Clear Authorization header synchronously before redirect so the axios interceptor skips refresh for post-logout 401s - Add isExternalRedirectRef to suppress silentRefresh and useEffect side effects during the redirect - Add JSDoc explaining why isSafeRedirect is bypassed * test: add LogoutController and AuthContext logout test coverage LogoutController.spec.js (13 tests): - id_token_hint from session and cookie fallback - client_id fallback, including undefined OPENID_CLIENT_ID guard - Disabled endpoint, missing issuer, non-OpenID user - post_logout_redirect_uri (custom and default) - Missing OpenID config and end_session_endpoint - Error handling and cookie clearing AuthContext.spec.tsx (3 tests): - OIDC redirect calls window.location.replace + setTokenHeader - Non-redirect logout path - Logout error handling * test: add coverage for setTokenHeader, axios interceptor guard, and silentRefresh suppression headers-helpers.spec.ts (3 tests): - Sets Authorization header with Bearer token - Deletes Authorization header when called with undefined - No-op when clearing an already absent header request-interceptor.spec.ts (2 tests): - Skips refresh when Authorization header is cleared (the race fix) - Attempts refresh when Authorization header is present AuthContext.spec.tsx (1 new test): - Verifies silentRefresh is not triggered after OIDC redirect * test: enhance request-interceptor tests with adapter restoration and refresh verification - Store the original axios adapter before tests and restore it after all tests to prevent side effects. - Add verification for the refresh endpoint call in the interceptor tests to ensure correct behavior during token refresh attempts. * test: enhance AuthContext tests with live rendering and improved logout error handling - Introduced a new `renderProviderLive` function to facilitate testing with silentRefresh. - Updated tests to use the live rendering function, ensuring accurate simulation of authentication behavior. - Enhanced logout error handling test to verify that auth state is cleared without external redirects. * test: update LogoutController tests for OpenID config error handling - Renamed test suite to clarify that it handles cases when OpenID config is not available. - Modified test to check for error thrown by getOpenIdConfig instead of returning null, ensuring proper logging of the error message. * refactor: improve OpenID config error handling in LogoutController - Simplified error handling for OpenID configuration retrieval by using a try-catch block. - Updated logging to provide clearer messages when the OpenID config is unavailable. - Ensured that the end session endpoint is only accessed if the OpenID config is successfully retrieved. --------- Co-authored-by: cloudspinner <stijn.tastenhoye@gmail.com>
This commit is contained in:
parent
c0236b4ba7
commit
b18915a96b
8 changed files with 568 additions and 17 deletions
|
|
@ -8,13 +8,16 @@ 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 refresh token from session; for others, use cookie */
|
||||
/** For OpenID users, read tokens from session (with cookie fallback) */
|
||||
let refreshToken;
|
||||
let idToken;
|
||||
if (isOpenIdUser && req.session?.openidTokens) {
|
||||
refreshToken = req.session.openidTokens.refreshToken;
|
||||
idToken = req.session.openidTokens.idToken;
|
||||
delete req.session.openidTokens;
|
||||
}
|
||||
refreshToken = refreshToken || parsedCookies.refreshToken;
|
||||
idToken = idToken || parsedCookies.openid_id_token;
|
||||
|
||||
try {
|
||||
const logout = await logoutUser(req, refreshToken);
|
||||
|
|
@ -31,21 +34,34 @@ const logoutController = async (req, res) => {
|
|||
isEnabled(process.env.OPENID_USE_END_SESSION_ENDPOINT) &&
|
||||
process.env.OPENID_ISSUER
|
||||
) {
|
||||
const openIdConfig = getOpenIdConfig();
|
||||
if (!openIdConfig) {
|
||||
logger.warn(
|
||||
'[logoutController] OpenID config not found. Please verify that the open id configuration and initialization are correct.',
|
||||
);
|
||||
} else {
|
||||
const endSessionEndpoint = openIdConfig
|
||||
? openIdConfig.serverMetadata().end_session_endpoint
|
||||
: null;
|
||||
let openIdConfig;
|
||||
try {
|
||||
openIdConfig = getOpenIdConfig();
|
||||
} catch (err) {
|
||||
logger.warn('[logoutController] OpenID config not available:', err.message);
|
||||
}
|
||||
if (openIdConfig) {
|
||||
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 */
|
||||
if (idToken) {
|
||||
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.',
|
||||
);
|
||||
}
|
||||
|
||||
response.redirect = endSessionUrl.toString();
|
||||
} else {
|
||||
logger.warn(
|
||||
|
|
|
|||
259
api/server/controllers/auth/LogoutController.spec.js
Normal file
259
api/server/controllers/auth/LogoutController.spec.js
Normal file
|
|
@ -0,0 +1,259 @@
|
|||
const cookies = require('cookie');
|
||||
|
||||
const mockLogoutUser = jest.fn();
|
||||
const mockLogger = { warn: jest.fn(), error: jest.fn() };
|
||||
const mockIsEnabled = jest.fn();
|
||||
const mockGetOpenIdConfig = jest.fn();
|
||||
|
||||
jest.mock('cookie');
|
||||
jest.mock('@librechat/api', () => ({ isEnabled: (...args) => mockIsEnabled(...args) }));
|
||||
jest.mock('@librechat/data-schemas', () => ({ logger: mockLogger }));
|
||||
jest.mock('~/server/services/AuthService', () => ({
|
||||
logoutUser: (...args) => mockLogoutUser(...args),
|
||||
}));
|
||||
jest.mock('~/strategies', () => ({ getOpenIdConfig: () => mockGetOpenIdConfig() }));
|
||||
|
||||
const { logoutController } = require('./LogoutController');
|
||||
|
||||
function buildReq(overrides = {}) {
|
||||
return {
|
||||
user: { _id: 'user1', openidId: 'oid1', provider: 'openid' },
|
||||
headers: { cookie: 'refreshToken=rt1' },
|
||||
session: {
|
||||
openidTokens: { refreshToken: 'srt', idToken: 'small-id-token' },
|
||||
destroy: jest.fn(),
|
||||
},
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
function buildRes() {
|
||||
const res = {
|
||||
status: jest.fn().mockReturnThis(),
|
||||
send: jest.fn().mockReturnThis(),
|
||||
json: jest.fn().mockReturnThis(),
|
||||
clearCookie: jest.fn(),
|
||||
};
|
||||
return res;
|
||||
}
|
||||
|
||||
const ORIGINAL_ENV = process.env;
|
||||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
process.env = {
|
||||
...ORIGINAL_ENV,
|
||||
OPENID_USE_END_SESSION_ENDPOINT: 'true',
|
||||
OPENID_ISSUER: 'https://idp.example.com',
|
||||
OPENID_CLIENT_ID: 'my-client-id',
|
||||
DOMAIN_CLIENT: 'https://app.example.com',
|
||||
};
|
||||
cookies.parse.mockReturnValue({ refreshToken: 'cookie-rt' });
|
||||
mockLogoutUser.mockResolvedValue({ status: 200, message: 'Logout successful' });
|
||||
mockIsEnabled.mockReturnValue(true);
|
||||
mockGetOpenIdConfig.mockReturnValue({
|
||||
serverMetadata: () => ({
|
||||
end_session_endpoint: 'https://idp.example.com/logout',
|
||||
}),
|
||||
});
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
process.env = ORIGINAL_ENV;
|
||||
});
|
||||
|
||||
describe('LogoutController', () => {
|
||||
describe('id_token_hint from session', () => {
|
||||
it('sets id_token_hint when session has idToken', async () => {
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toContain('id_token_hint=small-id-token');
|
||||
expect(body.redirect).not.toContain('client_id=');
|
||||
});
|
||||
});
|
||||
|
||||
describe('id_token_hint from cookie fallback', () => {
|
||||
it('uses cookie id_token when session has no tokens', async () => {
|
||||
cookies.parse.mockReturnValue({
|
||||
refreshToken: 'cookie-rt',
|
||||
openid_id_token: 'cookie-id-token',
|
||||
});
|
||||
const req = buildReq({ session: { 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=cookie-id-token');
|
||||
});
|
||||
});
|
||||
|
||||
describe('client_id fallback', () => {
|
||||
it('falls back to client_id when no idToken is available', async () => {
|
||||
cookies.parse.mockReturnValue({ refreshToken: 'cookie-rt' });
|
||||
const req = buildReq({ session: { destroy: jest.fn() } });
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toContain('client_id=my-client-id');
|
||||
expect(body.redirect).not.toContain('id_token_hint=');
|
||||
});
|
||||
|
||||
it('does not produce client_id=undefined when OPENID_CLIENT_ID is unset', async () => {
|
||||
delete process.env.OPENID_CLIENT_ID;
|
||||
cookies.parse.mockReturnValue({ refreshToken: 'cookie-rt' });
|
||||
const req = buildReq({ session: { destroy: jest.fn() } });
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).not.toContain('client_id=');
|
||||
expect(body.redirect).not.toContain('undefined');
|
||||
expect(mockLogger.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Neither id_token_hint nor OPENID_CLIENT_ID'),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('OPENID_USE_END_SESSION_ENDPOINT disabled', () => {
|
||||
it('does not include redirect when disabled', async () => {
|
||||
mockIsEnabled.mockReturnValue(false);
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('OPENID_ISSUER unset', () => {
|
||||
it('does not include redirect when OPENID_ISSUER is missing', async () => {
|
||||
delete process.env.OPENID_ISSUER;
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('non-OpenID user', () => {
|
||||
it('does not include redirect for non-OpenID users', async () => {
|
||||
const req = buildReq({
|
||||
user: { _id: 'user1', provider: 'local' },
|
||||
});
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('post_logout_redirect_uri', () => {
|
||||
it('uses OPENID_POST_LOGOUT_REDIRECT_URI when set', async () => {
|
||||
process.env.OPENID_POST_LOGOUT_REDIRECT_URI = 'https://custom.example.com/logged-out';
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
const url = new URL(body.redirect);
|
||||
expect(url.searchParams.get('post_logout_redirect_uri')).toBe(
|
||||
'https://custom.example.com/logged-out',
|
||||
);
|
||||
});
|
||||
|
||||
it('defaults to DOMAIN_CLIENT/login when OPENID_POST_LOGOUT_REDIRECT_URI is unset', async () => {
|
||||
delete process.env.OPENID_POST_LOGOUT_REDIRECT_URI;
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
const url = new URL(body.redirect);
|
||||
expect(url.searchParams.get('post_logout_redirect_uri')).toBe(
|
||||
'https://app.example.com/login',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('OpenID config not available', () => {
|
||||
it('warns and returns no redirect when getOpenIdConfig throws', async () => {
|
||||
mockGetOpenIdConfig.mockImplementation(() => {
|
||||
throw new Error('OpenID configuration has not been initialized');
|
||||
});
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toBeUndefined();
|
||||
expect(mockLogger.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('OpenID config not available'),
|
||||
'OpenID configuration has not been initialized',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('end_session_endpoint not in metadata', () => {
|
||||
it('warns and returns no redirect when end_session_endpoint is missing', async () => {
|
||||
mockGetOpenIdConfig.mockReturnValue({
|
||||
serverMetadata: () => ({}),
|
||||
});
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
const body = res.send.mock.calls[0][0];
|
||||
expect(body.redirect).toBeUndefined();
|
||||
expect(mockLogger.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('end_session_endpoint not found'),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('error handling', () => {
|
||||
it('returns 500 on logoutUser error', async () => {
|
||||
mockLogoutUser.mockRejectedValue(new Error('session error'));
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
expect(res.status).toHaveBeenCalledWith(500);
|
||||
expect(res.json).toHaveBeenCalledWith({ message: 'session error' });
|
||||
});
|
||||
});
|
||||
|
||||
describe('cookie clearing', () => {
|
||||
it('clears all auth cookies on successful logout', async () => {
|
||||
const req = buildReq();
|
||||
const res = buildRes();
|
||||
|
||||
await logoutController(req, res);
|
||||
|
||||
expect(res.clearCookie).toHaveBeenCalledWith('refreshToken');
|
||||
expect(res.clearCookie).toHaveBeenCalledWith('openid_access_token');
|
||||
expect(res.clearCookie).toHaveBeenCalledWith('openid_id_token');
|
||||
expect(res.clearCookie).toHaveBeenCalledWith('openid_user_id');
|
||||
expect(res.clearCookie).toHaveBeenCalledWith('token_provider');
|
||||
});
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue