1031264 - <a rel="noreferrer"> does not work if open in new tab

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[sjw]

If you open a link with the rel="noreferrer" attribute in a new tab (eg. over middle click) it will send a referrer anyway.

Comment 1

[Boris Zbarsky [:bzbarsky]]

That's handled completely in UI code, not in core; they need to look at the attribute.

Comment 2

[Rob Mueller]

Someone really needs to look into this. The current situation is almost more dangerous than not supporting rel="noreferrer" at all because it's documented incorrectly. Developers will assume that they can safely tell their users "you're safe on Firefox" and possibly remove other workarounds for Firefox 33+ when that's completely not true if the user just ctrl-click a link, it breaks rel="noreferrer".

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: make opening links in new {tabs,windows} honor rel="noreferrer"

This patch changes (I think) all of the necessary places the browser opens
links from <a> elements to honor rel="noreferrer".  I'm not overly happy about
having to replicate the logic in several different places, but I'm not sure
there's a better way to do it...maybe adding an extra |noreferrer| field to the
param object for openLinkIn instead?

Light testing in e10s and non-e10s suggests I haven't broken anything, but I
still need to run full tests.

Comment 4

[mdew]

Would noreferrer be applicable for text urls (non-links). So if someone rightclick and "Open Link..." on a url it doesn't send the referrer. Would be bonus if noreferrer on default for text urls.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8534446 [details] [diff] [review]
make opening links in new {tabs,windows} honor rel="noreferrer"

I *thought* I tagged mconley for review...apparently not.

You may want to see comment 3 for a slightly fuller explanation.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8534446 [details] [diff] [review]
make opening links in new {tabs,windows} honor rel="noreferrer"

>+    var rel = linkNode.getAttribute("rel");

let

>+    if (rel && rel.indexOf("noreferrer") != -1)

rel && rel.includes("noreferrer")?

>+    params['referrerURI'] = referrerURI;

params.referrerURI

>     let json = { button: event.button, shiftKey: event.shiftKey,
>                  ctrlKey: event.ctrlKey, metaKey: event.metaKey,
>                  altKey: event.altKey, href: null, title: null,
>-                 bookmark: false };
>+                 bookmark: false, noreferrer: false};

this shouldn't be needed

>       if (node) {
>         json.title = node.getAttribute("title");
>         if (event.button == 0 && !event.ctrlKey && !event.shiftKey &&
>             !event.altKey && !event.metaKey) {
>           json.bookmark = node.getAttribute("rel") == "sidebar";
>           if (json.bookmark) {
>             event.preventDefault(); // Need to prevent the pageload.
>           }
>         }
>+        var rel = node.getAttribute("rel");

let

>+    for (var p in extra)

let

>   openLinkInPrivateWindow : function () {
>     var doc = this.target.ownerDocument;
>     urlSecurityCheck(this.linkURL, this._unremotePrincipal(doc.nodePrincipal));
>     openLinkIn(this.linkURL, "window",
>-               { charset: doc.characterSet,
>-                 referrerURI: doc.documentURIObject,
>-                 private: true });
>+	       this._openLinkInParameters(doc, { private: true }));

indentation is off

>     openLinkIn(this.linkURL, "tab",
>-               { charset: doc.characterSet,
>-                 referrerURI: referrerURI,
>-                 allowMixedContent: persistAllowMixedContentInChildTab });
>+	       this._openLinkInParameters(doc,
>+					  { allowMixedContent: persistAllowMixedContentInChildTab }));

ditto

Comment 7

[Nathan Froyd [:froydnj]]

Thanks for the feedback.

(In reply to Dão Gottwald [:dao] from comment #6)
> >+    if (rel && rel.indexOf("noreferrer") != -1)
> 
> rel && rel.includes("noreferrer")?

I was getting errors about 'includes' not being a defined property on 'rel'.  It's possible my tree was out-of-date (~2-3 weeks or so), or I was doing something else wrong...

Comment 8

[Boris Zbarsky [:bzbarsky]]

"includes" is new.  It's also not clear (to me, at least) that it will stick around.

Comment 9

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

Comment on attachment 8534446 [details] [diff] [review]
make opening links in new {tabs,windows} honor rel="noreferrer"

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

Hey froydnj - thanks for the patch. This looks good - I just have a few suggestions to go along with Dao's.

I suggest against using Array.prototype.includes for now - we got burned before by using Array.prototype.contains in browser chrome code before it was ready, and I think we should be a bit more patient here. indexOf isn't so bad for now.

::: browser/base/content/browser.js
@@ +5701,5 @@
> +  var params = { charset: doc.characterSet,
> +                 allowMixedContent: persistAllowMixedContentInChildTab };
> +  var includeReferrer = true;
> +  if (linkNode) {
> +    var rel = linkNode.getAttribute("rel");

This pattern for checking that the node exists, that it has a rel attribute with noreferrer in it, and setting a boolean based on all of that... that's something we can probably stuff into toolkit/modules/BrowserUtils.jsm. A HasSetNoReferrer method or something. That might help reduce some of the duplication.

::: browser/base/content/nsContextMenu.js
@@ +870,5 @@
>    // Open linked-to URL in a new window.
>    openLink : function () {
>      var doc = this.target.ownerDocument;
>      urlSecurityCheck(this.linkURL, this._unremotePrincipal(doc.nodePrincipal));
> +    openLinkIn(this.linkURL, "window", this._openLinkInParameters(doc));

To increase readability and maybe reduce some of your indentation pain, it might be worth having an intermediate params variable, like:

let params = this._openLinkInParameters(doc);
openLinkIn(this.linkURL, "window", params);

That'll probably clean up the calls below as well.

::: browser/modules/ContentClick.jsm
@@ +69,5 @@
>      // Todo(903022): code for where == save
>  
> +    var params = { charset: browser.characterSet };
> +    if (!json.noreferrer)
> +      params['referrerURI'] = browser.documentURI;

params.referrerURI

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: make opening links in new {tabs,windows} honor rel="noreferrer"

OK, feedback addressed.  Let's do this again!

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8537916 [details] [diff] [review]
make opening links in new {tabs,windows} honor rel="noreferrer"

|git bz file| is failing me, apparently.

Comment 12

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

Comment on attachment 8537916 [details] [diff] [review]
make opening links in new {tabs,windows} honor rel="noreferrer"

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

Looks good - thanks froydnj!

::: browser/base/content/browser.js
@@ +5694,5 @@
>      catch (e) { }
>    }
>  
>    urlSecurityCheck(href, doc.nodePrincipal);
> +  var params = { charset: doc.characterSet,

let, not var

::: browser/base/content/nsContextMenu.js
@@ +858,5 @@
>      return aNode.spellcheck;
>    },
>  
> +  _openLinkInParameters : function (doc, extra) {
> +    var params = { charset: doc.characterSet };

let, not var

@@ +903,5 @@
>        catch (e) { }
>      }
>  
> +    let allowMixedContent = { allowMixedContent: persistAllowMixedContentInChildTab };
> +    let params = this._openLinkInParameters(doc, allowMixedContent);

Even better:

let params = this._openLinkInParameters(doc, {
  allowMixedContent: persistAllowMixedContentInChildTab,
});

::: browser/modules/ContentClick.jsm
@@ +67,5 @@
>        return false;
>  
>      // Todo(903022): code for where == save
>  
> +    var params = { charset: browser.characterSet };

let, not var

@@ +68,5 @@
>  
>      // Todo(903022): code for where == save
>  
> +    var params = { charset: browser.characterSet };
> +    if (!json.noreferrer)

nit - let's camel-casel this optional parameter to noReferrer

::: toolkit/modules/BrowserUtils.jsm
@@ +222,5 @@
> +
> +    // The HTML spec says that rel should be split on spaces before looking
> +    // for particular rel values.
> +    let values = rel.split(/[ \t\r\n\f]/);
> +    return values.some(function (e) { return e == 'noreferrer' });

I guess this is functionally equivalent to:

values.indexOf("noreferrer") != -1

however, the latter case is likely easier to grep for when we start using Array.prototype.includes. Let's go with the indexOf for now.

Comment 13

[Nathan Froyd [:froydnj]]

Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/430a23c74542

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/430a23c74542

Comment 15

[Jean-Yves Perrier [:teoli]]

Updated:
https://developer.mozilla.org/en-US/Firefox/Releases/37#HTML
and
https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types