[gwt-contrib] Re: Don't allow SafeHtml strings to end in a script or style context. (issue1608803)
On 2011/12/08 18:29:14, jlabanca wrote: LGTM LGTM http://gwt-code-reviews.appspot.com/1608803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Change SafeHtmlHostedModeUtils.isCompleteHtml() to public. (issue1606804)
On 2011/12/07 19:38:28, mdempsky wrote: LGTM. http://gwt-code-reviews.appspot.com/1606804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Rewrite SafeUriHostedModeUtils#isValid without regexp (issue1449814)
LGTM. Also, jlabanca kindly offered to submit this patch for you. Thanks! --xtof http://gwt-code-reviews.appspot.com/1449814/diff/5/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java File user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java (right): http://gwt-code-reviews.appspot.com/1449814/diff/5/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode65 user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:65: public static boolean isValidUriCharset(String uri) { Can you make this package private? I think this might not work with super sourced classes though... http://gwt-code-reviews.appspot.com/1449814/diff/5/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode136 user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:136: uri = UriUtils.encodeAllowEscapes(uri); On 2011/06/09 14:32:41, tbroyer wrote: This is a breaking change, as it now allows %foo (% not followed by 2-hex-digits) where it previously didn't. Given that we allow otherwise-erroneous unescaped unicode chars (along with a few others, such as unescaped and ), and that browsers allow %foo, I think it's a change for the better. I think that's fine. And it wouldn't actually break anything since it loosens validation (I assume that's why you put breaking change in quotes?) http://gwt-code-reviews.appspot.com/1449814/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Rewrite SafeUriHostedModeUtils#isValid without regexp (issue1449814)
http://gwt-code-reviews.appspot.com/1449814/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java File user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java (right): http://gwt-code-reviews.appspot.com/1449814/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode117 user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:117: if (Character.isHighSurrogate((char) codePoint) || Character.isLowSurrogate((char) codePoint)) { Long lines, here and below. http://gwt-code-reviews.appspot.com/1449814/diff/1/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java File user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java (right): http://gwt-code-reviews.appspot.com/1449814/diff/1/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java#newcode96 user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java:96: assertEquals(CONSTANT_URL, UriUtils.fromTrustedString(CONSTANT_URL).asString()); While you're here, could you change fromTrustedString to fromSafeConstant where the argument is a constant? In keeping with the desire that fromTrustedString should be used as little as possible... http://gwt-code-reviews.appspot.com/1449814/diff/1/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java#newcode109 user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java:109: public void testFromTrustedString_withInvalidUrl() { Given that this test passed even with the original regex check commented out, would it make sense to to split the character set check of isValidUri out and test it separately? http://gwt-code-reviews.appspot.com/1449814/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)
http://gwt-code-reviews.appspot.com/1447812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Disable regex checking of URLs in SafeUriHostedModeUtils; this regex appears to (issue1443813)
Reviewers: tbroyer, jlabanca, jat, pdr, Description: Disable regex checking of URLs in SafeUriHostedModeUtils; this regex appears to cause stack overflows in some cases. Please review this at http://gwt-code-reviews.appspot.com/1443813/ Affected files: M user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java Index: user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java === --- user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java (revision 10284) +++ user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java (working copy) @@ -104,9 +104,11 @@ } private static boolean isValidUri(String uri) { -if (!uri.matches(HREF_UCSCHAR)) { - return false; -} +// TODO(xtof): The regex appears to cause stack overflows in some cases. +// Investigate and re-enable) +// if (!uri.matches(HREF_UCSCHAR)) { +// return false; +// } /* * pre-process to turn href-ucschars into ucschars, and encode to URI. * -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Disable regex checking of URLs in SafeUriHostedModeUtils; this regex appears to (issue1443813)
http://gwt-code-reviews.appspot.com/1443813/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Make UriUtils#unsafeCastFromUntrustedString gracefully handle null for (issue1443814)
Reviewers: tbroyer, jlabanca, jat, pdr, rjrjr, Description: Make UriUtils#unsafeCastFromUntrustedString gracefully handle null for backwards compatibility. Please review this at http://gwt-code-reviews.appspot.com/1443814/ Affected files: M user/src/com/google/gwt/safehtml/shared/UriUtils.java Index: user/src/com/google/gwt/safehtml/shared/UriUtils.java === --- user/src/com/google/gwt/safehtml/shared/UriUtils.java (revision 10284) +++ user/src/com/google/gwt/safehtml/shared/UriUtils.java (working copy) @@ -263,7 +263,7 @@ * safe!/strong * * @param s the input String - * @return a SafeUri instance + * @return a SafeUri instance, or null if codes/code is null * @deprecated This method is intended only for use in APIs that use * {@link SafeUri} to represent URIs, but for backwards * compatibility have methods that accept URI parameters as plain @@ -271,7 +271,11 @@ */ @Deprecated public static SafeUri unsafeCastFromUntrustedString(String s) { -return new SafeUriString(s); +if (s != null) { + return new SafeUriString(s); +} else { + return null; +} } // prevent instantiation -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1446815)
Reviewers: tbroyer, jlabanca, jat, skybrian, Description: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. Support SafeUri-typed arguments in SafeHtmlTemplates. Also added a few overloads in c.g.g.user.client. Note that this is a breaking change in the sense that DataResource and ImageResource have a new getSafeUri method, as well as overloaded constructors taking URIs in the form of SafeUri. Patch by: t.bro...@gmail.com Fixes issues: 6145 Please review this at http://gwt-code-reviews.appspot.com/1446815/ Affected files: M tools/api-checker/config/gwt23_24userApi.conf M user/src/com/google/gwt/cell/client/ImageResourceCell.java M user/src/com/google/gwt/resources/Resources.gwt.xml M user/src/com/google/gwt/resources/client/DataResource.java M user/src/com/google/gwt/resources/client/ImageResource.java A user/src/com/google/gwt/resources/client/impl/DataResourcePrototype.java M user/src/com/google/gwt/resources/client/impl/ExternalTextResourcePrototype.java M user/src/com/google/gwt/resources/client/impl/ImageResourcePrototype.java M user/src/com/google/gwt/resources/css/Spriter.java M user/src/com/google/gwt/resources/rg/DataResourceGenerator.java M user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java M user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java M user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java M user/src/com/google/gwt/safehtml/shared/SafeHtmlUtils.java A user/src/com/google/gwt/safehtml/shared/SafeUri.java A user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java A user/src/com/google/gwt/safehtml/shared/SafeUriString.java M user/src/com/google/gwt/safehtml/shared/UriUtils.java M user/src/com/google/gwt/user/client/ui/AbstractImagePrototype.java M user/src/com/google/gwt/user/client/ui/FormPanel.java M user/src/com/google/gwt/user/client/ui/Frame.java M user/src/com/google/gwt/user/client/ui/Image.java M user/src/com/google/gwt/user/client/ui/ImageResourceRenderer.java M user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java M user/src/com/google/gwt/user/client/ui/impl/ClippedImageImplIE6.java M user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java A user/super/com/google/gwt/safehtml/super/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java M user/test/com/google/gwt/resources/client/ImageResourceTest.java M user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java A user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java A user/test/com/google/gwt/safehtml/shared/UriUtilsTest.java M user/test/com/google/gwt/user/client/ui/ImageTest.java M user/test/com/google/gwt/user/client/ui/impl/ClippedImagePrototypeTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)
http://gwt-code-reviews.appspot.com/1447812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)
Thanks for the review! Please take another look... --xtof http://gwt-code-reviews.appspot.com/1447812/diff/1/tools/api-checker/config/gwt23_24userApi.conf File tools/api-checker/config/gwt23_24userApi.conf (right): http://gwt-code-reviews.appspot.com/1447812/diff/1/tools/api-checker/config/gwt23_24userApi.conf#newcode155 tools/api-checker/config/gwt23_24userApi.conf:155: com.google.gwt.user.client.ui.impl.ClippedImageImpl::adjust(Lcom/google/gwt/dom/client/Element;Ljava/lang/String;) MISSING On 2011/06/02 13:47:05, jlabanca wrote: Instead of all of these exceptions for ClippedImageImpl, you can exclude com.google.gwt.user.client.ui.impl from api checks. We don't make any guarantees about the API of impl classes. Done. http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/resources/client/DataResource.java File user/src/com/google/gwt/resources/client/DataResource.java (right): http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/resources/client/DataResource.java#newcode66 user/src/com/google/gwt/resources/client/DataResource.java:66: * will be an absolute URL. On 2011/06/02 13:47:05, jlabanca wrote: Should this be deprecated like ImageResource? Done. http://gwt-code-reviews.appspot.com/1447812/diff/1/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/1447812/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode432 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:432: if (isSafeUri(parameterType)) { On 2011/06/02 13:47:05, jlabanca wrote: Is it safe to use safeUri in a text context? Seems like a mistake at the least. It would be safe here, since it's going to be HTML escaped just like any other string. I can't think of too many reasons anyone would legitimately do this. Perhaps in a template used to linkify URLs, as in a href='{0}'{0}/a where {0} is a SafeUri. Seems like a pretty unlikely scenario, and I think I'll remove this special case here in the interest of simplicity. In any case, per your comment above I've made the change so that this would throw an error. http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java File user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java (right): http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode32 user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:32: public class SafeUriHostedModeUtils { On 2011/06/02 13:47:05, jlabanca wrote: Should this class be package protected? It looks like an impl class, but its public in a non-impl package, which makes it look like anyone can use it. That doesn't seem to work; if I make it package private I get java.lang.IllegalAccessError: com/google/gwt/safehtml/shared/SafeUriHostedModeUtils at com.google.gwt.safehtml.shared.UriUtils.fromTrustedString(UriUtils.java:199 all over the unit tests. I'm guessing that super-sourced classes need to be public. http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode39 user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:39: * RFC 3987bis Web Addresses/a On 2011/06/02 13:47:05, jlabanca wrote: Will this look correct in javadoc? You might need to exceed the 100 line limit here. Done. http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java#newcode54 user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java:54: public static final String FORCE_CHECK_VALID_URI = com.google.gwt.safehtml.ForceCheckValidUri; On 2011/06/02 13:47:05, jlabanca wrote: Can you add a comment explaining how this is set? Done. http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/UriUtils.java File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right): http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode209 user/src/com/google/gwt/safehtml/shared/UriUtils.java:209: * safe!/strong On 2011/06/02 17:51:54, jlabanca wrote: unsafeCastFromUntrustedString() is better. Can we also deprecate the method? Done. I'm having second thoughts about deprecating its callers though (a bunch of methods of Image). We haven't deprecated plain string methods for SafeHtml versions elsewhere either, so this would be somewhat inconsistent. I think I'll leave the Image(String) methods alone for now? Use code should always be able to use one of the other methods. Only library code (GWT and libraries based on GWT) have the legacy support problem. On 2011/06/02 17:45:16, xtof wrote
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)
http://gwt-code-reviews.appspot.com/1447812/diff/1/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/1447812/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode432 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:432: if (isSafeUri(parameterType)) { On 2011/06/03 17:00:04, jlabanca wrote: On 2011/06/03 07:39:23, xtof wrote: On 2011/06/02 13:47:05, jlabanca wrote: Is it safe to use safeUri in a text context? Seems like a mistake at the least. It would be safe here, since it's going to be HTML escaped just like any other string. I can't think of too many reasons anyone would legitimately do this. Perhaps in a template used to linkify URLs, as in a href='{0}'{0}/a where {0} is a SafeUri. Seems like a pretty unlikely scenario, and I think I'll remove this special case here in the interest of simplicity. In any case, per your comment above I've made the change so that this would throw an error. I don't think its even possible. The check in emitParameterExpression ensures that SafeUri is only used in a URL_ATTRIBUTE_ENTIRE context. Right. Sorry that's what I meant to say, but clearly didn't state it very well :) http://gwt-code-reviews.appspot.com/1447812/diff/1003/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/1447812/diff/1003/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java#newcode58 user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:58: @Template(spanb{0}/bspan{1}/span) On 2011/06/03 17:00:04, jlabanca wrote: Missing a closing span. If you aren't testing something specific to the malformed HTML, I suggest you add the closing span back on. Done. http://gwt-code-reviews.appspot.com/1447812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)
http://gwt-code-reviews.appspot.com/1447812/diff/1/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/1447812/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode305 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:305: // not a fatal error. On 2011/06/02 13:47:05, jlabanca wrote: Why isn't this a fatal error? Using a SafeUri in the middle of a URL attribute seems just as bad as the above case, and using it outside of the URL context seems like a dev mistake. I think I agree; we should treat this analogous to the SafeStyles case above. Thomas, ok with you to make that change? http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/UriUtils.java File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right): http://gwt-code-reviews.appspot.com/1447812/diff/1/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode209 user/src/com/google/gwt/safehtml/shared/UriUtils.java:209: * safe!/strong On 2011/06/02 13:47:05, jlabanca wrote: This method worries me. When I saw the name, I assumed it was the equivalent of fromString(). Anyone who looks at the method name without reading the JavaDoc might assume the same. I suggest we remove the method and let users manage unsafe URIs. That forces the user to make the tough decisions, whether they sanitize the URI, or call fromTrustedString() even if the URI isn't trusted. This method is intended for use in places where a string we don't know anything about needs to be turned into a SafeUri in a legacy-API situation. For instance in this CL, the Image class has been refactored to use SafeUri internally. However, it retains the Image(String uri) constructor, which uses this method to turn the string into a SafeUri to call the Image(SafeUri uri) constructor with. I'd prefer that we don't use the fromTrustedString method in those situations: Use of that method is essentially an assertion by the programmer that they can ensure from context that the argument satisfies the SafeUri contract. In the Image(String) case, this is not so. I agree that the name isn't scary enough though. Perhaps, unsafeCastFromUntrustedString or something like that? http://gwt-code-reviews.appspot.com/1447812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)
Reviewers: tbroyer, jlabanca, jat, skybrian, Description: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. Support SafeUri-typed arguments in SafeHtmlTemplates. Also added a few overloads in c.g.g.user.client. Note that this is a breaking change in the sense that DataResource and ImageResource have a new getSafeUri method, as well as overloaded constructors taking URIs in the form of SafeUri. Patch by: t.bro...@gmail.com Fixes issues: 6145 Please review this at http://gwt-code-reviews.appspot.com/1447812/ Affected files: M tools/api-checker/config/gwt23_24userApi.conf M user/src/com/google/gwt/cell/client/ImageResourceCell.java M user/src/com/google/gwt/resources/Resources.gwt.xml M user/src/com/google/gwt/resources/client/DataResource.java M user/src/com/google/gwt/resources/client/ImageResource.java A user/src/com/google/gwt/resources/client/impl/DataResourcePrototype.java M user/src/com/google/gwt/resources/client/impl/ExternalTextResourcePrototype.java M user/src/com/google/gwt/resources/client/impl/ImageResourcePrototype.java M user/src/com/google/gwt/resources/css/Spriter.java M user/src/com/google/gwt/resources/rg/DataResourceGenerator.java M user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java M user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java M user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java M user/src/com/google/gwt/safehtml/shared/SafeHtmlUtils.java A user/src/com/google/gwt/safehtml/shared/SafeUri.java A user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java A user/src/com/google/gwt/safehtml/shared/SafeUriString.java M user/src/com/google/gwt/safehtml/shared/UriUtils.java M user/src/com/google/gwt/user/client/ui/AbstractImagePrototype.java M user/src/com/google/gwt/user/client/ui/FormPanel.java M user/src/com/google/gwt/user/client/ui/Frame.java M user/src/com/google/gwt/user/client/ui/Image.java M user/src/com/google/gwt/user/client/ui/ImageResourceRenderer.java M user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java M user/src/com/google/gwt/user/client/ui/impl/ClippedImageImplIE6.java M user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java A user/super/com/google/gwt/safehtml/super/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java M user/test/com/google/gwt/resources/client/ImageResourceTest.java M user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java A user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java A user/test/com/google/gwt/safehtml/shared/UriUtilsTest.java M user/test/com/google/gwt/user/client/ui/ImageTest.java M user/test/com/google/gwt/user/client/ui/impl/ClippedImagePrototypeTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1447812)
This is a continuation of the review of http://gwt-code-reviews.appspot.com/1380806, authored by http://gwt-code-reviews.appspot.com/user/tbroyer, to capture minor changes made by the submitter. For the main code review discussion, see issue 1380806. http://gwt-code-reviews.appspot.com/1447812/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1380806)
On 2011/06/01 00:10:58, tbroyer wrote: On 2011/05/31 23:38:17, xtof wrote: My apologies dropping the response on this thread (I'd missed the last question and had assumed this was good to submit). Also, I didn't realize that I actually need to fetch and submit this patch (I'm not part of GWT team proper and wasn't familiar with the process for changes from external developers). No problem. Otherwise, proposed changes SGTM. I can't seem to upload an updated patch set for this issue (presumably since you created it). I'll mail you the patch set separately. http://gwt-code-reviews.appspot.com/1380806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1380806)
http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gwt/safehtml/shared/SafeUri.java File user/src/com/google/gwt/safehtml/shared/SafeUri.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gwt/safehtml/shared/SafeUri.java#newcode41 user/src/com/google/gwt/safehtml/shared/SafeUri.java:41: * context in which the URL is used matters too (link {@code href} vs. image On 2011/05/02 18:57:13, skybrian wrote: This sounds somewhat dangerous, actually. It seems like if context matters, either each context should have its own type, or creators of SafeUrl instances should stick to the lowest common denominator. The reason is that the code creating a SafeUrl instance might be far removed from the code that uses it. If the creator believes that a URL will be used in iframe context, but actually it isn't, then reviewers cannot find the problem without having an end-to-end understanding of the program's data flow, and any refactoring of intermediate code has the potential to introduce a bug without a type error and without the reviewers seeing anything wrong. It seems like the whole point of having safe types with clear contracts is make sure that local reviews are sufficient to guarantee safety? I hate the complexity this is likely to introduce, but on the other hand, a SafeUrl type that isn't actually safe doesn't sound so great either. I think I agree with Brian on this one. Can we specify the contract such that a SafeUri is safe to use in any of these contexts (at the expense of being overly restrictive, for e.g. img src URIs)? If not, I think we need to introduces separate types. Otherwise we'll loose the local revieability benefit of the SafeXyz types. http://gwt-code-reviews.appspot.com/1380806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1380806)
LGTM http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode172 user/src/com/google/gwt/safehtml/shared/UriUtils.java:172: public static SafeUri fromTrustedString(String s) { On 2011/04/18 18:14:21, skybrian wrote: Could you put some examples of safe and unsafe URLs in the javadoc? This should make it more obvious to reviewers of calls to this method what they should be looking for. I think it's a bit more complicated than that. In particular, it matters where the value came from. For example, we need to consider data: URIs as inherently dangerous. _however_ a data: URI that's fully program controlled (e.g., a resource) is considered inherently safe. I.e. the provenance of the value matters more than what the actual value looks like. Which is also why the SafeUri contract (as well as the SafeHtml) has this vague language about safe from cross site scripting. In principle, the contract could actually be written to state that evaluating the URI must not result in execution of script, unless the script is fully under program control. I wonder if we should make that change; for instance one might create a SafeUri object for 'javascript:void(0);'. Which does execute script, but is harmless because the script is fully program controlled. Which means it's not cross site scripting. I wonder if we should specify this in the SafeUri contract at this level of detail? http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/user/client/ui/Image.java File user/src/com/google/gwt/user/client/ui/Image.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/user/client/ui/Image.java#newcode144 user/src/com/google/gwt/user/client/ui/Image.java:144: private String url = null; On 2011/04/23 14:36:22, tbroyer wrote: On 2011/04/18 16:22:32, xtof wrote: I'm still wondering about the change we discussed earlier, of making this a SafeUri, and using UriUtils#fromUntrustedString in the Image(String url) constructor. You'd have to add getSafeUri to ClippedState (or just change getUrl to return the SafeUri -- after all this is a private class so there should be only internal uses). Done. Changed everything to SafeUri internally, with String-based public APIs delegating to SafeUri-based ones, wrapping the String with fromUntrustedString. Nice! I'm glad this worked out :) http://gwt-code-reviews.appspot.com/1380806/diff/29005/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/1380806/diff/29005/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode302 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:302: // TODO(xtof): refactor HtmlContext with isStart/isEnd/isEntire accessors an simplified type. an[d] simplified? http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/user/client/ui/Image.java File user/src/com/google/gwt/user/client/ui/Image.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/user/client/ui/Image.java#newcode301 user/src/com/google/gwt/user/client/ui/Image.java:301: public abstract void setVisibleRect(Image image, int left, int top, int width, int height); Maybe undo this line wrap? http://gwt-code-reviews.appspot.com/1380806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: First step of isolating a bunch of code that is used for generating (issue1422807)
LGTM http://gwt-code-reviews.appspot.com/1422807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1380806)
Thanks! just a few more comments, otherwise pretty much LGTM. http://gwt-code-reviews.appspot.com/1380806/diff/25001/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/1380806/diff/25001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode302 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:302: if (HtmlContext.Type.URL_ATTRIBUTE_START == contextType) { Hmm, I just realized there's another case: When a parameter occurs in a URI attribute but not at the beginning (e.g., img src='{http://foo.com/{0}'). In that case we get contextType==ATTRIBUTE_VALUE, and would show the warning from the else branch, which wouldn't be exactly accurate. Fixing this unfortunately isn't trivial, because that case (in a URI attribute but not at the beginning) isn't exposed in HtmlContext. Perhaps take a TODO? I'm starting to think that this code needs some refactoring... http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode65 user/src/com/google/gwt/safehtml/shared/UriUtils.java:65: StringBuilder sb = new StringBuilder(); Maybe write this as the else branch, it'll be a little easier to read I think. http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode186 user/src/com/google/gwt/safehtml/shared/UriUtils.java:186: public static SafeUri fromUntrustedString(String s) { It might make sense to call maybeCheckValidUri here too? http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode236 user/src/com/google/gwt/safehtml/shared/UriUtils.java:236: if (isSafeUri(uri)) { Since you've created SafeUriHostedModeUtils.maybeCheckValidUri(s), we perhaps should call this here too (since it's a stricter check of URI well formedness), i.e. if (SafeUriHostedModeUtils.maybeCheckValidUri(uri) isSafeUri(uri)) ? http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/user/client/ui/Image.java File user/src/com/google/gwt/user/client/ui/Image.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/user/client/ui/Image.java#newcode144 user/src/com/google/gwt/user/client/ui/Image.java:144: private String url = null; I'm still wondering about the change we discussed earlier, of making this a SafeUri, and using UriUtils#fromUntrustedString in the Image(String url) constructor. You'd have to add getSafeUri to ClippedState (or just change getUrl to return the SafeUri -- after all this is a private class so there should be only internal uses). http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java File user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java#newcode67 user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java:67: public void testEncode_withEscapesInvalidEscapes() { Maybe add a test for a URL containing utf-8 characters? http://gwt-code-reviews.appspot.com/1380806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1380806)
http://gwt-code-reviews.appspot.com/1380806/diff/25001/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/1380806/diff/25001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode302 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:302: if (HtmlContext.Type.URL_ATTRIBUTE_START == contextType) { On 2011/04/18 17:03:27, tbroyer wrote: On 2011/04/18 16:22:32, xtof wrote: Hmm, I just realized there's another case: When a parameter occurs in a URI attribute but not at the beginning (e.g., img src='{http://foo.com/{0}'). In that case we get contextType==ATTRIBUTE_VALUE, and would show the warning from the else branch, which wouldn't be exactly accurate. Actually, the message in the other case is not accurate either for non-attribute contexts. Fixing this unfortunately isn't trivial, because that case (in a URI attribute but not at the beginning) isn't exposed in HtmlContext. Perhaps take a TODO? I'm starting to think that this code needs some refactoring... How about having isAttrStart() and isAttrEnd() in addition to simpler states (URL_ATTRIBUTE, CSS_ATTRIBUTE, ATTRIBUTE, etc.)? That way, checking for URL_ATTRIBUTE_ENTIRE would be type==URL_ATTRIBUTE isAttrStart() isAttrEnd(). I think John already proposed something similar in the discussion around the SafeStyles CL. I think I agree, it's probably time for something like that. Definitely in a separate change list though :) http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode65 user/src/com/google/gwt/safehtml/shared/UriUtils.java:65: StringBuilder sb = new StringBuilder(); On 2011/04/18 17:03:27, tbroyer wrote: On 2011/04/18 16:22:32, xtof wrote: Maybe write this as the else branch, it'll be a little easier to read I think. I was thinking about maybe put it in SafeUriHostedModeUtils (with some renaming of course!) as it already handles the prodmode vs. devmode/plain-JVM with super-source (i.e. even easier to read, but the super-source would no longer be a no-op). What do you think? That would work too; I don't think I have strong opinions one way or another though... http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode186 user/src/com/google/gwt/safehtml/shared/UriUtils.java:186: public static SafeUri fromUntrustedString(String s) { On 2011/04/18 17:03:27, tbroyer wrote: On 2011/04/18 16:22:32, xtof wrote: It might make sense to call maybeCheckValidUri here too? Any URL should pass the check, but if there's a case that doesn't, it'll be a breaking change (e.g. Image widget no-longer working). I'd rather be tempted to make the do not use warning more prominent in the javadoc, maybe something like: strongDespite this method creating a SafeUri instance, no checks are performed, so the returned SafeUri is absolutely NOT guaranteed to be safe!/strong SGTM. http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode236 user/src/com/google/gwt/safehtml/shared/UriUtils.java:236: if (isSafeUri(uri)) { On 2011/04/18 17:03:27, tbroyer wrote: On 2011/04/18 16:22:32, xtof wrote: Since you've created SafeUriHostedModeUtils.maybeCheckValidUri(s), we perhaps should call this here too (since it's a stricter check of URI well formedness), i.e. if (SafeUriHostedModeUtils.maybeCheckValidUri(uri) isSafeUri(uri)) ? Well, except that maybeCheckValidUri is a precondition/assert-style method. I can't think of any case where a value passing the isSafeUri check would be harmful, even less after the encodeAllowEscapes transformation. Fair point. And it could be a breaking change (along the same lines as you noted above). http://gwt-code-reviews.appspot.com/1380806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Escape single characters in SafeHtmlBuilder/SafeHtmlUtils (external issue 6222) (issue1413802)
LGTM. http://gwt-code-reviews.appspot.com/1413802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding a new annotation SafeHtmlTemplates.SafeForCss to specify that a parameter is known to be ... (issue1384801)
LGTM! http://gwt-code-reviews.appspot.com/1384801/diff/8001/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/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode315 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:315: logger.log(TreeLogger.WARN, On 2011/04/07 15:45:57, jlabanca wrote: I added a comment explaining that we've already checked that the user did not try to use SafeStyles in a non-CSS context. The reason for the split checks is that each Safe type is only valid in one context, so it makes sense to check that separately. Otherwise, we would have to have a !SafeHtml/!SafeStyles check in every context where those classes aren't supported. Conversely, some contexts have or will have a Safe value that is appropriate for that context. Adding the warning in the switch statement allows us to do check if the user could have used a Safe class specific to the current context. If we move the warnings out of the switch statement, then we would have to check the current context to see if the user could have chosen a better Safe value, which would basically add a duplicate (subset) switch statement. I see... Makes sense and SGTM. http://gwt-code-reviews.appspot.com/1384801/diff/8025/user/src/com/google/gwt/safecss/shared/SafeStyles.java File user/src/com/google/gwt/safecss/shared/SafeStyles.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/8025/user/src/com/google/gwt/safecss/shared/SafeStyles.java#newcode75 user/src/com/google/gwt/safecss/shared/SafeStyles.java:75: * liwidth: 1em;/li Should these be {@code}? http://gwt-code-reviews.appspot.com/1384801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1380806)
I think this is pretty much ready, except for one thing I just thought of. Sorry, I should have thought of that earlier :/ In ClippedImageImpl, we're using a SafeUri in the context of a url() style expression (background: url({3})). However, the contract of SafeUri is currently not tight enough for this to be safe I think: SafeUri would allow e.g., parentheses, colons and dashes in un-escaped form in the URL. In a template where the URL appears in a url() css expression as above, a URL like, http://harmless.com/foo);background:url(javascript:evil()); would pass UriUtils#sanitizeUri (without getting URI-escaped or anything), and would result in script execution via CSS injection. So I think we need to, - tighten the SafeUri contract so it explicitly specifies which character set can occur in a SafeUri - add a sanitizeAndEscapeUri method to UriUtils that both checks for an allowed scheme and URI/%-escapes characters not in the allowed charset (or maybe rejects URLs with certain funny characters in them). I'll have to do some reading up on what characters are OK in the URI in a CSS context, will get back to you on that. Thanks again for all your work on this! --xtof http://gwt-code-reviews.appspot.com/1380806/diff/16001/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/1380806/diff/16001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode127 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:127: * liIf the template parameter occurs at the start of a URI-valued There probably should be an explanation about the distinction between start of URL-valued attribute and entire URL-valued attribute? http://gwt-code-reviews.appspot.com/1380806/diff/16001/user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java File user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/16001/user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java#newcode46 user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java:46: UriUtils.fromUntrustedString(GWT.getModuleBaseURL() + clear.cache.gif); I think here it would actually be appropriate to use fromTrustedString -- the value we're passing through here is fully under the control of the program (at least I think that GWT.getModuleBaseURL cannot be modified from the outside). Actually, if GWT.getModuleBaseURL() + someFile is a common idiom, it might be worth introducing a utility method in UriUtils that returns an absolute URL based at moduleBaseURL? If it's not common, it won't be worth the clutter though. http://gwt-code-reviews.appspot.com/1380806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding a new annotation SafeHtmlTemplates.SafeForCss to specify that a parameter is known to be ... (issue1384801)
http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStyles.java File user/src/com/google/gwt/safecss/shared/SafeStyles.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStyles.java#newcode71 user/src/com/google/gwt/safecss/shared/SafeStyles.java:71: * attribute: Maybe instead say, comply with this type's contract (here and elsewhere) -- these examples are not inherently unsafe, they just don't comply with the type contract (specifically, the composability requirement). With that in mind, perhaps move the examples after the paragraph about composability? http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStyles.java#newcode88 user/src/com/google/gwt/safecss/shared/SafeStyles.java:88: * {@code http://trackingurl.com)} is appended to {@code background:url(}, the Hmm, so generally the SafeHtml contracts don't guarantee that the resulting HTML is side-effect free; in particular when we sanitize untrusted URIs we'll leave them alone as long as they have a whitelisted scheme. I.e. we'd allow img src='{0}' where the value of {0} is http://trackingurl.com/ Complete side effect freedom in this sense would be much harder to enforce (we'd have to somehow have a way to configure filters that know which domains are trusted and which ones are third-party). Perhaps a better example would be to use javascript:evil() as a url? http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java File user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java#newcode42 user/src/com/google/gwt/safecss/shared/SafeStylesBuilder.java:42: * any escaping to it. Perhaps, any escaping or sanitization? http://gwt-code-reviews.appspot.com/1384801/diff/8001/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/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode131 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:131: * liIf the parameter is not of type {@link SafeStyles}, the result is then I think this comment should remain unchanged; we always HTML escape the attribute value even if it came from a SafeStyles. http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode176 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:176: // TODO(xtof): Throw an exception if using SafeHtml within an attribute. I think you can remove this TODO, isn't this handled now in your new checks at the start of emitParameterExpression? http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode280 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:280: * definitely isn't what the user intended to do. Maybe user - developer? http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode315 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:315: logger.log(TreeLogger.WARN, I find the code is a bit hard to understand with the checks related to CSS_ATTRIBUTE contexts partially done here, and partially done up above in line 270ff. Might it be more readable if you move the checks from above to here (possibly splitting the two cases)? Not sure on this; might make the code more verbose... Otherwise, a comment noting the precondition established by the check in 270ff might help. http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/user/cellview/client/CellBrowser.java File user/src/com/google/gwt/user/cellview/client/CellBrowser.java (right): http://gwt-code-reviews.appspot.com/1384801/diff/8001/user/src/com/google/gwt/user/cellview/client/CellBrowser.java#newcode728 user/src/com/google/gwt/user/cellview/client/CellBrowser.java:728: private static final SafeHtml LEAF_IMAGE = SafeHtmlUtils Maybe break after = ? http://gwt-code-reviews.appspot.com/1384801/diff/8001/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/8001/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java#newcode60 user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:60: SafeHtml templateWithCssAttribute(int height /* generates a compiled time warning */, compile time http://gwt-code
[gwt-contrib] Re: Adds support for the URL_ATTRIBUTE_ENTIRE parse context to HtmlTemplateParser. (issue1396803)
http://gwt-code-reviews.appspot.com/1396803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds support for the URL_ATTRIBUTE_ENTIRE parse context to HtmlTemplateParser. (issue1396803)
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
[gwt-contrib] Re: Adds support for the URL_ATTRIBUTE_ENTIRE parse context to HtmlTemplateParser. (issue1396803)
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 == '\'', On 2011/04/01 23:39:50, tbroyer wrote: 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(). I'm inclined to go with your latter suggestion and completely disallow template variables in meta content= I can't think of any sensible reason why someone would write a SafeHtmlTemplate with meta tags, so I don't think this restriction would matter in practice. Thinking about it some more; I'm wondering if a value substituted into meta-content url context would have to be sanitized more stringently than a regular uri-valued attribute. I vaguely recall there were bugs that allowed a second URL to be injected (meta .. content=0; http://example.com/legit;javascript:evil()); not sure those are still an issue. 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); On 2011/04/01 23:39:50, tbroyer wrote: Add a test for the meta content='0;URL={0}' case? Done. http://gwt-code-reviews.appspot.com/1396803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Adds support for the URL_ATTRIBUTE_ENTIRE parse context to HtmlTemplateParser. (issue1396803)
Reviewers: jlabanca, tbroyer, Description: Adds support for the URL_ATTRIBUTE_ENTIRE parse context to HtmlTemplateParser. Please review this at http://gwt-code-reviews.appspot.com/1396803/ Affected files: M user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java M user/src/com/google/gwt/safehtml/rebind/ParsedHtmlTemplate.java M user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java M user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java M user/test/com/google/gwt/safehtml/rebind/ParsedHtmlTemplateTest.java Index: user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java === --- user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java (revision 9914) +++ user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java (working copy) @@ -32,7 +32,7 @@ * * p * This parser parses templates consisting of HTML markup, with template - * variables of the form {@code {n}}. For example, a template might look like, + * variables of the form {@code {n}}. For example, a template might look like, * * pre {@code * span style={0}a href={1}/{2}{3}/a/span @@ -70,10 +70,13 @@ * dt{@link HtmlContext.Type#TEXT} * ddThis context corresponds to basic inner text. In the above example, * parameter #3 would be tagged with this context. - * dt{@link HtmlContext.Type#URL_START} + * dt{@link HtmlContext.Type#URL_ATTRIBUTE_START} * ddThis context corresponds to a parameter that appears at the very start of * a URL-valued HTML attribute's value; in the above example this applies to * parameter #1. + * dt{@link HtmlContext.Type#URL_ATTRIBUTE_ENTIRE} + * ddThis context corresponds to a parameter that comprises an entire + * URL-valued attribute, for example in {@code img src='{0}'/}. * dt{@link HtmlContext.Type#CSS_ATTRIBUTE_START} * ddThis context corresponds to a parameter that appears at the very * beginning of a {@code style} attribute's value; in the above example this @@ -135,6 +138,18 @@ private int parsePosition; /** + * The character preceding a template parameter, at the time a template + * parameter is being parsed. + */ + private char lookBehind; + + /** + * The character succeeding a template parameter, at the time a template + * parameter is being parsed. + */ + private char lookAhead; + + /** * Creates a {@link HtmlTemplateParser}. * * @param logger the {@link TreeLogger} to log to @@ -163,7 +178,9 @@ // @VisibleForTesting void parseTemplate(String template) throws UnableToCompleteException { this.template = template; -this.parsePosition = 0; +parsePosition = 0; +lookBehind = 0; +lookAhead = 0; Matcher match = TEMPLATE_PARAM_PATTERN.matcher(template); int endOfPreviousMatch = 0; @@ -174,10 +191,16 @@ parseAndAppendTemplateSegment( template.substring(endOfPreviousMatch, match.start())); parsePosition = match.start(); +lookBehind = template.charAt(parsePosition - 1); } int paramIndex = Integer.parseInt(match.group(1)); parsePosition = match.end(); + if (parsePosition template.length()) { +lookAhead = template.charAt(parsePosition); + } else { +lookAhead = 0; + } parsedTemplate.addParameter( new ParameterChunk(getHtmlContextFromParseState(), paramIndex)); @@ -208,7 +231,6 @@ * This method checks for certain illegal/unsupported template constructs, * such as template variables that occur in an un-quoted attribute (see this * class' class documentation for details). - * * @throws UnableToCompleteException if an illegal/unuspported template * construct is encountered */ @@ -256,7 +278,18 @@ throw new UnableToCompleteException(); } if (streamHtmlParser.isUrlStart()) { -return new HtmlContext(HtmlContext.Type.URL_START, tag, attribute); +// Note that we have established above that the attribute is quoted. +Preconditions.checkState(lookBehind == '' || lookBehind == '\'', +At the start of a quoted attribute, lookBehind should be a quote character; at %s, +getTemplateParsedSoFar()); +// If the the character immediately succeeding the template parameter is +// a quote that matches the one that started the attribute, we know +// that the parameter comprises the entire attribute. +if (lookAhead == lookBehind) { + return new HtmlContext(HtmlContext.Type.URL_ATTRIBUTE_ENTIRE, tag, attribute); +} else { + return new HtmlContext(HtmlContext.Type.URL_ATTRIBUTE_START, tag, attribute); +} } else if (streamHtmlParser.inCss()) { if (streamHtmlParser.getValueIndex() == 0) { return new HtmlContext(HtmlContext.Type.CSS_ATTRIBUTE_START, tag, attribute);
[gwt-contrib] Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)
Reviewers: jlabanca, pdr, Description: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. Please review this at http://gwt-code-reviews.appspot.com/1392801/ Affected files: M user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java M user/src/com/google/gwt/safehtml/rebind/ParsedHtmlTemplate.java M user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java Index: user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java === --- user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java (revision 9880) +++ user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java (working copy) @@ -74,10 +74,14 @@ * ddThis context corresponds to a parameter that appears at the very start of * a URL-valued HTML attribute's value; in the above example this applies to * parameter #1. + * dt{@link HtmlContext.Type#CSS_ATTRIBUTE_START} + * ddThis context corresponds to a parameter that appears at the very + * beginning of a {@code style} attribute's value; in the above example this + * applies to parameter #0. * dt{@link HtmlContext.Type#CSS_ATTRIBUTE} * ddThis context corresponds to a parameter that appears in the context of a - * {@code style} attribute; in the above example this applies to - * parameter #0. + * {@code style} attribute, except at the very beginning of the attribute's + * value. * dt{@link HtmlContext.Type#ATTRIBUTE_VALUE} * ddThis context corresponds to a parameter that appears within an attribute * and is not in one of the more specific in-attribute contexts above. In @@ -250,7 +254,11 @@ if (streamHtmlParser.isUrlStart()) { return new HtmlContext(HtmlContext.Type.URL_START, tag, attribute); } else if (streamHtmlParser.inCss()) { -return new HtmlContext(HtmlContext.Type.CSS_ATTRIBUTE, tag, attribute); +if (streamHtmlParser.getValueIndex() == 0) { + return new HtmlContext(HtmlContext.Type.CSS_ATTRIBUTE_START, tag, attribute); +} else { + return new HtmlContext(HtmlContext.Type.CSS_ATTRIBUTE, tag, attribute); +} } else { return new HtmlContext( HtmlContext.Type.ATTRIBUTE_VALUE, tag, attribute); Index: user/src/com/google/gwt/safehtml/rebind/ParsedHtmlTemplate.java === --- user/src/com/google/gwt/safehtml/rebind/ParsedHtmlTemplate.java (revision 9880) +++ user/src/com/google/gwt/safehtml/rebind/ParsedHtmlTemplate.java (working copy) @@ -64,7 +64,11 @@ /** * CSS (style) attribute context. */ - CSS_ATTRIBUTE + CSS_ATTRIBUTE, + /** + * At the very start of a CSS (style) attribute context. + */ + CSS_ATTRIBUTE_START } private final Type type; Index: user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java === --- user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java (revision 9880) +++ user/test/com/google/gwt/safehtml/rebind/HtmlTemplateParserTest.java (working copy) @@ -134,9 +134,14 @@ // Test correct detection of CSS context. assertParseTemplateResult( [L(div class=\), P((ATTRIBUTE_VALUE,div,class),0), L(\ style=\), ++ P((CSS_ATTRIBUTE_START,div,style),2), L(\Hello ), ++ P((TEXT,null,null),1)], +div class=\{0}\ style=\{2}\Hello {1}); +assertParseTemplateResult( +[L(div class=\), P((ATTRIBUTE_VALUE,div,class),0), L(\ style=\color:green; ), + P((CSS_ATTRIBUTE,div,style),2), L(\Hello ), + P((TEXT,null,null),1)], -div class=\{0}\ style=\{2}\Hello {1}); +div class=\{0}\ style=\color:green; {2}\Hello {1}); assertParseTemplateResult( [L(div), P((TEXT,null,null),0), L(stylefoo ), + P((CSS,null,null),1), L(/style)], -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)
http://gwt-code-reviews.appspot.com/1392801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1380806)
Thanks for your changes! There's one more thing: In the past couple of days we've had some discussions around the in URL valued attribute context. The main issue discussed is that unlike SafeHtml, SafeUri is not generally composable: Two SafeHtmls concatenated together make another SafeHtml, but for SafeUri this doesn't really work. For example, String attackerControlled = evil(); SafeUri uri0 = UriUtils.fromTrustedString(javascript:void(0);); SafeUri uri1 = UriUtils.fromString(attackerControlled); sanitizeUri will leave attackerControlled alone (it looks like a relative URL), and both uri0 and uri1 are individually harmless. However, concatenated together, the resulting URI will execute the attacker controlled script when dereferenced. With that in mind, it seems safest to only allow SafeUri to be interpolated in a uri-valued attribute if the template variable makes up the whole attribute. I.e., @Template(img src='{0}'/) SafeHtml image(SafeUri uri); would be allowed, but @Template(img src='{0}/{1}'/) SafeHtml image(SafeUri baseUri, String path); would not. If that sounds reasonable to you too, I'll make a change to expose a corresponding parse state in the parser (ENTIRE_URL_ATTRIBUTE). Thanks! --xtof http://gwt-code-reviews.appspot.com/1380806/diff/5003/user/src/com/google/gwt/user/client/ui/Image.java File user/src/com/google/gwt/user/client/ui/Image.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/5003/user/src/com/google/gwt/user/client/ui/Image.java#newcode145 user/src/com/google/gwt/user/client/ui/Image.java:145: impl.createStructure(UriUtils.fromTrustedString(url), left, top, width, height)); I'm a bit concerned about the use of UriUtils.fromTrustedString here -- this code can't really guarantee that url is trusted, it may come from the Image(String url, ...) c'tor. Perhaps this could be made a bit cleaner as follows: - use SafeUri internally in ClippedState - introduce UriUtils#fromUntrustedString, with the same implementation as fromTrustedString, but documented to be used as an unsafe cast from String to SafeUri, to be used to support legacy APIs. - Implement the public Image(String url,...) constructors in terms of the Image(SafeUri url, ...) ones, using fromUntrustedString. (note, I haven't completely thought this through and doing so might end up making the change more messy rather than cleaner; if that's the case please ignore this suggestion :) http://gwt-code-reviews.appspot.com/1380806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)
On 2011/03/29 19:30:16, jlabanca wrote: LGTM I don't actually see the TODO though. You can add it before submitting. Oops, forgot to export git before uploading. Done. Thanks! --xtof http://gwt-code-reviews.appspot.com/1392801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds support for the CSS_ATTRIBUTE_START parse context to HtmlTemplateParser. (issue1392801)
http://gwt-code-reviews.appspot.com/1392801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Add missing CSS_ATTRIBUTE_START case in SafeHtmlTemplates code generator. (issue1395803)
Reviewers: jlabanca, rjrjr, pdr, Description: Add missing CSS_ATTRIBUTE_START case in SafeHtmlTemplates code generator. Please review this at http://gwt-code-reviews.appspot.com/1395803/ Affected files: M user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java M user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java Index: user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java === --- user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java (revision 9912) +++ user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java (working copy) @@ -245,6 +245,7 @@ break; case CSS_ATTRIBUTE: + case CSS_ATTRIBUTE_START: // TODO(xtof): Improve support for CSS. logger.log(TreeLogger.WARN, Template with variable in CSS context: + The template code generator cannot guarantee HTML-safety of Index: user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java === --- user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java (revision 9912) +++ user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java (working copy) @@ -62,6 +62,9 @@ @Template(spanimg src=\{0}/{1}\//span) SafeHtml templateWithTwoPartUriAttribute(String baseUrl, String urlPart); + +@Template(span style='{0}; color: green;'/span) +SafeHtml templateWithStyleAttribute(String style); } public void testSimpleTemplate() { @@ -107,4 +110,10 @@ templates.templateWithTwoPartUriAttribute( BAD_URL, xy).asString()); } + + public void testTemplateWithStyleAttribute() { +Assert.assertEquals( +span style='background: purple; color: green;'/span, +templates.templateWithStyleAttribute(background: purple).asString()); + } } -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (issue1380806)
http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/cell/client/ImageCell.java File user/src/com/google/gwt/cell/client/ImageCell.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/cell/client/ImageCell.java#newcode58 user/src/com/google/gwt/cell/client/ImageCell.java:58: sb.append(template.img(UriUtils.fromString(value))); It seems that in this case (and similar ones down the line), changing the template signature to img(SafeUri) results in unnecessary verbosity -- when the template took a string, the template generator inserts the sanitizeUri call automatically, to the same effect. Would it make sense to take a SafeUri and expose that in the render signature, i.e make this an AbstractCellSafeUri? Would presumably be a breaking API change though. http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/cell/client/ImageResourceCell.java File user/src/com/google/gwt/cell/client/ImageResourceCell.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/cell/client/ImageResourceCell.java#newcode39 user/src/com/google/gwt/cell/client/ImageResourceCell.java:39: value).getHTML()); It would be nice if there was a getSafeHtml, to avoid the need for a fromTrustedString here (see also comment in ClippedImagePrototype). http://gwt-code-reviews.appspot.com/1380806/diff/1/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/1380806/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode117 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:117: * liIf the template parameter occurs at the start of a URI-valued The comment isn't quite correct as written, because as is it implies that processing for start-of-URI happens even if it was a SafeUri (but it is actually omitted). http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode268 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:268: if (!SAFE_URI_FQCN.equals(parameterType.getQualifiedSourceName())) { I'm not sure we want to print a warning here. I think it's quite legitimate for a template to have a String-valued variable in URL_START context, and just let the template take care of sanitization (see also comment in ImageCell.java). This is analogous to using String vs SafeHtml in inner text context. In a lot of cases it makes sense for the argument be String valued and let the template take care of escaping; only if I don't want the template to touch it I need to supply a SafeHtml. http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode340 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:340: expression = expression + .asString(); This is one of those places where it's annoying that we decided to use asString() instead of just letting toString() return the wrapped contents of the SafeXyz. Too late now... http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUri.java File user/src/com/google/gwt/safehtml/shared/SafeUri.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUri.java#newcode21 user/src/com/google/gwt/safehtml/shared/SafeUri.java:21: * vulnerabilities) in an HTML context. This should be [...] in a URI context within an HTML document, for example in a URI-valued attribute, or some such... http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriString.java File user/src/com/google/gwt/safehtml/shared/SafeUriString.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriString.java#newcode65 user/src/com/google/gwt/safehtml/shared/SafeUriString.java:65: if (!(obj instanceof SafeHtml)) { instanceof SafeUri http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/safehtml/shared/SafeUriString.java#newcode68 user/src/com/google/gwt/safehtml/shared/SafeUriString.java:68: return uri.equals(((SafeHtml) obj).asString()); SafeUri http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java File user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java#newcode68 user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java:68: return impl.getHTML(url.asString(), left, top, width, height); Would it make sense to provide a getSafeHtml() method here that returns SafeHtml and
[gwt-contrib] Re: Adding a new annotation SafeHtmlTemplates.SafeForCss to specify that a parameter is known to be ... (issue1384801)
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/ --
[gwt-contrib] Re: Adding a new annotation SafeHtmlTemplates.SafeForCss to specify that a parameter is known to be ... (issue1384801)
http://gwt-code-reviews.appspot.com/1384801/diff/6006/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/6006/user/src/com/google/gwt/safecss/shared/SafeCssProperties.java#newcode46 user/src/com/google/gwt/safecss/shared/SafeCssProperties.java:46: * By convention, {@link SafeCssProperties} should only contain single quotes Since SafeHtmlTemplates has been changed to HTML-escape the value of style attributes, perhaps it might avoid some confusion to remove the suggestion about the quotes. It wouldn't hurt to instead remind users that SafeCssProperties strings may contain literal single or double quotes, and as such the entire CSS must be HTML escaped when used in a style attribute. One thing that is important to require is that SafeCssProperties may never contain literal angle brackets. Otherwise, it could be unsafe to place a SafeCssProperties into a style tag (where it can't be HTML escaped), e.g. if the SafeCssProperties such as font: 'foo /stylescriptevil/script' is used in a style sheet in a style tag; this could then break out of the style context into HTML. http://gwt-code-reviews.appspot.com/1384801/diff/6006/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/6006/user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java#newcode185 user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:185: // escaping it. Perhaps remove the without escaping it since it is now escaped after all? http://gwt-code-reviews.appspot.com/1384801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtml Rendering for UiBinder (issue1305801)
This is really great! It pretty much completely removes uibinder out of the security-relevant codebase. http://gwt-code-reviews.appspot.com/1305801/diff/55001/user/src/com/google/gwt/uibinder/elementparsers/HtmlMessageInterpreter.java File user/src/com/google/gwt/uibinder/elementparsers/HtmlMessageInterpreter.java (right): http://gwt-code-reviews.appspot.com/1305801/diff/55001/user/src/com/google/gwt/uibinder/elementparsers/HtmlMessageInterpreter.java#newcode77 user/src/com/google/gwt/uibinder/elementparsers/HtmlMessageInterpreter.java:77: return uiWriter.tokenForSafeHtmlExpression(messages.declareMessage(message)); Methods in Messages interfaces can themselves be declared to return SafeHtml (http://code.google.com/webtoolkit/doc/latest/DevGuideI18nMessages.html#SafeHtmlMessages). I'm wondering if it would work to change MessageWriter#writeDeclaration to emit declarations of Messages methods that return SafeHtml rather than String. And if that's done, would that remove the need to use tokenForSafeHtmlExpression here (in which case that method could be deleted altogether)? http://gwt-code-reviews.appspot.com/1305801/diff/55001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java File user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java (right): http://gwt-code-reviews.appspot.com/1305801/diff/55001/user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java#newcode686 user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java:686: public String tokenForSafeHtmlExpression(String expression) { This is the one place where potential HTML unsafety could be introduced (if this method were called on an expression that is not in fact a safe constant). The only use in this CL appears to be in HtmlMessageInterpreter; see a question there if it's possible to avoid relying on this method. If so, this method could be removed. http://gwt-code-reviews.appspot.com/1305801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/diff/44001/45010 File user/src/com/google/gwt/user/server/Util.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/44001/45010#newcode76 user/src/com/google/gwt/user/server/Util.java:76: * @throws IllegalStateException if duplicate cookies are detected. IllegalStateException is probably not quite the right one here; from its javadoc: Signals that a method has been invoked at an illegal or inappropriate time. In other words, the Java environment or Java application is not in an appropriate state for the requested operation. I can't really think of a standard java exception that really fits here; IllegalArgumentException maybe? I'm not sure it's worth declaring a custom exception here either though. jat - any opinions? http://gwt-code-reviews.appspot.com/1251801/diff/44001/45014 File user/src/com/google/gwt/user/server/rpc/XsrfProtectedServiceServlet.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/44001/45014#newcode37 user/src/com/google/gwt/user/server/rpc/XsrfProtectedServiceServlet.java:37: * XSRF token validation is performed by generating MD5 hash of the session Actually, why MD5 and not future proof things and use SHA-1 or SHA-2? I'm not c ryptographer enough to know if it really matters (and my guess is collision resi stance is not important in this application), but better err on the safe side? [sorry I meant to make this comment in the last round, but somehow it got dropped, guess I didn't hit save :/ http://gwt-code-reviews.appspot.com/1251801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
Hey Meder, looks good! I have a couple more comments, also in the tests which I hadn't looked at in detail yet... cheers --xtof http://gwt-code-reviews.appspot.com/1251801/diff/30001/31010 File user/src/com/google/gwt/user/server/Util.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/30001/31010#newcode76 user/src/com/google/gwt/user/server/Util.java:76: if (cookieName == null || request == null) { Minor: Wouldn't it be better to Precondition.checkNotNull these? Seems like if they're null it's almost certainly programmer error. http://gwt-code-reviews.appspot.com/1251801/diff/30001/31010#newcode84 user/src/com/google/gwt/user/server/Util.java:84: // ensure that it's the only one cookie, since duplicate cookies Since this check (and the exception thrown if it fails) is specific to RpcTokens, it might be good to change the name of the method to something less generic? http://gwt-code-reviews.appspot.com/1251801/diff/30001/31018 File user/test/com/google/gwt/user/client/rpc/XsrfProtectionTest.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/30001/31018#newcode60 user/test/com/google/gwt/user/client/rpc/XsrfProtectionTest.java:60: public void testRpcWithoutXsrfToken() throws Exception { This should perhaps be called testRpcWithoutXsrfTokenFails? http://gwt-code-reviews.appspot.com/1251801/diff/30001/31018#newcode60 user/test/com/google/gwt/user/client/rpc/XsrfProtectionTest.java:60: public void testRpcWithoutXsrfToken() throws Exception { Also, perhaps add one more test with an RPC token that's present but wrong? This is pretty much covered by the test below with a changed session cookie, but it is a slightly different scenario and probably deserves its own test. http://gwt-code-reviews.appspot.com/1251801/diff/30001/31018#newcode127 user/test/com/google/gwt/user/client/rpc/XsrfProtectionTest.java:127: public void testXsrfTokenWithSessionCookie() throws Exception { should perhaps be called testXsrfTokenWithDifferentSessionCookieFails? http://gwt-code-reviews.appspot.com/1251801/diff/30001/31021 File user/test/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServletTest.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/30001/31021#newcode31 user/test/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServletTest.java:31: private boolean isValidateCalled = false; perhaps set this to false in setUp()? http://gwt-code-reviews.appspot.com/1251801/diff/30001/31023 File user/test/com/google/gwt/user/server/rpc/XsrfTestServiceImpl.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/30001/31023#newcode32 user/test/com/google/gwt/user/server/rpc/XsrfTestServiceImpl.java:32: StringBuffer person = new StringBuffer(); One thing you could do perhaps: change this method so that it actually has a side effect (i.e., make 'person' a field), and then check in the tests where XSRF token validation failed, this method did not actually get invoked (i.e., the state change did not happen). http://gwt-code-reviews.appspot.com/1251801/diff/30001/31023#newcode37 user/test/com/google/gwt/user/server/rpc/XsrfTestServiceImpl.java:37: // public void setSessionCookieName(String cookieName) { Perhaps just delete this? http://gwt-code-reviews.appspot.com/1251801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/diff/30001/31010 File user/src/com/google/gwt/user/server/Util.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/30001/31010#newcode76 user/src/com/google/gwt/user/server/Util.java:76: if (cookieName == null || request == null) { On 2011/01/12 19:17:43, jat wrote: Precondition isn't part of GWT, and this seems too insignificant to add another external dependency. Oh right, I forgot (I've seen it in GWT internal code iirc). Anyway, the main point is that this method perhaps should fail with a NPE if its arguments are null, rather than silently returning null. Though I don't feel strongly about that either. http://gwt-code-reviews.appspot.com/1251801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12005 File user/src/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServlet.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/11001/12005#newcode80 user/src/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServlet.java:80: !method.equals(classMethods)) { Shouldn't this be !method.equals(classMethod)) (not classMethod*s*)? Seems like there's a test case missing for this? http://gwt-code-reviews.appspot.com/1251801/diff/11001/12008 File user/src/com/google/gwt/user/server/rpc/XsrfProtectedServiceServlet.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/11001/12008#newcode39 user/src/com/google/gwt/user/server/rpc/XsrfProtectedServiceServlet.java:39: * To prevent blind XSRF cookie overwrite by an active HTTP man-in-the-middle in I think it would be worth stating more clearly that using the default XSRF cookie is vulnerable to active attacks, and is not recommended. I'm almost inclined to suggest that we should perhaps not even provide that default mode. After all, pretty much any app that worries about XSRF also uses authentication, and the XSRF token can be derived off of a session cookie or similar. http://gwt-code-reviews.appspot.com/1251801/diff/11001/12008#newcode120 user/src/com/google/gwt/user/server/rpc/XsrfProtectedServiceServlet.java:120: private boolean isCookieValueValid(String cookieValue) { I'm not too fond of how this method performs two different kinds of functions, depending on how the class is configured: If the class is configured to use a session cookie, this method seems to check that the token value is as expected, whereas if no sessionCookieName is set, it just checks for well-formedness of the cookie. Either way, I'm a bit confused as to how this works... http://gwt-code-reviews.appspot.com/1251801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12009 File user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/11001/12009#newcode192 user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java:192: setCookieAndExpireOldCookies(newXsrfCookie); If I understand this correctly, the cookie is set both when the xsrf token is a random value, _and_ when the token is generated off of a SessionCookie. In the latter case, I don't understand why it's necessary to set the XSRF cookie, and if it's not necessary I think it should be avoided. http://gwt-code-reviews.appspot.com/1251801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12008 File user/src/com/google/gwt/user/server/rpc/XsrfProtectedServiceServlet.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/11001/12008#newcode120 user/src/com/google/gwt/user/server/rpc/XsrfProtectedServiceServlet.java:120: private boolean isCookieValueValid(String cookieValue) { On 2011/01/11 01:48:15, meder wrote: On 2011/01/11 00:32:23, xtof wrote: I'm not too fond of how this method performs two different kinds of functions, depending on how the class is configured: If the class is configured to use a session cookie, this method seems to check that the token value is as expected, whereas if no sessionCookieName is set, it just checks for well-formedness of the cookie. Either way, I'm a bit confused as to how this works... This method only checks if the value itself is sane code on line 107 will compare the two values. Got it... http://gwt-code-reviews.appspot.com/1251801/diff/11001/12009 File user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/11001/12009#newcode192 user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java:192: setCookieAndExpireOldCookies(newXsrfCookie); On 2011/01/11 01:48:15, meder wrote: On 2011/01/11 00:37:06, xtof wrote: If I understand this correctly, the cookie is set both when the xsrf token is a random value, _and_ when the token is generated off of a SessionCookie. In the latter case, I don't understand why it's necessary to set the XSRF cookie, and if it's not necessary I think it should be avoided. I thought that this would make it easier for apps to work with token, app would have to only issue getNewXsrfToken() once and subsequently simply read the value from the cookie and construct XsrfToken that way. I see. Well typically I'd think a GWT client would just call the getNewXsrfToken rpc and hang on to the token in client state; I'm not sure if it needs to be in a cookie. Come to think of it, if we do provide infrastructure code that stores values in cookies, we should make it configurable with respect to path of the cookie and 'secure' attribute. Which in turn seems to introduce a fair bit of complication. Plus, if a developer really wants to store the value in a cookie, they can do so. http://gwt-code-reviews.appspot.com/1251801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Expand self-closing div/ tag in SafeHtmlTemplate into separate open and close (issue1209801)
Reviewers: rice, Description: Expand self-closing div/ tag in SafeHtmlTemplate into separate open and close tag. Please review this at http://gwt-code-reviews.appspot.com/1209801/show Affected files: M user/src/com/google/gwt/user/cellview/client/CellTable.java Index: user/src/com/google/gwt/user/cellview/client/CellTable.java === --- user/src/com/google/gwt/user/cellview/client/CellTable.java (revision 9400) +++ user/src/com/google/gwt/user/cellview/client/CellTable.java (working copy) @@ -282,7 +282,7 @@ @Template(div style=\outline:none;\ tabindex=\{0}\ accessKey=\{1}\{2}/div) SafeHtml divFocusableWithKey(int tabIndex, char accessKey, SafeHtml contents); -@Template(div class=\{0}\/) +@Template(div class=\{0}\/div) SafeHtml loading(String loading); @Template(tabletbody{0}/tbody/table) -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for RpcTokens, which, if set, are sent with each RPCRequest to (issue1107801)
LGTM! http://gwt-code-reviews.appspot.com/1107801/diff/3001/4005 File user/src/com/google/gwt/user/client/rpc/ServiceDefTarget.java (right): http://gwt-code-reviews.appspot.com/1107801/diff/3001/4005#newcode43 user/src/com/google/gwt/user/client/rpc/ServiceDefTarget.java:43: RpcToken getRpcToken(); How likely is it that developers will be providing their own implementations of ServiceDefTarget? We'd be breaking them by adding methods here. http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009 File user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java (right): http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode109 user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:109: RpcToken.Class tokenClassToUse = remoteService.findAnnotationInTypeHierarchy( 80 columns http://gwt-code-reviews.appspot.com/1107801/diff/13001/14002 File user/src/com/google/gwt/user/client/rpc/RpcToken.java (right): http://gwt-code-reviews.appspot.com/1107801/diff/13001/14002#newcode28 user/src/com/google/gwt/user/client/rpc/RpcToken.java:28: public interface RpcToken extends Serializable { As mentioned in the draft design, I'm not sure RpcToken is the best name for this. It's really just a value that's implicitly passed along with each RPC. Maybe, RpcRequestHeader? But really I'd defer to GWT team for advice on naming here... http://gwt-code-reviews.appspot.com/1107801/diff/13001/14006 File user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java (right): http://gwt-code-reviews.appspot.com/1107801/diff/13001/14006#newcode82 user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java:82: if (((flags | VALID_FLAGS_MASK) ^ VALID_FLAGS_MASK) == 0) { Why not just return (that expression)? http://gwt-code-reviews.appspot.com/1107801/diff/13001/14008 File user/src/com/google/gwt/user/client/rpc/impl/RemoteServiceProxy.java (right): http://gwt-code-reviews.appspot.com/1107801/diff/13001/14008#newcode276 user/src/com/google/gwt/user/client/rpc/impl/RemoteServiceProxy.java:276: protected void checkRpcTokenType(RpcToken token) { Perhaps javadoc? http://gwt-code-reviews.appspot.com/1107801/diff/13001/14010 File user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java (right): http://gwt-code-reviews.appspot.com/1107801/diff/13001/14010#newcode192 user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:192: stob.addRootType(logger, icseType); Spurious whitespace at line-end. http://gwt-code-reviews.appspot.com/1107801/diff/13001/14020 File user/test/com/google/gwt/user/client/rpc/RpcTokenTest.java (right): http://gwt-code-reviews.appspot.com/1107801/diff/13001/14020#newcode81 user/test/com/google/gwt/user/client/rpc/RpcTokenTest.java:81: token.tokenValue = Drink kumys!; Yikes! http://gwt-code-reviews.appspot.com/1107801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Document limitations of current SafeHtmlTemplates code generator. (issue1006801)
Reviewers: rice, pdr, Description: Document limitations of current SafeHtmlTemplates code generator. Please review this at http://gwt-code-reviews.appspot.com/1006801/show Affected files: M user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java Index: user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java === --- user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java (revision 9045) +++ user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java (working copy) @@ -24,7 +24,7 @@ /** * A tag interface that facilitates compile-time binding of HTML templates to * generate SafeHtml strings. - * + * * pExample usage: * pre * public interface MyTemplate extends SafeHtmlTemplates { @@ -45,13 +45,24 @@ * TEMPLATE.messageWithLink(message, url, linkText, style); * } * /pre - * - * Instantiating a SafeHtmlTemplates interface with {...@code GWT.create()} returns - * an instance of an implementation that is generated at compile time. The code - * generator parses the value of each template method's {...@code @Template} - * annotation as a (X)HTML template, with template variables denoted by - * curly-brace placeholders that refer by index to the corresponding template - * method parameter. + * + * p + * Instantiating a {...@code SafeHtmlTemplates} interface with {...@code GWT.create()} + * returns an instance of an implementation that is generated at compile time. + * The code generator parses the value of each template method's + * {...@code @Template} annotation as a (X)HTML template, with template variables + * denoted by curly-brace placeholders that refer by index to the corresponding + * template method parameter. + * + * p + * bNote:/b The current implementation of the code generator cannot + * guarantee the {...@code SafeHtml} contract for templates with template variables + * in a CSS or JavaScript context (that is, within a {...@code style} attribute or + * tag; or within {...@code lt;scriptgt;} tags or {...@code onClick}, {...@code + * onError}, etc, attributes). Developers are advised to avoid such templates, + * or very carefully review the uses of corresponding template methods to ensure + * that values passed into the CSS or JavaScript context cannot result in + * unintended script execution. */ public interface SafeHtmlTemplates { -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Document limitations of current SafeHtmlTemplates code generator. (issue1006801)
http://gwt-code-reviews.appspot.com/1006801/diff/1/2 File user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java (right): http://gwt-code-reviews.appspot.com/1006801/diff/1/2#newcode53 user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java:53: * {...@code @Template} annotation as a (X)HTML template, with template variables On 2010/10/14 19:58:25, rice wrote: I think this needs to use #064; instead of a literal @ Thanks, done. http://gwt-code-reviews.appspot.com/1006801/diff/1/2#newcode62 user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java:62: * onError}, etc, attributes). Developers are advised to avoid such templates, On 2010/10/14 19:58:25, rice wrote: etc. Done. http://gwt-code-reviews.appspot.com/1006801/diff/1/2#newcode63 user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java:63: * or very carefully review the uses of corresponding template methods to ensure On 2010/10/14 19:58:25, rice wrote: or very carefully review the uses of corresponding template methods - or to review the uses of corresponding template methods very carefully Done. http://gwt-code-reviews.appspot.com/1006801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Document limitations of current SafeHtmlTemplates code generator. (issue1006801)
http://gwt-code-reviews.appspot.com/1006801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add streaming HTML parser library to tools/lib, as well as the guava library it depends on. (issue850801)
http://gwt-code-reviews.appspot.com/850801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeHtml support to UI widgets (2) (issue847801)
LGTM! http://gwt-code-reviews.appspot.com/847801/diff/8002/13005 File user/src/com/google/gwt/user/client/ui/Anchor.java (right): http://gwt-code-reviews.appspot.com/847801/diff/8002/13005#newcode117 user/src/com/google/gwt/user/client/ui/Anchor.java:117: this(html.asString(), true); This is dropping the href? this(html.asString(), true, href); http://gwt-code-reviews.appspot.com/847801/diff/8002/13026 File user/src/com/google/gwt/user/client/ui/StackLayoutPanel.java (right): http://gwt-code-reviews.appspot.com/847801/diff/8002/13026#newcode436 user/src/com/google/gwt/user/client/ui/StackLayoutPanel.java:436: public void setHeaderHTML(int index, SafeHtml html) { Since the setHeaderHtml(int, String) version casts the headerwidget to HasHtml an calls setHTML(String) on it, I wonder if it would make sense to mirror that here and cast to HasSafeHtml? Otoh, this would diverge from the usual pattern of delegating setHTML(SafeHtml) to setHTML(String). Not sure :) http://gwt-code-reviews.appspot.com/847801/diff/8002/13030 File user/src/com/google/gwt/user/client/ui/TabBar.java (right): http://gwt-code-reviews.appspot.com/847801/diff/8002/13030#newcode525 user/src/com/google/gwt/user/client/ui/TabBar.java:525: * {...@link #setTabText(int, String)} whenever possible. or {...@link #setTabText(int, SafeHtml)} http://gwt-code-reviews.appspot.com/847801/diff/8002/13034 File user/test/com/google/gwt/user/client/ui/AnchorTest.java (right): http://gwt-code-reviews.appspot.com/847801/diff/8002/13034#newcode48 user/test/com/google/gwt/user/client/ui/AnchorTest.java:48: static final String html = bhello/biworld/i; should this be private, and HTML? (here and elsewhere?) http://gwt-code-reviews.appspot.com/847801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add streaming HTML parser library to tools/lib, as well as the guava library it depends on. (issue850801)
http://gwt-code-reviews.appspot.com/850801/diff/1/11 File user/test/com/google/gwt/safehtml/SafeHtmlGwtSuite.java (right): http://gwt-code-reviews.appspot.com/850801/diff/1/11#newcode22 user/test/com/google/gwt/safehtml/SafeHtmlGwtSuite.java:22: import com.google.gwt.safehtml.shared.GwtSafeHtmlHostedModeUtilsTest; On 2010/09/08 13:17:31, pdr wrote: These might need to be alphabetical. Done. http://gwt-code-reviews.appspot.com/850801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeHtml support to ui widgets. (issue829801)
LGTM http://gwt-code-reviews.appspot.com/829801/diff/8020/24004 File user/src/com/google/gwt/user/client/ui/CheckBox.java (right): http://gwt-code-reviews.appspot.com/829801/diff/8020/24004#newcode265 user/src/com/google/gwt/user/client/ui/CheckBox.java:265: public void setSafeHtml(SafeHtml html) { Don't we already inherit this from ButtonBase? http://gwt-code-reviews.appspot.com/829801/diff/8020/24013 File user/test/com/google/gwt/user/client/ui/CheckBoxTest.java (right): http://gwt-code-reviews.appspot.com/829801/diff/8020/24013#newcode226 user/test/com/google/gwt/user/client/ui/CheckBoxTest.java:226: CheckBox box = new CheckBox(html); You could shorten this to new CheckBox(SafeHtmlUtils.fromSafeConstant(b... (here and elsewhere) http://gwt-code-reviews.appspot.com/829801/diff/8020/24015 File user/test/com/google/gwt/user/client/ui/HTMLTest.java (right): http://gwt-code-reviews.appspot.com/829801/diff/8020/24015#newcode43 user/test/com/google/gwt/user/client/ui/HTMLTest.java:43: sb.appendHtmlConstant(html); It's a bit of a discouraged pattern to pass the value of local variables to appendHtmlConstant (since in a more complicated method you have to squint at it to be sure it's really a constant). Can you make this static final constants? (here and elsewhere) http://gwt-code-reviews.appspot.com/829801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Cosmetic changes in safehtml package. (issue821801)
Reviewers: pdr, rice, Description: Cosmetic changes in safehtml package. Run SafeHtmlBuilderTest both as a GWT and a JRE test. Please review this at http://gwt-code-reviews.appspot.com/821801/show Affected files: M user/src/com/google/gwt/safehtml/shared/SafeHtmlBuilder.java M user/test/com/google/gwt/safehtml/SafeHtmlGwtSuite.java M user/test/com/google/gwt/safehtml/SafeHtmlJreSuite.java A user/test/com/google/gwt/safehtml/shared/GwtSafeHtmlBuilderTest.java M user/test/com/google/gwt/safehtml/shared/GwtSafeHtmlUtilsTest.java M user/test/com/google/gwt/safehtml/shared/SafeHtmlBuilderTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Cosmetic changes in safehtml package. (issue821801)
http://gwt-code-reviews.appspot.com/821801/diff/1/2 File user/src/com/google/gwt/safehtml/shared/SafeHtmlBuilder.java (right): http://gwt-code-reviews.appspot.com/821801/diff/1/2#newcode81 user/src/com/google/gwt/safehtml/shared/SafeHtmlBuilder.java:81: * @param c the number whose string representation to append On 2010/08/31 20:27:28, rice wrote: number-character Done. http://gwt-code-reviews.appspot.com/821801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Cosmetic changes in safehtml package. (issue821801)
http://gwt-code-reviews.appspot.com/821801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Update i18n Messages to include: (issue796802)
http://gwt-code-reviews.appspot.com/796802/diff/1/10 File user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java (right): http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode425 user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:425: private boolean returnsSafeHtml; final? http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode431 user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:431: * @param returnsSafeHtml if true, an expression of type {...@link SafeHtml} is typo: is be generated http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode506 user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:506: * Perhaps append a comment that this assumes the the caller is assumed to provide expressions that are String-valued? http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode753 user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:753: Only one list parameter supported in Would it be better to just completely fail here instead of continuing? http://gwt-code-reviews.appspot.com/796802/diff/1/10#newcode1001 user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:1001: gen.appendExpression(val1, false, false, false); appendStringValuedExpression? Do we actually know for sure that the val1 expression is String-valued? (I have to admit I'm not completely following what's going on here...) http://gwt-code-reviews.appspot.com/796802/diff/1/23 File user/test/com/google/gwt/i18n/client/TestAnnotatedMessages.java (right): http://gwt-code-reviews.appspot.com/796802/diff/1/23#newcode67 user/test/com/google/gwt/i18n/client/TestAnnotatedMessages.java:67: String australianDollars(double amount); Should these have SafeHtml equivalent tests too? http://gwt-code-reviews.appspot.com/796802/diff/1/23#newcode199 user/test/com/google/gwt/i18n/client/TestAnnotatedMessages.java:199: String reviewers(@PluralCount @Offset(2) int size, Might be useful to have tests of SafeHtml versions of these ones too? http://gwt-code-reviews.appspot.com/796802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtml Templates (issue793801)
LGTM http://gwt-code-reviews.appspot.com/793801/diff/1/2 File user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java (right): http://gwt-code-reviews.appspot.com/793801/diff/1/2#newcode42 user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java:42: * SafeHtmlLabel messageWithLinkLabel = new SafeHtmlLabel( Since there isn't going to be a SafeHtmlLabel widget (but rather GWT core widgets will implement HasSafeHtml), we should change this example. Probably just SafeHtml messageWithLink = TEMPLATE.messageWithLink(message, url, linkText, style); is sufficient. http://gwt-code-reviews.appspot.com/793801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Small SafeHtml method name refactoring for name consistency. (issue799801)
LGTM http://gwt-code-reviews.appspot.com/799801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: SafeHtml Templates (issue793801)
http://gwt-code-reviews.appspot.com/793801/diff/1/2 File user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java (right): http://gwt-code-reviews.appspot.com/793801/diff/1/2#newcode55 user/src/com/google/gwt/safehtml/client/SafeHtmlTemplates.java:55: public interface SafeHtmlTemplates { On 2010/08/24 18:58:38, rice wrote: Why the plural form? It sounds odd in an interface name. IIRC I chose to use the plural form because I modeled this after c.g.g.i18n.client.Messages, which is also plural. I suppose this makes sense because each interface that extends SHTemplates can in fact contain multiple template methods. http://gwt-code-reviews.appspot.com/793801/diff/1/3 File user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java (right): http://gwt-code-reviews.appspot.com/793801/diff/1/3#newcode45 user/src/com/google/gwt/safehtml/rebind/HtmlTemplateParser.java:45: * pre {...@code On 2010/08/24 18:58:38, rice wrote: Is the use of both pre and {...@code} intentional? I can't quite remember, but IIRC this was the only way I could get this to format as a block (as opposed to inline). No strong feelings about it. http://gwt-code-reviews.appspot.com/793801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added all safehtml packages. (issue771801)
http://gwt-code-reviews.appspot.com/771801/diff/1/10 File user/src/com/google/gwt/safehtml/shared/EscapeUtils.java (right): http://gwt-code-reviews.appspot.com/771801/diff/1/10#newcode29 user/src/com/google/gwt/safehtml/shared/EscapeUtils.java:29: * Returns a SafeHtml constructed from a safe string, i.e. without escaping This method's doc should stipulate the same constraints on its argument as does SafeHtmlBuilder#appendHtmlConstant(String). http://gwt-code-reviews.appspot.com/771801/diff/1/14 File user/src/com/google/gwt/safehtml/shared/SafeHtmlBuilder.java (right): http://gwt-code-reviews.appspot.com/771801/diff/1/14#newcode116 user/src/com/google/gwt/safehtml/shared/SafeHtmlBuilder.java:116: * Boolean and numeric types converted to String are always HTML safe -- no This comment should go above all the append(boolean/numeric type) methods (I think methods got reordered)? http://gwt-code-reviews.appspot.com/771801/diff/1/28 File user/test/com/google/gwt/safehtml/shared/SafeHtmlBuilderTest.java (right): http://gwt-code-reviews.appspot.com/771801/diff/1/28#newcode41 user/test/com/google/gwt/safehtml/shared/SafeHtmlBuilderTest.java:41: new SafeHtmlBuilder().appendHtmlConstant(Yabba dabba amp; doo\n).appendEscaped(What's up soso\n).append(html); Line length? http://gwt-code-reviews.appspot.com/771801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added all safehtml packages. (issue771801)
On 2010/08/17 23:05:06, tbroyer wrote: On 2010/08/17 20:19:04, xtof wrote: On 2010/08/17 18:39:28, jat wrote: On 2010/08/16 23:39:47, tbroyer wrote: The HtmlSanitizer is a good idea, but the implementation is very weak [2]. Note that the API is what is important, and SimpleHtmlSanitizer is just that, a simple implementation. A more involved implementation can be added later. Also, we aren't trying to parse HTML with a regex here, it simply looks for opening tags and allows unescaped on a small set of whitelisted tags -- everything else gets escaped. If you think it fails to do its job, can you supply a string which would not be propertly sanitized? Looking at the code more closely it would merely fail by overly rejecting tags that are whitelisted: i.e. b foo=ishould be bold would be sanitized to lt;b foo=ishould 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). But there's more: In other words, does SimpleHtmlSanitizer adhere to the SafeHtml type contract? I believe it does, but of course would like arguments/hunches towards the contrary. SafeHtml calls for XSS mitigation, and SimpleHtmlSanitizer doesn't sanitize most XSS attacks, that use attributes: b style='position:absolute;z-index:2147483646;left:0px;top:0px;right:0px;bottom:0px' onmousemove=\alert(quot;you've been highjackedquot;);\ That's not true, this would be turned into lt;b style, which is safe (though ugly, but again see the above). Having said that, I kind of agree that SimpleHtmlSanitizer is of pretty limited use; one of the few scenarios where I can see it used is for rendering messages/snippets that are formatted with limited HTML markup, and are obtained from a complex backend that the developer of the GWT client doesn't quite want to trust to uphold the SafeHtml type contract. I wouldn't be terribly opposed to droppping it. I didn't want to implement a full parser, because parsing full HTML (say in an email app) inside the browser generally seems like a bad approach. I agree, and I'd be OK with removing HtmlSanitizer from this patch to better reintroduce it later. http://gwt-code-reviews.appspot.com/771801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors