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
