diff --git a/config/defaults/development.config.js b/config/defaults/development.config.js index badc71e93..b05d7fccf 100644 --- a/config/defaults/development.config.js +++ b/config/defaults/development.config.js @@ -81,6 +81,22 @@ const config = { maxFiles: 2, json: false, }, + // Redaction extension points (optional — do NOT declare either key here, + // not even as `[]`): a module or project config may set + // `log.sensitivePathMarkers` / `log.sensitiveQueryKeys` (array of + // strings) to extend the built-in redaction lists consumed by + // `lib/helpers/redactUrl.js` (`redactPathSecrets` for PATH segments e.g. + // `/api/auth/reset/:token`; `redactUrl` for query params e.g. + // `?inviteToken=…`). Values are UNIONED with the built-in defaults there + // (['reset', 'verify', 'verify-email'] / ['inviteToken']), never + // replacing them. + // + // These two keys are intentionally OMITTED from this file: `deepMerge` + // (config/index.js) replaces arrays wholesale rather than merging them, + // so an explicit value here (Layer 2: global defaults) would silently + // clobber a module's own extension of the same key (Layer 1: module + // config) instead of unioning with it. An omitted key lets a Layer-1 + // value pass through untouched to redactUrl.js. }, csrf: { csrf: false, diff --git a/lib/helpers/redactUrl.js b/lib/helpers/redactUrl.js index eb35dd9c4..d92dfd537 100644 --- a/lib/helpers/redactUrl.js +++ b/lib/helpers/redactUrl.js @@ -1,3 +1,43 @@ +/** + * Module dependencies. + */ +import config from '../../config/index.js'; + +/** + * Built-in, authoritative base lists. `lib/` must stay module-agnostic, so a + * module/downstream project never edits this shared helper to add its own + * token-bearing routes — it extends `config.log.sensitiveQueryKeys` / + * `config.log.sensitivePathMarkers` (see below) instead. These built-ins + * ALWAYS apply, unioned with whatever config contributes: config can only + * ADD markers/keys, never remove or replace these, so redaction can never be + * silently disabled (an absent key or an explicit empty array in config both + * degrade to exactly these defaults, never to nothing). + */ +const DEFAULT_SENSITIVE_QUERY_KEYS = ['inviteToken']; +const DEFAULT_SENSITIVE_PATH_MARKERS = ['reset', 'verify', 'verify-email']; + +/** + * @desc Sanitize a config-provided redaction list so it is always an array + * before it is spread into the union below. A config typo (an object, a + * number, a bare string, …) must never throw at module-eval time — that + * would crash the whole app on boot — and must never silently degrade into + * a per-character array either (spreading a string splits it into individual + * characters, which is not the extension list a config author intended and + * would slip straight past an `Array.isArray`-less spread). Falls back to + * an empty array, which is a no-op for the union below (identical to an + * absent/missing key — the built-in defaults still apply), and logs a + * warning identifying the offending config path so the typo gets noticed. + * @param {*} value - the raw `config.log.` value + * @param {String} label - dotted config path for the warning message, e.g. 'log.sensitivePathMarkers' + * @returns {Array} `value` unchanged when it is already an array, otherwise `[]` + */ +const sanitizeConfigList = (value, label) => { + if (Array.isArray(value)) return value; + if (value == null) return []; + console.warn(`[redactUrl] config.${label} ignored: expected an array, got ${typeof value}`); + return []; +}; + /** * Sensitive query-string parameters that must never reach the request log. * @@ -5,8 +45,15 @@ * (the Vue client puts it there, not in the body). The morgan log pattern logs * `:url`, so without redaction a live single-use invite token lands in prod logs * (and any log shipper / aggregator downstream). Redact it to `REDACTED`. + * + * Union of `DEFAULT_SENSITIVE_QUERY_KEYS` and `config.log.sensitiveQueryKeys`: + * a module/downstream project extends the set purely additively from its own + * config, without editing this shared helper. Deduplicated via `Set`. Missing + * config, a missing key, an explicit empty array, or a malformed (non-array) + * value all resolve to exactly the built-in defaults — config can never + * shrink or clobber this list. */ -const SENSITIVE_QUERY_KEYS = ['inviteToken']; +const SENSITIVE_QUERY_KEYS = [...new Set([...DEFAULT_SENSITIVE_QUERY_KEYS, ...sanitizeConfigList(config?.log?.sensitiveQueryKeys, 'log.sensitiveQueryKeys')])]; /** * Path segments that are immediately followed by a single-use secret carried as @@ -15,8 +62,15 @@ const SENSITIVE_QUERY_KEYS = ['inviteToken']; * `GET /api/*(auth/)?invitations/verify/:token` embed a still-valid token * directly in the path, so the segment right after one of these markers must be * redacted before the URL reaches any log/analytics store. + * + * Union of `DEFAULT_SENSITIVE_PATH_MARKERS` and `config.log.sensitivePathMarkers`: + * a module/downstream project extends the set purely additively from its own + * config, without editing this shared helper. Deduplicated via `Set`. Missing + * config, a missing key, an explicit empty array, or a malformed (non-array) + * value all resolve to exactly the built-in defaults — config can never + * shrink or clobber this list. */ -const SENSITIVE_PATH_MARKERS = ['reset', 'verify', 'verify-email']; +const SENSITIVE_PATH_MARKERS = [...new Set([...DEFAULT_SENSITIVE_PATH_MARKERS, ...sanitizeConfigList(config?.log?.sensitivePathMarkers, 'log.sensitivePathMarkers')])]; /** * @desc Redact single-use secrets embedded as PATH parameters. Any segment that diff --git a/lib/helpers/tests/redactUrl.unit.tests.js b/lib/helpers/tests/redactUrl.unit.tests.js index d069382c8..434508866 100644 --- a/lib/helpers/tests/redactUrl.unit.tests.js +++ b/lib/helpers/tests/redactUrl.unit.tests.js @@ -1,5 +1,6 @@ -import { describe, test, expect } from '@jest/globals'; +import { jest, describe, test, expect } from '@jest/globals'; import redactUrl, { SENSITIVE_QUERY_KEYS, SENSITIVE_PATH_MARKERS, redactPathSecrets } from '../redactUrl.js'; +import developmentConfig from '../../../config/defaults/development.config.js'; describe('redactUrl', () => { test('redacts an inviteToken value, preserving the path', () => { @@ -113,3 +114,131 @@ describe('redactPathSecrets', () => { expect(SENSITIVE_PATH_MARKERS).toEqual(expect.arrayContaining(['reset', 'verify', 'verify-email'])); }); }); + +/** + * `SENSITIVE_QUERY_KEYS` / `SENSITIVE_PATH_MARKERS` are read from + * `config.log.*` once, at module-evaluation time (#3953), and UNIONED with + * the built-in `DEFAULT_SENSITIVE_*` lists — config is extension-only, it can + * never shrink or clobber the built-ins (deepMerge replaces arrays wholesale, + * so a naive fallback would let a module-level config array silently clobber + * the global default instead of extending it; union sidesteps that entirely). + * These tests mock `config/index.js` and re-import the module fresh (via + * `jest.resetModules()` + dynamic `import()` — see + * `posthog-context.middleware.unit.tests.js` for the same pattern) to + * exercise the union, the empty-array no-op, and the absent-config fallback, + * without disturbing the static-import tests above (which exercise the real + * config's current defaults, unchanged). + */ +describe('redactUrl config sourcing (#3953)', () => { + /** + * Load a fresh instance of the module under a given mocked config. + * @param {Object|undefined} mockConfig - the value `config/index.js` default-exports + * @returns {Promise} the freshly-loaded module exports + */ + const loadWithConfig = async (mockConfig) => { + jest.resetModules(); + jest.unstable_mockModule('../../../config/index.js', () => ({ default: mockConfig })); + return import('../redactUrl.js'); + }; + + test('unions a module-extended path marker from config with the built-in defaults', async () => { + const mod = await loadWithConfig({ + log: { sensitivePathMarkers: ['unsubscribe'] }, + }); + expect(mod.SENSITIVE_PATH_MARKERS).toEqual(expect.arrayContaining(['reset', 'verify', 'verify-email', 'unsubscribe'])); + // built-in marker still redacts — extension never clobbers it + expect(mod.redactPathSecrets('/api/auth/reset/SECRETTOKEN')).toBe('/api/auth/reset/REDACTED'); + // config-added marker redacts too + expect(mod.redactPathSecrets('/api/newsletter/unsubscribe/SECRETTOKEN')).toBe('/api/newsletter/unsubscribe/REDACTED'); + }); + + test('unions a module-extended query key from config with the built-in defaults', async () => { + const mod = await loadWithConfig({ + log: { sensitiveQueryKeys: ['magicToken'] }, + }); + expect(mod.SENSITIVE_QUERY_KEYS).toEqual(expect.arrayContaining(['inviteToken', 'magicToken'])); + // built-in key still redacts — extension never clobbers it + expect(mod.default('/x?inviteToken=secret')).toBe('/x?inviteToken=REDACTED'); + // config-added key redacts too + expect(mod.default('/x?magicToken=secret')).toBe('/x?magicToken=REDACTED'); + }); + + test('an explicit empty array in config is a no-op — built-in defaults still apply', async () => { + const mod = await loadWithConfig({ + log: { sensitivePathMarkers: [], sensitiveQueryKeys: [] }, + }); + expect(mod.SENSITIVE_PATH_MARKERS).toEqual(['reset', 'verify', 'verify-email']); + expect(mod.SENSITIVE_QUERY_KEYS).toEqual(['inviteToken']); + expect(mod.redactPathSecrets('/api/auth/reset/SECRETTOKEN')).toBe('/api/auth/reset/REDACTED'); + expect(mod.default('/x?inviteToken=secret')).toBe('/x?inviteToken=REDACTED'); + }); + + test('falls back to the built-in defaults when config.log is missing', async () => { + const mod = await loadWithConfig({}); + expect(mod.SENSITIVE_PATH_MARKERS).toEqual(['reset', 'verify', 'verify-email']); + expect(mod.SENSITIVE_QUERY_KEYS).toEqual(['inviteToken']); + expect(mod.redactPathSecrets('/api/auth/reset/SECRETTOKEN')).toBe('/api/auth/reset/REDACTED'); + expect(mod.default('/x?inviteToken=secret')).toBe('/x?inviteToken=REDACTED'); + }); + + test('falls back to the built-in defaults when config itself is undefined', async () => { + const mod = await loadWithConfig(undefined); + expect(mod.SENSITIVE_PATH_MARKERS).toEqual(['reset', 'verify', 'verify-email']); + expect(mod.SENSITIVE_QUERY_KEYS).toEqual(['inviteToken']); + }); + + /** + * A shape mismatch on the config value (e.g. a typo'd object instead of an + * array) must degrade the same way an absent/empty value does — never throw + * at module-eval time (`[...{}]` is not iterable, which would crash the + * whole app on boot) and never silently spread a string char-by-char + * (`[...'oops']` → `['o','o','p','s']`, which would over-redact app-wide). + */ + test('an object value for sensitivePathMarkers is ignored — defaults still apply, no throw, warns', async () => { + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const mod = await loadWithConfig({ + log: { sensitivePathMarkers: { notAnArray: true } }, + }); + expect(mod.SENSITIVE_PATH_MARKERS).toEqual(['reset', 'verify', 'verify-email']); + expect(mod.redactPathSecrets('/api/auth/reset/SECRETTOKEN')).toBe('/api/auth/reset/REDACTED'); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('log.sensitivePathMarkers')); + } finally { + warnSpy.mockRestore(); + } + }); + + test('a string value for sensitiveQueryKeys is ignored (not spread char-by-char) — defaults still apply, no throw, warns', async () => { + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const mod = await loadWithConfig({ + log: { sensitiveQueryKeys: 'oops' }, + }); + expect(mod.SENSITIVE_QUERY_KEYS).toEqual(['inviteToken']); + expect(mod.default('/x?inviteToken=secret')).toBe('/x?inviteToken=REDACTED'); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('log.sensitiveQueryKeys')); + } finally { + warnSpy.mockRestore(); + } + }); +}); + +/** + * Lockstep guard (#3953): `config/defaults/development.config.js` must never + * declare `log.sensitivePathMarkers` / `log.sensitiveQueryKeys` again, not + * even as `[]`. `deepMerge` (config/index.js) replaces arrays wholesale + * instead of merging them, so a Layer-2 (global defaults) value — including + * an explicit empty array — would silently clobber a Layer-1 (module config) + * extension of the same key. Omitting the key entirely is what lets a + * module's value pass through untouched to `lib/helpers/redactUrl.js`. This + * test pins that fix against regression. + */ +describe('development.config.js lockstep guard (#3953)', () => { + test('does not declare log.sensitivePathMarkers', () => { + expect(Object.prototype.hasOwnProperty.call(developmentConfig.log, 'sensitivePathMarkers')).toBe(false); + }); + + test('does not declare log.sensitiveQueryKeys', () => { + expect(Object.prototype.hasOwnProperty.call(developmentConfig.log, 'sensitiveQueryKeys')).toBe(false); + }); +});