fix(auth): dedupe and retry token refresh so concurrent refreshes don't spuriously log out (#2863)

The access JWT lives only 10 minutes, so the SPA constantly refreshes it via
the single-use, rotation-on-use refresh cookie. The only concurrency guard was
the Web Locks API, which exists exclusively in secure contexts. On insecure
HTTP deployments there is no coordination, so triggers that fire close together
(initial load, proactive timer, focus handler, 401 interceptor) each POST with
the SAME cookie before the rotated Set-Cookie lands. One rotates successfully,
the other matches 0 rows and gets a 401, and the loser bounces the user to
/login even though the rotation actually succeeded.

A: add a module-level in-flight promise in refreshToken() so concurrent calls
in the same tab share one underlying refresh, in every context (HTTP included),
independent of navigator.locks. The existing Web Locks usage stays as the
secondary cross-tab layer for secure contexts.

D: retry the refresh exactly once before treating a failure as a real logout.
After a lost race the browser cookie is already the rotated valid one, so a
fresh second attempt succeeds; only if both fail do we log out. Implemented DRY
via refreshTokenWithRetry() used by renewToken() and the expired-JWT path in
checkAuth().

The happy path (single tab, secure context, no race) is unchanged.

Fixes #2863

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
+	})
+})

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<void> | 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<void> {
+	if (inFlightRefresh) {
+		return inFlightRefresh
+	}
+	inFlightRefresh = doRefresh(persist).finally(() => {
+		inFlightRefresh = null
+	})
+	return inFlightRefresh
+}
+
+async function doRefresh(persist: boolean): Promise<void> {
 	// 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<void> {
 	// 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<void> {
 	}
 
 	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()
 	}
 }
 

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<typeof useAuthStore>) {
+		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)
+	})
+})

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<void> {
+	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) {