Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Newline/Paragraph separators not escaped for use in JSONP #1132

Closed
tidoust opened this Issue · 14 comments

4 participants

François Daoust TJ Holowaychuk Camilo Aguilar Douglas Christopher Wilson
François Daoust

For better or for worse, JSON is not a true subset of JavaScript as two characters sneaked their way into JSON:

  • \u2028: line separator
  • \u2029: paragraph separator

For JavaScript, these two characters are considered to be the same as \n. That's transparent most of the time, but JSONP is all about returning JSON wrapped into JavaScript and, as such, the JSON that gets wrapped must conform to the JavaScript language. JSON.stringify does not guarantee that (well, it depends on the parser used, but the one in node.js does not).

In particular, the following code will trigger a JavaScript exception if fetched from within a Web browser:

var express = require('express');
var app = express.createServer();

app.enable('jsonp callback');
app.get('/', function (req, res) {
 res.json({ property: 'hello\u2028world' }, { 'Content-Type': 'text/javascript' });
});
app.listen(5000);

The separator characters need to be escaped before they are returned, with code such as:
body.replace(/\u2028/g, '\\u2028').replace(/\u2029/g, '\\u2029')

See JSON: The JavaScript subset that isn't for a good summary of the problem.

TJ Holowaychuk
tj commented

it uses JSON.stringify(), so I dont want to mess with anything it does, if there really is a problem it's with v8

TJ Holowaychuk tj closed this
François Daoust

Er, not sure if I was clear enough. JSON.stringify() does the right thing: it does not escape \U+2028 and \U+2029 because they do not need to be escaped per the JSON spec. JSON.stringify() return a string that is perfectly valid in JS. That's not the problem.

The problem arises when this string is wrapped in JS code for use in JSONP. In that case, the client receives JavaScript, not JSON, and uses the JavaScript parser. When that parser bumps into the U+2028 character, it considers it's a newline and as newlines are not allowed in the middle of strings in JavaScript it throws an exception.

In short, the problem lies with the JSONP mode in Express, not with JSON.stringify().

TJ Holowaychuk
tj commented

ohhh sorry somehow I missed the P on JSON haha

TJ Holowaychuk tj reopened this
Camilo Aguilar

I'm running into this issue as well right now @visionmedia

TJ Holowaychuk
tj commented

I wont have time to look at it right away, I head out of town tomorrow morning

Camilo Aguilar

But, we have the case where the JSON returned by expressjs through res.json is not escaping \u2028 and \u2029 and it's breaking other applications. I think having body.replace(/\u2028/g, '\\u2028').replace(/\u2029/g, '\\u2029'); in res.json would be pretty useful and safe.

Douglas Christopher Wilson
Collaborator

It looks like the "bug" will not be fixed in v8, because fixing it would violate es5. Using the replace solution from @c4milo would be the correct solution, then.

body.replace(/\u2028/g, '\\u2028').replace(/\u2029/g, '\\u2029');

Or perhaps the following (not sure which is faster):

body.replace(/[\u2028\u2029]/g, function (c) { return '\\u' + c.charCodeAt(0).toString(16); });
Camilo Aguilar

@visionmedia any update?

TJ Holowaychuk tj closed this in 64a2349
Douglas Christopher Wilson
Collaborator

@c4milo @visionmedia the issue doesn't actually affect JSON. Those characters do not need to be escaped in JSON, only in JavaScript. Since JSONP is technically JavaScript, that's why it is a problem there.

// Works (JSON):
console.log(JSON.parse(JSON.stringify(["test\u2028ing"])))

// Doesn't work (JSONP):
eval('console.log(' + JSON.stringify(["test\u2028ing"]) + ')')

Putting the replaces in res.json is unnecessary and would just impact performance for no reason, i.m.o.

Douglas Christopher Wilson
Collaborator

@c4milo Your example there was not even JSON. JSON is always a string, but you gave an object as an argument to JSON.parse which is what the error is from. You meant to write JSON.parse("{\"foo\":\"h\u2028i\"}") which works, because the argument to JSON.parse is a string.

Camilo Aguilar

LOL, yeah you are totally right. Gah, that's what happen when you work from home when two kids are visiting

Douglas Christopher Wilson
Collaborator

Anyway, the thing is JSON.stringify in v8 does not escape the two characters, which is correct according to JSON spec. The issue is only when the JSON is inserted into JavaScript code, which is only when performing a JSONP response (not a direct JSON response, which is what the stock res.json does). Thus only res.jsonp needs the replacement done on the JSON string. Your example res.json override above is doing double-duty with JSON and JSONP, which the stock res.json does not do, thus why the replacements are not needed within res.json.

Camilo Aguilar

yes sir, no need for that middleware at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.