Security hardening: docs CSP, X-Request-ID validation, log redaction, webhook SSRF guard

README.md

@@ -784,6 +784,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
@@ -869,11 +884,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

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');

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 };

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,
 };

src/routes/webhooks.js

@@ -63,6 +63,23 @@ router.get("/webhooks/metrics", async (req, res) => {
   return res.json(dispatcher.getMetrics());
 });
 
+/**
+ * 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 {

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 };