-
Notifications
You must be signed in to change notification settings - Fork 402
Cloud/fix cookie timing race #6398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| import { createSingletonPromise } from '@vueuse/core' | ||
|
|
||
| import { api } from '@/scripts/api' | ||
| import { isCloud } from '@/platform/distribution/types' | ||
| import { useFirebaseAuthStore } from '@/stores/firebaseAuthStore' | ||
|
|
@@ -12,29 +14,17 @@ export const useSessionCookie = () => { | |
| * Called after login and on token refresh. | ||
| */ | ||
| const createSession = async (): Promise<void> => { | ||
| if (!isCloud) return | ||
| if (!isCloud || logoutInProgress) return | ||
|
|
||
| const authStore = useFirebaseAuthStore() | ||
| const authHeader = await authStore.getAuthHeader() | ||
|
|
||
| if (!authHeader) { | ||
| throw new Error('No auth header available for session creation') | ||
| } | ||
| const promise = createSessionSingleton() | ||
| inFlightCreateSession = promise | ||
|
|
||
| const response = await fetch(api.apiURL('/auth/session'), { | ||
| method: 'POST', | ||
| credentials: 'include', | ||
| headers: { | ||
| ...authHeader, | ||
| 'Content-Type': 'application/json' | ||
| try { | ||
| await promise | ||
| } finally { | ||
| if (inFlightCreateSession === promise) { | ||
| inFlightCreateSession = null | ||
| } | ||
| }) | ||
|
|
||
| if (!response.ok) { | ||
| const errorData = await response.json().catch(() => ({})) | ||
| throw new Error( | ||
| `Failed to create session: ${errorData.message || response.statusText}` | ||
| ) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -45,21 +35,90 @@ export const useSessionCookie = () => { | |
| const deleteSession = async (): Promise<void> => { | ||
| if (!isCloud) return | ||
|
|
||
| logoutInProgress = true | ||
|
|
||
| try { | ||
| if (inFlightCreateSession) { | ||
| currentCreateController?.abort() | ||
| try { | ||
| await inFlightCreateSession | ||
| } catch (error: unknown) { | ||
| if (!isAbortError(error)) { | ||
| throw error | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const response = await fetch(api.apiURL('/auth/session'), { | ||
| method: 'DELETE', | ||
| credentials: 'include' | ||
| }) | ||
|
|
||
| if (!response.ok) { | ||
| const errorData = await response.json().catch(() => ({})) | ||
| throw new Error( | ||
| `Failed to delete session: ${ | ||
| errorData.message || response.statusText | ||
| }` | ||
| ) | ||
| } | ||
| } finally { | ||
| logoutInProgress = false | ||
| await createSessionSingleton.reset() | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| createSession, | ||
| deleteSession | ||
| } | ||
| } | ||
|
|
||
| let inFlightCreateSession: Promise<void> | null = null | ||
| let currentCreateController: AbortController | null = null | ||
| let logoutInProgress = false | ||
|
|
||
| const createSessionSingleton = createSingletonPromise(async () => { | ||
| const authStore = useFirebaseAuthStore() | ||
| const authHeader = await authStore.getAuthHeader() | ||
|
|
||
| if (!authHeader) { | ||
| throw new Error('No auth header available for session creation') | ||
| } | ||
|
|
||
| const controller = new AbortController() | ||
| currentCreateController = controller | ||
|
|
||
| try { | ||
| const response = await fetch(api.apiURL('/auth/session'), { | ||
| method: 'DELETE', | ||
| credentials: 'include' | ||
| method: 'POST', | ||
| credentials: 'include', | ||
| signal: controller.signal, | ||
| headers: { | ||
| ...authHeader, | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| }) | ||
|
|
||
| if (!response.ok) { | ||
| const errorData = await response.json().catch(() => ({})) | ||
| throw new Error( | ||
| `Failed to delete session: ${errorData.message || response.statusText}` | ||
| `Failed to create session: ${errorData.message || response.statusText}` | ||
| ) | ||
| } | ||
| } catch (error: unknown) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [quality] medium Priority Issue: Missing error logging for debugging session creation failures |
||
| if (!isAbortError(error)) { | ||
| throw error | ||
| } | ||
| } finally { | ||
| if (currentCreateController === controller) { | ||
| currentCreateController = null | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| return { | ||
| createSession, | ||
| deleteSession | ||
| } | ||
| const isAbortError = (error: unknown): boolean => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [performance] low Priority Issue: The isAbortError function could be more efficient with direct property access |
||
| if (!error || typeof error !== 'object') return false | ||
| const name = 'name' in error ? (error as { name?: string }).name : undefined | ||
| return name === 'AbortError' | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| import { describe, expect, it, vi } from 'vitest' | ||
|
|
||
| const makeSuccessResponse = () => | ||
| new Response('{}', { | ||
| status: 200, | ||
| statusText: 'OK', | ||
| headers: { 'Content-Type': 'application/json' } | ||
| }) | ||
|
|
||
| type Deferred<T> = { | ||
| promise: Promise<T> | ||
| resolve: (value: T) => void | ||
| reject: (reason: unknown) => void | ||
| } | ||
|
|
||
| const createDeferred = <T>(): Deferred<T> => { | ||
| let resolve: (value: T) => void | ||
| let reject: (reason: unknown) => void | ||
|
|
||
| const promise = new Promise<T>((res, rej) => { | ||
| resolve = res | ||
| reject = rej | ||
| }) | ||
|
|
||
| // @ts-expect-error initialized via closure assignments above | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [quality] low Priority Issue: Test uses @ts-expect-error without providing a meaningful error message |
||
| return { promise, resolve, reject } | ||
| } | ||
|
|
||
| const mockModules = async () => { | ||
| vi.resetModules() | ||
|
|
||
| const getAuthHeader = vi.fn(async () => ({ Authorization: 'Bearer token' })) | ||
|
|
||
| vi.doMock('@/scripts/api', () => ({ | ||
| api: { | ||
| apiURL: vi.fn((path: string) => `/api${path}`) | ||
| } | ||
| })) | ||
|
|
||
| vi.doMock('@/platform/distribution/types', () => ({ | ||
| isCloud: true | ||
| })) | ||
|
|
||
| vi.doMock('@/stores/firebaseAuthStore', () => ({ | ||
| useFirebaseAuthStore: vi.fn(() => ({ | ||
| getAuthHeader | ||
| })) | ||
| })) | ||
|
|
||
| const module = await import('@/platform/auth/session/useSessionCookie') | ||
| return { getAuthHeader, useSessionCookie: module.useSessionCookie } | ||
| } | ||
|
|
||
| describe('useSessionCookie', () => { | ||
| it('deduplicates in-flight session creation', async () => { | ||
| const { useSessionCookie, getAuthHeader } = await mockModules() | ||
|
|
||
| const postDeferred = createDeferred<Response>() | ||
|
|
||
| const fetchSpy = vi | ||
| .spyOn(globalThis, 'fetch') | ||
| .mockImplementation(() => postDeferred.promise) | ||
|
|
||
| const { createSession } = useSessionCookie() | ||
|
|
||
| const firstCall = createSession() | ||
| const secondCall = createSession() | ||
|
|
||
| await Promise.resolve() | ||
|
|
||
| expect(fetchSpy).toHaveBeenCalledTimes(1) | ||
| expect(getAuthHeader).toHaveBeenCalledTimes(1) | ||
|
|
||
| postDeferred.resolve(makeSuccessResponse()) | ||
| await expect(firstCall).resolves.toBeUndefined() | ||
| await expect(secondCall).resolves.toBeUndefined() | ||
|
|
||
| fetchSpy.mockRestore() | ||
| }) | ||
|
|
||
| it('aborts pending create on logout and skips new ones while logout is in progress', async () => { | ||
| const { useSessionCookie, getAuthHeader } = await mockModules() | ||
|
|
||
| const firstPostDeferred = createDeferred<Response>() | ||
| const deleteDeferred = createDeferred<Response>() | ||
|
|
||
| let capturedSignal: AbortSignal | undefined | ||
|
|
||
| const fetchSpy = vi.spyOn(globalThis, 'fetch') | ||
| fetchSpy | ||
| .mockImplementationOnce((_, init?: RequestInit) => { | ||
| capturedSignal = init?.signal as AbortSignal | undefined | ||
| return firstPostDeferred.promise | ||
| }) | ||
| .mockImplementationOnce((_, init?: RequestInit) => { | ||
| expect(init?.method).toBe('DELETE') | ||
| return deleteDeferred.promise | ||
| }) | ||
| .mockImplementation((_, init?: RequestInit) => { | ||
| if (init?.method === 'POST') { | ||
| return Promise.resolve(makeSuccessResponse()) | ||
| } | ||
| return Promise.resolve(makeSuccessResponse()) | ||
| }) | ||
|
|
||
| const { createSession, deleteSession } = useSessionCookie() | ||
|
|
||
| const createPromise = createSession() | ||
|
|
||
| await Promise.resolve() | ||
|
|
||
| const logoutPromise = deleteSession() | ||
|
|
||
| await Promise.resolve() | ||
|
|
||
| expect(fetchSpy).toHaveBeenCalledTimes(1) | ||
| expect(capturedSignal?.aborted).toBe(true) | ||
|
|
||
| const abortError = new Error('aborted') | ||
| abortError.name = 'AbortError' | ||
| firstPostDeferred.reject(abortError) | ||
| await expect(createPromise).resolves.toBeUndefined() | ||
|
|
||
| await Promise.resolve() | ||
| expect(fetchSpy).toHaveBeenCalledTimes(2) | ||
| expect(getAuthHeader).toHaveBeenCalledTimes(1) | ||
|
|
||
| await expect(createSession()).resolves.toBeUndefined() | ||
| expect(fetchSpy).toHaveBeenCalledTimes(2) | ||
|
|
||
| deleteDeferred.resolve(makeSuccessResponse()) | ||
| await expect(logoutPromise).resolves.toBeUndefined() | ||
|
|
||
| await expect(createSession()).resolves.toBeUndefined() | ||
| expect(fetchSpy).toHaveBeenCalledTimes(3) | ||
|
|
||
| fetchSpy.mockRestore() | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[architecture] medium Priority
Issue: Module-level variables (inFlightCreateSession, currentCreateController, logoutInProgress) create shared mutable state that could cause issues in environments where the module is imported multiple times
Context: This breaks the isolation principle and could lead to unexpected behavior if useSessionCookie is used across different components or contexts
Suggestion: Move these variables inside the composable factory or use a proper singleton pattern with explicit state management