From be751ad5219e07e96bb0060b6d7420caf123f54a Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 17:55:20 +0200 Subject: [PATCH] fix(auth): start no new session for a password change over a Bearer token A password change ends every session of the user and re-issues one for the device it was made on, handed out as the session cookie. A caller that authenticated with a Bearer token never reads that cookie, so the new session sat in the session list as a live device nobody held until it expired. The controller now asks for the replacement session only when the request came with the session cookie, the same check the sliding renewal makes. The response body is unchanged. --- server/src/nest/auth/auth.controller.ts | 12 ++++++++---- server/src/nest/auth/auth.service.ts | 10 +++++----- server/tests/e2e/sessions.e2e.test.ts | 17 +++++++++++++++++ server/tests/unit/nest/auth.controller.test.ts | 12 +++++++++++- server/tests/unit/nest/auth.service.test.ts | 13 +++++++++++++ wiki/Security-Hardening.md | 2 +- 6 files changed, 55 insertions(+), 11 deletions(-) diff --git a/server/src/nest/auth/auth.controller.ts b/server/src/nest/auth/auth.controller.ts index 325c9a0bf..efd36e733 100644 --- a/server/src/nest/auth/auth.controller.ts +++ b/server/src/nest/auth/auth.controller.ts @@ -111,10 +111,14 @@ export class AuthController { async changePassword(@CurrentUser() user: User, @Body() body: ChangePasswordDto, @Req() req: Request, @Res({ passthrough: true }) res: Response) { await this.limit('login', req, 5); // Carry the session's remember choice into the re-issued token/cookie so a - // "remember me" login survives a password change (#1927). Bearer callers - // have no cookie → undefined → the historical default duration. - const remember = decodeSessionClaims((req.cookies as Record | undefined)?.trek_session)?.remember; - const result = await this.auth.changePassword(user.id, user.email, body, remember, sessionClientFrom(req)); + // "remember me" login survives a password change (#1927). The guard + // verified the cookie whenever there is one (extractToken prefers it), the + // same check the renewal interceptor makes. A Bearer caller has none: it + // would never see a re-issued token, so no new session is started for it. + const cookie = (req.cookies as Record | undefined)?.trek_session; + const remember = decodeSessionClaims(cookie)?.remember; + const cookieSession = typeof cookie === 'string' && cookie.length > 0; + const result = await this.auth.changePassword(user.id, user.email, body, remember, sessionClientFrom(req), cookieSession); if (result.error) { throw new HttpException({ error: result.error }, result.status!); } diff --git a/server/src/nest/auth/auth.service.ts b/server/src/nest/auth/auth.service.ts index f95b9b56f..41d8481ca 100644 --- a/server/src/nest/auth/auth.service.ts +++ b/server/src/nest/auth/auth.service.ts @@ -624,6 +624,7 @@ export class AuthService { rawBody: unknown, remember?: boolean, client?: SessionClient, + issueSession = true, ): Promise<{ error?: string; status?: number; success?: boolean; token?: string }> { const body = rawBody as { current_password?: string; new_password?: string }; if (await this.isOidcOnlyMode()) { @@ -654,9 +655,7 @@ export class AuthService { // before sessions were tracked) and prunes the MCP static token and OAuth // bearer-token stores to match, the same set the password reset revokes. await this.mcpTokens.deleteAllForUser(userId); - try { - await this.oauthTokens.revokeAllForUser(userId); - } catch { /* oauth_tokens table may not exist in very old installs */ } + try { await this.oauthTokens.revokeAllForUser(userId); } catch { /* no oauth_tokens in very old installs */ } // Push devices keep receiving notifications without any session, so they // go too. The device the change was made on registers again right away: // the client reloads the user after the change, and that re-syncs its @@ -667,8 +666,9 @@ export class AuthService { try { revokeUserSessions?.(userId); } catch { /* best-effort */ } - // Re-issue a session for the current device only, keeping the login's remember - // choice instead of downgrading it to the default duration (#1927). + // Re-issue a session for the current device only, with the login's remember choice (#1927). + // Not for a Bearer caller (`issueSession` false): it never receives the token (see the controller). + if (!issueSession) return { success: true }; const token = await this.generateToken({ id: userId, password_version: newPv }, remember, client); return { success: true, token }; } diff --git a/server/tests/e2e/sessions.e2e.test.ts b/server/tests/e2e/sessions.e2e.test.ts index aa785f98c..ad798f6c7 100644 --- a/server/tests/e2e/sessions.e2e.test.ts +++ b/server/tests/e2e/sessions.e2e.test.ts @@ -213,6 +213,23 @@ describe('Sessions e2e (sign-in sessions that can be ended)', () => { expect(list.body.sessions).toEqual([expect.objectContaining({ id: sessionIdOf(next), user_agent: 'Here', current: true })]); }, 15000); + it('a password change over a Bearer token ends every session and starts none, since no cookie carries it', async () => { + const { user, password } = freshUser('sess-password-bearer'); + const cookie = await signIn(user.email, password, 'Api client'); + const bearer = cookie.slice('trek_session='.length); + + const change = await request(server) + .put('/api/auth/me/password') + .set('Authorization', `Bearer ${bearer}`) + .send({ current_password: password, new_password: 'New1234!x' }); + expect(change.status).toBe(200); + expect(change.body).toEqual({ success: true }); + expect(((change.headers['set-cookie'] ?? []) as unknown as string[]).some((c) => c.startsWith('trek_session='))).toBe(false); + + const rows = sessionRows(db as never, user.id); + expect(rows).toEqual([expect.objectContaining({ id: sessionIdOf(cookie), revoked_at: expect.any(String) })]); + }, 15000); + it('a token from before sessions were tracked keeps working, is listed as untracked, and survives "sign out others"', async () => { const { user, password } = freshUser('sess-legacy'); const legacy = sessionCookie(user.id); diff --git a/server/tests/unit/nest/auth.controller.test.ts b/server/tests/unit/nest/auth.controller.test.ts index b01291be5..5d60d96dd 100644 --- a/server/tests/unit/nest/auth.controller.test.ts +++ b/server/tests/unit/nest/auth.controller.test.ts @@ -352,10 +352,20 @@ describe('AuthController (authenticated)', () => { const changePassword = vi.fn().mockReturnValue({ token: 'tk3' }); const c = ac(asvc({ changePassword, setAuthCookie } as Partial), rl()); expect(await c.changePassword(user, anyBody(), reqCookie, res)).toEqual({ success: true }); - expect(changePassword).toHaveBeenCalledWith(1, 'u@example.test', anyBody(), true, { userAgent: null }); + expect(changePassword).toHaveBeenCalledWith(1, 'u@example.test', anyBody(), true, { userAgent: null }, true); expect(setAuthCookie).toHaveBeenCalledWith(res, 'tk3', reqCookie, true); }); + it('change-password starts no new session for a Bearer caller, who has no cookie to receive it', async () => { + const setAuthCookie = vi.fn(); + const changePassword = vi.fn().mockReturnValue({ success: true }); + const bearer = { ip: '9.9.9.9', headers: { authorization: 'Bearer abc' } } as unknown as Request; + const c = ac(asvc({ changePassword, setAuthCookie } as Partial), rl()); + expect(await c.changePassword(user, anyBody(), bearer, res)).toEqual({ success: true }); + expect(changePassword).toHaveBeenCalledWith(1, 'u@example.test', anyBody(), undefined, { userAgent: null }, false); + expect(setAuthCookie).not.toHaveBeenCalled(); + }); + it('delete-account maps error, else audits and succeeds', async () => { expect(await thrownAsync(() => ac(asvc({ deleteAccount: vi.fn().mockReturnValue({ error: 'Last admin', status: 403 }) } as Partial), rl()).deleteAccount(user, req))).toEqual({ status: 403, body: { error: 'Last admin' } }); expect(await ac(asvc({ deleteAccount: vi.fn().mockReturnValue({}) } as Partial), rl()).deleteAccount(user, req)).toEqual({ success: true }); diff --git a/server/tests/unit/nest/auth.service.test.ts b/server/tests/unit/nest/auth.service.test.ts index 36382fae7..f803b5987 100644 --- a/server/tests/unit/nest/auth.service.test.ts +++ b/server/tests/unit/nest/auth.service.test.ts @@ -528,6 +528,19 @@ describe('changePassword — session invalidation', () => { expect(await verifyJwtAndLoadUser(stolen, await createTestUsersRepo(testDb), await createTestUserSessionsRepo(testDb))).toBeNull(); // invalidated by the pv bump }); + it('AUTH-DB-036g: without issueSession it ends every session and starts none (a Bearer caller)', async () => { + const { user, password } = createUser(testDb); + await svc.generateToken({ id: user.id }); + + const result = await svc.changePassword(user.id, user.email, { current_password: password, new_password: 'New1234!' }, undefined, {}, false); + + expect(result).toEqual({ success: true }); + expect(pvOf(user.id)).toBe(1); + const rows = sessionRows(testDb, user.id); + expect(rows).toHaveLength(1); + expect(rows.every((row) => row.revoked_at !== null)).toBe(true); + }); + it('AUTH-DB-036d: preserves the remember choice in the re-issued session (#1927)', async () => { // eslint-disable-next-line @typescript-eslint/no-require-imports const jwt = require('jsonwebtoken'); diff --git a/wiki/Security-Hardening.md b/wiki/Security-Hardening.md index 38e624c4f..295833505 100644 --- a/wiki/Security-Hardening.md +++ b/wiki/Security-Hardening.md @@ -42,7 +42,7 @@ A session ends when: | **Log out** (`POST /api/auth/logout`) | The one it is called with. Clearing the cookie alone used to leave the token valid. | | `DELETE /api/auth/sessions/{id}` | That one session of your own account. | | `POST /api/auth/sessions/revoke-others` | Every session of your account but the current one. | -| Password change | Every session; the device the change was made on gets a new one. | +| Password change | Every session. A browser that made the change with its session cookie gets a new one; an API client calling it with a Bearer token gets none, since it would never receive it. | | Password reset by email | Every session. | | Admin sets a new password for a user | Every session of that user. | | Recovery script `reset-admin.js` | Every session of the account it resets. |