fix(blockaid): stop dropping the site scan verdict on sign transaction

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 = <T,>() => {
+    let resolve!: (value: T) => void;
+    const promise = new Promise<T>((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<typeof makeDummyStore>) =>
+    ({ children }: { children: React.ReactNode }) => (
+      <Provider store={store}>{children}</Provider>
+    );
+
+  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<string>();
+
+    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<unknown>;
+    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<unknown>;
+    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);
+  });
+});

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<BlockAidScanSiteResult | null | undefined>(
+    undefined,
+  );
+  const iconsRef = useRef<AssetIcons>({} as AssetIcons);
   const { scanSite } = useAsyncSiteScan<SignTxData>(
     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,