Make target=_blank on anchors imply rel=noopener

RESOLVED FIXED in Firefox 65

Status

()

P2
normal
RESOLVED FIXED
5 months ago
5 days ago

People

(Reporter: Ehsan, Assigned: baku)

Tracking

(Depends on: 1 bug, {dev-doc-complete, site-compat})

unspecified
mozilla65
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

5 months ago
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.
Flags: needinfo?(nika)

Comment 1

4 months ago
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.
Flags: needinfo?(nika)
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?).
(Reporter)

Comment 3

4 months ago
(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.
> 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

4 months ago
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)?
Priority: -- → P2
(Reporter)

Comment 6

4 months ago
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...
Flags: needinfo?(overholt)
I've taken it to email but I'll leave my needinfo.
(Assignee)

Comment 8

4 months ago
Posted patch noopener.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #9026193 - Flags: review?(nika)
Flags: needinfo?(overholt)
(Reporter)

Comment 9

4 months ago
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!)
(Assignee)

Comment 10

4 months ago
Posted patch noopener.patch (obsolete) — Splinter Review
Comments applied.
Attachment #9026193 - Attachment is obsolete: true
Attachment #9026193 - Flags: review?(nika)
Attachment #9026637 - Flags: review?(nika)
(Assignee)

Comment 11

4 months ago
Posted patch noopener.patch (obsolete) — Splinter Review
Attachment #9026637 - Attachment is obsolete: true
Attachment #9026637 - Flags: review?(nika)
Attachment #9026642 - Flags: review?(nika)
We'll need to note this in the docs/compat data.
Keywords: dev-doc-needed
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.
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.
Keywords: site-compat
(Assignee)

Comment 15

4 months ago
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.
(Assignee)

Comment 16

4 months ago
Posted patch noopener.patch (obsolete) — Splinter Review
bz's comment applied.
Attachment #9026642 - Attachment is obsolete: true
Attachment #9026642 - Flags: review?(nika)
Attachment #9026709 - Flags: review?(nika)
(Reporter)

Comment 17

4 months ago
(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.  :-)
(Reporter)

Updated

4 months ago
See Also: → bug 1509346
(Assignee)

Comment 18

4 months ago
Posted patch WPTSplinter Review
Attachment #9027026 - Flags: review?(annevk)

Comment 19

4 months ago
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?
Attachment #9026709 - Flags: review?(nika) → review-

Comment 20

4 months ago
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.
Attachment #9027026 - Flags: review?(annevk) → review+
(Assignee)

Comment 21

4 months ago
Posted patch noopener.patch (obsolete) — Splinter Review
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.
Attachment #9027173 - Flags: review?(nika)

Comment 22

4 months ago
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).
(Assignee)

Comment 23

4 months ago
noopener wins.
Attachment #9026709 - Attachment is obsolete: true
Attachment #9027173 - Attachment is obsolete: true
Attachment #9027173 - Flags: review?(nika)
Attachment #9027531 - Flags: review?(nika)

Comment 24

4 months ago
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 :-)
Attachment #9027531 - Flags: review?(nika) → review+

Comment 25

4 months ago
Pushed by amarchesini@mozilla.com:
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

4 months ago
Pushed by amarchesini@mozilla.com:
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

4 months ago
Backout by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70abf246cac8
Backed out 3 changesets for landing incorrectly on a CLOSED TREE
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14257 for changes under testing/web-platform/tests
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)
Upstream PR was closed without merging

Comment 31

4 months ago
Pushed by amarchesini@mozilla.com:
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 33

4 months ago
Pushed by amarchesini@mozilla.com:
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

4 months ago
Pushed by amarchesini@mozilla.com:
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

4 months ago
Pushed by amarchesini@mozilla.com:
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
Upstream PR was closed without merging
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)
Depends on: 1512311
Upstream PR merged
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?
(Assignee)

Comment 41

3 months ago
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.
Flags: needinfo?(amarchesini) → needinfo?(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!
Flags: needinfo?(cmills)

Updated

2 months ago
Blocks: 1522083

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!

Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 44

2 months ago

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.

Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.