feat: add secure JSON parsing with prototype poisoning handling #603
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
secure-json-parse also has protection against constructor poisoning. Should I include that as well? Are there any use cases for it? |
bjohansebas
requested review from
Phillip9587,
wesleytodd,
UlisesGascon,
jonchurch and
ctcpip
lib/types/json.js
Outdated
| @@ -55,6 +56,7 @@ function json (options) { | |||
|
|
|||
| var reviver = options?.reviver | |||
| var strict = options?.strict !== false | |||
| const protoPoisoning = options?.onProtoPoisoning || 'ignore' | |||
There was a problem hiding this comment.
In a future major version, we can make this throw an error or ignore it, but in the meantime, to maintain compatibility, this option is handled.
| @@ -17,6 +17,7 @@ | |||
| "on-finished": "^2.4.1", | |||
| "qs": "^6.14.0", | |||
| "raw-body": "^3.0.0", | |||
| "secure-json-parse": "^3.0.2", | |||
There was a problem hiding this comment.
There's a version 4, but they theoretically support from node 20 onwards, so to avoid any potential issues, we’re sticking with version 3, which supports Node 18+
lib/types/json.js
Outdated
| @@ -74,7 +76,7 @@ function json (options) { | |||
|
|
|||
| try { | |||
| debug('parse json') | |||
| return JSON.parse(body, reviver) | |||
| return secureJson.parse(body, reviver, { protoAction: protoPoisoning }) | |||
There was a problem hiding this comment.
Another option would be to always protect against this issue by passing 'error' or 'remove', but I'm not sure. Other packages chose to make it a configurable option, and I think it would make some sense to do the same.
|
Do we want to cover this type of scenarios ( const obj = {};
const otherObj = {}
console.log("obj.polluted", obj.polluted) // undefined
console.log("otherObj.polluted", otherObj.polluted) // undefined
obj.constructor.prototype.polluted = true;
console.log("obj.polluted", obj.polluted) // true
console.log("otherObj.polluted", otherObj.polluted) // true |
|
Yeah, although I don't think it hurts to add an option for constructor poisoning, so I'm going to add tests for the constructor prototype and add the option |
…andling Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
bjohansebas
force-pushed
the
secure-json
branch
from
f4db4db to
6b4e150
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces secure JSON parsing that addresses prototype poisoning by leveraging the secure-json-parse library and adds middleware options to handle both proto and constructor.prototype poisoning.
- Added tests for various poisoning handling modes (ignore, error, remove) in the JSON parser.
- Updated the JSON parser in lib/types/json.js to make use of secure-json-parse with poisoning options.
- Enhanced documentation in README.md to detail the new onProto and onConstructor options.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/json.js | Added tests for prototype and constructor poisoning handling scenarios |
| lib/types/json.js | Updated JSON parser to use secure-json-parse with poisoning options |
| README.md | Extended documentation to explain new poisoning handling options |
Files not reviewed (1)
- package.json: Language not supported
Users of this middleware are given the option to control prototype poisoning
closes: #347