1321020 - When you open a new file content tab from the file content process the wrong remote type gets set.

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

If you open a new file:// (or data: URI) content tab using window.open or link from the file content process then the wrong remote type gets set on the new browser.
This is because the URI is null, so the web remote type gets used.

The correct process is used, because of TabParent::AutoUseNewTab being used, but if you then try to navigate to a normal web content page in that tab it loops, because it thinks it doesn't need to switch the remote type.

Thanks Henry for finding this and these STR:

1) Open a file content, which contains a <a href="data:xxxx" target="_blank">
2) Follow the <a> tag to load the data URI in a new tab.
3) Type "example.com" in the data URI tab navigation bar.
4) The data URI tab will not be able to load example.com

Comment 1

[Bob Owen (:bobowen)]

Attachment: Ensure that a new tab opened from content with non-default remote type, gets correct remote type

Comment 2

[Bob Owen (:bobowen)]

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

Comment 3

[:Gijs (he/him)]

Comment on attachment 8815411 [details] [diff] [review]
Ensure that a new tab opened from content with non-default remote type, gets correct remote type

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

Can we create a test for this? Also, is using the referrer possible on links with e.g. noreferrer (or similar things set via CSP / meta tag) ?

Comment 4

[Bob Owen (:bobowen)]

(In reply to :Gijs Kruitbosch from comment #3)
> Comment on attachment 8815411 [details] [diff] [review]
> Ensure that a new tab opened from content with non-default remote type, gets
> correct remote type
> 
> Review of attachment 8815411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can we create a test for this? Also, is using the referrer possible on links
> with e.g. noreferrer (or similar things set via CSP / meta tag) ?

The noreferrer side of things is controlled by params.noReferrer, further down.
We still pass through the referrer to this point though, as far as I can see.

Looking to see if I can work out how to test this.

Comment 5

[:Gijs (he/him)]

(In reply to Bob Owen (:bobowen) from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > Comment on attachment 8815411 [details] [diff] [review]
> > Ensure that a new tab opened from content with non-default remote type, gets
> > correct remote type
> > 
> > Review of attachment 8815411 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Can we create a test for this? Also, is using the referrer possible on links
> > with e.g. noreferrer (or similar things set via CSP / meta tag) ?
> 
> The noreferrer side of things is controlled by params.noReferrer, further
> down.
> We still pass through the referrer to this point though, as far as I can see.
> 
> Looking to see if I can work out how to test this.

The referrer tests in browser/base/content/test/referrer/ should provide a useful template. You should be able to use something like:

yield ContentTask.spawn(refToBrowserForFileTab, null, function*() {
  // run some chrome JS in the content process to verify we're running in the correct process.
});

to check that we're using the right process type. I'm assuming we expose the process type to JS somehow... though I guess I can't see anything for that in bug 1147911?

Comment 6

[Bob Owen (:bobowen)]

Attachment: Part 1: Ensure that a new tab opened from content with non-default remote type, gets correct remote type

Comment 7

[Bob Owen (:bobowen)]

Attachment: Part 2: Test that a file:// URI window opened from a file:// page can be navigated to web content

Finally got round to writing this test.
Although having just run the other ones in the directory, I'm fairly sure this is in the wrong place ... where do you think it should go?

Comment 8

[:Gijs (he/him)]

Comment on attachment 8821105 [details] [diff] [review]
Part 1: Ensure that a new tab opened from content with non-default remote type, gets correct remote type

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

::: browser/base/content/tabbrowser.xml
@@ +2227,5 @@
>              this.tabContainer.updateVisibility();
>  
> +            // If URI is about:blank and we don't have a preferred remote type,
> +            // then we need to use the referrer, if we have one, to get the
> +            // correct remote type for the new tab.

Does this work on links with referrers disabled, and/or can we ensure we have a non-empty preferred remote type here in these situations (ie pass something from the caller) ?

Comment 9

[:Gijs (he/him)]

Comment on attachment 8821110 [details] [diff] [review]
Part 2: Test that a file:// URI window opened from a file:// page can be navigated to web content

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

Re: where it should go: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/tabs/

I feel bad clearing review here, but I'm not sure I understand why we're testing the view-source case by using http: links...

::: browser/base/content/test/newtab/browser_newtab_file_navigated_to_web.js
@@ +1,4 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

Nit: don't need this for public domain ( https://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public-domain/ )

@@ +9,5 @@
> +  let dir = getChromeDir(getResolvedURI(gTestPath));
> +  dir.append(TEST_FILE);
> +  let uri = Services.io.newFileURI(dir);
> +
> +  // Note: added tabs are automatically removed by head.js cleanup function.

Moving the test probably breaks this (different head.js), and it's good practice to remove them in the test that adds them anyway. :-)

@@ +16,5 @@
> +  let numTabsBeforeOpen = gBrowser.tabs.length;
> +
> +  let windowOpened = yield ContentTask.spawn(browser, null, function* () {
> +    try {
> +      content.open(content.location.href + "?opened", "_blank");

Nit: might as well put the added page in a const somewhere, and pass it here as the argument (second arg s/null/MY_URL_CONST, the function*() will then get passed the IPC-cloned copy of the arg).

@@ +19,5 @@
> +    try {
> +      content.open(content.location.href + "?opened", "_blank");
> +      return true;
> +    } catch (e) {
> +      return false;

Under what circumstances does this throw? That is, do we really need the try...catch?

@@ +28,5 @@
> +  is(gBrowser.tabs.length, numTabsBeforeOpen + 1,
> +     "Check we have the correct number of tabs");
> +
> +  let openedTab = gBrowser.tabs[numTabsBeforeOpen];
> +  let openedBrowser = openedTab.linkedBrowser;

Rather than this, you could use BrowserTestUtils.waitForNewTab(gBrowser, url) (returns a promise that resolves with the tab that gets opened).

@@ +35,5 @@
> +  is(href, uri.spec + "?opened", "Check that opened file URL page has loaded");
> +
> +  openedBrowser.loadURI("http://example.org/");
> +  href = yield BrowserTestUtils.browserLoaded(openedBrowser);
> +  is(href, "http://example.org/", "Check that http page has loaded");

Is this like a bonus test that swapping it to an http page works? :-)

I would have expected view-source:file:... to be in the file: process as well...

::: browser/base/content/test/newtab/file_navigated_to_web.html
@@ +1,1 @@
> +<!doctype html>

Nit nit: as it doesn't matter what's in here, please hg cp dummy_page.html from browser/base/content/test/general and add it to the support-files annotation in browser.ini's default section (in the dir I mentioned earlier).

Comment 10

[:Gijs (he/him)]

Comment on attachment 8821105 [details] [diff] [review]
Part 1: Ensure that a new tab opened from content with non-default remote type, gets correct remote type

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

Oh, wow, we (a) had this discussion already, and (b) I thought this was bug 1324912. :-|

Very sorry. r=me!

Comment 11

[:Gijs (he/him)]

Comment on attachment 8821110 [details] [diff] [review]
Part 2: Test that a file:// URI window opened from a file:// page can be navigated to web content

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

Most of my nits, and suggestion where to put this, still stand, but with that r=me

Note that to remove tabs, you probably want:

yield BrowserTestUtils.removeTab(tab);

Comment 13

[Bob Owen (:bobowen)]

Attachment: Part 2: Test that a file:// URI window opened from a file:// page can be navigated to web content

r=gijs from comment 9, with issues addressed.

Thanks for all the test tips gijs.

Try push (before I fixed that lint issue):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aaac86bdc6dc756dde1042ee62346346c6099fc

(In reply to :Gijs Kruitbosch from comment #9)
> Comment on attachment 8821110 [details] [diff] [review]

> Re: where it should go:
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> tabs/

Done.
 
> @@ +9,5 @@
> > +  let dir = getChromeDir(getResolvedURI(gTestPath));
> > +  dir.append(TEST_FILE);
> > +  let uri = Services.io.newFileURI(dir);
> > +
> > +  // Note: added tabs are automatically removed by head.js cleanup function.
> 
> Moving the test probably breaks this (different head.js), and it's good
> practice to remove them in the test that adds them anyway. :-)

Done - I hope I'm using it correctly.
 
> @@ +28,5 @@
> > +  is(gBrowser.tabs.length, numTabsBeforeOpen + 1,
> > +     "Check we have the correct number of tabs");
> > +
> > +  let openedTab = gBrowser.tabs[numTabsBeforeOpen];
> > +  let openedBrowser = openedTab.linkedBrowser;
> 
> Rather than this, you could use BrowserTestUtils.waitForNewTab(gBrowser,
> url) (returns a promise that resolves with the tab that gets opened).

Nice, thanks.
Also got rid of all the other testing around window.open, I'd added that before I had a way to test for the tab opening, but this covers it all by the looks of it.
 
> ::: browser/base/content/test/newtab/file_navigated_to_web.html
> @@ +1,1 @@
> > +<!doctype html>
> 
> Nit nit: as it doesn't matter what's in here, please hg cp dummy_page.html
> from browser/base/content/test/general and add it to the support-files
> annotation in browser.ini's default section (in the dir I mentioned earlier).

Done.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b50562f277da
Part 1: Ensure that a new tab opened from content with non-default remote type, gets correct remote type. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/94dfb2c98d9c
Part 2: Test that a file:// URI window opened from a file:// page can be navigated to web content. r=gijs

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b50562f277da
https://hg.mozilla.org/mozilla-central/rev/94dfb2c98d9c

Comment 16

[Simona Badau, Desktop QA]

Reproduced using Nightly 53.0a1 (Build ID: 20161126030207) on Windows 10 x64.

Verified as fixed using the latest Nightly 54.0a1 (Build ID: 20170227030203) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.

I do have a question related to the way the local files get their process number. For instance, the process number of the local file that is opened in step 1 is the same with the process number of the local file from step 2, even if both local files are opened in different tabs. 

Bob, is this the expected behavior in this case?

Comment 17

[Bob Owen (:bobowen)]

(In reply to Simona B [:simonab ] from comment #16)
> Reproduced using Nightly 53.0a1 (Build ID: 20161126030207) on Windows 10 x64.
> 
> Verified as fixed using the latest Nightly 54.0a1 (Build ID: 20170227030203)
> on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.
> 
> I do have a question related to the way the local files get their process
> number. For instance, the process number of the local file that is opened in
> step 1 is the same with the process number of the local file from step 2,
> even if both local files are opened in different tabs. 
> 
> Bob, is this the expected behavior in this case?

Yes, the current default is to just have one file content process, similar to how we only have one normal content process.

You can configure this using: dom.ipc.processCount.web
Although the tabs can still end up in the same process if they are related in some way (one opened from the other for example).

Comment 18

[Simona Badau, Desktop QA]

Based on Comments 16 and 17, setting the tracking flag for firefox54 to verified.