diff --git a/frontend/src/helpers/auth.test.ts b/frontend/src/helpers/auth.test.ts new file mode 100644 index 000000000..69af2335f --- /dev/null +++ b/frontend/src/helpers/auth.test.ts @@ -0,0 +1,96 @@ +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 () => { + const p1 = refreshToken(true) + const p2 = refreshToken(true) + + // Both calls share one underlying request. + expect(postCallCount).toBe(1) + + settlePost() + await Promise.all([p1, p2]) + + 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 + }) +}) diff --git a/frontend/src/helpers/auth.ts b/frontend/src/helpers/auth.ts index 1d46258dc..1652d169d 100644 --- a/frontend/src/helpers/auth.ts +++ b/frontend/src/helpers/auth.ts @@ -35,16 +35,34 @@ export const removeToken = () => { localStorage.removeItem('desktopOAuthRefreshToken') } +// Coalesces concurrent refresh calls in the same tab into a single underlying +// refresh. The Web Locks API below only exists in secure contexts, so on +// insecure HTTP it falls back to an uncoordinated refresh — without this guard, +// triggers that fire close together (focus, proactive timer, 401 interceptor) +// each POST with the same single-use refresh cookie and all but one get a 401. +let inFlightRefresh: Promise | null = null + /** * 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. + * Concurrent calls in the same tab share one in-flight refresh. This is the + * always-on primary dedup and works in every context (HTTP included). The Web + * Locks API used inside is the secondary, cross-tab coordination layer that + * only exists in secure contexts. */ export async function refreshToken(persist: boolean): Promise { + if (inFlightRefresh) { + return inFlightRefresh + } + inFlightRefresh = doRefresh(persist).finally(() => { + inFlightRefresh = null + }) + return inFlightRefresh +} + +async function doRefresh(persist: boolean): Promise { // In desktop mode, refresh via IPC to the Electron main process if (isDesktopApp()) { const storedRefreshToken = localStorage.getItem('desktopOAuthRefreshToken') @@ -65,7 +83,7 @@ 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 () => { // 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') @@ -85,10 +103,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..d5bedc098 --- /dev/null +++ b/frontend/src/stores/auth.renewToken.test.ts @@ -0,0 +1,119 @@ +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}}, + }) +} + +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) + + refreshTokenMock + .mockRejectedValueOnce(refreshError()) + .mockResolvedValueOnce(undefined) + + await store.renewToken() + + // Two refresh attempts: the initial one and the single retry. + expect(refreshTokenMock).toHaveBeenCalledTimes(2) + // The retry succeeded, so the user is 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 e2576760f..91f72a10d 100644 --- a/frontend/src/stores/auth.ts +++ b/frontend/src/stores/auth.ts @@ -55,6 +55,20 @@ function redirectToSpecifiedProvider() { } } +// Refreshes the token, retrying exactly once if the first attempt fails. +// After a lost refresh race (common on insecure HTTP without Web Locks, or +// behind a proxy that delays the rotated Set-Cookie), the browser's cookie is +// already the rotated, valid one — a second attempt then succeeds. Only when +// both attempts fail is the session genuinely dead. Exactly one retry, so a +// truly dead session still logs out without looping. +async function refreshTokenWithRetry(persist: boolean): Promise { + try { + await refreshToken(persist) + } catch { + await refreshToken(persist) + } +} + function getLoggedInVia(): string | null { return localStorage.getItem('loggedInViaProvider') } @@ -352,7 +366,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, '/') @@ -511,8 +525,9 @@ export const useAuthStore = defineStore('auth', () => { const response = await HTTP.post('user/token') saveToken(response.data.token, false) } else { - // User sessions renew via the refresh-token cookie. - await refreshToken(true) + // User sessions renew via the refresh-token cookie. Retry once so + // a lost refresh race doesn't spuriously log the user out. + await refreshTokenWithRetry(true) } await checkAuth() } catch (e) {