diff --git a/CHANGELOG.md b/CHANGELOG.md index 01dcc2e..dd55842 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,17 @@ ## [Unreleased] +### Security + +- **`redactPaths` can no longer write through the prototype chain or into your application's objects** (`src/request-middleware.ts`, `README.md`; tests in `tests/request-middleware.spec.ts`). Resolves the CodeQL `js/prototype-polluting-assignment` finding (CWE-1321) on the path walker. The walker read each path segment as `container[segment]`, which follows inherited properties and runs getters, and it checked only the final target before writing. The `__proto__` / `constructor` / `prototype` deny-list kept it off `Object.prototype` itself, but three real defects remained: + - A path through an object parked on `Object.prototype` (by a library, or in an already polluted process) reached that one shared object and overwrote its field for every object in the process. + - A path through a response-header value kept by reference, such as a class instance with its own `toJSON()` or a `Date` carrying an own object field (`res.setHeader` stores non-string values as they are), reached a plain object your application owned and overwrote its field in the running app. With `redactPaths: ["responseHeaders.x-meta.inner.token"]`, `inner.token` read `"[REDACTED]"` after the request was logged. + - A getter on the path ran (application code during logging), and one that threw escaped the path pass, so the whole request went unlogged. + - The walker now steps through and writes into only the containers the package owns (arrays and objects whose prototype is exactly `Object.prototype`), descends from the entry only into the fields it copies (`requestBody`, `requestHeaders`, `responseHeaders`, `context`; a one-segment path such as `url` still redacts that field), reads only own data properties through their descriptors (never an inherited property, never an accessor, and the descriptor's own `value` / `writable`, so a polluted `Object.prototype.value` cannot make an accessor look writable and get its setter called), refuses `__proto__`, `constructor` and `prototype` on every segment with literal comparisons, and never throws, whatever the path holds. Every path that redacted before still redacts the same copy; a path that runs into a value kept by reference now stops there. If such a header's `toJSON()` printed the field the old write overwrote, the log line hid it only by corrupting your object; mask that header as a whole with `maskHeaderKeys` instead. + - `includeRequestHeaders` / `includeResponseHeaders` allow-lists now read only headers the request or response actually has. A listed header it lacked picked up a same-named property inherited from a polluted `Object.prototype`, logged it as that header, and passed the shared object to the path pass. + - A non-string entry in the `redactPaths` array is ignored. The middleware keeps using your array, so a path added after it was created still applies, but the construction-time check cannot see such an entry, and a non-string one made every request go unlogged. + - No option or output format changes. + ## [1.2.0] - 2026-09-25 ### Changed diff --git a/README.md b/README.md index a262799..3c6fe42 100644 --- a/README.md +++ b/README.md @@ -877,7 +877,7 @@ narrowly scoped `redactPaths` API. - `api_key`, `apikey`, `key` - `code` (covers OAuth callback codes) - `secret`, `password` -- **Surgical path redaction (`redactPaths`)**: opt-in array of dot-notation paths into the resolved entry (e.g. `["body.user.password"]`). The leading `body` segment is rewritten to `requestBody` to match the public field name. Missing intermediate keys are a graceful no-op — we never create new sub-paths just to write `[REDACTED]`. Body-scoped paths (`body.*` / `requestBody.*`) are applied to the object graph BEFORE the `maxBodyLength` truncation decision, so a secret targeted only by `redactPaths` (with no corresponding `maskBodyKeys` entry) cannot survive inside the `_preview` field of an over-limit body's truncation envelope. The request body, the `enrich()` context, and any captured request/response headers are all redacted on internal owned copies — even when the body or context is a class instance that defines its own `toJSON()` (a Mongoose document, a DTO, `req.user`) — and each `redactPaths` write is additionally confined to a package-owned plain object or array, so your original `req.body` / `req.session` / `req.user` / `req.headers` objects (and the live response-header values behind `res.getHeaders()`) are never mutated by a `redactPaths` write. Path redaction is also best-effort and never throws: a path that targets an array's `length`, a frozen or getter-only property, or any other slot whose write would raise is skipped rather than allowed to drop the entire log entry. +- **Surgical path redaction (`redactPaths`)**: opt-in array of dot-notation paths into the resolved entry (e.g. `["body.user.password"]`). The leading `body` segment is rewritten to `requestBody` to match the public field name. Missing intermediate keys are a graceful no-op — we never create new sub-paths just to write `[REDACTED]`. Body-scoped paths (`body.*` / `requestBody.*`) are applied to the object graph BEFORE the `maxBodyLength` truncation decision, so a secret targeted only by `redactPaths` (with no corresponding `maskBodyKeys` entry) cannot survive inside the `_preview` field of an over-limit body's truncation envelope. The request body, the `enrich()` context, and any captured request/response headers are all redacted on internal owned copies — even when the body or context is a class instance that defines its own `toJSON()` (a Mongoose document, a DTO, `req.user`) — and a path only ever steps through and writes into those package-owned plain objects and arrays, following their own data properties (never an inherited property, and never a getter, which is not run). So your original `req.body` / `req.session` / `req.user` / `req.headers` objects (and the live response-header values behind `res.getHeaders()`) are never mutated by a `redactPaths` write, and no path can reach `Object.prototype`: `__proto__`, `constructor` and `prototype` segments are refused. A header value the entry keeps by reference (a `Date`, a class instance with its own `toJSON()`, a `Buffer`, a `Map`) ends the path, because everything behind it is your application's state; to hide such a header, list it in `maskHeaderKeys`. A multi-segment path starts at `body` / `requestBody`, `requestHeaders`, `responseHeaders` or `context`; a single segment such as `url` redacts that field. Paths you add to the array after creating the middleware apply too; a non-string entry is ignored. Path redaction is also best-effort and never throws: a path that targets an array's `length`, a frozen or getter-only property, or any other slot it cannot safely write is skipped rather than allowed to drop the entire log entry. - **Body size guard (`maxBodyLength`)**: prevents log-flood attacks. Object/array bodies whose JSON form exceeds the limit return the envelope `{ _truncated: true, _originalLength, _preview }` (preserving JSON shape for downstream log shippers). String bodies are truncated to `maxBodyLength` Unicode code points (handles emoji surrogate pairs correctly) with a trailing `…`. - **Log injection (`escapeMessageNewlines`)**: untrusted user input concatenated into a log message — for example `logger.info(req.body.username)` where the request body carries `username = "alice\n[ERROR] (admin)\nfake critical event"` — writes three lines into the log file by default. The second and third lines are byte-for-byte indistinguishable from real `[ERROR]` entries the application itself produced and can mislead a SOC analyst, log parser, or incident responder into trusting forged data. Set `escapeMessageNewlines: true` on the `createLogger()` options to rewrite embedded `\n` / `\r` in string messages as the visible literal sequences `\n` / `\r` BEFORE the printf renders the line. Defaults to `false` for back-compat with consumers that intentionally emit multi-line messages; the recommendation for new applications is to opt in. diff --git a/src/request-middleware.ts b/src/request-middleware.ts index 2fffd43..beda205 100644 --- a/src/request-middleware.ts +++ b/src/request-middleware.ts @@ -439,12 +439,13 @@ const ownContext = (enriched: Record): Record /** * Property names that must NEVER be assigned through `acc[key] = …` when - * rebuilding a header bag or walking a `redactPaths` segment. `__proto__` - * invokes the prototype setter (mutating the local object's prototype chain - * AND, when the path-walker reaches `Object.prototype`, the global prototype - * itself). `constructor` / `prototype` are likewise structural fields whose - * assignment can corrupt instanceof checks. Mirrors the deny-list inside + * rebuilding a header bag, nor followed or written by a `redactPaths` segment. + * `__proto__` invokes the prototype setter (mutating the local object's + * prototype chain). `constructor` / `prototype` are likewise structural fields + * whose assignment can corrupt instanceof checks. Mirrors the deny-list inside * `src/redact.ts` for a single source of truth across the package. + * `redactEntryPath` spells the same three names as literal comparisons (so + * static analysis can see the guard); a test keeps that list equal to this Set. */ const FORBIDDEN_OBJECT_KEYS = new Set(["__proto__", "constructor", "prototype"]); @@ -515,11 +516,12 @@ const applyHeaderMask = ( * (`Map`, `Set`, `RegExp`) by identity — its documented rule for values owning * no key-addressable *string* secret. Those are therefore still shared with the * caller by reference. That sharing is safe ONLY because {@link redactEntryPath} - * refuses to write into any target that is not an owned plain object or array: - * a `Buffer`'s writable integer indices and a `RegExp`'s writable `lastIndex` - * DO pass a bare `hasOwnProperty` + writable-descriptor check, so the earlier - * claim that "such a value exposes none a path would target" was false — the - * guarantee is enforced at the write site, not by the shape of these values. + * refuses to step through or write into anything that is not an owned plain + * object or array: a `Buffer`'s writable integer indices and a `RegExp`'s + * writable `lastIndex` DO pass a bare `hasOwnProperty` + writable-descriptor + * check, and a plain object hanging off a shared `toJSON`-defining value is the + * application's own, so the guarantee is enforced by the walk, on every step, + * not by the shape of these values. */ const EMPTY_HEADER_MASK = new Set(); @@ -581,7 +583,14 @@ const normalizeHeaders = ( if (FORBIDDEN_OBJECT_KEYS.has(normalizedKey)) { return acc; } - if (normalized[normalizedKey] !== undefined) { + // Own keys only: `normalized` is an ordinary object, so a plain read of a + // header the request did not send would return whatever an inherited + // (polluted) `Object.prototype` property holds, log it as that header, and + // hand the shared object to the `redactPaths` pass by reference. + if ( + Object.prototype.hasOwnProperty.call(normalized, normalizedKey) && + normalized[normalizedKey] !== undefined + ) { acc[normalizedKey] = normalized[normalizedKey]; } return acc; @@ -726,6 +735,43 @@ const safeRedactedUrl = ( } }; +/** + * The entry fields a multi-segment `redactPaths` entry may descend into: the + * sub-graphs the package copies before the path pass (`requestBody` via + * `serializeBody`, the two header bags via `normalizeHeaders`, `context` via + * `ownContext`). The other fields (`method`, `url`, `ip`, `userAgent`, + * `requestId`, ...) are taken from the request as they are and typed as + * primitives; an adapter that breaks those types (a `req.get()` returning its + * own live array) would otherwise hand the walk the application's object. A + * single-segment path (`url`, `userAgent`) still redacts the field itself. + */ +const PATH_ROOT_FIELDS = new Set(["requestBody", "requestHeaders", "responseHeaders", "context"]); + +/** + * Whether `value` is a container {@link redactEntryPath} may step through or + * write into: an array, or an object whose prototype is exactly + * `Object.prototype`. Every container the package owns for redaction has one of + * these two shapes (JSON round-trip copies, the rebuilt header bag, and + * `redactValue`'s copies, which turn even an `Object.create(null)` input into a + * fresh `{}`), and every value the entry shares with the caller BY REFERENCE (a + * `Date`, a class instance, a `Buffer` / typed array, a `RegExp`, a `Map` / + * `Set`) has neither, so this is the ownership test. `Object.prototype` itself + * fails it (its prototype is `null`). A Proxy trap that throws here is caught by + * the caller's try/catch. + * + * Boundary: the test is by shape, so it trusts the copies to be fresh. They are + * for everything a request can carry (JSON round-trips, and `redactValue`'s + * rebuilds of plain objects). An application that builds a header value + * itself can still defeat it: an array with its own `map` or a replaced + * `constructor` / `Symbol.species` (the copy is made with `value.map` in + * `src/redact.ts`), or a Proxy whose `getPrototypeOf` trap answers differently + * on each call, can hand the walk an object the application owns. + */ +const isOwnedPathContainer = (value: unknown): value is Record => + typeof value === "object" && + value !== null && + (Array.isArray(value) || Object.getPrototypeOf(value) === Object.prototype); + /** * Surgically redacts a value at the supplied dot-notation path on the entry * object. Missing intermediate keys are a no-op (we never create new sub-paths @@ -739,120 +785,102 @@ const safeRedactedUrl = ( * - `requestBody.user.password` is also accepted as the explicit form. * - `context.user.token` → `entry.context.user.token = "[REDACTED]"`. * - * The final assignment is **best-effort and never throws**: an array `length` - * target, a getter-only / accessor property, a non-writable (frozen) slot, or - * any other value whose write would raise (a `Proxy` with a throwing `set` - * trap) is skipped rather than allowed to propagate. This is load-bearing — - * `finalize()` applies these paths inside the same try block that assembles the - * whole entry, so a throwing redaction would otherwise drop the ENTIRE log line - * (method, url, status included) over one path. Callers that must guarantee the - * caller's own object is never mutated by these in-place writes should pass a - * fully-owned copy of the target sub-graph (as `finalize()` does for - * `entry.context` and `serializeBody()` does for `entry.requestBody`). + * **The walk only ever passes through, and writes into, containers the package + * owns, and it only follows OWN DATA properties.** Each step must be an array or + * a plain object whose prototype is exactly `Object.prototype` (see + * {@link isOwnedPathContainer}), and each segment is read through + * `Object.getOwnPropertyDescriptor`, never `container[segment]`. Four + * guarantees follow, each pinned by a test: + * 1. **No prototype is ever reached.** An inherited property is not an own + * property, so the walk cannot step onto `Object.prototype` or onto an object + * parked there, and `__proto__` / `constructor` / `prototype` segments are + * refused outright (literal comparisons, below). The earlier walk read + * `cursor[segment]` through the prototype chain and relied on the deny-list + * alone; a path through an object held on `Object.prototype` rewrote that + * one object for the whole process, and CodeQL + * (`js/prototype-polluting-assignment`, CWE-1321) flagged the write. + * 2. **The caller's live objects are never written.** Everything the package + * owns for redaction is a plain object or an array (the `ownContext` / + * `serializeBody` JSON round-trips, the `normalizeHeaders` bag and its + * `ownHeaderValue` copies), and from the entry itself a path descends only + * into those copied fields ({@link PATH_ROOT_FIELDS}). Any other value in + * the entry (a `Date`, a class instance with its own `toJSON()`, a + * `Buffer`, a `RegExp`, a `Map`, an adapter's non-string field) was kept BY + * REFERENCE, so everything behind it is the application's state: the walk + * stops there. Checking only the final target was not enough: a path + * through a `toJSON`-defining response-header value reached a plain object + * the application owned and overwrote its field. + * 3. **No caller code runs.** An accessor is never invoked, on the path or at + * the target; the earlier walk ran inherited and own getters on the way. + * Descriptor fields are read as the descriptor's OWN properties, so an + * inherited (polluted) `value` / `writable` cannot make an accessor look + * like a writable data slot and get its setter called. + * 4. **It never throws**, whatever `path` holds. Anything that throws while + * splitting, reading or writing (a Proxy trap, an exotic host object) turns + * the path into a no-op. This is load-bearing: `finalize()` applies these + * paths inside the same try block that assembles the whole entry, so a + * throw here dropped the ENTIRE log line (method, url, status included) + * over one path. + * + * Other no-ops: a missing segment, a non-object step, an array `length` + * target, and a non-writable (frozen) slot. */ const redactEntryPath = (entry: Record, path: string): void => { - if (!path) { - return; - } - const segments = path.split(".").filter(Boolean); - if (segments.length === 0) { + // A non-string is never split: an object carrying its own `split` method + // would otherwise run caller code here. + if (typeof path !== "string" || path === "") { return; } - // Prototype-pollution guard, part 1 — INTERMEDIATE segments. A path like - // `body.__proto__.toString` would otherwise walk - // `cursor[segments[i]]` into `Object.prototype` and then assign on the - // (legitimate) final segment, mutating the global prototype. The deny-list - // mirrors the FORBIDDEN_OBJECT_KEYS set used by the header accumulators and - // the `redactValue` rebuild — single source of truth for prototype-pollution - // hardening across the package. (Part 2, the final-segment check, lives - // just above the assignment below.) - for (let i = 0; i < segments.length - 1; i += 1) { - if (FORBIDDEN_OBJECT_KEYS.has(segments[i])) { + try { + const segments = path.split(".").filter(Boolean); + if (segments.length === 0) { return; } - } - // Map the user-facing `body` alias to the on-entry `requestBody` field. - if (segments[0] === "body") { - segments[0] = "requestBody"; - } - - let cursor: unknown = entry; - for (let i = 0; i < segments.length - 1; i += 1) { - if (!cursor || typeof cursor !== "object") { - return; + // Map the user-facing `body` alias to the on-entry `requestBody` field. + if (segments[0] === "body") { + segments[0] = "requestBody"; + } + const lastIndex = segments.length - 1; + let container: unknown = entry; + for (let i = 0; i <= lastIndex; i += 1) { + const key = segments[i]; + // Prototype-pollution guard, checked on every segment at the point of + // use. Same names as FORBIDDEN_OBJECT_KEYS, written as the literal + // comparisons CodeQL's documentation recommends (it does not treat + // `Set.prototype.has` as a guard). The CodeQL finding itself is closed by + // the walk having no `container[key]` read at all. A test iterates + // FORBIDDEN_OBJECT_KEYS against this walk, so the two lists cannot drift + // apart. + if (key === "__proto__" || key === "constructor" || key === "prototype") { + return; + } + // From the entry itself, descend only into a sub-graph the package copied. + if (i === 0 && i < lastIndex && !PATH_ROOT_FIELDS.has(key)) { + return; + } + if (!isOwnedPathContainer(container)) { + return; + } + const descriptor = Object.getOwnPropertyDescriptor(container, key); + // Own data properties only: an absent (possibly inherited) property or an + // accessor ends the walk without reading it. The descriptor is an + // ordinary object, so `value` is tested as its OWN field: `"value" in` + // would also see an inherited (polluted) `Object.prototype.value` and + // take an accessor for a data property. A data descriptor always owns + // `writable` too, so reading it below cannot reach the prototype. + if (descriptor === undefined || !Object.prototype.hasOwnProperty.call(descriptor, "value")) { + return; + } + if (i < lastIndex) { + container = descriptor.value; + } else if (descriptor.writable === true && !(Array.isArray(container) && key === "length")) { + // Never an array's `length`: it is an own writable data property, but + // assigning a non-numeric value throws `RangeError: Invalid array length`. + container[key] = REDACTED; + } } - cursor = (cursor as Record)[segments[i]]; - } - if (!cursor || typeof cursor !== "object") { - return; - } - const finalKey = segments[segments.length - 1]; - // Prototype-pollution guard, part 2 — FINAL segment. Blocks an assignment - // of the form `target.__proto__ = "[REDACTED]"` (which would invoke the - // setter and clobber `target`'s prototype chain). This guard runs BEFORE - // the `hasOwnProperty` filter so CodeQL's taint flow for - // `js/prototype-polluting-assignment` (CWE-1321) can statically prove the - // assignment cannot reach a prototype slot. - if (FORBIDDEN_OBJECT_KEYS.has(finalKey)) { - return; - } - const target = cursor as Record; - if (!Object.prototype.hasOwnProperty.call(target, finalKey)) { - return; - } - // Never overwrite an array's `length`. `hasOwnProperty(arr, "length")` is - // `true`, but assigning a non-numeric value throws - // `RangeError: Invalid array length`; without this guard - // `redactPaths:["body.tags.length"]` would throw and `finalize()`'s catch - // would drop the ENTIRE log entry over a single path. - if (Array.isArray(target) && finalKey === "length") { - return; - } - // Only ever write into a container the package FULLY OWNS: a plain object - // (produced by the `ownContext` / `serializeBody` JSON round-trips or the - // `normalizeHeaders` bag rebuild) or an array (an owned header/body array). - // Every other reachable target is a value that `redactValue` handed back BY - // IDENTITY — a `Date`, a `Buffer` / typed-array, a `RegExp`, a bare `Error`, - // a `Map` / `Set` — i.e. STILL SHARED with the caller. Those are pass-through - // precisely because they own no key-addressable *string* secret, but that is - // not the same as owning no writable own key a numeric/reserved path could - // hit: a binary view exposes writable integer-index data properties - // (`Object.keys(Buffer.from([1,2,3]))` → `["0","1","2"]`) and a `RegExp` a - // writable `lastIndex`, both of which pass the `hasOwnProperty` + writable- - // descriptor guards below. Without this check - // `redactPaths:["responseHeaders.x-buf.0"]` over a `Buffer`-valued response - // header — reachable in plain Node, since `res.setHeader` stores non-string - // values unmodified and `res.getHeaders()` hands the same reference back — - // would coerce and zero a byte of the application's LIVE Buffer, the exact - // caller-mutation `ownHeaderValue` exists to prevent. Restricting the write - // to owned plain containers closes that whole class centrally and excludes - // nothing legitimate: every value the package owns for redaction (after the - // round-trips and the `forceCopy` header rebuild) is a plain object or an - // array, never one of these pass-through built-ins. (`redactValue` rebuilds - // every plain object into a fresh `{}` with `Object.prototype`, even an - // `Object.create(null)` input, so a null-prototype target is unreachable here - // and is not admitted — that keeps the guard's branches exactly the reachable - // ones: array, `Object.prototype`, or skip.) - if (!Array.isArray(target) && Object.getPrototypeOf(target) !== Object.prototype) { - return; - } - // Only overwrite a writable data property. A frozen target - // (`writable === false`), a getter-only / accessor property (`get` / `set`), - // or any other non-assignable slot cannot be written — in strict mode the - // assignment throws, and that throw would propagate to `finalize()`'s catch - // and drop the whole entry. Logging is best-effort, so a redaction that - // cannot be applied degrades to a no-op on that path instead. - const descriptor = Object.getOwnPropertyDescriptor(target, finalKey); - if (!descriptor || descriptor.get || descriptor.set || descriptor.writable === false) { - return; - } - // Defense-in-depth: even a writable data property can throw on assignment - // (an exotic host object, or a `Proxy` with a throwing `set` trap). A failed - // redaction must never take the log line down with it. - try { - target[finalKey] = REDACTED; } catch { - // best-effort: leave the value in place rather than dropping the entry + // Best-effort: leave the value in place rather than dropping the entry. } }; @@ -1021,6 +1049,10 @@ export const createRequestLogger = (options: RequestLoggerOptions = {}): Loggabl Array.isArray(maskBodyKeys) && maskBodyKeys.length > 0 ? new Set(maskBodyKeys.map((key) => key.toLowerCase())) : undefined; + // The caller's LIVE array, deliberately not a copy: a path an application + // adds after creating the middleware keeps applying (a copy would silently + // drop it and log what it was meant to hide). Validation above cannot see a + // value added later, so `redactEntryPath` ignores any non-string itself. const resolvedRedactPaths = Array.isArray(redactPaths) ? redactPaths : []; return (req: LoggableRequest, res: LoggableResponse, next: LoggableNext) => { @@ -1230,10 +1262,11 @@ export const createRequestLogger = (options: RequestLoggerOptions = {}): Loggabl // leaves `Date` / `Buffer` / `RegExp` / `Map`-shaped values shared by // identity — `redactValue`'s own rule for values owning no key- // addressable string secret. That is safe because `redactEntryPath` - // writes ONLY into an owned plain object or array and refuses every - // other target, so a path aimed at a shared `Buffer`'s writable integer - // index (or a `RegExp`'s `lastIndex`) is a no-op instead of a write into - // the caller's live value. + // steps through and writes into ONLY owned plain objects and arrays, + // following own data properties, so a path aimed at a shared `Buffer`'s + // writable integer index, a `RegExp`'s `lastIndex`, or a plain object + // behind a shared `toJSON`-defining value is a no-op instead of a write + // into the caller's live value. if (resolvedRedactPaths.length > 0) { resolvedRedactPaths.forEach((path) => redactEntryPath(entry as unknown as Record, path), @@ -1311,4 +1344,5 @@ export const __requestInternals = { VALID_LOG_LEVELS, DEFAULT_MASKED_HEADER_KEYS, DEFAULT_MASKED_QUERY_KEYS, + FORBIDDEN_OBJECT_KEYS, }; diff --git a/tests/request-middleware.spec.ts b/tests/request-middleware.spec.ts index 1e4bab1..d191ea0 100644 --- a/tests/request-middleware.spec.ts +++ b/tests/request-middleware.spec.ts @@ -1452,6 +1452,177 @@ describe("createRequestLogger", () => { expect(liveRe.lastIndex).toBe(3); }); + it.each([ + [ + "a class instance with its own toJSON()", + (): { value: unknown; inner: { token: string } } => { + class Meta { + public inner = { token: "LIVE-TOKEN" }; + public toJSON(): string { + return "meta"; + } + } + const value = new Meta(); + return { value, inner: value.inner }; + }, + '{"x-meta":"meta"}', + ], + [ + "a Date carrying an own object field", + (): { value: unknown; inner: { token: string } } => { + const inner = { token: "LIVE-TOKEN" }; + return { value: Object.assign(new Date(0), { inner }), inner }; + }, + '{"x-meta":"1970-01-01T00:00:00.000Z"}', + ], + ])( + "redactPaths never writes through %s shared by a response header into the app's own object", + (_label, make, rendered) => { + // A header value that defines `toJSON()` is kept by reference (it renders + // through that method), so everything reachable from it is the + // application's live state. A path must stop at it, not walk through it + // into a plain object the application owns and overwrite a field there. + const { value, inner } = make(); + + const { logger, log } = createMockLogger(); + const middleware = createRequestLogger({ + logger, + includeHttpContext: true, + includeResponseHeaders: true, + redactPaths: ["responseHeaders.x-meta.inner.token"], + }); + + const { res } = runMiddleware(middleware, {}, { getHeaders: () => ({ "x-meta": value }) }); + res.emit("finish"); + + expect(log).toHaveBeenCalledTimes(1); + // The application's object keeps its value ... + expect(inner).toEqual({ token: "LIVE-TOKEN" }); + // ... and the header renders exactly as its toJSON() renders it. + expect(JSON.stringify(log.mock.calls[0][0].http.responseHeaders)).toBe(rendered); + }, + ); + + it("a throwing getter on a shared header value's prototype is never run and cannot drop the entry", () => { + // A class instance with no enumerable own keys is kept by reference. A path + // into it used to read `detail` through the prototype, running the + // application's getter; its throw escaped the path pass and the whole + // request went unlogged. + let reads = 0; + class Lazy { + public get detail(): Record { + reads += 1; + throw new Error("getter ran"); + } + } + const errorSpy = jest.spyOn(console, "error").mockImplementation(() => undefined); + + const { logger, log } = createMockLogger(); + const middleware = createRequestLogger({ + logger, + includeHttpContext: true, + includeResponseHeaders: true, + redactPaths: ["responseHeaders.x-lazy.detail.token", "responseHeaders.x-secret"], + }); + + const { res } = runMiddleware( + middleware, + {}, + { getHeaders: () => ({ "x-lazy": new Lazy(), "x-secret": "HEADER-SECRET" }) }, + ); + res.emit("finish"); + + expect(log).toHaveBeenCalledTimes(1); + // The other path still applies to the same entry. + expect(log.mock.calls[0][0].http.responseHeaders["x-secret"]).toBe("[REDACTED]"); + expect(reads).toBe(0); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it("an allow-listed header the request lacks never picks up (or writes) a value held on Object.prototype", () => { + // In a process where something parked an object on `Object.prototype` under + // a lowercase name, the allow-list branch read `normalized[name]` through + // the prototype chain: it logged that shared object as a header and handed + // it to the path pass by reference, which overwrote its field. + Object.defineProperty(Object.prototype, "x-polluted", { + value: { token: "GLOBAL" }, + configurable: true, + writable: true, + enumerable: false, + }); + try { + const { logger, log } = createMockLogger(); + const middleware = createRequestLogger({ + logger, + includeHttpContext: true, + includeRequestHeaders: ["x-polluted", "user-agent"], + redactPaths: ["requestHeaders.x-polluted.token"], + }); + + const { res } = runMiddleware(middleware); + res.emit("finish"); + + expect(log).toHaveBeenCalledTimes(1); + expect(log.mock.calls[0][0].http.requestHeaders).toEqual({ "user-agent": "jest" }); + expect( + (Object.prototype as unknown as Record)["x-polluted"], + ).toEqual({ token: "GLOBAL" }); + } finally { + delete (Object.prototype as unknown as Record)["x-polluted"]; + } + }); + + it("applies paths added to the caller's redactPaths array later, and ignores non-strings in it", () => { + // The middleware keeps the caller's live array, so a path added after it + // was created still applies (dropping it would log what it was meant to + // hide). Construction-time validation cannot see a value added later: a + // non-string used to make the path pass throw on every request, so no + // request was logged at all. + const paths = ["body.password"]; + const { logger, log } = createMockLogger(); + const middleware = createRequestLogger({ + logger, + includeHttpContext: true, + includeRequestBody: true, + redactPaths: paths, + }); + + paths.push(42 as unknown as string, "body.email"); + + const { res } = runMiddleware(middleware); + res.emit("finish"); + + expect(log).toHaveBeenCalledTimes(1); + expect(log.mock.calls[0][0].http.requestBody).toEqual({ + email: "[REDACTED]", + password: "[REDACTED]", + }); + }); + + it("a path never descends into an entry field the package does not copy (an adapter's live array)", () => { + // `requestId` / `userAgent` / `method` / `ip` / `url` are typed as strings + // and taken from the request as they are. An adapter whose `get()` returns + // its own live array must not have that array written through a path. + const liveIds = ["REQ-ID-SECRET"]; + const { logger, log } = createMockLogger(); + const middleware = createRequestLogger({ + logger, + includeHttpContext: true, + redactPaths: ["requestId.0", "userAgent"], + }); + + const { res } = runMiddleware(middleware, { + get: ((name: string) => + name === "x-request-id" ? liveIds : "agent") as unknown as MockRequest["get"], + }); + res.emit("finish"); + + expect(log).toHaveBeenCalledTimes(1); + expect(liveIds).toEqual(["REQ-ID-SECRET"]); + // A path whose only segment names an entry field still redacts it. + expect(log.mock.calls[0][0].http.userAgent).toBe("[REDACTED]"); + }); + it("copying a header array changes nothing a consumer sees (output equivalence)", () => { const { logger, log } = createMockLogger(); const middleware = createRequestLogger({ @@ -2694,6 +2865,7 @@ describe("request middleware internals", () => { resolveMaskQueryKeys, DEFAULT_MASKED_HEADER_KEYS, DEFAULT_MASKED_QUERY_KEYS, + FORBIDDEN_OBJECT_KEYS, } = __requestInternals; it("serializes primitive bodies and truncates strings", () => { @@ -3337,6 +3509,139 @@ describe("request middleware internals", () => { expect((throwing as { token: string }).token).toBe("SECRET"); }); + it("redactEntryPath never writes into an object reachable only through the prototype chain", () => { + // Stand-in for a runtime where something already parked an object on + // `Object.prototype` (a polluted process, or a library doing it on purpose). + // A walk that reads inherited properties reaches that ONE shared object from + // every plain object, so a path through it rewrites state every object in + // the process sees. The walk must follow own properties only. + Object.defineProperty(Object.prototype, "sharedDefaults", { + value: { token: "GLOBAL" }, + configurable: true, + writable: true, + enumerable: false, + }); + try { + const entry = { context: { own: 1 } } as unknown as Record; + redactEntryPath(entry, "context.sharedDefaults.token"); + expect( + (Object.prototype as unknown as { sharedDefaults: { token: string } }).sharedDefaults, + ).toEqual({ token: "GLOBAL" }); + expect(entry).toEqual({ context: { own: 1 } }); + } finally { + delete (Object.prototype as unknown as Record).sharedDefaults; + } + }); + + it("redactEntryPath never runs a getter on the path (own data properties only)", () => { + let reads = 0; + const context = { + get nested(): Record { + reads += 1; + return { token: "SECRET" }; + }, + }; + const entry = { context } as unknown as Record; + expect(() => redactEntryPath(entry, "context.nested.token")).not.toThrow(); + expect(reads).toBe(0); + }); + + it("redactEntryPath swallows a throw while reading the path (Proxy with a throwing descriptor trap)", () => { + const nested = { token: "SECRET" }; + const hostile = new Proxy( + { nested }, + { + getOwnPropertyDescriptor() { + throw new Error("descriptor trap boom"); + }, + }, + ); + const entry = { context: hostile } as unknown as Record; + expect(() => redactEntryPath(entry, "context.nested.token")).not.toThrow(); + // Nothing behind the unreadable step is written. + expect(nested).toEqual({ token: "SECRET" }); + }); + + it("redactEntryPath refuses every FORBIDDEN_OBJECT_KEYS member as a middle or final segment, even as an own key", () => { + // `JSON.parse` mints each forbidden name as an OWN data property, which an + // own-property walk would otherwise follow or overwrite. The walk compares + // segments against literal names, so this pins that list to the Set. + expect([...FORBIDDEN_OBJECT_KEYS].sort()).toEqual(["__proto__", "constructor", "prototype"]); + for (const key of FORBIDDEN_OBJECT_KEYS) { + const own = JSON.parse(`{"${key}": {"token": "T"}, "k": {"${key}": "V"}}`) as Record< + string, + Record + >; + const entry = { context: own } as unknown as Record; + + redactEntryPath(entry, `context.${key}.token`); + redactEntryPath(entry, `context.k.${key}`); + + expect(Object.getOwnPropertyDescriptor(own, key)?.value).toEqual({ token: "T" }); + expect(Object.getOwnPropertyDescriptor(own.k, key)?.value).toBe("V"); + } + }); + + it("redactEntryPath ignores a path that is not a string, and never calls its methods", () => { + let splitCalls = 0; + const withSplit = { + split: (): string[] => { + splitCalls += 1; + return ["context", "token"]; + }, + }; + const entry = { context: { token: "SECRET" } } as unknown as Record; + for (const path of [42, {}, ["context", "token"], withSplit]) { + expect(() => redactEntryPath(entry, path as unknown as string)).not.toThrow(); + } + expect(splitCalls).toBe(0); + expect(entry).toEqual({ context: { token: "SECRET" } }); + }); + + it("redactEntryPath reads a descriptor's own fields only (an inherited `value` / `writable` cannot steer it)", () => { + // `Object.getOwnPropertyDescriptor` returns an ordinary object, so + // `"value" in descriptor` and `descriptor.writable` consult + // `Object.prototype`. In a process where those names were polluted, an + // accessor on the path looked like a writable data property: the walk + // stepped onto the polluted `value` and wrote into it, and a final + // accessor had its setter called. + let setterCalls = 0; + const context = { + get nested(): unknown { + return undefined; + }, + get secret(): string { + return "S"; + }, + set secret(_value: string) { + setterCalls += 1; + }, + }; + const entry = { context } as unknown as Record; + const pollutedValue = { token: "GLOBAL" }; + Object.defineProperty(Object.prototype, "value", { + value: pollutedValue, + configurable: true, + writable: true, + enumerable: false, + }); + Object.defineProperty(Object.prototype, "writable", { + value: true, + configurable: true, + writable: true, + enumerable: false, + }); + try { + redactEntryPath(entry, "context.nested.token"); + redactEntryPath(entry, "context.secret"); + } finally { + delete (Object.prototype as unknown as Record).value; + delete (Object.prototype as unknown as Record).writable; + } + expect(pollutedValue).toEqual({ token: "GLOBAL" }); + expect(setterCalls).toBe(0); + }); + it("resolveMaskHeaderKeys returns undefined when option is false and the default set otherwise", () => { expect(resolveMaskHeaderKeys(false)).toBeUndefined(); const defaults = resolveMaskHeaderKeys(undefined); @@ -3416,15 +3721,12 @@ describe("request middleware internals", () => { }); it("redactEntryPath final-segment guard catches __proto__ as an OWN property on the cursor", () => { - // The intermediate-segment guard (part 1) catches forbidden keys at - // segments[0..length-2]. This test exercises the FINAL-segment guard - // (part 2, the inline check right before the assignment) by feeding a - // path whose intermediate segments are all clean and whose final segment - // is `__proto__` — AND mounting the assignment target as a - // `JSON.parse('{"__proto__": ...}')` object that owns the key directly, - // so the `hasOwnProperty` filter would NOT short-circuit on its own. - // Without the inline guard the bracket-assignment would invoke the - // prototype setter and corrupt the chain. + // The walk refuses `__proto__` on every segment, the final one included. + // Here the path's other segments are clean and the target is a + // `JSON.parse('{"__proto__": ...}')` object that OWNS the key as a data + // property, so an own-property walk would otherwise overwrite it (an + // assignment to an own data property does not reach the prototype + // setter, but the refusal keeps every `__proto__` name out of the walk). const evil = JSON.parse('{"__proto__": "real"}') as Record; const entry = { requestBody: { user: evil } } as unknown as Record; const originalProto = Object.getPrototypeOf(evil);