Closed Bug 1133201 Opened 9 years ago Closed 9 years ago

stop sending referrer information when opening plain text links

Categories

(Firefox :: Tabbed Browser, defect)

37 Branch
x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 39

People

(Reporter: mp3geek, Assigned: froydnj)

References

Details

Attachments

(4 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150213193012

Steps to reproduce:

Since the landing of https://bugzilla.mozilla.org/show_bug.cgi?id=1118502 Plain text links are passing referrer information, and it doesn't need too. 


Actual results:

Visit http://www.whatismyreferer.com/
Right click -> new tab on the whatismyreferer.com link.
Now Right select the red text http://www.whatismyreferer.com/ into a new tab
Shows referrer information in new tab.


Expected results:

For privacy reasons text links don't need refferer details, and shouldn't need to be passed.
Attached image Currently Firefox Nightly (pre-1118502) (obsolete) —
Comment on attachment 8564520 [details]
Currently Firefox Nightly (pre-1118502)

My mistake. Wrong screenshot
Attachment #8564520 - Attachment is obsolete: true
Blocks: 1118502
Bug 1118502 landed on Aurora as well. mdew, can you confirm that the bug exists in Firefox Developer Edition?
Flags: needinfo?(mp3geek)
This is like the bug that will not die.

Is this just an artifact of assuming that we have an <a> node here:

https://hg.mozilla.org/mozilla-central/diff/430a23c74542/toolkit/modules/BrowserUtils.jsm#l1.29

and we should be saying that all non-<a> nodes automatically get rel="noreferrer"?  (If so, mconley++ for suggesting we split that out into its own function.)
Yes it now affects Firefox Developer Edition; Here is the bisect's

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-win32/
/mozilla-aurora-win32/1423730449 (12-Feb-2015 13:04) GOOD
/mozilla-aurora-win32/1423753645 (12-Feb-2015 22:32) BAD

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/
/mozilla-central-win32/1423753736 (12-Feb-2015 19:38) GOOD
/mozilla-central-win32/1423781785 (13-Feb-2015 01:32) BAD
Flags: needinfo?(mp3geek)
[Tracking Requested - why for this release]:
This is a privacy leak and a regression
Status: UNCONFIRMED → NEW
Component: Untriaged → Tabbed Browser
Ever confirmed: true
OK, so it looks like here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#151

we can have a null |this.link|, while still having a non-null |this.linkURI|.
And BrowserUtils.linkHasNoReferrer says that we should pass referrer
information for null links:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/BrowserUtils.jsm#215

So this patch does the obvious thing, and just treats null links as implicitly
specifying rel="noreferrer".

But here's the weird thing: testing this manually, I don't get referrer
information passed when I do open in a new tab.  But I *do* get referrer
information when I open in a new window, or even a new private window (!).
(This is all in e10s mode, if that makes any difference.)  Do I need to be
sending noReferrer here, too, when a |node| isn't present:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#674

?
Attachment #8565508 - Flags: feedback?(mconley)
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #9)
> But here's the weird thing: testing this manually, I don't get referrer
> information passed when I do open in a new tab.  But I *do* get referrer
> information when I open in a new window, or even a new private window (!).

OK, so it looks like we need to pass information about noreferrer all the way through Services.ww.openWindow / nsWindowWatcher::OpenWindow...ugh.
Ugh. Yep. The new window case. :(

Should we just back out the original fix on 37 and fix this on 38?
Flags: needinfo?(nfroyd)
The original fix does address a regression--the window ordering thing, so unless we had a really good reason to keep the regression, I think we should just plow ahead.

On second thought, I don't think we have to fix nsWindowWatcher::OpenWindow; I think we just need to respect noreferrer here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#268

and here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#330

and possibly set noReferrer here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#665

in the null-|node| case.  (Not really sure about that last one...)

Fixing the above does seem to fix the new window case, and the tab case.  WDYT?
Flags: needinfo?(nfroyd) → needinfo?(mconley)
Tracking this privacy sensitive bug for 37+.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #12)
> The original fix does address a regression--the window ordering thing, so
> unless we had a really good reason to keep the regression, I think we should
> just plow ahead.
> 
> On second thought, I don't think we have to fix nsWindowWatcher::OpenWindow;
> I think we just need to respect noreferrer here:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> utilityOverlay.js#268
> 
> and here:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> utilityOverlay.js#330
> 
> and possibly set noReferrer here:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.
> js#665
> 
> in the null-|node| case.  (Not really sure about that last one...)
> 

I think erring on the side of not passing the referrer seems like a good policy.

> Fixing the above does seem to fix the new window case, and the tab case. 
> WDYT?

I think it sounds fine, but I think we should get another reviewer on this as well - despite my best efforts, too many defects seem to be slipping through my reviews!
Flags: needinfo?(mconley)
Let's have Gijs also review this patch when it's ready.
Comment on attachment 8565508 [details] [diff] [review]
treat null links in BrowserUtils.linkHasNoReferrer as specifying rel="noreferrer"

I think this looks like the right idea to me.
Attachment #8565508 - Flags: feedback?(mconley) → feedback+
It turns out that treating null links as being OK for passing along
referrer information means that we now pass referrer information for
plain text "links" that are opened via the context menu.  For referrer
information, we should take a much more conservative approach, and
declare that null links are always treated as if they had
rel="noreferrer".

(I realize the above paragraph and the comment in the patch are slightly
non-sensical, but I think adding null checks at the callers will just
complicate things too much...)
Attachment #8565508 - Attachment is obsolete: true
Part 1 fixed sending referrer information when opening a plain text
"link" in a new tab through the context menu.  This patch fixes the same
problem, but for the case of opening in a new window, since we take
slightly different paths through |openLinkIn| for tabs vs. windows.

(I'm not completely sure about the content.js change, but it seems reasonable,
given the changes in part 1.)
Would this include users who right click on selected text and search on a website, the referrer wouldn't be passed here? (not sure how to test for this)
(In reply to mdew from comment #19)
> Would this include users who right click on selected text and search on a
> website, the referrer wouldn't be passed here? (not sure how to test for
> this)

That's a good question!  I don't think these changes would affect that, but I don't know the code well enough to say for certain.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #20)
> (In reply to mdew from comment #19)
> > Would this include users who right click on selected text and search on a
> > website, the referrer wouldn't be passed here? (not sure how to test for
> > this)
> 
> That's a good question!  I don't think these changes would affect that, but
> I don't know the code well enough to say for certain.

Assuming I understand things correctly, searching on selected text goes through this menu item:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-context.inc#310

which means that loadSearchFromContext gets invoked:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3539

which then calls into _loadSearch:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3489

which calls openLinkIn

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#209

but since the openLinkIn call there doesn't provide referrerURI in its params argument, we don't pass a Referer header at all.  Before or after the changes in this bug.
Are these ready for review, froydnj?
Flags: needinfo?(nfroyd)
Attachment #8565617 - Flags: review?(mconley)
Attachment #8565617 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8565618 [details] [diff] [review]
part 2 - don't send referrer information when opening new windows via context menu

Yes, apparently I fail at setting review? flags.  I blame git-bz.
Flags: needinfo?(nfroyd)
Attachment #8565618 - Flags: review?(mconley)
Attachment #8565618 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8565617 [details] [diff] [review]
part 1 - treat null links in BrowserUtils.linkHasNoReferrer as specifying rel="noreferrer"

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

::: toolkit/modules/BrowserUtils.jsm
@@ +212,5 @@
>     * @param linkNode The <a> element, or null.
>     * @return a boolean indicating if linkNode has a rel="noreferrer" attribute.
>     */
>    linkHasNoReferrer: function (linkNode) {
> +    // A null linkNode typically means that we're checking a link that wasn't

My nitpickiness now wonders if this is "just" typically or really just always. AFAICT the caller in browser.js (seems to be the only one you didn't update in any way) is essentially guaranteed to have a link element. Maybe that code should assert that that's the case?
Attachment #8565617 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8565618 [details] [diff] [review]
part 2 - don't send referrer information when opening new windows via context menu

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

Shouldn't we respect the indication of not wanting the referrer passed for "save" as well?

I think it might be safer to, in the case of aNoReferrer, to null out aReferrerURI completely, so that we don't run into this again for other edgecases.
Attachment #8565618 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #25)
> Comment on attachment 8565618 [details] [diff] [review]
> part 2 - don't send referrer information when opening new windows via
> context menu
> 
> Review of attachment 8565618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Shouldn't we respect the indication of not wanting the referrer passed for
> "save" as well?
> 
> I think it might be safer to, in the case of aNoReferrer, to null out
> aReferrerURI completely, so that we don't run into this again for other
> edgecases.

(to be clear, this is in utilityOverlay.js)
(In reply to :Gijs Kruitbosch from comment #25)
> Shouldn't we respect the indication of not wanting the referrer passed for
> "save" as well?

Ooo, that's a good point.  I wonder why we care about the referrer getting passed for saving in the first place?

Looks like we wind our way through to:

http://mxr.mozilla.org/mozilla-central/source/embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp#1255

so nulling out the referrer in the save case should be safe.

> I think it might be safer to, in the case of aNoReferrer, to null out
> aReferrerURI completely, so that we don't run into this again for other
> edgecases.

Hm.  Doing that got us into part of this mess; see bug 1118502.  gBrowser.loadOneTab depends on the referrer being passed to order tabs properly.  So we decided to propagate noreferrer-ness down to loadOneTab so it could still order things correctly, but suppress referrer information when necessary.  Does that change your opinion?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #27)
> (In reply to :Gijs Kruitbosch from comment #25)
> > Shouldn't we respect the indication of not wanting the referrer passed for
> > "save" as well?
> 
> Ooo, that's a good point.  I wonder why we care about the referrer getting
> passed for saving in the first place?
> 
> Looks like we wind our way through to:
> 
> http://mxr.mozilla.org/mozilla-central/source/embedding/components/
> webbrowserpersist/nsWebBrowserPersist.cpp#1255
> 
> so nulling out the referrer in the save case should be safe.

Great!

> > I think it might be safer to, in the case of aNoReferrer, to null out
> > aReferrerURI completely, so that we don't run into this again for other
> > edgecases.
> 
> Hm.  Doing that got us into part of this mess; see bug 1118502. 
> gBrowser.loadOneTab depends on the referrer being passed to order tabs
> properly.  So we decided to propagate noreferrer-ness down to loadOneTab so
> it could still order things correctly, but suppress referrer information
> when necessary.  Does that change your opinion?

Yes! I missed that. I don't think I would have done much better than mconley here. :-\
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8565617 - Flags: review?(mconley) → review+
Comment on attachment 8565618 [details] [diff] [review]
part 2 - don't send referrer information when opening new windows via context menu

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

This looks sane.

I'm serious though - we really need tests for this stuff. There are some many variations on ways we can open and process links, it's kind of a rats nest.
Attachment #8565618 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #29)
> I'm serious though - we really need tests for this stuff. There are some
> many variations on ways we can open and process links, it's kind of a rats
> nest.

So I'm totally down with writing tests, if to make sure that we don't regress all this, after plumbing it all the way through everything.  But I do wonder how effective tests would have been at preventing this sequence of noreferrer bugs from happening in the first place.  Maybe the tab-ordering issue would have been caught, but I doubt most of the other things would have been.  I sure didn't realize how deep the rabbit hole went here...
Part 1 fixed sending referrer information when opening a plain text
"link" in a new tab through the context menu.  This patch fixes the same
problem, but for the case of opening in a new window, since we take
slightly different paths through |openLinkIn| for tabs vs. windows.

Now updated with not passing the referrer for saving links.
Attachment #8565618 - Attachment is obsolete: true
Comment on attachment 8567165 [details] [diff] [review]
part 2 - don't send referrer information when opening new windows via context menu

Apparently git-bz really does not like it when I request review from Gijs.
Attachment #8567165 - Flags: review?(gijskruitbosch+bugs)
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #32)
> Comment on attachment 8567165 [details] [diff] [review]
> part 2 - don't send referrer information when opening new windows via
> context menu
> 
> Apparently git-bz really does not like it when I request review from Gijs.

I'm going to bet on the '+' in my bugmail address + URL escaping. Might be worth filing an issue with them?


(not that I mind fewer reviews in my queue... just kidding! ;-) )
</off-topic>
Comment on attachment 8567165 [details] [diff] [review]
part 2 - don't send referrer information when opening new windows via context menu

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

::: browser/base/content/utilityOverlay.js
@@ +265,5 @@
>  
>      sa.AppendElement(wuri);
>      sa.AppendElement(charset);
> +    if (!aNoReferrer)
> +      sa.AppendElement(aReferrerURI);

I should have caught this in my first review, but reading http://hg.mozilla.org/mozilla-central/annotate/5f1009731a97/browser/base/content/browser.js#l1223 , it looks like we make assumptions about the order of things, so I *think* this should append null if aNoReferrer. Does that sound right?

r+ with that fixed or with a justification why I'm wrong (it's happened before!)
Attachment #8567165 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #34)
> ::: browser/base/content/utilityOverlay.js
> @@ +265,5 @@
> >  
> >      sa.AppendElement(wuri);
> >      sa.AppendElement(charset);
> > +    if (!aNoReferrer)
> > +      sa.AppendElement(aReferrerURI);
> 
> I should have caught this in my first review, but reading
> http://hg.mozilla.org/mozilla-central/annotate/5f1009731a97/browser/base/
> content/browser.js#l1223 , it looks like we make assumptions about the order
> of things, so I *think* this should append null if aNoReferrer. Does that
> sound right?

Good catch, that sounds plausible.  I am a little frightened that nothing complained:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dfe458ed3d4

but...
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #35)
> I am a little frightened that nothing
> complained:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dfe458ed3d4
> 
> but...

Yeah, reasonsWeShouldReallyWriteTestsHere++...
(In reply to mdew from comment #0)
> Visit http://www.whatismyreferer.com/
> Right click -> new tab on the whatismyreferer.com link.
> Now Right select the red text http://www.whatismyreferer.com/ into a new tab
> Shows referrer information in new tab.

> Expected results:
> 
> For privacy reasons text links don't need refferer details, and shouldn't
> need to be passed.

I'm not really sure I understand why this is considered a bug. I don't have a particular objection to defaulting to not sending a referrer, but it's a change in behavior that's entirely unrelated to our rel=noreferrer support, right?
It's also, as far as I can tell, not actually a regression.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #38)
> It's also, as far as I can tell, not actually a regression.

It is not a regression from release, but I believe it is a regression from 37...or possibly some versions of 38, I forget what the exact versions I tested were.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #39)
> It is not a regression from release, but I believe it is a regression from
> 37...or possibly some versions of 38, I forget what the exact versions I
> tested were.

Can you explain why that is the case? When this feature was implemented it passed a referrer, as far as I can tell (https://hg.mozilla.org/mozilla-central/rev/921c0a30c2f3#l2.56). I don't see why that behavior would have changed until now.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #42)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #39)
> > It is not a regression from release, but I believe it is a regression from
> > 37...or possibly some versions of 38, I forget what the exact versions I
> > tested were.
> 
> Can you explain why that is the case? When this feature was implemented it
> passed a referrer, as far as I can tell
> (https://hg.mozilla.org/mozilla-central/rev/921c0a30c2f3#l2.56). I don't see
> why that behavior would have changed until now.

My money is on the behavior fixed by https://bugzilla.mozilla.org/attachment.cgi?id=8560608, since that fix ensured that we passed a content URI, rather than a chrome URI, as the referrer.  (We were accidentally passing a chrome URI as referrer thanks to bug 1031264.)  I think something must have been sanitizing the referrer by rejecting chrome URIs, which meant that plain text links were effectively being treated as rel=noreferrer.  Does that seem plausible?
Flags: needinfo?(gavin.sharp)
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #43)
> My money is on the behavior fixed by
> https://bugzilla.mozilla.org/attachment.cgi?id=8560608

That behavior was only present for approximately one cycle, while 37 was on trunk/aurora (introduced by bug 1031264, fixed by bug 1118502).

If I were to summarize what happened here, I would say:
- the "open plain text link" feature has sent a referrer header since its introduction in Firefox 4 (bug 454518)
- in the Firefox 37 cycle we temporarily introduced buggy behavior that caused it to not send a referrer (via bug 1031264)
- we then fixed that buggy behavior in bug 1118502, causing us to send a referrer again
- mdew noticed the fixing of the buggy behavior, and filed this bug suggesting that this reversion to the behavior we've had since Firefox 4 was itself a newly-introduced bug.

It's fine to suggest we should change whether we send the referrer for these links, but it's misleading to call it a regression fix rather than an intentional change in behavior.
Flags: needinfo?(gavin.sharp)
Summary: Referrer Information is being passed in plain text links → stop sending referrer information when opening plain text links
Firefox 37 is marked as affected but from comments 42-44 I'm unsure if that is correct. Does this fix need to be uplifted to 37?

ni Gavin as Nathan is away.
Flags: needinfo?(gavin.sharp)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #44)
> - mdew noticed the fixing of the buggy behavior, and filed this bug
> suggesting that this reversion to the behavior we've had since Firefox 4 was
> itself a newly-introduced bug.
> 
> It's fine to suggest we should change whether we send the referrer for these
> links, but it's misleading to call it a regression fix rather than an
> intentional change in behavior.

Suggestion during the development of 1031264 https://bugzilla.mozilla.org/show_bug.cgi?id=1031264#c4 (when I first brought up the issue regarding referrer information being passed, and when 1031264 was committed the plan text referrer seemed to be fixed)

So its not a regression from a previous Firefox version, but a regression from another Firefox Bug (1118502).
It's not a "regression" at all, per comment 44.

This is an intentional behavior change that we are introducing in Firefox 39. We don't need to track it for earlier releases.
Flags: needinfo?(gavin.sharp)
Blocks: 1409248
You need to log in before you can comment on or make changes to this bug.