1115333 - Make nsGlobalWindow's window.opener code on a chrome window return null if the opener is non-privileged/content

Status: RESOLVED FIXED

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

Description

[:Gijs (he/him)]

As best I can tell this means fixing the code here:

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp?rev=b7eb1ce0237d&mark=4458-4460#4447

to return nullptr instead if the opener is a content window, and |this| is not.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Discussion in bug 1096319 comment 18 is what led to this being filed, that bug has more context.

Smaug, do you think this is feasible?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

For some reason I can't see bug 1096319.

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #2)
> For some reason I can't see bug 1096319.

I added you to the CC list.

Comment 4

[:Gijs (he/him)]

Attachment: don't return window.opener if we're chrome and the opener is not,

Something like this?

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e00e019007f0

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8544243 [details] [diff] [review]
don't return window.opener if we're chrome and the opener is not,

>+  nsGlobalWindow *win = static_cast<nsGlobalWindow *>(opener.get());
nsGlobalWindow* win = static_cast<nsGlobalWindow*>(opener.get());

>+
>   // First, check if we were called from a privileged chrome script
>   if (nsContentUtils::IsCallerChrome()) {
>+    // Then, check we're not ourselves chrome, with the opener being content:
Tiny bit odd comment since we're actually checking whether we are chrome.

Comment 6

[:Gijs (he/him)]

The test runs here show that it's not this simple, unfortunately... Gavin/Olli, do you think we should find some way to bypass this for the chrome-privileged code that really wants this (window.unsafeOpener or something) and adjust the tests? Or wontfix this and move on?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

unsafeOpener or originalOpener sounds ok to me. Adding such attribute to ChromeWindow should be rather easy.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Sounds good to me too.

Comment 9

[:Gijs (he/him)]

Making me not forget this.

Comment 11

[:Gijs (he/him)]

Attachment: don't return window.opener if we're chrome and the opener is not,

I don't like this one bit. But it seems that the mochitest chrome docs don't count as chrome windows (IsChromeWindow() returns false for them) and so this code considers those openers content, while the resulting window.open, because the callers are chrome (I guess?) create chrome global windows, which then upset this logic. Rather than fixing every single chrome test that opened a window ever, I added a pref. It feels yucky. Olli, do you have a better idea?

Comment 12

[:Gijs (he/him)]

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

Comment 13

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #11)
> Created attachment 8555543 [details] [diff] [review]
> don't return window.opener if we're chrome and the opener is not,
> 
> I don't like this one bit. But it seems that the mochitest chrome docs don't
> count as chrome windows (IsChromeWindow() returns false for them) and so
> this code considers those openers content, while the resulting window.open,
> because the callers are chrome (I guess?) create chrome global windows,
> which then upset this logic. Rather than fixing every single chrome test
> that opened a window ever, I added a pref. It feels yucky. Olli, do you have
> a better idea?

One alternative here is verifying that the opener window is *both* not a chrome window *and* doesn't have the system principal, before forcibly returning nullptr instead of the opener window (IOW, allowing access to non-chromewindow system-principalled windows, as well as non-system-principalled chrome windows (which I don't think exist, but hey)). I don't know if that makes more sense than the stricter check I'm doing now (either a non-chrome window or a non-privileged window returns nullptr instead of the window). Bobby, can you comment as to the implications?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I'm lost here now. Why we want both pref and unsafeOpener. This is getting rather complicated.
What if unsafeOpener was a [ChromeOnly] attribute in Window?

Could we just rely on principals here and not IsChromeWindow() at all?
So, if window has chrome principal, and its opener doesn't have, return null.

mochitest chrome docs are in type=content <xul:browser> so they get normal Window object.

Comment 15

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #14)
> I'm lost here now. Why we want both pref and unsafeOpener. This is getting
> rather complicated.

unsafeOpener because it seems like there are legitimate reasons why chrome might want the content window.

the pref because I don't think updating every single mochitest-chrome test that opens new windows (s/opener/unsafeOpener/) is a good idea.

> What if unsafeOpener was a [ChromeOnly] attribute in Window?

That wouldn't solve the second problem of all those mochitest-chrome tests.

> Could we just rely on principals here and not IsChromeWindow() at all?
> So, if window has chrome principal, and its opener doesn't have, return null.

We could, I think. I hope Bobby can confirm this, as it would certainly make things a lot simpler.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gijs Kruitbosch from comment #15)
> unsafeOpener because it seems like there are legitimate reasons why chrome
> might want the content window.

Do you have some examples? I can't really think of any.

> We could, I think. I hope Bobby can confirm this, as it would certainly make
> things a lot simpler.

This sounds good.

Comment 17

[:Gijs (he/him)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #16)
> (In reply to :Gijs Kruitbosch from comment #15)
> > unsafeOpener because it seems like there are legitimate reasons why chrome
> > might want the content window.
> 
> Do you have some examples? I can't really think of any.

Considering we treat file open dialogs (when shown from [browse...] buttons in <input type=file>) as popups, I was expecting issues there. I've not actually tested whether this change breaks them, and probably should (I was assuming I'd see test failures, but it appears not).

The other example I was thinking about is the login manager stuff (see also earlier discussion in bug 1096319), which shows the doorhanger for password saving/updating on the parent of chromeless popups (because the popups tend to go away). However, AFAICT that accesses window.opener where |window| is already a content window, so that should continue to work.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8555543 [details] [diff] [review]
don't return window.opener if we're chrome and the opener is not,

So I think we should try to not take this rather complicated approach.

principal checks and then possibly [ChromeOnly] unsafeOpener if really needed.

Comment 19

[Bobby Holley (:bholley)]

I'm somewhat worried about spoofability with window principal checks, since the page could be navigated back and forth. But given that this is defense-in-depth anyway, that seems like a reasonable thing to do.

Comment 20

[:Gijs (he/him)]

Attachment: don't return window.opener if we're chrome and the opener is not,

Gavin, you wrote the password manager tests here... I think these replacement checks adequately ensure that this continues to be a modal dialog parented to the right content window, and then we don't need to bother implementing/exposing unsafeWindow at all, AFAICT. Try push will follow shortly.

Comment 21

[:Gijs (he/him)]

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

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8557174 [details] [diff] [review]
don't return window.opener if we're chrome and the opener is not,


>+  nsGlobalWindow *win = static_cast<nsGlobalWindow *>(opener.get());
nsGlobalWindow* win = static_cast<nsGlobalWindow*>(opener.get());

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8557174 [details] [diff] [review]
don't return window.opener if we're chrome and the opener is not,

I really don't recall any details here, unfortunately...

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 8557174 [details] [diff] [review]
don't return window.opener if we're chrome and the opener is not,

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

::: toolkit/components/passwordmgr/test/test_xhr.html
@@ +66,5 @@
>      case 2:
>          is(username, "xhruser2", "Checking provided username");
>          is(password, "xhrpass2", "Checking provided password");
>  
> +        // Check that the dialog is modal, chrome and dependent; 

Nit: trailing whitespace (in both tests)

@@ +158,5 @@
>          
>      case 2:
>          // Test correct parenting, by opening another window and
>          // making sure the prompt's opener is correct
> +        // NB: this actually opens a new tab in the same window

How about fixing the comment to more accurately describe the test now? e.g. s/window/tab/ and noting that we're checking that the modal dialog causes the selectedTab to change to the one with the XHR.

Comment 25

[:Gijs (he/him)]

Thanks everyone! With nits:

remote:   https://hg.mozilla.org/integration/fx-team/rev/7c9cbe8b347d

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c9cbe8b347d