fix: preserve __proto__ as an object key#17
Conversation
|
|
|
@rupayon123 is attempting to deploy a commit to the promplate Team on Vercel. A member of the Team first needs to authorize it. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses prototype pollution vulnerabilities during JSON parsing by replacing direct property assignment with Object.defineProperty. A test case has been added to verify that __proto__ properties are parsed safely without polluting the global prototype. The reviewer recommends optimizing this change by only using Object.defineProperty when the key is specifically "__proto__" to avoid unnecessary performance overhead on standard keys.
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.
| Object.defineProperty(obj, key, { | ||
| value, | ||
| writable: true, | ||
| enumerable: true, | ||
| configurable: true, | ||
| }); |
There was a problem hiding this comment.
Using Object.defineProperty for every single key in every parsed object introduces a significant performance penalty compared to direct property assignment (obj[key] = value). Since __proto__ is the only key that triggers prototype-related setter behavior on standard objects, we can optimize this by using Object.defineProperty only when key === '__proto__', and using direct assignment for all other keys.
if (key === "__proto__") {
Object.defineProperty(obj, key, {
value,
writable: true,
enumerable: true,
configurable: true,
});
} else {
obj[key] = value;
}![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Using
Object.definePropertyfor every object key may add avoidable overhead; consider falling back to simple assignment for most keys and only usingdefinePropertyfor the"__proto__"case (or other known edge keys) to keep the common path fast.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `Object.defineProperty` for every object key may add avoidable overhead; consider falling back to simple assignment for most keys and only using `defineProperty` for the `"__proto__"` case (or other known edge keys) to keep the common path fast.
## Individual Comments
### Comment 1
<location path="tests/examples.test.js" line_range="25-30" />
<code_context>
expect(parse('{"": "', OBJ)).toEqual({});
expect(parse('{"": "', OBJ | STR)).toEqual({ "": "" });
+ const protoValue = { polluted: true };
+ const parsed = parse('{"__proto__":{"polluted":true},"safe":1}');
+ expect(Object.prototype.hasOwnProperty.call(parsed, "__proto__")).toBe(true);
+ expect(parsed.__proto__).toEqual(protoValue);
+ expect(parsed.safe).toBe(1);
+ expect(Object.prototype.polluted).toBeUndefined();
+
expect(() => parse("{", STR)).toThrow(PartialJSON);
</code_context>
<issue_to_address>
**suggestion (testing):** Add more regression cases around `__proto__` to cover nested and non-object values and different parse flags.
Right now this only exercises a top-level `__proto__` object value with default flags. Please expand coverage with:
- A case where `__proto__` has a primitive value (e.g. `{"__proto__":1,"safe":1}`) to confirm it’s treated as a normal key and doesn’t affect the prototype.
- Nested `__proto__` keys (inside child objects and array elements) to ensure behavior is consistent beyond the top level.
- A variant using the same parse flags under which the original bug reproduced, to verify the fix in that configuration.
This will better guard against any remaining prototype-pollution paths.
```suggestion
const protoValue = { polluted: true };
// Top-level __proto__ with object value (default flags)
const parsed = parse('{"__proto__":{"polluted":true},"safe":1}');
expect(Object.prototype.hasOwnProperty.call(parsed, "__proto__")).toBe(true);
expect(parsed.__proto__).toEqual(protoValue);
expect(parsed.safe).toBe(1);
expect(Object.prototype.polluted).toBeUndefined();
// Top-level __proto__ with primitive value (default flags)
const parsedPrimitive = parse('{"__proto__":1,"safe":1}');
expect(Object.prototype.hasOwnProperty.call(parsedPrimitive, "__proto__")).toBe(true);
expect(parsedPrimitive.__proto__).toBe(1);
expect(parsedPrimitive.safe).toBe(1);
expect(Object.prototype.polluted).toBeUndefined();
// Nested __proto__ inside child object (default flags)
const nestedObject = parse('{"level1":{"__proto__":{"polluted":true}},"safe":1}');
expect(Object.prototype.hasOwnProperty.call(nestedObject.level1, "__proto__")).toBe(true);
expect(nestedObject.level1.__proto__).toEqual(protoValue);
expect(nestedObject.safe).toBe(1);
expect(Object.prototype.polluted).toBeUndefined();
// __proto__ inside array elements (default flags)
const arrayNested = parse(
'{"arr":[{"__proto__":{"polluted":true}},{"__proto__":1}],"safe":1}'
);
expect(Object.prototype.hasOwnProperty.call(arrayNested.arr[0], "__proto__")).toBe(true);
expect(Object.prototype.hasOwnProperty.call(arrayNested.arr[1], "__proto__")).toBe(true);
expect(arrayNested.arr[0].__proto__).toEqual(protoValue);
expect(arrayNested.arr[1].__proto__).toBe(1);
expect(arrayNested.safe).toBe(1);
expect(Object.prototype.polluted).toBeUndefined();
// Top-level __proto__ with object value using additional parse flags
const parsedWithFlags = parse(
'{"__proto__":{"polluted":true},"safe":1}',
OBJ | STR
);
expect(Object.prototype.hasOwnProperty.call(parsedWithFlags, "__proto__")).toBe(true);
expect(parsedWithFlags.__proto__).toEqual(protoValue);
expect(parsedWithFlags.safe).toBe(1);
expect(Object.prototype.polluted).toBeUndefined();
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const protoValue = { polluted: true }; | ||
| const parsed = parse('{"__proto__":{"polluted":true},"safe":1}'); | ||
| expect(Object.prototype.hasOwnProperty.call(parsed, "__proto__")).toBe(true); | ||
| expect(parsed.__proto__).toEqual(protoValue); | ||
| expect(parsed.safe).toBe(1); | ||
| expect(Object.prototype.polluted).toBeUndefined(); |
There was a problem hiding this comment.
suggestion (testing): Add more regression cases around __proto__ to cover nested and non-object values and different parse flags.
Right now this only exercises a top-level __proto__ object value with default flags. Please expand coverage with:
- A case where
__proto__has a primitive value (e.g.{"__proto__":1,"safe":1}) to confirm it’s treated as a normal key and doesn’t affect the prototype. - Nested
__proto__keys (inside child objects and array elements) to ensure behavior is consistent beyond the top level. - A variant using the same parse flags under which the original bug reproduced, to verify the fix in that configuration.
This will better guard against any remaining prototype-pollution paths.
| const protoValue = { polluted: true }; | |
| const parsed = parse('{"__proto__":{"polluted":true},"safe":1}'); | |
| expect(Object.prototype.hasOwnProperty.call(parsed, "__proto__")).toBe(true); | |
| expect(parsed.__proto__).toEqual(protoValue); | |
| expect(parsed.safe).toBe(1); | |
| expect(Object.prototype.polluted).toBeUndefined(); | |
| const protoValue = { polluted: true }; | |
| // Top-level __proto__ with object value (default flags) | |
| const parsed = parse('{"__proto__":{"polluted":true},"safe":1}'); | |
| expect(Object.prototype.hasOwnProperty.call(parsed, "__proto__")).toBe(true); | |
| expect(parsed.__proto__).toEqual(protoValue); | |
| expect(parsed.safe).toBe(1); | |
| expect(Object.prototype.polluted).toBeUndefined(); | |
| // Top-level __proto__ with primitive value (default flags) | |
| const parsedPrimitive = parse('{"__proto__":1,"safe":1}'); | |
| expect(Object.prototype.hasOwnProperty.call(parsedPrimitive, "__proto__")).toBe(true); | |
| expect(parsedPrimitive.__proto__).toBe(1); | |
| expect(parsedPrimitive.safe).toBe(1); | |
| expect(Object.prototype.polluted).toBeUndefined(); | |
| // Nested __proto__ inside child object (default flags) | |
| const nestedObject = parse('{"level1":{"__proto__":{"polluted":true}},"safe":1}'); | |
| expect(Object.prototype.hasOwnProperty.call(nestedObject.level1, "__proto__")).toBe(true); | |
| expect(nestedObject.level1.__proto__).toEqual(protoValue); | |
| expect(nestedObject.safe).toBe(1); | |
| expect(Object.prototype.polluted).toBeUndefined(); | |
| // __proto__ inside array elements (default flags) | |
| const arrayNested = parse( | |
| '{"arr":[{"__proto__":{"polluted":true}},{"__proto__":1}],"safe":1}' | |
| ); | |
| expect(Object.prototype.hasOwnProperty.call(arrayNested.arr[0], "__proto__")).toBe(true); | |
| expect(Object.prototype.hasOwnProperty.call(arrayNested.arr[1], "__proto__")).toBe(true); | |
| expect(arrayNested.arr[0].__proto__).toEqual(protoValue); | |
| expect(arrayNested.arr[1].__proto__).toBe(1); | |
| expect(arrayNested.safe).toBe(1); | |
| expect(Object.prototype.polluted).toBeUndefined(); | |
| // Top-level __proto__ with object value using additional parse flags | |
| const parsedWithFlags = parse( | |
| '{"__proto__":{"polluted":true},"safe":1}', | |
| OBJ | STR | |
| ); | |
| expect(Object.prototype.hasOwnProperty.call(parsedWithFlags, "__proto__")).toBe(true); | |
| expect(parsedWithFlags.__proto__).toEqual(protoValue); | |
| expect(parsedWithFlags.safe).toBe(1); | |
| expect(Object.prototype.polluted).toBeUndefined(); |
|
Warning Review limit reached
More reviews will be available in 24 minutes and 2 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIn Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Updated this for the review feedback. The parser now only uses
|
commit: |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Closing this in favor of #20, which is the cleaner current version of the same |
1 participant
Fixes #8
This keeps
__proto__as a normal parsed object key by defining parsed members as own data properties instead of assigning through bracket syntax. I also added a regression check for the edge case.Checks:
git diff --checkI could not run the JS build/tests locally because
tscandvitestare not installed in this checkout.