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).
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) 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.
- 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.
(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
