[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
