Fix #313: make disowning disown the opener browsing context too

[annevk]

Otherwise window names would still leak across.

[annevk]

@bzbarsky could you please review this?

[bzbarsky]

I would like to see changes like this come with web platform tests, and ideally the test results in various browsers. Especially because it's not immediately obvious which observable behavior is affected. I know link targeting is; is anything else?

On the change itself, I think it would be better to just make it be a toplevel browsing context instead of indistinguishable from one, unless we have carefully audited the spec to make sure it never simply switches on the browsing context type.

[mikewest]

Should the window still be "script closable"? (https://html.spec.whatwg.org/#script-closable)

Should we unset the browsing context's name?

[bzbarsky]

Right, that was the next question I was about to ask. In Gecko, setting opener to null makes navigation targeting cross-origin impossible, but window.close (in the window, or in the caller if the window is still same-origin) will still work afaict.

That's why we need tests and a clear idea of which spec sections are affected.

[annevk]

https://html.spec.whatwg.org/#traverse-the-history step 4.2.2 branches on auxiliary browsing context without a corresponding check to the opener browsing context. I can't really figure out how that would have an effect.

I guess another thing that would be worth clarifying here is how this relates to creator browsing contexts. Perhaps the origin still needs to be an alias and referrer still needs to be a copy.

As for tests, I don't think we really have good multi-window testing infrastructure. @jgraham?

[jgraham]

So, the problem is that we window.open a window and then set the opener to null? I think we can write tests for that scenario as long as there's some way to communicate between the windows e.g. via postMessage or localStorage or whatever. It could be a bit annoying if the window ends up not being closed but possibly that's avoidable in any case.

[annevk]

One problem I think is that it may require a user gesture for the invocation to succeed. They could communicate with BroadcastChannel or localStorage I suspect.

[mikewest]

@jgraham: Part of the point of the change is to remove the explicit communication channels. :) As @annevk notes, there are probably side channels we could exploit for a same-origin window. I'm not sure how we'd approach it for a cross-origin window.

@annevk: You're correct that (at least in Chrome) window.open requires a user gesture.

[bzbarsky]

We should at least create some manual tests or something. I mean, we need some conformance tests here..

[zcorpan]

Manual tests is OK (when automated is not possible). One day manual tests in wpt can be automated with WebDriver.

[annevk]

According to @Ms2ger and @jgraham wpt requires disabling the popup blocker so we can in fact test this automatically.

It would still be good to know what we want the results to be though. E.g., should document.cookie continue to leak for about:blank?

[annevk]

I tried to address some of the questions in #313 (comment). That also contains a few more demos.

[annevk]

Progress on tests:

1. I created a basic test to figure out how to make this work and it already fails in Gecko: Simple rel=noreferrer test web-platform-tests/wpt#2329. Not sure what is going on.
2. Testing cross-origin seems impossible without manual tests since there is no direct communication channel. Though perhaps using document.cookie will work combined with timers? Seems cumbersome and like the kind of test that gets disabled over time.

If the basic test is just illustrating a simple bug in Gecko it seems fairly easy to use that kind of framework to create multi-window tests as well. To ensure two windows have been opened with the same name rather than one that is being reused and such.

[mikewest]

Cookies or storage events should work as long as the windows are same-origin. I'll take a look at the PR to see how it reacts in Chrome...

I don't think we have a reasonable solution for cross-origin windows. That's somewhat intentional. :)

I think manual tests are the right way to validate this behavior. Browsers can add more intrusive tests in a variety of ways to ensure that the behavior doesn't regress (e.g. https://codereview.chromium.org/1443663003), and the manual tests can verify that those "magical" tests are verifying the right things.

[bzbarsky]

Note that if we decide we really care about automating cross-origin stuff we could require that in the testing framework firing of some particular event causes the browser to take some particular action that allows (async) communication as needed. Or something.

[annevk]

I created more tests:

• Reusing a window name for a same-origin rel=noreferrer hyperlink web-platform-tests/wpt#2344
• rel=noreferrer cross-origin manual tests web-platform-tests/wpt#2354

It seems window.name should continue to echo whatever it was set to by target. It seems window.close() should continue to function.

Given this I still think my proposed change here is correct.

[mikewest]

For example, I believe that in Gecko if A opens B and B opens C and they are all different origins then A is familiar with B and B is familiar with C but A is not familiar with C, whereas per this spec text it sounds to me like A would be familiar with C.

Chrome agrees with Firefox's behavior in this regard, and doesn't allow A to target C. See web-platform-tests/wpt#2363.

[mikewest]

Even if we ignore disowning, if A, origin 1, opens B, origin 2, which has a child C, origin 1, which has a child D (whatever origin), then A can target D. I guess per item 1 it can also target C... Do browsers actually do that??

I don't think we want the behavior that A can target B's nested browsing contexts, same-origin or cross-origin. That seems to be ripe for various kinds of phishing attacks, as it would allow folks to open up a page with named frames, and replace their contents arbitrarily. It looks like both Chrome and Firefox block this behavior, popping up a new window instead of allowing A to target C or D.

web-platform-tests/wpt#2364

[bzbarsky]

I don't think we want the behavior that A can target B's nested browsing contexts, same-origin or cross-origin.

You mean in the case when A and B are not same-origin? Seems to me like if A and B are same-origin and both toplevel and B is not disowned then A should be able to target at least B's immediate child browsing contexts.

For what it's worth, here is the explicit algorithm that Gecko uses right now to decide whether A can target B for navigation (ignoring some complexity around file:// URIs for the moment):

1. If A == B, return true.
2. If A is nested and B is the corresponding toplevel, return true.
3. If A is same-origin with B or some ancestor browsing context of B, return true.
4. If B is a nested browsing context, return false (note that at this point we know we're not same-origin with B or any of its ancestors).
5. If B has an opener (is not disowned, in particular) and A can target that opener for any of reasons 1-3 in this list, return true. Note that this does NOT recursively invoke this step 5.
6. Return false.

Now we're going to want to make changes to this stuff to deal with disowning well, obviously. Just wanted to get what we do written down.

[annevk]

The Chrome algorithm is https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/frame/Frame.cpp&l=201 per @mikewest.

@mikewest does not agree with a test in web-platform-tests/wpt#2356 which checks when A opens B that has a child C, that C can navigate A (with A, B, and C having different origins). The fact that C can navigate B to C' which can then navigate A was not convincing as that would be more noticeable to the user. Furthermore the fact that navigating B, especially cross-origin does not disown B, was also something @mikewest wanted to look into changing.

[bzbarsky]

Navigating B in what sense?

If A opens B and then navigates it a bunch, it should continue to be able to do so, right? Pretty sure that's needed for web compat.

If a user navigates B, things are more complicated, at least in terms of user expectations.

[annevk]

I guess I'm having a hard time writing down a proposed specification even with the feedback you've already given. One thing that seems weird to me is that the specification has "familiar with" and "allowed to navigate", but it seems like implementations only have the latter?

Here's a proposed rewrite of familiar with to take your feedback into account:

A is familiar with B if one of these is true:

• A is B.
• The origin of the active document of A is the same as the origin of the active document of B and neither A nor B are disowned.
• A is a nested browsing context with a top-level browsing context, and its top-level browsing context is B.
• B is an auxiliary browsing context and A is B's opener browsing context.
• B is neither top-level browsing context nor disowned, but there exists an ancestor browsing context of B, that is also not disowned, whose active document has the same origin as the active document of A (possibly in fact being A itself).

[bzbarsky]

but it seems like implementations only have the latter?

The spec's "allowed to navigate" algorithm is unrelated to link targeting. It's basically the algorithm that enforces the navigation-related sandboxing flags.

So basically the "familiar with" algorithm determines which browsing context a target="foo" resolves to. Then "allowed to navigate" determines whether you can actually navigate it and only matters if you're sandboxed to start with. Browsers implement both of these algorithms, in some form.

Here's a proposed rewrite of familiar with to take your feedback into account:

Thank you for putting this together. I think these rules have at least the following possibly-undesirable implications:

1. If A and B are both nested browsing contexts and are same-origin, then A is familiar with B (per the second rule in the list) even if their toplevel browsing contexts are disowned and all that. Specifically, if A is a nested browsing context which opens a new window C, then C sets window.opener to null, then C loads a subframe B that's same-origin with A, suddenly A can target B and vice versa.
2. Say B is a disowned browsing context. It does window.open to create a new browsing context A. Let's say A and B are same-origin. In this situation, should A be familiar with B? I would argue yes, but none of the bullet points in the list apply here.

In general, it seems that if we want disowning to have the semantics of "just put this in a new process" then if A opens B and B is disowned, targeting of B and things it loads-as-subframes/opens from should be prevented from A. Similarly, A and the things it loads-as-subframes or the things it opens or the things that opened it should not be targetable from B. But things B opens should be able to target B and be targeted by B, unless those things are themselves disowned, right?

[bzbarsky]

So thinking about this a bit more... What we perhaps really want for the noopener/noreferrer thing is to make the two browsing contexts involved (opener and opened thing) not "directly reachable" in the sense of https://html.spec.whatwg.org/#directly-reachable-browsing-contexts

That will also make them not be in the same "unit of related browsing contexts", right?

Note that the actual spec for targeting at https://html.spec.whatwg.org/#the-rules-for-choosing-a-browsing-context-given-a-browsing-context-name says:

and there exists a browsing context whose name is the same as the given browsing context name, and the current browsing context is familiar with that browsing context, and the user agent determines that the two browsing contexts are related enough that it is ok if they reach each other

We could try to require that "related enough" include "in the same unit of related browsing contexts". This would match absolutely no one's current implementation, I'm pretty sure, but would be possible to converge on, probably.

That still leaves the problem of what to do with dynamic sets of window.opener to null. We could try to require that those also cut the "directly reachable" link (in both directions) and hence change whether the two things are in the same "unit of related browsing contexts". Implementing this might be a bit more annoying, but is probably doable...

Then, within a single unit of related browsing contexts, we can think about what the "familiar with" rules should be, but without having to worry about disowning anymore, since that's already handled.

[annevk]

Okay, so we would make these changes instead:

• "related enough" becomes "directly reachable"
• We change the definition of "directly reachable" to account for "disowned"
• We change "familiar with" to require that A can "directly reach(able)" B

[bzbarsky]

Right, that's basically what I was thinking about.

[annevk]

I have attempted to update the PR to take that into account. I also updated the commit message.

[annevk]

@bzbarsky ping.

[bzbarsky]

This doesn't seem right. Just because the parent is disowned, doesn't mean that .opener should return null.

Should this be checking whether the current browsing context is disowned instead?

[bzbarsky]

So in general, we can have disowned things that can still reach each other with script references (e.g. A window.opens B, B sets opener to null, now B cannot reach A unless it stashed a ref somewhere, but A can reach B, perhaps not in the "reachable" sense of the spec, though).

And as I said above, being disowned does not make one "no longer directly reachable". Just no longer directly reachable from the opener. Still directly reachable from its kids or things it opens, right?

Does that make sense?

[annevk]

Sorry for following up so late. I'm feeling a little lost here, since I'm not super familiar with how all these things work and fit together. Since I noticed you are working on landing some stuff around this in Gecko I thought I'd give this PR another try. Will fix opener first. I agree I was checking the wrong browsing context.

[annevk]

Per your comments it seems this is flawed too. I'm not sure that leaves much in this PR other than cleanup. I think I lost track 😟

[annevk]

The last couple of comments in #313 provide a way forward here. This is distraction at this point.