Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disowning owner should perhaps affect link targeting #313

Open
bzbarsky opened this Issue Nov 5, 2015 · 37 comments

Comments

5 participants
@bzbarsky
Copy link
Collaborator

bzbarsky commented Nov 5, 2015

Consider this document:

<div onclick="w = window.open('http://example.com', 'foo'); w.opener= null">
  Click me first
</div>
<div onclick="w = window.open('http://example.org', 'foo'); w.opener= null">
  Click me second
</div>

I believe per spec clicking the first link then the second one should only open one window, since disowning the owner doesn't affect link targeting as far as I can tell.

In both Gecko and Blink, two windows are opened instead.

In Edge only one window is opened and the second click doesn't navigate it (no matter what I do with the opener) which seems a bit odd.

@mikewest

This comment has been minimized.

Copy link
Member

mikewest commented Nov 5, 2015

I prefer Gecko and Blink's current behavior, FWIW. That is, I agree that the spec should change.

annevk added a commit that referenced this issue Nov 9, 2015

Fix #313: make disowning disown the opener browsing context too
Otherwise window names would still leak across.
@annevk

This comment has been minimized.

Copy link
Member

annevk commented Nov 13, 2015

I also get two windows for

<a rel=noreferrer target=x href=http://example.org>test</a>
<a rel=noreferrer target=x href=http://example.org>test</a>

and if I remove rel=noreferrer I no longer do.

When I do <a target=x href="data:text/html,<script>alert(opener)</script>">test</a> in Firefox I get a modal dialog asking whether I want to let the domain containing that link to control this tab. In Chrome this throws a security error of sorts rather than simply returning null.

Anyway, it seems pretty clear that not having an opener should affect link targeting.

I do get the same set of cookies if open about:blank with or without rel=noreferrer but there is no way to inspect this other than through the developer console. So about:blank inheriting a bunch of stuff should likely happen either way, although it is rather pointless since if you cannot access the window anymore you cannot navigate it anywhere or inject any kind of content.

window.name also seems preserved. Which makes it a one-way communication channel. Should we disable that for noopener or is that fine? I suspect folks might want to leak data to ads through other means than just Referer...

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Nov 13, 2015

So @bzbarsky points out that an important factor here is same-origin vs cross-origin. E.g., in Gecko today

data:text/html,<a rel="noreferrer" href="about:blank" target="foo">Click me</a><a target="foo" href="javascript:/* oh, hello */">And then click me</a>

would likely let the javascript URL read all the inherited stuff from the about:blank context, even with rel=noreferrer.

@mikewest

This comment has been minimized.

Copy link
Member

mikewest commented Nov 14, 2015

In an ideal world, I'd like to get to a place where noopener and noreferrer make it possible to put the new window in a completely separate process, even for same-origin documents. That means there must be no mechanism to gain direct DOM access to another same-origin window. With that in mind:

I also get two windows for

<a rel=noreferrer target=x href=http://example.org>test</a>
<a rel=noreferrer target=x href=http://example.org>test</a>

and if I remove rel=noreferrer I no longer do.

This seems like correct behavior to me. We want to decouple these windows, so they shouldn't be familiar with each other. If y'all agree, we'll need to change the "familiar with" definition to adjust the same-origin bit to exclude this case.

<a target=x href="data:text/html,<script>alert(opener)</script>">test</a>

Chrome throws an error because we treat data: as a unique origin.

window.name also seems preserved

It would be good to prevent this, as it would enable the opened window to gain access to a cooperative opener. I'd like to prevent this in the same way we'd prevent targeting the one window from the other.

data:text/html,Click meAnd then click me

Wouldn't the targeting change fix this? That is, "foo" != "foo" if the windows aren't familiar with each other.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Nov 16, 2015

I talked with @bzbarsky and we would be okay with changing our behavior for the same-origin case, to also not allow targeting by name there.

That would mean that the "familiar with" clause needs to change to "Either A is same-origin with B and neither A nor B is disowned, or", correct?

What is the reason for not setting window.name? How can a cooperative opener allow access?

@jeisinger

This comment has been minimized.

Copy link
Member

jeisinger commented Nov 16, 2015

In blink, we only do the lookup by frame name in the local process. So in all cases where we host the navigation in a different process, the lookup by name just never works.

Ideally, that would be spec'd somewhere :)

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Nov 16, 2015

Agreed and that is what this issue is about. However, that does not mean that the browsing context name cannot be set for (what will be) a different process.

@jeisinger

This comment has been minimized.

Copy link
Member

jeisinger commented Nov 16, 2015

right

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

bzbarsky commented Nov 16, 2015

I talked with @bzbarsky and we would be okay with changing our behavior for the same-origin case

Specifically, I would be OK with changing to a behavior that is (1) unambiguously specified (which I don't think the current spec is, nor is Chrome's "local process" thing, since nothing defines what does and doesn't share processes in Chrome, and in fact it depends on various runtime things like how many processes are live already, last I checked) and (2) very likely to actually be web-compatible (because I don't want to change this behavior more than once).

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

bzbarsky commented Nov 16, 2015

Put another way, we need to prevent targeting by name in the same process for same-origin stuff in some cases to have a well-defined spec here that doesn't depend on arbitrary implementation details.

annevk added a commit that referenced this issue Nov 16, 2015

Fix #313: make disowning disown the opener browsing context too
Otherwise window names would still leak across.

annevk added a commit that referenced this issue Nov 16, 2015

Fix #313: make disowning nullify the opener browsing context
With this change, whenever an auxiliary browsing context is disowned,
whether through setting window.opener to null, through using the
rel=noreferrer and rel=noopener keywords, or through using the
window.open() noopener /features/ argument, it isolates that browsing
context from its opener.

It will no longer be "familiar with" it, even same-origin, and its
opener browsing context is explicitly set to null.

It will still get its assigned name (e.g., from the target=""
attribute) and will still be script closable.
@annevk

This comment has been minimized.

Copy link
Member

annevk commented Nov 16, 2015

I updated the patch in #313 to do that. I also posted an update there on the test front: #323 (comment). Input appreciated.

annevk added a commit that referenced this issue Jan 13, 2016

Fix #313: make disowning a browsing context work
This change attempts to make it so that when using rel=noopener, rel=noreferrer, and when setting window.opener to null, the browsing contexts involve can no longer directly reach each other. They end up in their own unit of related browsing contexts. (This is what some in the ECMAScript community refer to as a vat or continent.)

We do this by making directly reachable account for disowning, by making familiar with account for directly reachable, and by changing the rules for choosing a browsing context given a browsing context name to no longer say "related enough", but instead clearly say that the browsing contexts have to be familiar with each other, and by extension, have to be directly reachable.

annevk added a commit that referenced this issue Jan 22, 2016

Fix #313: make disowning a browsing context work
This change attempts to make it so that when using rel=noopener, rel=noreferrer, and when setting window.opener to null, the browsing contexts involve can no longer directly reach each other. They end up in their own unit of related browsing contexts. (This is what some in the ECMAScript community refer to as a vat or continent.)

We do this by making directly reachable account for disowning, by making familiar with account for directly reachable, and by changing the rules for choosing a browsing context given a browsing context name to no longer say "related enough", but instead clearly say that the browsing contexts have to be familiar with each other, and by extension, have to be directly reachable.

annevk added a commit that referenced this issue Apr 28, 2016

Fix #313: make disowning a browsing context work
This change attempts to make it so that when using rel=noopener, rel=noreferrer, and when setting window.opener to null, the browsing contexts involve can no longer directly reach each other. They end up in their own unit of related browsing contexts. (This is what some in the ECMAScript community refer to as a vat or continent.)

We do this by making directly reachable account for disowning, by making familiar with account for directly reachable, and by changing the rules for choosing a browsing context given a browsing context name to no longer say "related enough", but instead clearly say that the browsing contexts have to be familiar with each other, and by extension, have to be directly reachable.
@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 4, 2017

Instead of adjusting "familiar with", how about noopener effectively ends up creating a new "unit of related browsing contexts" and name targeting only searches within that unit (per #1440)?

As long as we then restrict "familiar with" to play within a "unit of related browsing contexts" I think we're good and don't run into the numerous complications I ran into with my PR.

@bzbarsky does that make sense?

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

bzbarsky commented May 4, 2017

This issue is explicitly about disowning openers via setting window.opener = null (which is under the control of the thing that was opened), not noopener (which is under the control of the thing doing the opening). They're not the same thing.

I'm happy to discuss noopener behavior in #1826 if you would like.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 4, 2017

Sorry.

For this issue, if I refactored the specification sufficiently such that I can allocate "unit of related browsing contexts" and I refactored that so it actually consists of a set of browsing context trees (which I think is what implementations do), I could allocate a new unit of related browsing contexts and just move one item of the set (whose top-level browsing context's window's opener was set to null) to this new unit.

Or would we then end up with more separation than we'd like? (Reading through #323 it seems like this might be okay, but maybe things have changed.)

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 4, 2017

I guess that setup doesn't quite work when A creates auxiliary B and B creates auxiliary C and then B sets opener to null. You'd then want (A), (B, C) I think, although it gets weird if A already accessed (nested) bits of B or C.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 4, 2017

Anyway, if we can figure out the actual structure of a "unit of related browsing contexts" (tab group in Firefox, dunno about Chrome), we can then debate how to divide it and whether that works or whether we'll need some access restrictions on top of that structure for a couple special cases (such as link targeting). Hopefully that makes sense.

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

bzbarsky commented May 4, 2017

I think there are two separate concepts here:

  1. Which things might conceivably be able to target each other, assuming all documents involved are same-origin? Using "unit of related browsing contexts" is convenient here in that it's an existing spec concept. But note that I think the unit of related browsing contexts a browsing context belongs to should be determined at browsing context creation time. Moving a thing from one unit of related browsing contexts to another dynamically probably does not interact well with allowing implementations to have a separate event loop for every unit of related browsing contexts, and allowing that is a pretty explicit goal in the spec. Indeed, the spec aims to allow a separate event loop for every "unit of related similar-origin browsing contexts"; I'm not sure whether it succeeds. So if we want opener-disowning to affect targeting in the same-origin case, then using "unit of related browsing contexts" probably doesn't do what we want.

  2. When different-origin documents are involved, what additional checks are done within the unit of related browsing contexts in terms of targeting? Are these checks affected by whether openers have been disowned or not?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 4, 2017

I think the event loop, if Chrome's out-of-process-iframe experiments continue well, ends up being scoped to similar-origin window agent, which is a "subset" of "unit related browsing contexts" (currently called "unit of related similar-origin browsing contexts" though that is a bit of a misnomer as it's really documents/windows, not browsing contexts).

So splitting "unit of related browsing contexts" could work out and my main goal there was mostly that it would be a little cleaner and potentially have benefits beyond link targeting (although I'm not sure which, granted).

Anyway, even if that cannot work, defining the layout of a "unit of related browsing contexts" would help a lot. In particular if it's a collection of browsing context trees (root of which is auxiliary or top-level browsing context) we could give each item in that collection a flag and make it very clear how accessing between them goes.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 5, 2017

A browsing context tree is the wrong model as child browsing contexts are tied to a document (or element, really), not a browsing context.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 5, 2017

Per previous discussion at #313 (comment) same-origin was considered an acceptable change. That was end of 2015 however.

I'm not sure how to best change the model to take the constraints of your first point into account. One thing I can think of is that:

  1. Each top-level browsing context gets an auxiliaries set. This is where we store window.open() calls / <a target>/ etc.
  2. We change the flag to be a pointer to the opener's top-level browsing context (called exclude).
  3. When we target, we look at the top-level browsing context, store its exclude in an exclusion set, append exclude's auxiliaries to that set, then look at browsing context group's set and remove all in exclusion from it and look then go through each top-level browsing context and their descendants that are left.
@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

bzbarsky commented May 5, 2017

Per previous discussion at #313 (comment) same-origin was considered an acceptable change.

That was in the noopener/noreferrer case. That case already creates a new unit of related browsing contexts no matter what, right? In particular, it creates a new toplevel browsing context, not an auxiliary browsing context. That is, what noopener/noreferrer do is fundamentally quite different from what the "disown the owner" operation does (and we should actually spec it not in terms of disowning the owner!). There was agreement to that effect before gecko implemented noopener; I'm pretty sure there are open spec issues where that was discussed, but I don't have links offhand. I can hunt them down if needed.

I'm not sure how to best change the model to take the constraints of your first point into account

It might be worth checking what UAs actually do.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 5, 2017

That was in the noopener/noreferrer case.

Good point. Okay, so we need to take that into account.

It might be worth checking what UAs actually do.

Sure, we'll need to do that too. But it helps a lot to have a clear (ideal) model to test expectations against. It's much easier to describe deviations from a model than to describe the model each user agent has in my experience.

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

bzbarsky commented May 5, 2017

That's fair, but if we're having trouble coming up with the basic principles behind a model, it might be useful to see what principles, if any, the UAs use.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 8, 2017

Chrome/Firefox disallow cross-origin navigation from the opener after the openee sets opener to null (create a new auxiliary browsing context instead). They do indeed allow same-origin navigation. Edge/Safari always allow navigation. (I haven't tested what flavor of same-origin is being used yet. But I'm convinced we need some now.)

I also realize when I said "first point" above I meant "second point" since I preceded with a sketch for the first point. And it seems the first point, which related to same-origin or not, can be accounted for comparing the origin of the active document of the browsing context that has the separation flag set.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 8, 2017

I also checked that navigating the openee (which has set opener to null) across origins disallows the opener from navigating it in the same manner in Chrome/Firefox.

annevk added a commit to web-platform-tests/wpt that referenced this issue May 8, 2017

@annevk annevk self-assigned this Feb 4, 2018

@annevk

This comment was marked as outdated.

Copy link
Member

annevk commented Feb 4, 2018

So I think I managed to confuse myself here by dragging in noopener/noreferrer. My apologies. Reading this again, if this just needs to affect whether to open a new browsing context or not, it seems we need to modify step 6 of https://html.spec.whatwg.org/#the-rules-for-choosing-a-browsing-context-given-a-browsing-context-name and that's it. (And maybe incorporate some of the other editorial changes I proposed in the PR.)

I'll work on an updated PR that does that and write some tests.

annevk added a commit to web-platform-tests/wpt that referenced this issue Feb 4, 2018

@annevk

This comment was marked as outdated.

Copy link
Member

annevk commented Feb 5, 2018

Bit too optimistic over the weekend as I cannot figure out the exact check needed. E.g., if A opens cross-origin B named "hi" and B embeds C named "bye" then you cannot target C from A (using target=bye), but you can target B unless opener is set to null.

A proper fix would require figuring out the actual model browsers use for maintaining browsing context and their names.

@annevk annevk removed their assignment Feb 5, 2018

@bzbarsky

This comment was marked as resolved.

Copy link
Collaborator Author

bzbarsky commented Feb 5, 2018

Fwiw, I would be happy to document Gecko's current model if that would be useful.

@annevk

This comment was marked as resolved.

Copy link
Member

annevk commented Feb 5, 2018

Yeah, it might.

@annevk

This comment was marked as resolved.

Copy link
Member

annevk commented Sep 26, 2018

@bzbarsky I now think a complete description of named targeting would be useful. Having it might also help browsers keep a consistent implementation as they move to more processes.

cc @mystor @farre

kisg pushed a commit to paul99/webkit-mips that referenced this issue Oct 15, 2018

cdumez@apple.com
Restrict browsing context lookup by name to frames that are related t…
…o one another

https://bugs.webkit.org/show_bug.cgi?id=190475

Reviewed by Alex Christensen.

Source/WebCore:

Update our frame lookup by name logic to take in the active / requesting frame and
only a return a frame that is related to it. By related to it, I mean:
- Ancestor <-> Descendant relationship
- Opener <-> Openee relationship

Being able to look up unrelated frames makes process swapping difficult so we need
to be stricter.

This change is being discussed at:
- whatwg/html#313

Tests: http/tests/dom/new-window-can-target-opener.html
       http/tests/dom/noopener-window-cannot-target-opener.html
       http/tests/dom/noopener-window-not-targetable.html
       http/tests/dom/noopener-window-not-targetable2.html
       http/tests/dom/noreferrer-window-not-targetable.html
       http/tests/dom/opened-window-not-targetable-after-disowning-opener.html

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::findFrameForNavigation):
* page/FrameTree.cpp:
(WebCore::isFrameFamiliarWith):
(WebCore::FrameTree::find const):
* page/FrameTree.h:
* rendering/HitTestResult.cpp:
(WebCore::HitTestResult::targetFrame const):

Source/WebKit:

* WebProcess/Plugins/PluginView.cpp:
(WebKit::PluginView::performJavaScriptURLRequest):

Source/WebKitLegacy/mac:

* WebView/WebFrame.mm:
(-[WebFrame findFrameNamed:]):

LayoutTests:

* http/tests/dom/new-window-can-target-opener-expected.txt: Added.
* http/tests/dom/new-window-can-target-opener.html: Added.
* http/tests/dom/noopener-window-cannot-target-opener-expected.txt: Added.
* http/tests/dom/noopener-window-cannot-target-opener.html: Added.
* http/tests/dom/noopener-window-not-targetable-expected.txt: Added.
* http/tests/dom/noopener-window-not-targetable.html: Added.
* http/tests/dom/noopener-window-not-targetable2-expected.txt: Added.
* http/tests/dom/noopener-window-not-targetable2.html: Added.
* http/tests/dom/noreferrer-window-not-targetable-expected.txt: Added.
* http/tests/dom/noreferrer-window-not-targetable.html: Added.
* http/tests/dom/opened-window-not-targetable-after-disowning-opener-expected.txt: Added.
* http/tests/dom/opened-window-not-targetable-after-disowning-opener.html: Added.
* http/tests/dom/resources/new-window-can-target-opener-win.html: Added.
* http/tests/dom/resources/noopener-window-cannot-target-opener-win.html: Added.
Add layout test coverage.

* fast/dom/Window/a-rel-noopener-expected.txt:
* fast/dom/Window/area-rel-noopener-expected.txt:
* fast/dom/Window/resources/rel-noopener.js:
* http/tests/navigation/no-referrer-target-blank-expected.txt:
* http/tests/navigation/resources/no-referrer-helper.php:
* platform/mac-wk1/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt:
* platform/wk2/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt:
Update / rebaseline existing tests to reflect behavior change.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@237112 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

bzbarsky commented Jan 11, 2019

Gecko's current behavior is as follows, ignoring the special _blank, _parent, _top names:

First, we define a CanAccess primitive which takes as input two browsing
contexts: a target and a source. It is implemented as follows:

  1. If no source provided (not sure whether this can happen in a web context), return true.
  2. If source and target are the same object, return true.
    • Browsing contexts can target themselves.
  3. If source is a nested browsing context and target is an ancestor of source and target is a top-level browsing context, return true.
    • Frames can target their top-level browsing context.
  4. If source's active document is same-origin with the active document of target or some ancestor browsing context of target, return true. There are some complications here around file:// URIs.
    • Documents can target documents that are same-origin or nested through same-origin documents. Except that's not quite what this implements, because it walks browsing context parent chains, not nesting chains. But maybe we don't reach this for browsing contexts that are nested through inactive documents...
  5. If target is not a toplevel browsing context, return false.
    • Can't target subframes in cases not covered above.
  6. If target is not an auxiliary browsing context, return false.
    • Can't target random non-same-origin top-level browsing contexts.
  7. Repeat steps 1-4 with the same source but target set to target's window's opener's browsing context, if target's window has an opener. This can be affected by .opener = null calls, so doesn't match the concept of "opener browsing context" in the spec, I think! Might return from this algorithm.
    • Can target an auxiliary browsing context if we can target its opener But this only goes back "one level", so to say: we never examine the opener's opener.
  8. Return false.

Note that I can't claim that this CanAccess is at all principled. It sort of
started out as return true and then various restrictions were added to it to
prevent specific attacks while trying to not break web compat. It might well be
too tight or too loose or both in various cases.

Armed with this CanAccess, we can define IsValidTarget which takes a browsing
context "source", a browsing context "target", and a string "name" and does:

  1. If target's name is not "name", return false.
  2. If CanAccess(target, source) returns false, return false.
  3. Return true.

the named target search for a string "name" now works like this:

  1. Set "source" to be the source browsing context for the search. For targeted
    form submissions and link clicks this is the browsing context of the node's
    document. For window.open this is the browsing context of the "this" object.
    Though there is some weirdness around the entry global's browsing context in
    here too, to decide whether we're opening a new window. Chances are, there are
    bugs around this stuff when document.domain is involved, in Firefox.

  2. Check whether IsValidTarget(source, source, name). If true, return source.

  3. For all descendants of "source", in depth-first preorder, check whether
    IsValidTarget(source, descendant, name). If true, return descendant.

  4. Repeat the search for all ancestors of "source", in each case skipping all
    browsing contexts we have already considered and doing depth-first preorder
    walks on the browsing context tree. If any browsing context there returns true
    from IsValidTarget(), return it. So this considers the parent of "source", then
    all the parent's descendants that we haven't looked at yet, then the
    grandparent, then all its descendants, etc.

  5. If nothing has been found so far, repeat the walk starting at all the
    toplevel browsing contexts in the same unit of related browsing contexts, in
    some order. Again, check IsValidTarget each time, return if something is found.

  6. Return null.

@mystor

This comment has been minimized.

Copy link
Contributor

mystor commented Jan 11, 2019

Thanks for writing this up! It's good to know that my understanding of our algorithm wasn't too far off :-)

If source's active document is same-origin with the active document of target or some ancestor browsing context of target, return true. There are some complications here around file:// URIs.

* Documents can target documents that are same-origin or nested through same-origin documents.  Except that's not quite what this implements, because it walks browsing context parent chains, not nesting chains. But maybe we don't reach this for browsing contexts that are nested through inactive documents...

For inactive documents here, are your referring to documents in the BFCache? IIRC currently we consider documents framed by a cached document to not have a parent, meaning that they wouldn't be able to target their _top. We might still be able to target auxiliary browsing contexts, however.

@bzbarsky

This comment has been minimized.

Copy link
Collaborator Author

bzbarsky commented Jan 11, 2019

For inactive documents here, are your referring to documents in the BFCache?

Yes, or just unloaded in general.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 4, 2019

The above algorithm also needs to be influenced by window.close() as that'll remove a browsing context from consideration despite it not being discarded yet. (#4402 adds an "is closing" boolean for this additional branch.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.