Fix Transaction Signer verification race condition and repair PortfolioChartWidget - #1411
Merged
emdevelopa merged 2 commits intoAug 28, 2026
Merged
Conversation
Closes emdevelopa#1340 Closes emdevelopa#1341 verifyTransactionSignatureSecure's replay check (Step 2) and replay record (Step 6, inside recordVerificationSuccess) are separated by an async Horizon network call (Step 4, verifyTransactionSignature). Without any concurrency guard, two requests carrying the same txHash that arrive close together both observe "not yet replayed" before either finishes, so both proceed through the full pipeline independently: - Race condition (emdevelopa#1340): replay protection is bypassed for the duration of the race — the whole point of replay detection is "this txHash has already been verified," and for concurrent duplicates that's briefly false for both callers. - Data inconsistency (emdevelopa#1341): two independent pipeline runs for the same key can race each other's cache/replay-cache writes and, in principle, resolve to different outcomes for what must be a single logical result per txHash. Fix: track one in-flight Promise per normalized txHash (`inFlightVerifications`). A second concurrent call for a hash already in flight awaits that same Promise instead of re-entering the pipeline, so every caller sees one consistent result and the core verifier/cache/replay-cache are touched exactly once per hash per verification. Steps 2-6 were extracted into `runVerificationPipeline` so the caller (`verifyTransactionSignatureSecure`) only has to own the dedup bookkeeping and the timer. Added a "Concurrent duplicate requests" test suite: the core verifier is called exactly once for N concurrent identical requests, all callers get the same result object, none of them see a false replay against each other, a later non-concurrent request still runs fresh once the in-flight one resolves, and distinct hashes are deduped independently of each other. `npx vitest run src/lib/transaction-signer-refactored.test.js` — 26/27 passing (5 new; the 1 failure is a pre-existing, unrelated assertion about a raw exception message, confirmed failing identically on a clean `main` checkout via `git stash` before this change). Also ran the sibling transaction-signer*.test.js suites: 88/110 passing, same 22 pre-existing unrelated failures as on clean `main` (missing exports in two other files) — zero regressions from this change.
Closes emdevelopa#1342 Closes emdevelopa#1343 PortfolioChartWidget.tsx and its test file both contained two divergent implementations concatenated together by a bad merge: the component had a second `return (...)` statement, `const containerVariants`/ `itemVariants` declarations sitting mid-JSX (invalid), a duplicated pair of allocation/trend toggle buttons (one plain-`<button>` version using dead `t('portfolioChart.allocation') || 'Allocation'`-style keys, one `motion.button` version using the real `t('allocation')` keys that match this component's actual translation dictionary), and an entire second, non-motion copy of the chart container + asset list appended after the first one's `</div>` with no closing tag reconciliation. The file did not parse. The test file had the same story in miniature: two conflicting `vi.mock('next-intl', ...)` calls, several assertions checking raw untranslated keys (`'portfolioChart.valueTitle'`) that only made sense against the *other* half's translation approach, and an `if (assetElement)` block referencing a variable never declared in that test (`ReferenceError`). Since the component couldn't even render, "fix the responsive layout" (emdevelopa#1342) and "resolve the accessibility violation" (emdevelopa#1343) were unaddressable as filed — there was no single coherent implementation to audit. Resolved by keeping the richer, already-more-accessible half (the motion/AnimatePresence version: loading skeletons, `role="status"` + `aria-live="polite"` loading state, `role="alert"` error state, `aria-pressed`/`disabled` toggle buttons, responsive flex/overflow layout) and removing the orphaned duplicate half entirely, fixing the test file to match (single next-intl mock, translated-string assertions, dropped the broken dead code). On top of restoring the file to a working state, closed a real, separate accessibility gap in the surviving version: asset rows in the list were only mouse-clickable (`onClick` with no `role`, `tabIndex`, or key handler) despite being an interactive selection control. Added `role="button"`, `tabIndex`, `aria-pressed`, an Enter/Space `onKeyDown` handler, and a `focus-visible` outline, matching the keyboard-accessibility pattern already used elsewhere in this component (the toggle buttons). `npx vitest run src/components/PortfolioChartWidget.test.tsx` — 13/13 passing (1 new: keyboard operability of asset rows; also fixed a pre-existing flaky assertion comparing a locale-formatted currency string containing a non-breaking space against a literal, and the `ReferenceError` described above). `npx tsc --noEmit` — no errors in this component.
|
@praizehimm is attempting to deploy a commit to the Emmanuel's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@praizehimm Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#1340 / #1341 — Transaction Signer race condition / data inconsistency
verifyTransactionSignatureSecure's replay check and replay record are separated by an async Horizon network call. Without a concurrency guard, two requests carrying the sametxHasharriving close together both observe "not yet replayed" before either finishes, so both proceed through the full pipeline independently — bypassing replay protection for the duration of the race (#1340) and risking two independent pipeline runs racing each other's cache/replay-cache writes for what must be a single logical result per hash (#1341).Fixed by tracking one in-flight
Promiseper normalizedtxHash. A second concurrent call for a hash already in flight awaits that same promise instead of re-entering the pipeline, so every caller gets one consistent result and the core verifier/cache/replay-cache are touched exactly once per hash per verification window. Extracted the replay/cache/verify/record steps intorunVerificationPipelineso the outer function only owns the dedup bookkeeping.#1342 / #1343 — PortfolioChartWidget
Found something more serious than a layout or a11y tweak:
PortfolioChartWidget.tsx(and its test file) each contained two divergent implementations concatenated together by a bad merge — a secondreturn (...),constdeclarations sitting mid-JSX, a duplicated pair of toggle buttons (one dead, one live), and an entire second non-motion copy of the chart container + asset list with no closing-tag reconciliation. The component did not parse. The test file had matching damage: two conflictingvi.mock('next-intl', ...)calls and aReferenceErrorfrom a variable used but never declared.Since the component couldn't render at all, "fix the responsive layout" and "resolve the accessibility violation" weren't addressable as filed — there was nothing coherent to audit. Resolved by keeping the richer, already-more-accessible half (loading skeletons,
role="status"+aria-live="polite",role="alert"error state,aria-pressed/disabledtoggles, responsive flex/overflow layout) and removing the orphaned duplicate half, then fixing the test file to match.On top of that repair, closed a real, separate accessibility gap in the surviving version: asset rows were mouse-only (
onClickwith norole/tabIndex/key handler) despite being an interactive selection control. Addedrole="button",tabIndex,aria-pressed, an Enter/SpaceonKeyDownhandler, and afocus-visibleoutline, matching the pattern already used by this component's toggle buttons.Test plan
npx vitest run src/lib/transaction-signer-refactored.test.js(backend) — 26/27 passing (5 new concurrency tests); the 1 failure is a pre-existing, unrelated assertion, confirmed identical on a cleanmaincheckout viagit stashtransaction-signer*.test.jssuites — 88/110 passing, same 22 pre-existing unrelated failures as cleanmain, zero regressionsnpx vitest run src/components/PortfolioChartWidget.test.tsx(frontend) — 13/13 passing (1 new: keyboard operability)npx tsc --noEmit— no errors in the touched component