I probably won't have time to update the patch before (at least) the
second half of the week, so here's my first reaction to your comments
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 16:22:32, xtof wrote:
Hmm, I just realized there's another case: When a parameter occurs in
a URI
attribute but not at the beginning (e.g., <img
src='{http://foo.com/{0}'>). In
that case we get contextType==ATTRIBUTE_VALUE, and would show the
warning from
the else branch, which wouldn't be exactly accurate.
Actually, the message in the other case is not accurate either for
non-attribute contexts.
Fixing this unfortunately isn't trivial, because that case (in a URI
attribute
but not at the beginning) isn't exposed in HtmlContext.
Perhaps take a TODO? I'm starting to think that this code needs some
refactoring...
How about having isAttrStart() and isAttrEnd() in addition to simpler
states (URL_ATTRIBUTE, CSS_ATTRIBUTE, ATTRIBUTE, etc.)? That way,
checking for "URL_ATTRIBUTE_ENTIRE" would be "type==URL_ATTRIBUTE &&
isAttrStart() && isAttrEnd()".
I think John already proposed something similar in the discussion around
the SafeStyles CL.
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.
I was thinking about maybe put it in SafeUriHostedModeUtils (with some
renaming of course!) as it already handles the prodmode vs.
devmode/plain-JVM with super-source (i.e. even easier to read, but the
super-source would no longer be a no-op).
What do you think?
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 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>
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/UriUtils.java#newcode236
user/src/com/google/gwt/safehtml/shared/UriUtils.java:236: if
(isSafeUri(uri)) {
On 2011/04/18 16:22:32, xtof wrote:
Since you've created SafeUriHostedModeUtils.maybeCheckValidUri(s), we
perhaps
should call this here too (since it's a stricter check of URI well
formedness),
i.e.
if (SafeUriHostedModeUtils.maybeCheckValidUri(uri) && isSafeUri(uri))
?
Well, except that maybeCheckValidUri is a precondition/assert-style
method. I can't think of any case where a value passing the isSafeUri
check would be harmful, even less after the encodeAllowEscapes
transformation.
http://gwt-code-reviews.appspot.com/1380806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
