feat: add secure JSON parsing with prototype poisoning handling

[bjohansebas]

Users of this middleware are given the option to control prototype poisoning closes: #347

[socket-security[bot]]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	secure-json-parse@​3.0.2

View full report

[bjohansebas]

secure-json-parse also has protection against constructor poisoning. Should I include that as well? Are there any use cases for it?

[UlisesGascon]

Do we want to cover this type of scenarios (constructor.prototype) or just __proto__? Anyway... I will like to include constructor.prototype in the tests so we can document the current situation. Otherwise might we misleading as most developers are aware of __proto__ but not constructor behaviors.

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

[bjohansebas]

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

[jonchurch]

Im not crazy about the current form of the PR. I'd prefer we fight against bloating the options further, especially in cases where there is no strong reason for someone to deviate from the default.

Philosophically...

I don't like these kinds of mitigations for language level issues, they should be fixed in the language. That ship has long since sailed, as `Object.assign` is likely never to change. At least the spread operator was fixed to no longer set prototype.

I also am not convinced that the output of body-parser should ever be considered trusted input. There is a vuln here only in that some code may pass req.body into Object.assign. This particular issue happens to have a name because it is so common, but is one of many such issues that arise when developers take the objects they receive from users and blindly copy them.

We cannot code around all of them, but since this one is common and the intent of developers so clear (nobody is meaning to pollute the root proto, whether there was something malicious or not in the input) it is worth considering.

I'd consider dropping __proto__, constructor.prototype (and possibly even constructor outright) from user inputs silently in a major, and not providing an option for people to toggle it off. If someone needs to deviate that far, they can extend the Generic parser to do it.

But whatever we do, lets be consistent. the extended urlencoded parser (using qs) already drops proto, and there is no way to turn that off. The non extended urlencode uses querystring and would still pass through proto.

[bjohansebas]

I'd consider dropping proto, constructor.prototype (and possibly even constructor outright) from user inputs silently in a major, and not providing an option for people to toggle it off. If someone needs to deviate that far, they can extend the Generic parser to do it.

I agree with that approach, adding the option was more about being permissive, but I’d really prefer to just remove it.

[jonchurch]

Btw I was wrong about:

the extended urlencoded parser (using qs) already drops proto, and there is no way to turn that off. The non extended urlencode uses querystring and would still pass through proto.

1. non-extended went to qs in 2.0.0
2. qs DOES drop proto by default, but is being configured with allowPrototypes: true in bodyParser.urlencoded https://github.com/expressjs/body-parser/blob/b24797bf5976dd65e55193fd62630beb15d65b10/lib/types/urlencoded.js#L144, there is no option to configure that on body parser side was my point but my facts were wrong. The reason it is overridden was back compat when updating qs inside a major

So for the consistency argument here when landing a proto pollution mitigation to bodyParser.json that was always on, we'd drop overriding the default in qs to be consistent in both