From d21a51964dfc8a72b024b42d110fa94c972e0f2e Mon Sep 17 00:00:00 2001 From: praizehimm Date: Fri, 28 Aug 2026 22:02:33 +0100 Subject: [PATCH 1/2] fix: dedupe concurrent verifications of the same txHash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1340 Closes #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 (#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 (#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. --- .../lib/transaction-signer-refactored.test.js | 66 ++++++++ backend/src/lib/transaction-signer.js | 151 +++++++++++------- 2 files changed, 163 insertions(+), 54 deletions(-) diff --git a/backend/src/lib/transaction-signer-refactored.test.js b/backend/src/lib/transaction-signer-refactored.test.js index bb184a8d..1c0a6156 100644 --- a/backend/src/lib/transaction-signer-refactored.test.js +++ b/backend/src/lib/transaction-signer-refactored.test.js @@ -200,6 +200,72 @@ describe("Transaction Signer — Refactored Module (Issue #1077)", () => { }); }); + // ── Concurrency (Issues #1340, #1341) ─────────────────────────────────────── + + describe("Concurrent duplicate requests", () => { + it("dedupes concurrent verifications of the same txHash into a single core-verifier call", async () => { + // Before the fix: the replay check and replay record are separated by + // an async Horizon call, so two concurrent requests for the same hash + // would both pass the "not yet replayed" check and both invoke the + // core verifier independently. + const results = await Promise.all([ + verifyTransactionSignatureSecure(VALID_TX_HASH), + verifyTransactionSignatureSecure(VALID_TX_HASH), + verifyTransactionSignatureSecure(VALID_TX_HASH), + ]); + + for (const res of results) { + expect(res.valid).toBe(true); + } + expect(mockVerifyTransactionSignature).toHaveBeenCalledTimes(1); + }); + + it("returns a consistent result to every concurrent caller, not divergent ones", async () => { + const results = await Promise.all([ + verifyTransactionSignatureSecure(VALID_TX_HASH), + verifyTransactionSignatureSecure(VALID_TX_HASH), + ]); + + expect(results[0]).toBe(results[1]); + }); + + it("does not report a false replay between two concurrent requests for the same hash", async () => { + // Neither concurrent call should see "replay: txHash was already + // verified" — that would only be correct for a *second, later* request + // after the first has actually completed and recorded the hash. + const results = await Promise.all([ + verifyTransactionSignatureSecure(VALID_TX_HASH), + verifyTransactionSignatureSecure(VALID_TX_HASH), + ]); + + for (const res of results) { + expect(res.replay).toBeFalsy(); + } + }); + + it("still runs a fresh verification for a later, non-concurrent request after the in-flight one resolves", async () => { + await verifyTransactionSignatureSecure(VALID_TX_HASH); + expect(mockVerifyTransactionSignature).toHaveBeenCalledTimes(1); + + clearReplayCache(); + resetTransactionSignerCacheForTest(); + + await verifyTransactionSignatureSecure(VALID_TX_HASH); + expect(mockVerifyTransactionSignature).toHaveBeenCalledTimes(2); + }); + + it("dedupes concurrent requests independently per distinct txHash", async () => { + const otherHash = "b".repeat(64); + + await Promise.all([ + verifyTransactionSignatureSecure(VALID_TX_HASH), + verifyTransactionSignatureSecure(otherHash), + ]); + + expect(mockVerifyTransactionSignature).toHaveBeenCalledTimes(2); + }); + }); + // ── Error handling ────────────────────────────────────────────────────────── describe("Error handling", () => { diff --git a/backend/src/lib/transaction-signer.js b/backend/src/lib/transaction-signer.js index e0fcf6e4..61c4926a 100644 --- a/backend/src/lib/transaction-signer.js +++ b/backend/src/lib/transaction-signer.js @@ -249,6 +249,23 @@ export class DistributedReplayCache { const replayCache = new ReplayCache(); +/** + * In-flight verification promises keyed by normalized txHash. + * + * Race condition / data inconsistency fix (#1340, #1341): the replay check + * (Step 2 below) and the replay *record* (Step 6, `recordVerificationSuccess`) + * are separated by an async Horizon network call (Step 4). Without this map, + * two concurrent requests carrying the same txHash both observe "not yet + * replayed" before either finishes, so both proceed to verify independently + * — defeating replay protection for the duration of the race, and risking + * two divergent results (e.g. one cache write racing another, or a caller + * treating the transaction as verified twice) for what must be a single + * logical outcome per txHash. Concurrent duplicate requests now await the + * one in-progress verification instead of each running the full pipeline. + * @type {Map>} + */ +const inFlightVerifications = new Map(); + /** * Module-level distributed replay cache. Starts without Redis; call * `initDistributedReplayCache(redisClient)` from app startup to enable @@ -384,70 +401,96 @@ export async function verifyTransactionSignatureSecure(txHash, options = {}) { const normalizedHash = txHash.toLowerCase(); - // ── Step 2: Replay detection ─────────────────────────────────────────────── - // Check local cache first (O(1), no I/O), then Redis (VULN-06 fix: cross- - // instance replay protection in horizontally scaled deployments). - replayCache.prune(); - const localReplay = replayCache.has(normalizedHash); - const distributedReplay = localReplay ? false : await distributedReplayCache.has(normalizedHash); - - if (localReplay || distributedReplay) { - txSignatureReplayAttempts.inc(); - txSignatureVerificationErrors.inc({ error_type: "replay_attempt" }); - logger.warn( - { txHash: normalizedHash, source: localReplay ? "local" : "distributed" }, - "TransactionSigner: replay attempt detected — txHash already verified", - ); - return { valid: false, reason: "replay: txHash was already verified", replay: true }; - } - - // ── Step 3: Verification cache lookup ────────────────────────────────────── - const cache = getTransactionSignerCache(); - const cached = await cache.get(normalizedHash); - if (cached.hit) { - // NPE-10: cached.result can theoretically be null if the cache entry was - // evicted or corrupted between the hit flag being set and the result being - // read (e.g. NPE-08 guard rejecting a malformed Redis payload). Fall - // through to a fresh verification rather than returning null to callers. - if (cached.result == null) { - logger.warn( - { txHash: normalizedHash }, - "TransactionSigner: cache hit but result is null — falling through to fresh verification", - ); - } else { - txSignatureVerificationTotal.inc({ outcome: cached.result?.valid ? "valid" : "invalid" }); - logger.debug( - { txHash: normalizedHash, cached: true }, - "TransactionSigner: returning cached verification result", - ); - return cached.result; - } + // A verification for this exact hash is already in flight — await its + // result instead of racing it (see `inFlightVerifications` above). + const existing = inFlightVerifications.get(normalizedHash); + if (existing) { + return await existing; } - // ── Step 4: Core cryptographic verification ──────────────────────────────── - let result; + const verificationPromise = runVerificationPipeline(normalizedHash, options); + inFlightVerifications.set(normalizedHash, verificationPromise); try { - result = await verifyTransactionSignature(normalizedHash, options); - } catch (err) { - return recordVerificationException(normalizedHash, err); + return await verificationPromise; + } finally { + inFlightVerifications.delete(normalizedHash); } + } finally { + timerEnd(); + } +} - const finalResult = result ?? { valid: false, reason: "verifier returned no result" }; - - // ── Step 5: Cache the result ─────────────────────────────────────────────── - await cache.set(normalizedHash, finalResult, !!finalResult.valid); +/** + * Steps 2–6 of the verification pipeline (replay detection through result + * caching/metrics), for a single normalized txHash. Only ever invoked once + * per in-flight txHash — see `inFlightVerifications` in the caller. + * + * @param {string} normalizedHash + * @param {object} options + * @returns {Promise<{ valid: boolean, reason?: string, replay?: boolean, [key: string]: unknown }>} + */ +async function runVerificationPipeline(normalizedHash, options) { + // ── Step 2: Replay detection ─────────────────────────────────────────────── + // Check local cache first (O(1), no I/O), then Redis (VULN-06 fix: cross- + // instance replay protection in horizontally scaled deployments). + replayCache.prune(); + const localReplay = replayCache.has(normalizedHash); + const distributedReplay = localReplay ? false : await distributedReplayCache.has(normalizedHash); + + if (localReplay || distributedReplay) { + txSignatureReplayAttempts.inc(); + txSignatureVerificationErrors.inc({ error_type: "replay_attempt" }); + logger.warn( + { txHash: normalizedHash, source: localReplay ? "local" : "distributed" }, + "TransactionSigner: replay attempt detected — txHash already verified", + ); + return { valid: false, reason: "replay: txHash was already verified", replay: true }; + } - // ── Step 6: Metrics and logging ──────────────────────────────────────────── - if (finalResult.valid) { - recordVerificationSuccess(normalizedHash, finalResult); + // ── Step 3: Verification cache lookup ────────────────────────────────────── + const cache = getTransactionSignerCache(); + const cached = await cache.get(normalizedHash); + if (cached.hit) { + // NPE-10: cached.result can theoretically be null if the cache entry was + // evicted or corrupted between the hit flag being set and the result being + // read (e.g. NPE-08 guard rejecting a malformed Redis payload). Fall + // through to a fresh verification rather than returning null to callers. + if (cached.result == null) { + logger.warn( + { txHash: normalizedHash }, + "TransactionSigner: cache hit but result is null — falling through to fresh verification", + ); } else { - recordVerificationFailure(normalizedHash, finalResult); + txSignatureVerificationTotal.inc({ outcome: cached.result?.valid ? "valid" : "invalid" }); + logger.debug( + { txHash: normalizedHash, cached: true }, + "TransactionSigner: returning cached verification result", + ); + return cached.result; } + } - return finalResult; - } finally { - timerEnd(); + // ── Step 4: Core cryptographic verification ──────────────────────────────── + let result; + try { + result = await verifyTransactionSignature(normalizedHash, options); + } catch (err) { + return recordVerificationException(normalizedHash, err); } + + const finalResult = result ?? { valid: false, reason: "verifier returned no result" }; + + // ── Step 5: Cache the result ─────────────────────────────────────────────── + await cache.set(normalizedHash, finalResult, !!finalResult.valid); + + // ── Step 6: Metrics and logging ──────────────────────────────────────────── + if (finalResult.valid) { + recordVerificationSuccess(normalizedHash, finalResult); + } else { + recordVerificationFailure(normalizedHash, finalResult); + } + + return finalResult; } // ── Replay Cache Exports (Testing) ─────────────────────────────────────────── From fcf3220e248feb456db88e62593013ea27dc61c9 Mon Sep 17 00:00:00 2001 From: praizehimm Date: Fri, 28 Aug 2026 22:02:52 +0100 Subject: [PATCH 2/2] fix: repair merged-duplicate PortfolioChartWidget and fix keyboard a11y MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1342 Closes #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-` - - - - {/* Chart Container */} -
-
- {chartType === 'pie' ? ( - - - handleAssetClick(entry.payload.payload)} - > - {assetsWithColors.map((asset) => ( - - ))} - - formatCurrency(value as number)} - contentStyle={{ - backgroundColor: '#1F2937', - border: '1px solid #374151', - borderRadius: '0.375rem', - color: '#F3F4F6', - }} - /> - { - const asset = (entry as unknown as { payload: { payload: PortfolioAsset } }).payload.payload; - return `${asset.symbol} (${asset.percentage.toFixed(1)}%)`; - }} - wrapperStyle={{ - paddingTop: '20px', - }} - /> - - - ) : ( - - - - - - - - - - )} -
-
+ - {/* Asset List */} -
- {assetsWithColors.map((asset) => ( -
handleAssetClick(asset)} - className={`flex items-center gap-3 p-3 rounded-md cursor-pointer transition-all duration-200 ${ - selectedAsset === asset.id - ? 'bg-blue-50 dark:bg-blue-900/30 border border-blue-200 dark:border-blue-700' - : 'bg-gray-50 dark:bg-gray-800 hover:bg-gray-100 dark:hover:bg-gray-700' - }`} - role="button" - tabIndex={0} - onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - handleAssetClick(asset); - } - }} - > -
-
-
- - {asset.symbol} - - - {asset.percentage.toFixed(1)}% - -
-
- - {asset.amount.toFixed(4)} {asset.symbol} - - - {formatCurrency(asset.value)} - -
-
-
- ))} -
-
!isChartLoading && handleAssetClick(asset)} - className={`flex cursor-pointer items-center gap-3 rounded-md p-3 transition-all ${ + className={`flex cursor-pointer items-center gap-3 rounded-md p-3 transition-all focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-blue-500 ${ selectedAsset === asset.id ? 'border border-blue-200 bg-blue-50 dark:border-blue-700 dark:bg-blue-900/30' : 'bg-gray-50 hover:bg-gray-100 dark:bg-gray-800 dark:hover:bg-gray-700' } ${isChartLoading ? 'pointer-events-none opacity-60' : ''}`} whileHover={isChartLoading ? undefined : { x: 4 }} whileTap={isChartLoading ? undefined : { scale: 0.98 }} + role="button" + tabIndex={isChartLoading ? -1 : 0} + aria-pressed={selectedAsset === asset.id} + onKeyDown={(e) => { + if (!isChartLoading && (e.key === 'Enter' || e.key === ' ')) { + e.preventDefault(); + handleAssetClick(asset); + } + }} >