fix(serializer): prevent unsafe deserialization paths#1543
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a security check in setProp to prevent prototype pollution by blocking unsafe path keys (proto, constructor, and prototype), along with corresponding unit tests. The reviewer points out that blocking constructor and prototype is overly restrictive and unnecessary, as the existing type guards already prevent traversing constructor functions. It is recommended to only block proto and update the tests accordingly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return (target: T | undefined) => keys.every((key) => hasProp(target, key)); | ||
| } | ||
|
|
||
| const UnsafePathKeys = new Set<keyof any>(["__proto__", "constructor", "prototype"]); |
There was a problem hiding this comment.
While blocking "__proto__" is absolutely necessary to prevent prototype pollution, blocking "constructor" and "prototype" is overly restrictive and can break legitimate use cases (e.g., setting or traversing properties named "constructor" or "prototype" on plain objects, such as metadata or configuration objects).
Because setProp strictly enforces that the target at each step must be a plain object or an array (!R.isPlainObject(target) && !R.isArray(target)), any attempt to traverse through a real constructor function (like Object or Array) will automatically throw a TypeError on the next iteration.
For example, if paths is ["constructor", "prototype", "polluted"]:
- In the first iteration,
targetbecomes the globalObjectconstructor function. - In the second iteration, the check
!R.isPlainObject(target) && !R.isArray(target)will fail becauseObjectis a function, throwing"Only plain objects and arrays are supported!".
Thus, prototype pollution via constructor.prototype is already inherently prevented by the existing type guards. We only need to block "__proto__" to be fully secure while maintaining compatibility with legitimate use cases.
| const UnsafePathKeys = new Set<keyof any>(["__proto__", "constructor", "prototype"]); | |
| const UnsafePathKeys = new Set<keyof any>(["__proto__"]); |
| it.each(["__proto__", "constructor", "prototype"])( | ||
| 'Rejects deserialization path key "%s"', | ||
| async (unsafeKey) => { |
There was a problem hiding this comment.
Update the test to only verify the rejection of the "__proto__" path key, as "constructor" and "prototype" are safe to traverse on plain objects and are already protected from prototype pollution by the isPlainObject check.
it(
'Rejects deserialization path key "__proto__"',
async () => {
const unsafeKey = "__proto__";Signed-off-by: Yinka Metrics <115735904+YinkaMetrics@users.noreply.github.com>
1fdd0f0 to
28f2b94
Compare
|
Updated the patch to address the compatibility concern from the automated review. The guard now rejects only |
Tomas2D
left a comment
There was a problem hiding this comment.
Correct, well-scoped fix — blocks the __proto__ path key and hardens the nested assignment, with solid regression tests confirming constructor/prototype stay safe as data keys. CI green. Thanks @YinkaMetrics! 🐝
Summary
setPropbefore nested assignment__proto__,constructor, andprototypepathsFixes #1532.
Verification
yarn vitest src/serializer/serializer.test.ts --runyarn vitest run srcyarn tsc --noEmityarn eslint src/internals/helpers/object.ts src/serializer/serializer.test.tsyarn prettier --log-level warn --check .NODE_OPTIONS=--max-old-space-size=16384 yarn tsup