1503681 - Make target=_blank on anchors imply rel=noopener

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[(no longer active)]

Safari is experimenting with this (https://trac.webkit.org/changeset/237144/webkit/), we should add a pref and turn it on on prerelease channels for a while to get webcompat data as well.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Just want to add a few notes about what implementation of this would look like:
 1. We would need to relax the check here: https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/docshell/base/nsDocShell.cpp#9411 to also run that code path for _blank loads. 
 2. Unfortunately, that code path only supports link-style loads, and not loads like form submissions etc. We'll need to add extra checks to confirm that we're performing a link load, otherwise we'll hit the assertions bz added.
 3. In the future, we will probably want to add the ability for noopener loads with POST data, in which case we can fully relax that check.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Another option for now would be to just add a thing to the flag calculation at https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/docshell/base/nsDocShell.cpp#13261-13282 that adds INTERNAL_LOAD_FLAGS_NO_OPENER when target="_blank".  That would automatically restrict to link (<a> and <area>, though Safari did not do <area>, so maybe we shouldn't either?) loads.

If we want to support the rel="opener" opt-in the Safari changeset talks about, this might also be somewhat simpler in the flag-calculation code, since it's supposed to override rel="noopener" (or at least does in the Safari impl... should it, really?).

Comment 3

[(no longer active)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)
> Another option for now would be to just add a thing to the flag calculation
> at
> https://searchfox.org/mozilla-central/rev/
> b096dcf0ea226af628fe03f7e7acb56a25853533/docshell/base/nsDocShell.cpp#13261-
> 13282 that adds INTERNAL_LOAD_FLAGS_NO_OPENER when target="_blank".  That
> would automatically restrict to link (<a> and <area>, though Safari did not
> do <area>, so maybe we shouldn't either?) loads.

Is there a downside to including <area>?  It shouldn't be all too common in today's web hopefully.

> If we want to support the rel="opener" opt-in the Safari changeset talks
> about, this might also be somewhat simpler in the flag-calculation code,
> since it's supposed to override rel="noopener" (or at least does in the
> Safari impl... should it, really?).

That is a strange choice, I would imagine we want noopener to override opener.  Might be worth filing a bug on webkit to ask them to consider changing that behavior.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Is there a downside to including <area>?

Apart from getting to interop, no.

> That is a strange choice

I agree.  But whichever direction we want the overriding to go, it's best in the flags-calculation code where we can explicitly order our checks.

Comment 5

[Anne (:annevk)]

This would be good for Fission as well. Standards-wise this is tracked by https://github.com/whatwg/html/issues/4078 where I asked about <area> and <form> in Safari. It'd make sense to include them. For Firefox we also have to consider <link> (tracked by https://github.com/whatwg/html/issues/2617 whether others want to copy us on that)?

Comment 6

[(no longer active)]

It would be nice if we can find an owner for this.  This would be helpful for the next phase of our antitracking project as well...

Comment 7

[Andrew Overholt [:overholt]]

I've taken it to email but I'll leave my needinfo.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: noopener.patch

Comment 9

[(no longer active)]

Two questions:

  * What is the relationship of this pref and fingerprinting resistance?

  * We probably want to start to keep this on for early beta and earlier first for a while before rushing to ship it to everyone, no?

(Also please send an intent to implement email, thanks!)

Comment 10

[Andrea Marchesini [:baku]]

Attachment: noopener.patch

Comments applied.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: noopener.patch

Comment 12

[Chris Mills [:cmills]]

We'll need to note this in the docs/compat data.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9026637 [details] [diff] [review]
noopener.patch

>+      aContent->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::target,
>+                                     target);

Why can't you use aTargetSpec?  That's what it's there for.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Also, the commit message should describe the intended interaction of rel="noopener", rel="opener", and target="_blank", and we should get spec or webkit issues filed as needed if the thing we implement doesn't match those.

Comment 15

[Andrea Marchesini [:baku]]

Following https://github.com/whatwg/html/issues/4078, webkit is currently testing anchor element. They are not supporting area yet but, I guess we can test both elements in nightly for 1 cycle to see if there are regressions.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: noopener.patch

bz's comment applied.

Comment 17

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #15)
> Following https://github.com/whatwg/html/issues/4078, webkit is currently
> testing anchor element. They are not supporting area yet but, I guess we can
> test both elements in nightly for 1 cycle to see if there are regressions.

Having wpt tests may be helpful in incentivizing them to support <area> too.  :-)

Comment 18

[Andrea Marchesini [:baku]]

Attachment: WPT

Comment 19

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9026709 [details] [diff] [review]
noopener.patch

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

r- mostly because I want more clarity around the interactions between opener/noopener/noreferrer :-)

::: docshell/base/nsDocShell.cpp
@@ +13275,5 @@
>      if (aNoOpenerImplied) {
>        flags |= INTERNAL_LOAD_FLAGS_NO_OPENER;
>      }
> +
> +    if (!(flags & INTERNAL_LOAD_FLAGS_NO_OPENER) &&

Hmm - Should having `rel="noopener opener"` override the noopener back to opener? I think it'd be cleaner to change the order around (which would have that effect):

if (enabled() && isBlank) {
  flags |= NO_OPENER
}
while (...) {
  if (token.LowerCaseEqualsLiteral("opener") && enabled()) {
    flags &= ~INTERNAL_LOAD_FLAGS_NO_OPENER
  }
}

You'd also have to drop the `break` from `noreferrer`, as this flag could override the noopener part of it in that case.

@@ +13278,5 @@
> +
> +    if (!(flags & INTERNAL_LOAD_FLAGS_NO_OPENER) &&
> +        StaticPrefs::dom_targetBlankNoOpener_enabled() &&
> +        !hasOpener &&
> +        target.EqualsLiteral("_blank")) {

I think this should be LowerCaseEqualsLiteral - IIRC _blank isn't case sensitive. (https://searchfox.org/mozilla-central/rev/876022232b15425bb9efde189caf747823b39567/docshell/base/nsDocShell.cpp#3165)

::: dom/html/test/browser_targetBlankNoOpener.js
@@ +9,5 @@
> +
> +      let p = BrowserTestUtils.browserLoaded(newBrowser);
> +
> +      ContentTask.spawn(newBrowser, null, async _ => {
> +        content.window.postMessage("check opener", "*");

I feel like this would be cleaner as:

let hasOpener = await ContentTask.spawn(newBrowser, null, _ => {
    return !!content.window.opener;
});

This would also let you avoid the need for a second load & the spawn. To close the tab you could also use `await BrowserTestUtils.removeTab(e.target);`.

@@ +54,5 @@
> +
> +  info("Creating an anchor with target=_blank rel=opener");
> +  let p = hasOpener();
> +  await ContentTask.spawn(browser, TEST_IFRAME_URL, async url => {
> +    await new content.Promise(resolve => {

I don't think the nested promise is needed here, you're always resolving it synchronously anyway :-)

@@ +107,5 @@
> +  });
> +
> +  ok(!(await p), "We don't want the opener");
> +
> +  info("Removing the tab");

It might be cleaner for these functions if we could instead do something like:

await checkOpener(browser, {target: "_blank", rel: "noopener"});

CheckOpener would then do something like:

let p = hasOpener();
options.href = TEST_IFRAME_URL;
await ContentTask.spawn(browser, options, async options => {
  let anchor = ...;
  for (let key of Object.keys(options)) {
    anchor.setAttribute(key, options[key]);
  }
  anchor.click();
});
return await p;



--

I'd also like tests for weird rel values like rel="noopener opener", rel="opener opener" etc.

@@ +194,5 @@
> +
> +      area.click();
> +      resolve();
> +    });
> +  });

If there's some way we could unify the test cases more with the first version (perhaps have two helpers & call with both helpers?) so it's easy to add more test cases (e.g. for weird rel cases) that would be nice :-)

::: modules/libpref/init/StaticPrefList.h
@@ +453,5 @@
>  )
>  
> +// For area and anchor elements with target=_blank and no rel set to
> +// opener/noopener, this pref sets noopener by default.
> +#ifdef EARLY_BETA_OR_EARLIER

Hmm - isn't this backwards? I would imagine we'd want to turn this pref on for nightly/early beta and off for late beta/release, no?

Comment 20

[Anne (:annevk)]

Comment on attachment 9027026 [details] [diff] [review]
WPT

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

r+ with nit addressed. I don't really know the .ini format, but I trust that it's fine.

::: testing/web-platform/tests/html/semantics/links/links-created-by-a-and-area-elements/target_blank_implicit_noopener.tentative.html
@@ +35,5 @@
> +  tests.forEach(data => {
> +    async_test(
> +      test => {
> +        let bc = new BroadcastChannel(data.id);
> +        bc.addEventListener("message", e => {

You want to wrap the callback here with test.step_func_done(e => ...). You can then also remove test.done(). That ensures that if an exception is thrown it gets reported back to this specific test, rather than fail the entire run.

Comment 21

[Andrea Marchesini [:baku]]

Attachment: noopener.patch

I think we should have this behavior:

1. noreferrer always wins. if you do: noreferrer opener, we don't set the opener.
2. the last noopener or opener has priority. This means that rel="opener noopener opener" => we have opener.

Comment 22

[Anne (:annevk)]

Can we change 2 to simply make "noopener" win in that scenario? I don't think we there's any scenario right now where order matters for the rel="" attribute (there used to be one around "up", but that got removed).

Comment 23

[Andrea Marchesini [:baku]]

Attachment: noopener.patch

noopener wins.

Comment 24

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9027531 [details] [diff] [review]
noopener.patch

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

::: docshell/base/nsDocShell.cpp
@@ +13296,1 @@
>      while (tok.hasMoreTokens()) {

Let's add a comment to the effect of:

// The opener behaviour follows a hierarchy, such that if a higher priority 
// behaviour is specified, it always takes priority. That priority is currently:
// norefrerer > noopener > opener > default

::: dom/html/test/browser_targetBlankNoOpener.js
@@ +3,5 @@
> +async function checkOpener(browser, elm, name, rel) {
> +  let p = new Promise(resolve => {
> +    gBrowser.tabContainer.addEventListener("TabOpen", async e => {
> +      let newBrowser = e.target.linkedBrowser;
> +      await BrowserTestUtils.browserLoaded(newBrowser);

Can we use BrowserTestUtils.waitForNewTab here? (I'm only being picky because I've seen frontend struggle with updating tests for changes before :-) )

i.e.:

let newTab = BrowserTestUtils.waitForNewTab(gBrowser, TEST_URL, true);
await ContentTask.spawn(browser, {url:....}, ...);
await newTab;
let hasOpener = await ContentTask.spawn(newTab.linkedBrowser, null, _ => !!content.window.opener);
await BrowserTestUtils.removetab(newTab);
return hasOpener;

@@ +15,5 @@
> +      resolve(hasOpener);
> +    }, { once: true });
> +  });
> +
> +  await ContentTask.spawn(browser, {url: TEST_URL, name, rel, elm }, async obj => {

nit: inconsistent whitespace in { ... } braces.

@@ +75,5 @@
> +    ["dom.disable_open_during_load", true],
> +    ["dom.targetBlankNoOpener.enabled", true],
> +  ]});
> +
> +  let tab = BrowserTestUtils.addTab(gBrowser, TEST_URL);

Let's do

await BrowserTestUtils.withNewTab({gBrowser, url: TEST_URL}, async browser => {
    await BrowserTestUtils.browserLoaded(browser);
    await runTests(browser, 'anchor');
    await runTests(browser, 'area');
});

It automatically handles foregrounding & removing the tab when we're done for us :-)

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99ae47766ba9
rel=noopener implicit for target=_blank in anchor and area elements when no rel attribute is set, r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7483919d6be
rel=noopener implicit for target=_blank in anchor and area elements when no rel attribute is set - WPT, r=annevk

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aa0df597d30
rel=noopener implicit for target=_blank in anchor and area elements when no rel attribute is set - disabled by default, r=me

Comment 27

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70abf246cac8
Backed out 3 changesets for landing incorrectly on a CLOSED TREE

Comment 28

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14257 for changes under testing/web-platform/tests

Comment 29

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/14257
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/fq_0-1nTQkWpZUgoiCJEuw)

Comment 30

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a19fe6be68c
rel=noopener implicit for target=_blank in anchor and area elements when no rel attribute is set, r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/076cc5bece1d
rel=noopener implicit for target=_blank in anchor and area elements when no rel attribute is set - WPT, r=annevk
https://hg.mozilla.org/integration/mozilla-inbound/rev/211c7dfdc408
rel=noopener implicit for target=_blank in anchor and area elements when no rel attribute is set - fix tests, r=me

Comment 32

[Cristian Brindusan [:cbrindusan]]

Backed out 3 changesets (Bug 1503681) for geckoview failures on org.mozilla.geckoview.test.GeckoSessionTestRuleTest.waitForChromeJS.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b526d511169a3ba8ea647d6de9183a62df0da1c8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=211c7dfdc4083e48ef9f86a91d219e22fe7f72b7&selectedJob=214318134

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=214318134&repo=mozilla-inbound&lineNumber=1879

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3451e101fd67
rel=noopener implicit for target=_blank in anchor and area elements when no rel attribute is set, r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/45e6f25036c5
rel=noopener implicit for target=_blank in anchor and area elements when no rel attribute is set - WPT, r=annevk
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a4330eb15f
rel=noopener implicit for target=_blank in anchor and area elements when no rel attribute is set - fix tests, r=me

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d75d80adf8a1
rel=noopener implicit for target=_blank in anchor and area elements when no rel attribute is set - fix tests, r=me CLOSED TREE

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90b9a0bd1fc5
rel=noopener implicit for target=_blank in anchor and area elements when no rel attribute is set - fix tests, r=me

Comment 36

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/3451e101fd67
https://hg.mozilla.org/mozilla-central/rev/45e6f25036c5
https://hg.mozilla.org/mozilla-central/rev/e6a4330eb15f
https://hg.mozilla.org/mozilla-central/rev/d75d80adf8a1
https://hg.mozilla.org/mozilla-central/rev/90b9a0bd1fc5

Comment 37

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 38

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/14257
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/bodJM1h6RUWHjudHuA7hlw)

Comment 39

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 40

[Chris Mills [:cmills]]

Note to MDN writers:

I have added a note to the Fx65 rel notes to cover this:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#HTML

In terms of docs, it looks like we'll need to add a description to the <a> page to cover this new behavior, and possibly update BCD?

Comment 41

[Andrea Marchesini [:baku]]

Chris, this feature is not enabled by default in 65. Just in nightly. We want to wait 1 extra cycle before enabling it by default.

Comment 42

[Chris Mills [:cmills]]

(In reply to Andrea Marchesini [:baku] from comment #41)
> Chris, this feature is not enabled by default in 65. Just in nightly. We
> want to wait 1 extra cycle before enabling it by default.

Ah, OK, I've removed the note again in that case. Thanks baku!

Comment 43

[Chris Mills [:cmills]]

I've documented this. Since this is currently an early experiment, I've decided not to mention this on the <a> page, and instead just include an entry in the Experimental Features page:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Experimental_features#Security

Let me know if you think that looks OK. Thanks!

Comment 44

[Andrea Marchesini [:baku]]

I've documented this. Since this is currently an early experiment, I've decided not to mention this on the <a> page, and instead just include an entry in the Experimental Features page:

Bug 1522083 enables this feature by default in FF67.

Comment 45

[Victor Carciu]

We did run a mozregression because Bug 1344376 has been recently reported as regressed, and mozregression has identified this change as the potential source for that regression.

Can you please help us to see if this is the source of regression ?

Comment 46

[Boris Zbarsky [:bzbarsky]]

It's quite possible, yes, since the load started happening in a different process. A simple way to test is to see whether a testcase like the one for bug 1344376 but using <a target="_blank" rel="noopener"> shows the following behavior:

1. Fires onCreatedNavigationTarget right after bug 1344376 is fixed.
2. Stops firing it once bug 1365032 is fixed.
3. Fires it on current nightly (that is, including the fix for this bug) if the
   "dom.noopener.newprocess.enabled" preference is set to false.

If that's the behavior, we should file a new bug to track firing onCreatedNavigationTarget as needed when doing a cross-process load, in "WebExtensions:Request Handling".

Comment 47

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #46)

It's quite possible, yes, since the load started happening in a different process. A simple way to test is to see whether a testcase like the one for bug 1344376 but using <a target="_blank" rel="noopener"> shows the following behavior:

1. Fires onCreatedNavigationTarget right after bug 1344376 is fixed.
2. Stops firing it once bug 1365032 is fixed.
3. Fires it on current nightly (that is, including the fix for this bug) if the
   "dom.noopener.newprocess.enabled" preference is set to false.

Thanks for the STR, I can confirm that this is the case (I just quickly tried by changing the existing testcase to use rel="noopener").

If that's the behavior, we should file a new bug to track firing onCreatedNavigationTarget as needed when doing a cross-process load, in "WebExtensions:Request Handling".

Definitely, I just filed it as Bug 1543647.