From 0817fbad9264884dd8fea021a725af9761aa40ab Mon Sep 17 00:00:00 2001 From: Tonio Date: Sun, 16 Aug 2026 21:55:50 -0700 Subject: [PATCH] fix(ui): scope invite membership checks to the signed-in account (#11417) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Which companies a person belongs to is an authorization fact the server owns, and the UI caches the answer under a single `["companies"]` key > - That cache entry carries no account identity, and `main.tsx` sets `staleTime: 30_000` for every query, so for thirty seconds after a sign-in the previous account's list is served with no request at all > - The invite landing page reads that list to decide whether the person is already a member of the inviting company > - A list that arrives with no loading state and no error therefore looks authoritative while describing somebody else > - This pull request makes the page trust only a list it fetched itself, for the account signed in now > - The benefit is that a membership decision stops depending on cache freshness, which nothing in the app guarantees ## Linked Issues or Issue Description No public issue exists. Refs #11380, #11382. The problem follows. **What happened?** `InviteLanding` read the shared `["companies"]` cache entry as proof of membership in two places: - The post-sign-in redirect called `fetchQuery(companiesListQueryOptions)`, which returns the cached entry without a request while it is inside the app-wide `staleTime`. - An effect cleared the pending invite token whenever the cached list contained the invited company. Neither checked that the list belonged to the account signed in now. A second account signing in on a warm tab, or a session that lapses server-side, is enough to reach both. **Expected behavior** The page decides membership from a company list fetched for the current session. **Steps to reproduce** 1. On a self-hosted instance in `authenticated` mode, sign in as account A, which belongs to company X. 2. Within thirty seconds, open an invite link for company X and sign in as account B, which does not belong to it. 3. The page reads A's cached list, finds company X, and treats B as already a member. **Paperclip version or commit** `master` at `2a4b4bc63`. ## What Changed - `ui/src/pages/InviteLanding.tsx` — the membership query sets `staleTime: 0` so it revalidates on mount, and the verdict is withheld until that fetch lands, keyed on `isFetchedAfterMount`. The token-clearing effect and the "already a member" branch both read through that gate. - `ui/src/pages/InviteLanding.tsx` — the post-sign-in path cancels anything still in flight for the previous session, then forces a fetch for the new one with `staleTime: 0`. - `ui/src/pages/Auth.tsx` — sign-in resets the companies query instead of invalidating it. Invalidation leaves the previous account's list readable, and its fetch running, until the refetch returns. - `ui/src/pages/InviteLanding.test.tsx` — coverage for the warm-cache case, the token-clearing effect, and the `local_trusted` exemption. ### `local_trusted` is exempt Those instances have no accounts, so the shared list is the only identity there is. `membershipIsAccountScoped` is false there and the gate stays open. ### Rebased onto the account-keyed cache #11488 landed while this was open and keys the company list by account, so the page can no longer reach another account's list at all. Two things changed here as a result: - The post-sign-in read now calls `fetchCompanyListForCurrentAccount`, which replaces the `cancelQueries` plus forced `fetchQuery` this PR originally carried. The helper is strictly stronger: it detaches the in-flight `/companies` request inside the query function, and it resolves the account identity past the session invalidation immediately above rather than trusting the session entry still in the cache. - The observer reads through `useCompanyListQuery`. The mount-scoped `isFetchedAfterMount` gate is **kept**, not removed. Its purpose has narrowed — cross-account leakage is now structurally impossible, so what remains is holding the verdict until this page has a list rather than acting on a pending one. It is still load-bearing: disabling it fails two tests here. Removing a defense in the same change that rebases onto a new foundation is the wrong order; that is a follow-up once the keying has proven itself. `Auth.tsx` can safely reset, because it navigates away on success and `InviteLanding` mounts fresh afterward. Measurements of exactly when that rewind does and does not bite are in [#11380](https://github.com/paperclipai/paperclip/pull/11380#issuecomment-5300984911). ## Verification - `InviteLanding.test.tsx`, `Auth.test.tsx`, `companies-query.test.ts`, `CompanyContext.test.tsx` together: **51 passed**, run twice. - `pnpm tsc -b`: clean. - `InviteLanding.test.tsx` and `Auth.test.tsx` together: 21 passed. Both failures are pre-existing and unrelated. Each reproduces on a tree that does not contain this change, in files this change does not touch: | Failure | Why it fails | | --- | --- | | `IssueProperties.test.tsx` | Timezone-dependent: expects `4:08 PM`, gets `9:08 AM` | | `StatusCards/format.test.ts` | Time-of-day dependent: "only counts updates started today" breaks near midnight | **Not done:** no manual two-account run in a browser. The path needs two accounts on an `authenticated` instance, which a local dev instance cannot exercise. ## Risks Low. The failure direction is a membership verdict withheld for one extra round trip, which resolves itself; the direction it removes is one account's membership granted to another, which does not. It adds one request per invite-page mount, on a query key the app already uses. **This does not close the class.** The shared list is still unscoped for every other consumer. #11380 clears it on sign-out and #11382 handles the onboarding draft gate; all three are needed, because an account can change without passing through any one of those paths. ## Model Used Claude Opus 5 (`claude-opus-5`), through Claude Code. Extended thinking enabled. Tool use enabled: file read and edit, shell for typecheck and test runs. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 --- ui/src/pages/Auth.tsx | 6 +- ui/src/pages/InviteLanding.test.tsx | 225 ++++++++++++++++++++++++++++ ui/src/pages/InviteLanding.tsx | 26 +++- 3 files changed, 250 insertions(+), 7 deletions(-) diff --git a/ui/src/pages/Auth.tsx b/ui/src/pages/Auth.tsx index 670677ea33..bbd4707a4a 100644 --- a/ui/src/pages/Auth.tsx +++ b/ui/src/pages/Auth.tsx @@ -55,7 +55,11 @@ export function AuthPage() { setError(null); await queryClient.invalidateQueries({ queryKey: queryKeys.auth.session }); await queryClient.invalidateQueries({ queryKey: queryKeys.health }); - await queryClient.invalidateQueries({ queryKey: queryKeys.companies.all }); + // Reset rather than invalidate: the `["companies"]` entry is shared app-wide and + // is not account-scoped, so invalidating leaves the previous account's list + // readable (and any fetch for that session in flight) until the refetch lands. + // Sign-in can change accounts, so drop the list outright. + await queryClient.resetQueries({ queryKey: queryKeys.companies.all }); navigate(nextPath, { replace: true }); }, onError: (err) => { diff --git a/ui/src/pages/InviteLanding.test.tsx b/ui/src/pages/InviteLanding.test.tsx index 891dbfecc0..4ab029172b 100644 --- a/ui/src/pages/InviteLanding.test.tsx +++ b/ui/src/pages/InviteLanding.test.tsx @@ -767,6 +767,231 @@ describe("InviteLandingPage", () => { }); }); + // The `["companies"]` cache entry is shared app-wide and carries no account + // identity, so a list fetched moments ago for a different account is still inside + // the app-wide staleTime. Membership decisions on this page — the post-sign-in + // redirect, the "already a member" panel, and clearing the pending invite token — + // must run against a list fetched for the account that is signed in now. + describe("membership checks against a previous account's cached company list", () => { + const PREVIOUS_ACCOUNT_COMPANIES = [{ id: "company-1", name: "Acme Robotics" }]; + + // Mirrors the app-wide default in main.tsx. Without it every cache entry is + // stale on read and the staleness these tests cover cannot occur. + // `ownerId` is whose list this warm entry is. Since #11488 the entry is keyed + // by account, so "a list left by a previous account" means putting it under + // that account's key — which is also why the page can no longer reach it. + // `null` is the signed-out / local_trusted key. + function createAppLikeQueryClient(ownerId: string | null) { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false, staleTime: 30_000 } }, + }); + queryClient.setQueryData(queryKeys.companies.list(ownerId), { + companies: PREVIOUS_ACCOUNT_COMPANIES, + unauthorized: false, + }); + return queryClient; + } + + it("re-reads membership for the account that just signed in", async () => { + getSessionMock.mockResolvedValueOnce(null); + getSessionMock.mockResolvedValue({ + session: { id: "session-2", userId: "user-2" }, + user: { + id: "user-2", + name: "Sam Example", + email: "sam@example.com", + image: null, + }, + }); + // The account signing in here has no companies of its own. + listCompaniesMock.mockResolvedValue([]); + acceptInviteMock.mockResolvedValue({ + id: "join-1", + companyId: "company-1", + requestType: "human", + status: "pending_approval", + }); + + const root = createRoot(container); + const queryClient = createAppLikeQueryClient("user-1"); + + await act(async () => { + root.render( + + + + } /> + + + , + ); + }); + await flushReact(); + await flushReact(); + + const inputValueSetter = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, "value")?.set; + const existingAccountButton = Array.from(container.querySelectorAll("button")).find( + (button) => button.textContent === "I already have an account", + ); + await act(async () => { + existingAccountButton?.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + await flushReact(); + + const emailInput = container.querySelector('input[name="email"]') as HTMLInputElement | null; + const passwordInput = container.querySelector('input[name="password"]') as HTMLInputElement | null; + + await act(async () => { + inputValueSetter!.call(emailInput, "sam@example.com"); + emailInput!.dispatchEvent(new Event("input", { bubbles: true })); + inputValueSetter!.call(passwordInput, "supersecret"); + passwordInput!.dispatchEvent(new Event("input", { bubbles: true })); + }); + + const authForm = container.querySelector('[data-testid="invite-inline-auth"]') as HTMLFormElement | null; + await act(async () => { + authForm?.dispatchEvent(new Event("submit", { bubbles: true, cancelable: true })); + }); + await flushReact(); + await flushReact(); + await flushReact(); + await flushReact(); + + expect(signInEmailMock).toHaveBeenCalledWith({ + email: "sam@example.com", + password: "supersecret", + }); + // The cached list was refused, not read as proof of membership. + expect(listCompaniesMock).toHaveBeenCalled(); + expect(queryClient.getQueryData(queryKeys.companies.list("user-2"))).toMatchObject({ + companies: [], + unauthorized: false, + }); + // The previous account's entry is untouched — and unreachable from here. + expect(queryClient.getQueryData(queryKeys.companies.list("user-1"))).toMatchObject({ + companies: PREVIOUS_ACCOUNT_COMPANIES, + }); + expect(setSelectedCompanyIdMock).not.toHaveBeenCalled(); + expect(acceptInviteMock).toHaveBeenCalledWith("pcp_invite_test", { requestType: "human" }); + expect(container.textContent).toContain("Request to join Acme Robotics"); + + await act(async () => { + root.unmount(); + }); + }); + + it("re-reads membership on mount for an already signed-in account", async () => { + getSessionMock.mockResolvedValue({ + session: { id: "session-2", userId: "user-2" }, + user: { + id: "user-2", + name: "Sam Example", + email: "sam@example.com", + image: null, + }, + }); + listCompaniesMock.mockResolvedValue([]); + acceptInviteMock.mockResolvedValue({ + id: "join-1", + companyId: "company-1", + requestType: "human", + status: "pending_approval", + }); + + const root = createRoot(container); + const queryClient = createAppLikeQueryClient("user-1"); + + await act(async () => { + root.render( + + + + } /> + + + , + ); + }); + await flushReact(); + await flushReact(); + await flushReact(); + await flushReact(); + + expect(listCompaniesMock).toHaveBeenCalled(); + expect(container.textContent).not.toContain("Already in this company"); + expect(acceptInviteMock).toHaveBeenCalledWith("pcp_invite_test", { requestType: "human" }); + expect(container.textContent).toContain("Request to join Acme Robotics"); + + await act(async () => { + root.unmount(); + }); + }); + + it("keeps the pending invite token when the session has lapsed", async () => { + // Session gone server-side, previous account's list still warm in the cache. + // Nobody is signed in, so nothing here proves membership — the token has to + // survive for the sign-in that follows. + getSessionMock.mockResolvedValue(null); + + const root = createRoot(container); + const queryClient = createAppLikeQueryClient("user-1"); + + await act(async () => { + root.render( + + + + } /> + + + , + ); + }); + await flushReact(); + await flushReact(); + await flushReact(); + + expect(container.querySelector('[data-testid="invite-inline-auth"]')).not.toBeNull(); + expect(localStorage.getItem("paperclip:pending-invite-token")).toBe("pcp_invite_test"); + + await act(async () => { + root.unmount(); + }); + }); + + it("still reads the shared list as membership on a local_trusted instance", async () => { + // No accounts exist in this mode, so the shared list cannot belong to anyone + // else and stays authoritative. + healthGetMock.mockResolvedValue({ status: "ok", deploymentMode: "local_trusted" }); + getSessionMock.mockResolvedValue(null); + + const root = createRoot(container); + const queryClient = createAppLikeQueryClient(null); + + await act(async () => { + root.render( + + + + } /> + + + , + ); + }); + await flushReact(); + await flushReact(); + await flushReact(); + + expect(container.textContent).toContain("Already in this company"); + expect(acceptInviteMock).not.toHaveBeenCalled(); + + await act(async () => { + root.unmount(); + }); + }); + }); + it("shows invite details instead of auto-redirecting for signed-in existing members", async () => { getSessionMock.mockResolvedValue({ session: { id: "session-1", userId: "user-1" }, diff --git a/ui/src/pages/InviteLanding.tsx b/ui/src/pages/InviteLanding.tsx index abf46f297f..99825026a9 100644 --- a/ui/src/pages/InviteLanding.tsx +++ b/ui/src/pages/InviteLanding.tsx @@ -242,10 +242,21 @@ export function InviteLandingPage() { retry: false, }); + // The company list is keyed by account now (#11488), so a list belonging to + // somebody else cannot be read here at all. The mount-scoped gate below is kept + // as a second line rather than removed with the first: this page turns the list + // into an authorization verdict, and it should not be the place that discovers + // a hole in the keying. `local_trusted` instances have no accounts, so there the + // shared list is the only identity there is and the gate stays open. const companiesQuery = useCompanyListQuery({ enabled: !!sessionQuery.data && !!inviteQuery.data?.companyId, + staleTime: 0, }); - const companyList = companiesQuery.data?.companies ?? []; + const membershipIsAccountScoped = healthQuery.data?.deploymentMode !== "local_trusted"; + const membershipListIsCurrent = membershipIsAccountScoped + ? Boolean(sessionQuery.data) && companiesQuery.isFetchedAfterMount + : true; + const companyList = membershipListIsCurrent ? companiesQuery.data?.companies ?? [] : []; useEffect(() => { if (token) rememberPendingInviteToken(token); @@ -256,18 +267,19 @@ export function InviteLandingPage() { }, [token]); useEffect(() => { + if (!membershipListIsCurrent) return; const list = companiesQuery.data?.companies; if (!list || !inviteQuery.data?.companyId) return; if (list.some((c) => c.id === inviteQuery.data!.companyId)) { clearPendingInviteToken(token); } - }, [companiesQuery.data, inviteQuery.data, token]); + }, [companiesQuery.data, inviteQuery.data, membershipListIsCurrent, token]); const invite = inviteQuery.data; const isCheckingExistingMembership = Boolean(sessionQuery.data) && Boolean(invite?.companyId) && - companiesQuery.isLoading; + !membershipListIsCurrent; const isCurrentMember = Boolean(invite?.companyId) && companyList.some((company) => company.id === invite?.companyId); @@ -369,9 +381,11 @@ export function InviteLandingPage() { await queryClient.invalidateQueries({ queryKey: queryKeys.auth.session }); await queryClient.invalidateQueries({ queryKey: queryKeys.health }); await queryClient.invalidateQueries({ queryKey: queryKeys.access.currentBoardAccess }); - // Keyed to the account that just signed in, and forced past whatever is - // cached for it, so this cannot be answered with the previous account's - // membership. + // Keyed to the account that just signed in — the helper resolves the + // identity past the invalidation above rather than trusting the session + // entry still sitting in the cache — and forced past whatever is cached + // for it. This replaces the hand-rolled cancel-and-refetch this PR + // originally carried, which #11488 made both unnecessary and weaker. const { companies: freshCompanies } = await fetchCompanyListForCurrentAccount(queryClient); if (invite?.companyId && freshCompanies.some((company) => company.id === invite.companyId)) {