-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: only emit prototype pollution error when there's metadata emitted #281
Conversation
@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 |
Does it fail on SuperJSON.serialize({
mySchema: {
properties: {
constructor: { type: 'string', _pattern: /foo/ },
},
},
createdAt: new Date(),
}); |
Yes, it will fail on that I believe. Good catch, we should add another test case for that! |
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. |
Which specific (Honest question, not trying to fight you on this, just want to better understand the trade-offs that I'm making for myself) |
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 If you're intending to send data from client to server, there needs to be some sort of protection against malicious prototype pollution 🤷 |
That section specifically targets their
|
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. |
Closes #279