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:03:27, tbroyer wrote:
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.
I think I agree, it's probably time for something like that. Definitely
in a separate change list though :)
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 17:03:27, tbroyer wrote:
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?
That would work too; I don't think I have strong opinions one way or
another though...
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: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.
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 17:03:27, tbroyer wrote:
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.
Fair point. And it could be a breaking change (along the same lines as
you noted above).
http://gwt-code-reviews.appspot.com/1380806/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
