1328824 - Write a test that about:blocked can load successfully in (mixed-content) frames

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[:Gijs (out for now; he/him)]

See bug 1293476 for context. We should ensure that there are tests for the failure in that bug so it doesn't happen again (either due to mixed-content-blocking or due to other changes).

Note that bug 1195242 might change things here so that a blocking page appears for the toplevel document (to avoid the blocking being in a hidden frame). Unfortunately solving that is not entirely trivial, because trivially 'promoting' the 'bad' frame from a (potentially hidden) iframe to the toplevel document in order to show the about:blocked page changes the URL in the URL bar to the likely-malicious one, which we don't want.

If/when we do fix bug 1195242, the test here should be updated - but we should have a test in the meantime.

Henry, can you own writing a test?

Comment 1

[Henry Chang [:hchang]]

Sure I can! 

So, the test we need for now is to make sure the about:blocked frame can be loaded in the secure context (https). I am wondering if the test case is still needed after fixing Bug 1195242.
The entire page will show about:blocked after Bug 1195242 so the top level document
is "perceptually" redirected. What does the test case for this bug can do? (there is no
iframe to test).

Comment 2

[:Gijs (out for now; he/him)]

(In reply to Henry Chang [:henry][:hchang] from comment #1)
> Sure I can! 
> 
> So, the test we need for now is to make sure the about:blocked frame can be
> loaded in the secure context (https). I am wondering if the test case is
> still needed after fixing Bug 1195242.
> The entire page will show about:blocked after Bug 1195242 so the top level
> document
> is "perceptually" redirected. What does the test case for this bug can do?
> (there is no
> iframe to test).

bug 1195242 would require changes to this test. But AFAIK we have no plans right now (how) to fix bug 1195242, and I think we should write a test for the current behaviour.

Comment 3

[Henry Chang [:hchang]]

I tried to modify the existing MCB test case to test this issue by

1) Create a new iframe which points to "https://itisatrap.org/firefox/its-a-trap.html",
   which is the hard-coded URL that will be blocked. [1]
2) When a site is blocked, a custom event "AboutBlockedLoaded" will be sent by [2].
3) If we can catch the "AboutBlockedLoaded" event, then we do know about:blocked
   is successfully loaded. 

That's my plan. However, I was struggling catching the event sent from blockedSite.xhtml [2].
To the best of my knowledge, the event is sent inside the iframe so we can't receive it
outside if "iframe.src" is cross-origin, which means

iframe.addEventListener
iframe.contentDocument.addEventListener

don't work.

The original use of "AboutBlockedLoaded" is in [3], which is not the iframe case.
I tried wrapping the iframe with SpecialPowers such as

var wrappedIframe = SpecialPowers.wrap(iframe);

then do addEventListener. Unfortunately, they didn't work, too :(

Francois, Gij, do you have any idea about this?

Thanks!


[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/SafeBrowsing.jsm#333
[2] https://dxr.mozilla.org/mozilla-central/rev/b548da4e16f067e5b69349376e37b2db97983cf7/browser/base/content/blockedSite.xhtml#138
[3] https://dxr.mozilla.org/mozilla-central/rev/b548da4e16f067e5b69349376e37b2db97983cf7/browser/components/safebrowsing/content/test/browser_whitelisted.js#39

Comment 4

[Thomas Nguyen (:tnguyen)]

You may have to add bubbles true to make the event work in iframe
var event = new CustomEvent("AboutBlockedLoaded", {bubbles:true});

Comment 5

[:Gijs (out for now; he/him)]

Use a browser mochitest and in a contenttask, add a listener for the custom event on |this| in the contenttask, then load the frame and you should be able to get this event similar to how content.js does for about:neterror:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#267

Comment 6

[Henry Chang [:hchang]]

Thanks Thomas and :Gijs!

I was also thinking using the browser mochitest as my final solution 
if I didn't manage to test in normal mochitest.

BTW, do you know why we are unable to receive the event sent from the iframe 
in the normal mochitest even with SpecialPowers? Or is it able to be done
but just way much easier in browser mochitest?

Thanks!

Comment 7

[:Gijs (out for now; he/him)]

(In reply to Henry Chang [:henry][:hchang] from comment #6)
> BTW, do you know why we are unable to receive the event sent from the iframe 
> in the normal mochitest even with SpecialPowers? Or is it able to be done
> but just way much easier in browser mochitest?

I don't know. You'll need to bubble the event either way, though, as Thomas said. I wonder if that'll let the other unprivileged frames see it. I would hope not, but I'm not sure.

Comment 8

[Henry Chang [:hchang]]

After one day investigation, my current understanding is: 

if an event is sent inside the iframe, the event target list, which is constructed at [1], 
would be (suppose we call dispatchEvent on window.document inside an iframe)

0) nsDocment
1) nsGloblaWindow
2) nsInProcessTabChildGlobal
3) XULElement
4) ...
...
...
18) nsGlobalChromeWindow
19) nsWindowRoot

where nsDocument is the source of the event and nsGloblaWindow is iframe.contentWindow.
nsInProcessTabChildGlobal is a little tricky. It is obtained in [2], where the
|frameElement| is the frame used to load the "top scriptable window" [3] of the
iframe.contentWindow. In other words, the iframe inner event will still bubble cross 
the iframe boundary but only directly jump to the top level window's frame loader owner. 
It explains why we can only receive the event in the browser level.

I was hoping to use SpecialPowers.wrap(window.top.frameElement)
                      .QI(nsIFrameLoaderOwner).frameLoader.ownerElement

to addEventListern but it didn't work. Maybe I am still missing something.
I may try another day to get the iframe inner event in content mochitest.
Will switch to browser chrome test if I failed again ^^"


[1] https://dxr.mozilla.org/mozilla-central/rev/d192a99be4b436f2dc839435319f7630d5d8f4b0/dom/events/EventDispatcher.cpp#766
[2] https://dxr.mozilla.org/mozilla-central/rev/d192a99be4b436f2dc839435319f7630d5d8f4b0/dom/base/nsGlobalWindow.cpp#3245
[3] https://dxr.mozilla.org/mozilla-central/rev/d192a99be4b436f2dc839435319f7630d5d8f4b0/dom/base/nsGlobalWindow.cpp#3242

Comment 9

[Henry Chang [:hchang]]

Attachment: Bug 1328824 - Test if about:blocked can be loaded in a secure context.

Review commit: https://reviewboard.mozilla.org/r/103518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103518/

Comment 10

[Henry Chang [:hchang]]

I still failed to obtain (2) (i.e. nsInProcessTabChildGlobal in non-e10s case) 
in the plain mochitest so I took chrome mochitest. As how it's written in the 
attached patch, the event listener is added to the top level window frame loader, 
it can successfully receive the "AboutBlockedLoaded" event.

Comment 11

[Henry Chang [:hchang]]

Hi Gijs, francois,

I think you both will be interested in reviewing this patch. After a million failures 
of writing this test in plain mochitest, I decided to use a chrome mochitest and open up
a browser, use ContentTask.spawn to inject frame script to do the test so it works very
well. However, the try result shows the test case still suffers from non-local network
request. I don't know the root cause yet (I have turned off the report actually) but
it shouldn't be tough to fix.

Comment 12

[:Gijs (out for now; he/him)]

Comment on attachment 8825357 [details]
Bug 1328824 - Test if about:blocked can be loaded in a secure context.

https://reviewboard.mozilla.org/r/103518/#review104554

::: toolkit/components/url-classifier/tests/mochitest/chrome.ini:27
(Diff revision 1)
>  [test_trackingprotection_whitelist.html]
>  tags = trackingprotection
>  [test_safebrowsing_bug1272239.html]
>  [test_donottrack.html]
>  [test_classifier_changetablepref.html]
> +[test_mcb_meets_safebrowsing.html]

Our naming of tests is very unfortunate. I meant a browser mochitest: https://developer.mozilla.org/en-US/docs/Mozilla/Browser_chrome_tests . You've written a chrome test, but those only run in non-e10s (because XUL in the content process isn't really something we want to support, AIUI). So now we'll never be testing this in e10s.

It should not be hard to write this as a browser mochitest instead - you mostly will get rid of the HTML boilerplate, and you won't need to open a new window and can just create a new tab instead.

Comment 13

[Henry Chang [:hchang]]

Comment on attachment 8825357 [details]
Bug 1328824 - Test if about:blocked can be loaded in a secure context.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103518/diff/1-2/

Comment 14

[Henry Chang [:hchang]]

Finally done the browser mochitest test case! 

In order to avoid hitting non-local network, I had to redirect https://itisatrap.org to localhost. ("http://itisatrap.org" has been done so because 
it's being used in other test cases.)

Comment 15

[:Gijs (out for now; he/him)]

Comment on attachment 8825357 [details]
Bug 1328824 - Test if about:blocked can be loaded in a secure context.

https://reviewboard.mozilla.org/r/103518/#review105200

This looks pretty good. I have some suggestions to tidy it up a bit. See below.

::: browser/components/safebrowsing/content/test/browser_mixedcontent_aboutblocked.js:9
(Diff revision 2)
> +let tabbrowser = null;
> +
> +registerCleanupFunction(function() {
> +  tabbrowser = null;
> +  while (gBrowser.tabs.length > 1) {
> +    gBrowser.removeCurrentTab();
> +  }
> +});

You don't need this, and it might remove the wrong tab.

::: browser/components/safebrowsing/content/test/browser_mixedcontent_aboutblocked.js:21
(Diff revision 2)
> +  let observerService = Cc["@mozilla.org/observer-service;1"]
> +                        .getService(Ci.nsIObserverService);

You can use `Services.obs` throughout.

::: browser/components/safebrowsing/content/test/browser_mixedcontent_aboutblocked.js:33
(Diff revision 2)
> +  observerService.addObserver(function() {
> +    ok(true, "Received internal event!");
> +    callbackOnce();
> +  }, "mozentries-update-finished", false);

This will keep this observer forever and might cause leaks. Please ensure removeObserver is called in callbackOnce. You'll probably need to put the observer function in a local variable so you can reference it.

::: browser/components/safebrowsing/content/test/browser_mixedcontent_aboutblocked.js:45
(Diff revision 2)
> +  let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"]
> +               .getService(Ci.nsIScriptSecurityManager);
> +  let iosvc = Cc["@mozilla.org/network/io-service;1"]
> +              .getService(Ci.nsIIOService);

Services.scriptSecurityManager and Services.io, respectively.

::: browser/components/safebrowsing/content/test/browser_mixedcontent_aboutblocked.js:54
(Diff revision 2)
> +    QueryInterface: function(iid)
> +    {

Nit: brace on same line, here and for the other method.

Nit: just use:
```js
XPCOMUtils.generateQI([Ci.nsIUrlClassifierUpdateObserver])
```

to get a QI method. You don't need to add Ci.nsISupports in the array you pass it.

::: browser/components/safebrowsing/content/test/browser_mixedcontent_aboutblocked.js:56
(Diff revision 2)
> +      if (iid.equals(Ci.nsISupports) ||
> +        iid.equals(Ci.nsIUrlClassifierUpdateObserver))

Nit: indentation.

::: browser/components/safebrowsing/content/test/browser_mixedcontent_aboutblocked.js:71
(Diff revision 2)
> +  let dbService = Cc["@mozilla.org/url-classifier/dbservice;1"]
> +                .getService(Ci.nsIUrlClassifierDBService);

Nit: indentation.

::: browser/components/safebrowsing/content/test/browser_mixedcontent_aboutblocked.js:77
(Diff revision 2)
> +  tabbrowser = gBrowser;
> +  let tab = tabbrowser.selectedTab = tabbrowser.addTab();
> +  let testingBrowser = tab.linkedBrowser;
> +
> +  // Load an secure conatiner page.
> +  yield BrowserTestUtils.loadURI(testingBrowser, SECURE_CONTAINER_URL);
> +  yield BrowserTestUtils.browserLoaded(testingBrowser);

Instead of these lines, use:

```js
let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, SECURE_CONTAINER_URL);
let testingBrowser = tab.linkedBrowser;
```

and then add the end:

```js
yield BrowserTestUtils.removeTab(tab);
```

Alternatively, you can use the `withNewTab` wrapper that takes a generator function and takes care of opening and closing the tab for you:

```js
add_task(function* testNormalBrowsing() {
  yield BrowserTestUtils.withNewTab(SECURE_CONTAINER_URL, function* (browser) {
    // 'browser' is now a reference to the tab's linkedBrowser that you can use with ContentTask.spawn.
  });

::: browser/components/safebrowsing/content/test/browser_mixedcontent_aboutblocked.js:81
(Diff revision 2)
> +add_task(function* testNormalBrowsing() {
> +  tabbrowser = gBrowser;
> +  let tab = tabbrowser.selectedTab = tabbrowser.addTab();
> +  let testingBrowser = tab.linkedBrowser;
> +
> +  // Load an secure conatiner page.

If the comment stays: nit: spelling 'container'.

::: browser/components/safebrowsing/content/test/browser_mixedcontent_aboutblocked.js:98
(Diff revision 2)
> +        removeEventListener('AboutBlockedLoaded', listener, false, true);
> +        resolve();
> +      };
> +      addEventListener('AboutBlockedLoaded', listener, false, true);
> +
> +      // Create an iframw which loads a phish url.

Nit: spelling 'iframe'

Comment 16

[Henry Chang [:hchang]]

Comment on attachment 8825357 [details]
Bug 1328824 - Test if about:blocked can be loaded in a secure context.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103518/diff/2-3/

Comment 17

[Henry Chang [:hchang]]

Hi :Gijs,

Thanks for your review and suggestions! They are all very helpful. Sorry for my unfamiliarity with
browser chrome test. The new patch is addressed all comments from the previous review. Could you
please review again? Thanks!

Comment 18

[:Gijs (out for now; he/him)]

Comment on attachment 8825357 [details]
Bug 1328824 - Test if about:blocked can be loaded in a secure context.

https://reviewboard.mozilla.org/r/103518/#review105656

Two points below, but with that, r=me

::: browser/components/safebrowsing/content/test/browser_mixedcontent_aboutblocked.js:39
(Diff revisions 2 - 3)
> -  const table = "test-phish-simple";
> -
> +  let dbService = Cc["@mozilla.org/url-classifier/dbservice;1"]
> +                .getService(Ci.nsIUrlClassifierDBService);

Indenting is still odd here. Either 2-space indent from the start of the previous line, or align the '.' with the '[' on the previous line.

::: browser/components/safebrowsing/content/test/browser_mixedcontent_aboutblocked.js:49
(Diff revisions 2 - 3)
>          callbackOnce();
>        }
>      },
> -  };
>  
> -  let dbService = Cc["@mozilla.org/url-classifier/dbservice;1"]
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsIUrlClassifierUpdateObserver])

Huh, this is actually wrong (which I assume is wrong in the place you copied it from!) - you're writing a nsIUrlClassifierCallback, not an nsIUrlClassifierUpdateObserver (which doesn't have a handleEvent method). See https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsIUrlClassifierDBService.idl#79-81

Happily, the nsIUrlClassifierCallback interface has the `function` keyword so you don't need an object at all. So just do:

```js
dbService.lookup(principal, PHISH_TABLE, value => {
  if (value == PHISH_TABLE) {
    ok(true, "DB lookup success!");
    callbackOnce();
  }
});
```

Comment 19

[Henry Chang [:hchang]]

Comment on attachment 8825357 [details]
Bug 1328824 - Test if about:blocked can be loaded in a secure context.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103518/diff/3-4/

Comment 20

[Henry Chang [:hchang]]

Try result:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82da0280021f518c1f79542f7cdddd3f6470f967&selectedJob=69460447

The test case is still suffered from the following error [1]:

//////////////
FATAL ERROR: Non-local network connections are disabled and a connection attempt to itisatrap.org (63.245.213.12) was made.
////////////// 

However, I have added "https://itisatrao.org:443" to server-locations.txt [2]. I am pretty sure
"https://itisatrao.org:443" is redirected when I run the test case locally.




[1] https://public-artifacts.taskcluster.net/T25cIKsHTlOUr4VOLdNehw/0/public/logs/live_backing.log
[2] https://hg.mozilla.org/try/rev/349b0260c8d872480518ee7f8317f2b649d1166d#l5.34

Comment 21

[:Gijs (out for now; he/him)]

(In reply to Henry Chang [:henry][:hchang] from comment #20)
> Try result:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=82da0280021f518c1f79542f7cdddd3f6470f967&selectedJob=6
> 9460447
> 
> The test case is still suffered from the following error [1]:
> 
> //////////////
> FATAL ERROR: Non-local network connections are disabled and a connection
> attempt to itisatrap.org (63.245.213.12) was made.
> ////////////// 
> 
> However, I have added "https://itisatrao.org:443" to server-locations.txt
> [2]. I am pretty sure
> "https://itisatrao.org:443" is redirected when I run the test case locally.
> 
> 
> 
> 
> [1]
> https://public-artifacts.taskcluster.net/T25cIKsHTlOUr4VOLdNehw/0/public/
> logs/live_backing.log
> [2]
> https://hg.mozilla.org/try/rev/349b0260c8d872480518ee7f8317f2b649d1166d#l5.34

I wasted a lot of time on this until Ted helpfully pointed out that your server-locations entry has "www." and your test URL does not. Add "www." to PHISH_URL in the test and the problem goes away.

If I remove the fix, I can reproduce this locally when using --run-until-failure with the mochitest invocation for this specific testfile. You should be able to verify that change locally that way and then push to autoland.

Comment 22

[Henry Chang [:hchang]]

(In reply to :Gijs from comment #21)
> (In reply to Henry Chang [:henry][:hchang] from comment #20)
> > Try result:
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=82da0280021f518c1f79542f7cdddd3f6470f967&selectedJob=6
> > 9460447
> > 
> > The test case is still suffered from the following error [1]:
> > 
> > //////////////
> > FATAL ERROR: Non-local network connections are disabled and a connection
> > attempt to itisatrap.org (63.245.213.12) was made.
> > ////////////// 
> > 
> > However, I have added "https://itisatrao.org:443" to server-locations.txt
> > [2]. I am pretty sure
> > "https://itisatrao.org:443" is redirected when I run the test case locally.
> > 
> > 
> > 
> > 
> > [1]
> > https://public-artifacts.taskcluster.net/T25cIKsHTlOUr4VOLdNehw/0/public/
> > logs/live_backing.log
> > [2]
> > https://hg.mozilla.org/try/rev/349b0260c8d872480518ee7f8317f2b649d1166d#l5.34
> 
> I wasted a lot of time on this until Ted helpfully pointed out that your
> server-locations entry has "www." and your test URL does not. Add "www." to
> PHISH_URL in the test and the problem goes away.
> 
> If I remove the fix, I can reproduce this locally when using
> --run-until-failure with the mochitest invocation for this specific
> testfile. You should be able to verify that change locally that way and then
> push to autoland.

Oh my god how stupid I am :(

Really thank you and Ted! I am almost gonna blame that the try server
is caching old server location :p I believe I can finally land this case
tomorrow!

Comment 23

[Henry Chang [:hchang]]

Comment on attachment 8825357 [details]
Bug 1328824 - Test if about:blocked can be loaded in a secure context.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103518/diff/4-5/

Comment 24

[Henry Chang [:hchang]]

Comment on attachment 8825357 [details]
Bug 1328824 - Test if about:blocked can be loaded in a secure context.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103518/diff/5-6/

Comment 25

[Henry Chang [:hchang]]

Sadly I've got another lint error [1][2] for the following line

yield new Promise(resolve => waitForDBInit(resolve));

After a long while try-and-error, I finally found it's just because "yield" is called
in a non-generating function. Already fixed it and I will definitely land it today!

[1] https://public-artifacts.taskcluster.net/a3XHdFVFQZKy1hnrhDrsMw/0/public/logs/live_backing.log
[2] [task 2017-01-18T02:06:50.449416Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/safebrowsing/content/test/browser_mixedcontent_aboutblocked.js:54:11 | Parsing error: Unexpected token new (eslint)

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10f82a72aa02
Test if about:blocked can be loaded in a secure context. r=Gijs

Comment 27

[Iris Hsiao [:ihsiao]]

sorry had to back out for eslint failure, https://treeherder.mozilla.org/logviewer.html#?job_id=69891771&repo=autoland&lineNumber=8369

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b84c8830194f
Backed out changeset 10f82a72aa02 for eslint failure

Comment 29

[Henry Chang [:hchang]]

I wonder why try build eslint test didn't complain those errors. Neither from my local eslint test. 

:Gijs, do you have any idea that why try server uses maybe different eslint config from
other taskcluster machine uses?

Comment 30

[Ryan VanderMeulen [:RyanVM]]

That rule change landed very recently (bug 1331661). I'd bet your Try push was just on top of a parent that was prior to it landing.

Comment 31

[Henry Chang [:hchang]]

Thanks Ryan! Have fixed it and pushed to mi :)

Comment 32

[Ryan VanderMeulen [:RyanVM]]

No problem, thanks for taking this on!

Comment 33

[:Gijs (out for now; he/him)]

(In reply to Henry Chang [:henry][:hchang] from comment #31)
> Thanks Ryan! Have fixed it and pushed to mi :)

For some reason no pulsebot:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b83bc5087ec7dbd464ac5a1c0efd2f9b7e1b89d

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b83bc5087ec