From 4a2e50a54e16188c288df0d1fb4461a7759d0142 Mon Sep 17 00:00:00 2001 From: Aristides Staffieri Date: Mon, 10 Aug 2026 10:40:27 -0600 Subject: [PATCH] fix(blockaid): stop the sign-tx site scan and icon fetch clobbering each other MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The site scan runs unawaited while fetchData keeps awaiting changeTrust icon lookups. Both write to the same reducer slot, and helpers/request.ts full-replaces `data`, so the later dispatch owned the whole payload and dropped the other writer's field. The final dispatch read `firstRenderPayload.siteScanData`, which is `undefined` at construction and never mutated — the scan callback returns a new object rather than writing back — so it always wrote `undefined`. When the scan landed first, a real is_malicious verdict was replaced by `undefined`, which getSiteSecurityStates reads as "scan in flight": no banner, and btnIsDestructive loses isSiteMalicious. For an already-allow-listed dApp the site banner is the only site-level scam signal, so the warning was suppressed entirely. The reverse ordering lost the icons. The existing guard meant to prevent that inspected firstRenderPayload.icons, hardcoded {} and never mutated, so its condition never held. Use refs as the shared source of truth for both contested fields, reset per fetchData. Reading reducer state here would not work — fetchData closes over a stale render's copy. --- .../hooks/__tests__/useGetSignTxData.test.tsx | 189 +++++++++++++++++- .../hooks/useGetSignTxData.tsx | 39 ++-- 2 files changed, 213 insertions(+), 15 deletions(-) diff --git a/extension/src/popup/views/SignTransaction/hooks/__tests__/useGetSignTxData.test.tsx b/extension/src/popup/views/SignTransaction/hooks/__tests__/useGetSignTxData.test.tsx index c485287f28..b2695781bd 100644 --- a/extension/src/popup/views/SignTransaction/hooks/__tests__/useGetSignTxData.test.tsx +++ b/extension/src/popup/views/SignTransaction/hooks/__tests__/useGetSignTxData.test.tsx @@ -8,10 +8,15 @@ import { TEST_CANONICAL, TEST_PUBLIC_KEY, } from "popup/__testHelpers__"; -import { TESTNET_NETWORK_DETAILS } from "@shared/constants/stellar"; +import { + MAINNET_NETWORK_DETAILS, + TESTNET_NETWORK_DETAILS, +} from "@shared/constants/stellar"; import * as AccountHelpers from "popup/helpers/account"; import * as BlockaidHelpers from "popup/helpers/blockaid"; -import { useGetSignTxData } from "../useGetSignTxData"; +import * as FetchHelpers from "popup/helpers/fetch"; +import { getSiteSecurityStates } from "popup/helpers/blockaid"; +import { ResolvedData, useGetSignTxData } from "../useGetSignTxData"; import * as GetAppDataHooks from "helpers/hooks/useGetAppData"; import * as GetBalancesHooks from "helpers/hooks/useGetBalances"; import { AppDataType } from "helpers/hooks/useGetAppData"; @@ -353,3 +358,183 @@ describe("useGetSignTxData", () => { ); }); }); + +/** + * The site scan is kicked off unawaited (useGetSignTxData `scanSite(...)`) while + * `fetchData` keeps awaiting the changeTrust icon lookup. Both writers land in + * the same reducer slot, and `helpers/request.ts` full-replaces `data`, so + * whichever dispatch runs last owns the whole payload. These two tests pin both + * resolution orderings: neither writer may drop the other's field. + */ +describe("useGetSignTxData site scan / icon fetch interleaving", () => { + // A changeTrust op for RUBTC whose issuer is absent from the store icon cache + // below, so the awaited getIconUrlFromIssuer path is forced. + const changeTrustTx = + "AAAAAgAAAABngBTmbmUycqG2cAMHcomSR80dRzGtKzxM6gb3yySD5AAAAGQCjnUGAAABUQAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAABgAAAAJSVUJUQwAAAAAAAAAAAAAAF7DK9H3uJ/qYfQakv93qidEVa/Hh7mAXrDl2fbEgVQh//////////wAAAAAAAAAA"; + const DOMAIN = "https://malicious.example.com"; + const ICON_URL = "https://icon.example/icon.png"; + const MALICIOUS_SITE = { status: "hit", is_malicious: true }; + + /** A promise the test resolves by hand, to control resolution order. */ + const deferred = () => { + let resolve!: (value: T) => void; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; + }; + + // Drains microtasks plus one macrotask turn, so the unawaited scan chain has + // run as far as its next unresolved await. + const flush = () => new Promise((r) => setTimeout(r, 0)); + + const preloadedState = { + auth: { publicKey: TEST_PUBLIC_KEY }, + cache: { + balanceData: {}, + icons: {}, + tokenLists: [], + homeDomains: {}, + }, + settings: { + assetsLists: [], + networkDetails: MAINNET_NETWORK_DETAILS, + }, + }; + + const Wrapper = + (store: ReturnType) => + ({ children }: { children: React.ReactNode }) => ( + {children} + ); + + const renderSignTxData = () => + renderHook( + () => + useGetSignTxData( + { xdr: changeTrustTx, url: DOMAIN }, + { showHidden: false, includeIcons: false }, + "G123", + DOMAIN, + ), + { wrapper: Wrapper(makeDummyStore(preloadedState)) }, + ); + + const resolvedData = (result: { current: { state: { data: unknown } } }) => + result.current.state.data as ResolvedData; + + beforeEach(() => { + // Mainnet, so isBlockaidEnabled passes and the real site scan runs. + jest.spyOn(GetAppDataHooks, "useGetAppData").mockReturnValue({ + fetchData: () => + Promise.resolve({ + type: AppDataType.RESOLVED, + account: { + publicKey: TEST_PUBLIC_KEY, + allAccounts: mockAccounts, + }, + settings: { networkDetails: MAINNET_NETWORK_DETAILS }, + }), + } as any); + // Benign transaction scan, so the site scan is the only security signal. + jest.spyOn(BlockaidHelpers, "useScanTx").mockReturnValue({ + scanTx: () => + Promise.resolve({ + simulation: null, + validation: null, + request_id: "1", + }), + } as any); + }); + + /** + * Sets up the two racing network leaves. `useAsyncSiteScan`, `useScanSite`, + * the updatePayload closure, the reducer and getSiteSecurityStates all stay + * real — only the fetches at the edges are stubbed. + */ + const stubRacingFetches = () => { + const siteScan = deferred<{ data: unknown; error: null }>(); + const icon = deferred(); + + jest + .spyOn(FetchHelpers, "fetchJson") + .mockImplementation((url: string) => + url.includes("/scan-dapp") + ? (siteScan.promise as any) + : Promise.reject(new Error(`unexpected fetchJson: ${url}`)), + ); + jest + .spyOn(GetIconUrlFromIssuerHelpers, "getIconUrlFromIssuer") + .mockReturnValue(icon.promise as any); + + return { siteScan, icon }; + }; + + it("keeps a malicious site verdict when the scan resolves before the changeTrust icon fetch", async () => { + const { siteScan, icon } = stubRacingFetches(); + const { result } = renderSignTxData(); + + // Advance until fetchData is suspended on the icon fetch. + let fetchDataPromise: Promise; + await act(async () => { + fetchDataPromise = result.current.fetchData(); + await flush(); + }); + expect(resolvedData(result).siteScanData).toBeUndefined(); + + // The scan lands first with the malicious verdict. + await act(async () => { + siteScan.resolve({ data: MALICIOUS_SITE, error: null }); + await flush(); + }); + expect(resolvedData(result).siteScanData).toEqual(MALICIOUS_SITE); + + // The slow icon fetch lands last and fetchData dispatches its final payload. + await act(async () => { + icon.resolve(ICON_URL); + await flush(); + await fetchDataPromise; + }); + + const siteScanData = resolvedData(result).siteScanData; + expect(siteScanData).toEqual(MALICIOUS_SITE); + + // The exact gate SignTransaction/index.tsx uses to raise the banner and mark + // Confirm destructive. + const states = getSiteSecurityStates( + siteScanData, + null, + MAINNET_NETWORK_DETAILS, + ); + expect(states.isMalicious).toBe(true); + }); + + it("keeps the fetched changeTrust icons when the scan resolves after the icon fetch", async () => { + const { siteScan, icon } = stubRacingFetches(); + const { result } = renderSignTxData(); + + let fetchDataPromise: Promise; + await act(async () => { + fetchDataPromise = result.current.fetchData(); + await flush(); + }); + + // Reversed ordering: the icon fetch lands first, so fetchData runs to + // completion and dispatches the icons... + await act(async () => { + icon.resolve(ICON_URL); + await flush(); + await fetchDataPromise; + }); + expect(Object.values(resolvedData(result).icons)).toEqual([ICON_URL]); + + // ...then the scan dispatch lands last and must not drop them. + await act(async () => { + siteScan.resolve({ data: MALICIOUS_SITE, error: null }); + await flush(); + }); + + expect(Object.values(resolvedData(result).icons)).toEqual([ICON_URL]); + expect(resolvedData(result).siteScanData).toEqual(MALICIOUS_SITE); + }); +}); diff --git a/extension/src/popup/views/SignTransaction/hooks/useGetSignTxData.tsx b/extension/src/popup/views/SignTransaction/hooks/useGetSignTxData.tsx index f07c9edb13..6500a64b65 100644 --- a/extension/src/popup/views/SignTransaction/hooks/useGetSignTxData.tsx +++ b/extension/src/popup/views/SignTransaction/hooks/useGetSignTxData.tsx @@ -1,4 +1,4 @@ -import { useReducer, useState } from "react"; +import { useReducer, useRef, useState } from "react"; import { Account, @@ -78,25 +78,30 @@ function useGetSignTxData( const { assetsLists } = useSelector(settingsSelector); const { scanTx } = useScanTx(); const blockaidOverrideState = useBlockaidOverrideState() ?? null; + // The site scan runs unawaited while `fetchData` keeps awaiting icon lookups, + // so both write to the same reducer slot in an order we do not control. The + // reducer full-replaces `data` (see `helpers/request.ts`), which means the + // later dispatch owns the entire payload. These refs are the source of truth + // for the two fields each writer would otherwise drop: reading `state` here + // would not work, since `fetchData` closes over a stale render's copy. + const siteScanDataRef = useRef( + undefined, + ); + const iconsRef = useRef({} as AssetIcons); const { scanSite } = useAsyncSiteScan( domain, dispatch, (payload, scanData) => { // Type guard to ensure we're working with ResolvedData if (payload.type === AppDataType.RESOLVED) { - const resolvedPayload = payload as ResolvedData; - const updated = { - ...resolvedPayload, + siteScanDataRef.current = scanData; + return { + ...(payload as ResolvedData), siteScanData: scanData, + // Carry whatever icons have been fetched so far, so a scan that + // resolves after the final dispatch does not blank them out. + icons: iconsRef.current, } as ResolvedData; - // Preserve icons if they've been fetched - if ( - resolvedPayload.icons && - Object.keys(resolvedPayload.icons).length > 0 - ) { - updated.icons = resolvedPayload.icons; - } - return updated; } return payload; }, @@ -105,6 +110,10 @@ function useGetSignTxData( const fetchData = async (newPublicKey?: string) => { dispatch({ type: "FETCH_DATA_START" }); + // Clear both slots so a refetch never inherits the previous run's verdict + // or icons. + siteScanDataRef.current = undefined; + iconsRef.current = {} as AssetIcons; try { if (newPublicKey) { await reduxDispatch(makeAccountActive(newPublicKey)); @@ -297,11 +306,15 @@ function useGetSignTxData( } } + iconsRef.current = icons; const payload = { type: AppDataType.RESOLVED, balances: balancesResult, scanResult, - siteScanData: firstRenderPayload.siteScanData, + // Read the ref, not `firstRenderPayload` — the scan callback returns a + // new object rather than mutating it, so that field is always the + // initial `undefined` and would blank out a verdict already dispatched. + siteScanData: siteScanDataRef.current, blockaidOverrideState: firstRenderPayload.blockaidOverrideState, publicKey, applicationState: appData.account.applicationState,