1222516 - Implement rel=noopener

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Anne (:annevk)]

The rel=noopener feature is rel=noreferrer without the no `Referer` header part. There's a related feature for window.open() that nukes the return value and has the same effect otherwise that we should also implement.

See https://github.com/whatwg/html/pull/290 for context.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Implement support for rel=noopener on links

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove dead codepath for the case when we opened a window even though popupState was openAbused, since we never open a window in that situation

Comment 3

[Boris Zbarsky [:bzbarsky]]

Those patches implement the "rel" value, and do some prep work for the window.open bits, but actually implementing the window.open bits is ... complicated.  In particular, the interaction with the popup blocker is complicated: we want these things to count as popups, but right now the popup blocker code needs to mark the newly created window as a popup.  But that means we have to hand the window out of the windowwatcher, and then the caller needs to mark it as popup but NOT return it to the calling script, based on this new window feature.  But window feature processing is all inside the windowwatcher.  So we need to either propagate back the fact that this feature was present or push the popup blocker bits into the window watcher or something.  I haven't had a chance to think through which is uglier.

Comment 4

[Jayden]

Having to opt out doesn't seem like the best solution.  Most people aren't technical and are not going to realize they need to add this to urls.  Shouldn't the better solution be to have to opt in?

Comment 5

[Boris Zbarsky [:bzbarsky]]

If we were doing green-field design, yes.  But we can't just change behavior of things that are already shipping and used all over without breaking sites all over the place, unfortunately.

Comment 6

[:Atoll]

This bug was referenced by https://mathiasbynens.github.io/rel-noopener/ which contains the below of bugs filed about this issue for other browsers. I'm linking them here in case any of them have useful guidance on the ugly decision described in comment 3.

Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1222516 (you are here)
WebKit: https://bugs.webkit.org/show_bug.cgi?id=155166
Edge: https://wpdev.uservoice.com/forums/257854-microsoft-edge-developer/suggestions/12942405-implement-rel-noopener
Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=168988 (shipped in Chrome 49)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment 3 is pretty specific to how window opening is implemented in Gecko (and in particular about how window opening in Gecko interacts with embeddings like Firefox)...

What this bug _really_ needs is an owner who has time to think things through and then implement.

Comment 8

[:Atoll]

No worries, there was only a tiny chance it would help anyways.

Comment 9

[Simon Wright]

Perhaps a correct solution be to retain the current window.opener behaviour for links which stay within the same root domain, but then disable-by-default for links which cross domains? This would be consistent with many other javascript security procedures.

Comment 10

[Simon Wright]

The example of social media tools was raised as a possible cross-domain / cross-origin usage of current behaviour. In these instances, the website has probably loaded some JS resource from that social network.

Perhaps Firefox could automatically permit access to window.opener for the current origin, plus any domain/origin for which JS resources have been loaded. This would also be consistent with other javascript security procedures.

(In fact, correct me if I'm wrong but isn't this pretty much exactly how cross-origin AJAX security works today?)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Simon, see comment 4 and comment 5.

For what it's worth, I think we should top holding rel=noopener hostage to the window.open mess and just check that part in.  I'll probably split this into two bugs on Monday.

Comment 12

[Simon Wright]

Hi Boris, that's exactly what I was replying to.

Comment 13

[Boris Zbarsky [:bzbarsky]]

My point was that the answer in comment 5 applies to your proposal just as much as the one in comment 4 (and in fact, I assumed that the comment 4 proposal was in fact restricting to different-origin cases, because it's obviously a non-starter in the same-origin case).  I strongly suspect the different-origin case would also brek too many things, though.

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK, I've spun out the popup blocker cleanup bit into bug 1267338 and the window.open thing into bug 1267339.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8696307 [details] [diff] [review]
part 1.  Implement support for rel=noopener on links

And a test? Same origin opening and then using broadcast channel for communication might be quite simple setup for it.

Comment 16

[Manish Goregaokar [:manishearth]]

Boris, are you still working on this? If not could I finish it up?

Comment 17

[Boris Zbarsky [:bzbarsky]]

I'm not currently working on this, though I keep meaning to get back to it.  Finishing this up is a bit complicated, but you're welcome to do it!

The complication is that due to the feature-detection situation we want to land bug 1267339 at the same time as this one; otherwise there is no way to feature-detect the window.open feature.  That's what I haven't had time to get back to, really.

Also, you will want to add "noopener" to the list in sSupportedRelValues in HTMLLinkElement.cpp, so the feature-detection works correctly...

Comment 18

[Manish Goregaokar [:manishearth]]

Since folks are asking about this: I'm not currently working on this, and probably won't in the future.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Implement support for rel=noopener on links

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8796378 [details] [diff] [review]
Implement support for rel=noopener on links

Turns out as soon as I wrote some tests people decided the spec is wrong...  Canceling review for now; this needs to be totally redone.  :(

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a window API for opening a window with navigation and a given docshell loadinfo to use for the navigation

This will be used to pass through information like the triggering principal and
whatnot, as well as the boolean for not sending a referrer, for rel=noreferrer
links.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add a window API for opening a window passing through a boolean indicating that no opener should be set on the result

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Rejigger our rel="noreferrer" support to avoid setting an opener altogether instead of setting one and then nulling it out

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Implement support for rel=noopener on links

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 with now-passing existing test marked as passing

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: And actually, part 3 with a stupid mistake that assertions caught fixed

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8801384 [details] [diff] [review]
part 1.  Add a window API for opening a window with navigation and a given docshell loadinfo to use for the navigation

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

::: embedding/components/windowwatcher/nsPIWindowWatcher.idl
@@ +61,5 @@
>        @param aForceNoOpener If true, force noopener behavior.  This means not
>                              looking for existing windows with the given name,
>                              not setting an opener on the newly opened window,
>                              and returning null from this method.
> +      @param aLoadInfo if aNavigate is true, this allows the caller to pass in

Great documentation here - super clear. Thanks for that.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8801385 [details] [diff] [review]
part 2.  Add a window API for opening a window passing through a boolean indicating that no opener should be set on the result

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

::: dom/base/nsGlobalWindow.h
@@ +1481,1 @@
>     * @param aReturn [out] The window that was opened, if any.

Should we mention here that the window will be nullptr if noopener was set (or forced)?

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8801387 [details] [diff] [review]
part 4.  Implement support for rel=noopener on links

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

::: testing/web-platform/tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html
@@ +7,5 @@
> +<a href="support/noopener-target-2.html" rel="noopener" target="ourpopup"></a>
> +<a href="support/noopener-target-2.html" rel="noopener" target="oursubframe"></a>
> +<script>
> +var tests = [];
> +// First test the special targets

Same as with the last patch that I reviewed with tests - it took a bit of reading, but I now see what we're testing here. We should try to make it explicit with documentation where possible.

Can you please add docstrings to the 3 subtests in this file that describe exactly what scenarios are being tested? I know you've got such documentation on lines 31-32 and in the async_test strings, but they seem to build off the previous documentation (for example, "And now check..."), which hasn't really laid any groundwork for what the tests are doing.

I hope the above makes sense!

@@ +18,5 @@
> +    test.done();
> +  });
> +}
> +for (var target of ["self", "parent", "top"]) {
> +  var t = async_test("Check that rel=noopener with target=_"+target+" does a normal load");

Spaces on either side of +

@@ +42,5 @@
> +});
> +t1.step(function() {
> +  w = window.open("", "ourpopup");
> +  assert_equals(w.opener, window);
> +  Math.sin();

Math.sin()? I'm guessing this is leftover debugging stuff.

@@ +50,5 @@
> +var t2 = async_test("Check that targeting of rel=noopener with a given name ignores an existing subframe with that name");
> +var channel = new BroadcastChannel("oursubframe");
> +channel.onmessage = t2.step_func_done(function(e) {
> +  var data = e.data;
> +  assert_false(data.hasOpener);

Is it also worthwhile to make sure that the iframe hasn't been navigated anywhere?

::: testing/web-platform/tests/html/semantics/links/links-created-by-a-and-area-elements/support/noopener-popup.html
@@ +13,5 @@
> +<a rel="noopener" target="_self" id="selftarget"
> +   href="noopener-target-1.html"></a>
> +<iframe srcdoc='
> +        <a rel="noopener" target="_parent" id="parenttarget"
> +                   href="noopener-target-1.html"></a>

I don't think I understand the alignment of this line. Should we not align "href" with "rel" on line 16?

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8801438 [details] [diff] [review]
And actually, part 3 with a stupid mistake that assertions caught fixed

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

Good stuff. I especially like all of the assertions.

::: testing/web-platform/meta/html/browsers/windows/noreferrer-window-name.html.ini
@@ -1,2 @@
> -[noreferrer-window-name.html]
> -  type: testharness

Am I interpreting this correctly, that this is re-enabling a test that had been timing out?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Should we mention here that the window will be nullptr if noopener was set (or forced)?

Yes, we should.  Done.

> Am I interpreting this correctly, that this is re-enabling a test that had been timing out?

Well, the test was getting run, so it wasn't _disabled_.  It just timed out, because our implementation of "noreferrer" didn't match what the test expected (it reused an existing window when it shouldn't, per proposed spec bits).  Now it no longer times out, because we're aligning with the proposed spec bits.  ;)  But yes, this is removing our "we know this test fails" annotation, because we pass it now.

> Can you please add docstrings to the 3 subtests in this file that
> describe exactly what scenarios are being tested? 

Done.

> Spaces on either side of +

Done.

> Math.sin()? I'm guessing this is leftover debugging stuff.

Yep, removed.

> Is it also worthwhile to make sure that the iframe hasn't been navigated anywhere?

Good idea.  Done.

> I don't think I understand the alignment of this line.

It's just misaligned.  ;)  Fixed.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Implement support for rel=noopener on links

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39a7487b250f
part 1.  Add a window API for opening a window with navigation and a given docshell loadinfo to use for the navigation.  r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/8765085f6ff7
part 2.  Add a window API for opening a window passing through a boolean indicating that no opener should be set on the result.  r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c0cd94d5636
part 3.  Rejigger our rel="noreferrer" support to avoid setting an opener altogether instead of setting one and then nulling it out.  r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb691bd6d3d
part 4.  Implement support for rel=noopener on links.  r=mconley

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/39a7487b250f
https://hg.mozilla.org/mozilla-central/rev/8765085f6ff7
https://hg.mozilla.org/mozilla-central/rev/7c0cd94d5636
https://hg.mozilla.org/mozilla-central/rev/bdb691bd6d3d

Comment 35

[github]

This rel="noopener" is the wrong solution.

How many websites actually make legitimate use of window.opener() ? And shouldn't those websites actually be using ajax and service workers instead?

Here's the proper fix.

When a web page wants to allow another page script access via window.opener() that page needs to be served with a HTTP header whitelisting what domains may have script access via window.opener().

You could even default whitelist the same origin domain if it is being served over HTTPS.

When a domain attempting to use window.opener() is not in the white list, it may not have script access to the window.

That way the few sites that legitimately use this feature only need a .htaccess rule to send the header to still work, and the rest of the web has a secure default.

Fix FireFox the right way, rel="noopener" is conceptually wrong. Fix your product the right way.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Please take that concern to the spec.

That said, the problem with your proposal is that it breaks backwards compat and hence would break existing sites.  That makes it quite difficult to deploy in practice.

Comment 37

[github]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #36)
> Please take that concern to the spec.
> 
> That said, the problem with your proposal is that it breaks backwards compat
> and hence would break existing sites.  That makes it quite difficult to
> deploy in practice.

So for the sake of a few sites that could add a header, a ton of sites have to modify their code and if they don't, they aren't secure.

Yeah, that makes a lot of sense. While we are at it, we should bring back export ciphers too.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> So for the sake of a few sites that could add a header, a ton of sites have to modify
> their code and if they don't, they aren't secure.

Yes, backwards compat can be a pain.

That said, if "few" and "ton" could be quantified, there could be a useful discussion about the tradeoffs.  Right now there is no clear evidence that "few" is in fact few in any meaningful sense.

Comment 39

[Chris Mills [:cmills]]

I've made sure that this type is covered in the Link types reference:
https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types

I've also added a note to the Fx52 release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/52#HTML

Let me know if this is OK. Thanks!

Comment 40

[Deepak_Singla]

The rel="noopener noreferrer" tag should not be removed. If it does you are compromising your online security. The tag has bee introduced to prevent you from getting exposed to risks possessed by the target"_blank" tag. Read here to know more in detail:
https://blog.templatetoaster.com/rel-noreferrer-noopener-seo/