JohnWeisz / TypedJSON

Typed JSON parsing and serializing for TypeScript that preserves type information.
MIT License
603 stars 64 forks source link

Using `.eval`? #83

Closed michalrus closed 5 years ago

michalrus commented 6 years ago

Hey,

in one place of our frontend code, we allow the user to supply their own JSON. Obviously, we want to validate it.

Apparently,

TypedJSON.parse('alert("hmm")');

displays the alert.

Can you elaborate? Why is this user-supplied code evaluated?

JohnWeisz commented 6 years ago

I'm convinced this is done by the JSON polyfill, which is automatically included with the earlier version of TypedJSON. The polyfill has since been removed, and is no longer present in the new version (in the v1-experimental folder).

michalrus commented 6 years ago

Oh, I see!

But should it be used at all in my browser (recent Chrome), if I have:

> JSON.parse
ƒ parse() { [native code] }

?

Taking this into consideration, would you recommend against using that earlier version of TypedJSON.parse to validate user-supplied text, security-wise (XSS etc.)?

michalrus commented 6 years ago

What if some backend sent us malformed JSON, containing JS code? That’s pretty disconcerting. =(

JohnWeisz commented 6 years ago

Looking back at it now, I'm convinced this polyfill was coming from the JSON page on MDN. Since then, the polyfill has been removed from the English version, but as of today, it's still available on the Italian version, I believe it's an exact match to the polyfill used in the older version of TypedJSON.

I believe the reason for removal is not at all unlike the concerns you phrased here, although I'm not sure why the polyfill is being used in your case, any reasonably recent Chrome should have native JSON support.

michalrus commented 6 years ago

@JohnWeisz, so, basically, stable version of TypedJSON.parse introduces XSS with every use on untrusted JSON data, doesn’t it?

Could you then, please, remove that eval() shim from the stable version?

michalrus commented 6 years ago

although I'm not sure why the polyfill is being used in your case, any reasonably recent Chrome should have native JSON support.

I don’t know that either.

Could it have something to do with how the project is built? It’s built with something called Angular-CLI, and I’ve heard we’re using Angular v2. Maybe that polyfill’s if is evaluated earlier than in browser’s runtime (in build-time?), and the polyfill is used for all browsers…


My FE dev decided to introduce a regexp that tells them whether a string is JS or JSON, and reject the first case. Is that a reasonable approach? IMO not really, and a grotesque one…

michalrus commented 6 years ago

Anyhow,

Could you then, please, remove that eval() shim from the stable version?

Thank you in advance!

Neos3452 commented 5 years ago

In the latest version (1.2.3) there is no eval used. (It will still show up if you grep for it since the old sources are present, but it is not used.)

michalrus commented 5 years ago

Thank you.