diff --git a/ui/src/components/OnboardingWizard.test.tsx b/ui/src/components/OnboardingWizard.test.tsx index f837407c66..47e8afd88c 100644 --- a/ui/src/components/OnboardingWizard.test.tsx +++ b/ui/src/components/OnboardingWizard.test.tsx @@ -496,11 +496,15 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); it("does not hand one account's draft to the next when a refetch fails after a switch", async () => { // The attack path in full. Account A onboards and leaves a draft naming - // its company. A signs out — which does not clear the companies cache, - // because `useSignOut` invalidates only the session and health queries. - // B signs in and the companies refetch fails, so A's list is still in + // its company. The account then changes without this component's company + // cache being cleared, and the refetch fails, so A's list is still in // hand. A list that still contains A's company must not be read as proof // that B owns it. + // + // `useSignOut` now resets account-scoped caches, which closes the + // sign-out-button route into this state (see its own regression test). The + // gate is asserted here independently of that: it must hold for any route + // that leaves a stale list behind, not only the one that has been fixed. window.localStorage.setItem( ONBOARDING_STORAGE_KEY, JSON.stringify({ diff --git a/ui/src/components/OnboardingWizard.tsx b/ui/src/components/OnboardingWizard.tsx index 827e98832f..98b00435f4 100644 --- a/ui/src/components/OnboardingWizard.tsx +++ b/ui/src/components/OnboardingWizard.tsx @@ -205,10 +205,15 @@ export function OnboardingWizard() { // trusts "not loading, no error" finds the old company id in them and hands // one account's onboarding draft to the next. // - // Sign-out does not help: `useSignOut` never clears the company cache, and - // on the self-hosted path there is no document reload to clear it either. - // Even once that is fixed, an account change can skip the button entirely - - // a session lapsing server-side, a second account signing in on a warm tab. + // Sign-out is no longer the hole it was: `useSignOut` now resets every + // account-scoped cache entry rather than invalidating two of them, so the + // ordinary A-signs-out-then-B-signs-in path does not leave A's companies in + // hand. + // + // That covers the button, not the question. An account can change without + // it - a session lapsing server-side, a second account signing in on a warm + // tab, a caller supplying the company context from somewhere else - so this + // gate stays independent of that fix rather than deferring to it. // // So this asks for a list fetched *for this mount*, rather than reading the // one in hand. `isFetchedAfterMount` is the part that matters; `staleTime: 0` diff --git a/ui/src/hooks/useSignOut.test.tsx b/ui/src/hooks/useSignOut.test.tsx index 0745ca1187..3a292a4b5e 100644 --- a/ui/src/hooks/useSignOut.test.tsx +++ b/ui/src/hooks/useSignOut.test.tsx @@ -1,11 +1,12 @@ // @vitest-environment jsdom +import { act } from "react"; import { flushSync } from "react-dom"; import { createRoot } from "react-dom/client"; -import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { QueryClient, QueryClientProvider, useQuery } from "@tanstack/react-query"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { queryKeys } from "@/lib/queryKeys"; -import { useSignOut } from "./useSignOut"; +import { isAccountScopedQueryKey, useSignOut } from "./useSignOut"; const mockAuthApi = vi.hoisted(() => ({ signOut: vi.fn() })); const mockNavigateTopLevel = vi.hoisted(() => vi.fn()); @@ -75,13 +76,22 @@ describe("useSignOut", () => { flushSync(() => root.unmount()); }); - it("keeps self-hosted sign-out pending until the local request finishes, then invalidates caches", async () => { + it("keeps self-hosted sign-out pending until the local request finishes, then clears account caches", async () => { let resolveSignOut: (() => void) | undefined; mockAuthApi.signOut.mockImplementation(() => new Promise((resolve) => { resolveSignOut = resolve; })); queryClient.setQueryData(queryKeys.health, { status: "ok", deploymentMode: "authenticated" }); queryClient.setQueryData(queryKeys.auth.session, { session: { id: "session-1" } }); + queryClient.setQueryData(queryKeys.companies.all, { + companies: [{ id: "company-a", name: "Account A Co" }], + unauthorized: false, + }); + queryClient.setQueryData(queryKeys.access.currentBoardAccess, { + isInstanceAdmin: false, + companyIds: ["company-a"], + }); + queryClient.setQueryData(queryKeys.issues.list("company-a"), [{ id: "issue-a" }]); const onSignedOut = vi.fn(); const root = renderHarness(onSignedOut); @@ -97,16 +107,101 @@ describe("useSignOut", () => { expect(captured?.error).toBeNull(); expect(onSignedOut).toHaveBeenCalledOnce(); - expect(queryClient.getQueryState(queryKeys.auth.session)?.isInvalidated).toBe(true); + + // Account-scoped entries are gone outright, not merely marked stale. An + // invalidated entry keeps serving its old value until a refetch succeeds, + // which is the leak this guards. + expect(queryClient.getQueryData(queryKeys.auth.session)).toBeUndefined(); + expect(queryClient.getQueryData(queryKeys.companies.all)).toBeUndefined(); + expect(queryClient.getQueryData(queryKeys.access.currentBoardAccess)).toBeUndefined(); + expect(queryClient.getQueryData(queryKeys.issues.list("company-a"))).toBeUndefined(); + + // Health describes the instance, not the account, and several observers + // read it with `enabled: false`. It is refreshed in place. + expect(queryClient.getQueryData(queryKeys.health)).toEqual({ + status: "ok", + deploymentMode: "authenticated", + }); expect(queryClient.getQueryState(queryKeys.health)?.isInvalidated).toBe(true); flushSync(() => root.unmount()); }); - it("exposes a stable error without closing chrome or invalidating caches", async () => { + it("leaves the previous account nothing to read when the next account's refetch fails", async () => { + // The end-to-end shape of the leak. Account A's company list is in cache, + // A signs out, B signs in, and B's companies request fails. + // `companiesListQueryOptions` sets `retry: false`, so that single failure + // is final — and under `invalidateQueries` the observer would still be + // holding A's list, which consumers read as "this account owns these + // companies". + mockAuthApi.signOut.mockResolvedValue(undefined); + queryClient.setQueryData(queryKeys.health, { status: "ok", deploymentMode: "authenticated" }); + + const companiesQueryFn = vi + .fn() + .mockResolvedValueOnce({ + companies: [{ id: "company-a", name: "Account A Co" }], + unauthorized: false, + }) + .mockRejectedValue(new Error("refetch failed for the new account")); + + // A live observer, standing in for CompanyProvider — which sits above the + // router and stays mounted across the whole sign-out/sign-in cycle, so it + // never gets a fresh mount to clear it. + let observed: unknown = "unset"; + function CompaniesObserver() { + const { data } = useQuery({ + queryKey: queryKeys.companies.all, + queryFn: companiesQueryFn, + retry: false, + }); + observed = data; + return null; + } + + const root = createRoot(container); + await act(async () => { + root.render( + + + + , + ); + }); + + await act(async () => { + await vi.waitFor(() => + expect(observed).toEqual({ + companies: [{ id: "company-a", name: "Account A Co" }], + unauthorized: false, + }), + ); + }); + + await act(async () => { + captured?.mutate(); + // The sign-out resolves, its cache reset fires, and the refetch that + // reset kicks off is the one that fails. + await vi.waitFor(() => expect(captured?.isSuccess).toBe(true)); + await vi.waitFor(() => expect(companiesQueryFn).toHaveBeenCalledTimes(2)); + }); + + expect(queryClient.getQueryData(queryKeys.companies.all)).toBeUndefined(); + expect(observed).toBeUndefined(); + + await act(async () => { + root.unmount(); + }); + }); + + it("exposes a stable error without closing chrome or clearing caches", async () => { mockAuthApi.signOut.mockRejectedValue(new Error("Sign-out request failed")); queryClient.setQueryData(queryKeys.health, { status: "ok", deploymentMode: "authenticated" }); queryClient.setQueryData(queryKeys.auth.session, { session: { id: "session-1" } }); + queryClient.setQueryData(queryKeys.companies.all, { + companies: [{ id: "company-a", name: "Account A Co" }], + unauthorized: false, + }); const onSignedOut = vi.fn(); const root = renderHarness(onSignedOut); @@ -115,9 +210,62 @@ describe("useSignOut", () => { await vi.waitFor(() => expect(captured?.error?.message).toBe("Sign-out request failed")); expect(captured?.isPending).toBe(false); expect(onSignedOut).not.toHaveBeenCalled(); + // The account is still signed in, so its caches must survive intact — + // a failed sign-out must not blank the app the user is still using. + expect(queryClient.getQueryData(queryKeys.auth.session)).toEqual({ session: { id: "session-1" } }); + expect(queryClient.getQueryData(queryKeys.companies.all)).toEqual({ + companies: [{ id: "company-a", name: "Account A Co" }], + unauthorized: false, + }); expect(queryClient.getQueryState(queryKeys.auth.session)?.isInvalidated).toBe(false); expect(queryClient.getQueryState(queryKeys.health)?.isInvalidated).toBe(false); flushSync(() => root.unmount()); }); + + it("leaves the Cloud cache to the document reload rather than clearing it locally", async () => { + // The Cloud path never reaches onSuccess's clearing branch: it hands off to + // a top-level navigation, and the resulting document load builds a new + // QueryClient from scratch. Clearing here would only blank the UI during + // the frames before that navigation commits. + queryClient.setQueryData(queryKeys.health, { + status: "ok", + cloud: { + managed: true, + managedBy: "paperclip-cloud", + stackSlug: "acme", + cloudBaseUrl: "https://cloud.example.test", + }, + }); + queryClient.setQueryData(queryKeys.companies.all, { + companies: [{ id: "company-a", name: "Account A Co" }], + unauthorized: false, + }); + const root = renderHarness(); + + flushSync(() => captured?.mutate()); + + await vi.waitFor(() => expect(mockNavigateTopLevel).toHaveBeenCalledOnce()); + expect(queryClient.getQueryData(queryKeys.companies.all)).toEqual({ + companies: [{ id: "company-a", name: "Account A Co" }], + unauthorized: false, + }); + + flushSync(() => root.unmount()); + }); +}); + +describe("isAccountScopedQueryKey", () => { + it("keeps only the instance-scoped health entry", () => { + expect(isAccountScopedQueryKey(queryKeys.health)).toBe(false); + }); + + it("treats every other key root as account-scoped, including ones added later", () => { + expect(isAccountScopedQueryKey(queryKeys.companies.all)).toBe(true); + expect(isAccountScopedQueryKey(queryKeys.auth.session)).toBe(true); + expect(isAccountScopedQueryKey(queryKeys.access.currentBoardAccess)).toBe(true); + expect(isAccountScopedQueryKey(queryKeys.issues.list("company-a"))).toBe(true); + expect(isAccountScopedQueryKey(queryKeys.secrets.list("company-a"))).toBe(true); + expect(isAccountScopedQueryKey(["some-key-nobody-has-written-yet"])).toBe(true); + }); }); diff --git a/ui/src/hooks/useSignOut.ts b/ui/src/hooks/useSignOut.ts index 0d91d66a98..1a61ec164a 100644 --- a/ui/src/hooks/useSignOut.ts +++ b/ui/src/hooks/useSignOut.ts @@ -6,6 +6,27 @@ import { useCloudInstance } from "./useCloudInstance"; const CLOUD_SIGN_OUT_PATH = "/cloud/logout"; +/** + * Query-key roots that describe the *instance* rather than the account that was + * signed in, and so are allowed to outlive a sign-out. + * + * `health` is the only one. It carries deployment mode, bootstrap state and + * Cloud metadata — nothing account-scoped — and `useCloudInstance` observes it + * with `enabled: false`, deliberately leaving the fetch to CloudAccessGate. + * Dropping the entry would strand every such observer on `null` until the gate + * happened to refetch, flipping Cloud instances into their self-hosted + * rendering mid-sign-out. It is refreshed in place instead. + * + * Everything *not* listed here is treated as account-scoped and cleared. That + * direction is the point: a query key added later is account-scoped unless + * someone deliberately says otherwise, so forgetting this file fails closed. + */ +const INSTANCE_SCOPED_QUERY_ROOTS: readonly unknown[] = [queryKeys.health[0]]; + +export function isAccountScopedQueryKey(queryKey: readonly unknown[]): boolean { + return !INSTANCE_SCOPED_QUERY_ROOTS.includes(queryKey[0]); +} + interface UseSignOutOptions { onSignedOut?: () => void; } @@ -14,8 +35,10 @@ interface UseSignOutOptions { * Owns the app-wide sign-out decision. * * Cloud-managed tenants must enter the harness-owned logout sequence without - * first clearing the tenant session. Authenticated self-hosted instances keep - * the local API flow and invalidate the auth-dependent caches afterward. + * first clearing the tenant session; that path is a top-level navigation, so + * the document reload it triggers builds a new QueryClient and there is nothing + * left here to clear. Authenticated self-hosted instances keep the local API + * flow and drop the account-scoped caches afterward. */ export function useSignOut({ onSignedOut }: UseSignOutOptions = {}) { const cloud = useCloudInstance(); @@ -36,10 +59,34 @@ export function useSignOut({ onSignedOut }: UseSignOutOptions = {}) { if (target === "cloud") return; onSignedOut?.(); - await Promise.all([ - queryClient.invalidateQueries({ queryKey: queryKeys.auth.session }), - queryClient.invalidateQueries({ queryKey: queryKeys.health }), - ]); + + // Drop every account-scoped cache entry, rather than invalidating a + // couple of them. `invalidateQueries` only marks an entry stale and goes + // on serving the old value until a refetch succeeds — and the companies + // query sets `retry: false`, so a single failed request is enough to + // leave the previous account's company list readable for the whole of + // the *next* account's session. Consumers that read a non-empty list as + // authoritative (company auto-selection, the invite-landing "already a + // member" check, the board-access gate) would then act on it. + // + // `resetQueries` rather than `removeQueries`: removal empties the cache + // but does not notify the observers already subscribed to those entries, + // so a mounted `useQuery` keeps returning its last result until some + // unrelated re-render happens to rebuild the query. That is not a corner + // case here — CompanyProvider sits above the router and stays mounted + // across the whole sign-out/sign-in cycle. Reset notifies them, so the + // old data is gone from the cache *and* from everything reading it. + // + // Not awaited: the refetches this kicks off are expected to 401 now that + // the session is gone, and the sign-out button should not sit pending + // while they fail. + void queryClient.resetQueries({ + predicate: (query) => isAccountScopedQueryKey(query.queryKey), + }); + + // Instance-scoped, so refreshed in place rather than dropped — see + // INSTANCE_SCOPED_QUERY_ROOTS. + await queryClient.invalidateQueries({ queryKey: queryKeys.health }); }, }); } diff --git a/ui/src/pages/InstanceGeneralSettings.test.tsx b/ui/src/pages/InstanceGeneralSettings.test.tsx index 9eff02c7a1..961b540c5c 100644 --- a/ui/src/pages/InstanceGeneralSettings.test.tsx +++ b/ui/src/pages/InstanceGeneralSettings.test.tsx @@ -101,17 +101,27 @@ describe("InstanceGeneralSettings sign-out", () => { expect(mockAuthApi.signOut).not.toHaveBeenCalled(); }); - it("keeps authenticated self-hosted sign-out local and invalidates auth caches", async () => { + it("keeps authenticated self-hosted sign-out local and drops the account caches", async () => { const invalidateQueries = vi.spyOn(queryClient, "invalidateQueries"); await renderPage(SELF_HOSTED_HEALTH); + queryClient.setQueryData(queryKeys.auth.session, { session: { id: "session-1" } }); + queryClient.setQueryData(queryKeys.companies.all, { + companies: [{ id: "company-a", name: "Account A Co" }], + unauthorized: false, + }); flushSync(() => signOutButton()?.click()); await vi.waitFor(() => expect(mockAuthApi.signOut).toHaveBeenCalledOnce()); - await vi.waitFor(() => expect(invalidateQueries).toHaveBeenCalledWith({ - queryKey: queryKeys.auth.session, - })); + // Account-scoped entries are cleared outright, not marked stale — a stale + // entry keeps serving the previous account's data until a refetch succeeds. + await vi.waitFor(() => + expect(queryClient.getQueryData(queryKeys.auth.session)).toBeUndefined(), + ); + expect(queryClient.getQueryData(queryKeys.companies.all)).toBeUndefined(); + // Health describes the instance, so it is refreshed rather than dropped. expect(invalidateQueries).toHaveBeenCalledWith({ queryKey: queryKeys.health }); + expect(queryClient.getQueryData(queryKeys.health)).toEqual(SELF_HOSTED_HEALTH); expect(mockNavigateTopLevel).not.toHaveBeenCalled(); });