adding HtmlEscape to dart:convert

adding HtmlEscape to dart:convert

BUG=https://code.google.com/p/dart/issues/detail?id=1657
R=blois@google.com, floitsch@google.com, rnystrom@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27211

[kevmoo-old, 2013-08-26 22:52:43]

Getting started on this...FYI at the moment

[kevmoo-old, 2013-08-26 22:59:32]

This replaces copy-paste impls in pkg classes

Looking at html5lib -
https://github.com/dart-lang/html5lib/blob/32980d2eee3fb55e7c8d2c9abc2b700408...

Open questions:
- support for \u00A0 ->   ?
- support bool attributeMode?
- use a StringBuilder instead of chained replaceAll?

TODOs:
- docs
- startChunkedConversion
- tests

[Jennifer Messerly, 2013-08-26 23:01:28]

https://codereview.chromium.org/23492002/diff/1/sdk/lib/convert/html.dart
File sdk/lib/convert/html.dart (right):

https://codereview.chromium.org/23492002/diff/1/sdk/lib/convert/html.dart#new...
sdk/lib/convert/html.dart:13: String convert(String data) {
I wonder if this impl should be based on HTML5 specs:

spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-end.html#esca...

and here is an impl from html5lib:
https://github.com/dart-lang/bleeding_edge/blob/af36812bd97d7b280869bbd364ce6...

the differences:
* single quote does not need to be escaped
* non-breaking space is escaped
* less-than and greater-than don't need to be escaped in attribute values, only
in other contexts

[Jennifer Messerly, 2013-08-26 23:02:26]

On 2013/08/26 23:01:28, John Messerly wrote:
> https://codereview.chromium.org/23492002/diff/1/sdk/lib/convert/html.dart
> File sdk/lib/convert/html.dart (right):
> 
>
https://codereview.chromium.org/23492002/diff/1/sdk/lib/convert/html.dart#new...
> sdk/lib/convert/html.dart:13: String convert(String data) {
> I wonder if this impl should be based on HTML5 specs:
> 
> spec:
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-end.html#esca...
> 
> and here is an impl from html5lib:
>
https://github.com/dart-lang/bleeding_edge/blob/af36812bd97d7b280869bbd364ce6...
> 
> the differences:
> * single quote does not need to be escaped
> * non-breaking space is escaped
> * less-than and greater-than don't need to be escaped in attribute values,
only
> in other contexts

oops, just saw your follow-up :)

[Lasse Reichstein Nielsen, 2013-08-27 06:07:40]

Definitely attributeMode, but an enum, not a bool. We want both single-quoted
and double-quoted, and then default to element mode. So, call it "HTMLEscapeMode
mode" with three options "ELEMENT", "ATTRIBUTE_SQ", "ATTRIBUTE_DQ" (names as
suggestion, even if I don't like abbreviations, I also don't like writing too
much).

Definitely use a string buffer!

What is the reason for escaping U+00A0?

https://codereview.chromium.org/23492002/diff/1/sdk/lib/convert/html.dart
File sdk/lib/convert/html.dart (right):

https://codereview.chromium.org/23492002/diff/1/sdk/lib/convert/html.dart#new...
sdk/lib/convert/html.dart:13: String convert(String data) {
We could have different escapes for different contexts.
The obvious contexts would be:
- element content (escape & and <, and maybe >)
- attribute content
  - double quoted (escape " and &)
  - single quoted (escape ' and &)
The characters that need encoding are different in each case.
We could then have a default version that escapes everything needed for every
context, but a knowledgeable user can pick a more specific one if they want.

For unquoted attribute content, we should probably not make an escape - if you
think you need escaping, you should quote it. Unquoted attribute values have a
very restricted set of characters allowed.

https://codereview.chromium.org/23492002/diff/1/sdk/lib/convert/html.dart#new...
sdk/lib/convert/html.dart:18: .replaceAll("'", "&apos;");
This is horribly inefficient. It's ok as a first go, but we should really
optimize it for a single traversal, or two at most (one to check if there is
anything to replace, and if there is, copy from the start to there into a
StringBuffer and continue).

[Lasse Reichstein Nielsen, 2013-08-27 06:10:34]

And we want a mode "UNKNOWN", which is the default and escapes everything,
ofcourse.

[Bob Nystrom, 2013-08-27 18:03:57]

Unittest changes LGTM.

https://codereview.chromium.org/23492002/diff/12001/sdk/lib/convert/html_esca...
File sdk/lib/convert/html_escape.dart (right):

https://codereview.chromium.org/23492002/diff/12001/sdk/lib/convert/html_esca...
sdk/lib/convert/html_escape.dart:9: class HtmlEscape extends Converter<String,
String> {
Document all of this.

[Jennifer Messerly, 2013-08-27 18:26:17]

btw, thinking about this more, the html5 escaper might be a starting point, but
it's specifically concerned with use in *DOM serialization* and therefore only
needs to be worry about the specified serialized form of HTML, which uses double
quoted attributes *always*. If unquoted or single quoted attributes are a
concern, those modes would need to be supported too.

As http://wonko.com/post/html-escaping mentions, if you are injecting user input
in *unquoted* HTML attributes, you're gonna have a bad time :). You'd need to
escape a lot more things in that context.

(fwiw, using Dart's innerHtml should be pretty safe from XSS now that Pete's
change is in. So client side is in pretty good shape.)

@lrn -- off the top of my head I have no idea about why   is escaped in DOM
serialization. It is an "ambiguous character" in visual appearance but other
than that, I dunno. It might just be historical quirk.

https://codereview.chromium.org/23492002/diff/1/sdk/lib/convert/html.dart
File sdk/lib/convert/html.dart (right):

https://codereview.chromium.org/23492002/diff/1/sdk/lib/convert/html.dart#new...
sdk/lib/convert/html.dart:13: String convert(String data) {
On 2013/08/27 06:07:40, Lasse Reichstein Nielsen wrote:
> We could have different escapes for different contexts.
> The obvious contexts would be:
> - element content (escape & and <, and maybe >)
> - attribute content
>   - double quoted (escape " and &)
>   - single quoted (escape ' and &)
> The characters that need encoding are different in each case.
> We could then have a default version that escapes everything needed for every
> context, but a knowledgeable user can pick a more specific one if they want.
> 
> For unquoted attribute content, we should probably not make an escape - if you
> think you need escaping, you should quote it. Unquoted attribute values have a
> very restricted set of characters allowed.

Using an enum for context SGTM. For attribute context you have single quoted,
double quoted, and unquoted as options.

I'm not sure the intent of this escaper but if it's to prevent XSS, there's good
data here:

https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_S...

and here:

http://www.w3.org/International/questions/qa-escapes#use

https://codereview.chromium.org/23492002/diff/1/sdk/lib/convert/html.dart#new...
sdk/lib/convert/html.dart:18: .replaceAll("'", "&apos;");
On 2013/08/27 06:07:40, Lasse Reichstein Nielsen wrote:
> This is horribly inefficient. It's ok as a first go, but we should really
> optimize it for a single traversal, or two at most (one to check if there is
> anything to replace, and if there is, copy from the start to there into a
> StringBuffer and continue).

+1

[kevmoo-old, 2013-08-27 20:52:22]

Using StringBuffer, better tests, introduced enum

still cranking on this.
I'll let you know when *I* think I'm done.
But happy to get feedback in the meantime.

https://codereview.chromium.org/23492002/diff/1/sdk/lib/convert/html.dart
File sdk/lib/convert/html.dart (right):

https://codereview.chromium.org/23492002/diff/1/sdk/lib/convert/html.dart#new...
sdk/lib/convert/html.dart:18: .replaceAll("'", "'");
On 2013/08/27 06:07:40, Lasse Reichstein Nielsen wrote:
> This is horribly inefficient. It's ok as a first go, but we should really
> optimize it for a single traversal, or two at most (one to check if there is
> anything to replace, and if there is, copy from the start to there into a
> StringBuffer and continue).

Done.

https://codereview.chromium.org/23492002/diff/1/sdk/lib/convert/html.dart#new...
sdk/lib/convert/html.dart:18: .replaceAll("'", "'");
On 2013/08/27 18:26:17, John Messerly wrote:
> On 2013/08/27 06:07:40, Lasse Reichstein Nielsen wrote:
> > This is horribly inefficient. It's ok as a first go, but we should really
> > optimize it for a single traversal, or two at most (one to check if there is
> > anything to replace, and if there is, copy from the start to there into a
> > StringBuffer and continue).
> 
> +1

Done.

[floitsch, 2013-08-30 15:52:00]

LGTM.
Please add test that must escape the first and/or last character of a string.

https://codereview.chromium.org/23492002/diff/24001/sdk/lib/convert/html_esca...
File sdk/lib/convert/html_escape.dart (right):

https://codereview.chromium.org/23492002/diff/24001/sdk/lib/convert/html_esca...
sdk/lib/convert/html_escape.dart:38: var ch = text[i];
Nit: we usually type everything in the dart: libraries.

Use codeUnitAt instead of operator[]. It avoids allocating a new string. (You
will have to introduce a few constants for the characters that need to be
escaped below. Ex:
const _AMPERSAND = 0x26;

https://codereview.chromium.org/23492002/diff/24001/sdk/lib/convert/html_esca...
sdk/lib/convert/html_escape.dart:76:
_sink.add(_escape.convert(chunk.substring(start, end)));
Add:
 TODO(floitsch): Optimize.

https://codereview.chromium.org/23492002/diff/24001/tests/lib/convert/html_es...
File tests/lib/convert/html_escape_test.dart (right):

https://codereview.chromium.org/23492002/diff/24001/tests/lib/convert/html_es...
tests/lib/convert/html_escape_test.dart:14: const _OUTPUT_ATTRIBUTE = "A <test>
of &nbsp; &quot;double&quot; &amp; \'single\' values";
80 chars.

https://codereview.chromium.org/23492002/diff/24001/tests/lib/convert/html_es...
tests/lib/convert/html_escape_test.dart:16: const _OUTPUT_ELEMENT = 'A
&lt;test&gt; of &nbsp; "double" &amp; \'single\' values';
80 chars.

https://codereview.chromium.org/23492002/diff/24001/tests/lib/convert/html_es...
tests/lib/convert/html_escape_test.dart:18: void main() {
move main to the bottom of the file.

[Jennifer Messerly, 2013-08-30 18:13:59]

https://codereview.chromium.org/23492002/diff/24001/sdk/lib/convert/html_esca...
File sdk/lib/convert/html_escape.dart (right):

https://codereview.chromium.org/23492002/diff/24001/sdk/lib/convert/html_esca...
sdk/lib/convert/html_escape.dart:38: var ch = text[i];
On 2013/08/30 15:52:00, floitsch wrote:
> Nit: we usually type everything in the dart: libraries.

it is a shame styles in different parts of the code have diverged.
should we update:
https://www.dartlang.org/articles/style-guide/#prefer-using-var-without-a-typ...
?

[blois, 2013-09-04 15:59:58]

lgtm

[floitsch, 2013-09-04 16:37:28]

One last request: could you please create an issue "Document html_escape" and
add TODOs(issueNumber) to the most important constants and functions.

[kevmoo-old, 2013-09-05 19:14:20]

https://codereview.chromium.org/23492002/diff/24001/tests/lib/convert/html_es...
File tests/lib/convert/html_escape_test.dart (right):

https://codereview.chromium.org/23492002/diff/24001/tests/lib/convert/html_es...
tests/lib/convert/html_escape_test.dart:14: const _OUTPUT_ATTRIBUTE = "A <test>
of &nbsp; &quot;double&quot; &amp; \'single\' values";
On 2013/08/30 15:52:00, floitsch wrote:
> 80 chars.

Done.

https://codereview.chromium.org/23492002/diff/24001/tests/lib/convert/html_es...
tests/lib/convert/html_escape_test.dart:16: const _OUTPUT_ELEMENT = 'A
&lt;test&gt; of &nbsp; "double" &amp; \'single\' values';
On 2013/08/30 15:52:00, floitsch wrote:
> 80 chars.

Done.

https://codereview.chromium.org/23492002/diff/24001/tests/lib/convert/html_es...
tests/lib/convert/html_escape_test.dart:18: void main() {
On 2013/08/30 15:52:00, floitsch wrote:
> move main to the bottom of the file.

Done.

[kevmoo-old, 2013-09-05 20:30:05]

Optimized addSlice in latest commit

https://codereview.chromium.org/23492002/diff/24001/sdk/lib/convert/html_esca...
File sdk/lib/convert/html_escape.dart (right):

https://codereview.chromium.org/23492002/diff/24001/sdk/lib/convert/html_esca...
sdk/lib/convert/html_escape.dart:76:
_sink.add(_escape.convert(chunk.substring(start, end)));
On 2013/08/30 15:52:00, floitsch wrote:
> Add:
>  TODO(floitsch): Optimize.

Does the last change handle your request to optimize?
https://codereview.chromium.org/23492002/diff2/39001:42001/sdk/lib/convert/ht...

[kevmoo-old, 2013-09-05 20:31:21]

Committed patchset #10 manually as r27211 (presubmit successful).

[Jennifer Messerly, 2013-09-05 20:41:36]

drive by comments

https://codereview.chromium.org/23492002/diff/5001/sdk/lib/convert/html_escap...
File sdk/lib/convert/html_escape.dart (right):

https://codereview.chromium.org/23492002/diff/5001/sdk/lib/convert/html_escap...
sdk/lib/convert/html_escape.dart:10: class HtmlEscapeMode {
nit: I think you could just have one TODO about the documentation issue on the
class:

// TODO(floitsch) - Document - Issue 13097

https://codereview.chromium.org/23492002/diff/5001/sdk/lib/convert/html_escap...
sdk/lib/convert/html_escape.dart:17: static const HtmlEscapeMode UNKNOWN =
suggestion: rename UNKNOWN_QUOTED_ATTRIBUTE

(the current escaping rules are not be safe to use in an unknown context or
unquoted attributes)

suggested documentation comment:

/**
 * Escape the string for use in an HTML quoted attribute context, when it
 * is not known if the HTML uses single or double quotes.
 *
 * This will escape less-than `<`, greater-than `>`, single-quote `'`, 
 * double-quote `"`, ampersand `&`, and no-break-space `U+00A0`.
 */

https://codereview.chromium.org/23492002/diff/5001/sdk/lib/convert/html_escap...
sdk/lib/convert/html_escape.dart:21: static const HtmlEscapeMode ATTRIBUTE =
DOUBLE_QUOTE_ATTRIBUTE

and add SINGLE_QUOTE_ATTRIBUTE variant.


alternatively:

QUOTED_ATTRIBUTE

that escapes single and double quote.

https://codereview.chromium.org/23492002/diff/5001/sdk/lib/convert/html_escap...
sdk/lib/convert/html_escape.dart:22: const HtmlEscapeMode._('attribute', false,
true, false);
Shouldn't this include less than and greater than as well? even the relatively
minimalist DOM serializer escapes those even in doube quoted attributes

https://codereview.chromium.org/23492002/diff/5001/sdk/lib/convert/html_escap...
sdk/lib/convert/html_escape.dart:25: static const HtmlEscapeMode ELEMENT =
ELEMENT_CONTENT

https://codereview.chromium.org/23492002/diff/5001/sdk/lib/convert/html_escap...
sdk/lib/convert/html_escape.dart:29: const HtmlEscapeMode._(this._name,
this.escapeLtGt, this.escapeQuot,
named arguments to make call the definitions more readable?

[kevmoo-old, 2013-09-07 20:34:15]

Replies a couple of John's comments.

Stay tuned for a new CL for comments.

https://codereview.chromium.org/23492002/diff/5001/sdk/lib/convert/html_escap...
File sdk/lib/convert/html_escape.dart (right):

https://codereview.chromium.org/23492002/diff/5001/sdk/lib/convert/html_escap...
sdk/lib/convert/html_escape.dart:22: const HtmlEscapeMode._('attribute', false,
true, false);
On 2013/09/05 20:41:37, John Messerly wrote:
> Shouldn't this include less than and greater than as well? even the relatively
> minimalist DOM serializer escapes those even in doube quoted attributes

The argument is that they are unneeded. Only in elements. If one wants to be
paranoid, use the default behavior.

https://codereview.chromium.org/23492002/diff/5001/sdk/lib/convert/html_escap...
sdk/lib/convert/html_escape.dart:29: const HtmlEscapeMode._(this._name,
this.escapeLtGt, this.escapeQuot,
On 2013/09/05 20:41:37, John Messerly wrote:
> named arguments to make call the definitions more readable?

Agreed.