Ensure that 'noopener' does not reuse a browsing context #1842

mikewest · 2016-09-30T12:19:11Z

WIP: All of this would benefit from more substantial refactoring, but
let's decide that this is the right way to approach things first.

#1826

mikewest · 2016-09-30T12:20:38Z

This is basically the minimal change that does what I think ends up being sane. There's a lot of refactoring that could be done in this area to be more explicit and less repetitious; if this is the direction we want to go, then we can make it prettier.

WDYT, @bzbarsky, @annevk?

annevk · 2016-09-30T12:39:26Z

source

-  <p><dfn>The rules for choosing a browsing context given a browsing context name</dfn> are as
-  follows. The rules assume that they are being applied in the context of a <span>browsing
-  context</span>, as part of the execution of a <span data-x="concept-task">task</span>.</p>
+  <p><dfn>The rules for choosing a browsing context</dfn> given a browsing context name and disown


The old ID should be preserved somehow.

Is there an existing pattern for maintaining old IDs?

bzbarsky

This looks good to me in general, but there are some edge cases that may not have been considered (mentioned below) that I would like to make sure we have agreement on before I spend more time on implementation....

bzbarsky · 2016-09-30T16:31:14Z

source

@@ -18647,8 +18647,8 @@ interface <dfn>HTMLAnchorElement</dfn> : <span>HTMLElement</span> {
    <span>triggered by user activation</span>; or, if the user has not indicated a specific
    <span>browsing context</span> for following the link, and the element's <code
    data-x="attr-hyperlink-target">target</code> attribute is present, and applying <span>the rules
-    for choosing a browsing context given a browsing context name</span>, using the value of the
-    <code data-x="attr-hyperlink-target">target</code> attribute as the browsing context name, would
+    for choosing a browsing context</span>, using the value of the <code


Doesn't this need to pass a value for the disown opener flag?

bzbarsky · 2016-09-30T16:31:55Z

source

-    context name, would result in there not being a chosen browsing context, then run these
-    substeps:</p>
+    <span>the rules for choosing a browsing context</span>, using the value of the <code
+    data-x="attr-hyperlink-target">target</code> attribute as the browsing context name, would


And need to pass disown opener flag value here too...

bzbarsky · 2016-09-30T16:32:17Z

source

-   context</var> as the context in which the algorithm is executed, and let <var>target
-   browsing context</var> be the resulting <span>browsing context</span>.</p></li>
+   Otherwise, apply <span>the rules for choosing a browsing context</span> using <var>target</var>
+   as the name and <var>form browsing context</var> as the context in which the algorithm is


bzbarsky · 2016-09-30T16:32:46Z

source

-      given browsing context name (otherwise, it has no name). The chosen browsing context must be
-      this new browsing context.</p>
+      <p>A new <span>top-level browsing context</span> must be created. Its <span>opener
+      browsing context</span> is the current one if the disown opener flag is false, and empty


Is "empty" the right terminology?

Reworded to avoid the question, since it's not answered anywhere else in the document. :)

bzbarsky · 2016-09-30T16:36:11Z

source

+  <p><dfn>The rules for choosing a browsing context</dfn> given a browsing context name and disown
+  opener flag are as follows. The rules assume that they are being applied in the context of a
+  <span>browsing context</span>, as part of the execution of a <span
+  data-x="concept-task">task</span>.</p>


OK, so let's think about edge cases:

window.open("", "_self", "noopener"); window.open("", "_top", "noopener"); window.open("", "_parent", "noopener");

bzbarsky · 2016-09-30T16:44:49Z

source

+    <p>Otherwise, apply <span>the rules for choosing a browsing context</span> using
+    <var>target</var> as the name, <var>disown opener</var>, and <var>source browsing context</var>
+    as the context in which the algorithm is executed. If this results in there not being a chosen
+    browsing context, then throw an <span>"<code>InvalidAccessError</code>"</span>


Huh, really? This is not what any browser does. Filed #1845 on this, but for purposes of this review I guess this is a preexisting issue.

bzbarsky · 2016-09-30T16:46:00Z

source

-     <li><p>If the result of <span data-x="split a string on commas">splitting <var>features</var>
-     on commas</span> contains the token "<code data-x="">noopener</code>", <span data-x="disowned
-     its opener">disown <var>target browsing context</var>'s opener</span>, and return <code
+     <li><p>If <var>disown opener</var> is true, <span data-x="disowned its opener">disown


OK, so this is the thing we need to sort out for _self and company. Should a window be able to disown its own opener by doing open("", "_self", "noopener"), for example? Should such a call return null?

Hrm, looking at this again, I don't think we actually need to disown the opener here since we're not setting the owner browsing context when choosing a browsing context. Changing this to something like "if disown opener is true, and target browsing context was just created, return null" should be enough.

bzbarsky · 2016-10-04T01:30:29Z

@mikewest So I'd like to make a concrete proposal, because shipping noopener in Firefox is currently blocked on this being resolved, unless we were to just make something up and ship it...

We take the changes from this pull request, so that noopener never reuses an existing named browsing context.
Noopener is ignored when targeting _self, _top, or _parent, so that it does not disown the opener in that case.
Possibly add verbiage to the effect that noopener is ignored when it ends up targeting an existing browsing context (e.g. if the UA diverts the window.open into an existing tab). Because really, it only makes sense to do the disowning thing if we in fact created a new browsing context...

mikewest · 2016-10-05T07:08:20Z

SGTM.
This sounds fine. The return value is somewhat unobservable, as _self, _top, and _parent will all unload the current document as a side-effect of navigating their target, but I agree that the disowning aspect of noopener should be ignored for the navigation.
We could add this as a note, though it's not clear to me that it would ever happen given the targeting rules in this patch and the suggestion you've made in 2 above.

WIP: All of this would benefit from more substantial refactoring, but let's decide that this is the right way to approach things first. #1826

bzbarsky · 2016-10-05T07:26:59Z

The return value is somewhat unobservable

It's totally observable. Off the top of my head, some ways:

Have a page window.open a same-origin window without using noopener. Then have that window navigate _self to another same-origin thing, with noopener stuff involved. Examine the state at various points from the original page that's sitting around.
Have a page window.open a same-origin window without using noopener. Then have that window navigate _self to a javascript: URL that returns a non-string. Examine the state from whichever page you want; the opened thing is not being unloaded at all here.

though it's not clear to me that it would ever happen given the targeting rules in this patch

It can totally happen, per spec. See https://html.spec.whatwg.org/multipage/browsers.html#the-rules-for-choosing-a-browsing-context-given-a-browsing-context-name step 5 the "If the user agent has been configured such that in this instance it will reuse the current browsing context" case. That basically says "If the UA is set up to treat _blank as a synonym for _self". Note also the "User agent implementors are encouraged to provide a way for users to configure the user agent to always reuse the current browsing context." bit after that point.

What probably makes the most sense is to just do the noopener disowning thing in the existing "If the chosen browsing context picked above, if any, is a new browsing context" section that currently does sandbox flag propagation.

mikewest · 2016-10-05T08:37:55Z

It's totally observable. Off the top of my head, some ways:

Have a page window.open a same-origin window without using noopener. Then have that window navigate _self to another same-origin thing, with noopener stuff involved. Examine the state at various points from the original page that's sitting around.

You're right. I hadn't considered something like window.opener.postMessage(window.open("", "_self", "noopener")).

Have a page window.open a same-origin window without using noopener. Then have that window navigate _self to a javascript: URL that returns a non-string. Examine the state from whichever page you want; the opened thing is not being unloaded at all here.

sigh I would dearly love to remove navigation to javascript: (string result or not) from the platform. :(

mikewest · 2016-10-05T08:39:30Z

What probably makes the most sense is to just do the noopener disowning thing in the existing "If the chosen browsing context picked above, if any, is a new browsing context" section that currently does sandbox flag propagation.

I don't actually think we need to actively disown the opener if we never set the "opener browsing context". Does disowning have side-effects other than removing that relationship? https://html.spec.whatwg.org/#disowned-its-opener is a bit light on expectations.

bzbarsky · 2016-10-05T14:11:42Z

Disowning is somewhat underspecified right now, yes.

I think the main question is what the impact of never setting (as opposed to disowning) the opener is on the https://html.spec.whatwg.org/multipage/browsers.html#script-closable concept. For that matter, it's not clear to me whether the thing opened via noopener should be script-closable, conceptually. Looks like in Chrome it is, both in the window.open case and in the <a> case....

mikewest · 2016-10-06T10:44:09Z

Hrm. They're script-closable because they're "a top-level browsing context whose session history contains only one Document." I guess I wouldn't be terribly sad if they were no longer script-closable, but it doesn't seem obviously wrong that they are.

Do you think we need to address that in this patch?

bzbarsky · 2016-10-06T13:10:57Z

They're script-closable because they're "a top-level browsing context whose session history contains only one Document."

Ah, interesting. I did verify that if I use the testcase from #1866 but rel="noopener" in test.html then the close button does not work in Chrome (but does in Firefox and Safari, at least). For the same testcase without rel="noopener" the button works in Chrome, even if I add a window.opener = null in test2.html.

So it certainly looks like at least in Chrome the behavior of rel="noopener" on a link is different from the same link without noopener followed by disowning the opener...

Do you think we need to address that in this patch?

I think what we should do in this patch is not set the opener at all, rather than disowning it. And we can sort out the script-closable behavior in general in #1866 because the current spec seems pretty broken to me (e.g. doesn't match UAs).

domenic · 2016-10-07T17:12:28Z

Is this perchance related to @annevk's earlier forays in #313/#323?

bzbarsky · 2016-10-07T17:23:39Z

Is this perchance related to @annevk's earlier forays in #313/#323?

In the sense that those tried to define what disowning actually means, yes.

bzbarsky · 2016-10-14T22:06:12Z

What I plan to implement in Gecko is that noopener does NOT return null in the _self/_parent/_top cases, and will write tests accordingly.

annevk · 2018-02-05T15:08:10Z

Closing this as this is not the correct fix and the specification has changed quite a bit (noopener is being passed around these days). Probably best to first figure out what everyone wants to align on and go from there.

annevk reviewed Sep 30, 2016

View reviewed changes

bzbarsky reviewed Sep 30, 2016

View reviewed changes

Ensure that 'noopener' does not reuse a browsing context

6d2bd6b

WIP: All of this would benefit from more substantial refactoring, but let's decide that this is the right way to approach things first. #1826

fixup @bzbarsky

bc7dff9

mikewest force-pushed the noopener branch from 29101db to bc7dff9 Compare October 5, 2016 08:30

domenic added do not merge yet Pull request must not be merged per rationale in comment needs tests Moving the issue forward requires someone to write tests labels Oct 7, 2016

domenic assigned mikewest Oct 7, 2016

annevk closed this Feb 5, 2018

annevk deleted the noopener branch February 5, 2018 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that 'noopener' does not reuse a browsing context #1842

Ensure that 'noopener' does not reuse a browsing context #1842

mikewest commented Sep 30, 2016

mikewest commented Sep 30, 2016

annevk Sep 30, 2016

mikewest Oct 5, 2016

bzbarsky left a comment

bzbarsky Sep 30, 2016

bzbarsky Sep 30, 2016

bzbarsky Sep 30, 2016

bzbarsky Sep 30, 2016

mikewest Oct 5, 2016

bzbarsky Sep 30, 2016

bzbarsky Sep 30, 2016

bzbarsky Sep 30, 2016

mikewest Oct 5, 2016

bzbarsky commented Oct 4, 2016

mikewest commented Oct 5, 2016

bzbarsky commented Oct 5, 2016

mikewest commented Oct 5, 2016

mikewest commented Oct 5, 2016

bzbarsky commented Oct 5, 2016

mikewest commented Oct 6, 2016

bzbarsky commented Oct 6, 2016 •

edited

Loading

domenic commented Oct 7, 2016

bzbarsky commented Oct 7, 2016

bzbarsky commented Oct 14, 2016

annevk commented Feb 5, 2018

Ensure that 'noopener' does not reuse a browsing context #1842

Ensure that 'noopener' does not reuse a browsing context #1842

Conversation

mikewest commented Sep 30, 2016

mikewest commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzbarsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzbarsky commented Oct 4, 2016

mikewest commented Oct 5, 2016

bzbarsky commented Oct 5, 2016

mikewest commented Oct 5, 2016

mikewest commented Oct 5, 2016

bzbarsky commented Oct 5, 2016

mikewest commented Oct 6, 2016

bzbarsky commented Oct 6, 2016 • edited Loading

domenic commented Oct 7, 2016

bzbarsky commented Oct 7, 2016

bzbarsky commented Oct 14, 2016

annevk commented Feb 5, 2018

bzbarsky commented Oct 6, 2016 •

edited

Loading