Strip constructor, prototype, and __proto__ properties in the serialize step

[tmcw]

• Fixes SuperJSON will serialize into objects that it can't deserialize #266

In GHSA-5888-ffcr-r425, SuperJSON had an issue in which objects with prototype, constructor, or __proto__ properties would be reconstituted into potentially dangerous combinations - triggering prototype pollution.

This PR adds to that fix: where there currently is an inability to round-trip an object like { constructor: false } which results in SuperJSON throwing an error, this PR avoids serializing those properties in the first place, preventing a crash when they're deserialized.

[Skn0tt]

@all-contributors please add @tmcw for bugs, code!

[allcontributors]

@Skn0tt

I've put up a pull request to add @tmcw! 🎉

[Skn0tt]

Hi Tom! Thanks for reporting & fixing this bug! I have one small ask for change, let me explain:

I was wondering if we should see this as a breaking change, given it will start altering the behaviour of SuperJSON.serialize. I think it is: Even though I don't consider the serialization format to be part of SuperJSON's API contract, I think we should maintain the behaviour of SuperJSON.deserialize(SuperJSON.serialize(...)). In the past, we were throwing an error during deserialization, if __proto__ etc. were part of the serialized object. If this PR merged unchanged, we'd be swallowing the properties instead - that's a breaking change!

Instead of swallowing these properties, let's raise an exception instead.

[tmcw]

Okay! Updated and updated the test to match.

[Skn0tt]

Published in https://github.com/blitz-js/superjson/releases/tag/v2.1.0

[tmcw]

Thanks for the quick turnaround!

[jonluca]

This is breaking message parsing in electron, through https://github.com/jsonnull/electron-trpc - would it be possible to add a flag for this?

There is an object that was being passed around with prototype on it, I guess, so maybe this is ok? It did have side effects though, where our trpc client calls were resolving on 2.0.0 and failing on 2.1.0

[Skn0tt]

Yes, this did have side-effects. I assume in your case, it moved the errors from the client side to the server side. You can read my reasoning as to why I don't think that's a breaking change in #267 (review).

[jonluca]

I don't believe we had errors in our client side prior to this though - we were using electron-trpc and the code worked properly on v2.0.0 and does not in the latest.

The object that was causing issues was

{
    "id": "alex-mercer-v1-prototype22333333",
    "last_modified": "2023-07-30T01:09:38.901Z",
    "size": 59881413,
    "name": "Alex Mercer",
    "metadata": {
        "name": "Alex Mercer",
        "classification": "unknown",
        "epochs": 22333333,
        "gender": "unknown",
        "extra_info": {
            "version": 1,
            "prototype": true
        },
        "model": "gpt-3.5-turbo-16k-0613"
    },
    "downloaded": false
}

In the extra_info key there was a tag called prototype: true. Do we expect this to be an issue for superjson?

[tmcw]

I think that the reason why I was hitting errors around deserializing objects with prototype members is because the original testcase is

SuperJSON.deserialize(SuperJSON.serialize({ constructor: undefined }))

Since constructor is undefined, that makes SuperJSON encode it separately in its fancy types, rather than directly in json. So any value for constructor, __proto__, or prototype that's undefined, a date, a bigint, etc, will cause a crash deserializing in 2.0.0, but basic JSON values won't.

Fwiw, for my usecase, dropping these properties instead of throwing would work great, or – I would need to really study up on the attack that the original PR was preventing, but – only throwing when one of these properties has a dangerous value.

[Skn0tt]

I belive we should be able to narrow down the error message. Opened a PR here: #274
Could you take a look and let me know what you think about the change?