Sure, saying that javascript: and data: URL's are potentially unsafe would be a good start. Someone who knows more should give some suggestions.
On Sat, Apr 23, 2011 at 7:36 AM, <[email protected]> wrote: > 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
