Fix Transaction Signer verification race condition and repair PortfolioChartWidget

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", () => {

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<string, Promise<object>>}
+ */
+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) ───────────────────────────────────────────

frontend/src/components/PortfolioChartWidget.test.tsx

@@ -59,10 +59,6 @@ vi.mock('recharts', () => ({
   CartesianGrid: () => <div data-testid="cartesian-grid" />,
 }));
 
-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(<PortfolioChartWidget {...defaultProps} />);
 
-    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(<PortfolioChartWidget {...defaultProps} historyData={historyData} />);
 
-    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(<PortfolioChartWidget {...defaultProps} historyData={[]} />);
 
-    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(<PortfolioChartWidget {...defaultProps} />);
 
-    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(<PortfolioChartWidget {...defaultProps} onAssetClick={onAssetClick} />);
+
+    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]);
   });
 });