From 4a61e3464d2fac1854b8d4bed7f90043ef69f76c Mon Sep 17 00:00:00 2001 From: Daniel Bates Date: Thu, 19 Mar 2026 10:39:01 -0700 Subject: [PATCH] fix(common): prevent infinite auth refresh retry loop on permanent token failure (#5893) Co-authored-by: nivedin --- .../helpers/__tests__/retryAuthGuard.spec.ts | 205 ++++++++++++++++++ .../src/helpers/backend/GQLClient.ts | 21 +- .../src/helpers/retryAuthGuard.ts | 78 +++++++ .../src/platform/auth/desktop/index.ts | 6 +- .../src/platform/auth/web/index.ts | 2 - .../hoppscotch-sh-admin/src/helpers/auth.ts | 31 ++- .../src/helpers/retryAuthGuard.ts | 82 +++++++ packages/hoppscotch-sh-admin/src/main.ts | 27 ++- 8 files changed, 427 insertions(+), 25 deletions(-) create mode 100644 packages/hoppscotch-common/src/helpers/__tests__/retryAuthGuard.spec.ts create mode 100644 packages/hoppscotch-common/src/helpers/retryAuthGuard.ts create mode 100644 packages/hoppscotch-sh-admin/src/helpers/retryAuthGuard.ts diff --git a/packages/hoppscotch-common/src/helpers/__tests__/retryAuthGuard.spec.ts b/packages/hoppscotch-common/src/helpers/__tests__/retryAuthGuard.spec.ts new file mode 100644 index 00000000..cf8cc8db --- /dev/null +++ b/packages/hoppscotch-common/src/helpers/__tests__/retryAuthGuard.spec.ts @@ -0,0 +1,205 @@ +import { describe, test, expect, vi } from "vitest" +import { createAuthRetryGuard } from "../retryAuthGuard" + +const refreshSuccess = () => Promise.resolve(true) +const refreshFailure = () => Promise.resolve(false) +const refreshThrow = () => Promise.reject(new Error("network error")) + +describe("createAuthRetryGuard", () => { + describe("success resets failure count", () => { + test("returns true on successful refresh", async () => { + const guard = createAuthRetryGuard(vi.fn()) + + expect(await guard.execute(refreshSuccess)).toBe(true) + }) + + test("resets failure count after a success", async () => { + const onExhausted = vi.fn() + const guard = createAuthRetryGuard(onExhausted) + + // Accumulate 2 failures + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + + // Success resets the counter + await guard.execute(refreshSuccess) + + // 3 more failures needed to exhaust + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + expect(onExhausted).not.toHaveBeenCalled() + + await guard.execute(refreshFailure) + expect(onExhausted).toHaveBeenCalledOnce() + }) + }) + + describe("exhaustion after MAX_RETRIES (3)", () => { + test("calls onExhausted after 3 consecutive failures", async () => { + const onExhausted = vi.fn() + const guard = createAuthRetryGuard(onExhausted) + + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + expect(onExhausted).not.toHaveBeenCalled() + + await guard.execute(refreshFailure) + expect(onExhausted).toHaveBeenCalledOnce() + }) + + test("returns false for every failed attempt", async () => { + const guard = createAuthRetryGuard(vi.fn()) + + expect(await guard.execute(refreshFailure)).toBe(false) + expect(await guard.execute(refreshFailure)).toBe(false) + expect(await guard.execute(refreshFailure)).toBe(false) + }) + + test("short-circuits to false after exhaustion without calling refreshFn", async () => { + const guard = createAuthRetryGuard(vi.fn()) + + // Exhaust the guard + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + + const refreshFn = vi.fn(refreshSuccess) + expect(await guard.execute(refreshFn)).toBe(false) + expect(refreshFn).not.toHaveBeenCalled() + }) + }) + + describe("onExhausted runs at most once", () => { + test("does not call onExhausted again on subsequent execute calls", async () => { + const onExhausted = vi.fn() + const guard = createAuthRetryGuard(onExhausted) + + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + + expect(onExhausted).toHaveBeenCalledOnce() + }) + }) + + describe("thrown errors count as failures", () => { + test("treats a thrown refreshFn as a failed attempt", async () => { + const onExhausted = vi.fn() + const guard = createAuthRetryGuard(onExhausted) + + await guard.execute(refreshThrow) + await guard.execute(refreshThrow) + await guard.execute(refreshThrow) + + expect(onExhausted).toHaveBeenCalledOnce() + }) + + test("does not propagate the error to the caller", async () => { + const guard = createAuthRetryGuard(vi.fn()) + + await expect(guard.execute(refreshThrow)).resolves.toBe(false) + }) + }) + + describe("onExhausted failure handling", () => { + test("stays exhausted if onExhausted throws", async () => { + const onExhausted = vi.fn(() => { + throw new Error("sign-out failed") + }) + const guard = createAuthRetryGuard(onExhausted) + + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + + // Guard is still exhausted — short-circuits + const refreshFn = vi.fn(refreshSuccess) + expect(await guard.execute(refreshFn)).toBe(false) + expect(refreshFn).not.toHaveBeenCalled() + }) + + test("does not propagate onExhausted error to caller", async () => { + const guard = createAuthRetryGuard(() => + Promise.reject(new Error("sign-out failed")) + ) + + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + + await expect(guard.execute(refreshFailure)).resolves.toBe(false) + }) + }) + + describe("reset()", () => { + test("re-enables the guard after exhaustion", async () => { + const onExhausted = vi.fn() + const guard = createAuthRetryGuard(onExhausted) + + // Exhaust + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + + guard.reset() + + // Guard is usable again + expect(await guard.execute(refreshSuccess)).toBe(true) + }) + + test("resets the failure counter", async () => { + const onExhausted = vi.fn() + const guard = createAuthRetryGuard(onExhausted) + + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + + guard.reset() + + // Need 3 fresh failures to exhaust again + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + expect(onExhausted).not.toHaveBeenCalled() + + await guard.execute(refreshFailure) + expect(onExhausted).toHaveBeenCalledOnce() + }) + + test("is a no-op while onExhausted is in-flight", async () => { + let resolveExhausted!: () => void + const exhaustedPromise = new Promise((resolve) => { + resolveExhausted = resolve + }) + const onExhausted = vi.fn(() => exhaustedPromise) + const guard = createAuthRetryGuard(onExhausted) + + await guard.execute(refreshFailure) + await guard.execute(refreshFailure) + + // Start the 3rd failure — onExhausted is now in-flight. + // Don't await: we want to call reset() while it's still pending. + const thirdCall = guard.execute(refreshFailure) + + // Flush microtasks so execute() progresses past `await refreshFn()` + // and sets exhaustionPromise before we call reset(). + await Promise.resolve() + + // reset() while onExhausted hasn't resolved yet — should be a no-op + guard.reset() + + // Guard should still be exhausted + const refreshFn = vi.fn(refreshSuccess) + expect(await guard.execute(refreshFn)).toBe(false) + expect(refreshFn).not.toHaveBeenCalled() + + // Let onExhausted finish + resolveExhausted() + await thirdCall + + // Now reset() should work + guard.reset() + expect(await guard.execute(refreshSuccess)).toBe(true) + }) + }) +}) diff --git a/packages/hoppscotch-common/src/helpers/backend/GQLClient.ts b/packages/hoppscotch-common/src/helpers/backend/GQLClient.ts index 20ba2fc0..d79204de 100644 --- a/packages/hoppscotch-common/src/helpers/backend/GQLClient.ts +++ b/packages/hoppscotch-common/src/helpers/backend/GQLClient.ts @@ -21,8 +21,9 @@ import * as E from "fp-ts/Either" import * as TE from "fp-ts/TaskEither" import { pipe, constVoid, flow } from "fp-ts/function" import { subscribe, pipe as wonkaPipe } from "wonka" -import { filter, map, Subject } from "rxjs" +import { filter, map, Subject, Subscription } from "rxjs" import { platform } from "~/platform" +import { createAuthRetryGuard } from "~/helpers/retryAuthGuard" // TODO: Implement caching @@ -65,6 +66,8 @@ const createSubscriptionClient = () => { }) } +const authRetryGuard = createAuthRetryGuard(() => platform.auth.signOutUser()) + const createHoppClient = () => { const exchanges = [ // devtoolsExchange, @@ -107,9 +110,9 @@ const createHoppClient = () => { }, async refreshAuth() { const refresh = platform.auth.refreshAuthToken - // should we logout if refreshAuthToken is not defined? if (!refresh) return - await refresh() + + await authRetryGuard.execute(() => refresh.call(platform.auth)) }, } }), @@ -146,11 +149,23 @@ const createHoppClient = () => { } let subscriptionClient: SubscriptionClient | null +let authEventSubscription: Subscription | null = null export const client = ref() export function initBackendGQLClient() { client.value = createHoppClient() + // Reset the retry guard only on successful login, not on every + // client recreation (which also fires on logout/token_refresh). + authEventSubscription?.unsubscribe() + authEventSubscription = platform.auth + .getAuthEventsStream() + .subscribe((event) => { + if (event.event === "login") { + authRetryGuard.reset() + } + }) + platform.auth.onBackendGQLClientShouldReconnect(() => { const currentUser = platform.auth.getCurrentUser() diff --git a/packages/hoppscotch-common/src/helpers/retryAuthGuard.ts b/packages/hoppscotch-common/src/helpers/retryAuthGuard.ts new file mode 100644 index 00000000..546ae63b --- /dev/null +++ b/packages/hoppscotch-common/src/helpers/retryAuthGuard.ts @@ -0,0 +1,78 @@ +/** + * Maximum number of consecutive auth refresh failures before signing out. + * @see https://github.com/hoppscotch/hoppscotch/issues/5885 + */ +const MAX_RETRIES = 3 + +/** + * Creates an auth retry guard that tracks consecutive refresh failures + * and triggers a sign-out after {@link MAX_RETRIES} consecutive failures. + * + * After exhaustion, subsequent calls short-circuit to `false` without + * invoking `refreshFn` or `onExhausted` again. Call `reset()` on + * successful login to re-enable refresh attempts. + * + * @see https://github.com/hoppscotch/hoppscotch/issues/5885 + */ +export function createAuthRetryGuard(onExhausted: () => void | Promise) { + let failCount = 0 + let isExhausted = false + let exhaustionPromise: Promise | null = null + + return { + /** + * Wraps an auth refresh attempt with retry tracking. + * Resets on success. Calls `onExhausted` after {@link MAX_RETRIES} + * consecutive failures and stays exhausted until `reset()` is called. + */ + async execute(refreshFn: () => Promise): Promise { + // isExhausted covers the normal path; failCount >= MAX_RETRIES covers + // the concurrent-call edge case where two callers both passed the check + // at failCount = 2 before either could set isExhausted = true. + if (isExhausted || failCount >= MAX_RETRIES) { + return false + } + + let success: boolean + try { + success = await refreshFn() + } catch (_) { + // Treat thrown errors (network failures, etc.) as a failed refresh + // so they count toward exhaustion and don't bypass the guard. + success = false + } + + if (success) { + failCount = 0 + return true + } + + failCount++ + if (failCount >= MAX_RETRIES && !isExhausted) { + isExhausted = true + try { + exhaustionPromise = Promise.resolve().then(() => onExhausted()) + await exhaustionPromise + } catch (_) { + // Sign-out failed (e.g. network error), but the guard stays + // exhausted so we don't re-enter the refresh loop. + } finally { + exhaustionPromise = null + } + } + + return false + }, + + /** + * Reset the failure counter (e.g. on login or manual logout). + * No-op while an exhaustion callback (sign-out) is still in-flight. + */ + reset() { + if (exhaustionPromise) return + + failCount = 0 + isExhausted = false + }, + } +} diff --git a/packages/hoppscotch-selfhost-web/src/platform/auth/desktop/index.ts b/packages/hoppscotch-selfhost-web/src/platform/auth/desktop/index.ts index 07accc24..c590efb8 100644 --- a/packages/hoppscotch-selfhost-web/src/platform/auth/desktop/index.ts +++ b/packages/hoppscotch-selfhost-web/src/platform/auth/desktop/index.ts @@ -222,7 +222,7 @@ async function refreshToken() { try { const refreshToken = await persistenceService.getLocalConfig("refresh_token") - if (!refreshToken) return null + if (!refreshToken) return false const { response } = interceptorService.execute({ id: Date.now(), @@ -523,7 +523,7 @@ export const def: AuthPlatformDef = { async refreshAuthToken() { const refreshed = await refreshToken() - return refreshed ?? false + return refreshed }, /** @@ -553,6 +553,6 @@ export const def: AuthPlatformDef = { } const refreshed = await refreshToken() - return refreshed ?? false + return refreshed }, } diff --git a/packages/hoppscotch-selfhost-web/src/platform/auth/web/index.ts b/packages/hoppscotch-selfhost-web/src/platform/auth/web/index.ts index 74889468..a65d75df 100644 --- a/packages/hoppscotch-selfhost-web/src/platform/auth/web/index.ts +++ b/packages/hoppscotch-selfhost-web/src/platform/auth/web/index.ts @@ -339,8 +339,6 @@ export const def: AuthPlatformDef = { }, async signOutUser() { - // if (!currentUser$.value) throw new Error("No user has logged in") - await logout() probableUser$.next(null) diff --git a/packages/hoppscotch-sh-admin/src/helpers/auth.ts b/packages/hoppscotch-sh-admin/src/helpers/auth.ts index 1002d666..48e4291a 100644 --- a/packages/hoppscotch-sh-admin/src/helpers/auth.ts +++ b/packages/hoppscotch-sh-admin/src/helpers/auth.ts @@ -56,12 +56,12 @@ export type OnboardingStatus = { const currentUser$ = new BehaviorSubject(null); const signOut = async (reloadWindow = false) => { - await authQuery.logout(); - - // Reload the window if both `access_token` and `refresh_token`is invalid - // there by the user is taken to the login page - if (reloadWindow) { - window.location.reload(); + // Best-effort backend logout — local state must be cleared regardless + // so the UI never stays stuck in an authenticated state. + try { + await authQuery.logout(); + } catch (_) { + // Backend unreachable — continue with local cleanup } currentUser$.next(null); @@ -70,6 +70,12 @@ const signOut = async (reloadWindow = false) => { authEvents$.next({ event: 'logout', }); + + // Reload the window if both `access_token` and `refresh_token` are invalid + // thereby the user is taken to the login page + if (reloadWindow) { + window.location.reload(); + } }; const getUserDetails = async () => { @@ -132,10 +138,15 @@ const setInitialUser = async () => { const refreshToken = async () => { try { const res = await authQuery.refreshToken(); - authEvents$.next({ - event: 'token_refresh', - }); - return res.status === 200; + const isSuccessful = res.status === 200; + + if (isSuccessful) { + authEvents$.next({ + event: 'token_refresh', + }); + } + + return isSuccessful; } catch { return false; } diff --git a/packages/hoppscotch-sh-admin/src/helpers/retryAuthGuard.ts b/packages/hoppscotch-sh-admin/src/helpers/retryAuthGuard.ts new file mode 100644 index 00000000..4742cf9f --- /dev/null +++ b/packages/hoppscotch-sh-admin/src/helpers/retryAuthGuard.ts @@ -0,0 +1,82 @@ +/** + * Maximum number of consecutive auth refresh failures before signing out. + * @see https://github.com/hoppscotch/hoppscotch/issues/5885 + */ +const MAX_RETRIES = 3 + +/** + * Creates an auth retry guard that tracks consecutive refresh failures + * and triggers a sign-out after {@link MAX_RETRIES} consecutive failures. + * + * After exhaustion, subsequent calls short-circuit to `false` without + * invoking `refreshFn` or `onExhausted` again. Call `reset()` on + * successful login to re-enable refresh attempts. + * + * NOTE: This is a copy of `@hoppscotch/common/helpers/retryAuthGuard.ts`. + * `sh-admin` cannot depend on `@hoppscotch/common`, so the utility is + * duplicated here. Keep both copies in sync. + * + * @see https://github.com/hoppscotch/hoppscotch/issues/5885 + */ +export function createAuthRetryGuard(onExhausted: () => void | Promise) { + let failCount = 0 + let isExhausted = false + let exhaustionPromise: Promise | null = null + + return { + /** + * Wraps an auth refresh attempt with retry tracking. + * Resets on success. Calls `onExhausted` after {@link MAX_RETRIES} + * consecutive failures and stays exhausted until `reset()` is called. + */ + async execute(refreshFn: () => Promise): Promise { + // isExhausted covers the normal path; failCount >= MAX_RETRIES covers + // the concurrent-call edge case where two callers both passed the check + // at failCount = 2 before either could set isExhausted = true. + if (isExhausted || failCount >= MAX_RETRIES) { + return false + } + + let success: boolean + try { + success = await refreshFn() + } catch (_) { + // Treat thrown errors (network failures, etc.) as a failed refresh + // so they count toward exhaustion and don't bypass the guard. + success = false + } + + if (success) { + failCount = 0 + return true + } + + failCount++ + if (failCount >= MAX_RETRIES && !isExhausted) { + isExhausted = true + try { + exhaustionPromise = Promise.resolve().then(() => onExhausted()) + await exhaustionPromise + } catch (_) { + // Sign-out failed (e.g. network error), but the guard stays + // exhausted so we don't re-enter the refresh loop. + } finally { + exhaustionPromise = null + } + } + + return false + }, + + /** + * Reset the failure counter (e.g. on login or manual logout). + * No-op while an exhaustion callback (sign-out) is still in-flight. + */ + reset() { + if (exhaustionPromise) return + + failCount = 0 + isExhausted = false + }, + } +} diff --git a/packages/hoppscotch-sh-admin/src/main.ts b/packages/hoppscotch-sh-admin/src/main.ts index 1313c7a6..75909af0 100644 --- a/packages/hoppscotch-sh-admin/src/main.ts +++ b/packages/hoppscotch-sh-admin/src/main.ts @@ -1,6 +1,7 @@ import { authExchange } from '@urql/exchange-auth'; import urql, { cacheExchange, createClient, fetchExchange } from '@urql/vue'; import { createApp, h } from 'vue'; +import * as O from 'fp-ts/Option'; import App from './App.vue'; import ErrorComponent from './pages/_.vue'; @@ -13,12 +14,24 @@ import '../assets/scss/styles.scss'; import '../assets/scss/tailwind.scss'; // END STYLES -import { pipe } from 'fp-ts/function'; -import * as O from 'fp-ts/Option'; -import { auth } from './helpers/auth'; +import { auth, authEvents$ } from './helpers/auth'; import { GRAPHQL_UNAUTHORIZED } from './helpers/errors'; +import { createAuthRetryGuard } from './helpers/retryAuthGuard'; import { HOPP_MODULES } from './modules'; +/** + * Auth retry guard — prevents infinite refreshAuth loops when tokens + * are permanently invalid. Stays exhausted until the page reloads. + * @see https://github.com/hoppscotch/hoppscotch/issues/5885 + */ +const authRetryGuard = createAuthRetryGuard(() => auth.signOutUser(true)); + +authEvents$.subscribe((event) => { + if (event.event === 'login') { + authRetryGuard.reset(); + } +}); + (async () => { try { // Create URQL client @@ -35,10 +48,10 @@ import { HOPP_MODULES } from './modules'; return operation; }, async refreshAuth() { - pipe( - await auth.performAuthRefresh(), - O.getOrElseW(() => auth.signOutUser(true)) - ); + await authRetryGuard.execute(async () => { + const result = await auth.performAuthRefresh(); + return O.isSome(result); + }); }, didAuthError(error, _operation) { return error.message === GRAPHQL_UNAUTHORIZED;