Stop blocking 'http://127.0.0.1/' as mixed content.

Stop blocking 'http://127.0.0.1/' as mixed content.

Currently, mixed content checks block http://127.0.0.1 from loading in a
page delivered over TLS. I'm (belatedly) coming around to the idea that
that restriction does more harm than good. In particular, I'll note that
folks are installing new trusted roots and self-signing certs for that
IP address, exposing themselves to additional risk for minimal benefit.
Helpful locally installed software is doing the same, with even more
associated risk.

This patch aligns our mixed content checks with the Secure Contexts
notion of "potentially trustworthy", allowing 'http://127.0.0.1'
accordingly.

BUG=607878
R=estark@chromium.org,rsleevi@chromium.org

Committed: https://crrev.com/130ee686fa00b617bfc001ceb3bb49782da2cb4e
Cr-Commit-Position: refs/heads/master@{#401363}

[Mike West, 2016-04-29 11:46:33]

WDYT, folks?

LayoutTest changes are mostly `s|ws://127.0.0.1|ws://example.test|` and
`s|http://127.0.0.1|http://example.test|`. The unit tests are new, as is
`insecure-localhost-allowed.https.html`.

[estark, 2016-05-02 04:48:46]

Couple nits and questions about the tests that I left here; couple questions
about the proposal that I'll leave in the bug.

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-localhost-allowed.https.html
(right):

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-localhost-allowed.https.html:10:
async_test(t => {
optional nit: maybe add a sanity check that fetching http://example.test in the
same way is blocked

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-sync-post-xhr-allowed-expected.txt
(right):

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-sync-post-xhr-allowed-expected.txt:5:
This test opens a HTTPS window that loads insecure data via XHR. We should
trigger a mixed content callback because the main frame in the window is HTTPS
but now has insecure data.
This description looks out of date now.

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-sync-post-xhr-blocked-expected.txt
(right):

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-sync-post-xhr-blocked-expected.txt:5:
This test opens a HTTPS window that loads insecure data via XHR. We should
trigger a mixed content callback and block the load.
same here, description needs to be updated (or, actually, is this test supposed
to be testing that mixed content is triggered, so it should be updated to
example.test?

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-texttrack-in-main-frame-blocked-expected.txt
(right):

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-texttrack-in-main-frame-blocked-expected.txt:3:
This test opens a window that loads an insecure track. We should trigger a mixed
content callback as the main frame in the window is HTTPS and is running
insecure track.
Same here, looks like either description needs to change or it needs to change
to example.test to actually trigger the mixed content callback.

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-xhr-in-main-frame-expected.txt
(right):

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-xhr-in-main-frame-expected.txt:1:
This test opens a HTTPS window that loads insecure data via XHR. We should
trigger a mixed content callback because the main frame in the window is HTTPS
but now has insecure data.
ditto

[Mike West, 2016-05-04 07:50:13]

Thanks!

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-localhost-allowed.https.html
(right):

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-localhost-allowed.https.html:10:
async_test(t => {
On 2016/05/02 at 04:48:46, estark wrote:
> optional nit: maybe add a sanity check that fetching http://example.test in
the same way is blocked

Done.

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-sync-post-xhr-blocked-expected.txt
(right):

https://codereview.chromium.org/1931063004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-sync-post-xhr-blocked-expected.txt:5:
This test opens a HTTPS window that loads insecure data via XHR. We should
trigger a mixed content callback and block the load.
On 2016/05/02 at 04:48:46, estark wrote:
> same here, description needs to be updated (or, actually, is this test
supposed to be testing that mixed content is triggered, so it should be updated
to example.test?

Yup. I missed localhost. Adding `sed -i''
-e's|http://localhost:|http://example.test:|'` to the mechanical changes.
Thanks!

[estark, 2016-05-05 00:23:52]

Looks good but it looks like there are still some failing tests on the bots. :(
Also, what are you thoughts on this CL in light of the public-webappsec thread
about localhost name resolution? Do you want to go ahead with this and possibly
later roll it back to just 127.0.0.1? I'm a little worried about causing extra
thrash for developers (one day all their mixed content warnings go away and the
next day some of them come back).

[Mike West, 2016-05-05 08:17:25]

On 2016/05/05 at 00:23:52, estark wrote:
> Looks good but it looks like there are still some failing tests on the bots.
:( Also, what are you thoughts on this CL in light of the public-webappsec
thread about localhost name resolution? Do you want to go ahead with this and
possibly later roll it back to just 127.0.0.1? I'm a little worried about
causing extra thrash for developers (one day all their mixed content warnings go
away and the next day some of them come back).

We can drop `localhost` in a separate CL; let's just make sure they both land in
the same release. If we get consensus (and I think we're there?) quickly, I'll
send an intent to ship and try to hit ~52.

[estark, 2016-05-05 20:09:46]

On 2016/05/05 08:17:25, Mike West (OOO until the 6th) wrote:
> On 2016/05/05 at 00:23:52, estark wrote:
> > Looks good but it looks like there are still some failing tests on the bots.
> :( Also, what are you thoughts on this CL in light of the public-webappsec
> thread about localhost name resolution? Do you want to go ahead with this and
> possibly later roll it back to just 127.0.0.1? I'm a little worried about
> causing extra thrash for developers (one day all their mixed content warnings
go
> away and the next day some of them come back).
> 
> We can drop `localhost` in a separate CL; let's just make sure they both land
in
> the same release. If we get consensus (and I think we're there?) quickly, I'll
> send an intent to ship and try to hit ~52.

Ok, lgtm then once the bots are happy. (I spot-checked the mechanical test
changes, lmk if you think closer review would be useful.)

[Ryan Sleevi, 2016-06-10 01:15:47]

Mike: Are you waiting on me? Abandoning this? Having fun doing more exciting
things? :)

[Mike West, 2016-06-21 07:43:19]

On 2016/06/10 at 01:15:47, rsleevi wrote:
> Mike: Are you waiting on me? Abandoning this? Having fun doing more exciting
things? :)

Not waiting on you. I just got busy. :/ I'll land this this week.

[Mike West, 2016-06-22 10:01:30]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-06-22 10:01:30]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org
Link to the patchset: https://codereview.chromium.org/1931063004/#ps80001
(title: "Tests.")

[commit-bot: I haz the power, 2016-06-22 10:01:45]

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

[Mike West, 2016-06-22 18:26:16]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-06-22 18:26:17]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org
Link to the patchset: https://codereview.chromium.org/1931063004/#ps120001
(title: "Ugh.")

[commit-bot: I haz the power, 2016-06-22 18:27:02]

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

[commit-bot: I haz the power, 2016-06-22 18:36:05]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-22 18:39:21]

Description was changed from

==========
Stop blocking 'http://127.0.0.1/' as mixed content.

Currently, mixed content checks block http://127.0.0.1 from loading in a
page delivered over TLS. I'm (belatedly) coming around to the idea that
that restriction does more harm than good. In particular, I'll note that
folks are installing new trusted roots and self-signing certs for that
IP address, exposing themselves to additional risk for minimal benefit.
Helpful locally installed software is doing the same, with even more
associated risk.

This patch aligns our mixed content checks with the Secure Contexts
notion of "potentially trustworthy", allowing 'http://127.0.0.1'
accordingly.

BUG=607878
R=estark@chromium.org,rsleevi@chromium.org
==========

to

==========
Stop blocking 'http://127.0.0.1/' as mixed content.

Currently, mixed content checks block http://127.0.0.1 from loading in a
page delivered over TLS. I'm (belatedly) coming around to the idea that
that restriction does more harm than good. In particular, I'll note that
folks are installing new trusted roots and self-signing certs for that
IP address, exposing themselves to additional risk for minimal benefit.
Helpful locally installed software is doing the same, with even more
associated risk.

This patch aligns our mixed content checks with the Secure Contexts
notion of "potentially trustworthy", allowing 'http://127.0.0.1'
accordingly.

BUG=607878
R=estark@chromium.org,rsleevi@chromium.org

Committed: https://crrev.com/130ee686fa00b617bfc001ceb3bb49782da2cb4e
Cr-Commit-Position: refs/heads/master@{#401363}
==========

[commit-bot: I haz the power, 2016-06-22 18:39:23]

Patchset 7 (id:??) landed as
https://crrev.com/130ee686fa00b617bfc001ceb3bb49782da2cb4e
Cr-Commit-Position: refs/heads/master@{#401363}

[jam, 2016-09-07 00:37:09]

jam@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2016-09-07 00:37:10]

https://codereview.chromium.org/1931063004/diff/120001/chrome/test/data/ssl/p...
File chrome/test/data/ssl/page_displays_insecure_content.html (right):

https://codereview.chromium.org/1931063004/diff/120001/chrome/test/data/ssl/p...
chrome/test/data/ssl/page_displays_insecure_content.html:12: <img id="bad_image"
src="http://does-not-exist.test/ssl/google_files/logo.gif"/>
Is this intended for all the tests that use this file?

i.e. I'm looking at SSLUITest.TestRunsInsecuredContentRandomizeHash which
includes this file. With this change, the JS doesn't run which renders the test
useless.