From 70f7d9811f34dfeffab5468c9174ef0dcfdced12 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 06:16:25 +0000 Subject: [PATCH] fix(wallets): stop getUserWallets listing anyone's wallets without a session wallet.getUserWallets skipped its address check when the caller had no session at all, so an anonymous request for any signer address (public on-chain) returned the full wallet rows that name it: names, descriptions, signer labels and import bodies. getWallet already refuses anonymous callers; the list now does too (401 without a session, 403 for an address the session does not hold). The client relied on the anonymous read to show the list before the user signed the session nonce. useUserWallets now waits for authorization and reports needsAuthorization, and /wallets shows an "Authorize wallet" prompt (reopening the auth modal) instead of claiming the user has no wallets. The two wallet-creation lists on that page are protectedProcedures and are gated the same way, with 401 no longer retried. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017rkSXEzcv4V5UecnbAuNvd --- src/__tests__/trpc/userWalletsAuth.test.ts | 115 ++++++++++++++++++ .../pages/homepage/wallets/index.tsx | 53 ++++++-- src/hooks/useUserWallets.ts | 60 ++++++--- src/server/api/routers/wallets.ts | 16 ++- 4 files changed, 214 insertions(+), 30 deletions(-) create mode 100644 src/__tests__/trpc/userWalletsAuth.test.ts diff --git a/src/__tests__/trpc/userWalletsAuth.test.ts b/src/__tests__/trpc/userWalletsAuth.test.ts new file mode 100644 index 00000000..7e79f2ec --- /dev/null +++ b/src/__tests__/trpc/userWalletsAuth.test.ts @@ -0,0 +1,115 @@ +import { beforeAll, beforeEach, describe, expect, it, jest } from "@jest/globals"; + +import { makeAnonymousCtx, makeSessionCtx, makeWalletCtx } from "./helpers"; + +jest.mock("superjson", () => ({ + __esModule: true, + default: { + serialize: (value: unknown) => value, + deserialize: (value: unknown) => value, + }, +})); + +jest.mock("@/server/auth", () => ({ + __esModule: true, + getServerAuthSession: jest.fn(), +})); + +let createCaller: typeof import("@/server/api/root").createCaller; + +const SIGNER = "addr_test1signer"; +const OTHER = "addr_test1other"; + +const walletRow = { + id: "wallet-1", + name: "Treasury", + description: "Board treasury", + signersAddresses: [SIGNER, OTHER], + signersDescriptions: ["Alice", "Bob"], +}; + +const makeMockDb = () => ({ + wallet: { + findMany: jest.fn(async (_args: unknown) => [walletRow]), + }, +}); + +/** + * wallet.getUserWallets lists every wallet that names an address as a signer. + * Signer addresses are public on-chain, so the address alone proves nothing: + * the caller must hold a session for it. + */ +describe("wallet.getUserWallets authorization", () => { + beforeAll(async () => { + ({ createCaller } = await import("@/server/api/root")); + }); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it("refuses an anonymous caller without touching the database", async () => { + const db = makeMockDb(); + const caller = createCaller(makeAnonymousCtx(db) as any); + + await expect( + caller.wallet.getUserWallets({ address: SIGNER }), + ).rejects.toMatchObject({ code: "UNAUTHORIZED" }); + + expect(db.wallet.findMany).not.toHaveBeenCalled(); + }); + + it("refuses a wallet session asking for someone else's address", async () => { + const db = makeMockDb(); + const caller = createCaller(makeWalletCtx(OTHER, db) as any); + + await expect( + caller.wallet.getUserWallets({ address: SIGNER }), + ).rejects.toMatchObject({ code: "FORBIDDEN" }); + + expect(db.wallet.findMany).not.toHaveBeenCalled(); + }); + + it("refuses a NextAuth session asking for someone else's address", async () => { + const db = makeMockDb(); + const caller = createCaller(makeSessionCtx(OTHER, db) as any); + + await expect( + caller.wallet.getUserWallets({ address: SIGNER }), + ).rejects.toMatchObject({ code: "FORBIDDEN" }); + + expect(db.wallet.findMany).not.toHaveBeenCalled(); + }); + + it("lists the wallets of the address the wallet session holds", async () => { + const db = makeMockDb(); + const caller = createCaller(makeWalletCtx(SIGNER, db) as any); + + await expect( + caller.wallet.getUserWallets({ address: SIGNER }), + ).resolves.toEqual([walletRow]); + + expect(db.wallet.findMany).toHaveBeenCalledWith({ + where: { signersAddresses: { has: SIGNER } }, + }); + }); + + it("lists the wallets for any address in a multi-wallet session", async () => { + const db = makeMockDb(); + const ctx = { ...makeWalletCtx(OTHER, db), sessionWallets: [OTHER, SIGNER] }; + const caller = createCaller(ctx as any); + + await expect( + caller.wallet.getUserWallets({ address: SIGNER }), + ).resolves.toEqual([walletRow]); + }); + + it("lists the wallets of a NextAuth user's own address", async () => { + const db = makeMockDb(); + const caller = createCaller(makeSessionCtx(SIGNER, db) as any); + + await expect( + caller.wallet.getUserWallets({ address: SIGNER }), + ).resolves.toEqual([walletRow]); + }); +}); diff --git a/src/components/pages/homepage/wallets/index.tsx b/src/components/pages/homepage/wallets/index.tsx index 0be9ab03..1a50051f 100644 --- a/src/components/pages/homepage/wallets/index.tsx +++ b/src/components/pages/homepage/wallets/index.tsx @@ -29,8 +29,32 @@ import { ImportTransferDialog } from "./import-transfer-dialog"; import BotManagementCard from "@/components/pages/user/BotManagementCard"; +/** + * Shown to a connected wallet that has not signed the session nonce yet. The + * wallet list is only served to a session for the address, so without this + * the page would claim the user has no wallets at all. + */ +function AuthorizeToSeeWallets() { + const requestReauth = useUserStore((state) => state.requestReauth); + return ( +
+

+ Authorize your connected wallet to see the multisig wallets it signs + for. +

+ +
+ ); +} + export default function PageWallets() { - const { wallets, isLoading: isLoadingWallets } = useUserWallets(); + const { + wallets, + isLoading: isLoadingWallets, + needsAuthorization, + } = useUserWallets(); const [showArchived, setShowArchived] = useState(false); const userAddress = useUserStore((state) => state.userAddress); const address = userAddress ?? ""; @@ -43,18 +67,17 @@ export default function PageWallets() { refetchOnWindowFocus: false, }, ); + // Both wallet-creation lists are protectedProcedures: without a session for + // this address they can only answer 401/403. const isAuthorized = walletSession?.authorized ?? false; - const sessionWallets = walletSession?.wallets ?? []; - // If there's no wallet-session constraint, public read queries can run without authorization - const canQueryPublicWalletData = sessionWallets.length === 0 || isAuthorized; const { data: newPendingWallets, isLoading: isLoadingNewWallets } = api.wallet.getUserNewWallets.useQuery( { address }, { // Only enable query when user is authorized (prevents 403 errors) - enabled: address.length > 0 && canQueryPublicWalletData, + enabled: address.length > 0 && isAuthorized, retry: (failureCount, error) => { - // Don't retry on authorization errors (403) + // Don't retry on authorization errors (401/403) if (error && typeof error === "object") { const err = error as { code?: string; @@ -65,8 +88,11 @@ export default function PageWallets() { const errorMessage = err.message || err.shape?.message || ""; const isAuthError = err.code === "FORBIDDEN" || + err.code === "UNAUTHORIZED" || err.data?.code === "FORBIDDEN" || + err.data?.code === "UNAUTHORIZED" || err.data?.httpStatus === 403 || + err.data?.httpStatus === 401 || err.shape?.code === "FORBIDDEN" || errorMessage.includes("Address mismatch"); if (isAuthError) return false; @@ -81,9 +107,9 @@ export default function PageWallets() { { address }, { // Only enable query when user is authorized (prevents 403 errors) - enabled: address.length > 0 && canQueryPublicWalletData, + enabled: address.length > 0 && isAuthorized, retry: (failureCount, error) => { - // Don't retry on authorization errors (403) + // Don't retry on authorization errors (401/403) if (error && typeof error === "object") { const err = error as { code?: string; @@ -94,8 +120,11 @@ export default function PageWallets() { const errorMessage = err.message || err.shape?.message || ""; const isAuthError = err.code === "FORBIDDEN" || + err.code === "UNAUTHORIZED" || err.data?.code === "FORBIDDEN" || + err.data?.code === "UNAUTHORIZED" || err.data?.httpStatus === 403 || + err.data?.httpStatus === 401 || err.shape?.code === "FORBIDDEN" || errorMessage.includes("Address mismatch"); if (isAuthError) return false; @@ -147,9 +176,13 @@ export default function PageWallets() { )} - {!isLoadingWallets && address.length > 0 && (!wallets || wallets.length === 0) && ( - + {!isLoadingWallets && needsAuthorization && ( + )} + {!isLoadingWallets && + !needsAuthorization && + address.length > 0 && + (!wallets || wallets.length === 0) && } {!isLoadingWallets && wallets && wallets .filter((wallet) => showArchived || !wallet.isArchived) diff --git a/src/hooks/useUserWallets.ts b/src/hooks/useUserWallets.ts index adde9bc1..e9e01271 100644 --- a/src/hooks/useUserWallets.ts +++ b/src/hooks/useUserWallets.ts @@ -11,27 +11,32 @@ export default function useUserWallets() { const address = userAddress ?? ""; // Check wallet session authorization before enabling queries - const { data: walletSession } = api.auth.getWalletSession.useQuery( - { address }, - { - enabled: address.length > 0, - refetchOnWindowFocus: false, - }, - ); + const { data: walletSession, isError: sessionCheckFailed } = + api.auth.getWalletSession.useQuery( + { address }, + { + enabled: address.length > 0, + refetchOnWindowFocus: false, + }, + ); + // The wallet list is only served to a caller with a session for `address` + // (the server answers 401/403 otherwise), so wait for the authorization + // step instead of firing a request that is bound to fail. The layout + // refetches the session after authorizing, which flips this on. const isAuthorized = walletSession?.authorized ?? false; - const sessionWallets = walletSession?.wallets ?? []; - // If there's no wallet-session constraint, public read queries can run without authorization - const canQueryPublicWalletData = sessionWallets.length === 0 || isAuthorized; - - const { data: wallets, isLoading } = api.wallet.getUserWallets.useQuery( + + const { + data: wallets, + isLoading, + error: walletsError, + } = api.wallet.getUserWallets.useQuery( { address }, { - // Only enable query when user is authorized (prevents 403 errors) - enabled: address.length > 0 && canQueryPublicWalletData, + enabled: address.length > 0 && isAuthorized, staleTime: 1 * 60 * 1000, // 1 minute (user/wallet data) gcTime: 5 * 60 * 1000, // 5 minutes retry: (failureCount, error) => { - // Don't retry on authorization errors (403) + // Don't retry on authorization errors (401/403) if (error && typeof error === "object") { const err = error as { code?: string; @@ -42,8 +47,11 @@ export default function useUserWallets() { const errorMessage = err.message || err.shape?.message || ""; const isAuthError = err.code === "FORBIDDEN" || + err.code === "UNAUTHORIZED" || err.data?.code === "FORBIDDEN" || + err.data?.code === "UNAUTHORIZED" || err.data?.httpStatus === 403 || + err.data?.httpStatus === 401 || err.shape?.code === "FORBIDDEN" || errorMessage.includes("Address mismatch"); if (isAuthError) return false; @@ -61,5 +69,25 @@ export default function useUserWallets() { .map((wallet) => buildWallet(wallet, network)); }, [wallets, network]); - return { wallets: mappedWallets, isLoading }; + // Until the session question is answered the list is effectively loading; + // once it is answered "no", callers should ask the user to authorize rather + // than render "you have no wallets". + // A failed session check is not a "no": the auth modal only opens on an + // answer, so prompting then would offer a button that does nothing. A list + // request refused with 401 is a "no" (the cookie expired since the session + // check was cached). + const isCheckingSession = + address.length > 0 && walletSession === undefined && !sessionCheckFailed; + const listRefused = + walletsError?.data?.code === "UNAUTHORIZED" || + walletsError?.data?.httpStatus === 401; + const needsAuthorization = + address.length > 0 && + ((walletSession !== undefined && !isAuthorized) || listRefused); + + return { + wallets: mappedWallets, + isLoading: isLoading || isCheckingSession, + needsAuthorization, + }; } diff --git a/src/server/api/routers/wallets.ts b/src/server/api/routers/wallets.ts index d3714ae0..ca892148 100644 --- a/src/server/api/routers/wallets.ts +++ b/src/server/api/routers/wallets.ts @@ -223,7 +223,14 @@ function walletDataFromPayload( } export const walletRouter = createTRPCRouter({ - // Read operations stay public but validate signer membership by address param + // Lists every wallet that names `address` as a signer, so the caller must + // have proved control of that address (wallet session or NextAuth user). + // + // This used to skip the check when there was no session at all, which let + // anyone look up any address — signer addresses are public on-chain — and + // read back full wallet rows: names, descriptions, the signers' own labels + // and import bodies. getWallet already refuses anonymous callers; the list + // must not hand out more than the single-wallet read does. getUserWallets: publicProcedure .input(z.object({ address: z.string() })) .query(async ({ ctx, input }) => { @@ -233,9 +240,10 @@ export const walletRouter = createTRPCRouter({ : ctx.sessionAddress ? [ctx.sessionAddress] : []; - // If user has an active session, validate that the requested address is authorized - // Throw error if address doesn't match (security: prevent unauthorized access) - if (addresses.length > 0 && !addresses.includes(input.address)) { + if (addresses.length === 0) { + throw new TRPCError({ code: "UNAUTHORIZED" }); + } + if (!addresses.includes(input.address)) { throw new TRPCError({ code: "FORBIDDEN", message: "Address mismatch" }); } // Query wallets where the user is a signer