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
