From d77991ba44523c8970e2b6d39ed89ca0eeebdeaa Mon Sep 17 00:00:00 2001 From: Arjan Singh Date: Wed, 20 Aug 2025 20:14:36 -0700 Subject: [PATCH 1/7] [fix] gracefully handle Firebase auth failure --- src/scripts/app.ts | 14 +++++++++++--- src/stores/firebaseAuthStore.ts | 12 +++++++++++- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/scripts/app.ts b/src/scripts/app.ts index ca784b66fc..cd1b869d33 100644 --- a/src/scripts/app.ts +++ b/src/scripts/app.ts @@ -1303,8 +1303,16 @@ export class ComfyApp { const executionStore = useExecutionStore() executionStore.lastNodeErrors = null - let comfyOrgAuthToken = - (await useFirebaseAuthStore().getIdToken()) ?? undefined + let comfyOrgAuthToken = await useFirebaseAuthStore() + .getIdToken() + .catch((error: unknown) => { + useDialogService().showErrorDialog(error, { + title: t('errorDialog.defaultTitle'), + reportType: 'authenticationError' + }) + console.error(error) + }) + let comfyOrgApiKey = useApiKeyAuthStore().getApiKey() try { @@ -1321,7 +1329,7 @@ export class ComfyApp { const p = await this.graphToPrompt(this.graph) try { - api.authToken = comfyOrgAuthToken + api.authToken = comfyOrgAuthToken ?? undefined api.apiKey = comfyOrgApiKey ?? undefined const res = await api.queuePrompt(number, p, { partialExecutionTargets: queueNodeIds diff --git a/src/stores/firebaseAuthStore.ts b/src/stores/firebaseAuthStore.ts index b8befb0932..cdbd8c082f 100644 --- a/src/stores/firebaseAuthStore.ts +++ b/src/stores/firebaseAuthStore.ts @@ -90,7 +90,17 @@ export const useFirebaseAuthStore = defineStore('firebaseAuth', () => { const getIdToken = async (): Promise => { if (currentUser.value) { - return currentUser.value.getIdToken() + try { + return await currentUser.value.getIdToken() + } catch (error: any) { + if (error?.code === 'auth/network-request-failed') { + console.warn( + 'Could not authenticate with Firebase. Features requiring authentication might not work.' + ) + return null // handle gracefully + } + throw error + } } return null } From fbc80a8bb03345ee82e7a8c1f89aff40f558ca99 Mon Sep 17 00:00:00 2001 From: snomiao Date: Tue, 5 Aug 2025 16:53:38 +0000 Subject: [PATCH 2/7] [test] Add failing tests to reproduce Firebase Auth network issue #4468 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add test cases that demonstrate the current problematic behavior where Firebase Auth makes network requests when offline without graceful error handling, causing toast error messages and degraded offline experience. Tests reproduce: - getIdToken() throwing auth/network-request-failed instead of returning null - getAuthHeader() failing to fallback gracefully when Firebase token refresh fails These tests currently pass by expecting the error to be thrown. After implementing the fix, the tests should be updated to verify graceful handling (returning null instead of throwing). Related to issue #4468: Firebase Auth makes network requests when offline without evicting token 🤖 Generated with Claude Code Co-Authored-By: Claude --- .../tests/store/firebaseAuthStore.test.ts | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests-ui/tests/store/firebaseAuthStore.test.ts b/tests-ui/tests/store/firebaseAuthStore.test.ts index 7d51685b4c..81320766a8 100644 --- a/tests-ui/tests/store/firebaseAuthStore.test.ts +++ b/tests-ui/tests/store/firebaseAuthStore.test.ts @@ -331,6 +331,68 @@ describe('useFirebaseAuthStore', () => { const tokenAfterLogout = await store.getIdToken() expect(tokenAfterLogout).toBeNull() }) + + it('should handle network errors gracefully when offline (reproduces issue #4468)', async () => { + // This test reproduces the issue where Firebase Auth makes network requests when offline + // and fails without graceful error handling, causing toast error messages + + // Simulate a user with an expired token that requires network refresh + mockUser.getIdToken.mockReset() + + // Mock network failure (auth/network-request-failed error from Firebase) + const networkError = new Error( + 'Firebase: Error (auth/network-request-failed).' + ) + networkError.name = 'FirebaseError' + ;(networkError as any).code = 'auth/network-request-failed' + + mockUser.getIdToken.mockRejectedValue(networkError) + + // This should fail until the fix is implemented + // The expected behavior is to return null gracefully instead of throwing + await expect(store.getIdToken()).rejects.toThrow( + 'auth/network-request-failed' + ) + + // TODO: After implementing the fix, this test should pass: + const token = await store.getIdToken() + expect(token).toBeNull() // Should return null instead of throwing + }) + }) + + describe('getAuthHeader', () => { + it('should handle network errors gracefully when getting Firebase token (reproduces issue #4468)', async () => { + // This test reproduces the issue where getAuthHeader fails due to network errors + // when Firebase Auth tries to refresh tokens offline + + // Mock useApiKeyAuthStore to return null (no API key fallback) + const mockApiKeyStore = { + getAuthHeader: vi.fn().mockReturnValue(null) + } + vi.doMock('@/stores/apiKeyAuthStore', () => ({ + useApiKeyAuthStore: () => mockApiKeyStore + })) + + // Setup user with network error on token refresh + mockUser.getIdToken.mockReset() + const networkError = new Error( + 'Firebase: Error (auth/network-request-failed).' + ) + networkError.name = 'FirebaseError' + ;(networkError as any).code = 'auth/network-request-failed' + mockUser.getIdToken.mockRejectedValue(networkError) + + // This should fail until the fix is implemented + // The expected behavior is to fallback to API key or return null gracefully + await expect(store.getAuthHeader()).rejects.toThrow( + 'auth/network-request-failed' + ) + + // TODO: After implementing the fix, this should pass: + // const authHeader = await store.getAuthHeader() + // expect(authHeader).toBeNull() // Should fallback gracefully + // expect(mockApiKeyStore.getAuthHeader).toHaveBeenCalled() // Should try API key fallback + }) }) describe('social authentication', () => { From 0d05359a0e1b80e0e631fc6154bc1d00ca341b1a Mon Sep 17 00:00:00 2001 From: Arjan Singh Date: Wed, 20 Aug 2025 20:27:26 -0700 Subject: [PATCH 3/7] [test] update firebaseAuthStore tests They match the behavior of the implemented solution now --- .../tests/store/firebaseAuthStore.test.ts | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/tests-ui/tests/store/firebaseAuthStore.test.ts b/tests-ui/tests/store/firebaseAuthStore.test.ts index 81320766a8..17b3c251ba 100644 --- a/tests-ui/tests/store/firebaseAuthStore.test.ts +++ b/tests-ui/tests/store/firebaseAuthStore.test.ts @@ -348,13 +348,6 @@ describe('useFirebaseAuthStore', () => { mockUser.getIdToken.mockRejectedValue(networkError) - // This should fail until the fix is implemented - // The expected behavior is to return null gracefully instead of throwing - await expect(store.getIdToken()).rejects.toThrow( - 'auth/network-request-failed' - ) - - // TODO: After implementing the fix, this test should pass: const token = await store.getIdToken() expect(token).toBeNull() // Should return null instead of throwing }) @@ -382,16 +375,8 @@ describe('useFirebaseAuthStore', () => { ;(networkError as any).code = 'auth/network-request-failed' mockUser.getIdToken.mockRejectedValue(networkError) - // This should fail until the fix is implemented - // The expected behavior is to fallback to API key or return null gracefully - await expect(store.getAuthHeader()).rejects.toThrow( - 'auth/network-request-failed' - ) - - // TODO: After implementing the fix, this should pass: - // const authHeader = await store.getAuthHeader() - // expect(authHeader).toBeNull() // Should fallback gracefully - // expect(mockApiKeyStore.getAuthHeader).toHaveBeenCalled() // Should try API key fallback + const authHeader = await store.getAuthHeader() + expect(authHeader).toBeNull() // Should fallback gracefully }) }) From 1fd6da7876069b36528fa0b04aa57feb85666990 Mon Sep 17 00:00:00 2001 From: Arjan Singh Date: Wed, 20 Aug 2025 20:40:12 -0700 Subject: [PATCH 4/7] [test] add firebaseAuthStore.getTokenId test for non-network errors --- tests-ui/tests/store/firebaseAuthStore.test.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests-ui/tests/store/firebaseAuthStore.test.ts b/tests-ui/tests/store/firebaseAuthStore.test.ts index 17b3c251ba..6887303cd7 100644 --- a/tests-ui/tests/store/firebaseAuthStore.test.ts +++ b/tests-ui/tests/store/firebaseAuthStore.test.ts @@ -351,6 +351,23 @@ describe('useFirebaseAuthStore', () => { const token = await store.getIdToken() expect(token).toBeNull() // Should return null instead of throwing }) + + it('should throw error when getIdToken fails with non-network error', async () => { + // This test verifies that non-network errors are thrown instead of handled gracefully + mockUser.getIdToken.mockReset() + + // Mock a non-network error using actual Firebase Auth error code + const authError = new Error('User account is disabled.') + authError.name = 'FirebaseError' + ;(authError as any).code = 'auth/user-disabled' + + mockUser.getIdToken.mockRejectedValue(authError) + + // Should throw the error instead of returning null + await expect(store.getIdToken()).rejects.toThrow( + 'User account is disabled.' + ) + }) }) describe('getAuthHeader', () => { From 5338eaa7301dcc6e4a8f84705fb6710e28a06d08 Mon Sep 17 00:00:00 2001 From: Arjan Singh Date: Thu, 21 Aug 2025 12:06:36 -0700 Subject: [PATCH 5/7] [chore] code review feedback --- src/scripts/app.ts | 13 +-- src/stores/firebaseAuthStore.ts | 22 +++-- .../tests/store/firebaseAuthStore.test.ts | 85 +++++++++++-------- 3 files changed, 66 insertions(+), 54 deletions(-) diff --git a/src/scripts/app.ts b/src/scripts/app.ts index cd1b869d33..debcc26278 100644 --- a/src/scripts/app.ts +++ b/src/scripts/app.ts @@ -1303,16 +1303,7 @@ export class ComfyApp { const executionStore = useExecutionStore() executionStore.lastNodeErrors = null - let comfyOrgAuthToken = await useFirebaseAuthStore() - .getIdToken() - .catch((error: unknown) => { - useDialogService().showErrorDialog(error, { - title: t('errorDialog.defaultTitle'), - reportType: 'authenticationError' - }) - console.error(error) - }) - + let comfyOrgAuthToken = await useFirebaseAuthStore().getIdToken() let comfyOrgApiKey = useApiKeyAuthStore().getApiKey() try { @@ -1329,7 +1320,7 @@ export class ComfyApp { const p = await this.graphToPrompt(this.graph) try { - api.authToken = comfyOrgAuthToken ?? undefined + api.authToken = comfyOrgAuthToken api.apiKey = comfyOrgApiKey ?? undefined const res = await api.queuePrompt(number, p, { partialExecutionTargets: queueNodeIds diff --git a/src/stores/firebaseAuthStore.ts b/src/stores/firebaseAuthStore.ts index cdbd8c082f..452ab3d6ee 100644 --- a/src/stores/firebaseAuthStore.ts +++ b/src/stores/firebaseAuthStore.ts @@ -1,5 +1,7 @@ +import { FirebaseError } from 'firebase/app' import { type Auth, + AuthErrorCodes, GithubAuthProvider, GoogleAuthProvider, type User, @@ -20,6 +22,7 @@ import { useFirebaseAuth } from 'vuefire' import { COMFY_API_BASE_URL } from '@/config/comfyApi' import { t } from '@/i18n' +import { useDialogService } from '@/services/dialogService' import { useApiKeyAuthStore } from '@/stores/apiKeyAuthStore' import { type AuthHeader } from '@/types/authTypes' import { operations } from '@/types/comfyRegistryTypes' @@ -88,21 +91,28 @@ export const useFirebaseAuthStore = defineStore('firebaseAuth', () => { lastBalanceUpdateTime.value = null }) - const getIdToken = async (): Promise => { + const getIdToken = async (): Promise => { if (currentUser.value) { try { return await currentUser.value.getIdToken() - } catch (error: any) { - if (error?.code === 'auth/network-request-failed') { + } catch (error: unknown) { + if ( + error instanceof FirebaseError && + error.code === AuthErrorCodes.NETWORK_REQUEST_FAILED + ) { console.warn( 'Could not authenticate with Firebase. Features requiring authentication might not work.' ) - return null // handle gracefully + return } - throw error + + useDialogService().showErrorDialog(error, { + title: t('errorDialog.defaultTitle'), + reportType: 'authenticationError' + }) + console.error(error) } } - return null } /** diff --git a/tests-ui/tests/store/firebaseAuthStore.test.ts b/tests-ui/tests/store/firebaseAuthStore.test.ts index 6887303cd7..e01257beda 100644 --- a/tests-ui/tests/store/firebaseAuthStore.test.ts +++ b/tests-ui/tests/store/firebaseAuthStore.test.ts @@ -1,8 +1,10 @@ +import { FirebaseError } from 'firebase/app' import * as firebaseAuth from 'firebase/auth' import { createPinia, setActivePinia } from 'pinia' import { beforeEach, describe, expect, it, vi } from 'vitest' import * as vuefire from 'vuefire' +import { useDialogService } from '@/services/dialogService' import { useFirebaseAuthStore } from '@/stores/firebaseAuthStore' // Mock fetch @@ -46,21 +48,24 @@ vi.mock('vue-i18n', () => ({ }) })) -vi.mock('firebase/auth', () => ({ - signInWithEmailAndPassword: vi.fn(), - createUserWithEmailAndPassword: vi.fn(), - signOut: vi.fn(), - onAuthStateChanged: vi.fn(), - signInWithPopup: vi.fn(), - GoogleAuthProvider: class { - setCustomParameters = vi.fn() - }, - GithubAuthProvider: class { - setCustomParameters = vi.fn() - }, - browserLocalPersistence: 'browserLocalPersistence', - setPersistence: vi.fn().mockResolvedValue(undefined) -})) +vi.mock('firebase/auth', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + signInWithEmailAndPassword: vi.fn(), + createUserWithEmailAndPassword: vi.fn(), + signOut: vi.fn(), + onAuthStateChanged: vi.fn(), + signInWithPopup: vi.fn(), + GoogleAuthProvider: class { + setCustomParameters = vi.fn() + }, + GithubAuthProvider: class { + setCustomParameters = vi.fn() + }, + setPersistence: vi.fn().mockResolvedValue(undefined) + } +}) // Mock useToastStore vi.mock('@/stores/toastStore', () => ({ @@ -70,11 +75,7 @@ vi.mock('@/stores/toastStore', () => ({ })) // Mock useDialogService -vi.mock('@/services/dialogService', () => ({ - useDialogService: () => ({ - showSettingsDialog: vi.fn() - }) -})) +vi.mock('@/services/dialogService') describe('useFirebaseAuthStore', () => { let store: ReturnType @@ -93,6 +94,12 @@ describe('useFirebaseAuthStore', () => { beforeEach(() => { vi.resetAllMocks() + // Setup dialog service mock + vi.mocked(useDialogService, { partial: true }).mockReturnValue({ + showSettingsDialog: vi.fn(), + showErrorDialog: vi.fn() + }) + // Mock useFirebaseAuth to return our mock auth object vi.mocked(vuefire.useFirebaseAuth).mockReturnValue(mockAuth as any) @@ -297,7 +304,7 @@ describe('useFirebaseAuthStore', () => { const token = await store.getIdToken() - expect(token).toBeNull() + expect(token).toBeUndefined() }) it('should return null for token after login and logout sequence', async () => { @@ -329,7 +336,7 @@ describe('useFirebaseAuthStore', () => { // Verify token is null after logout const tokenAfterLogout = await store.getIdToken() - expect(tokenAfterLogout).toBeNull() + expect(tokenAfterLogout).toBeUndefined() }) it('should handle network errors gracefully when offline (reproduces issue #4468)', async () => { @@ -340,20 +347,19 @@ describe('useFirebaseAuthStore', () => { mockUser.getIdToken.mockReset() // Mock network failure (auth/network-request-failed error from Firebase) - const networkError = new Error( - 'Firebase: Error (auth/network-request-failed).' + const networkError = new FirebaseError( + firebaseAuth.AuthErrorCodes.NETWORK_REQUEST_FAILED, + 'mock error' ) - networkError.name = 'FirebaseError' - ;(networkError as any).code = 'auth/network-request-failed' mockUser.getIdToken.mockRejectedValue(networkError) const token = await store.getIdToken() - expect(token).toBeNull() // Should return null instead of throwing + expect(token).toBeUndefined() // Should return undefined instead of throwing }) - it('should throw error when getIdToken fails with non-network error', async () => { - // This test verifies that non-network errors are thrown instead of handled gracefully + it('should show error dialog when getIdToken fails with non-network error', async () => { + // This test verifies that non-network errors trigger the error dialog mockUser.getIdToken.mockReset() // Mock a non-network error using actual Firebase Auth error code @@ -363,10 +369,16 @@ describe('useFirebaseAuthStore', () => { mockUser.getIdToken.mockRejectedValue(authError) - // Should throw the error instead of returning null - await expect(store.getIdToken()).rejects.toThrow( - 'User account is disabled.' - ) + // Should call the error dialog instead of throwing + const token = await store.getIdToken() + + expect( + vi.mocked(useDialogService)().showErrorDialog + ).toHaveBeenCalledWith(authError, { + title: 'errorDialog.defaultTitle', + reportType: 'authenticationError' + }) + expect(token).toBeUndefined() }) }) @@ -385,11 +397,10 @@ describe('useFirebaseAuthStore', () => { // Setup user with network error on token refresh mockUser.getIdToken.mockReset() - const networkError = new Error( - 'Firebase: Error (auth/network-request-failed).' + const networkError = new FirebaseError( + firebaseAuth.AuthErrorCodes.NETWORK_REQUEST_FAILED, + 'mock error' ) - networkError.name = 'FirebaseError' - ;(networkError as any).code = 'auth/network-request-failed' mockUser.getIdToken.mockRejectedValue(networkError) const authHeader = await store.getAuthHeader() From f46eaf35b3894e70e6906c434da63e907a1937c2 Mon Sep 17 00:00:00 2001 From: Arjan Singh <1598641+arjansingh@users.noreply.github.com> Date: Thu, 21 Aug 2025 17:38:25 -0700 Subject: [PATCH 6/7] [test] use FirebaseError Co-authored-by: Alexander Brown --- tests-ui/tests/store/firebaseAuthStore.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests-ui/tests/store/firebaseAuthStore.test.ts b/tests-ui/tests/store/firebaseAuthStore.test.ts index e01257beda..68e6d5f256 100644 --- a/tests-ui/tests/store/firebaseAuthStore.test.ts +++ b/tests-ui/tests/store/firebaseAuthStore.test.ts @@ -363,9 +363,10 @@ describe('useFirebaseAuthStore', () => { mockUser.getIdToken.mockReset() // Mock a non-network error using actual Firebase Auth error code - const authError = new Error('User account is disabled.') - authError.name = 'FirebaseError' - ;(authError as any).code = 'auth/user-disabled' + const authError = new FirebaseError( + /** whichever one is 'auth/user-disabled' */, + 'User account is disabled.' + ) mockUser.getIdToken.mockRejectedValue(authError) From babc6abab4db4d9cd395c4bb9df26614af7d1160 Mon Sep 17 00:00:00 2001 From: Arjan Singh Date: Thu, 21 Aug 2025 17:50:39 -0700 Subject: [PATCH 7/7] [fix] remove indentation and fix test --- src/stores/firebaseAuthStore.ts | 37 +++++++++---------- .../tests/store/firebaseAuthStore.test.ts | 7 ++-- 2 files changed, 21 insertions(+), 23 deletions(-) diff --git a/src/stores/firebaseAuthStore.ts b/src/stores/firebaseAuthStore.ts index 452ab3d6ee..65b468001a 100644 --- a/src/stores/firebaseAuthStore.ts +++ b/src/stores/firebaseAuthStore.ts @@ -92,26 +92,25 @@ export const useFirebaseAuthStore = defineStore('firebaseAuth', () => { }) const getIdToken = async (): Promise => { - if (currentUser.value) { - try { - return await currentUser.value.getIdToken() - } catch (error: unknown) { - if ( - error instanceof FirebaseError && - error.code === AuthErrorCodes.NETWORK_REQUEST_FAILED - ) { - console.warn( - 'Could not authenticate with Firebase. Features requiring authentication might not work.' - ) - return - } - - useDialogService().showErrorDialog(error, { - title: t('errorDialog.defaultTitle'), - reportType: 'authenticationError' - }) - console.error(error) + if (!currentUser.value) return + try { + return await currentUser.value.getIdToken() + } catch (error: unknown) { + if ( + error instanceof FirebaseError && + error.code === AuthErrorCodes.NETWORK_REQUEST_FAILED + ) { + console.warn( + 'Could not authenticate with Firebase. Features requiring authentication might not work.' + ) + return } + + useDialogService().showErrorDialog(error, { + title: t('errorDialog.defaultTitle'), + reportType: 'authenticationError' + }) + console.error(error) } } diff --git a/tests-ui/tests/store/firebaseAuthStore.test.ts b/tests-ui/tests/store/firebaseAuthStore.test.ts index 68e6d5f256..ffe7a8d995 100644 --- a/tests-ui/tests/store/firebaseAuthStore.test.ts +++ b/tests-ui/tests/store/firebaseAuthStore.test.ts @@ -364,7 +364,7 @@ describe('useFirebaseAuthStore', () => { // Mock a non-network error using actual Firebase Auth error code const authError = new FirebaseError( - /** whichever one is 'auth/user-disabled' */, + firebaseAuth.AuthErrorCodes.USER_DISABLED, 'User account is disabled.' ) @@ -372,10 +372,9 @@ describe('useFirebaseAuthStore', () => { // Should call the error dialog instead of throwing const token = await store.getIdToken() + const dialogService = useDialogService() - expect( - vi.mocked(useDialogService)().showErrorDialog - ).toHaveBeenCalledWith(authError, { + expect(dialogService.showErrorDialog).toHaveBeenCalledWith(authError, { title: 'errorDialog.defaultTitle', reportType: 'authenticationError' })