Skip to content

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

Open
aristidesstaffieri wants to merge 2 commits into
masterfrom
bug/stale-site-scan
Open

fix(blockaid): stop dropping the site scan verdict on sign transaction#2948
aristidesstaffieri wants to merge 2 commits into
masterfrom
bug/stale-site-scan

Conversation

@aristidesstaffieri

@aristidesstaffieri aristidesstaffieri commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

On the sign-transaction approval screen, a Blockaid is_malicious verdict for the requesting dApp could be discarded before it reached the UI: no malicious-site banner rendered, and Confirm was not marked destructive. useGetSignTxData starts the site scan unawaited and then dispatches a final payload assembled from a stale local object, while the reducer full-replaces data — so whichever write landed last owned the entire payload and dropped the other writer's field. In the opposite resolution order the same defect silently blanked the fetched changeTrust asset icons.

Both contested fields now live in refs shared by the two writers, so neither ordering loses data.

Root cause

firstRenderPayload.siteScanData is undefined at construction and is never mutated, because the scan callback returns a new object rather than writing back into it. The final dispatch read that field, so it wrote undefined unconditionally; when the scan resolved first, its real verdict was overwritten. getSiteSecurityStates treats undefined as "scan in flight" and returns all-clear, which suppresses both the banner and the destructive Confirm styling rather than degrading to "unable to scan".

The reverse ordering lost the icons. The guard intended to prevent that inspected firstRenderPayload.icons, which is hardcoded {} and never mutated, so its condition never held.

Impact is bounded to dApps the user has already allow-listed. renderBanner is priority-ordered and shows the domain-not-allowed banner first, so for every other origin the Blockaid site banner was already not the visible signal.

What's in this PR

  • useGetSignTxData.tsxsiteScanDataRef and iconsRef as the shared source of truth between the unawaited scan callback and the final fetchData dispatch, reset per fetchData.
  • useGetSignTxData.tsx — removes the previous icon-preservation guard, which could never fire.
  • __tests__/useGetSignTxData.test.tsx — two tests pinning both resolution orderings, driven by hand-resolved deferreds. useAsyncSiteScan, useScanSite, the reducer and getSiteSecurityStates stay real; only the two network leaves are stubbed.

Test plan

  • Both new tests fail on the unmodified hook and pass with the fix; their mid-flow assertions confirm each value reached state before being overwritten, so neither is tautological
  • yarn jest --ci --selectProjects jsdom --runTestsByPath extension/src/popup/views/SignTransaction/hooks/__tests__/useGetSignTxData.test.tsx — 6/6
  • yarn jest --ci --selectProjects jsdom --runTestsByPath extension/src/popup/views/__tests__/SignTransaction.test.tsx — 12/12
  • yarn jest --ci --selectProjects jsdom -t "Blockaid" — 77 passed, no test failures
  • yarn tsc --noEmit clean; eslint clean on the changed source
  • CI green
  • Manual: with the dev Blockaid override forcing a malicious site verdict, sign a changeTrust transaction for an issuer absent from the icon cache from an allow-listed dApp. The banner must stay visible for the life of the popup and the asset icon must render — the two failure modes are mutually exclusive on master, so both holding at once confirms the fix.

Followups

  • The approval UI is interactive before the verdict lands. isSubmitDisabled in SignTransaction/index.tsx does not consider scan state, so a fast click can approve before the banner renders. This PR guarantees the verdict is not lost, not that it arrives before the user can act.
  • The reducer full-replaces data, so any future async enrichment added to this hook must extend the same ref bookkeeping or it will reintroduce the clobber. Awaiting the scan promise before the final dispatch — concurrent with the icon fetches, so no added latency in the slow path — would eliminate the class of bug. Deliberately out of scope here.

…ach other

  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.
@aristidesstaffieri aristidesstaffieri self-assigned this Aug 10, 2026
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

PR Preview build is ready: https://github.com/stellar/freighter/releases/tag/untagged-81f1f3e38ddd4e970ee3 (SDF collaborators only — install instructions in the release description)

@aristidesstaffieri aristidesstaffieri changed the title fix(blockaid): stale site scan fix(blockaid): stop dropping the site scan verdict on sign transaction Aug 10, 2026
@aristidesstaffieri
aristidesstaffieri marked this pull request as ready for review August 10, 2026 18:06
Copilot AI balanced review requested due to automatic review settings August 10, 2026 18:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes race conditions that could discard Blockaid site verdicts or transaction asset icons.

Changes:

  • Shares scan verdict and icon state through refs.
  • Resets shared state for each fetch.
  • Adds tests for both async resolution orders.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
useGetSignTxData.tsx Preserves scan verdicts and icons across async dispatches.
useGetSignTxData.test.tsx Tests both race-condition orderings.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@aristidesstaffieri
aristidesstaffieri requested a review from a team August 11, 2026 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants