+1 on backing out the templates. On Aug 18, 2010 7:15 AM, "Christoph Kern" <[email protected]> wrote: > 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
-- http://groups.google.com/group/Google-Web-Toolkit-Contributors
