On Wed, Aug 18, 2010 at 01:44, <[email protected]> wrote:
> On 2010/08/17 23:23:39, xtof wrote:
>
>> On 2010/08/17 23:05:06, tbroyer wrote:
>> >
>> > Looking at the code more closely it would merely "fail" by overly
>> > rejecting tags that are whitelisted: i.e. "<b foo=<i>should be
>> > bold" would be sanitized to "<b foo=<i>should be bold" and the
>> > end part would be italicized instead of bold.
>>
>
> And that is exactly correct behavior for this class as document. It
>> only claims to accept HTML with attribute-free tags within the
>> whitelist. It doesn't claim to do anything particularly sensible
>> with input that doesn't fit this constraint; it does however claim
>> that whatever it outputs is safe (will not result in XSS/script
>> execution).
>>
>
> Oops, yes, sorry, I can't tell how it happened but I misread the
> "whitelisting" code (matches the whole thing between '<' and '>', so any
> attribute, or even whitespace, as in "<b >bold</b>", would make it fail
> and thus be escaped).
> It's fine then. Sorry again for the noise.
>
No prob, always better to err on the side of caution!
>
> Still, there's a small issue with the fact that
> SafeHtmlTemplatesGenerator doesn't use the HTML5 serialization algorithm
> (or any similar one): @Template("<br/>") will result in "<br></br>"
> which is interpreted by browsers as "<br><br>" [1], which makes it
> impossible to generate a single "line break" in a SafeHtmlTemplates.
>
> [1] http://www.w3.org/TR/html5/tokenization.html#parsing-main-inbody
> (search for « An end tag whose tag name is "br" », it's there for
> "compat with the Web")
Agreed. Another potential problem with this parser is that it's too strict
-- it insists on well-formed XHTML, but that's a much stronger constraint
than needed to ensure the SafeHtml type contract.
For example,
@Template("<a href=\"{0}\">")
SafeHtml openATag(String href);
is perfectly safe, and might be useful in something where one needs to
conditionally assemble various pieces of HTML markup, like,
SafeHtmlBuilder shb;
//...
shb.append(openATag(someUrl));
if (...) {
shb.append(someThing)
} else {
shb.append(someThingElse);
}
To ensure the SafeHtml type contract, the parser need only record the
HTML-context ("inner text", "url-valued attribute", "style", etc) of the
positional parameters, and require that the template ends in "inner text"
context (to ensure that SafeHtmls are safely appendable to each other).
Note that a bug in code like the above could result in non-well-formed HTML
(like unbalanced tags), but not unsafe HTML that might cause XSS (that is of
course absent bugs in the SafeHtml generators themselves).
As I'd mentioned, I'm proposing to replace the current XML based parser with
the java streamhtmlparser, which has this property.
I'd be fine with removing the templating code from this change and land it
separately along with the new parser.
cheers,
--xtof
>
> http://gwt-code-reviews.appspot.com/771801/show
>
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
