Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement rel=noopener

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: annevk, Assigned: bz)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla52
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed)

Details

(URL)

Attachments

(4 attachments, 6 obsolete attachments)

22.18 KB, patch
mconley
: review+
Details | Diff | Splinter Review
12.88 KB, patch
mconley
: review+
Details | Diff | Splinter Review
5.59 KB, patch
mconley
: review+
Details | Diff | Splinter Review
9.07 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8696307 [details] [diff] [review]
part 1.  Implement support for rel=noopener on links
(Assignee)

Comment 2

2 years ago
Created attachment 8696308 [details] [diff] [review]
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
(Assignee)

Comment 3

2 years ago
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.

Updated

2 years ago
Blocks: 1239351
Keywords: dev-doc-needed

Comment 4

a year ago
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?
(Assignee)

Comment 5

a year ago
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.
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)
See Also: → bug 1257060
(Assignee)

Comment 7

a year ago
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.
No worries, there was only a tiny chance it would help anyways.

Comment 9

a year ago
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

a year ago
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?)
(Assignee)

Comment 11

a year ago
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

a year ago
Hi Boris, that's exactly what I was replying to.
(Assignee)

Comment 13

a year ago
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.
(Assignee)

Comment 14

a year ago
OK, I've spun out the popup blocker cleanup bit into bug 1267338 and the window.open thing into bug 1267339.
Assignee: nobody → bzbarsky
Summary: Implement rel=noopener and its window.open() friend → Implement rel=noopener
(Assignee)

Updated

a year ago
Attachment #8696307 - Flags: review?(bugs)
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.
Attachment #8696307 - Flags: review?(bugs) → review+
Boris, are you still working on this? If not could I finish it up?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 17

11 months ago
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...
Flags: needinfo?(bzbarsky)
Since folks are asking about this: I'm not currently working on this, and probably won't in the future.

Updated

10 months ago
Blocks: 1277066

Updated

10 months ago
Blocks: 1304140
(Assignee)

Comment 19

10 months ago
Created attachment 8796378 [details] [diff] [review]
Implement support for rel=noopener on links
Attachment #8796378 - Flags: review?(mconley)
(Assignee)

Updated

10 months ago
Attachment #8696307 - Attachment is obsolete: true
(Assignee)

Comment 20

10 months ago
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.  :(
Attachment #8796378 - Flags: review?(mconley)
(Assignee)

Comment 21

9 months ago
Created 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

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.
Attachment #8801384 - Flags: review?(mconley)
(Assignee)

Comment 22

9 months ago
Created 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
Attachment #8801385 - Flags: review?(mconley)
(Assignee)

Comment 23

9 months ago
Created attachment 8801386 [details] [diff] [review]
part 3.  Rejigger our rel="noreferrer" support to avoid setting an opener altogether instead of setting one and then nulling it out
Attachment #8801386 - Flags: review?(mconley)
(Assignee)

Comment 24

9 months ago
Created attachment 8801387 [details] [diff] [review]
part 4.  Implement support for rel=noopener on links
Attachment #8801387 - Flags: review?(mconley)
(Assignee)

Updated

9 months ago
Attachment #8796378 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8696308 - Attachment is obsolete: true
(Assignee)

Comment 25

9 months ago
Created attachment 8801437 [details] [diff] [review]
Part 3 with now-passing existing test marked as passing
Attachment #8801437 - Flags: review?(mconley)
(Assignee)

Updated

9 months ago
Attachment #8801386 - Attachment is obsolete: true
Attachment #8801386 - Flags: review?(mconley)
(Assignee)

Comment 26

9 months ago
Created attachment 8801438 [details] [diff] [review]
And actually, part 3 with a stupid mistake that assertions caught fixed
Attachment #8801438 - Flags: review?(mconley)
(Assignee)

Updated

9 months ago
Attachment #8801437 - Attachment is obsolete: true
Attachment #8801437 - Flags: review?(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.
Attachment #8801384 - Flags: review?(mconley) → review+
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)?
Attachment #8801385 - Flags: review?(mconley) → review+
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?
Attachment #8801387 - Flags: review?(mconley) → review+
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?
Attachment #8801438 - Flags: review?(mconley) → review+
(Assignee)

Comment 31

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

Comment 32

9 months ago
Created attachment 8803103 [details] [diff] [review]
part 4.  Implement support for rel=noopener on links
(Assignee)

Updated

9 months ago
Attachment #8801387 - Attachment is obsolete: true

Comment 33

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

9 months ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox52: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Comment 35

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

Comment 36

8 months ago
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

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

Comment 38

8 months ago
> 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.
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!
Keywords: dev-doc-needed → dev-doc-complete

Updated

3 months ago
Depends on: 1358469
Comment hidden (spam)
Blocks: 1365032
You need to log in before you can comment on or make changes to this bug.