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 improper character escaping #21

Closed
wants to merge 1 commit into from

Conversation

norwood
Copy link

@norwood norwood commented Feb 19, 2014

this will switch express-state to use json2.

pertinent lines in json2: https://github.com/douglascrockford/JSON-js/blob/master/json2.js#L351

@ericf
Copy link
Collaborator

ericf commented Feb 19, 2014

Could you explain in more detail why the built-in JSON.stringify is not sufficient? I want to understand an example of where someone would hit the escaping difference. Also, is there a related v8 bug or discussion for this? Thanks.

@norwood
Copy link
Author

norwood commented Feb 19, 2014

we had user data where someone had used http://www.fileformat.info/info/unicode/char/2028/index.htm which is a line terminator, which is not a problem in json, but is in js. when the browser tries to parse the js it reads it as an unescaped newline in the middle of the json string.

relevant blog post: http://timelessrepo.com/json-isnt-a-javascript-subset
here is a relevant stackoverflow article: http://stackoverflow.com/questions/2965293/javascript-parse-error-on-u2028-unicode-character
here is the ecmascript standards portion: http://www.ecma-international.org/ecma-262/5.1/#sec-7.3

@ericf
Copy link
Collaborator

ericf commented Feb 19, 2014

@norwood thanks for providing this info! I'll take a look and all this and evaluate whether we should simply escape the white space chars, or go the full route of using JSON2.

@ericf ericf added bug and removed enhancement labels Feb 20, 2014
@ericf ericf mentioned this pull request Feb 20, 2014
@ericf
Copy link
Collaborator

ericf commented Feb 20, 2014

@norwood I created #22 as an alternate fix for this issue, instead of adding JSON2 as a dependency it instead escapes the line terminator characters that are invalid in JavaScript.

@ericf
Copy link
Collaborator

ericf commented Feb 20, 2014

@mathiasbynens your feedback on this issue and #22 would be much appreciated. Thanks!

@mathiasbynens
Copy link

Yep, U+2028 and U+2029 are different in JavaScript vs. JSON. From a quick glance, the patch in #22 seems to account for those perfectly.

Also note that you may want to use escape sequences for lone surrogates in JSON-formatted data. If raw lone surrogate ‘symbols’ are used in strings, JSON parsing/serialization still works fine, but it may cause issues when the serialized JSON data is then passed to a UTF-8 decoder.

The second paragraph of the jsesc README mentions these problems: https://github.com/mathiasbynens/jsesc#readme

To work around both of them, you could use jsesc with its json option enabled: http://mths.be/jsesc#json

@ericf
Copy link
Collaborator

ericf commented Feb 21, 2014

@mathiasbynens thanks for your help!

@ericf ericf closed this in 4d60e48 Feb 21, 2014
@ericf
Copy link
Collaborator

ericf commented Feb 21, 2014

@norwood I've merged #22 (the alternate implantation that doesn't introduce a new dependency) and released v1.1.2: https://github.com/yahoo/express-state/releases/tag/v1.1.2

@norwood
Copy link
Author

norwood commented Feb 21, 2014

awesome. thanks for the quick turnaround

@mathiasbynens
Copy link

So you won’t be taking care of lone surrogates? Just wondering what the rationale is there.

@ericf
Copy link
Collaborator

ericf commented Feb 21, 2014

I will address those in another PR. I ran into issues with jsesc because of how this package needs to serialize values and revive those in the browser. I need to dig into it more, but didn't want that to hold this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants