From f4bbe80144b783c297130e9da204e22a030999bf Mon Sep 17 00:00:00 2001 From: Tink Date: Sun, 21 Jun 2026 18:22:30 +0200 Subject: [PATCH] fix(auth): dedupe and retry token refresh to prevent spurious logouts (#2948) --- frontend/src/helpers/auth.test.ts | 153 ++++++++++++++++++++ frontend/src/helpers/auth.ts | 59 +++++++- frontend/src/stores/auth.renewToken.test.ts | 139 ++++++++++++++++++ frontend/src/stores/auth.ts | 15 +- 4 files changed, 358 insertions(+), 8 deletions(-) create mode 100644 frontend/src/helpers/auth.test.ts create mode 100644 frontend/src/stores/auth.renewToken.test.ts diff --git a/frontend/src/helpers/auth.test.ts b/frontend/src/helpers/auth.test.ts new file mode 100644 index 000000000..c57f896b8 --- /dev/null +++ b/frontend/src/helpers/auth.test.ts @@ -0,0 +1,153 @@ +import {describe, it, expect, vi, beforeEach, afterEach} from 'vitest' + +import {refreshToken, removeToken} from './auth' + +// Count how many times the refresh endpoint is actually POSTed. The whole point +// of the in-flight dedup is that concurrent refreshToken() calls share a single +// underlying POST, independent of the Web Locks API. +let postCallCount = 0 +let resolvePost: ((value: unknown) => void) | null = null + +vi.mock('@/helpers/fetcher', () => ({ + HTTPFactory: () => ({ + post: vi.fn(() => { + postCallCount++ + return new Promise((resolve) => { + resolvePost = resolve + }) + }), + }), +})) + +vi.mock('@/helpers/desktopAuth', () => ({ + isDesktopApp: () => false, + refreshDesktopToken: vi.fn(), +})) + +const FAKE_TOKEN = 'header.payload.signature' + +function settlePost() { + resolvePost?.({data: {token: FAKE_TOKEN}}) +} + +describe('refreshToken in-flight dedup', () => { + const originalLocks = navigator.locks + + beforeEach(() => { + postCallCount = 0 + resolvePost = null + removeToken() + localStorage.clear() + }) + + afterEach(() => { + Object.defineProperty(navigator, 'locks', { + value: originalLocks, + configurable: true, + writable: true, + }) + }) + + it('coalesces concurrent calls into a single POST when Web Locks is available', async () => { + // Stub a minimal Web Locks API: happy-dom leaves navigator.locks + // undefined, so without this the test would silently fall through to + // the insecure-HTTP branch and never exercise navigator.locks.request. + const requestSpy = vi.fn((_name: string, cb: () => unknown) => cb()) + Object.defineProperty(navigator, 'locks', { + value: {request: requestSpy}, + configurable: true, + writable: true, + }) + + const p1 = refreshToken(true) + const p2 = refreshToken(true) + + // Both calls share one underlying request. + expect(postCallCount).toBe(1) + + settlePost() + await Promise.all([p1, p2]) + + // The Web Locks branch actually ran... + expect(requestSpy).toHaveBeenCalledWith('vikunja-token-refresh', expect.any(Function)) + // ...and the in-flight dedup still collapsed both calls into one POST. + expect(postCallCount).toBe(1) + }) + + it('coalesces concurrent calls into a single POST on insecure HTTP (no Web Locks)', async () => { + // Simulate an insecure HTTP context where navigator.locks is undefined. + Object.defineProperty(navigator, 'locks', { + value: undefined, + configurable: true, + writable: true, + }) + + const p1 = refreshToken(true) + const p2 = refreshToken(true) + const p3 = refreshToken(true) + + expect(postCallCount).toBe(1) + + settlePost() + await Promise.all([p1, p2, p3]) + + expect(postCallCount).toBe(1) + }) + + it('allows a fresh refresh after the previous one settled', async () => { + const p1 = refreshToken(true) + settlePost() + await p1 + expect(postCallCount).toBe(1) + + // The in-flight promise was reset, so a later refresh runs anew. + const p2 = refreshToken(true) + expect(postCallCount).toBe(2) + settlePost() + await p2 + }) + + it('does not re-persist the token when logout happens during an in-flight refresh', async () => { + const p1 = refreshToken(true) + expect(postCallCount).toBe(1) + + // User logs out while the refresh POST is still in flight. + removeToken() + + // The in-flight POST resolves afterwards — it must not undo the logout. + settlePost() + await p1 + + expect(localStorage.getItem('token')).toBeNull() + }) + + it('an older refresh settling does not clobber a newer in-flight one', async () => { + // Refresh A starts and stays in flight. + const pA = refreshToken(true) + expect(postCallCount).toBe(1) + const resolveA = resolvePost + + // User logs out, which drops the in-flight reference to A. + removeToken() + + // Refresh B starts; it must claim the in-flight slot. + const pB = refreshToken(true) + expect(postCallCount).toBe(2) + const resolveB = resolvePost + + // A settles after B started. Its cleanup must NOT null the in-flight + // slot, since that slot now belongs to B. Without the `=== p` guard, + // A's .finally would clobber B and let a concurrent caller fire a + // second parallel POST. + resolveA?.({data: {token: FAKE_TOKEN}}) + await pA + + // A concurrent caller while B is still in flight must dedup to B — + // no third POST. + const pB2 = refreshToken(true) + expect(postCallCount).toBe(2) + + resolveB?.({data: {token: FAKE_TOKEN}}) + await Promise.all([pB, pB2]) + }) +}) diff --git a/frontend/src/helpers/auth.ts b/frontend/src/helpers/auth.ts index 1d46258dc..04ed29f2e 100644 --- a/frontend/src/helpers/auth.ts +++ b/frontend/src/helpers/auth.ts @@ -33,18 +33,53 @@ export const removeToken = () => { savedToken = null localStorage.removeItem('token') localStorage.removeItem('desktopOAuthRefreshToken') + + // Bump the epoch and drop the in-flight refresh so a refresh that started + // before this logout can't re-persist a token after we cleared it. + authEpoch++ + inFlightRefresh = null } +// Coalesces concurrent same-tab refreshes into one POST. Web Locks (below) is +// secure-context-only, so on insecure HTTP there's no cross-tab coordination — +// without this guard, refreshes firing close together each spend the single-use +// cookie and all but one get a 401. +let inFlightRefresh: Promise | null = null + +// Incremented on every removeToken()/logout. A refresh captures the epoch when +// it starts and only persists its result if the epoch is unchanged, so a +// refresh that resolves after a logout can't undo it. +let authEpoch = 0 + /** * Refreshes an auth token while ensuring it is updated everywhere. * The refresh token is sent automatically as an HttpOnly cookie. * The server rotates the cookie on every call. * - * Uses the Web Locks API to coordinate across browser tabs. Only one tab - * performs the actual refresh; other tabs waiting for the lock detect that - * the token in localStorage was already updated and adopt it directly. + * Same-tab concurrent calls share one in-flight refresh (always-on dedup); the + * Web Locks API inside adds cross-tab coordination only in secure contexts. */ export async function refreshToken(persist: boolean): Promise { + if (inFlightRefresh) { + return inFlightRefresh + } + const p = doRefresh(persist) + inFlightRefresh = p + // Only clear if it still points to this promise — a logout (or a newer + // refresh started after it) may have replaced inFlightRefresh meanwhile. + p.finally(() => { + if (inFlightRefresh === p) { + inFlightRefresh = null + } + }) + return p +} + +async function doRefresh(persist: boolean): Promise { + // Snapshot the epoch so we can tell if a logout happened while we awaited. + const epochAtStart = authEpoch + const loggedOutSinceStart = () => authEpoch !== epochAtStart + // In desktop mode, refresh via IPC to the Electron main process if (isDesktopApp()) { const storedRefreshToken = localStorage.getItem('desktopOAuthRefreshToken') @@ -53,6 +88,9 @@ export async function refreshToken(persist: boolean): Promise { } try { const tokens = await refreshDesktopToken(window.API_URL, storedRefreshToken) + if (loggedOutSinceStart()) { + return + } saveToken(tokens.access_token, persist) localStorage.setItem('desktopOAuthRefreshToken', tokens.refresh_token) } catch (e) { @@ -65,7 +103,13 @@ export async function refreshToken(persist: boolean): Promise { // if another tab refreshed while we were queued. const tokenBeforeLock = localStorage.getItem('token') - const doRefresh = async () => { + const refreshUnderLock = async () => { + // A logout may have happened while we waited for the lock — don't + // re-adopt or re-fetch a token after the user signed out. + if (loggedOutSinceStart()) { + return + } + // If the token in localStorage changed while waiting for the lock, // another tab already refreshed. Just adopt the new token. const currentToken = localStorage.getItem('token') @@ -78,6 +122,9 @@ export async function refreshToken(persist: boolean): Promise { const HTTP = HTTPFactory() try { const response = await HTTP.post('user/token/refresh') + if (loggedOutSinceStart()) { + return + } saveToken(response.data.token, persist) } catch (e) { throw new Error('Error renewing token: ', {cause: e}) @@ -85,10 +132,10 @@ export async function refreshToken(persist: boolean): Promise { } if (navigator.locks) { - await navigator.locks.request('vikunja-token-refresh', doRefresh) + await navigator.locks.request('vikunja-token-refresh', refreshUnderLock) } else { // Fallback for environments without Web Locks (e.g. insecure HTTP) - await doRefresh() + await refreshUnderLock() } } diff --git a/frontend/src/stores/auth.renewToken.test.ts b/frontend/src/stores/auth.renewToken.test.ts new file mode 100644 index 000000000..331f86856 --- /dev/null +++ b/frontend/src/stores/auth.renewToken.test.ts @@ -0,0 +1,139 @@ +import {describe, it, expect, beforeEach, vi} from 'vitest' +import {setActivePinia, createPinia} from 'pinia' + +import {useAuthStore} from './auth' +import {AUTH_TYPES} from '@/modelTypes/IUser' + +const {refreshTokenMock, routerPushMock, getTokenMock} = vi.hoisted(() => ({ + refreshTokenMock: vi.fn(), + routerPushMock: vi.fn(), + getTokenMock: vi.fn(() => null as string | null), +})) + +vi.mock('@/helpers/auth', () => ({ + refreshToken: refreshTokenMock, + getToken: getTokenMock, + saveToken: vi.fn(), + removeToken: vi.fn(), +})) + +vi.mock('@/router', () => ({ + default: {push: routerPushMock}, +})) + +vi.mock('@/composables/useWebSocket', () => ({ + useWebSocket: () => ({disconnect: vi.fn(), connect: vi.fn()}), +})) + +function fakeHttp() { + return { + post: vi.fn().mockResolvedValue({data: {}}), + get: vi.fn().mockResolvedValue({data: {}}), + request: vi.fn().mockResolvedValue({data: {}}), + interceptors: { + request: {use: vi.fn()}, + response: {use: vi.fn()}, + }, + } +} + +vi.mock('@/helpers/fetcher', () => ({ + HTTPFactory: () => fakeHttp(), + AuthenticatedHTTPFactory: () => fakeHttp(), + getApiBaseUrl: () => 'http://localhost/api/v1/', +})) + +vi.mock('@/helpers/redirectToProvider', () => ({ + getRedirectUrlFromCurrentFrontendPath: vi.fn(), + redirectToProvider: vi.fn(), + redirectToProviderOnLogout: vi.fn(), +})) + +// A refresh failure that looks like a real network/HTTP error so renewToken's +// "is this a genuine logout?" check (it inspects the error cause's status) fires. +function refreshError() { + return new Error('Error renewing token: ', { + cause: {response: {status: 401}}, + }) +} + +// A JWT carrying a not-yet-expired user session, so the checkAuth() call that +// renewToken() runs after a successful refresh treats the session as live. +function freshUserJwt() { + const payload = { + id: 1, + type: AUTH_TYPES.USER, + exp: Math.floor(Date.now() / 1000) + 3600, + } + const encoded = btoa(JSON.stringify(payload)) + return `header.${encoded}.signature` +} + +describe('auth store renewToken retry (issue #2863)', () => { + beforeEach(() => { + setActivePinia(createPinia()) + refreshTokenMock.mockReset() + routerPushMock.mockReset() + getTokenMock.mockReset().mockReturnValue(null) + }) + + function setupExpiredUserSession(store: ReturnType) { + store.setAuthenticated(true) + // Expired exp so renewToken treats a refresh failure as a real logout. + store.setUser({ + id: 1, + type: AUTH_TYPES.USER, + exp: Math.floor(Date.now() / 1000) - 60, + } as never, false) + } + + it('does NOT log out when the first refresh fails but the retry succeeds', async () => { + const store = useAuthStore() + setupExpiredUserSession(store) + + // The retry "succeeds" only if it actually leaves a usable token behind: + // renewToken() runs checkAuth() afterwards, which reads getToken(). Start + // with no token, then hand back a fresh JWT once the refresh resolves. + getTokenMock.mockReturnValue(null) + refreshTokenMock + .mockRejectedValueOnce(refreshError()) + .mockImplementationOnce(async () => { + getTokenMock.mockReturnValue(freshUserJwt()) + }) + + await store.renewToken() + + // Two refresh attempts: the initial one and the single retry. + expect(refreshTokenMock).toHaveBeenCalledTimes(2) + // The retry recovered the session: the user is still authenticated... + expect(store.authenticated).toBe(true) + // ...and was not bounced to login. + expect(routerPushMock).not.toHaveBeenCalledWith({name: 'user.login'}) + }) + + it('logs out when BOTH the refresh and its retry fail', async () => { + const store = useAuthStore() + setupExpiredUserSession(store) + + refreshTokenMock + .mockRejectedValueOnce(refreshError()) + .mockRejectedValueOnce(refreshError()) + + await store.renewToken() + + expect(refreshTokenMock).toHaveBeenCalledTimes(2) + expect(routerPushMock).toHaveBeenCalledWith({name: 'user.login'}) + }) + + it('retries exactly once (no infinite loop) when the session is genuinely dead', async () => { + const store = useAuthStore() + setupExpiredUserSession(store) + + refreshTokenMock.mockRejectedValue(refreshError()) + + await store.renewToken() + + // Initial attempt + exactly one retry — never more. + expect(refreshTokenMock).toHaveBeenCalledTimes(2) + }) +}) diff --git a/frontend/src/stores/auth.ts b/frontend/src/stores/auth.ts index e9a7c8ee0..7a444acc7 100644 --- a/frontend/src/stores/auth.ts +++ b/frontend/src/stores/auth.ts @@ -55,6 +55,17 @@ function redirectToSpecifiedProvider() { } } +// A race-loser's refresh fails but the rotated cookie is already valid, so a +// second attempt succeeds — recovering what would otherwise be a spurious +// logout. Exactly one retry: a genuinely dead session still logs out, no loop. +async function refreshTokenWithRetry(persist: boolean): Promise { + try { + await refreshToken(persist) + } catch { + await refreshToken(persist) + } +} + function getLoggedInVia(): string | null { return localStorage.getItem('loggedInViaProvider') } @@ -352,7 +363,7 @@ export const useAuthStore = defineStore('auth', () => { // refresh before giving up. This lets users who reopen the app // after the short JWT TTL seamlessly resume their session. try { - await refreshToken(true) + await refreshTokenWithRetry(true) const freshJwt = getToken() if (freshJwt) { const b64 = freshJwt.split('.')[1].replace(/-/g, '+').replace(/_/g, '/') @@ -512,7 +523,7 @@ export const useAuthStore = defineStore('auth', () => { saveToken(response.data.token, false) } else { // User sessions renew via the refresh-token cookie. - await refreshToken(true) + await refreshTokenWithRetry(true) } await checkAuth() } catch (e) {