1410364 - opener shouldn't be used when deciding if a webpage is a secure context

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P1)

Description

[Jonathan Kingston [:jkt] he/him]

Given the recent change in the specification: https://github.com/w3c/webappsec-secure-contexts/issues/42 we should follow suit to remove the risk of breaking websites.

We should change our implementation of isSecureContext to our implementation of isSecureContextIfOpenerIgnored and remove the relevant code.

Comment 1

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Always ignore the window opener when calculating secure context

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8925823 [details] [diff] [review]
Always ignore the window opener when calculating secure context

Review of attachment 8925823 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/test/browser/browser_http_autofill.js
@@ +22,2 @@
>  add_task(async function test_http_autofill() {
>    for (let scheme of ["http", "https"]) {

Did you intend to keep this test commented out? We normally don't land commented-out code.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8925823 [details] [diff] [review]
Always ignore the window opener when calculating secure context

>+++ b/testing/web-platform/meta/secure-contexts/basic-popup-and-iframe-tests.html.ini

Please pull in https://github.com/w3c/web-platform-tests/commit/dd83f891b80aa9b05e5fe35d1680e53c6a435b07 instead, since that already exists.  If you just land it on our side as part of your changes, the merge with upstream will be a no-op and everything will be happy without us losing test coverage at any point.

>+++ b/toolkit/components/passwordmgr/test/browser/browser.ini
>+  form_cross_origin_insecure_action.html

This file is not in the diff, nor is it used by anything in the patch.  Why this change?

> +++ b/toolkit/components/passwordmgr/test/browser/browser_http_autofill.js

Please either just remove the commented-out code if it's no longer needed or document clearly why it's commented out.

The code changes look awesome, but r- for the test issues.

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 8925823 [details] [diff] [review]
Always ignore the window opener when calculating secure context

Review of attachment 8925823 [details] [diff] [review]:
-----------------------------------------------------------------

Took a quick skim and it looks good, but I'll wait to r+ the next version bz asked for.

Comment 5

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Pull over the secure context test changes from Mike West

Update the web platform tests with the changes from https://github.com/w3c/web-platform-tests/commit/dd83f891b80aa9b05e5fe35d1680e53c6a435b07?diff=unified

Comment 6

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Always ignore the window opener when calculating secure context

Fixed the issues from comments 2 and 3.

The reason I added form_cross_origin_insecure_action.html to browser.ini is that when I ran the tests locally as a single test, it wasn't copied over, so that portion of the test would fail. I moved it over specifically to under toolkit/components/passwordmgr/test/browser/browser_http_autofill.js so that the dependency is more clear.

try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2287184bc2286c34b70efa26ea5dab6e4001a853

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8928361 [details] [diff] [review]
Always ignore the window opener when calculating secure context

>+support-files =
>+  form_cross_origin_insecure_action.html

Ah, thank you for explaining what's going on here.

This test (browser_http_autofill.js) also needs form_cross_origin_secure_action.html as a support file, right?  Do you mind adding it here as well?

r=me

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8928361 [details] [diff] [review]
Always ignore the window opener when calculating secure context

Review of attachment 8928361 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you very much for cleaning this up Kate!

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8928360 [details] [diff] [review]
Pull over the secure context test changes from Mike West

Review of attachment 8928360 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, great stuff!

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 8928361 [details] [diff] [review]
Always ignore the window opener when calculating secure context

Review of attachment 8928361 [details] [diff] [review]:
-----------------------------------------------------------------

r=dveditz

Comment 11

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Pull over the secure context test changes from Mike West

Comment 12

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Always ignore the window opener when calculating secure context

Rebased for nsGlobalWindow{Inner,Outer} changes
try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d3e975eede12cd20c516003702d47d06b474acc

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8930586 [details] [diff] [review]
Pull over the secure context test changes from Mike West

This already landed by way of bug 1419296.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/681fece780ae
Don't consider opener when calculating IsSecureContext. r=bz, r=dveditz

Comment 15

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 681fece780ae (bug 1410364) for failing in /secure-contexts/basic-popup-and-iframe-tests.html r=backout a=backout on a CLOSED TREE

https://hg.mozilla.org/integration/mozilla-inbound/rev/52ff139184daf909e342f5940805231ab3659ca9

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=636b8219055715091580f3719210b6c941423982&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Comment 16

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Always ignore the window opener when calculating secure context

1) Rebased
2) Remove .ini file for Web Platform Tests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=163f63df2ba0eabae218a5ce6921c5936d22f7b1

Will mark as checkin-needed when tests finish

Comment 17

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Always ignore the window opener when calculating secure context

Rebased to current

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6784a27f54ff
Don't consider opener when calculating IsSecureContext. r=bz, r=dveditz

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6784a27f54ff