http://gwt-code-reviews.appspot.com/1384801/diff/3003/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/3003/user/src/com/google/gwt/safecss/shared/SafeCssProperties.java#newcode37 user/src/com/google/gwt/safecss/shared/SafeCssProperties.java:37: * All implementing classes must maintain the class invariant (by design and This description of the class invariant is rather abstract. I think users would find it helpful if we gave them some examples of valid and invalid strings. As I understand it, the empty string, "width: 1em;" and "width: 1em; height: 1em;" are safe, but just "1em" is not. It's also an error to leave off the trailing semicolon. http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/src/com/google/gwt/safecss/shared/SafeCssPropertiesString.java File user/src/com/google/gwt/safecss/shared/SafeCssPropertiesString.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/src/com/google/gwt/safecss/shared/SafeCssPropertiesString.java#newcode40 user/src/com/google/gwt/safecss/shared/SafeCssPropertiesString.java:40: SafeCssPropertiesString(String css) { perhaps have an assert that the string is either empty or ends with a semicolon, just to catch blatantly wrong usage? http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/src/com/google/gwt/safecss/shared/SafeCssPropertiesUtils.java File user/src/com/google/gwt/safecss/shared/SafeCssPropertiesUtils.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/src/com/google/gwt/safecss/shared/SafeCssPropertiesUtils.java#newcode31 user/src/com/google/gwt/safecss/shared/SafeCssPropertiesUtils.java:31: * code should be carefully reviewed to ensure the argument meets the As a convenience to the user, maybe put some concrete examples here too. (I suspect this JavaDoc would be the first documentation seen by someone reading user code in an IDE, and concrete examples will make it blindingly obvious if someone's doing it wrong.) http://gwt-code-reviews.appspot.com/1384801/diff/3003/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/3003/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode146 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:146: * <li>If the parameter is not of type {@link String}, it is first converted need to update javadoc http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode185 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:185: doEscape = false; My understanding is that we're in the middle of a style="" attribute? I'm somewhat out of my depth on XSS attacks, but it looks like turning off escaping is unsafe. If I'm reading this correctly, an unescaped quote is particularly dangerous, and someone calling fromTrustedString() would probably think it's okay: http://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet#RULE_.234_-_CSS_Escape_Before_Inserting_Untrusted_Data_into_HTML_Style_Property_Values Also, see the very bottom of this page: http://www.w3.org/TR/CSS21/syndata.html I'll see if I can get someone on the security team to review. http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java File user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/3003/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java#newcode80 user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:80: public void testTemplateWithCssAttribute() { Could you add a test for SafeCssProperties value that contains a quote character? (For example, the "content" property.) Also, content strings containing escaped versions of these characters: "<>& http://gwt-code-reviews.appspot.com/1384801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
