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

README: Replace deserialize eval with JSON.parse #38

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

unlobito
Copy link

From json.org:

The eval function is very fast. However, it can compile and execute any JavaScript program, so there can be security issues. The use of eval is indicated when the source is trusted and competent. It is much safer to use a JSON parser. In web applications over XMLHttpRequest, communication is permitted only to the same origin that provide that page, so it is trusted. But it might not be competent. If the server is not rigorous in its JSON encoding, or if it does not scrupulously validate all of its inputs, then it could deliver invalid JSON text that could be carrying dangerous script. The eval function would execute the script, unleashing its malice.

This updates README.md to suggest using JSON.parse instead.

@yahoocla
Copy link

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

This updates README.md to suggest using [JSON.parse](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse) instead of eval to address security concerns.
@gronke
Copy link

gronke commented Jun 18, 2018

But isn't the serialization of complex objects the main purpose of this utility? So being able to deserialize it in the same way seems to be a plausible cause for the eval.

@unlobito you're right that this is dangerous and the warning sign should be huge.

@redonkulus redonkulus merged commit 85b0435 into yahoo:master Jan 9, 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.

4 participants