Dangerous bug with unsafe JSON characters

[aseemk]

Hey folks,

We just had site-wide downtime this morning due to some user input that was stringifying to JSON just fine on the server, but was failing to parse as JS in the browser.

Our Toffee snippet looked like this:

<script>
    // ...
    var props = #{props};
    // ...
</script>

props is a JS object, so this was using the default JSON stringifying Toffee provides.

However, it turns out there are two unsafe characters in JSON that fail to parse as JS:

https://medium.com/joys-of-javascript/json-js-42a28471221d

And indeed, our user input had one.

—

As that article notes, the fix is to explicitly escape those two characters:

function jsStringify(obj) {
 return JSON.stringify(obj)
 .replace(/\u2028/g, '\\u2028')
 .replace(/\u2029/g, '\\u2029');
}

Since Toffee is a templating language, and since I'm sure a common use (like for us) of the default JSON escaping is to render to JS, it feels like it'd be good for this to be the behavior in Toffee.

(Or at the very least, this could be a separate #{js ...} escape function... but either way, feels like that should be the default for cases like #{props}.)

Do you guys agree? If so, I think it's just expanding this line, is that right?

https://github.com/malgorithms/toffee/blob/4b0dda409cc7576f315e5db66365f54444b0bc72/src/view.coffee#L44

Thanks for the consideration.

/cc @tarajane @gscottolson

[malgorithms]

wow, it's a clear failure of my use of github that I somehow let this issue slip. I don't recall ever actually reading it! apologies to the 3 of you (I see the other 2 cc'ed)...I assume you moved off toffee since this wasn't fixed for a year...

@aseemk I actually added you as a collaborator on the project. if you want to check out the pr I just wrote, you can see if it looks satisfactory including a test.

aside, looking back at this stuff: toffee works pretty well, but wow, it really is pretty messy code/project setup, because it's one of the first things I ever made in node or coffeescript. good thing it has a bunch of tests.

[malgorithms]

I believe this is fixed, so closing out...