node-cache - fix: prototype pollution vulnerability in mget methods#1613
node-cache - fix: prototype pollution vulnerability in mget methods#1613
Conversation
Use Object.create(null) for mget() result objects in both NodeCache and NodeCacheStore so that __proto__ keys are treated as plain properties instead of polluting Object.prototype. Fixes #1612 https://claude.ai/code/session_01MhMFGu517ERhcLM4M5DsM8
There was a problem hiding this comment.
Code Review
This pull request addresses potential prototype pollution vulnerabilities in the mget methods of NodeCache and NodeCacheStore by initializing result objects with Object.create(null). It also introduces test cases to verify that proto keys do not affect the global object prototype. Review feedback suggests removing redundant type assertions for better code clarity and using bracket notation instead of dot notation for proto property access in tests to adhere to standard practices.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd9ce08c01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1613 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 26
Lines 2485 2485
Branches 554 555 +1
=========================================
Hits 2485 2485 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Replace result.__proto__ value check with Object.getPrototypeOf(result) === null and Object.hasOwn(result, "__proto__") assertions that actually verify the Object.create(null) fix rather than passing vacuously. https://claude.ai/code/session_01MhMFGu517ERhcLM4M5DsM8
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
Bug fix - Security
Description
This PR fixes a prototype pollution vulnerability in the
mgetmethods of bothNodeCacheandNodeCacheStoreclasses. The vulnerability occurred because the result object was created using object literal syntax ({}), which inherits fromObject.prototype. When a key named__proto__was stored and retrieved, it could pollute the prototype chain.Changes
NodeCache.mget()to create the result object usingObject.create(null)instead of{}NodeCacheStore.mget()to create the result object usingObject.create(null)instead of{}__proto__keys does not polluteObject.prototypeThe
Object.create(null)approach creates an object with no prototype chain, preventing any possibility of prototype pollution while maintaining the same functional behavior for legitimate use cases.Test Plan
Added unit tests that verify:
__proto__key can be stored and retrieved normallyObject.prototyperemains unpolluted after operations with__proto__keys__proto__property without affecting the global prototypeAll existing tests continue to pass with this change.
https://claude.ai/code/session_01MhMFGu517ERhcLM4M5DsM8