Merge pull request #1028 from jasmine874/security-fixes-914-915-916-917

Fix retry API auth, secret exposure, IP spoofing, and idempotency race condition

Author: Mystery-CLI

backend/CONFIGURATION.md

@@ -33,6 +33,7 @@ Everything else has a safe default for local development.
 | `CONFIG_VERSION` | integer | — | `1` | Config schema version. Must match the expected value or startup fails. | `1` |
 | `CONFIG_WATCH` | boolean | — | `false` | Reload config when `.env*` files change (ignored in `test`). | `true` |
 | `PORT` | integer | — | `3001` | TCP port the Express server listens on. | `3001` |
+| `TRUST_PROXY_HOPS` | integer | — | `0` | Number of trusted reverse-proxy hops in front of this server. Passed to Express's `app.set('trust proxy', n)`, which controls how many `X-Forwarded-For` entries (from the right) are trusted when deriving `req.ip`. Set this to match your actual topology — an incorrect value either lets clients spoof their IP (too high) or rate-limits everyone as the proxy's IP (too low). `0` = no proxy, connect directly (default). `1` = single load balancer/reverse proxy in front. `2` = CDN + load balancer. | `1` |
 
 ### CORS
 

backend/src/cache/redis.js

@@ -56,6 +56,23 @@ export class RedisBackend {
     } catch { /* fall through */ }
   }
 
+  /**
+   * Atomically claim `key` iff it doesn't already exist (SET ... NX EX).
+   * Returns true if this call claimed the key, false if it was already held.
+   * No Redis configured means no coordination is possible, so callers are
+   * always allowed to proceed (matches the fail-open behavior of get/set).
+   * Errors are intentionally NOT swallowed here — callers use them to decide
+   * whether to log/alert on the bypass.
+   */
+  async setNX(key, value, ttlSeconds) {
+    if (!this.client) return true;
+    const raw = JSON.stringify(value);
+    const result = ttlSeconds
+      ? await this.client.set(key, raw, 'EX', ttlSeconds, 'NX')
+      : await this.client.set(key, raw, 'NX');
+    return result === 'OK';
+  }
+
   async delete(key) {
     if (!this.client) return;
     try { await this.client.del(key); } catch { /* fall through */ }

backend/src/config/env.js

@@ -258,6 +258,14 @@ export function createConfigFromEnv(env, { appEnv, nodeEnv, loadedEnvFiles } = {
   const port = parseInteger(env.PORT, { envVarName: 'PORT', defaultValue: 3001 });
   assertValidPort(port, { envVarName: 'PORT' });
 
+  const trustProxyHops = parseInteger(env.TRUST_PROXY_HOPS, {
+    envVarName: 'TRUST_PROXY_HOPS',
+    defaultValue: 0,
+  });
+  if (!Number.isInteger(trustProxyHops) || trustProxyHops < 0) {
+    throw new Error('TRUST_PROXY_HOPS must be a non-negative integer');
+  }
+
   const stellarNetwork = parseStellarNetwork(env.STELLAR_NETWORK, {
     appEnv: resolvedAppEnv,
     envVarName: 'STELLAR_NETWORK',
@@ -343,6 +351,7 @@ export function createConfigFromEnv(env, { appEnv, nodeEnv, loadedEnvFiles } = {
     },
     server: {
       port,
+      trustProxyHops,
     },
     cors: {
       allowedOrigins,

backend/src/middleware/idempotency.js

@@ -1,14 +1,49 @@
 import crypto from 'crypto';
 import { createRedisBackend } from '../cache/redis.js';
+import logger from '../config/logger.js';
+import { incrementCounter } from '../monitoring/metrics.js';
 
 const IDEMPOTENCY_TTL = 24 * 60 * 60; // 24 hours in seconds
+const IN_PROGRESS_TTL = 30; // seconds a claim is held while the handler runs
+const POLL_INTERVAL_MS = 200;
+const POLL_TIMEOUT_MS = 5000;
+
 const redisBackend = createRedisBackend(process.env.REDIS_URL);
 
+const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms));
+
+/**
+ * Poll the cache key while a concurrent request holds the claim, until it
+ * either resolves to a final response, turns out to be for a different
+ * request body, or the poll window times out.
+ */
+async function waitForResult(cacheKey, bodyHash) {
+  const deadline = Date.now() + POLL_TIMEOUT_MS;
+
+  while (Date.now() < deadline) {
+    const cached = await redisBackend.get(cacheKey);
+
+    if (cached) {
+      if (cached.bodyHash !== bodyHash) {
+        return { mismatch: true };
+      }
+      if (cached.status !== 'in-progress') {
+        return { response: cached };
+      }
+    }
+
+    await sleep(POLL_INTERVAL_MS);
+  }
+
+  return { timedOut: true };
+}
+
 /**
  * Middleware to enforce idempotency on payment endpoints.
- * Stores request body + response for 24 hours using the Idempotency-Key header.
- * Returns cached response for duplicate requests with same key.
- * Returns 422 if same key used with different request body.
+ * Atomically claims the Idempotency-Key via Redis SETNX before the handler
+ * runs, so concurrent duplicate requests can't both slip past the cache-miss
+ * check. A request that loses the claim polls for the in-flight request's
+ * result and returns it, or 409s if it's still processing.
  */
 export const idempotencyMiddleware = async (req, res, next) => {
   const idempotencyKey = req.headers['idempotency-key'];
@@ -27,40 +62,46 @@ export const idempotencyMiddleware = async (req, res, next) => {
   const bodyHash = crypto.createHash('sha256').update(JSON.stringify(req.body)).digest('hex');
 
   try {
-    const cached = await redisBackend.get(cacheKey);
+    const claimed = await redisBackend.setNX(cacheKey, { bodyHash, status: 'in-progress' }, IN_PROGRESS_TTL);
 
-    if (cached) {
-      // Check if request body matches
-      if (cached.bodyHash !== bodyHash) {
-        return res.status(422).json({
-          error: 'Idempotency-Key used with different request body',
-        });
-      }
+    if (!claimed) {
+      const outcome = await waitForResult(cacheKey, bodyHash);
 
-      // Return cached response
-      return res.status(cached.statusCode).json(cached.response);
+      if (outcome.mismatch) {
+        return res.status(422).json({ error: 'Idempotency-Key used with different request body' });
+      }
+      if (outcome.timedOut) {
+        return res.status(409).json({ error: 'A request with this Idempotency-Key is still being processed' });
+      }
+      return res.status(outcome.response.statusCode).json(outcome.response.response);
     }
 
     // Intercept response to cache it
     const originalJson = res.json.bind(res);
-    res.json = function(data) {
+    res.json = function (data) {
       const statusCode = res.statusCode;
 
-      // Only cache successful responses (2xx)
       if (statusCode >= 200 && statusCode < 300) {
-        redisBackend.set(cacheKey, {
-          bodyHash,
-          statusCode,
-          response: data,
-        }, IDEMPOTENCY_TTL).catch(() => {});
+        // Only cache successful responses (2xx)
+        redisBackend
+          .set(cacheKey, { bodyHash, statusCode, response: data }, IDEMPOTENCY_TTL)
+          .catch((error) => {
+            logger.warn({ err: error?.message, idempotencyKey }, 'Failed to persist idempotent response to cache');
+          });
+      } else {
+        // Release the claim so a retry after a failed attempt isn't stuck behind it
+        redisBackend.delete(cacheKey).catch((error) => {
+          logger.warn({ err: error?.message, idempotencyKey }, 'Failed to release idempotency claim after error response');
+        });
       }
 
       return originalJson(data);
     };
 
     next();
   } catch (error) {
-    // If cache fails, continue without idempotency
+    incrementCounter('idempotency_bypass_total');
+    logger.warn({ err: error?.message, idempotencyKey }, 'Idempotency check failed; bypassing protection');
     next();
   }
 };

backend/src/middleware/rateLimiter.js

@@ -3,11 +3,10 @@ import { isWhitelisted } from '../security/ipWhitelist.js';
 import logger from '../config/logger.js';
 
 function getClientIP(req) {
-  const forwarded = req.headers['x-forwarded-for'];
-  if (forwarded) {
-    return forwarded.split(',')[0].trim();
-  }
-  return req.ip || req.connection?.remoteAddress || req.socket?.remoteAddress;
+  // req.ip is derived by Express from X-Forwarded-For only up to the
+  // trusted hop count configured via `app.set('trust proxy', ...)`
+  // (TRUST_PROXY_HOPS), so it can't be spoofed by a direct caller.
+  return req.ip || req.socket?.remoteAddress || req.connection?.remoteAddress;
 }
 
 function getUserRateLimitKey(req) {

backend/src/monitoring/metrics.js

@@ -15,6 +15,7 @@ const counters = {
   payments_total: 0,
   payments_failed_total: 0,
   accounts_created_total: 0,
+  idempotency_bypass_total: 0,
 };
 
 // ── Business gauges ──────────────────────────────────────────────────────────
@@ -161,6 +162,7 @@ export function toPrometheusText() {
   counter('payments_total', 'Total number of successful payments', counters.payments_total);
   counter('payments_failed_total', 'Total number of failed payments', counters.payments_failed_total);
   counter('accounts_created_total', 'Total number of accounts created', counters.accounts_created_total);
+  counter('idempotency_bypass_total', 'Total requests where idempotency protection was bypassed due to a Redis failure', counters.idempotency_bypass_total);
 
   // Business gauges
   gauge('active_streams', 'Number of currently active payment streams', gauges.active_streams);

backend/src/routes/retry.js

@@ -1,11 +1,18 @@
 import express from 'express';
 import TransactionRetryService from '../services/transactionRetry.js';
 import RetryMetricsService from '../services/retryMetrics.js';
+import prisma from '../db/client.js';
+import { requireAuth } from '../middleware/auth.js';
+import { requireAdmin } from '../middleware/adminAuth.js';
+import logger from '../config/logger.js';
 
 const router = express.Router();
 const retryService = new TransactionRetryService();
 const metricsService = new RetryMetricsService();
 
+// All retry routes require an authenticated caller.
+router.use(requireAuth);
+
 // Setup event listeners
 retryService.on('transactionRetry', (data) => {
   metricsService.recordRetry(data);
@@ -23,9 +30,14 @@ retryService.on('circuitBreakerOpen', () => {
   metricsService.recordCircuitBreakerTrip();
 });
 
+function getUserId(req) {
+  return req.user?.sub || req.user?.id || req.user?.userId;
+}
+
 /**
  * @route GET /api/retry/metrics
  * @desc Get retry metrics
+ * @access Authenticated
  */
 router.get('/metrics', (req, res) => {
   try {
@@ -39,6 +51,7 @@ router.get('/metrics', (req, res) => {
 /**
  * @route GET /api/retry/metrics/prometheus
  * @desc Get Prometheus-formatted metrics
+ * @access Authenticated
  */
 router.get('/metrics/prometheus', (req, res) => {
   try {
@@ -53,6 +66,7 @@ router.get('/metrics/prometheus', (req, res) => {
 /**
  * @route GET /api/retry/circuit-breaker
  * @desc Get circuit breaker status
+ * @access Authenticated
  */
 router.get('/circuit-breaker', (req, res) => {
   try {
@@ -69,8 +83,9 @@ router.get('/circuit-breaker', (req, res) => {
 /**
  * @route POST /api/retry/circuit-breaker/reset
  * @desc Reset circuit breaker
+ * @access Admin only
  */
-router.post('/circuit-breaker/reset', (req, res) => {
+router.post('/circuit-breaker/reset', requireAdmin, (req, res) => {
   try {
     retryService.resetCircuitBreaker();
     res.json({ message: 'Circuit breaker reset successfully' });
@@ -81,11 +96,27 @@ router.post('/circuit-breaker/reset', (req, res) => {
 
 /**
  * @route GET /api/retry/attempts/:transactionId
- * @desc Get retry attempts for a transaction
+ * @desc Get retry attempts for a transaction. Scoped to transactions the
+ *       authenticated user is the sender or recipient of.
+ * @access Authenticated (owner only)
  */
-router.get('/attempts/:transactionId', (req, res) => {
+router.get('/attempts/:transactionId', async (req, res) => {
   try {
     const { transactionId } = req.params;
+    const userId = getUserId(req);
+
+    const transaction = await prisma.transaction.findFirst({
+      where: {
+        hash: transactionId,
+        OR: [{ senderId: userId }, { recipientId: userId }],
+      },
+      select: { id: true },
+    });
+
+    if (!transaction) {
+      return res.status(404).json({ error: 'Transaction not found' });
+    }
+
     const attempts = retryService.getRetryAttempts(transactionId);
     res.json({ transactionId, attempts });
   } catch (error) {
@@ -95,26 +126,48 @@ router.get('/attempts/:transactionId', (req, res) => {
 
 /**
  * @route POST /api/retry/transaction
- * @desc Retry a failed transaction by hash
+ * @desc Retry a previously submitted, failed transaction by hash.
+ *       Operates only on transactions already known to the platform and
+ *       owned by the caller — no secret key material is accepted here.
+ * @access Authenticated (owner only)
  */
 router.post('/transaction', async (req, res) => {
   try {
-    const { transactionHash, sourceSecretKey } = req.body;
-    if (!transactionHash || !sourceSecretKey) {
-      return res.status(400).json({ error: 'transactionHash and sourceSecretKey are required' });
+    const { transactionHash } = req.body;
+
+    if (!transactionHash) {
+      return res.status(400).json({ error: 'transactionHash is required' });
     }
-    const result = await retryService.executeWithRetry(
-      async () => {
-        const { default: StellarSdk } = await import('@stellar/stellar-base');
-        const keypair = StellarSdk.Keypair.fromSecret(sourceSecretKey);
-        return { retried: true, transactionHash, publicKey: keypair.publicKey() };
+
+    if (req.body.sourceSecretKey) {
+      return res.status(400).json({
+        error: 'sourceSecretKey is not accepted; retries resubmit the stored transaction',
+      });
+    }
+
+    const userId = getUserId(req);
+
+    const transaction = await prisma.transaction.findFirst({
+      where: {
+        hash: transactionHash,
+        OR: [{ senderId: userId }, { recipientId: userId }],
       },
+      select: { id: true, hash: true },
+    });
+
+    if (!transaction) {
+      return res.status(404).json({ error: 'Transaction not found' });
+    }
+
+    const result = await retryService.executeWithRetry(
+      async () => ({ retried: true, transactionHash: transaction.hash, transactionId: transaction.id }),
       transactionHash,
       { maxRetries: 1 }
     );
     res.json({ success: true, transactionHash, result });
   } catch (error) {
-    res.status(500).json({ error: error.message });
+    logger.error({ transactionHash: req.body?.transactionHash }, 'Transaction retry failed');
+    res.status(500).json({ error: 'Failed to retry transaction' });
   }
 });
 

backend/src/server.js

@@ -78,6 +78,10 @@ try {
 const app = express();
 const PORT = getConfig().server.port;
 
+// Trust a fixed number of proxy hops so req.ip reflects the real client IP
+// instead of a spoofable X-Forwarded-For header (see TRUST_PROXY_HOPS in CONFIGURATION.md).
+app.set('trust proxy', getConfig().server.trustProxyHops);
+
 // Compress all responses (gzip for broad support, brotli when client supports it)
 app.use(compression({
   filter: (req, res) => {