Repository navigation
fix(request-logger): walk redactPaths through owned data properties only - #2
Merged
Merged
Conversation
redactEntryPath read each segment as container[segment], which follows inherited properties and runs getters, and it checked ownership only at the final target. CodeQL flagged the write as js/prototype-polluting-assignment (CWE-1321). The deny-list kept the walk off Object.prototype itself, but a path could still overwrite an object parked on Object.prototype, write into a plain object the application owns behind a by-reference header value (a toJSON-defining instance, a Date), and run a getter whose throw dropped the whole log entry. The walk now steps through and writes into arrays and Object.prototype objects only, reads own data properties through their descriptors, refuses __proto__ / constructor / prototype on every segment with literal comparisons, and never throws.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
- normalizeHeaders' allow-list takes only the bag's own keys, so a listed header the request lacks cannot pick up a value inherited from a polluted Object.prototype and hand that shared object to the path pass. - redactEntryPath tests the descriptor's own `value`, so an inherited `value` / `writable` cannot make an accessor look like a writable data slot; descends from the entry only into the fields the package copies; and keeps the split inside its try, so it never throws. - createRequestLogger copies redactPaths at construction, so later changes to the caller's array cannot bypass validation or make the body pass and the entry pass disagree.
…g paths Copying redactPaths at construction would silently drop a path an application adds later and log what it was meant to hide. The middleware keeps the caller's array as before, and redactEntryPath returns for a non-string path before reading it, so an entry that bypassed validation can neither break logging nor run a caller-supplied split(). Also records the shape-based ownership boundary next to isOwnedPathContainer.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves the CodeQL alert
js/prototype-polluting-assignment(CWE-1321) onredactEntryPathinsrc/request-middleware.ts.Root cause
redactEntryPathread eachredactPathssegment ascontainer[segment], which follows inherited properties and runs getters, and it checked ownership only at the final target. The__proto__/constructor/prototypedeny-list (aSet.hascheck CodeQL does not recognize as a sanitizer) kept it offObject.prototypeitself, but probing the built 1.2.0 package showed three real defects:Object.prototypeoverwrote that one process-wide object.toJSON(), aDatewith an own object field) wrote"[REDACTED]"into a plain object the application owns, in the running app.Fix
The walk now steps through and writes into arrays and objects whose prototype is exactly
Object.prototypeonly (every container the package owns has one of those shapes; every by-reference value has neither), reads own data properties throughObject.getOwnPropertyDescriptor(never an inherited property, never an accessor), refuses__proto__/constructor/prototypeon every segment with literal comparisons, and never throws. No option or output format changes; every path that redacted a package-owned copy still does.Tests
7 new tests in
tests/request-middleware.spec.ts: 6 failed on the old code for the reasons above, 1 pins the literal deny-list toFORBIDDEN_OBJECT_KEYS. Four deliberate regressions (dropping a literal, reading through the prototype chain, checking ownership only at the final target, removing the try/catch) each turn at least one of them red.Local:
npm run verifypasses; the full suite passes on Node 18, 22 and 24 (1092 passed, 1 Windows-only skip, 100% coverage).