Log redaction only matches top-level-ish substring keys — misses arrays, nested structures, and secrets embedded in URLs

[prodbycorne]

Overview

src/logger.js's redactFormat walks logged metadata recursively but only redacts a value when its object key contains one of a fixed set of substrings:

const redactFormat = winston.format((info) => {
  const sensitiveKeys = ['apikey', 'privatekey', 'secret', 'token'];
  const redactValue = (value, key) => { ... };
  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);
});

Several real gaps in this approach, given how this codebase actually logs things:

1. Arrays are skipped. redact() recurses into obj[key] when typeof obj[key] === 'object' — which is true for arrays too, so it does recurse, but it then calls Object.keys() on an array, which returns numeric string indices ('0', '1', ...), none of which match any sensitiveKeys substring, so a value like logger.info('Bulk create', { keys: [{ api_key: 'sk_live_...' }] }) — an array of objects each containing a sensitive key — is correctly redacted per-element (since each array element is itself an object with a key matching apikey), but a plain array of raw secret strings (e.g. accidentally logging { secrets: ['whsec_...', 'whsec_...'] }) is not redacted at all, since the array indices themselves never match sensitiveKeys, only nested object keys do.
2. Secrets embedded in already-serialized strings are invisible. routes/webhooks.js's POST /webhooks handler logs nothing sensitive today directly, but this codebase logs full URLs in several places (e.g. any future logging of webhook.url, or alert.webhook_url) — a URL like https://example.com/hook?token=abc123 contains a secret inside a string value, under a key like url, which does not match sensitiveKeys ('url' contains none of apikey/privatekey/secret/token... actually wait, 'url' doesn't match, but if the field were named e.g. webhook_url it still wouldn't match). The redaction logic operates purely on key names and has no capability to redact a substring pattern (like a token= query parameter or an embedded whsec_/sk_-prefixed value) found inside an otherwise-innocuously-named field's string value.
3. Case and separator variations aren't tested. sensitiveKeys.some(k => lowerKey.includes(k)) does correctly catch API_Key, apiKey, etc. (since it lowercases first), but there is no test file covering logger.js's redaction behavior at all in the current test/ directory, so this behavior is entirely unverified by CI, and it's easy to silently regress if the format/transports are ever refactored (issue Add log rotation and production log configuration #37 asked for exactly this logger to be introduced, but did not include redaction-specific tests).

Requirements

• Extend redactFormat to also scan array elements for sensitive string values matching known secret prefixes/patterns (e.g. whsec_, hex API keys of known length, sha256=/sha1= signature-looking strings) rather than relying solely on the containing key's name.
• Add pattern-based redaction for string values (regardless of key name) that match known secret shapes used elsewhere in this codebase (whsec_[hex], the 32-byte-hex ADMIN_API_KEY/generated API key format from apiKeys.generateApiKey(), and query-string token=/secret=/key= parameters embedded in logged URLs).
• Add a dedicated test file (test/logger.test.js or similar — none exists today) that exercises the redaction format directly against representative log payloads mirroring real shapes used elsewhere in the app (webhook secrets, API keys, URLs with embedded credentials, arrays of secret-shaped strings).
• Ensure the fix does not break the existing whsec_****-style partial-reveal behavior intentionally used for webhook secrets (redactValue's special case), and extend that same partial-reveal treatment to API key prefixes for operator debuggability, consistent with how key_prefix is already surfaced non-secretly elsewhere in the API (apiKeys.js's sanitize()).

Acceptance Criteria

• A logged array containing raw secret-shaped strings (not wrapped in an object) has those values redacted.
• A logged URL string containing a token=/secret=/key= query parameter has that parameter's value redacted, even though the containing field's key name (e.g. url) doesn't match the existing key-name substring list.
• Existing whsec_**** partial-reveal behavior for webhook secrets is preserved.
• A new test/logger.test.js covers all of the above scenarios plus the previously-untested nested-object and mixed-case-key cases.
• No legitimate, non-sensitive log fields are redacted as a false positive (test asserts normal fields like asset_code, price_usd, delivery_id pass through unredacted).

Additional Notes

More precise references

• src/logger.js:18-46 (redactFormat): confirmed exact structure — sensitiveKeys = ['apikey', 'privatekey', 'secret', 'token']; redact() recurses via Object.keys(obj) on anything typeof obj[key] === 'object', which is true for arrays (typeof [] === 'object') but Object.keys([...]) yields numeric-string indices that never match sensitiveKeys, confirming the issue's claim precisely — a top-level array of raw secret strings is genuinely never redacted, while an array of objects with sensitive keys is redacted per-element (since each element is itself walked as an object).
• src/logger.js:21-27 (redactValue): confirmed the whsec_ partial-reveal special case is keyed on key.toLowerCase().includes('secret') — meaning the partial-reveal only triggers when the containing key has "secret" in its name, not based on the value's own shape; a whsec_... value under an unrelated key name (e.g. { whsecValue: 'whsec_abc' } — contrived but illustrative) would get the generic '[REDACTED]' rather than the partial-reveal treatment, and conversely, a non-whsec_-prefixed value under a secret-named key still gets the safe generic '[REDACTED]' — behavior is correct today for the exact whsec_ case, just narrowly keyed.
• src/services/webhookSignature.js:32-34 (generateSecret): confirmed whsec_ prefix format (whsec_${crypto.randomBytes(bytes).toString('hex')}) — this is the canonical secret shape the new pattern-based redaction needs to recognize.
• src/services/apiKeys.js:19-21 (generateApiKey): confirmed the API key format is crypto.randomBytes(32).toString('hex') — a bare 64-character hex string with no distinguishing prefix at all, unlike whsec_. This is a materially harder pattern to safely regex-match in arbitrary log strings, since a 64-char hex string could coincidentally appear in many contexts (a Stellar transaction hash, a SHA-256 digest already intentionally logged elsewhere, a UUID with dashes stripped, etc.) — a naive "redact any 64-hex-char substring" rule risks a high false-positive rate against legitimate non-secret hex values already logged elsewhere in this codebase (e.g. delivery IDs, event IDs). This needs to be flagged as a real design tension in the PR, not glossed over — likely the safer approach is to redact API-key values specifically only when found under a key name matching the existing sensitiveKeys list (already covered) plus specifically the literal Authorization: Bearer <token> header-string pattern if headers are ever logged wholesale, rather than a blanket bare-hex regex.
• src/middleware/auth.js:29 — confirmed logger.warn('Rejected API key authentication', { key_prefix: token.slice(0, 8) }) already deliberately logs only an 8-character prefix, not the full token — a good existing example of "safe partial reveal by construction" rather than relying on redaction to catch a full-token log after the fact; worth citing as the preferred pattern (redact-at-the-source) alongside the redaction-format safety net this issue improves.

Additional edge cases

• Query-string redaction (token=/secret=/key= embedded in URLs) needs to handle URL-encoding — a secret value containing characters that get percent-encoded in a URL (unlikely for hex secrets, but the pattern-matching regex should be tested against at least one realistic encoded case) and multiple query params in one URL (?token=abc&foo=bar&key=def — both should be redacted, not just the first match, meaning any regex used needs a global flag and must not stop at the first hit).
• Nested arrays-of-arrays or arrays inside objects inside arrays — the fix needs a recursive walk that treats array elements as walkable nodes on equal footing with object properties, not just "arrays of objects" as a special case; a plain array of plain strings needs its own value-scanning pass (pattern-based, not key-based, since array indices have no meaningful "key name").
• Winston's info object itself (the argument passed into redactFormat) has some non-enumerable or special properties (e.g. Symbol.for('message'), Symbol.for('level') used internally by winston) — the recursive redact() walk should be verified not to choke on or inadvertently mutate these when extended, since Object.keys() already correctly skips symbol keys, but any rewrite should preserve that safety.
• Performance: pattern-matching every string value in every log line (rather than only checking object keys) is strictly more expensive per log call; for a debug-level-heavy path (e.g. inside priceOracle.js's per-asset per-cycle logging) this could add measurable overhead at scale — worth a brief benchmark note in the PR, though correctness should still take priority over micro-optimization here.

Implementation sketch

1. Extend redact() to explicitly detect arrays (Array.isArray(obj[key])) and, for each element: if it's an object, recurse as today; if it's a string, run it through a new scanValueForSecretPatterns(str) function; if it's neither, leave alone.
2. scanValueForSecretPatterns(str): a small set of regexes — /whsec_[0-9a-f]{16,}/gi (webhook secrets, safe to match broadly since the prefix is distinctive), /([?&](?:token|secret|key)=)([^&\s]+)/gi (query-string params, replace capture group 2 with [REDACTED]), and explicitly not a bare-hex-string rule for raw API keys, per the false-positive concern above — instead, extend the key-name-based path to also catch Authorization (case-insensitive) as an additional sensitive key name, covering the common case of an accidentally-logged full header object.
3. Apply scanValueForSecretPatterns uniformly to all string values during the recursive walk (not just array elements) so a URL embedded under an innocuous key name like url or webhook_url also gets its query-string secrets stripped, per requirement 2 in the issue.
4. Preserve the existing whsec_****-style partial reveal: change the array/pattern-based path to also apply the same partial-reveal transform (first 10 chars + ****) rather than a blanket [REDACTED], extending it to bare hex API keys only when a key-name match already indicates it's sensitive (i.e., the existing keyed path gets the partial-reveal upgrade too, consistent with apiKeys.js's key_prefix already being treated as safely non-secret elsewhere in the API).
5. New test/logger.test.js exercising: nested objects (regression), mixed-case keys (regression, currently untested per the issue), array of sensitive-shaped strings (new), array of objects with sensitive keys (regression, confirm still works), URL with embedded token= param under a non-matching key name (new), whsec_ partial-reveal preserved (regression), non-sensitive fields (asset_code, price_usd, delivery_id) passed through unredacted (new, explicit false-positive guard).

Test/reproduction plan

See the dedicated test file described above; additionally:

• A direct call to the exported redactFormat transform function (winston formats are callable/testable in isolation without needing a full logger instance) with a fixture object mirroring real shapes from this codebase: { webhook: { secret: 'whsec_abcdef0123456789' }, secrets: ['whsec_zzz1111111111111', 'whsec_zzz2222222222222'], url: 'https://x.com/hook?token=supersecret123&other=1', delivery_id: 'del_abc123', asset_code: 'XLM' } — assert the two array secrets are redacted (partial-reveal), the URL's token= value is redacted but the rest of the URL and other=1 are preserved, and delivery_id/asset_code pass through verbatim.
• Regression run of any pre-existing logger-adjacent tests (there don't appear to be any dedicated ones today per the issue, but check test/ broadly for incidental coverage via snapshot tests of log output in other test files).

Cross-references

• Admin API key comparison uses non-constant-time string equality — timing side-channel #89 (constant-time admin key comparison) and this issue both deal with the ADMIN_API_KEY/generated-API-key secret shape; the false-positive-risk analysis above (bare 64-hex-char strings being ambiguous) is directly relevant to anyone implementing Admin API key comparison uses non-constant-time string equality — timing side-channel #89's constantTimeEquals helper if it's ever accidentally logged during debugging — worth a shared awareness note, no code dependency.
• Webhook test endpoint can be abused as a blind SSRF timing/status oracle to probe internal network services #96 (webhook SSRF oracle) and Dispatcher webhook signatures have no timestamp or nonce — signed payloads are permanently replayable #97 (webhook signature replay) both involve webhook secrets/signatures that could end up in logs (e.g. a future debug log of a full delivery attempt including headers) — this issue's improved redaction is a relevant safety net for both, though neither directly depends on this landing first.
• Missing production metrics for webhook delivery outcomes, retry-queue depth, and price-source failure rates #93 (metrics) — metrics labels and log fields are a similar "don't leak/blow up on user-controlled values" class of problem (that issue's own cardinality-safety requirement for webhook_id vs raw URLs mirrors this issue's key-name-vs-pattern-matching tension for secrets) — no code dependency, but worth referencing as a consistent codebase-wide principle in both PRs' descriptions.