http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safecss/shared/SafeCssProperties.java
File user/src/com/google/gwt/safecss/shared/SafeCssProperties.java
(right):

http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safecss/shared/SafeCssProperties.java#newcode107
user/src/com/google/gwt/safecss/shared/SafeCssProperties.java:107: * -
Some implementations of SafeCssProperties would in principle be able to
This sentence got formatted wrong?

http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):

http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode185
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:185:
// SafeCss is safe in a CSS context, so we can use the string without
I think we should HTML escape the values of style= attributes; this will
prevent the attribute-escape bug that's mentioned in the
SafeHtmlProperties documentation.
Unless I'm totally confused, the browser will first HTML-unescape the
style attribute and then pass it to the CSS parser, i.e. HTML escaping
quotes and single quotes (and <> markup) will not change behavior, but
will make sure the HTML attribute is correctly parsed in its entirety.

http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode264
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:264:
*
Trailing blank, here and below...

http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode284
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:284:
* Verify that the parameter type if used in the correct context. Safe
"is used"?

http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode328
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:328:
// Warn against using unsafe parameters in a CSS attribute context.
It's worth noting that even if a SafeCssProperties is used, the template
isn't 100% guaranteed to be safe, because we don't know if the template
variable appears in a "composable" CSS context.  For example,
@Template("<div style=\"background:url('{0}')\">"
SafeHtml div(SafeCssProperties safeCss);
will not result in a compiler warning, and _could_ potentially mask an
exploitable bug -- since CSS quotes in the parameter are "inside out", a
CSS property escape could happen, e.g if the value of CSS is something
like,
http://foo'; background:url(javascript:evil()); font'
Now, it's very unlikely that the code that constructs safeCss will allow
this, but the potential exists.

There are a few ways to clean this up:
- Augment the stream parser to dive into CSS and determine CSS context
(I'm not sure how hard this is; presumably not easy, otherwise they'd
done it)

- Only allow template variables in CSS_ATTRIBUTE context to appear at
the very beginning of the style attribute (without warning). This by
definition is a composable CSS context we can be sure of.  This smells a
bit awkward, but might be an overall reasonable solution.

http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safehtml/shared/SafeHtml.java
File user/src/com/google/gwt/safehtml/shared/SafeHtml.java (right):

http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/safehtml/shared/SafeHtml.java#newcode63
user/src/com/google/gwt/safehtml/shared/SafeHtml.java:63: * Notes
regarding serialization: - It may be reasonable to allow
This got formatted funny as well at some point.

http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/user/cellview/client/CellTree.java
File user/src/com/google/gwt/user/cellview/client/CellTree.java (right):

http://gwt-code-reviews.appspot.com/1384801/diff/3008/user/src/com/google/gwt/user/cellview/client/CellTree.java#newcode954
user/src/com/google/gwt/user/cellview/client/CellTree.java:954:
cssBuilder.append("width: " + res.getWidth() + "px;");
I wonder if code like this might be a bit more readable if
SafeCssPropertiesBuilder was used. Perhaps to avoid excessive verbosity,
we could have
safeCssBuilder.appendFromTrustedString("width:" + .. + "px;")
as a shorthand for
safeCssBuilder.append(SafeCssUtils.fromTrustedString(...));

The advantage is that the code can then be written such that each append
adds a single CSS property, and to code review for correctness I have to
only eyeball each .append individually rather than having to consider
the whole builder.  Perhaps not a big deal in practice though.

http://gwt-code-reviews.appspot.com/1384801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to