I didn't address Brian's requests because I honestly didn't know what to put (javascript: URIs? an example of data: URI with HTML or SVG containing a script?)
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:18:11, xtof wrote:
I think I agree, it's probably time for something like that.
Definitely in a
separate change list though :)
Added a TODO(xtof) calling for a 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(); 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. Done. 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:18:11, xtof wrote:
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: > <strong>Despite this method creating a SafeUri instance, no checks
are
> performed, so the returned SafeUri is absolutely NOT guaranteed to
be
> safe!</strong> SGTM.
Done. 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/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. 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() { On 2011/04/18 16:22:32, xtof wrote:
Maybe add a test for a URL containing utf-8 characters?
Done. http://gwt-code-reviews.appspot.com/1380806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
