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)
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.
Comment on attachment 8564520 [details]
Currently Firefox Nightly (pre-1118502)
My mistake. Wrong screenshot
Attachment #8564520 -
Attachment is obsolete: true
Comment 5•9 years ago
|
||
Bug 1118502 landed on Aurora as well. mdew, can you confirm that the bug exists in Firefox Developer Edition?
Flags: needinfo?(mp3geek)
Assignee | ||
Comment 6•9 years ago
|
||
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)
Comment 8•9 years ago
|
||
[Tracking Requested - why for this release]: This is a privacy leak and a regression
Status: UNCONFIRMED → NEW
status-firefox36:
--- → unaffected
status-firefox37:
--- → affected
tracking-firefox37:
--- → ?
Component: Untriaged → Tabbed Browser
Ever confirmed: true
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
(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.
Comment 11•9 years ago
|
||
Ugh. Yep. The new window case. :( Should we just back out the original fix on 37 and fix this on 38?
Updated•9 years ago
|
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 12•9 years ago
|
||
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)
Comment 13•9 years ago
|
||
Tracking this privacy sensitive bug for 37+.
Comment 14•9 years ago
|
||
(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)
Comment 15•9 years ago
|
||
Let's have Gijs also review this patch when it's ready.
Comment 16•9 years ago
|
||
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+
Assignee | ||
Comment 17•9 years ago
|
||
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
Assignee | ||
Comment 18•9 years ago
|
||
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.)
Reporter | ||
Comment 19•9 years ago
|
||
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)
Assignee | ||
Comment 20•9 years ago
|
||
(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.
Assignee | ||
Comment 21•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Attachment #8565617 -
Flags: review?(mconley)
Attachment #8565617 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 23•9 years ago
|
||
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 24•9 years ago
|
||
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 25•9 years ago
|
||
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)
Comment 26•9 years ago
|
||
(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)
Assignee | ||
Comment 27•9 years ago
|
||
(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)
Comment 28•9 years ago
|
||
(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)
Updated•9 years ago
|
Attachment #8565617 -
Flags: review?(mconley) → review+
Comment 29•9 years ago
|
||
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+
Assignee | ||
Comment 30•9 years ago
|
||
(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...
Assignee | ||
Comment 31•9 years ago
|
||
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
Assignee | ||
Comment 32•9 years ago
|
||
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)
Comment 33•9 years ago
|
||
(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 34•9 years ago
|
||
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+
Assignee | ||
Comment 35•9 years ago
|
||
(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...
Comment 36•9 years ago
|
||
(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++...
Comment 37•9 years ago
|
||
(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?
Comment 38•9 years ago
|
||
It's also, as far as I can tell, not actually a regression.
Assignee | ||
Comment 39•9 years ago
|
||
(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.
Assignee | ||
Comment 40•9 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f0a2ebf81a4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a2cf62a366f7
Assignee: nobody → nfroyd
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/9f0a2ebf81a4 https://hg.mozilla.org/mozilla-central/rev/a2cf62a366f7
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Comment 42•9 years ago
|
||
(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.
Assignee | ||
Comment 43•9 years ago
|
||
(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)
Comment 44•9 years ago
|
||
(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
Comment 45•9 years ago
|
||
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)
Reporter | ||
Comment 46•9 years ago
|
||
(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).
Comment 47•9 years ago
|
||
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.
status-firefox35:
unaffected → ---
status-firefox36:
unaffected → ---
status-firefox37:
affected → ---
status-firefox38:
affected → ---
status-firefox39:
fixed → ---
tracking-firefox37:
+ → ---
tracking-firefox38:
+ → ---
Flags: needinfo?(gavin.sharp)
You need to log in
before you can comment on or make changes to this bug.
Description
•