On Tue, Mar 29, 2011 at 13:30, <t.bro...@gmail.com> wrote: > I totally agree with your analysis and ENTIRE_URL_ATTRIBUTE state (I had > similar concerns, but couldn't find a use case where it would be > problematic: I didn't think about the javascript: URLs; and there > actually are similar issues with data: URLs). > Yes, indeed.
> > Should I wait for the ENTIRE_URL_ATTRIBUTE state (I'll probably only > have time this week-end, so maybe you'll have it before I come back to > this code, but just in case), or go with the other changes and let you > do that last change (should only be a matter of adding a warning/error > in SafeHtmlTemplatesImplMethodCreator#emitParameterExpression and remove > the check and sanitization in #emitAttributeContextParameterExpression) > > I'll try to send out this change today. If possible, it would be preferable to constrain SafeUri template parameters from the beginning, otherwise it'll introduce a breaking change later on. > > > > 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)); > On 2011/03/29 20:11:11, xtof wrote: > >> I'm a bit concerned about the use of UriUtils.fromTrustedString here >> > > To tell the truth, me too! > > -- this code can't really guarantee that url is trusted, >> >> it may come from the Image(String url, ...) c'tor. >> > > Yes, it's mostly "required for backwards compatibility with the 'unsafe' > API". > > > Perhaps this could be made a bit cleaner as follows: >> > > - use SafeUri internally in ClippedState >> > > I had an iteration in my local repo with such a change, then thought > that the fromTrustedString could maybe be inlined by the compiler, while > I suppose it wouldn't be possible with the SafeUri. > > True. I'm not sure how much of a concern this is, I'd let rjrjr or jlabanca comment on that. > > - 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. >> > > Nice idea! > > > - Implement the public Image(String url,...) constructors in terms of >> > the > >> Image(SafeUri url, ...) ones, using fromUntrustedString. >> > > I'm really not sure about that last one, as the getUrl would still have > to be a String for backwards compat', and the DOM-level accesses are all > String-based too. > > I see. Since ClippedState is strictly internal, it probably doesn't make a big difference. I think for the time being it would also be fine to leave as it is currently, but replace fromTrustedString with fromUntrustedString and add some comments about what's going on. Just to avoid the impression that the code guarantees something that it really can't guarantee. > (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 :) >> > > I'll give it a try ;-) > > > http://gwt-code-reviews.appspot.com/1380806/ > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
