pugjs / js-stringify

Stringify an object so it can be safely inlined in JavaScript code
MIT License
19 stars 4 forks source link

Escape html #2

Closed ForbesLindesay closed 9 years ago

ForbesLindesay commented 9 years ago

This change will make js-stringify safe to use on un-trusted input in inline scripts within HTML

TimothyGu commented 9 years ago

This sounds kind of out-of-scope for a module whose goal is to make code safe to be "inlined in JavaScript code." How about an option instead?

Also, why "Ensure output is never valid JSON"?

Plus, this breaks on {'<script>': 'blah'}.

ForbesLindesay commented 9 years ago

The issue is that it doesn't make code safe to inline within a script tag, which is probably a moderately common use case.

The "<script>" as a key example is a good point though, clearly I have a bit more work to do to get this right.

I don't really want to make it an option, because I think libraries should be "secure by default".

Because it often won't be valid JSON all the time, we should make sure people don't accidentally use it in place of JSON.stringify.

ForbesLindesay commented 9 years ago

I've removed the brackets since they were an un-related change. I've used unicode escapes instead of string concatenation, which is more efficient and works for keys as well as values. In addition, this means that unless the top level item is undefined or a Date, the output will still be valid JSON. This can therefore be released as a non-breaking change.

TimothyGu commented 9 years ago

Looks good.

alubbe commented 9 years ago

I'm on the road right now and can't really read code, but I have two questions:

  1. where / when do we use this? We are using a .replace chain that we know we can do faster, so what's the impact of that?
  2. can't we reuse jade_escape for 4 of the replacements? And if depending on jade-runtime is too big, can't we extract jade_escape into its own repo?

------ Originalnachricht ------ Von: "Timothy Gu" notifications@github.com An: "jadejs/js-stringify" js-stringify@noreply.github.com Gesendet: 27.08.2015 17:52:54 Betreff: Re: [js-stringify] Escape html (#2)

Looks good.

— Reply to this email directly or view it on GitHub.

TimothyGu commented 9 years ago

@alubbe said:

  1. where / when do we use this? We are using a .replace chain that we know we can do faster, so what's the impact of that?

We don't strictly need to do faster. It is used in the jade compilation process which is already very slow.

alubbe commented 9 years ago

Then LGTM Am 27.08.2015 18:53 schrieb "Timothy Gu" notifications@github.com:

@alubbe https://github.com/alubbe said:

  1. where / when do we use this? We are using a .replace chain that we know we can do faster, so what's the impact of that?

We don't strictly need to do faster. It is used in the jade compilation process which is already very slow.

— Reply to this email directly or view it on GitHub https://github.com/jadejs/js-stringify/pull/2#issuecomment-135521918.

ForbesLindesay commented 9 years ago

Only potential issue is that this will increase the size of the output code of jade by escaping all occurrences of < and > in the source (which is a lot). Any minifier would fix that though, and it's necessary if you wanted to be able to safely inline a compiled jade template in a script tag.