Skip to content
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

Closed

Conversation

Skn0tt
Copy link
Collaborator

@Skn0tt Skn0tt commented Dec 5, 2023

Closes #279

@Skn0tt Skn0tt self-assigned this Dec 5, 2023
@Skn0tt
Copy link
Collaborator Author

Skn0tt commented Dec 5, 2023

@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
Copy link

Janpot commented Dec 5, 2023

Does it fail on

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

@Skn0tt
Copy link
Collaborator Author

Skn0tt commented Dec 5, 2023

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

@Skn0tt
Copy link
Collaborator Author

Skn0tt commented Dec 5, 2023

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
Copy link

Janpot commented Dec 5, 2023

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
Copy link
Collaborator Author

Skn0tt commented Dec 6, 2023

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:

Screenshot 2023-12-06 at 11 57 42

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

@Janpot
Copy link

Janpot commented Dec 20, 2023

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
Copy link
Collaborator Author

Skn0tt commented Dec 20, 2023

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.

@Skn0tt Skn0tt closed this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

superjson throws on constructor property
2 participants