Small improvements to MixedContentChecker.

Small improvements to MixedContentChecker.

Just a couple of improvements to MixedContentChecker:
- Remove uneeded check for data protocol: it's already included in the protocols
checked in SecurityOrigin::isSecure.
- Improved check for localhost by name to be more complete.

BUG=607878, 624275
Committed: https://crrev.com/c9d34b4a2d425b25bdf90e9bf785cd7132f73037
Cr-Commit-Position: refs/heads/master@{#406000}

[carlosk, 2016-07-13 16:19:20]

carlosk@chromium.org changed reviewers:
+ mkwst@chromium.org

[carlosk, 2016-07-13 16:19:20]

mkwst@ PTAL.

One thing I'm still in doubt about is what I changed from the 1st and 2nd patch
sets. Looking into SecurityOrigin::isLocalhost it seems not accept as local any
kind of IPv6 host except for "[::1]". So at first I tried to mimic that.

But looking at how IsLocalHostname (in net/base/url_util.cc) is implemented it
does consider other forms of IPv6 addresses as being for a localhost and that
seemed more correct to me. But WDYT?

[Mike West, 2016-07-14 05:50:17]

https://codereview.chromium.org/2151473002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (left):

https://codereview.chromium.org/2151473002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:95: bool isAllowed
= url.protocolIsData() || SecurityOrigin::isSecure(url) ||
SecurityOrigin::create(url)->isPotentiallyTrustworthy();
Do we have test coverage for `data:`? I'm pretty sure we do, but would you mind
checking?

https://codereview.chromium.org/2151473002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/NetworkUtils.h (right):

https://codereview.chromium.org/2151473002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/NetworkUtils.h:17: PLATFORM_EXPORT
bool isLocalHostname(const String& host, bool* isLocal6);
Should we use this for `SecurityOrigin::isLocalhost()` as well?

[Mike West, 2016-07-18 08:36:44]

ping?

[carlosk, 2016-07-18 09:38:36]

Sorry, we were on holidays Thu and Fri last week.

https://codereview.chromium.org/2151473002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (left):

https://codereview.chromium.org/2151473002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:95: bool isAllowed
= url.protocolIsData() || SecurityOrigin::isSecure(url) ||
SecurityOrigin::create(url)->isPotentiallyTrustworthy();
On 2016/07/14 05:50:17, Mike West wrote:
> Do we have test coverage for `data:`? I'm pretty sure we do, but would you
mind
> checking?

In TEST(MixedContentCheckerTest, IsMixedContent) we test these cases:

{"http://example.com/foo", "data:text/html,<p>Hi!</p>", false},
{"https://example.com/foo", "data:text/html,<p>Hi!</p>", false},

So I think that should do it.

https://codereview.chromium.org/2151473002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/NetworkUtils.h (right):

https://codereview.chromium.org/2151473002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/NetworkUtils.h:17: PLATFORM_EXPORT
bool isLocalHostname(const String& host, bool* isLocal6);
On 2016/07/14 05:50:17, Mike West wrote:
> Should we use this for `SecurityOrigin::isLocalhost()` as well?

Possibly but I didn't have a use for that one in this change so I didn't add it.

My objective here was to being bringing Blink concepts closer to what we use
browser side. I recently noticed many differences around networking and
security. This seemed like a simple first step.

[Mike West, 2016-07-18 12:24:17]

https://codereview.chromium.org/2151473002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/NetworkUtils.h (right):

https://codereview.chromium.org/2151473002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/NetworkUtils.h:17: PLATFORM_EXPORT
bool isLocalHostname(const String& host, bool* isLocal6);
On 2016/07/18 at 09:38:36, carlosk wrote:
> On 2016/07/14 05:50:17, Mike West wrote:
> > Should we use this for `SecurityOrigin::isLocalhost()` as well?
> 
> Possibly but I didn't have a use for that one in this change so I didn't add
it.

I'd suggest changing it. In fact, I'd suggest replacing the whole thing with a
call to `net::IsLocalhost()`.

> My objective here was to being bringing Blink concepts closer to what we use
browser side. I recently noticed many differences around networking and
security. This seemed like a simple first step.

If you want to keep this patch small, then please just replace the `if (m_host
== "localhost")` piece of `SecurityOrigin::isLocalhost()` and add a TODO to pipe
through `net::IsLocalhost`. That looks like the only other place in C++ where
we've hardcoded the name.

[carlosk, 2016-07-18 13:14:02]

Thanks!

https://codereview.chromium.org/2151473002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/NetworkUtils.h (right):

https://codereview.chromium.org/2151473002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/NetworkUtils.h:17: PLATFORM_EXPORT
bool isLocalHostname(const String& host, bool* isLocal6);
On 2016/07/18 12:24:17, Mike West wrote:
> On 2016/07/18 at 09:38:36, carlosk wrote:
> > On 2016/07/14 05:50:17, Mike West wrote:
> > > Should we use this for `SecurityOrigin::isLocalhost()` as well?
> > 
> > Possibly but I didn't have a use for that one in this change so I didn't add
> it.
> 
> I'd suggest changing it. In fact, I'd suggest replacing the whole thing with a
> call to `net::IsLocalhost()`.
> 
> > My objective here was to being bringing Blink concepts closer to what we use
> browser side. I recently noticed many differences around networking and
> security. This seemed like a simple first step.
> 
> If you want to keep this patch small, then please just replace the `if (m_host
> == "localhost")` piece of `SecurityOrigin::isLocalhost()` and add a TODO to
pipe
> through `net::IsLocalhost`. That looks like the only other place in C++ where
> we've hardcoded the name.

Done, the simpler version.

[carlosk, 2016-07-18 13:16:44]

Patchset #3 (id:40001) has been deleted

[carlosk, 2016-07-18 13:22:17]

Running ahead of the trybots mkwst@ found out that that change would break
tests. So I deleted the latest patch set that would change the implementation of
SecurityOrigin::isLocalhost and replaced it with a new one that only adds a TODO
there.

[Mike West, 2016-07-18 13:31:15]

Bah. LGTM.

[carlosk, 2016-07-18 14:05:31]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2016-07-18 14:05:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 15:45:51]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-07-18 15:46:02]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-18 15:48:27]

Description was changed from

==========
Small improvements to MixedContentChecker.

Just a couple of improvements to MixedContentChecker:
- Remove uneeded check for data protocol: it's already included in the protocols
checked in SecurityOrigin::isSecure.
- Improved check for localhost by name to be more complete.

BUG=607878,624275
==========

to

==========
Small improvements to MixedContentChecker.

Just a couple of improvements to MixedContentChecker:
- Remove uneeded check for data protocol: it's already included in the protocols
checked in SecurityOrigin::isSecure.
- Improved check for localhost by name to be more complete.

BUG=607878,624275

Committed: https://crrev.com/c9d34b4a2d425b25bdf90e9bf785cd7132f73037
Cr-Commit-Position: refs/heads/master@{#406000}
==========

[commit-bot: I haz the power, 2016-07-18 15:48:28]

Patchset 3 (id:??) landed as
https://crrev.com/c9d34b4a2d425b25bdf90e9bf785cd7132f73037
Cr-Commit-Position: refs/heads/master@{#406000}