Closed Bug 1253538 Opened 4 years ago Closed 4 years ago

Window.open() should not reuse an existing window if it's running a different container

Categories

(Core :: DOM: Core & HTML, defect)

47 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(2 files)

Attached patch testSplinter Review
I wrote a test to check what bz suggested in bug 1249224 comment 9. Here the scenario I want to test:

. tabA: userContextId=1 window.name='tab-1'
. tabB: userContextId=2 window.name='tab-2'
. tabA does: window.open(something, 'tab-2');

What I want to see is that we create a new tabC with userContextId=1 and window.name='tab-2'.
In our current code we are actually reusing tabB because it's called 'tab-2' ignoring the fact that its userContextId is different than 1.
Attachment #8726620 - Flags: feedback?(bzbarsky)
Attached patch fix.patchSplinter Review
Attachment #8726629 - Flags: review?(bzbarsky)
Blocks: 1249224
Whiteboard: btpp-active
Comment on attachment 8726620 [details] [diff] [review]
test

I was trying to figure out why this test is failing, even though my manual tests pass.

The answer is that this test is doing the window.open() call from system code.  So when we end up in nsDocShell::ValidateOrigin (called from nsDocShell::CanAccessItem) we hit this code block:

  if (nsContentUtils::GetCurrentJSContext() &&
      nsContentUtils::IsCallerChrome()) {
    return true;
  }

and the targeting is allowed.  If this line in the test:

>+      let w = content.window.open(opts.url, 'tab-2');

is changed to:

      let w = content.window.wrappedJSObject.open(opts.url, 'tab-2');

then the test passes.

So I guess an important question here is: do we want chrome code to be able to trigger linking from one user context to another one (in which case the existing setup is fine), or do we want to forbid that, in which case we probably need the actual patch?

>+    Cu.import("resource://gre/modules/PromiseUtils.jsm");

Why not just use normal Promises?

>+      w.onload = function() { resolve(); }

This is a bit annoying because the test failing manifests as a test timeout, but I guess we can live with that...  

>+      is(browser.contentDocument.nodePrincipal.userContextId, 1, "Tab3 UCI must be 1");

Can we also assert that "browser" is neither "browser1" nor "browser2"?
Attachment #8726620 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8726629 [details] [diff] [review]
fix.patch

r=me assuming we want to prevent the "chrome can allow targeting across user context ids" thing.

That said, it looks like now we're checking that GetIsInIsolatedMozBrowserElement() matches, GetAppId() matches, mUserContextId matches.  The only parts of OriginAttributes::operator== we're not checking are mAddonId and mSignedPkg.  Please file a followup bug to see whether we should be checking those too (and probably needinfo bholley).
Attachment #8726629 - Flags: review?(bzbarsky) → review+
Blocks: 1254103
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.