Changes FormatURL to not strip http if the host starts with ftp or https[!a-z]. This...

Changes FormatURL to not strip http if the host starts with ftp or https[!a-z]. This
is necessitated by the omnibox treating ftp.www.com as
ftp://www.com as well as security problems.

I also changed FormatURL to always reset the supplied
url_parse::Parsed. I did this as in writing my test I uncovered a long
standing bug where a component wasn't getting reset. It seems much
safer to always reset than rely on all code paths setting every
component.

BUG=41585, 41652
TEST=make sure the omnibox doesn't strip http off urls that start with
ftp.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=44943

[Peter Kasting, 2010-04-16 16:43:09]

This change is fine, but we should also include the scheme if the URL starts
with "https" -- see bug 41652.

http://codereview.chromium.org/1574034/diff/1/3
File net/base/net_util.cc (right):

http://codereview.chromium.org/1574034/diff/1/3#newcode1449
net/base/net_util.cc:1449: spec.compare(parsed.host.begin,
Nit: I would probably explicitly compare against 0 for clarity.

[sky, 2010-04-16 17:49:08]

Ok, I've incorporated the fix for 41652 as well.

  -Scott

[Peter Kasting, 2010-04-16 18:06:59]

LGTM with one substantive change

http://codereview.chromium.org/1574034/diff/10001/11002
File net/base/net_util.cc (right):

http://codereview.chromium.org/1574034/diff/10001/11002#newcode771
net/base/net_util.cc:771: const char* const kFTP = "ftp.";
Nit: If you're going to put kHTTPS underneath the associated comment block, I
would put this under the respective comment block too.

http://codereview.chromium.org/1574034/diff/10001/11002#newcode773
net/base/net_util.cc:773: // The omnibox treats ftp.foo.com as ftp://foo.com.
This means that if we
Nit: "ftp://ftp.foo.com", not "ftp://foo.com" (two places)

http://codereview.chromium.org/1574034/diff/10001/11002#newcode777
net/base/net_util.cc:777: // don't strip http off url's whose scheme is http and
the host starts with
Nit: Grammar.

How about this for the comment:

// The omnibox normalizes hosts that begin with "ftp." to have the "ftp" scheme,
instead of the "http" scheme that other bare hostnames get.  So if a user types
"ftp.foo.com", they're sent to "ftp://ftp.foo.com/" instead of
"http://ftp.foo.com/".  This means that we should not strip "http://" from a URL
whose host begins "ftp.", since if the user then re-navigates to the displayed
URL, they'll wind up at a different location.

http://codereview.chromium.org/1574034/diff/10001/11002#newcode782
net/base/net_util.cc:782: // If the host starts with https[!a-z] we dont strip
either, otherwise the
I just realized that this misses one obvious case:

http://https//paypal.com

Here the hostname is just "https".  So I think we should also not strip the
scheme if the hostname is "https" alone.

An updated comment might be:

// If the host is "https" (or "https" followed by a non-alphabetic character),
this might be an attempt to spoof a user, so we don't hide the scheme here
either.

http://codereview.chromium.org/1574034/diff/10001/11002#newcode788
net/base/net_util.cc:788: host[kHTTPSSize] < 'a' || host[kHTTPSSize] > 'z') {
Nit: How about just

return (host.compare(0, kHTTPSSize, kHTTPS) != 0) || ((host.size() > kHTTPSSize)
&& (host[kHTTPSSize] >= 'a') && (host[kHTTPSSize] <= 'z'));

(This includes returning "false" for a host of "https")

BTW, is the host lowercased at this point so comparing against 'a' and 'z' is
OK?

[sky, 2010-04-16 18:28:36]

http://codereview.chromium.org/1574034/diff/10001/11002
File net/base/net_util.cc (right):

http://codereview.chromium.org/1574034/diff/10001/11002#newcode782
net/base/net_util.cc:782: // If the host starts with https[!a-z] we dont strip
either, otherwise the
On 2010/04/16 18:07:00, Peter Kasting wrote:
> I just realized that this misses one obvious case:
> 
> http://https//paypal.com
> 
> Here the hostname is just "https".  So I think we should also not strip the
> scheme if the hostname is "https" alone.

I thought of this, but then is someone really going to have an internal domain
as https? But I'll add coverage for it.

http://codereview.chromium.org/1574034/diff/10001/11002#newcode788
net/base/net_util.cc:788: host[kHTTPSSize] < 'a' || host[kHTTPSSize] > 'z') {
On 2010/04/16 18:07:00, Peter Kasting wrote:
> Nit: How about just
> 
> return (host.compare(0, kHTTPSSize, kHTTPS) != 0) || ((host.size() >
kHTTPSSize)
> && (host[kHTTPSSize] >= 'a') && (host[kHTTPSSize] <= 'z'));
> 
> (This includes returning "false" for a host of "https")

I realize I could do that, in fact I could have combined it all into a single if
but I find it more readable separated out this.

> BTW, is the host lowercased at this point so comparing against 'a' and 'z' is

Yes, Brett says it's always lowercase.