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
