add nullPrototype option, improve performance

[Uzlopak]

If you have Objects were you want to allow also prototype and constructor as valid keys without having the risk of prototype pollution, you have to create Objects with prototype set to null. This PR adds the nullPrototype option. It also improves the performance, by avoiding creating unnecessary intermediary Objects

[Uzlopak]

@Eomm

[mcollina]

lgtm

[Uzlopak]

wait please. I am currently implementing your proposal, and trying to get it back to 100% test coverage

[Uzlopak]

@Eomm ready

[mcollina]

I'm not sure exactly why are we adding this here. Offering prototype pollution protection is asking for security researchers to find holes in this algorithm. I would rather avoid the problem if we don't need it elsewhere.

[Uzlopak]

I added it here, because when we merge two json schemas and they could theoretically contain the key prototype or constructor than merging it into a normal Object would result in prototype pollution. What is your concern? If you dont set nullPrototype you have normal prototype pollution.

[mcollina]

JSON schemas are code. There is nothing to worry about prototype pollutions.

[Uzlopak]

Please decide. I will then just take the performance improvements to the master branch

[Uzlopak]

@mcollina
Is really json schema always code?

https://github.com/fastify/fast-json-stringify/blob/a7c56974a7e44ee8f0bd4410dbd7a79ab2e26beb/index.js#L527

[mcollina]

What's the link about?

JSON Schema should be treated as code, see https://www.fastify.io/docs/latest/Reference/Validation-and-Serialization/. Neither AJV nor fast-json-stringify are to be considered safe against malicious schemas.

[Uzlopak]

Ah now I get what you mean with "Has to be handled like code".
Then we sould maybe just add to the docs, that keys like prototype or constructor are not allowed for JSON Schemas.

Should I close this PR?

[mcollina]

Then we sould maybe just add to the docs, that keys like prototype or constructor are not allowed for JSON Schemas.

Maybe I'm not understanding the issue. I think those properties are allowed in JSON Schemas, aren't them?

Should I close this PR?

[Uzlopak]

I just fear that fast-json-stringify will not be able to handle those keys when doing merges on the schema, which are simple objects.