Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(404)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 7 months ago by tbroyer
Modified:
3 years, 4 months ago
CC:
google-web-toolkit-contributors_googlegroups.com
Base URL:
Visibility:
Public.

Description

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

What matters most is direct support in SafeHtmlTemplates.

Also added a few overloads in c.g.g.user.client.

Note that this is a breaking change in the sense that DataResource and
ImageResource have a new getSafeUri method.

See http://code.google.com/p/google-web-toolkit/issues/detail?id=6145

Patch Set 1

Total comments: 19

Patch Set 2 : Addresses xtof feedback

Total comments: 3

Patch Set 3 : UriUtils.fromUntrustedString and URL_ATTRIBUTE_ENTIRE

Total comments: 4

Patch Set 4 : Added URI validation and encoding

Total comments: 24

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

Total comments: 11

Patch Set 6 : feedback and format

Total comments: 2

Patch Set 7 : Updated to latest trunk (r10241)

Patch Set 8 : Update on behalf of xtof

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M tools/api-checker/config/gwt23_24userApi.conf View 1 2 3 4 5 6 7 2 chunks +17 lines, -0 lines 0 comments Download
M user/src/com/google/gwt/cell/client/ImageResourceCell.java View 1 2 3 4 5 6 1 chunk +0 lines, -7 lines 0 comments Download
user/src/com/google/gwt/resources/Resources.gwt.xml View 1 chunk +2 lines, -0 lines 0 comments Download
M user/src/com/google/gwt/resources/client/DataResource.java View 2 chunks +7 lines, -0 lines 0 comments Download
M user/src/com/google/gwt/resources/client/ImageResource.java View 1 2 3 4 5 2 chunks +9 lines, -0 lines 0 comments Download
A user/src/com/google/gwt/resources/client/impl/DataResourcePrototype.java View 1 chunk +47 lines, -0 lines 0 comments Download
M user/src/com/google/gwt/resources/client/impl/ExternalTextResourcePrototype.java View 1 2 3 4 5 4 chunks +7 lines, -7 lines 0 comments Download
M user/src/com/google/gwt/resources/client/impl/ImageResourcePrototype.java View 1 2 3 4 5 5 chunks +12 lines, -10 lines 0 comments Download
M user/src/com/google/gwt/resources/css/Spriter.java View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M user/src/com/google/gwt/resources/rg/DataResourceGenerator.java View 1 2 3 4 5 2 chunks +15 lines, -31 lines 0 comments Download
M user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java View 1 2 3 4 5 6 7 5 chunks +8 lines, -6 lines 0 comments Download
M user/src/com/google/gwt/resources/rg/ImageResourceGenerator.java View 1 2 3 4 5 24 chunks +62 lines, -81 lines 0 comments Download
M user/src/com/google/gwt/safehtml/rebind/SafeHtmlTemplatesImplMethodCreator.java View 1 2 3 4 5 6 7 17 chunks +109 lines, -60 lines 0 comments Download
M user/src/com/google/gwt/safehtml/shared/SafeHtmlUtils.java View 1 2 3 4 5 4 chunks +5 lines, -8 lines 0 comments Download
A user/src/com/google/gwt/safehtml/shared/SafeUri.java View 1 2 3 4 5 6 7 1 chunk +80 lines, -0 lines 0 comments Download
A user/src/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java View 1 2 3 1 chunk +119 lines, -0 lines 0 comments Download
A user/src/com/google/gwt/safehtml/shared/SafeUriString.java View 1 1 chunk +78 lines, -0 lines 0 comments Download
M user/src/com/google/gwt/safehtml/shared/UriUtils.java View 1 2 3 4 5 6 7 5 chunks +185 lines, -13 lines 0 comments Download
M user/src/com/google/gwt/user/client/ui/AbstractImagePrototype.java View 1 2 3 4 5 3 chunks +24 lines, -3 lines 0 comments Download
M user/src/com/google/gwt/user/client/ui/FormPanel.java View 1 2 3 4 5 5 chunks +15 lines, -7 lines 0 comments Download
M user/src/com/google/gwt/user/client/ui/Frame.java View 2 chunks +10 lines, -0 lines 0 comments Download
M user/src/com/google/gwt/user/client/ui/Image.java View 1 2 3 4 5 6 21 chunks +107 lines, -40 lines 0 comments Download
M user/src/com/google/gwt/user/client/ui/ImageResourceRenderer.java View 1 2 3 4 5 6 2 chunks +1 line, -2 lines 0 comments Download
M user/src/com/google/gwt/user/client/ui/impl/ClippedImageImpl.java View 1 2 3 4 5 6 7 2 chunks +39 lines, -22 lines 0 comments Download
M user/src/com/google/gwt/user/client/ui/impl/ClippedImageImplIE6.java View 1 2 3 4 5 6 7 7 chunks +25 lines, -24 lines 0 comments Download
M user/src/com/google/gwt/user/client/ui/impl/ClippedImagePrototype.java View 1 2 3 4 5 6 7 5 chunks +15 lines, -8 lines 0 comments Download
A user/super/com/google/gwt/safehtml/super/com/google/gwt/safehtml/shared/SafeUriHostedModeUtils.java View 1 2 3 1 chunk +35 lines, -0 lines 0 comments Download
M user/test/com/google/gwt/resources/client/ImageResourceTest.java View 1 2 3 4 5 4 chunks +10 lines, -3 lines 0 comments Download
M user/test/com/google/gwt/safehtml/client/SafeHtmlTemplatesTest.java View 1 2 3 4 5 6 7 9 chunks +48 lines, -17 lines 0 comments Download
A user/test/com/google/gwt/safehtml/shared/GwtUriUtilsTest.java View 1 2 3 4 5 6 7 1 chunk +139 lines, -0 lines 0 comments Download
A user/test/com/google/gwt/safehtml/shared/UriUtilsTest.java View 1 2 3 4 5 6 7 1 chunk +37 lines, -0 lines 0 comments Download
M user/test/com/google/gwt/user/client/ui/ImageTest.java View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M user/test/com/google/gwt/user/client/ui/impl/ClippedImagePrototypeTest.java View 1 2 3 4 5 6 7 7 chunks +24 lines, -47 lines 0 comments Download

Messages

Total messages: 42
tbroyer
3 years, 7 months ago #1
tbroyer
I tried to limit the changes to non-formatting ones. I also didn't go as far ...
3 years, 7 months ago #2
tbroyer
I'm having a hard time integrating it with UiBinder because of the way attributes are ...
3 years, 7 months ago #3
rjrjr
Christoph, can you take this review? On Sat, Mar 19, 2011 at 10:04 AM, <t.broyer@gmail.com> ...
3 years, 7 months ago #4
xtof
On Wed, Mar 23, 2011 at 09:54, Ray Ryan <rjrjr@google.com> wrote: > Christoph, can you ...
3 years, 7 months 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 ...
3 years, 7 months ago #6
tbroyer
3 years, 6 months ago #7
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 ...
3 years, 6 months ago #8
xtof
Thanks for your changes! There's one more thing: In the past couple of days we've ...
3 years, 6 months ago #9
tbroyer
I totally agree with your analysis and ENTIRE_URL_ATTRIBUTE state (I had similar concerns, but couldn't ...
3 years, 6 months ago #10
xtof
On Tue, Mar 29, 2011 at 13:30, <t.broyer@gmail.com> wrote: > I totally agree with your ...
3 years, 6 months ago #11
tbroyer
On 2011/03/29 20:38:29, xtof wrote: > > > > - Implement the public Image(String url,...) ...
3 years, 6 months 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: ...
3 years, 6 months ago #13
tbroyer
I followed the approach from the SafeStyles for the checks in the generator, with the ...
3 years, 6 months ago #14
xtof
I think this is pretty much ready, except for one thing I just thought of. ...
3 years, 6 months ago #15
tbroyer
On 2011/04/10 18:32:38, xtof wrote: > I think this is pretty much ready, except for ...
3 years, 6 months ago #16
xtof
On Sun, Apr 10, 2011 at 17:02, <t.broyer@gmail.com> wrote: > On 2011/04/10 18:32:38, xtof wrote: ...
3 years, 6 months ago #17
tbroyer
On 2011/04/11 23:18:13, xtof wrote: > If we're both in agreement on that, I think ...
3 years, 6 months ago #18
xtof
On Mon, Apr 11, 2011 at 17:54, <t.broyer@gmail.com> wrote: > > Note that the CSS ...
3 years, 6 months ago #19
tbroyer
On 2011/04/12 01:14:10, xtof wrote: > On Mon, Apr 11, 2011 at 17:54, <t.broyer@gmail.com> wrote: ...
3 years, 6 months 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: ...
3 years, 6 months ago #21
tbroyer
It took me a bit more time than I initially thought but I'm happy with ...
3 years, 6 months ago #22
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 ...
3 years, 6 months ago #23
tbroyer
I probably won't have time to update the patch before (at least) the second half ...
3 years, 6 months ago #24
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 ...
3 years, 6 months ago #25
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 ...
3 years, 6 months ago #26
tbroyer
I didn't address Brian's requests because I honestly didn't know what to put (javascript: URIs? ...
3 years, 6 months ago #27
skybrian
Sure, saying that javascript: and data: URL's are potentially unsafe would be a good start. ...
3 years, 6 months ago #28
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 ...
3 years, 5 months ago #29
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? ...
3 years, 5 months 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, ...
3 years, 5 months ago #31
tbroyer
I addressed the feedback (including Brian's one, adding a paragraph to SafeUri javadoc) and formatted ...
3 years, 5 months ago #32
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 ...
3 years, 5 months 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 ...
3 years, 5 months 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 > ...
3 years, 5 months ago #35
xtof
On Tue, May 3, 2011 at 09:17, <t.broyer@gmail.com> wrote: > On 2011/05/03 16:11:37, xtof wrote: ...
3 years, 4 months ago #36
tbroyer
On 2011/05/31 23:38:17, xtof wrote: > My apologies dropping the response on this thread (I'd ...
3 years, 4 months ago #37
xtof
On 2011/06/01 00:10:58, tbroyer wrote: > On 2011/05/31 23:38:17, xtof wrote: > > My apologies ...
3 years, 4 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 ...
3 years, 4 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: ...
3 years, 4 months ago #40
tbroyer
I don't know why I was trying such convoluted ways of applying the patch on ...
3 years, 4 months ago #41
xtof
3 years, 4 months ago #42
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld revision f51cb906c4ad+