Closed Bug 1249224 Opened 4 years ago Closed 4 years ago

window.open() should open a new window in the same container

Categories

(Core :: Security, 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

(Keywords: addon-compat)

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1245124 +++

Bug 1245124 fixes a similar issue but when the new content is opened in a new tab. When window.open() shows the content in a new window, we don't follow the same container policy yet.
Is this relating to the browser.link.open_newwindow;2 pref? Which can also be changed under about:preferences#general?

I'm seeing the same behaviour as I did in bug # 1245124, STR:

* login into gmail/bugzilla via the personal container
* login into gmail via the work container
* change browser.link.open_newwindow;2 (or using about:preferences#general)
* click on a bugzilla link inside the gmail instance from the personal container
* a new window appears with you being logged into bugzilla
* type in gmail.com into the new window, and you'll be logged into the same account you used via the personal container

This will open a new window with you being logged into bugzilla. If you load gmail, you'll be logged into the same account you've logged into via the personal container. I'm assuming this new window is also a personal container and is just missing the UI indicators.

Maybe I'm looking at this incorrectly?
baku, is this still a bug?
Flags: needinfo?(amarchesini)
Yes, UI indicators are missing.
Flags: needinfo?(amarchesini)
This is half of the work. With this patch we propagate the userContextId correctly to the child window but we still don't display the container UI.
Attachment #8725633 - Flags: review?(bzbarsky)
Comment on attachment 8725633 [details] [diff] [review]
patch 1 - propagate the userContextId

Do we really want to do this even if !windowIsNew?  Seems a bit fishy...
Flags: needinfo?(amarchesini)
Attached patch bugzilla2.patch (obsolete) — Splinter Review
Attachment #8725633 - Attachment is obsolete: true
Attachment #8725633 - Flags: review?(bzbarsky)
Flags: needinfo?(amarchesini)
Attachment #8725759 - Flags: review?(bzbarsky)
That didn't answer my question.  Do we want to propagate this state if the window is not new?  Or can we assert that we're already in the same container in that case or something?
Flags: needinfo?(amarchesini)
Attachment #8725759 - Flags: review?(bzbarsky)
if we 'reuse' windows, yes. I don't know docShell enough to answer this question.
What I want is that, all the time a window is used for loading a content, we force the userContextId from the parent docShell.
Flags: needinfo?(amarchesini) → needinfo?(bzbarsky)
So per IRC discussion, I think this is what we should do:

1)  Ensure that looking for existing windows by name considers user context ids when deciding whether to allow things to be found.  Maybe we're already doing that, because OriginAttributes::operator== considers it, so principal subsumes checks do, so principal Equals() checks do, so nsDocShell::CanAccessItem does via ValidateOrigin.  we should write some tests for this situation (e.g. opening two tabs with the same origin but different user context ids, having one of them give itself a nonempty window.name, having the other try to target that name with a link and via window.open and ensuring that those don't find the tab with the wrong user context id, then making sure that it does work if they have the same user context id).

2)  When we call nsIWindowProvider::ProvideWindow, if the return value does not have windowIsNew set we want to check whether it's already got the user context id we want (if any) and if not we want to ignore that return value (pretend that null was returned) and go on to open a new toplevel window.  In practice, we expect our in-tree window providers to not screw this up and either return something with the same user context id or return a new window, so this is really just belt-and-suspenders for cases when some extension installs a window provider that does something weird.

3)  At the point where this patch is adding code, we check for windowIsNew.  If that's true, we set the user context id as this patch does.  If it's false, we assert that the window we got already has the right user context id.
Flags: needinfo?(bzbarsky)
Depends on: 1253538
Attached patch bugzilla2.patchSplinter Review
Attachment #8725759 - Attachment is obsolete: true
Attachment #8726661 - Flags: review?(bzbarsky)
Comment on attachment 8726661 [details] [diff] [review]
bugzilla2.patch

>+  if (windowIsNew &&subjectPrincipal) {

Space after "&&" please.

Shouldn't this block come before we dispatch toplevel-window-ready?  Seems like it should.

r=me with that.
Attachment #8726661 - Flags: review?(bzbarsky) → review+
Depends on: 1254103
Backed out for bustage in https://hg.mozilla.org/integration/mozilla-inbound/rev/487c401fb823

baku on IRC:
> baku: ah no. everything changed!
> baku: SetUserContextId doesn't exist anymore.
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/ed7f68038724
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(In reply to Boris Zbarsky [:bz] from comment #9)
> 1)  Ensure that looking for existing windows by name considers user context
> ids when deciding whether to allow things to be found.  Maybe we're already
> doing that, because OriginAttributes::operator== considers it, so principal
> subsumes checks do, so principal Equals() checks do, so
> nsDocShell::CanAccessItem does via ValidateOrigin.  we should write some
> tests for this situation (e.g. opening two tabs with the same origin but
> different user context ids, having one of them give itself a nonempty
> window.name, having the other try to target that name with a link and via
> window.open and ensuring that those don't find the tab with the wrong user
> context id, then making sure that it does work if they have the same user
> context id).
> 
Hi Baku,
Did you write the tests mentioned by Boris above?
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
Duplicate of this bug: 1238766
You need to log in before you can comment on or make changes to this bug.