Update EscapeExternalHandlerValue to keep '#', '[', and ']'

Update EscapeExternalHandlerValue to keep '#', '[', and ']'

According to RFC 3986, '#' is used as a separator of fragment part, and
'[' and ']' are used for IPv6 address.
So they should be kept as they are in a URL string.
And EscapeExternalHandlerValue should escape '%' if it isn't used as a
prefix for %XX.

BUG=419257
Committed: https://crrev.com/9e212e96140c2cfc9d259ef6ce620895d0e5a384
Cr-Commit-Position: refs/heads/master@{#301013}

[Jaekyun Seok (inactive), 2014-10-13 01:42:56]

jaekyun@chromium.org changed reviewers:
+ asanka@chromium.org

[Jaekyun Seok (inactive), 2014-10-13 01:47:10]

Asanka, could you please review this change because this is followup of
https://codereview.chromium.org/632843003/ which you reviewed actively?

[Jaekyun Seok (inactive), 2014-10-14 04:48:43]

PTAL.

I modified codes to sanitize fragment and path parts as well.

[asanka, 2014-10-14 16:46:03]

https://codereview.chromium.org/646333002/diff/80001/chrome/browser/android/u...
File chrome/browser/android/url_sanitize_utils.cc (right):

https://codereview.chromium.org/646333002/diff/80001/chrome/browser/android/u...
chrome/browser/android/url_sanitize_utils.cc:52: }};
Are these tables different from net/base/escape.cc? If not, we should use the
implementation there to escape individual components of the URL.

https://codereview.chromium.org/646333002/diff/80001/chrome/browser/android/u...
File chrome/browser/android/url_sanitize_utils.h (right):

https://codereview.chromium.org/646333002/diff/80001/chrome/browser/android/u...
chrome/browser/android/url_sanitize_utils.h:15: // a URI query part. See RFC
2396 for the list of reserved characters.
* Mention that this function escapes all but the strict set of allowed
characters in each component of the URL.

* Note why this is required.

* Note differences between what GURL already does.

[Jaekyun Seok (inactive), 2014-10-15 08:02:25]

PTAL.

https://codereview.chromium.org/646333002/diff/80001/chrome/browser/android/u...
File chrome/browser/android/url_sanitize_utils.cc (right):

https://codereview.chromium.org/646333002/diff/80001/chrome/browser/android/u...
chrome/browser/android/url_sanitize_utils.cc:52: }};
Yes, both are different from net/base/escape.cc.

Moreover, functions in net/base/escape.cc escape already escaped characters
(%XX). So I can't use them anyway.

https://codereview.chromium.org/646333002/diff/80001/chrome/browser/android/u...
File chrome/browser/android/url_sanitize_utils.h (right):

https://codereview.chromium.org/646333002/diff/80001/chrome/browser/android/u...
chrome/browser/android/url_sanitize_utils.h:15: // a URI query part. See RFC
2396 for the list of reserved characters.
On 2014/10/14 16:46:03, asanka wrote:
> * Mention that this function escapes all but the strict set of allowed
> characters in each component of the URL.
> 
> * Note why this is required.
> 
> * Note differences between what GURL already does.

Done.

[Jaekyun Seok (inactive), 2014-10-16 21:35:26]

jaekyun@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Jaekyun Seok (inactive), 2014-10-16 21:37:25]

Ryan, could you please review this change?

This is followup of https://codereview.chromium.org/632843003/ which you
reviewed.

Thanks,

[Ryan Sleevi, 2014-10-16 21:40:48]

Its generally considered poor code review etiquette to switch reviewers midway.
I'm going to decline reviewing this to allow Asanka to finish his review. If he
thinks I need to, I'm happy to look at this, but generally you should continue
with your same reviewer and ensure they are happy.

[Jaekyun Seok (inactive), 2014-10-16 21:47:37]

Really sorry if my asking was unpleasant.

I didn't intent to switch reviewers, but I just wanted to receive some feedbacks
about my change.

Anyway, I will wait for Asanka's review.

Thanks,

[asanka, 2014-10-17 00:07:28]

https://codereview.chromium.org/646333002/diff/80001/chrome/browser/android/u...
File chrome/browser/android/url_sanitize_utils.cc (right):

https://codereview.chromium.org/646333002/diff/80001/chrome/browser/android/u...
chrome/browser/android/url_sanitize_utils.cc:52: }};
On 2014/10/15 08:02:25, Jaekyun Seok wrote:
> Yes, both are different from net/base/escape.cc.
> 
> Moreover, functions in net/base/escape.cc escape already escaped characters
> (%XX). So I can't use them anyway.

Doesn't net::EscapeExternalHandlerValue() do what you want?

[Jaekyun Seok (inactive), 2014-10-17 00:14:36]

> Doesn't net::EscapeExternalHandlerValue() do what you want?

EscapeExternalHandlerValue() just passes "%", so I can't use it because "%"
isn't allowed in query/fragment/path part according to RFC 2396.

[Jaekyun Seok (inactive), 2014-10-17 00:17:05]

On 2014/10/17 00:14:36, Jaekyun Seok wrote:
> > Doesn't net::EscapeExternalHandlerValue() do what you want?
> 
> EscapeExternalHandlerValue() just passes "%", so I can't use it because "%"
> isn't allowed in query/fragment/path part according to RFC 2396.

I meant that "%" isn't allowed in query/fragment/path parts if it isn't used as
a prefix for escaped characters according to RFC 2396.

[asanka, 2014-10-17 00:30:11]

On 2014/10/17 00:17:05, Jaekyun Seok wrote:
> On 2014/10/17 00:14:36, Jaekyun Seok wrote:
> > > Doesn't net::EscapeExternalHandlerValue() do what you want?
> > 
> > EscapeExternalHandlerValue() just passes "%", so I can't use it because "%"
> > isn't allowed in query/fragment/path part according to RFC 2396.
> 
> I meant that "%" isn't allowed in query/fragment/path parts if it isn't used
as
> a prefix for escaped characters according to RFC 2396.

Then we should consider fixing EscapeExternalHandlerValue() rather than adding a
separate function.

Interestingly this function is named poorly and doesn't have unittests. You
should be able to reuse the tests you've introduced in
url_sanitize_util_unittest.cc.

Apologies for the route this change is taking, but it's best to get it right
rather than add more nearly duplicate code elsewhere.

I feel like ultimately we should move all the URL fiddling into the /url/
directory and not have this sort of code in /net/. But that's a discussion for
another day.

[Jaekyun Seok (inactive), 2014-10-17 01:02:48]

> Interestingly this function is named poorly and doesn't have unittests. You
> should be able to reuse the tests you've introduced in
> url_sanitize_util_unittest.cc.

I can fix it, and I might reuse it for query/fragment parts, but there isn't the
same character set with one for path part in escape.cc.

kPathCharmap in escape.cc doesn't allow ":", but URI path can have it according
to RFC 2396.

Do you want me to add a new character set for URI path into escape.cc?

[Jaekyun Seok (inactive), 2014-10-17 02:34:48]

I might have misunderstood your comments.

Do you mean that EscapeExternalHandlerValue() can be used to sanitize a whole
url after fixing some bugs on it?

[asanka, 2014-10-17 19:28:37]

On 2014/10/17 02:34:48, Jaekyun Seok wrote:
> I might have misunderstood your comments.
> 
> Do you mean that EscapeExternalHandlerValue() can be used to sanitize a whole
> url after fixing some bugs on it?

The latter. I.e. update the function to escape all but the union of allowable
characters in a URL. You can be conservative and only touch the components that
allows percent encoded characters, although if you need to escape characters
elsewhere (scheme? port?) the URL wasn't valid to begin with.

Basically, that function should take a valid URL (with the exception of a few
permissive characters) and produce a valid URL that conforms to the strict
character limitations imposed by RFC 2396. If the input isn't considered valid
by GURL on input, you can just return the string as-is and let the caller deal
with it.

[Jaekyun Seok (inactive), 2014-10-20 04:50:30]

PTAL.

I updated EscapeExternalHandlerValue() to keep '#', '[', and ']' with some
unittests for it.

And I modified Escape() to escape '%' if it isn't used as a prefix for %XX.

[Jaekyun Seok (inactive), 2014-10-21 03:04:48]

Ping.

[Jaekyun Seok (inactive), 2014-10-22 08:14:11]

Asanka, could you please take a look at this change?

I updated EscapeExternalHandlerValue as you commented.

[asanka, 2014-10-22 22:51:40]

Sorry about the delay. I'll review this momentarily.

[asanka, 2014-10-23 07:20:04]

https://codereview.chromium.org/646333002/diff/140001/net/base/escape.cc
File net/base/escape.cc (right):

https://codereview.chromium.org/646333002/diff/140001/net/base/escape.cc#newc...
net/base/escape.cc:302: // !'()*-._~#[]
Why remove '%'? Shouldn't we escape a % if it isn't a valid percent escape
sequence?

https://codereview.chromium.org/646333002/diff/140001/net/base/escape.h
File net/base/escape.h (right):

https://codereview.chromium.org/646333002/diff/140001/net/base/escape.h#newco...
net/base/escape.h:46: // chracters (;/?:@&=+$,#[]), and ones which are already
escaped.
Since % is already mentioned in the exclusion list above, you should elaborate
what "ones which are already escaped" means.

https://codereview.chromium.org/646333002/diff/140001/net/base/escape_unittes...
File net/base/escape_unittest.cc (right):

https://codereview.chromium.org/646333002/diff/140001/net/base/escape_unittes...
net/base/escape_unittest.cc:456: TEST(EscapeTest, EscapeExternalHandlerValue) {
Have a test case for our identity set:

"!#$&'()*+,-./0123456789:;=?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[]_abcdefghijklmnopqrstuvwxyz~"

It should pass through unchanged.

https://codereview.chromium.org/646333002/diff/140001/net/base/escape_unittes...
net/base/escape_unittest.cc:456: TEST(EscapeTest, EscapeExternalHandlerValue) {
Add a test case for invalid percent escape sequences:

E.g.: "%9k", "a%8", "a%"

[Jaekyun Seok (inactive), 2014-10-23 12:43:19]

PTAL.

https://codereview.chromium.org/646333002/diff/140001/net/base/escape.cc
File net/base/escape.cc (right):

https://codereview.chromium.org/646333002/diff/140001/net/base/escape.cc#newc...
net/base/escape.cc:302: // !'()*-._~#[]
Yes. And so '%' should be removed because kExternalHandlerCharmap includes
everything except characters which aren't escaped.

I introduced a separated logic to keep '%' only if it is a valid percent escape
sequence.

https://codereview.chromium.org/646333002/diff/140001/net/base/escape.h
File net/base/escape.h (right):

https://codereview.chromium.org/646333002/diff/140001/net/base/escape.h#newco...
net/base/escape.h:46: // chracters (;/?:@&=+$,#[]), and ones which are already
escaped.
Sorry, I should have removed '%' because here we are listing exceptional
characters which will not be escaped by EscapeExternalHandlerValue. Now I
removed '%' here.

And I updated the comments about "ones which are already escaped".

https://codereview.chromium.org/646333002/diff/140001/net/base/escape_unittes...
File net/base/escape_unittest.cc (right):

https://codereview.chromium.org/646333002/diff/140001/net/base/escape_unittes...
net/base/escape_unittest.cc:456: TEST(EscapeTest, EscapeExternalHandlerValue) {
On 2014/10/23 07:20:04, asanka wrote:
> Add a test case for invalid percent escape sequences:
> 
> E.g.: "%9k", "a%8", "a%"

Done.

https://codereview.chromium.org/646333002/diff/140001/net/base/escape_unittes...
net/base/escape_unittest.cc:456: TEST(EscapeTest, EscapeExternalHandlerValue) {
On 2014/10/23 07:20:04, asanka wrote:
> Have a test case for our identity set:
> 
>
"!#$&'()*+,-./0123456789:;=?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[]_abcdefghijklmnopqrstuvwxyz~"
> 
> It should pass through unchanged.

Done.

[asanka, 2014-10-23 19:17:13]

lgtm

https://codereview.chromium.org/646333002/diff/180001/net/base/escape_unittes...
File net/base/escape_unittest.cc (right):

https://codereview.chromium.org/646333002/diff/180001/net/base/escape_unittes...
net/base/escape_unittest.cc:504: EscapeExternalHandlerValue("%8k,a%,%a,a%8"));
I meant these to be separate tests cases. The "% near end of string" test cases
don't work when they are in the middle of a string.

Also, add a test to demonstrate how %ab and %AB are escaped (that their case is
preserved).

[Jaekyun Seok (inactive), 2014-10-24 00:01:47]

Really thanks for the long review and giving LGTM!

https://codereview.chromium.org/646333002/diff/180001/net/base/escape_unittes...
File net/base/escape_unittest.cc (right):

https://codereview.chromium.org/646333002/diff/180001/net/base/escape_unittes...
net/base/escape_unittest.cc:504: EscapeExternalHandlerValue("%8k,a%,%a,a%8"));
On 2014/10/23 19:17:13, asanka wrote:
> I meant these to be separate tests cases. The "% near end of string" test
cases
> don't work when they are in the middle of a string.
> 
> Also, add a test to demonstrate how %ab and %AB are escaped (that their case
is
> preserved).

Done.

[Jaekyun Seok (inactive), 2014-10-24 00:02:06]

The CQ bit was checked by jaekyun@chromium.org

[commit-bot: I haz the power, 2014-10-24 00:03:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/646333002/200001

[commit-bot: I haz the power, 2014-10-24 01:11:23]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2014-10-24 01:12:01]

Patchset 8 (id:??) landed as
https://crrev.com/9e212e96140c2cfc9d259ef6ce620895d0e5a384
Cr-Commit-Position: refs/heads/master@{#301013}