Issue 1380806: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context.

tbroyer

I'm having a hard time integrating it with UiBinder because of the way attributes are ...

2 years, 2 months ago #3

rjrjr

Christoph, can you take this review? On Sat, Mar 19, 2011 at 10:04 AM, <t.broyer@gmail.com> ...

2 years, 1 month ago #4

xtof

On Wed, Mar 23, 2011 at 09:54, Ray Ryan <rjrjr@google.com> wrote: > Christoph, can you ...

2 years, 1 month ago #5

xtof

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/cell/client/ImageCell.java File user/src/com/google/gwt/cell/client/ImageCell.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/cell/client/ImageCell.java#newcode58 user/src/com/google/gwt/cell/client/ImageCell.java:58: sb.append(template.img(UriUtils.fromString(value))); It seems that in this case (and similar ...

2 years, 1 month ago #6

tbroyer

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/cell/client/ImageCell.java File user/src/com/google/gwt/cell/client/ImageCell.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/cell/client/ImageCell.java#newcode58 user/src/com/google/gwt/cell/client/ImageCell.java:58: sb.append(template.img(UriUtils.fromString(value))); On 2011/03/23 20:42:52, xtof wrote: > It seems ...

2 years, 1 month ago #8

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/ce...
File user/src/com/google/gwt/cell/client/ImageCell.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/ce...
user/src/com/google/gwt/cell/client/ImageCell.java:58:
sb.append(template.img(UriUtils.fromString(value)));
On 2011/03/23 20:42:52, xtof wrote:
> It seems that in this case (and similar ones down the line), changing the
> template signature to img(SafeUri) results in unnecessary verbosity -- when
the
> template took a string, the template generator inserts the sanitizeUri call
> automatically, to the same effect.
> 
> Would it make sense to take a SafeUri and expose that in the render signature,
> i.e make this an AbstractCell<SafeUri>? 

That sure would be far better!

Note however that with the removal of the warning for using String parameter in
URL context, we can just revert that change.
(Done.)

> Would presumably be a breaking API change though.

Maybe add a new Cell then?

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/ce...
File user/src/com/google/gwt/cell/client/ImageResourceCell.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/ce...
user/src/com/google/gwt/cell/client/ImageResourceCell.java:39:
value).getHTML());
On 2011/03/23 20:42:52, xtof wrote:
> It would be nice if there was a getSafeHtml, to avoid the need for a
> fromTrustedString here (see also comment in ClippedImagePrototype).

Done.

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/sa...
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/sa...
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:117:
*   <li>If the template parameter occurs at the start of a URI-valued
On 2011/03/23 20:42:52, xtof wrote:
> The comment isn't quite correct as written, because as is it implies that
> processing for start-of-URI happens even if it was a SafeUri (but it is
actually
> omitted).

Done.

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/sa...
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:268:
if (!SAFE_URI_FQCN.equals(parameterType.getQualifiedSourceName())) {
On 2011/03/23 20:42:52, xtof wrote:
> I'm not sure we want to print a warning here.  I think it's quite legitimate
for
> a template to have a String-valued variable in URL_START context, and just let
> the template take care of sanitization (see also comment in ImageCell.java).
> 
> This is analogous to using String vs SafeHtml in "inner text" context.  In a
lot
> of cases it makes sense for the argument be String valued and let the template
> take care of escaping; only if I don't want the template to touch it I need to
> supply a SafeHtml.

Done.

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/sa...
File user/src/com/google/gwt/safehtml/shared/SafeUri.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/sa...
user/src/com/google/gwt/safehtml/shared/SafeUri.java:21: * vulnerabilities) in
an HTML context.
On 2011/03/23 20:42:52, xtof wrote:
> This should be "[...] in a URI context within an HTML document, for example in
a
> URI-valued attribute", or some such...

Done.

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/sa...
File user/src/com/google/gwt/safehtml/shared/SafeUriString.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/sa...
user/src/com/google/gwt/safehtml/shared/SafeUriString.java:65: if (!(obj
instanceof SafeHtml)) {
On 2011/03/23 20:42:52, xtof wrote:
> instanceof SafeUri

Done.

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/sa...
user/src/com/google/gwt/safehtml/shared/SafeUriString.java:68: return
uri.equals(((SafeHtml) obj).asString());
On 2011/03/23 20:42:52, xtof wrote:
> SafeUri

Done.

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/us...
File user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java
(right):

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/src/com/google/gwt/us...
user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java:68:
return impl.getHTML(url.asString(), left, top, width, height);
On 2011/03/23 20:42:52, xtof wrote:
> Would it make sense to provide a getSafeHtml() method here that returns
SafeHtml
> and wraps getHTML() with SafeHtmlUtils.fromTrustedString?
> 
> This would push the fromTrusted closer to where the HTML is assembled in a
> supposedly trusted manner, making it more easily verified as correct.
> 
> Perhaps SafeHtml could even be pushed into the impls, and use
SafeHtmlTemplates
> within them?

Done.

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/test/com/google/gwt/s...
File user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
(right):

http://gwt-code-reviews.appspot.com/1380806/diff/1/user/test/com/google/gwt/s...
user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java:110:
UriUtils.fromTrustedString(GOOD_URL),
SafeHtmlUtils.fromSafeConstant(HTML_MARKUP)).asString());
On 2011/03/23 20:42:52, xtof wrote:
> 100 chars, here and line 114

The files need to be reformatted using the new formatting rules anyway, so I
didn't even try to fit in the 100-chars line length (to not mix formatting
changes with other "code changes" and make it easier to review)

Sign in to reply to this message.

Thanks for your changes!

There's one more thing:  In the past couple of days we've had some discussions
around the "in URL valued attribute" context.

The main issue discussed is that unlike SafeHtml, SafeUri is not generally
composable:  Two SafeHtmls concatenated together make another SafeHtml, but for
SafeUri this doesn't really work.
For example, 
String attackerControlled = "evil()";
SafeUri uri0 = UriUtils.fromTrustedString("javascript:void(0);");
SafeUri uri1 = UriUtils.fromString(attackerControlled);

sanitizeUri will leave attackerControlled alone (it looks like a relative URL),
and both uri0 and uri1 are individually harmless.  However, concatenated
together, the resulting URI will execute the attacker controlled script when
dereferenced.

With that in mind, it seems safest to only allow SafeUri to be interpolated in a
uri-valued attribute if the template variable makes up the whole attribute. 
I.e., 

@Template("<img src='{0}'/>")
SafeHtml image(SafeUri uri);

would be allowed, but
@Template("<img src='{0}/{1}'/>")
SafeHtml image(SafeUri baseUri, String path);
would not.

If that sounds reasonable to you too, I'll make a change to expose a
corresponding parse state in the parser (ENTIRE_URL_ATTRIBUTE).

Thanks!
--xtof

http://gwt-code-reviews.appspot.com/1380806/diff/5003/user/src/com/google/gwt...
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/src/com/google/gwt/user/client/ui/Image.java:145:
impl.createStructure(UriUtils.fromTrustedString(url), left, top, width,
height));
I'm a bit concerned about the use of UriUtils.fromTrustedString here -- this
code can't really guarantee that url is trusted, it may come from the
Image(String url, ...) c'tor.

Perhaps this could be made a bit cleaner as follows:

- use SafeUri internally in ClippedState
- 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.
- Implement the  public Image(String url,...) constructors in terms of the
Image(SafeUri url, ...) ones, using fromUntrustedString.

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

Sign in to reply to this message.

tbroyer

I totally agree with your analysis and ENTIRE_URL_ATTRIBUTE state (I had similar concerns, but couldn't ...

2 years, 1 month ago #10

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...
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/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 ;-)

Sign in to reply to this message.

xtof

On Tue, Mar 29, 2011 at 13:30, <t.broyer@gmail.com> wrote: > I totally agree with your ...

2 years, 1 month ago #11

On Tue, Mar 29, 2011 at 13:30, <t.broyer@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...
> 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/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/
>

Sign in to reply to this message.

tbroyer

On 2011/03/29 20:38:29, xtof wrote: > > > > - Implement the public Image(String url,...) ...

2 years, 1 month ago #12

xtof

On Wed, Mar 30, 2011 at 01:47, <t.broyer@gmail.com> wrote: > On 2011/03/29 20:38:29, xtof wrote: ...

2 years, 1 month ago #13

On Wed, Mar 30, 2011 at 01:47, <t.broyer@gmail.com> wrote:

> On 2011/03/29 20:38:29, xtof wrote:
>
>> >
>> >  - 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.
>>
>
> Thinking a bit more about it, how about keeping the use of
> fromTrustedString, not introducing fromUntrustedString, and instead just
> documenting that we're using fromTrustedString here for backwards
> compatibility only, and cannot guarantee the URL safety?
>
The advantage of using a separate method is that it helps code reviews:
 When I do a security code review, I need to find all the instances of what
are effectively unsafe casts to SafeUri where the programmer intended to
make the promise that the value is safe; I need to review the code to check
that assumption.  In the "legacy" unsafe cast, there really isn't anything
to review -- it just means there are code paths that are going to be safe
until the API is fully refactored.  I guess I'd want to review those too,
but with a different angle.  Using two different methods helps the review,
since I can just use the IDE's show call hierarchy function to find them (or
grep).

...and adding docs to all the String-based (public) methods about the
> caller having to care about security and either sanitize the URI or use
> the SafeUri-based overload?
> (note however that no such doc was added to setHTML when setSafeHtml was
> introduced)

I think the difference to SafeHtml is that here, we're actually changing the
internal implementation to use SafeUri, but we still have an external method
that takes a plain String.  For widgets that support setHTML(SafeHtml), and
setHTML(String), the situation is a bit simpler since each typically either
passes the value down to an inner widget that takes SafeHtml/String,
respectively, or ends up calling setInnerHTML, which just takes String.

If we for example had a complicated widget that is refactored to use
SafeHtmlBuilder intenally, but still exposes a setHTML(String) method, we'd
probably have to similarly introduce a such an "unsafe cast" method.

BTW, I'm still working on the ENTIRE_URL_ATTRIBUTE cl. Hoping to get that
out this morning.

cheers,

--xtof


>
> http://gwt-code-reviews.appspot.com/1380806/
>

Sign in to reply to this message.

tbroyer

I followed the approach from the SafeStyles for the checks in the generator, with the ...

2 years, 1 month ago #14

I think this is pretty much ready, except for one thing I just thought of.
Sorry, I should have thought of that earlier :/

In ClippedImageImpl, we're using a SafeUri in the context of a url() style
expression ("background: url({3})"). However, the contract of SafeUri is
currently not tight enough for this to be safe I think:  SafeUri would allow
e.g., parentheses, colons and dashes in un-escaped form in the URL.  In a
template where the URL appears in a url() css expression as above, a URL like,
http://harmless.com/foo);background:url(javascript:evil());
would pass UriUtils#sanitizeUri (without getting URI-escaped or anything), and
would result in script execution via CSS injection.

So I think we need to,
- tighten the SafeUri contract so it explicitly specifies which character set
can occur in a SafeUri
- add a sanitizeAndEscapeUri method to UriUtils that both checks for an allowed
scheme and URI/%-escapes characters not in the allowed charset (or maybe rejects
URLs with certain funny characters in them).

I'll have to do some reading up on what characters are OK in the URI in a CSS
context, will get back to you on that.


Thanks again for all your work on this!
--xtof

http://gwt-code-reviews.appspot.com/1380806/diff/16001/user/src/com/google/gw...
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):

http://gwt-code-reviews.appspot.com/1380806/diff/16001/user/src/com/google/gw...
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:127:
*   <li>If the template parameter occurs at the start of a URI-valued
There probably should be an explanation about the distinction between "start of
URL-valued attribute" and "entire URL-valued attribute"?

http://gwt-code-reviews.appspot.com/1380806/diff/16001/user/src/com/google/gw...
File user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/16001/user/src/com/google/gw...
user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java:46:
UriUtils.fromUntrustedString(GWT.getModuleBaseURL() + "clear.cache.gif");
I think here it would actually be appropriate to use fromTrustedString -- the
value we're passing through here is fully under the control of the program (at
least I think that GWT.getModuleBaseURL cannot be modified from the outside).

Actually, if GWT.getModuleBaseURL() + someFile is a common idiom, it might be
worth introducing a utility method in UriUtils that returns an absolute URL
based at moduleBaseURL?  If it's not common, it won't be worth the clutter
though.

Sign in to reply to this message.

tbroyer

On 2011/04/10 18:32:38, xtof wrote: > I think this is pretty much ready, except for ...

2 years, 1 month ago #16

On 2011/04/10 18:32:38, xtof wrote:
> I think this is pretty much ready, except for one thing I just thought of.
> Sorry, I should have thought of that earlier :/
> 
> In ClippedImageImpl, we're using a SafeUri in the context of a url() style
> expression ("background: url({3})"). However, the contract of SafeUri is
> currently not tight enough for this to be safe I think:  SafeUri would allow
> e.g., parentheses, colons and dashes in un-escaped form in the URL.  In a
> template where the URL appears in a url() css expression as above, a URL like,
> http://harmless.com/foo);background:url(javascript:evil());
> would pass UriUtils#sanitizeUri (without getting URI-escaped or anything), and
> would result in script execution via CSS injection.

I hadn't looked closely at the SafeStyles API and thought it would be covered
there (and ClippedImageImpl would of course be updated to use SafeStyles).
But note that, similarly, http://harmless.com"onclick="evil()" would pass
sanitizeUri and could result in script execution via HTML injection (not through
SafeHtml/SafeHtmlTemplates though, as the quotes would be escaped).

The CSS spec says whitespace, quotes (single and double) and parentheses can be
escaped using a backslash: http://www.w3.org/TR/CSS2/syndata.html#uri
So maybe we could special-case SafeUri in CSS contexts in the generator? (we
already special-case it for it's .asString(), and special-case URL contexts for
non-SafeUri values to sanitize them, so...)

> So I think we need to,
> - tighten the SafeUri contract so it explicitly specifies which character set
> can occur in a SafeUri
> - add a sanitizeAndEscapeUri method to UriUtils that both checks for an
allowed
> scheme and URI/%-escapes characters not in the allowed charset (or maybe
rejects
> URLs with certain funny characters in them).

We can quite simply %-escape those chars that are special to CSS (see above), or
use the more radical approach of re-%-encode the whole URI: i.e. %-encode any
"funny char" (except % chars that are not followed by a pair of hex digits;
similar to SafeHtmlUtils#htmlEscapeAllowEntities, but for %-escaping).
Or we can reverse-engineer the (proposed) Web Address algorithm to only %-encode
those chars that would cause issues: http://www.w3.org/html/wg/href/draft
continued at http://tools.ietf.org/html/draft-ietf-iri-3987bis-05#section-7.2

But I wonder if this shouldn't be the rule for sanitizeUri (instead of
introducing a new method and leaving a "hole" in sanitizeUri under certain
circumstances).
There should probably be an encodeAllowEscapes too, similar to
htmlEscapeAllowEntities, when e.g. you know your URL is safe, but you're unsure
if it's "encoded enough" for every context (including CSS), i.e. the URL could
contain unescaped parentheses or single quotes.

Should we defer this patch? or tackle this in a following one? (maybe adding a
TODO in fromTrustedString/fromString/fromUntrustedString/sanitizeUri?)

Sign in to reply to this message.

xtof

On Sun, Apr 10, 2011 at 17:02, <t.broyer@gmail.com> wrote: > On 2011/04/10 18:32:38, xtof wrote: ...

2 years, 1 month ago #17

On Sun, Apr 10, 2011 at 17:02, <t.broyer@gmail.com> wrote:

> On 2011/04/10 18:32:38, xtof wrote:
>
>> I think this is pretty much ready, except for one thing I just thought
>>
> of.
>
>> Sorry, I should have thought of that earlier :/
>>
>
>  In ClippedImageImpl, we're using a SafeUri in the context of a url()
>>
> style
>
>> expression ("background: url({3})"). However, the contract of SafeUri
>>
> is
>
>> currently not tight enough for this to be safe I think:  SafeUri would
>>
> allow
>
>> e.g., parentheses, colons and dashes in un-escaped form in the URL.
>>
> In a
>
>> template where the URL appears in a url() css expression as above, a
>>
> URL like,
>
>> http://harmless.com/foo);background:url(javascript:evil());
>> would pass UriUtils#sanitizeUri (without getting URI-escaped or
>>
> anything), and
>
>> would result in script execution via CSS injection.
>>
>
> I hadn't looked closely at the SafeStyles API and thought it would be
> covered there (and ClippedImageImpl would of course be updated to use
> SafeStyles).
> But note that, similarly, http://harmless.com"onclick="evil()" would
> pass sanitizeUri and could result in script execution via HTML injection
> (not through SafeHtml/SafeHtmlTemplates though, as the quotes would be
> escaped).
>

Ok, I think you've convinced me :)

I think what you're saying is essentially this: The SafeUri contract only
promises that a URL will not result in script execution if dereferenced
as-is (which in practice means that its scheme is on a whitelist of ones
that don't cause script exec, and in particular isn't javascript:, etc).
However, when a SafeUri is embedded in a specific context, it *still* needs
to be appropriately escaped.

Which is what happens for uri-valued attribute context in SafeHtmlTemplates,
and I agree with you that we should treat URI-in-CSS context the same way,
and deal with any necessary escaping (or sanitization) there.

I do think that it might be worth to constrain SafeUri to only allow
syntactically valid URIs, or at least only strings consisting of characters
valid in URIs per the RFC.  Unfortunately, GWT doesn't implement
java.net.URI and the former might be hard to do efficiently in browser-side
code; I can't think of any reason the latter isn't sufficient.

If we're both in agreement on that, I think we should change the doc of the
SafeUri contract to be a bit more specific about that.  I.e., instead of

 18 /**
 19  * An object that implements this interface encapsulates a URI that is
 20  * guaranteed to be safe to use (with respect to potential
Cross-Site-Scripting
 21  * vulnerabilities) in a URL context, for example in a URL-typed
attribute in
 22  * an HTML document.

more something along the lines:

/**
An object that implements this interface encapsulates a lexically valid
(with respect to the set of valid characters specified RFC 3986) URI that
when dereferenced, will not result in browser-side execution of script that
is not under program control.

<p>
Note that this type's contract does not constrain the set of characters that
may appear in the URI beyond the requirements of RFC 3986. If the value of a
SafeUri object is used in certain contexts of a HTML document, appropriate
escaping must be applied.

>
> The CSS spec says whitespace, quotes (single and double) and parentheses
> can be escaped using a backslash:
> http://www.w3.org/TR/CSS2/syndata.html#uri

I believe I've read somewhere that this doesn't work in IE, but I can't find
the reference now...

One problem I'd worried about is this:  parentheses are legal in URIs per
the RFC.  At the same time, \xxxxxx escaping them in the CSS seems to not
actually be sufficient; such escaping doesn't prevent the string from being
interpreted as CSS syntax (what were they thinking???), see
http://code.google.com/p/browsersec/wiki/Part1#CSS_character_encoding.
I think we'll just have to %-escape parentheses and single quotes when using
a SafeUri in a CSS url(). I'm not really aware of them meaning anything
special in *http* URIs; even if RFC 3986 says that %-escaping parentheses
won't result in an equivalent URI I don't know of any reason doing so would
result in a different resource being addressed on a web server.

>
> So maybe we could special-case SafeUri in CSS contexts in the generator?
>
I think we should just ban it (or at least warn), since we don't parse the
CSS and can't really tell what context in the CSS the URI is used in.  A
safe style builder API would know, and would escape the URI correctly.

(we already special-case it for it's .asString(), and special-case URL
> contexts for non-SafeUri values to sanitize them, so...)
>
>
>  So I think we need to,
>> - tighten the SafeUri contract so it explicitly specifies which
>>
> character set
>
>> can occur in a SafeUri
>> - add a sanitizeAndEscapeUri method to UriUtils that both checks for
>>
> an allowed
>
>> scheme and URI/%-escapes characters not in the allowed charset (or
>>
> maybe rejects
>
>> URLs with certain funny characters in them).
>>
>
> We can quite simply %-escape those chars that are special to CSS (see
> above), or use the more radical approach of re-%-encode the whole URI:
> i.e. %-encode any "funny char" (except % chars that are not followed by
> a pair of hex digits; similar to SafeHtmlUtils#htmlEscapeAllowEntities,
> but for %-escaping).
> Or we can reverse-engineer the (proposed) Web Address algorithm to only
> %-encode those chars that would cause issues:
> http://www.w3.org/html/wg/href/draft continued at
> http://tools.ietf.org/html/draft-ietf-iri-3987bis-05#section-7.2
>
> But I wonder if this shouldn't be the rule for sanitizeUri (instead of
> introducing a new method and leaving a "hole" in sanitizeUri under
> certain circumstances).
>
I think you're right; sanitizeUri should %-escape characters not allowed by
RFC 3986.  Looks like passing the string through
http://google-web-toolkit.googlecode.com/svn/javadoc/latest/com/google/gwt/ht...
be sufficient?  Anyway, I'll take a TODO item for this.

I'm still wondering if we should properly parse the whole URL and insist
it's syntactically valid; when I wrote sanitizeUri I chose not to because
there's no easily available way to do it in GWT (since java.net.URI isn't
implemented), and it would seem to be fairly costly to do.

There should probably be an encodeAllowEscapes too, similar to
> htmlEscapeAllowEntities, when e.g. you know your URL is safe, but you're
> unsure if it's "encoded enough" for every context (including CSS), i.e.
> the URL could contain unescaped parentheses or single quotes.
>
> Should we defer this patch? or tackle this in a following one? (maybe
> adding a TODO in
> fromTrustedString/fromString/fromUntrustedString/sanitizeUri?)
>
>
>
> http://gwt-code-reviews.appspot.com/1380806/
>

Sign in to reply to this message.

tbroyer

On 2011/04/11 23:18:13, xtof wrote: > If we're both in agreement on that, I think ...

2 years, 1 month ago #18

On 2011/04/11 23:18:13, xtof wrote:
> If we're both in agreement on that, I think we should change the doc of the
> SafeUri contract to be a bit more specific about that.  I.e., instead of
> 
>  18 /**
>  19  * An object that implements this interface encapsulates a URI that is
>  20  * guaranteed to be safe to use (with respect to potential
> Cross-Site-Scripting
>  21  * vulnerabilities) in a URL context, for example in a URL-typed
> attribute in
>  22  * an HTML document.
> 
> more something along the lines:
> 
> /**
> An object that implements this interface encapsulates a lexically valid
> (with respect to the set of valid characters specified RFC 3986) URI that
> when dereferenced, will not result in browser-side execution of script that
> is not under program control.
> 
> <p>
> Note that this type's contract does not constrain the set of characters that
> may appear in the URI beyond the requirements of RFC 3986. If the value of a
> SafeUri object is used in certain contexts of a HTML document, appropriate
> escaping must be applied.

SGTM

> > The CSS spec says whitespace, quotes (single and double) and parentheses
> > can be escaped using a backslash:
> > http://www.w3.org/TR/CSS2/syndata.html#uri
> 
> I believe I've read somewhere that this doesn't work in IE, but I can't find
> the reference now...
> 
> One problem I'd worried about is this:  parentheses are legal in URIs per
> the RFC.  At the same time, \xxxxxx escaping them in the CSS seems to not
> actually be sufficient; such escaping doesn't prevent the string from being
> interpreted as CSS syntax (what were they thinking???), see
> http://code.google.com/p/browsersec/wiki/Part1#CSS_character_encoding.

Note that the CSS spec talks about using '\(', not '\028'. I however have
absolutely no idea how well this is supported by browsers.

> I think we'll just have to %-escape parentheses and single quotes when using
> a SafeUri in a CSS url(). I'm not really aware of them meaning anything
> special in *http* URIs; even if RFC 3986 says that %-escaping parentheses
> won't result in an equivalent URI I don't know of any reason doing so would
> result in a different resource being addressed on a web server.

+1

> > So maybe we could special-case SafeUri in CSS contexts in the generator?
> >
> I think we should just ban it (or at least warn), since we don't parse the
> CSS and can't really tell what context in the CSS the URI is used in.  A
> safe style builder API would know, and would escape the URI correctly.

We already have the warning about "SafeUri outside URL-attribute context", we
sure could specialize the message a bit for the CSS context.


> I think you're right; sanitizeUri should %-escape characters not allowed by
> RFC 3986.  Looks like passing the string through
>
http://google-web-toolkit.googlecode.com/svn/javadoc/latest/com/google/gwt/ht...
> be sufficient? Anyway, I'll take a TODO item for this.

URL.encode wouldn't preserve existing %-escapes and would replace, e.g. %28 with
%2528. The algorithm should probably be similar to htmlEscapeAllowEntities (i.e.
split on %-escapes and encode the rest).
Also, URL.encode doesn't %-escape single quotes and parentheses.

Finally, URL.encode is "client side only", so an equivalent "pure Java"
algorithm should be written for "the JVM" (that would be less efficient in the
browser). Maybe let's start with a single, shared, algorithm andadd the
URL.encode-based one later (and benchmark!)

> I'm still wondering if we should properly parse the whole URL and insist
> it's syntactically valid; when I wrote sanitizeUri I chose not to because
> there's no easily available way to do it in GWT (since java.net.URI isn't
> implemented), and it would seem to be fairly costly to do.

I believe %-escaping à la URL.encode would provide enough safety.

Sign in to reply to this message.

tbroyer

On 2011/04/12 01:14:10, xtof wrote: > On Mon, Apr 11, 2011 at 17:54, <t.broyer@gmail.com> wrote: ...

2 years, 1 month ago #20

xtof

On Thu, Apr 14, 2011 at 01:01, <t.broyer@gmail.com> wrote: > On 2011/04/12 01:14:10, xtof wrote: ...

2 years, 1 month ago #21

On Thu, Apr 14, 2011 at 01:01, <t.broyer@gmail.com> wrote:

> On 2011/04/12 01:14:10, xtof wrote:
>
>> On Mon, Apr 11, 2011 at 17:54, <t.broyer@gmail.com> wrote:
>>
>
>  > Note that the CSS spec talks about using '\(', not '\028'. I however
>> > have absolutely no idea how well this is supported by browsers.
>> >
>> I think I read somewhere that this doesn't work in IE (which seems to
>> interpret \ literally in URLs because windows users keep typing things
>>
> like
>
>> http:\\example.com\ into URL bars).  I should test this...
>>
>
> That's required for web compat'
>
> http://tools.ietf.org/html/draft-ietf-iri-3987bis-05#section-7.2
> A quick test in IE8 confirms that it correctly parsed "\" as an escape
> though:
> body { background:
> url('http://www.google.com/images/logos/ps_logo2.png\'\(\)'); }
> doesn't display the image, and shows with value
> "url(http://www.google.com/images/logos/ps_logo2.png'())" in the IE8
> Developer Tools (which we all know is bad at serializing its internal
> values). I tried with and without the enclosing single quotes.
>
Ok.  Maybe there were problems in older IEs that got fixed by 8 (pretty
common, that).


>
> I've rebased my working copy on the latest trunk, with the SafeStyles
> change. I'll correctly merge/integrate my changes in and re-test. Maybe
> I'll add the URL-escaping too (though for the CSS context, given the
> above quick-test, I'll probably rather \-escape).

I wouldn't worry about that if it gets too involved for this CL -- I agree
with your earlier point that this ought to belong into an extended
SafeStylesBuilder that knows about properties with url(...) values.   I
think we should just disallow SafeUri values in CSS context in a template.


> I'll see if we can add
> a check in UriUtils.fromTrustedString that it only contains "valid URI
> chars".
>
That would be useful; probably should be an assert so it doesn't affect
performance in compiled mode.


> If it ever happens to take a bit more time than "acceptable", though,
> I'll put TODOs instead, as these are rather edge cases anyway.
>
> Yes, that sounds good.

Thanks!
--xtof

>
>
> http://gwt-code-reviews.appspot.com/1380806/
>

Sign in to reply to this message.

It took me a bit more time than I initially thought but I'm happy with what I
finally came up with!

Following Postel's law, we're lenient in what we're accepting as valid URIs
(using the Web Addresses character set from iri-bis, initially coming from
HTML5) but strict in what we produce from UriUtils.encode and
UriUtils.encodeAllowEscapes.
There are two code paths: one for prod mode and one for dev mode and JVM. Dev
mode doesn't use JSNI then, which should speeds things up despite the more
complex algorithm.

I ran the SafeHtmlTemplatesTest and UriUtilsTest in all of JVM, dev mode and
prod mode (all in htmlunit only though).
I'm a bit tired now (it's 2AM here) so I hope I didn't break anything! ;-)

http://gwt-code-reviews.appspot.com/1380806/diff/16001/user/src/com/google/gw...
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):

http://gwt-code-reviews.appspot.com/1380806/diff/16001/user/src/com/google/gw...
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:127:
*   <li>If the template parameter occurs at the start of a URI-valued
On 2011/04/10 18:32:38, xtof wrote:
> There probably should be an explanation about the distinction between "start
of
> URL-valued attribute" and "entire URL-valued attribute"?

Done.

http://gwt-code-reviews.appspot.com/1380806/diff/16001/user/src/com/google/gw...
File user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/16001/user/src/com/google/gw...
user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java:46:
UriUtils.fromUntrustedString(GWT.getModuleBaseURL() + "clear.cache.gif");
On 2011/04/10 18:32:38, xtof wrote:
> I think here it would actually be appropriate to use fromTrustedString -- the
> value we're passing through here is fully under the control of the program (at
> least I think that GWT.getModuleBaseURL cannot be modified from the outside).

Done.

> Actually, if GWT.getModuleBaseURL() + someFile is a common idiom, it might be
> worth introducing a utility method in UriUtils that returns an absolute URL
> based at moduleBaseURL?  If it's not common, it won't be worth the clutter
> though.

I think we can expect most uses of "resources within the module 'space'" to be
using ClientBundle.

Sign in to reply to this message.

xtof

Thanks! just a few more comments, otherwise pretty much LGTM. 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 ...

2 years, 1 month ago #23

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/gw...
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
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/gw...
File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
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/gw...
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/gw...
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.

Sign in to reply to this message.

xtof

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

2 years, 1 month ago #25

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
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/gw...
File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
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/gw...
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/gw...
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).

Sign in to reply to this message.

skybrian

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/SafeUri.java File user/src/com/google/gwt/safehtml/shared/SafeUri.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gwt/safehtml/shared/SafeUri.java#newcode37 user/src/com/google/gwt/safehtml/shared/SafeUri.java:37: * the sense that doing so must not cause ...

2 years, 1 month ago #26

tbroyer

I didn't address Brian's requests because I honestly didn't know what to put (javascript: URIs? ...

2 years ago #27

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/gw...
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
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/gw...
File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
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/gw...
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/gw...
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/gw...
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/g...
File user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/test/com/google/g...
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.

Sign in to reply to this message.

skybrian

Sure, saying that javascript: and data: URL's are potentially unsafe would be a good start. ...

2 years ago #28

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, <t.broyer@gmail.com> 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/gw...
> File
>
>
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
> (right):
>
>
>
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
>
>
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/gw...
> File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right):
>
>
>
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
> 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/gw...
> 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/gw...
> 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/gw...
> 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/g...
> File user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
> (right):
>
>
>
http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/test/com/google/g...
> 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/
>

Sign in to reply to this message.

xtof

LGTM 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#newcode172 user/src/com/google/gwt/safehtml/shared/UriUtils.java:172: public static SafeUri fromTrustedString(String s) { On 2011/04/18 ...

2 years ago #29

LGTM

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
user/src/com/google/gwt/safehtml/shared/UriUtils.java:172: public static SafeUri
fromTrustedString(String s) {
On 2011/04/18 18:14:21, skybrian wrote:
> Could you put some examples of safe and unsafe URLs in the javadoc? This
should
> make it more obvious to reviewers of calls to this method what they should be
> looking for.

I think it's a bit more complicated than that.  In particular, it matters where
the value came from.  For example, we need to consider data: URIs as inherently
dangerous.  _however_ a data: URI that's fully program controlled (e.g., a
resource) is considered inherently safe.

I.e. the provenance of the value matters more than what the actual value looks
like.

Which is also why the SafeUri contract (as well as the SafeHtml) has this vague
language about "safe from cross site scripting".  In principle, the contract
could actually be written to state that evaluating the URI must not result in
execution of script, unless the script is fully under program control.

I wonder if we should make that change; for instance one might create a SafeUri
object for 'javascript:void(0);'.  Which does execute script, but is harmless
because the script is fully program controlled.   Which means it's not "cross
site" scripting.

I wonder if we should specify this in the SafeUri contract at this level of
detail?

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
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/gw...
user/src/com/google/gwt/user/client/ui/Image.java:144: private String url =
null;
On 2011/04/23 14:36:22, tbroyer wrote:
> 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.

Nice!  I'm glad this worked out :)

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:302:
// TODO(xtof): refactor HtmlContext with isStart/isEnd/isEntire accessors an
simplified type.
an[d] simplified?

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
File user/src/com/google/gwt/user/client/ui/Image.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
user/src/com/google/gwt/user/client/ui/Image.java:301: public abstract void
setVisibleRect(Image image, int left, int top, int width, int height);
Maybe undo this line wrap?

Sign in to reply to this message.

jat

LGTM with nits. http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/client/ImageResource.java File user/src/com/google/gwt/resources/client/ImageResource.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gwt/resources/client/ImageResource.java#newcode127 user/src/com/google/gwt/resources/client/ImageResource.java:127: String getURL(); Should this be deprecated? ...

2 years ago #30

skybrian

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#newcode172 user/src/com/google/gwt/safehtml/shared/UriUtils.java:172: public static SafeUri fromTrustedString(String s) { On 2011/04/27 17:18:58, ...

2 years ago #31

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/25001/user/src/com/google/gw...
user/src/com/google/gwt/safehtml/shared/UriUtils.java:172: public static SafeUri
fromTrustedString(String s) {
On 2011/04/27 17:18:58, xtof wrote:
> 
> I think it's a bit more complicated than that.  In particular, it matters
where
> the value came from.  For example, we need to consider data: URIs as
inherently
> dangerous.  _however_ a data: URI that's fully program controlled (e.g., a
> resource) is considered inherently safe.
> 
> I.e. the provenance of the value matters more than what the actual value looks
> like.
> 
> Which is also why the SafeUri contract (as well as the SafeHtml) has this
vague
> language about "safe from cross site scripting".  In principle, the contract
> could actually be written to state that evaluating the URI must not result in
> execution of script, unless the script is fully under program control.
> 
> I wonder if we should make that change; for instance one might create a
SafeUri
> object for 'javascript:void(0);'.  Which does execute script, but is harmless
> because the script is fully program controlled.   Which means it's not "cross
> site" scripting.
> 
> I wonder if we should specify this in the SafeUri contract at this level of
> detail?

Hmm. We could start by saying in the SafeUri contract what you just said:
provenance matters and SafeUri could contain any type of URL provided that it's
hard-coded by the app. For untrusted strings, we can refer people to
EscapeUtils.isSafeUri, which has a list of safe URL schemes.

From a reviewer's standpoint, I believe that if some code implementing the
SafeUri interface only constructs URL's for the schemes listed in isSafeUri()
then I know it's okay, and if not, I'd better get a real security review from
someone who knows web security better.

Sign in to reply to this message.

tbroyer

I addressed the feedback (including Brian's one, adding a paragraph to SafeUri javadoc) and formatted ...

2 years ago #32

I addressed the feedback (including Brian's one, adding a paragraph to SafeUri
javadoc) and formatted the files.

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
File user/src/com/google/gwt/resources/client/ImageResource.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
user/src/com/google/gwt/resources/client/ImageResource.java:127: String
getURL();
On 2011/04/27 18:01:32, jat wrote:
> Should this be deprecated?

Done.

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
File user/src/com/google/gwt/resources/rg/DataResourceGenerator.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
user/src/com/google/gwt/resources/rg/DataResourceGenerator.java:68:
sw.println("new " + DataResourcePrototype.class.getName() + "(");
On 2011/04/27 18:01:32, jat wrote:
> Should this be getCanonicalName()?  In this case they are the same, but in
> general getName() is not going to return a name you can use in the source.
> 
> If you change, change them all in this change.

ALL resource generators, and a whole bunch of other classes use getName()
instead getCanonicalName() (there's even a getName().replace('$','.') in
GwtCreateResourceGenerator!) so I'm gonna leave them as is.

Moreover, those classes are public API so they're not going to turn into nested
classes, unexpectedly breaking the generator, so I think it's OK to use
getName() in this case (getCanonicalName would be better, but getName is OK).

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
File user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java:481: new
String[]{bundle.getNormalContentsFieldName(), bundle.getRtlContentsFieldName()};
On 2011/04/27 18:01:32, jat wrote:
> space between ] and {

Done.

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
File
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java
(right):

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java:302:
// TODO(xtof): refactor HtmlContext with isStart/isEnd/isEntire accessors an
simplified type.
On 2011/04/27 17:18:58, xtof wrote:
> an[d] simplified?

Done.

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
File user/src/com/google/gwt/user/client/ui/Image.java (right):

http://gwt-code-reviews.appspot.com/1380806/diff/29005/user/src/com/google/gw...
user/src/com/google/gwt/user/client/ui/Image.java:301: public abstract void
setVisibleRect(Image image, int left, int top, int width, int height);
On 2011/04/27 18:01:32, jat wrote:
> On 2011/04/27 17:18:58, xtof wrote:
> > Maybe undo this line wrap?
> 
> Since we changed to 100 chars, my understanding is each file when edited
should
> be reformatted, though ideally as a separate change to avoid obfuscating the
> real change.  In this case, I don't care much either way.

Done.

Sign in to reply to this message.

skybrian

http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gwt/safehtml/shared/SafeUri.java File user/src/com/google/gwt/safehtml/shared/SafeUri.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gwt/safehtml/shared/SafeUri.java#newcode41 user/src/com/google/gwt/safehtml/shared/SafeUri.java:41: * context in which the URL is used matters ...

2 years ago #33

xtof

http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gwt/safehtml/shared/SafeUri.java File user/src/com/google/gwt/safehtml/shared/SafeUri.java (right): http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gwt/safehtml/shared/SafeUri.java#newcode41 user/src/com/google/gwt/safehtml/shared/SafeUri.java:41: * context in which the URL is used matters ...

2 years ago #34

tbroyer

On 2011/05/03 16:11:37, xtof wrote: > http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gwt/safehtml/shared/SafeUri.java > File user/src/com/google/gwt/safehtml/shared/SafeUri.java (right): > > http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gwt/safehtml/shared/SafeUri.java#newcode41 > ...

2 years ago #35

On 2011/05/03 16:11:37, xtof wrote:
>
http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gw...
> File user/src/com/google/gwt/safehtml/shared/SafeUri.java (right):
> 
>
http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gw...
> user/src/com/google/gwt/safehtml/shared/SafeUri.java:41: * context in which
the
> URL is used matters too (link {@code href} vs. image
> On 2011/05/02 18:57:13, skybrian wrote:
> > This sounds somewhat dangerous, actually. It seems like if context matters,
> > either each context should have its own type, or creators of SafeUrl
instances
> > should stick to the lowest common denominator.
> > 
> > The reason is that the code creating a SafeUrl instance might be far removed
> > from the code that uses it. If the creator believes that a URL will be used
in
> > iframe context, but actually it isn't, then reviewers cannot find the
problem
> > without having an end-to-end understanding of the program's data flow, and
any
> > refactoring of intermediate code has the potential to introduce a bug
without
> a
> > type error and without the reviewers seeing anything wrong.
> > 
> > It seems like the whole point of having safe types with clear contracts is
> make
> > sure that local reviews are sufficient to guarantee safety?
> > 
> > I hate the complexity this is likely to introduce, but on the other hand, a
> > SafeUrl type that isn't actually safe doesn't sound so great either.
> 
> I think I agree with Brian on this one.  Can we specify the contract such that
a
> SafeUri is safe to use in any of these contexts (at the expense of being
overly
> restrictive, for e.g. img src URIs)?  If not, I think we need to introduces
> separate types.  Otherwise we'll loose the local revieability benefit of the
> SafeXyz types.

If that's the only point left, can you make the edit yourself before submitting?
(as, while I agree, I don't know what to say exactly here)
Otherwise, if there are other comments requiring a new patchset, then I'll try
to address this one at the same time.

TIA

Sign in to reply to this message.

xtof

On Tue, May 3, 2011 at 09:17, <t.broyer@gmail.com> wrote: > On 2011/05/03 16:11:37, xtof wrote: ...

1 year, 11 months ago #36

On Tue, May 3, 2011 at 09:17, <t.broyer@gmail.com> wrote:

> On 2011/05/03 16:11:37, xtof wrote:
>
>
>
http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gw...
>
>> File user/src/com/google/gwt/safehtml/shared/SafeUri.java (right):
>>
>
>
>
>
http://gwt-code-reviews.appspot.com/1380806/diff/28033/user/src/com/google/gw...
>
>> user/src/com/google/gwt/safehtml/shared/SafeUri.java:41: * context in
>>
> which the
>
>> URL is used matters too (link {@code href} vs. image
>> On 2011/05/02 18:57:13, skybrian wrote:
>> > This sounds somewhat dangerous, actually. It seems like if context
>>
> matters,
>
>> > either each context should have its own type, or creators of SafeUrl
>>
> instances
>
>> > should stick to the lowest common denominator.
>> >
>> > The reason is that the code creating a SafeUrl instance might be far
>>
> removed
>
>> > from the code that uses it. If the creator believes that a URL will
>>
> be used in
>
>> > iframe context, but actually it isn't, then reviewers cannot find
>>
> the problem
>
>> > without having an end-to-end understanding of the program's data
>>
> flow, and any
>
>> > refactoring of intermediate code has the potential to introduce a
>>
> bug without
>
>> a
>> > type error and without the reviewers seeing anything wrong.
>> >
>> > It seems like the whole point of having safe types with clear
>>
> contracts is
>
>> make
>> > sure that local reviews are sufficient to guarantee safety?
>> >
>> > I hate the complexity this is likely to introduce, but on the other
>>
> hand, a
>
>> > SafeUrl type that isn't actually safe doesn't sound so great either.
>>
>
>  I think I agree with Brian on this one.  Can we specify the contract
>>
> such that a
>
>> SafeUri is safe to use in any of these contexts (at the expense of
>>
> being overly
>
>> restrictive, for e.g. img src URIs)?  If not, I think we need to
>>
> introduces
>
>> separate types.  Otherwise we'll loose the local revieability benefit
>>
> of the
>
>> SafeXyz types.
>>
>
> If that's the only point left, can you make the edit yourself before
> submitting? (as, while I agree, I don't know what to say exactly here)
> Otherwise, if there are other comments requiring a new patchset, then
> I'll try to address this one at the same time.
>
>
My apologies dropping the response on this thread (I'd missed the last
question and had assumed this was good to submit).

Also, I didn't realize that I actually need to fetch and submit this patch
(I'm not part of GWT team proper and wasn't familiar with the process for
changes from external developers).  I've now grabbed your latest patch set
and will prepare it for commit (I'm making a few minor fixes to make
extended presubmit tests pass).  I'll post an updated patch set for your
final review shortly.

With respect to the question about the SafeUri docs: I've changed the
relevant paragraphs to,

 * <p>
 * All implementing classes must maintain the class invariant (by design and
 * implementation and/or convention of use), that invoking {@link
#asString()}
 * on any instance will return a string that is safe to assign to a
URL-typed
 * DOM or CSS property in a browser (or to use similarly in a "URL
context"), in
 * the sense that doing so must not cause unintended execution of script in
the
 * browser.
 *
 * <p>
 * In determining safety of a URL both the value itself as well as its
 * provenance matter. An arbitrary URI, including e.g. a
 * <code>javascript:</code> URI, can be deemed safe in the sense of this
type's
 * contract if it is entirely under the program's control (e.g., a string
 * literal, {@see UriUtils#fromSafeConstant}).

Sound good?

Note:  UriUtils#fromSafeConstant doesn't exist in the current patch, but I
think it should; analogous to SafeHtmlUtils#fromSafeConstant.  This would
allow developers to concisely write,
SafeUri voidJs = UriUtils.fromSafeConstant("javascript:void(0)");
and similar.

Unless there are objections I'll add it; the implementation of
fromSafeConstant would be the exact same as fromTrustedString; however its
use signifies that its value is supposed to be a compile time constant,
whereas fromTrustedString may be passed a value that depends on user input,
but is somehow (though not trivially) known to be safe.   We shouldn't be
worried to see many "fromSafeConstant(<literal>)" uses in a program, but
should be worried if we see a lot of fromTrustedString ones (which would
have to be carefully manually reviewed, and if there's lots of uses it may
be an indication that a special case sanitizer is needed.

Again, sorry for letting this sit for several weeks :/

--xtof

> TIA
>
>
> http://gwt-code-reviews.appspot.com/1380806/
>

Sign in to reply to this message.

tbroyer

On 2011/05/31 23:38:17, xtof wrote: > My apologies dropping the response on this thread (I'd ...

1 year, 11 months ago #37

xtof

On 2011/06/01 00:10:58, tbroyer wrote: > On 2011/05/31 23:38:17, xtof wrote: > > My apologies ...

1 year, 11 months ago #38

tbroyer

On 2011/06/01 04:48:04, xtof wrote: > On 2011/06/01 00:10:58, tbroyer wrote: > > On 2011/05/31 ...

1 year, 11 months ago #39

xtof

On Wed, Jun 1, 2011 at 03:09, <t.broyer@gmail.com> wrote: > On 2011/06/01 04:48:04, xtof wrote: ...

1 year, 11 months ago #40

On Wed, Jun 1, 2011 at 03:09, <t.broyer@gmail.com> wrote:

> On 2011/06/01 04:48:04, xtof wrote:
>
>> On 2011/06/01 00:10:58, tbroyer wrote:
>> > On 2011/05/31 23:38:17, xtof wrote:
>> > > My apologies dropping the response on this thread (I'd missed the
>>
> last
>
>> > > question and had assumed this was good to submit).
>> > >
>> > > Also, I didn't realize that I actually need to fetch and submit
>>
> this patch
>
>> > > (I'm not part of GWT team proper and wasn't familiar with the
>>
> process for
>
>> > > changes from external developers).
>> >
>> > No problem.
>> >
>> > Otherwise, proposed changes SGTM.
>>
>
>  I can't seem to upload an updated patch set for this issue (presumably
>>
> since you
>
>> created it).  I'll mail you the patch set separately.
>>
>
> I hope I didn't mess things up during the merge, as I couldn't simply
> upload the patch to Rietveld (when you create an issue with
> git-cl/upload.py, it appears that you cannot update it "manually"
> afterwards; and I couldn't find an option to upload.py to give it the
> patch file either).
>

How annoying; this doesn't make collaborative editing particularly easy :)

Anyway, the merge seems to have come pretty close, though there were a few
differences (some of the changes that I made to appease checkstyle didn't
take, and one of the tests in SafeHtmlTemplatesTest seems to have gotten
duplicated.  I've downloaded your patch set and applied it to a clean client
at the same base CL; the diff of what I have vs what I got from your patch
set is appended below.



> Alternatively, you could create another issue in Rietveld…
>
> Done: http://gwt-code-reviews.appspot.com/1447812



>
> http://gwt-code-reviews.appspot.com/1380806/
>

diff --git
a/google3/third_party/java_src/gwt/svn/trunk/user/src/com/google/gwt/user/client/ui/impl/ClippedImageImplIE6.java
b/google3/third_party/java_src/gwt/svn/trunk/user/src/com/google/gwt/user/client/ui/impl/ClippedImageImplIE6.java
index d69fe3b..5d623ca 100644
---
a/google3/third_party/java_src/gwt/svn/trunk/user/src/com/google/gwt/user/client/ui/impl/ClippedImageImplIE6.java
+++
b/google3/third_party/java_src/gwt/svn/trunk/user/src/com/google/gwt/user/client/ui/impl/ClippedImageImplIE6.java
@@ -132,4 +132,12 @@ public class ClippedImageImplIE6 extends
ClippedImageImpl {

     return SafeHtmlUtils.fromTrustedString(clippedImgHtml);
   }
+
+  @Override
+  public Element getImgElement(Image image) {
+    if (!isIE6()) {
+      return super.getImgElement(image);
+    }
+    return image.getElement().getFirstChildElement();
+  }
 }
diff --git
a/google3/third_party/java_src/gwt/svn/trunk/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
b/google3/third_party/java_src/gwt/svn/trunk/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
index bc80a97..4721856 100644
---
a/google3/third_party/java_src/gwt/svn/trunk/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
+++
b/google3/third_party/java_src/gwt/svn/trunk/user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java
@@ -110,6 +110,16 @@ public class SafeHtmlTemplatesTest extends GWTTestCase
{
     Assert.assertEquals(
         "<span><a href=\"#\"><b>" + HTML_MARKUP + "</b></a></span>",
         templates.templateWithUriAttribute(
+            BAD_URL,
SafeHtmlUtils.fromSafeConstant(HTML_MARKUP)).asString());
+    // UriUtils.fromString: sanitized by fromString
+    Assert.assertEquals(
+        "<span><a href=\"" + GOOD_URL_ENCODED + "\"><b>" + HTML_MARKUP
+            + "</b></a></span>",
+        templates.templateWithUriAttribute(
+            UriUtils.fromString(GOOD_URL),
SafeHtmlUtils.fromSafeConstant(HTML_MARKUP)).asString());
+    Assert.assertEquals(
+        "<span><a href=\"#\"><b>" + HTML_MARKUP + "</b></a></span>",
+        templates.templateWithUriAttribute(
             UriUtils.fromString(BAD_URL),
SafeHtmlUtils.fromSafeConstant(HTML_MARKUP)).asString());
     // UriUtils.fromTrustedString: not sanitized
     Assert.assertEquals(
diff --git
a/google3/third_party/java_src/gwt/svn/trunk/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
b/google3/third_party/java_src/gwt/svn/trunk/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
index 4329e82..5c891dd 100644
---
a/google3/third_party/java_src/gwt/svn/trunk/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
+++
b/google3/third_party/java_src/gwt/svn/trunk/user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java
@@ -34,13 +34,13 @@ public class GwtUriUtilsTest extends GWTTestCase {
     StringBuilder sb = new StringBuilder(UriUtils.DONT_NEED_ENCODING);
     final int upcaseOffset = 'A' - 'a';
     for (char c = 'a'; c <= 'z'; c++) {
-      sb.append(c).append((char) (c + upcaseOffset));
+      sb.append(c).append((char)(c + upcaseOffset));
     }
     for (char c = '0'; c <= '9'; c++) {
       sb.append(c);
     }
     final String expected = sb.toString();
-
+
     assertEquals(expected, UriUtils.encode(expected));
   }

diff --git
a/google3/third_party/java_src/gwt/svn/trunk/user/test/com/google/gwt/safehtml/shared/UriUtilsTest.java
b/google3/third_party/java_src/gwt/svn/trunk/user/test/com/google/gwt/safehtml/shared/UriUtilsTest.java
index 71d3746..e0a650b 100644
---
a/google3/third_party/java_src/gwt/svn/trunk/user/test/com/google/gwt/safehtml/shared/UriUtilsTest.java
+++
b/google3/third_party/java_src/gwt/svn/trunk/user/test/com/google/gwt/safehtml/shared/UriUtilsTest.java
@@ -15,9 +15,7 @@
  */
 package com.google.gwt.safehtml.shared;

-/**
- * Unit tests for {@link UriUtils}.
- */
+
 public class UriUtilsTest extends GwtUriUtilsTest {

   // This forces a GWTTestCase to run as a vanilla JUnit TestCase.

Sign in to reply to this message.

Thanks :)

On Wed, Jun 1, 2011 at 13:27, <t.broyer@gmail.com> wrote:

> I don't know why I was trying such convoluted ways of applying the patch
> on top of my branch… Anyway, I replaced the 8th patch set with the
> (hopefully) "correct" one (and you'll commit your version anyway, as the
> differences, if any, are only very minor; I thought I'd better try
> updating this CL though as it makes the changes easier to review for
> others)
>
>
> http://gwt-code-reviews.appspot.com/1380806/
>

Sign in to reply to this message.

Issue 1380806: Add SafeUri type, similar to SafeHtml but for values in a URL attribute context. (Closed)

Description

Patch Set 1

Patch Set 2 : Addresses xtof feedback

Patch Set 3 : UriUtils.fromUntrustedString and URL_ATTRIBUTE_ENTIRE

Patch Set 4 : Added URI validation and encoding

Patch Set 5 : Switched Image internals to SafeUri + other feedback

Patch Set 6 : feedback and format

Patch Set 7 : Updated to latest trunk (r10241)

Patch Set 8 : Update on behalf of xtof

Messages