fix: only emit prototype pollution error when there's metadata emitted

[Skn0tt]

Closes https://github.com/blitz-js/superjson/issues/279

[Skn0tt]

@janpot could you take a look at this, and maybe play around with the added test to see if this fixes all of your occurences for this error? You were mentioning that the value of constructor in your case is always a POJO, so this should work I think.

[Janpot]

Does it fail on

SuperJSON.serialize({
  mySchema: {
    properties: {
      constructor: { type: 'string', _pattern: /foo/ },
    },
  },
  createdAt: new Date(),
});

[Skn0tt]

Yes, it will fail on that I believe. Good catch, we should add another test case for that!

[Skn0tt]

I've tried adding this to our test suite, but this is exactly one of the payloads that can lead to prototype pollution when being parsed. I'm afraid we can't build a workaround for that.

[Janpot]

Which specific superjson features mandate this behavior? When I try the same thing with e.g. devalue, it doesn't seem to suffer from the same constraints. Or would devalue just be vulnerable to the same GHSA-5888-ffcr-r425?

(Honest question, not trying to fight you on this, just want to better understand the trade-offs that I'm making for myself)

[Skn0tt]

It depends on the direction you're sending data. Devalue specifically calls out that it can't be used when the serialised string comes from an untrusted party, like a browser: https://github.com/Rich-Harris/devalue#other-security-considerations So devalue can be used to send data from the server to the client, but not the other way around. They mention using (0, eval) - but I wouldn't trust that too much:

If you're intending to send data from client to server, there needs to be some sort of protection against malicious prototype pollution 🤷

[Janpot]

It depends on the direction you're sending data. Devalue specifically calls out that it can't be used when the serialised string comes from an untrusted party, like a browser: https://github.com/Rich-Harris/devalue#other-security-considerations So devalue can be used to send data from the server to the client, but not the other way around. They mention using (0, eval) - but I wouldn't trust that too much:

That section specifically targets their uneval function. It doesn't look like it applies to stringify/parse.

Use stringify and parse when evaluating JavaScript isn't an option.

[Skn0tt]

Interesting, I hadn't heard about stringify/parse before!

Looking at its implementation, I could see it being subject to the same vulnerability, but I haven't checked.