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) ─────────────────────────────────────────── diff --git a/frontend/src/components/PortfolioChartWidget.test.tsx b/frontend/src/components/PortfolioChartWidget.test.tsx index aa25323f..f3248caf 100644 --- a/frontend/src/components/PortfolioChartWidget.test.tsx +++ b/frontend/src/components/PortfolioChartWidget.test.tsx @@ -59,10 +59,6 @@ vi.mock('recharts', () => ({ CartesianGrid: () =>
, })); -vi.mock('next-intl', () => ({ - useTranslations: () => (key: string) => key, -})); - describe('PortfolioChartWidget', () => { const mockAssets: PortfolioAsset[] = [ { @@ -105,7 +101,7 @@ describe('PortfolioChartWidget', () => { it('renders the localized title and portfolio value', () => { render(); - expect(screen.getByText('portfolioChart.valueTitle')).toBeInTheDocument(); + expect(screen.getByText('Portfolio Value')).toBeInTheDocument(); expect(screen.getByText('$4,000.00')).toBeInTheDocument(); }); @@ -118,7 +114,7 @@ describe('PortfolioChartWidget', () => { /> ); - const portfolioValue = screen.getByText(/portfolioChart\.valueTitle/i).parentElement; + const portfolioValue = screen.getByText(/Portfolio Value/i).parentElement; expect(portfolioValue).toBeInTheDocument(); }); @@ -133,14 +129,14 @@ describe('PortfolioChartWidget', () => { it('switches to the history view when history data is available', async () => { render(); - const trendButton = screen.getByText('portfolioChart.trend'); + const trendButton = screen.getByText('Trend'); fireEvent.click(trendButton); await waitFor(() => { expect(screen.getByTestId('line-chart')).toBeInTheDocument(); }); - const allocationButton = screen.getByText('portfolioChart.allocation'); + const allocationButton = screen.getByText('Allocation'); fireEvent.click(allocationButton); await waitFor(() => { @@ -165,13 +161,6 @@ describe('PortfolioChartWidget', () => { it('shows an empty history state when no trend data exists', async () => { render(); - if (assetElement) { - fireEvent.click(assetElement); - expect(assetElement).toHaveClass('bg-blue-50'); - - fireEvent.click(assetElement); - expect(assetElement).toBeInTheDocument(); - } fireEvent.click(screen.getByRole('button', { name: 'Trend' })); await waitFor(() => { @@ -210,8 +199,16 @@ describe('PortfolioChartWidget', () => { /> ); + // Compare with non-breaking spaces normalized to regular spaces: the + // exact separator Intl.NumberFormat emits between the amount and the + // currency symbol is ICU-data-dependent and not what this test cares + // about — it cares that the value is formatted es-ES/EUR-style. + const normalize = (s: string) => s.replace(/ /g, ' '); + expect(screen.getByText('Valor del portafolio')).toBeInTheDocument(); - expect(screen.getByText(formattedValue)).toBeInTheDocument(); + expect( + screen.getByText((text) => normalize(text) === normalize(formattedValue)) + ).toBeInTheDocument(); }); it('handles asset selection and onAssetClick callbacks', () => { @@ -242,7 +239,7 @@ describe('PortfolioChartWidget', () => { /> ); - expect(screen.getByText(/portfolioChart\.valueTitle/)).toBeInTheDocument(); + expect(screen.getByText(/Portfolio Value/)).toBeInTheDocument(); }); it('handles large portfolio values', () => { @@ -272,10 +269,18 @@ describe('PortfolioChartWidget', () => { it('displays asset color indicators', () => { render(); - const colorDots = screen.getAllByTestId('cell').length; - expect(colorDots).toBeGreaterThanOrEqual(0); - expect(screen.getByRole('alert')).toHaveTextContent( - 'Unable to load the latest portfolio snapshot.' - ); + expect(screen.getAllByTestId('cell').length).toBe(mockAssets.length); + }); + + it('makes asset rows keyboard-operable', () => { + const onAssetClick = vi.fn(); + render(); + + const assetRow = screen.getByText('XLM').closest('[role="button"]'); + expect(assetRow).toBeInTheDocument(); + expect(assetRow).toHaveAttribute('tabindex', '0'); + + fireEvent.keyDown(assetRow as Element, { key: 'Enter' }); + expect(onAssetClick).toHaveBeenCalledWith(mockAssets[0]); }); }); \ No newline at end of file diff --git a/frontend/src/components/PortfolioChartWidget.tsx b/frontend/src/components/PortfolioChartWidget.tsx index 38009884..3359888e 100644 --- a/frontend/src/components/PortfolioChartWidget.tsx +++ b/frontend/src/components/PortfolioChartWidget.tsx @@ -60,7 +60,6 @@ const DEFAULT_COLORS = [ /** * PortfolioChartWidget - A responsive portfolio visualization component * Displays asset allocation with pie chart and includes state management - * Optimized: removed framer-motion dependency for smaller bundle size */ export function PortfolioChartWidget({ assets = [], @@ -137,15 +136,6 @@ export function PortfolioChartWidget({ [currency, locale] ); - return ( -
- {/* Header */} -
-
-

- {t('portfolioChart.valueTitle') || 'Portfolio Value'} const containerVariants: Variants = { hidden: { opacity: 0 }, visible: { @@ -192,28 +182,6 @@ export function PortfolioChartWidget({

- -
-
- - {/* 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); + } + }} >