mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-18 21:56:33 +01:00
🔏 fix: Remove Federated Tokens from OpenID Refresh Response (#12264)
* 🔒 fix: Remove OpenID federated tokens from refresh endpoint response The refresh controller was attaching federatedTokens (including the refresh_token) to the user object returned in the JSON response, exposing HttpOnly-protected tokens to client-side JavaScript. The tokens are already stored server-side by setOpenIDAuthTokens and re-attached by the JWT strategy on authenticated requests. * 🔒 fix: Strip sensitive fields from OpenID refresh response user object The OpenID refresh path returned the raw findOpenIDUser result without field projection, unlike the non-OpenID path which excludes password, __v, totpSecret, and backupCodes via getUserById projection. Destructure out sensitive fields before serializing. Also strengthens the regression test: uses not.toHaveProperty for true property-absence checks (expect.anything() misses null/undefined), adds positive shape assertion, and DRYs up duplicated mock user setup.
This commit is contained in:
parent
381ed8539b
commit
d17ac8f06d
2 changed files with 32 additions and 22 deletions
|
|
@ -119,14 +119,8 @@ const refreshController = async (req, res) => {
|
|||
|
||||
const token = setOpenIDAuthTokens(tokenset, req, res, user._id.toString(), refreshToken);
|
||||
|
||||
user.federatedTokens = {
|
||||
access_token: tokenset.access_token,
|
||||
id_token: tokenset.id_token,
|
||||
refresh_token: refreshToken,
|
||||
expires_at: claims.exp,
|
||||
};
|
||||
|
||||
return res.status(200).send({ token, user });
|
||||
const { password: _pw, __v: _v, totpSecret: _ts, backupCodes: _bc, ...safeUser } = user;
|
||||
return res.status(200).send({ token, user: safeUser });
|
||||
} catch (error) {
|
||||
logger.error('[refreshController] OpenID token refresh error', error);
|
||||
return res.status(403).send('Invalid OpenID refresh token');
|
||||
|
|
|
|||
|
|
@ -163,6 +163,16 @@ describe('refreshController – OpenID path', () => {
|
|||
exp: 9999999999,
|
||||
};
|
||||
|
||||
const defaultUser = {
|
||||
_id: 'user-db-id',
|
||||
email: baseClaims.email,
|
||||
openidId: baseClaims.sub,
|
||||
password: '$2b$10$hashedpassword',
|
||||
__v: 0,
|
||||
totpSecret: 'encrypted-totp-secret',
|
||||
backupCodes: ['hashed-code-1', 'hashed-code-2'],
|
||||
};
|
||||
|
||||
let req, res;
|
||||
|
||||
beforeEach(() => {
|
||||
|
|
@ -174,6 +184,7 @@ describe('refreshController – OpenID path', () => {
|
|||
mockTokenset.claims.mockReturnValue(baseClaims);
|
||||
getOpenIdEmail.mockReturnValue(baseClaims.email);
|
||||
setOpenIDAuthTokens.mockReturnValue('new-app-token');
|
||||
findOpenIDUser.mockResolvedValue({ user: { ...defaultUser }, error: null, migration: false });
|
||||
updateUser.mockResolvedValue({});
|
||||
|
||||
req = {
|
||||
|
|
@ -189,13 +200,6 @@ describe('refreshController – OpenID path', () => {
|
|||
});
|
||||
|
||||
it('should call getOpenIdEmail with token claims and use result for findOpenIDUser', async () => {
|
||||
const user = {
|
||||
_id: 'user-db-id',
|
||||
email: baseClaims.email,
|
||||
openidId: baseClaims.sub,
|
||||
};
|
||||
findOpenIDUser.mockResolvedValue({ user, error: null, migration: false });
|
||||
|
||||
await refreshController(req, res);
|
||||
|
||||
expect(getOpenIdEmail).toHaveBeenCalledWith(baseClaims);
|
||||
|
|
@ -229,13 +233,6 @@ describe('refreshController – OpenID path', () => {
|
|||
it('should fall back to claims.email when configured claim is absent from token claims', async () => {
|
||||
getOpenIdEmail.mockReturnValue(baseClaims.email);
|
||||
|
||||
const user = {
|
||||
_id: 'user-db-id',
|
||||
email: baseClaims.email,
|
||||
openidId: baseClaims.sub,
|
||||
};
|
||||
findOpenIDUser.mockResolvedValue({ user, error: null, migration: false });
|
||||
|
||||
await refreshController(req, res);
|
||||
|
||||
expect(findOpenIDUser).toHaveBeenCalledWith(
|
||||
|
|
@ -243,6 +240,25 @@ describe('refreshController – OpenID path', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('should not expose sensitive fields or federatedTokens in refresh response', async () => {
|
||||
await refreshController(req, res);
|
||||
|
||||
const sentPayload = res.send.mock.calls[0][0];
|
||||
expect(sentPayload).toEqual({
|
||||
token: 'new-app-token',
|
||||
user: expect.objectContaining({
|
||||
_id: 'user-db-id',
|
||||
email: baseClaims.email,
|
||||
openidId: baseClaims.sub,
|
||||
}),
|
||||
});
|
||||
expect(sentPayload.user).not.toHaveProperty('federatedTokens');
|
||||
expect(sentPayload.user).not.toHaveProperty('password');
|
||||
expect(sentPayload.user).not.toHaveProperty('totpSecret');
|
||||
expect(sentPayload.user).not.toHaveProperty('backupCodes');
|
||||
expect(sentPayload.user).not.toHaveProperty('__v');
|
||||
});
|
||||
|
||||
it('should update openidId when migration is triggered on refresh', async () => {
|
||||
const user = { _id: 'user-db-id', email: baseClaims.email, openidId: null };
|
||||
findOpenIDUser.mockResolvedValue({ user, error: null, migration: true });
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue