From ac5b9e2aa3622eb648ec9dfcd69c4a14c6ca4df6 Mon Sep 17 00:00:00 2001
From: junman140 <godswillitina@gmail.com>
Date: Wed, 26 Aug 2026 14:18:18 +0100
Subject: [PATCH] Security hardening: CSP for docs, X-Request-ID validation,
 log redaction, webhook SSRF guard

- #129: relax helmet's default CSP for the /api-docs Swagger UI route only
- #133: validate client X-Request-ID (max 128 chars, [A-Za-z0-9_-]); fall back to a server-generated ID otherwise; document trust boundary
- #94: extend log redaction to arrays, string-embedded secret query params, and Authorization keys; preserve whsec_**** partial reveal; cover non-sensitive fields
- #96: add delivery-time SSRF guard (DNS resolve + IP pinning, no redirects) blocking private/internal targets; reduce test-endpoint error detail to a generic category with raw detail kept server-side; add per-IP rate-limit aggregation test
---
 README.md                         |  34 ++++++-
 src/errors/AppError.js            |   1 +
 src/index.js                      |   2 +
 src/logger.js                     |  32 +-----
 src/middleware/csp.js             |  29 ++++++
 src/middleware/requestId.js       |  21 +++-
 src/routes/webhooks.js            |  19 +++-
 src/services/logRedaction.js      | 126 +++++++++++++++++++++++
 src/services/ssrfGuard.js         | 147 +++++++++++++++++++++++++++
 src/services/webhookDispatcher.js |  19 +++-
 test/api-docs.test.js             |  29 ++++++
 test/logger.test.js               |  71 +++++++++++++
 test/requestId.test.js            |  49 +++++++++
 test/webhookDispatcher.test.js    |   7 ++
 test/webhooks.routes.test.js      |   9 ++
 test/webhooks.ssrf.test.js        | 159 ++++++++++++++++++++++++++++++
 16 files changed, 717 insertions(+), 37 deletions(-)
 create mode 100644 src/middleware/csp.js
 create mode 100644 src/services/logRedaction.js
 create mode 100644 src/services/ssrfGuard.js
 create mode 100644 test/logger.test.js
 create mode 100644 test/webhooks.ssrf.test.js

diff --git a/README.md b/README.md
index 51173fa..f1ad793 100644
--- a/README.md
+++ b/README.md
@@ -531,6 +531,21 @@ POST /api/v1/webhooks/:id/test
 ```
 Sends a synthetic `pool.assets_locked` payload to the registered URL and returns the resulting delivery summary. Limited to 5 calls/min/IP by default.
 
+> **SSRF protection.** Webhook targets are validated against private/internal
+> network ranges (RFC-1918, loopback, link-local, IPv6 ULA/link-local, CGNAT,
+> etc.) both when registered **and** again at delivery time — and the outbound
+> connection is pinned to the validated public IP, with redirects disabled — so
+> a `test` call (or any real dispatch) cannot be used as an internal-network
+> reconnaissance oracle. A blocked target is refused up front with a `422
+> WEBHOOK_TARGET_BLOCKED` error and is never delivered.
+>
+> **Reduced error detail.** The `last_error` field returned by the test endpoint
+> is a coarse category (`unreachable` | `error_response` | `delivery_failed`),
+> not the raw low-level network error string (e.g. `ECONNREFUSED`). The raw
+> detail is still written to server-side logs for operators; only the public
+> response is sanitized, to avoid turning the test endpoint into an information
+> leak about internal reachability. See issue #96.
+
 #### Inspect deliveries (admin dashboard feed)
 ```
 GET /api/v1/webhooks/:id/deliveries?limit=50
@@ -615,11 +630,26 @@ The API returns appropriate HTTP status codes:
 ```json
 {
   "error": "Error type",
-  "message": "Detailed error message"
+  "message": "Detailed error message",
+  "request_id": "req_…"
 }
-
 ```
 
+### `X-Request-ID` correlation header
+
+Every response carries an `X-Request-ID` header, and every JSON response body
+(and every error body) includes a `request_id` field, so you can correlate a
+client request with the server's logs.
+
+`X-Request-ID` is an **optional client hint**, not an authoritative or
+guaranteed-unique identifier. A client may supply its own value via the
+`X-Request-ID` request header to tie its own logs to SmartDrop's; if the value
+is missing, malformed (contains characters outside `[A-Za-z0-9_-]`), or longer
+than 128 characters, the server **ignores it and generates a fresh ID instead**.
+Treat the returned `request_id` purely as a correlation aid — multiple unrelated
+requests can share a client-chosen value, so it must not be used as a security
+or uniqueness anchor. See issue #133.
+
 ---
 
 ## Development
diff --git a/src/errors/AppError.js b/src/errors/AppError.js
index 35c636b..de51cfb 100644
--- a/src/errors/AppError.js
+++ b/src/errors/AppError.js
@@ -5,6 +5,7 @@ const ERROR_CODES = Object.freeze({
   UNAUTHORIZED: { statusCode: 401 },
   NOT_FOUND: { statusCode: 404 },
   PAYLOAD_TOO_LARGE: { statusCode: 413 },
+  WEBHOOK_TARGET_BLOCKED: { statusCode: 422 },
   RATE_LIMITED: { statusCode: 429 },
   UPSTREAM_ERROR: { statusCode: 502 },
   INTERNAL_ERROR: { statusCode: 500 },
diff --git a/src/index.js b/src/index.js
index 52ea147..f3c6310 100644
--- a/src/index.js
+++ b/src/index.js
@@ -14,6 +14,7 @@ const { makeLeaderAwareJob } = require('./jobs/leaderAwareJob');
 const { warmCache } = require('./startup/cacheWarm');
 const buildCorsMiddleware = require('./middleware/cors');
 const buildRateLimit = require('./middleware/rateLimit');
+const { docsCspMiddleware } = require('./middleware/csp');
 const { requestIdMiddleware } = require('./middleware/requestId');
 const { requireApiKey } = require('./middleware/auth');
 const { errorHandler, notFoundHandler } = require('./middleware/errorHandler');
@@ -166,6 +167,7 @@ app.use('/api/v1', indexerRouter);
 app.use('/api/v1', webhooksRouter);
 app.use('/api/v1', airdropsRouter);
 app.use('/api-docs', globalApiLimit);
+app.use('/api-docs', docsCspMiddleware);
 app.use('/api-docs', apiDocsRouter);
 
 app.use(notFoundHandler);
diff --git a/src/logger.js b/src/logger.js
index 23238e5..79a3429 100644
--- a/src/logger.js
+++ b/src/logger.js
@@ -2,6 +2,7 @@ const winston = require('winston');
 const DailyRotateFile = require('winston-daily-rotate-file');
 const { name: serviceName, version } = require('../package.json');
 const { requestContext } = require('./middleware/requestId');
+const { redactFormat } = require('./services/logRedaction');
 
 // ==================== LOG LEVEL ====================
 const getLogLevel = () => {
@@ -14,37 +15,6 @@ const getLogLevel = () => {
   return 'debug';
 };
 
-// ==================== REDACTION ====================
-const redactFormat = winston.format((info) => {
-  const sensitiveKeys = ['apikey', 'privatekey', 'secret', 'token'];
-
-  const redactValue = (value, key) => {
-    if (typeof value !== 'string') return '[REDACTED]';
-    if (key.toLowerCase().includes('secret') && value.startsWith('whsec_')) {
-      return 'whsec_****';
-    }
-    return '[REDACTED]';
-  };
-
-  const redact = (obj) => {
-    if (!obj || typeof obj !== 'object') return obj;
-
-    for (const key of Object.keys(obj)) {
-      const lowerKey = key.toLowerCase();
-      const isSensitive = sensitiveKeys.some(k => lowerKey.includes(k));
-
-      if (isSensitive) {
-        obj[key] = redactValue(obj[key], key);
-      } else if (typeof obj[key] === 'object') {
-        redact(obj[key]);
-      }
-    }
-    return obj;
-  };
-
-  return redact(info);
-});
-
 // ==================== FORMAT DECISION ====================
 const env = process.env.NODE_ENV || 'development';
 const logFormat = process.env.LOG_FORMAT || (env === 'production' ? 'json' : 'pretty');
diff --git a/src/middleware/csp.js b/src/middleware/csp.js
new file mode 100644
index 0000000..e278016
--- /dev/null
+++ b/src/middleware/csp.js
@@ -0,0 +1,29 @@
+'use strict';
+
+/**
+ * Content-Security-Policy override for the Swagger UI docs route (/api-docs).
+ *
+ * helmet() applies a strict default CSP to every response (good for the API),
+ * but swagger-ui-express renders by injecting inline <script>/<style> tags and
+ * data:-URL assets that the default CSP blocks, leaving a broken docs page in
+ * development. This middleware relaxes the CSP *only* for the docs route
+ * (mounted immediately before the apiDocs router) by overwriting the header
+ * helmet set. It is scoped to /api-docs so the strict default still protects
+ * the real API surface. See #129.
+ */
+const DOCS_CSP_DIRECTIVES = [
+  "default-src 'self'",
+  "script-src 'self' 'unsafe-inline'",
+  "style-src 'self' 'unsafe-inline'",
+  "img-src 'self' data: https:",
+  "font-src 'self' data:",
+  "connect-src 'self'",
+  "frame-ancestors 'none'",
+].join('; ');
+
+function docsCspMiddleware(_req, res, next) {
+  res.setHeader('Content-Security-Policy', DOCS_CSP_DIRECTIVES);
+  next();
+}
+
+module.exports = { docsCspMiddleware, DOCS_CSP_DIRECTIVES };
diff --git a/src/middleware/requestId.js b/src/middleware/requestId.js
index 737f863..e9e2ff5 100644
--- a/src/middleware/requestId.js
+++ b/src/middleware/requestId.js
@@ -5,6 +5,23 @@ const { AsyncLocalStorage } = require('node:async_hooks');
 
 const requestContext = new AsyncLocalStorage();
 
+// X-Request-ID is an OPTIONAL client hint used to correlate a client's own logs
+// with this server's logs. It is NOT authoritative and NOT guaranteed unique:
+// a client can send anything, so we only honor values that look like a sane
+// correlation ID (alphanumeric plus -/_ and within a modest length). Anything
+// else is discarded and a fresh server-generated ID is used instead. See #133.
+const MAX_REQUEST_ID_LENGTH = 128;
+const SAFE_REQUEST_ID_RE = /^[A-Za-z0-9_-]+$/;
+
+function isValidRequestId(value) {
+  return (
+    typeof value === 'string' &&
+    value.length > 0 &&
+    value.length <= MAX_REQUEST_ID_LENGTH &&
+    SAFE_REQUEST_ID_RE.test(value)
+  );
+}
+
 function nanoid(size = 21) {
   const alphabet = '0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz-';
   const bytes = crypto.randomBytes(size);
@@ -14,7 +31,8 @@ function nanoid(size = 21) {
 }
 
 function requestIdMiddleware(req, res, next) {
-  req.id = req.get('x-request-id') || `req_${nanoid()}`;
+  const clientId = req.get('x-request-id');
+  req.id = isValidRequestId(clientId) ? clientId : `req_${nanoid()}`;
   res.setHeader('X-Request-ID', req.id);
 
   const originalJson = res.json.bind(res);
@@ -38,4 +56,5 @@ module.exports = {
   requestIdMiddleware,
   requestContext,
   nanoid,
+  isValidRequestId,
 };
diff --git a/src/routes/webhooks.js b/src/routes/webhooks.js
index 16599c1..0a2e7e8 100644
--- a/src/routes/webhooks.js
+++ b/src/routes/webhooks.js
@@ -36,6 +36,23 @@ const testLimit = buildRateLimit({
 
 router.use('/webhooks', manageLimit);
 
+/**
+ * Map a raw delivery error string to a coarse, non-leaky category for the
+ * externally-visible test-endpoint response. The raw low-level network error
+ * (ECONNREFUSED/ETIMEDOUT/ECONNRESET, etc.) is kept in server-side logs but
+ * must not be echoed back to the caller, since it is exactly what makes the
+ * test endpoint a useful internal-network reconnaissance oracle (see #96).
+ */
+function deliveryErrorCategory(rawError) {
+  if (!rawError) return null;
+  const msg = String(rawError);
+  if (/ECONNREFUSED|ENOTFOUND|ETIMEDOUT|ECONNRESET|ENETUNREACH|EHOSTUNREACH|ECONNABORTED|socket hang up|network error/i.test(msg)) {
+    return 'unreachable';
+  }
+  if (/^HTTP \d+/.test(msg)) return 'error_response';
+  return 'delivery_failed';
+}
+
 function publicView(webhook) {
   if (!webhook) return null;
   return {
@@ -123,7 +140,7 @@ router.post('/webhooks/:id/test', validateRouteIdParams, testLimit, async (req,
       status: delivery.status,
       attempts: delivery.attempts,
       response_status: delivery.response_status,
-      last_error: delivery.last_error,
+      last_error: deliveryErrorCategory(delivery.last_error),
     });
   } catch (err) {
     return next(err);
diff --git a/src/services/logRedaction.js b/src/services/logRedaction.js
new file mode 100644
index 0000000..32be1f7
--- /dev/null
+++ b/src/services/logRedaction.js
@@ -0,0 +1,126 @@
+'use strict';
+
+const winston = require('winston');
+
+/**
+ * Log redaction.
+ *
+ * Winston's JSON formatter already JSON-escapes string values, so the risk
+ * here is NOT structurally corrupting log lines. The risks are:
+ *   1. Correlation/secret leakage: sensitive values flowing into every log line.
+ *   2. Volume: extremely long values (e.g. an oversized X-Request-ID) bloating
+ *      every downstream log line.
+ *
+ * This format redacts two ways:
+ *   - Key-based: any object key containing apikey/privatekey/secret/token/
+ *     authorization triggers redaction of its value.
+ *   - Pattern-based: every string value (regardless of key) is scanned for
+ *     webhook secret shapes (`whsec_…`, partially revealed as `whsec_****` for
+ *     operator debuggability) and for credentials embedded in URLs
+ *     (`?token=…`, `?secret=…`, `?key=…`), which are replaced in place.
+ *
+ * Arrays are walked as first-class nodes: a plain array of secret-shaped
+ * strings (not wrapped in an object) is now redacted, not just arrays of
+ * objects with sensitive keys.
+ */
+
+const SENSITIVE_KEYS = ['apikey', 'privatekey', 'secret', 'token', 'authorization'];
+
+// Matches webhook secrets (whsec_ + hex). Safe to match broadly: the prefix is
+// distinctive and only ever precedes a secret in this codebase.
+const WHSEC_RE = /whsec_[0-9a-f]+/gi;
+
+// Matches token=/secret=/key= query parameters embedded in any string value,
+// capturing the parameter name so we can preserve it and only redact the value.
+const QUERY_SECRET_RE = /([?&](?:token|secret|key)=)([^&#\s]+)/gi;
+
+// Normalize a key for matching: treat `_`/`-` as nothing so `api_key`,
+// `ApiKey`, `PRIVATE_KEY` all match their tokens.
+function normKey(key) {
+  return String(key).toLowerCase().replace(/[_-]/g, '');
+}
+
+function isSensitiveKey(key) {
+  const n = normKey(key);
+  return SENSITIVE_KEYS.some((k) => n.includes(k));
+}
+
+function redactWhsec(value) {
+  return value.replace(WHSEC_RE, 'whsec_****');
+}
+
+function redactQuerySecrets(value) {
+  return value.replace(QUERY_SECRET_RE, '$1[REDACTED]');
+}
+
+// Scan a string value for secret *shapes* regardless of the key it sits under.
+function scanString(value) {
+  let out = redactWhsec(value);
+  out = redactQuerySecrets(out);
+  return out;
+}
+
+function redactValue(value, key) {
+  if (typeof value !== 'string') return '[REDACTED]';
+  if (normKey(key).includes('secret') && value.startsWith('whsec_')) {
+    return 'whsec_****';
+  }
+  return '[REDACTED]';
+}
+
+// A sensitive-keyed value may itself be a nested object/array (e.g. `secrets`
+// is an array of secret-shaped strings). Recurse into it so each leaf is
+// redacted by shape/key rather than blanket-replacing the whole structure with
+// `[REDACTED]` — which would also lose the `whsec_****` partial reveal.
+function redactSensitiveValue(value, key, seen) {
+  if (typeof value === 'string') return redactValue(value, key);
+  if (Array.isArray(value)) {
+    return value.map((el) => {
+      if (typeof el === 'string') return redactValue(el, key);
+      if (el && typeof el === 'object') return redact(el, seen);
+      return el;
+    });
+  }
+  if (value && typeof value === 'object') return redact(value, seen);
+  return '[REDACTED]';
+}
+
+function redact(node, seen) {
+  if (!node || typeof node !== 'object') return node;
+  if (seen.has(node)) return node;
+  seen.add(node);
+
+  if (Array.isArray(node)) {
+    for (let i = 0; i < node.length; i += 1) {
+      const el = node[i];
+      if (typeof el === 'string') {
+        node[i] = scanString(el);
+      } else if (el && typeof el === 'object') {
+        redact(el, seen);
+      }
+    }
+    return node;
+  }
+
+  for (const key of Object.keys(node)) {
+    const val = node[key];
+
+    if (isSensitiveKey(key)) {
+      node[key] = redactSensitiveValue(val, key, seen);
+    } else if (typeof val === 'string') {
+      node[key] = scanString(val);
+    } else if (val && typeof val === 'object') {
+      redact(val, seen);
+    }
+  }
+  return node;
+}
+
+function redactInfo(info) {
+  // Track visited objects to avoid infinite recursion on circular structures.
+  return redact(info, new Set());
+}
+
+const redactFormat = winston.format(redactInfo);
+
+module.exports = { redactInfo, redactFormat, SENSITIVE_KEYS };
diff --git a/src/services/ssrfGuard.js b/src/services/ssrfGuard.js
new file mode 100644
index 0000000..be81b78
--- /dev/null
+++ b/src/services/ssrfGuard.js
@@ -0,0 +1,147 @@
+'use strict';
+
+const dns = require('dns').promises;
+const net = require('net');
+const { URL } = require('url');
+const AppError = require('../errors/AppError');
+
+/**
+ * Server-side SSRF guard for outbound webhook deliveries.
+ *
+ * Why this lives at delivery time (not just registration time): a hostname
+ * that resolves to a public IP when the webhook is *registered* can be
+ * re-pointed at an internal address by the time it is *dispatched* (classic
+ * DNS-rebinding). So we re-resolve and re-validate the target on every actual
+ * outbound request, and — to close the rebinding window entirely — we pin the
+ * connection to the specific public IP we just validated (sending the original
+ * hostname as the `Host` header) instead of letting axios re-resolve the name
+ * at connect time. `maxRedirects` is forced to 0 so a 30x from the target
+ * cannot bounce us onto an unvalidated internal address.
+ *
+ * Private/blocked ranges covered: IPv4 loopback (127/8), RFC-1918 (10/8,
+ * 172.16/12, 192.168/16), link-local (169.254/16), CGNAT (100.64/10),
+ * unspecified (0/8), multicast/reserved (224/4+), IPv6 loopback (::1),
+ * unspecified (::), unique-local (fc00::/7), link-local (fe80::/10), multicast
+ * (ff00::/8), and IPv4-mapped IPv6 (::ffff:a.b.c.d).
+ */
+
+function isPrivateIpv4(ip) {
+  const parts = ip.split('.');
+  if (parts.length !== 4) return true;
+  const octets = parts.map((p) => Number(p));
+  if (octets.some((n) => Number.isNaN(n) || n < 0 || n > 255)) return true;
+  const [a, b] = octets;
+  if (a === 10) return true; // RFC-1918
+  if (a === 172 && b >= 16 && b <= 31) return true; // RFC-1918
+  if (a === 192 && b === 168) return true; // RFC-1918
+  if (a === 127) return true; // loopback
+  if (a === 169 && b === 254) return true; // link-local
+  if (a === 0) return true; // unspecified
+  if (a === 100 && b >= 64 && b <= 127) return true; // CGNAT / shared
+  if (a >= 224) return true; // multicast + reserved (224/4, 240/4)
+  return false;
+}
+
+function isPrivateIpv6(ip) {
+  const lower = ip.toLowerCase();
+  if (lower === '::1' || lower === '::') return true;
+  if (lower.startsWith('fe80')) return true; // link-local
+  if (lower.startsWith('fc') || lower.startsWith('fd')) return true; // unique-local fc00::/7
+  if (lower.startsWith('ff')) return true; // multicast
+  return false;
+}
+
+function isPrivateIp(ip) {
+  if (typeof ip !== 'string') return true;
+  const addr = ip.trim();
+  if (addr.startsWith('::ffff:') && net.isIPv4(addr.slice(7))) {
+    return isPrivateIpv4(addr.slice(7));
+  }
+  if (net.isIPv4(addr)) return isPrivateIpv4(addr);
+  if (net.isIPv6(addr)) return isPrivateIpv6(addr);
+  return true;
+}
+
+function normalizeHostname(hostname) {
+  return hostname.replace(/^\[/, '').replace(/\]$/, '');
+}
+
+async function resolveAddresses(hostname) {
+  const host = normalizeHostname(hostname);
+  if (net.isIP(host)) return [host]; // already an IP literal — no DNS lookup
+  const records = await dns.lookup(host, { all: true });
+  return records.map((r) => r.address);
+}
+
+function assertProtocol(url) {
+  let parsed;
+  try {
+    parsed = new URL(url);
+  } catch {
+    throw new AppError('INVALID_URL', 'Webhook URL is not a valid URL', 400);
+  }
+  if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') {
+    throw new AppError('INVALID_URL', 'Webhook URL must use http or https', 400);
+  }
+  return parsed;
+}
+
+/**
+ * Synchronous, network-free registration-time check. Rejects obviously
+ * private targets expressed as a literal IP (covers the common cases without
+ * a DNS round-trip). Hostname-based private targets are caught by the
+ * delivery-time `assertPublicTarget` below. Returns the URL unchanged.
+ */
+function assertPublicUrlSync(url) {
+  const parsed = assertProtocol(url);
+  const host = normalizeHostname(parsed.hostname);
+  if (net.isIP(host) && isPrivateIp(host)) {
+    throw new AppError(
+      'WEBHOOK_TARGET_BLOCKED',
+      'Webhook target must not point to a private or internal network address',
+      422,
+    );
+  }
+  return url;
+}
+
+/**
+ * Delivery-time check. Resolves the target (if it's a hostname), rejects any
+ * private/internal address, and returns a connection target pinned to a
+ * validated public IP plus the original hostname to send as `Host`. Throws
+ * AppError('WEBHOOK_TARGET_BLOCKED') when the target is not allowed.
+ */
+async function assertPublicTarget(url) {
+  const parsed = assertProtocol(url);
+  const addresses = await resolveAddresses(parsed.hostname);
+  if (addresses.length === 0) {
+    throw new AppError(
+      'WEBHOOK_TARGET_BLOCKED',
+      'Webhook target could not be resolved to a public address',
+      422,
+    );
+  }
+  for (const addr of addresses) {
+    if (isPrivateIp(addr)) {
+      throw new AppError(
+        'WEBHOOK_TARGET_BLOCKED',
+        'Webhook target must not point to a private or internal network address',
+        422,
+      );
+    }
+  }
+
+  const chosen = addresses[0];
+  const isV6 = net.isIP(chosen) === 6;
+  const pinned = new URL(url);
+  pinned.hostname = isV6 ? `[${chosen}]` : chosen;
+  const originalHost = parsed.hostname;
+  const hostHeader = net.isIP(normalizeHostname(originalHost)) ? null : originalHost;
+  return { targetUrl: pinned.toString(), host: hostHeader };
+}
+
+module.exports = {
+  assertPublicTarget,
+  assertPublicUrlSync,
+  isPrivateIp,
+};
diff --git a/src/services/webhookDispatcher.js b/src/services/webhookDispatcher.js
index 9ab7e83..61ce447 100644
--- a/src/services/webhookDispatcher.js
+++ b/src/services/webhookDispatcher.js
@@ -8,6 +8,7 @@ const signature = require('./webhookSignature');
 const events = require('./webhookEvents');
 const webhookRepo = require('../repositories/webhookRepository');
 const deliveryRepo = require('../repositories/deliveryRepository');
+const { assertPublicTarget } = require('./ssrfGuard');
 
 const USER_AGENT = 'SmartDrop-Webhooks/1.0';
 
@@ -60,11 +61,20 @@ function buildHeaders(secret, body, eventType, deliveryId) {
 }
 
 async function postOnce(url, headers, body) {
-  return axios.post(url, body, {
-    headers,
+  // assertPublicTarget re-resolves + re-validates the target at request time
+  // (closing the DNS-rebinding window) and pins the connection to a validated
+  // public IP. `maxRedirects: 0` stops a 30x from bouncing us onto an
+  // unvalidated internal address.
+  const { targetUrl, host } = await assertPublicTarget(url);
+  const requestHeaders = { ...headers };
+  if (host) requestHeaders.Host = host;
+
+  return axios.post(targetUrl, body, {
+    headers: requestHeaders,
     timeout: config.webhooks.timeoutMs,
     transformRequest: [(data) => data],
     validateStatus: () => true,
+    maxRedirects: 0,
   });
 }
 
@@ -168,6 +178,11 @@ async function attempt(deliveryId) {
 }
 
 async function deliverToWebhook(webhook, eventType, eventId, payload) {
+  // Validate the target before doing any work so a blocked (private/internal)
+  // target is refused up front with a clear error rather than recorded as a
+  // failed delivery. postOnce re-validates at connect time for defense in depth.
+  await assertPublicTarget(webhook.url);
+
   const delivery = await deliveryRepo.create({
     webhook_id: webhook.id,
     event_id: eventId,
diff --git a/test/api-docs.test.js b/test/api-docs.test.js
index ff5767c..bc7cf78 100644
--- a/test/api-docs.test.js
+++ b/test/api-docs.test.js
@@ -4,6 +4,7 @@ const path = require('path');
 const fs = require('fs');
 const express = require('express');
 const request = require('supertest');
+const helmet = require('helmet');
 
 describe('OpenAPI specification', () => {
   test('openapi.yaml exists and is valid YAML', () => {
@@ -95,3 +96,31 @@ describe('Swagger UI', () => {
       });
   });
 });
+
+describe('Swagger UI CSP exception (#129)', () => {
+  test('docs route sends a relaxed CSP allowing inline scripts/styles (Swagger UI needs them)', () => {
+    const NODE_ENV = process.env.NODE_ENV;
+    process.env.NODE_ENV = 'development';
+
+    jest.resetModules();
+    const apiDocsRouter = require('../src/routes/apiDocs');
+    const { docsCspMiddleware } = require('../src/middleware/csp');
+
+    const app = express();
+    app.use(helmet());
+    app.use('/api-docs', docsCspMiddleware);
+    app.use('/api-docs', apiDocsRouter);
+
+    return request(app)
+      .get('/api-docs/')
+      .expect(200)
+      .then((res) => {
+        const csp = res.headers['content-security-policy'];
+        expect(csp).toBeDefined();
+        expect(csp).toContain("script-src 'self' 'unsafe-inline'");
+        expect(csp).toContain("style-src 'self' 'unsafe-inline'");
+        expect(res.text).toContain('swagger-ui');
+        process.env.NODE_ENV = NODE_ENV;
+      });
+  });
+});
diff --git a/test/logger.test.js b/test/logger.test.js
new file mode 100644
index 0000000..926d755
--- /dev/null
+++ b/test/logger.test.js
@@ -0,0 +1,71 @@
+'use strict';
+
+const { redactInfo } = require('../src/services/logRedaction');
+
+function redact(input) {
+  // redactInfo mutates in place; clone so each test starts clean.
+  return redactInfo(JSON.parse(JSON.stringify(input)));
+}
+
+describe('log redaction (#94)', () => {
+  test('redacts a sensitive value in a nested object (regression)', () => {
+    const out = redact({ webhook: { secret: 'whsec_abcdef0123456789' } });
+    expect(out.webhook.secret).toBe('whsec_****');
+  });
+
+  test('redacts mixed-case sensitive key names (regression)', () => {
+    const out = redact({ ApiKey: 'sk_live_abc', PRIVATEKEY: 'x', TOKEN: 'y' });
+    expect(out.ApiKey).toBe('[REDACTED]');
+    expect(out.PRIVATEKEY).toBe('[REDACTED]');
+    expect(out.TOKEN).toBe('[REDACTED]');
+  });
+
+  test('preserves the whsec_**** partial reveal for webhook secrets', () => {
+    const out = redact({ secret: 'whsec_abcdef0123456789' });
+    expect(out.secret).toBe('whsec_****');
+  });
+
+  test('redacts an array of raw secret-shaped strings (not wrapped in an object)', () => {
+    const out = redact({
+      secrets: ['whsec_zzz1111111111111', 'whsec_zzz2222222222222'],
+    });
+    expect(out.secrets).toEqual(['whsec_****', 'whsec_****']);
+  });
+
+  test('redacts an array of objects with sensitive keys (regression)', () => {
+    const out = redact({
+      keys: [{ api_key: 'sk_live_xxxxxxxxxxxx' }, { token: 'tops3cret' }],
+    });
+    expect(out.keys[0].api_key).toBe('[REDACTED]');
+    expect(out.keys[1].token).toBe('[REDACTED]');
+  });
+
+  test('redacts token=/secret=/key= query params embedded in a URL under a non-sensitive key', () => {
+    const out = redact({
+      url: 'https://x.com/hook?token=supersecret123&other=1&key=leaky',
+    });
+    expect(out.url).toContain('token=[REDACTED]');
+    expect(out.url).toContain('key=[REDACTED]');
+    expect(out.url).toContain('other=1');
+    expect(out.url).not.toContain('supersecret123');
+    expect(out.url).not.toContain('leaky');
+  });
+
+  test('does not redact legitimate, non-sensitive fields (false-positive guard)', () => {
+    const out = redact({
+      delivery_id: 'del_abc123',
+      asset_code: 'XLM',
+      price_usd: 0.1234,
+      event_type: 'pool.assets_locked',
+    });
+    expect(out.delivery_id).toBe('del_abc123');
+    expect(out.asset_code).toBe('XLM');
+    expect(out.price_usd).toBe(0.1234);
+    expect(out.event_type).toBe('pool.assets_locked');
+  });
+
+  test('redacts an Authorization header value under an authorization key', () => {
+    const out = redact({ authorization: 'Bearer sk_live_abcdef123456' });
+    expect(out.authorization).toBe('[REDACTED]');
+  });
+});
diff --git a/test/requestId.test.js b/test/requestId.test.js
index 4651b7b..41ef7cd 100644
--- a/test/requestId.test.js
+++ b/test/requestId.test.js
@@ -119,4 +119,53 @@ describe('requestId on app routes', () => {
     expect(res.status).toBe(200);
     expect(res.headers['x-request-id']).toBeDefined();
   });
+});
+
+describe('requestId client-supplied X-Request-ID validation (#133)', () => {
+  function buildApp() {
+    const app = express();
+    app.use(requestIdMiddleware);
+    app.get('/echo', (req, res) => res.json({ ok: true }));
+    app.post('/boom', (req, res) => {
+      res.status(400).json({ error: { code: 'BAD', message: 'bad', request_id: req.id } });
+    });
+    return app;
+  }
+
+  test('honors a well-formed, reasonably-sized client X-Request-ID', async () => {
+    const app = buildApp();
+    const res = await request(app).get('/echo').set('X-Request-ID', 'client-corr-123_X');
+
+    expect(res.headers['x-request-id']).toBe('client-corr-123_X');
+    expect(res.body.request_id).toBe('client-corr-123_X');
+  });
+
+  test('rejects an oversized X-Request-ID and generates a server ID instead', async () => {
+    const app = buildApp();
+    // Exceeds MAX_REQUEST_ID_LENGTH (128) but stays within Node's header-size
+    // limit so the server actually receives and re-evaluates it.
+    const huge = 'A'.repeat(200);
+    const res = await request(app).get('/echo').set('X-Request-ID', huge);
+
+    expect(res.headers['x-request-id']).not.toBe(huge);
+    expect(res.headers['x-request-id'].length).toBeLessThan(200);
+    expect(res.headers['x-request-id']).toMatch(/^req_[0-9A-Za-z_-]+$/);
+    expect(res.body.request_id).toBe(res.headers['x-request-id']);
+  });
+
+  test('rejects a malformed (disallowed-character) X-Request-ID and generates a server ID instead', async () => {
+    const app = buildApp();
+    const res = await request(app).get('/echo').set('X-Request-ID', 'evil.example.com/path');
+
+    expect(res.headers['x-request-id']).not.toBe('evil.example.com/path');
+    expect(res.headers['x-request-id']).toMatch(/^req_[0-9A-Za-z_-]+$/);
+  });
+
+  test('a rejected client value is not reflected into error response bodies', async () => {
+    const app = buildApp();
+    const res = await request(app).post('/boom').set('X-Request-ID', '::malformed::');
+
+    expect(res.body.error.request_id).toMatch(/^req_[0-9A-Za-z_-]+$/);
+    expect(res.body.error.request_id).not.toBe('::malformed::');
+  });
 });
\ No newline at end of file
diff --git a/test/webhookDispatcher.test.js b/test/webhookDispatcher.test.js
index b176252..0a6e8df 100644
--- a/test/webhookDispatcher.test.js
+++ b/test/webhookDispatcher.test.js
@@ -9,6 +9,13 @@ jest.mock('../src/services/cache', () => mockHelper.cacheMock);
 jest.mock('../src/logger', () => ({
   info: jest.fn(), warn: jest.fn(), error: jest.fn(), debug: jest.fn(),
 }));
+// Passthrough SSRF guard so dispatcher tests don't perform live DNS resolution
+// and so the original (un-pinned) URL reaches the axios mock as before.
+jest.mock('../src/services/ssrfGuard', () => ({
+  assertPublicTarget: jest.fn(async (url) => ({ targetUrl: url, host: null })),
+  assertPublicUrlSync: jest.fn((url) => url),
+  isPrivateIp: jest.fn(() => false),
+}));
 
 const mockAxiosPost = jest.fn();
 jest.mock('axios', () => ({ post: (...args) => mockAxiosPost(...args) }));
diff --git a/test/webhooks.routes.test.js b/test/webhooks.routes.test.js
index 4a17d0f..a17c40b 100644
--- a/test/webhooks.routes.test.js
+++ b/test/webhooks.routes.test.js
@@ -15,6 +15,15 @@ jest.mock('../src/logger', () => ({
 const mockAxiosPost = jest.fn();
 jest.mock('axios', () => ({ post: (...args) => mockAxiosPost(...args) }));
 
+// Mock the SSRF guard so registration/delivery-time target validation does not
+// perform live DNS resolution in unit tests (the real guard's behavior is
+// covered by test/webhooks.ssrf.test.js).
+jest.mock('../src/services/ssrfGuard', () => ({
+  assertPublicTarget: jest.fn(async (url) => ({ targetUrl: url, host: null })),
+  assertPublicUrlSync: jest.fn((url) => url),
+  isPrivateIp: jest.fn(() => false),
+}));
+
 const webhooksRouter = require('../src/routes/webhooks');
 
 function buildApp() {
diff --git a/test/webhooks.ssrf.test.js b/test/webhooks.ssrf.test.js
new file mode 100644
index 0000000..88f6390
--- /dev/null
+++ b/test/webhooks.ssrf.test.js
@@ -0,0 +1,159 @@
+'use strict';
+
+// Keep the test rate-limit budget small so we can prove that testing many
+// *different* webhooks from one IP does NOT bypass the per-IP limit. Must be
+// set before config is required below.
+process.env.WEBHOOK_TEST_RATELIMIT_MAX = '5';
+process.env.WEBHOOK_TEST_RATELIMIT_WINDOW = '60';
+
+const express = require('express');
+const request = require('supertest');
+const { createCacheMock } = require('./helpers/cacheMock');
+
+const mockHelper = createCacheMock();
+const { reset } = mockHelper;
+
+jest.mock('../src/services/cache', () => mockHelper.cacheMock);
+
+const mockAxiosPost = jest.fn();
+jest.mock('axios', () => ({ post: (...args) => mockAxiosPost(...args) }));
+
+const webhooksRouter = require('../src/routes/webhooks');
+const webhookRepo = require('../src/repositories/webhookRepository');
+const logger = require('../src/logger');
+const { errorHandler, notFoundHandler } = require('../src/middleware/errorHandler');
+
+// A real, public IP literal (example.com's historical address) — used so the
+// SSRF guard's resolution path is exercised without any live DNS lookup.
+const PUBLIC_TARGET = 'http://93.184.216.34/';
+
+function buildApp() {
+  const app = express();
+  app.use(express.json());
+  app.use('/api/v1', webhooksRouter);
+  app.use(notFoundHandler);
+  app.use(errorHandler);
+  return app;
+}
+
+async function seedWebhook(url) {
+  return webhookRepo.create({
+    url,
+    events: ['*'],
+    secret: 'whsec_aaaaaaaaaaaaaaaa',
+  });
+}
+
+beforeEach(() => {
+  reset();
+  mockAxiosPost.mockReset();
+});
+
+describe('webhook SSRF guard (#96)', () => {
+  const app = buildApp();
+
+  test.each([
+    ['RFC1918', 'http://10.0.0.5/'],
+    ['loopback', 'http://127.0.0.1/'],
+    ['link-local cloud metadata', 'http://169.254.169.254/latest/meta-data/'],
+    ['IPv6 loopback', 'http://[::1]/'],
+  ])('refuses testing a webhook targeting a private/internal address (%s)', async (label, url) => {
+    const webhook = await seedWebhook(url);
+
+    const res = await request(app).post(`/api/v1/webhooks/${webhook.id}/test`);
+
+    expect(res.status).toBe(422);
+    expect(res.body.error.code).toBe('WEBHOOK_TARGET_BLOCKED');
+    expect(mockAxiosPost).not.toHaveBeenCalled();
+  });
+
+  test('blocks a private target even when seeded directly, bypassing create-time validation (defense in depth)', async () => {
+    // The route-level schema would reject a private URL at POST /webhooks, so
+    // seed straight into the repository to simulate a bypass/refactor gap and
+    // prove the delivery-time check is what actually stops it.
+    const webhook = await seedWebhook('http://192.168.1.1/');
+    const res = await request(app).post(`/api/v1/webhooks/${webhook.id}/test`);
+
+    expect(res.status).toBe(422);
+    expect(res.body.error.code).toBe('WEBHOOK_TARGET_BLOCKED');
+  });
+
+  test('allows testing a public target and reports success', async () => {
+    const webhook = await seedWebhook(PUBLIC_TARGET);
+    mockAxiosPost.mockResolvedValueOnce({ status: 200 });
+
+    const res = await request(app).post(`/api/v1/webhooks/${webhook.id}/test`);
+
+    expect(res.status).toBe(202);
+    expect(res.body.status).toBe('success');
+  });
+});
+
+describe('webhook test endpoint error detail reduction (#96)', () => {
+  const app = buildApp();
+  let warnSpy;
+
+  beforeEach(() => {
+    warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
+  });
+  afterEach(() => {
+    warnSpy.mockRestore();
+  });
+
+  test('does not leak raw network error codes; returns a generic category but logs the raw detail', async () => {
+    const webhook = await seedWebhook(PUBLIC_TARGET);
+    mockAxiosPost.mockRejectedValueOnce(new Error('ECONNREFUSED'));
+
+    const res = await request(app).post(`/api/v1/webhooks/${webhook.id}/test`);
+
+    expect(res.status).toBe(202);
+    expect(res.body.last_error).toBe('unreachable');
+    expect(String(res.body.last_error)).not.toContain('ECONNREFUSED');
+
+    // Raw low-level detail is preserved server-side for operators.
+    const loggedRaw = warnSpy.mock.calls.some(([msg, meta]) =>
+      String(msg).toLowerCase().includes('failed') &&
+      meta && String(meta.error).includes('ECONNREFUSED'));
+    expect(loggedRaw).toBe(true);
+  });
+
+  test('does not leak raw HTTP error strings; returns a generic category', async () => {
+    const webhook = await seedWebhook(PUBLIC_TARGET);
+    mockAxiosPost.mockResolvedValueOnce({ status: 500 });
+
+    const res = await request(app).post(`/api/v1/webhooks/${webhook.id}/test`);
+
+    expect(res.status).toBe(202);
+    expect(res.body.last_error).toBe('error_response');
+    expect(String(res.body.last_error)).not.toMatch(/^HTTP /);
+  });
+});
+
+describe('webhook test rate-limit aggregates per IP (#96)', () => {
+  const app = buildApp();
+
+  test('testing N different webhooks from one IP shares a single budget', async () => {
+    const webhooks = [];
+    for (let i = 0; i < 10; i += 1) {
+      // Distinct public targets so the SSRF guard passes for each.
+      webhooks.push(await seedWebhook(`http://93.184.216.${34 + i % 3}/`));
+    }
+    mockAxiosPost.mockResolvedValue({ status: 200 });
+
+    const results = [];
+    for (const wh of webhooks) {
+      results.push(await request(app).post(`/api/v1/webhooks/${wh.id}/test`));
+    }
+
+    const succeeded = results.filter((r) => r.status === 202).length;
+    const limited = results.filter((r) => r.status === 429).length;
+
+    // Budget is 5 per IP per window — testing 10 *different* webhooks must not
+    // each get their own allowance; only 5 succeed, the rest are limited.
+    expect(succeeded).toBe(5);
+    expect(limited).toBe(5);
+    results.filter((r) => r.status === 429).forEach((r) => {
+      expect(r.body.error.code).toBe('RATE_LIMITED');
+    });
+  });
+});