Added support for constructorAction

[delvedor]

As titled.
Now the following code is supported as well:

t.deepEqual(
  j.parse(
    '{"a": 5, "b": 6, "constructor":{"prototype":{"bar":"baz"}}, "__proto__": { "x": 7 } }',
      { protoAction: 'remove', constructorAction: 'remove' }
  ),
  { a: 5, b: 6 }
)

cc @watson

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows Code of conduct

[mcollina]

LGTM.

[delvedor]

@mcollina I fear this should be considered a breaking change since the default constructorAction behavior is error. What do you think?

[mcollina]

Is there any possibility that valid code could break because of this change?

[watson]

I left a few nit review comments which aren't technically related to this PR, but since it contains a lot of style changes and refactorings anyway, I thought I might as well 😄

Beside those review comments, there was a few lines I couldn't make comments on which I just wrote here instead:

Line 9 in index.js:

-    if (reviver != null && typeof reviver === 'object') {
+    if (reviver !== null && typeof reviver === 'object') {

Line 29 in index.js:

-  if (!obj || typeof obj !== 'object') {
+  if (obj === null || typeof obj !== 'object') {

[watson]

@mcollina I guess you can't rule out that someone has a legitimate use for parsing {"constructor":...} 🤷‍♂

[delvedor]

@mcollina yes, in the following case for example:

// old behavior
j.parse(
  '{"a": 5, "b": 6, "constructor":{"prototype":{"bar":"baz"}}, "__proto__": { "x": 7 } }',
  { protoAction: 'remove' }
) // => { a: 5: b:6, constructor: { prototype: { bar: 'baz' } } }

// new bahavior
j.parse(
  '{"a": 5, "b": 6, "constructor":{"prototype":{"bar":"baz"}}, "__proto__": { "x": 7 } }',
  { protoAction: 'remove' }
) // => SyntaxError

// for having the same behavior as before:
j.parse(
  '{"a": 5, "b": 6, "constructor":{"prototype":{"bar":"baz"}}, "__proto__": { "x": 7 } }',
  { protoAction: 'remove', constructorAction: 'ignore' }
) // => { a: 5: b:6, constructor: { prototype: { bar: 'baz' } } }

[mcollina]

I would do a 2 stage release, where we ship the new option with the ignore flag, and then we flip the default in a major.

[watson]

@mcollina unless you want to release some other features or fixes in this major, you might as well bump the major as this feature will not benefit anybody unless the user turns it on - in which case they might as well upgrade to the next major. So there's no real value in doing the gymnastics of releasing this in a non-breaking way under the current major. At least that's how I would probably do it 🤷‍♂

[delvedor]

Yes, I agree with @watson, we are not planning any other major feature, and in case of a bug, we can release a fix anytime.
Regarding the usage in Fastify itself, we can update the library in the next major, or use the ignore setting in the current release line.

[mcollina]

LGTM