Remove entire first line when JSON XSSI prevention seen #283
Conversation
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
I think you have some conflicts with current master, I believe you'll need to rebase and regenerate the dist files.
I put r- because the tests fail in node <= 0.12. Otherwise I left some nits that you're free to approve or ignore :)
lib/util.js
Outdated
| * JSON. | ||
| */ | ||
| function parseJSON(str) { | ||
| if (str.startsWith(")]}'")) { |
There was a problem hiding this comment.
startsWith fails in node 0.10/0.12, sadly: https://travis-ci.org/mozilla/source-map/builds/279505665
There was a problem hiding this comment.
I wouldn't mind to stop supporting these versions but then I think you'll need a major version change.
There was a problem hiding this comment.
I'd reuse the previous regexp to do this as a replacement.
There was a problem hiding this comment.
I'm planning a major version bump anyhow, due to other pending PRs.
0.10 was still seeing new releases last year; so, ugh, I guess we carry on.
| function SourceMapConsumer(aSourceMap) { | ||
| var sourceMap = aSourceMap; | ||
| if (typeof aSourceMap === 'string') { | ||
| sourceMap = JSON.parse(aSourceMap.replace(/^\)\]\}'/, '')); |
There was a problem hiding this comment.
note: if you reuse the same regexp (see below), please note that only the parens needs escaping.
There was a problem hiding this comment.
I did do it this way fwiw.
lib/util.js
Outdated
| // No line, return nothing. | ||
| return null; | ||
| } | ||
| str = str.substring(index + 1); |
There was a problem hiding this comment.
Wondering if we should then assert that the source map format revision is > 3. I think we don't need to, but it's your call.
There was a problem hiding this comment.
I think that's done elsewhere.
lib/util.js
Outdated
| var index = str.indexOf("\n"); | ||
| if (index < 0) { | ||
| // No line, return nothing. | ||
| return null; |
There was a problem hiding this comment.
I'd rather throw an error, just like JSON.parse throws an error if there is no input.
lib/util.js
Outdated
| } | ||
| return JSON.parse(str); | ||
| } | ||
| exports.parseJSON = parseJSON; |
There was a problem hiding this comment.
nitpicking: I don't like the name as it looks like the only thing it does is parsing JSON :) Maybe "parseSourceMapInput" ?
tromey
force-pushed
the
json-xssi-avoidance
branch
from
b3d026b to
54e79b8
Compare
lib/util.js
Outdated
| * JSON. | ||
| */ | ||
| function parseSourceMapInput(str) { | ||
| return JSON.parse(str.replace(/^\)]}'(?:[^\n]*?\n)/, '')); |
There was a problem hiding this comment.
Some simplifications:
- You don't need the grouping :)
- you don't need the ungreedy variant
*?thanks to your character class
so I think /^\)]}'[^\n]*\n/ works fine and is simpler :)
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
Let's go, but with the simpler version of the regexp :)
The source map specification says that the entire first line should be removed when the source map string starts with the JSON XSSI preventing string. Fixes mozilla#278
tromey
force-pushed
the
json-xssi-avoidance
branch
from
54e79b8 to
de68f7f
Compare
2 participants
The source map specification says that the entire first line should be
removed when the source map string starts with the JSON XSSI preventing
string.
Fixes #278