Hi Thomas,
thank you very much for the detailed and thorough review -- I really
appreciate it!
On 2011/04/01 23:39:50, tbroyer wrote:
Sorry, it took me some time but I went read the StreamHtmlParser code
to better
understand how it works, and how it's used here in the generator.
I thus found that there's a special-case for "meta refresh" that this
patch
doesn't handle (see comments below), and that the ATTR_TYPE.URI is
based on
HTML4 attributes only [1], while HTML5 added a few more:
- formaction
- icon (<command icon="">)
- manifest (<html manifest="">)
- poster (<video poster="">)
Given that we're interested in security here, I thought I'd bring this
issue.
You might want to talk to the team behind JSilver (as they're all
Googlers too)
so they update the code, and/or patch the version of the
StreamHtmlParser used
in GWT, and/or handle it in the generator (instead of, or in addition
to,
testing isUrlStart, add a test for
HTML5_URI_ATTRIBUTES.contains(streamHtmlParser.getAttribute()).
Thanks for bringing this up. I think addressing this in the parser is
the right place. I know the author of the parser and will bring this up
with him.
Thanks again!
--xtof
LGTM otherwise.
[1]
http://code.google.com/p/jsilver/source/browse/trunk/src/com/google/streamhtmlparser/util/HtmlUtils.java?r=10#137
http://gwt-code-reviews.appspot.com/1396803/diff/1/user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java
File user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java
(right):
http://gwt-code-reviews.appspot.com/1396803/diff/1/user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java#newcode282
user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java:282:
Preconditions.checkState(lookBehind == '"' || lookBehind == '\'',
This will fail if someone includes <meta content="0;URL={0}"> in a
template,
while isUrlStart will be true. This is the single case where
isUrlStart() should
be used instead of getValueIndex()==0 (the other special case is if
insertText()
was called in a ATTR_TYPE.URI, but we never call insertText()).
Replacing the above isUrlStart() with
getAttributeType()==ATTR_TYPE.URI &&
getValueIndex()==0 would make the <meta> example above fall into the
HtmlContext.Type.ATTRIBUTE_VALUE case where the URL wouldn't be
treated as a URL
and sanitized, so it's not acceptable.
The only solution seems to be to keep the isUrlStart() test but look
at the char
preceeding the attribute value in the template for the quote char.
Something
like:
template.charAt(parsePosition - streamHtmlParser.getValueIndex() -
1);
In most cases, getValueIndex() would be 0 so it will be equivalent to
lookBehind, and in the <meta> case it would still work OK to "find"
the end of
the attribute value (in the <meta> case, the URL should end the
attribute value,
so it's OK to then compare with the 'lookAhead' char).
Other solution: explicitly test for the <meta content=> case
("meta".equals(getTag()) && "content".equals(getAttribute())) and
throw an
UnableToCompleteException().
http://gwt-code-reviews.appspot.com/1396803/diff/1/user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java
File
user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java
(right):
http://gwt-code-reviews.appspot.com/1396803/diff/1/user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java#newcode122
user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java:122:
"<a
href='{0}'>{1}</a>");
Add a test for the <meta content='0;URL={0}'> case?
http://gwt-code-reviews.appspot.com/1396803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
