The default sending of HTTP referer headers is a long standing privacy issue for users. While there are lots of old bugs that touch upon this: https://bugzilla.mozilla.org/show_bug.cgi?id=55477 https://bugzilla.mozilla.org/show_bug.cgi?id=69046 https://bugzilla.mozilla.org/show_bug.cgi?id=86949 There is also a bug specific to private browsing which is not quite the same: https://bugzilla.mozilla.org/show_bug.cgi?id=467257. The referer bugs are 5+ years old, and so I decided to start a fresh bug to tackle this issue. The importance of limiting the sending of referers was recognized since its inception in RFC 1945. From that document (http://tools.ietf.org/html/rfc1945#section-10.13): Note: Because the source of a link may be private information or may reveal an otherwise private information source, it is strongly recommended that the user be able to select whether or not the Referer field is sent. For example, a browser client could have a toggle switch for browsing openly/anonymously, which would respectively enable/disable the sending of Referer and From information. Yet to date no such toggle switch exists in any Firefox UI element. Not only that, but the deep configuration options do not allow for fine-grained user control over referers, and instead the user must install an extension to achieve this. I've attached a document outlining the privacy concerns with referers, as well as standard use cases. I think it is reasonable to start with a patch that allows for more configuration options for users, in particular the proposal from the document to have a (relaxed) same-origin policy that governs whether the referer is sent, i.e. it is sent only if the host of the referer is the host of the HTTP request. I am presently working on this patch.

Please see also bug 704320 - Implement <meta name="referrer"> which talks about implementing http://wiki.whatwg.org/wiki/Meta_referrer and has a lot of discussion of issues and ideas around referrer, particularly wrt privacy Also see https://wiki.mozilla.org/Privacy/Features/Shortened_HTTP_Referer_header which has some more ideas and background

Thanks, I've seen that thread and the referer ideas surrounding Meta referrer tag and CSP, but think that these are orthogonal to giving the user greater control. I am attaching a patch shortly which stuffs lots of different policies that the user can set via network.http.sendRefererHeader. I am open to different implementation ideas, but think it's important that a patch lands that gives the user greater control than exists now.

Dan, we'd like to implement the meta referrer thing, but with user-facing controls so users can decide if they'd like to strip off the path/querystring or block referrers entirely. How does the design of your patch align with the feature Ian mentioned in comment 1?

To what feature are you referring? I am wholly supportive of the referrer meta data attribute. I'm not clear on what you have in mind in terms of using-facing controls, but presumably you mean user-facing control governed by the meta referrer attribute? Once again, I support this effort. My patch just gives advanced users options to affect how referrers are sent irrespective of any changes made by websites or policies that exist in documents. Is there a reason why the two ideas can't exist in parallel?

Dan, they can't be parallel *and* orthogonal. It sounds like you wanna give users ability to control *when* the referrer is sent (not what parts). Can we work it into the "Shortened HTTP Referrer Header" proposal Ian linked above -- and do both? I'd like to have all the referrer-control stuff done as a cohesive unit instead of picking off one use case at a time.

Ah, I think there was a miscommunication -- I thought you were referring to the meta referrer project, which I think is orthogonal to the project of creating referrer controls that are strictly user-facing (and hence the two can be worked on in parallel :). As far as the "Shortened HTTP Referrer Header" proposal, I agree these two efforts should be combined. I'd like to give users to ability to control when the referrer is sent AND what is sent. Is there any reason to not give the user level of control along both of these axes? My patch includes functionality for both, but it is not yet a strict super-set of the functionality from that proposal. Beyond that written proposal, are there any active bugs or patches of which I'm unaware? If I create all requisite functionality from that proposal, I think that my patch can be seen as implementing "phase 1" from the proposal and then some. As to the question of testing, I am happy to provide a unit test as well, but would like guidance if there needs to be comprehensive testing beyond this. Please let me know if this is a reasonable way to proceed, or if I've missed efforts already underway and my code is duplicating those efforts.

Perhaps "orthogonal" is too strong with respect to the meta referrer or CSP projects. Certainly all of these efforts can share functionality, and exist together on the well laid out road map regarding what to do about privacy problems with the referrer. I am trying to implement "phase 1" of the proposal above, which I don't think should block on work being done in parallel on the meta referer project. Hope that helps put this patch in context.

For me, the problem with the current hidden pref is that it affects all sites, but I have one site that doesn't work correctly with the Referer header omitted: Ubuntu's Launchpad. So it would be useful to have a way to turn Referer off (except for same-origin embedded content—i.e. turn it off for navigation and for different-origin embedded content) and then have an exception list. Unfortunately, an exception list would be, by its nature, a power user feature only for people who can correctly diagnose when they need to add something to the exception list.

(In reply to Henri Sivonen (:hsivonen) from comment #10) > For me, the problem with the current hidden pref is that it affects all > sites, but I have one site that doesn't work correctly with the Referer > header omitted: Ubuntu's Launchpad. So it would be useful to have a way to > turn Referer off (except for same-origin embedded content—i.e. turn it off > for navigation and for different-origin embedded content) and then have an > exception list. The patch adds the following numbers to the network.sendRefererHeader pref: + // 0: never send referer + // 1: send referer for direct user action + // 2: always send referer + // 3: always spoof referer with current full URI + // 4: always spoof referer with current host + // 5: send referer ONLY when hosts match + // 6: send spoofed referer ONLY when hosts match + // 7: send just the host ONLY when hosts match Incidentally, the patch in bug 55477 does a similar thing: +#define REFERRER_NONE 0 /* Never send the referrer */ +#define REFERRER_USER_ACTION 1 /* Actions directly initiated by the user (e.g. clicking on a link) */ +#define REFERRER_INLINE_CONTENT 2 /* Images or other inline content */ +#define REFERRER_NON_HTTP 3 /* NOT USED - remains for backwards compatability */ +#define REFERRER_SAME_HOST_ONLY 4 /* Send the referrer only for requests from the same host, otherwise send no referrer. */ +#define REFERRER_3RDPARTY_PREPATH 5 /* Send the referrer only for requests from the same host, otherwise send target URI's pre-path as the referrer. */ +#define REFERRER_3RDPARTY_NO_PREPATH 6 /* Strip off the path from the referrer for 3rd party requests, otherwise leave it alone. */ +#define REFERRER_PREPATH_URI_ALWAYS 7 /* Always send the target URI's pre-path as the referrer. */ Dan, we can't have two bugs open with such similar patches. Either dupe this to bug 55477 which has many people monitoring its progress and obsolete the patch there with yours, or dupe bug 55477 to this one.

Thanks, Mardeg. The last comment from that bug is from almost 9 years ago, and so I can't imagine people are actively monitoring the progress. Given that, my sense is that duping bug 55477 to this one is probably the most productive way forward at this point. I will do that unless there are objections.

Henri, I think the ideal solution to your problem is to get Launchpad's website to stop relying on the Referer header. I've been hounding websites about this on and off for a few months and can add this to the list. In the mean time, it would be great to think about a user-defined list of exceptions, but I hope to focus on the basic functionality first so that we can get some test pilot studies out the door to better understand what breaks when referers are turned off.

(In reply to Dan Auerbach from comment #13) > Henri, I think the ideal solution to your problem is to get Launchpad's > website to stop relying on the Referer header. I've been hounding websites > about this on and off for a few months and can add this to the list. In the > mean time, it would be great to think about a user-defined list of > exceptions, but I hope to focus on the basic functionality first so that we > can get some test pilot studies out the door to better understand what > breaks when referers are turned off. In case it helps, here are the exceptions I've had to set to the default Forge policy in my RefControl extension. (Though I haven't had time to verify that they're all still required): disqus.com (Comment embeds on other sites only load if the referrer is unaltered) snopes.com osap.gov.on.ca (May not be testable without being logged in as a student) stoicstudio.com (Logged-in users are forced to the site root if the referrer is forged) www.systemshock.org

(In reply to Stephan Sokolow from comment #14) > In case it helps, here are the exceptions I've had to set to the default > Forge policy in my RefControl extension. (Though I haven't had time to > verify that they're all still required): > > disqus.com (Comment embeds on other sites only load if the referrer is > unaltered) > snopes.com > osap.gov.on.ca (May not be testable without being logged in as a student) > stoicstudio.com (Logged-in users are forced to the site root if the referrer > is forged) > www.systemshock.org How many of these work correctly if the referer is correct but restricted to the origin? I.e. if the referer would be http://subdomain.example.org:123/my-path?my-query#my-hash, then the site would receive "Referer: http://subdomain.example.org:123/".

(In reply to Brian Smith (:bsmith) from comment #15) > (In reply to Stephan Sokolow from comment #14) > > In case it helps, here are the exceptions I've had to set to the default > > Forge policy in my RefControl extension. (Though I haven't had time to > > verify that they're all still required): > > > > disqus.com (Comment embeds on other sites only load if the referrer is > > unaltered) > > snopes.com > > osap.gov.on.ca (May not be testable without being logged in as a student) > > stoicstudio.com (Logged-in users are forced to the site root if the referrer > > is forged) > > www.systemshock.org > > How many of these work correctly if the referer is correct but restricted to > the origin? I.e. if the referer would be > http://subdomain.example.org:123/my-path?my-query#my-hash, then the site > would receive "Referer: http://subdomain.example.org:123/". My default RefControl behaviour is "Forge", which means that, unless whitelisted, a request sends the root of the same domain as the referrer. (eg. Requesting http://something.akamai.net/foo/bar/baz.png would always send a referrer of http://something.akamai.net/) The domains I listed are all ones which broke under that behaviour last time I tested them. (In the case of Snopes.com, forging the referrer used to break navigation as if they were trying to somehow CSRF protect GET requests but now it just shows that their image hotlinking protection is so overzealous that it considers the site root an invalid referrer)

This patch implemented the user global control (phase 1) as described in: https://wiki.mozilla.org/Privacy/Features/Shortened_HTTP_Referer_header. Move the patch over from bug 704320 to here for review and feedback.

Owen, nice work. One quick point of feedback at a high level: if you look at my patch it allows user control over other dimensions of the referer as well, beyond what portion of the referer is sent. In particular, it allows a user to spoof a referer (send the destination URL as the referer URL), and also to send the referer conditionally, only when the destination host matches the referer host. I think there is no reason not to put all of this in the same patch. That way, we will be able to do a user pilot study that tests configurations like "only send shortened referer consisting of host when referer host is same as destination host." I strongly suspect such a configuration will not disrupt user experience on the vast majority of websites. I care much more about this functionality getting into Firefox than about using my patch vs yours. But perhaps we could team up, or at least have a quick discussion about it? I have partially cleaned up my patch (and used different settings for each of the dimensions above, instead of just stuffing them all in a single 1-9 setting) but haven't posted it yet since I hadn't added in the port option. My sense is that we will streamline the review process by combining efforts here. I'm dtauerbach@gmail.com on jabber, and dtauerbach on IRC (currently on OFTC). Thanks!

(In reply to Dan Auerbach from comment #18) > Owen, nice work. One quick point of feedback at a high level: if you look at > my patch it allows user control over other dimensions of the referer as > well, beyond what portion of the referer is sent. In particular, it allows a > user to spoof a referer (send the destination URL as the referer URL), and > also to send the referer conditionally, only when the destination host > matches the referer host. I think there is no reason not to put all of this > in the same patch. That way, we will be able to do a user pilot study that > tests configurations like "only send shortened referer consisting of host > when referer host is same as destination host." I strongly suspect such a > configuration will not disrupt user experience on the vast majority of > websites. > I can confirm that, for the vast majority of websites, forging a referrer pointing to the root of the requested page's domain works perfectly. I've currently got that behaviour set up using the RefControl extension and, in the year or two since I've installed it, I've only needed to whitelist 6 sites. (Disqus seems to use the referrer to determine which comment thread to load despite the Javascript embed containing all the necessary information, a couple of sites have overzealous image hotlinking protection, and a couple of forums I've visited trap you in the site root if you pass through the login process while forging the referrer.)

Stephan, thanks for that update -- it is very valuable to get feedback from folks using RefControl. Perhaps if it is only 6 sites, we can get them to change their behavior. Working with Owen and his collaborators, I've uploaded an updated patch that includes functionality of the above two but is a bit cleaned up. Owen please let me know if I am missing something, else we can obsolete your patch above, since it should be integrated into this one. In particular user preferences have been added for: 1. Sending partial referer levels: full, scheme+host+port+path, scheme+host+port, host 2. Sending spoofed referer field with destination URI 3. Setting policy to only send referer when hosts match I've decided to use xpcshell test instead of Mochitest based on the documentation which suggests one should try to avoid Mochitest if possible (https://developer.mozilla.org/en-US/docs/Mochitest says "Try to avoid Mochitest"). The xpcshell tests are nice because they don't even require a request to actually be sent.

Well, after using RefControl for a year with a default behavior to do not send referer to third party sites - I have only one exception in my list: forum.top.rbc.ru I do not use disqus, however.

Comment on attachment 726379 [details] [diff] [review] Updated patch to add user controlled referer options Review of attachment 726379 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good, some notes below. Please address the comments, then flag :jduell for review/feedback. ::: modules/libpref/src/init/all.js @@ +895,5 @@ > > // Headers > pref("network.http.accept.default", "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8"); > + > +// Headers allowing granular control of referers Nit: this comment suggests the following things are all headers that allow the control. You probably want something like "prefs to allow granular control..." @@ +900,2 @@ > pref("network.http.sendRefererHeader", 2); // 0=don't send any, 1=send only on clicks, 2=send on image requests as well > +pref("network.http.refererHeaderSource", 0); // 0=real referer, 1=spoofed referer Make this a bool and call it "spoofReferrer" @@ +900,4 @@ > pref("network.http.sendRefererHeader", 2); // 0=don't send any, 1=send only on clicks, 2=send on image requests as well > +pref("network.http.refererHeaderSource", 0); // 0=real referer, 1=spoofed referer > +pref("network.http.refererHeaderPartial", 0); > +pref("network.http.refererHeaderPolicy", 0); What are the valid values for these other two? I think since there's gonna be lots of referrer prefs here, make a new branch: network.http.referer.{spoofSource, trimmingPolicy, sendAcrossOrigins} But this raises some questions: how will this map to a meta referrer implementation (and to potential UI)? Will people use these as separate dimensions, or exclusively? Would hate to use a lock-pick set to open an unlocked door here... ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +854,5 @@ > + int userReferrerLevel = gHttpHandler->ReferrerLevel(); > + > + // 0: use real referrer > + // 1: spoof with URI of the current request > + int userReferrerSource = gHttpHandler->ReferrerSource(); There are only two options, this should be a boolean. @@ +864,5 @@ > + int userPartialReferrerLevel = gHttpHandler->PartialReferrerLevel(); > + > + // 0: send referer no matter what > + // 1: send referer ONLY when hosts match > + int userReferrerPolicy = gHttpHandler->ReferrerPolicy(); There are only two options, this should be a boolean. @@ +977,4 @@ > // (2) keep a reference to it after returning from this function > // > // Use CloneIgnoringRef to strip away any fragment per RFC 2616 section 14.36 > + Nit: adding extraneous whitespace here, don't do that please. @@ +993,5 @@ > + // check policy for sending ref only when hosts match > + if (userReferrerPolicy == 1 && currenthost != referrerhost) > + return NS_OK; > + > + // send spoofed referrer To be clear, this isn't an arbitrary referrer, it's sending a referrer that's equal to the target, right? @@ +1009,5 @@ > > nsAutoCString spec; > + > + // check how much referer to send > + if (userPartialReferrerLevel == 0) { This looks like a good place to use a switch statement. @@ +1029,5 @@ > + if (queryStart != -1) { > + int32_t queryLen = path.Length() - queryStart; > + path.Cut(queryStart, queryLen); > + } > + spec = prepath + path; There's gotta be an easier way to do this, perhaps with nsIURL. Check out: http://mxr.mozilla.org/mozilla-central/source/content/base/src/Link.cpp#356 and http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIURL.idl

> @@ +900,2 @@ > > pref("network.http.sendRefererHeader", 2); // 0=don't send any, 1=send only on clicks, 2=send on image requests as well > > +pref("network.http.refererHeaderSource", 0); // 0=real referer, 1=spoofed referer You'll really want to support sending on image requests but not clicks. All the situations I've found where sites require sending on clicks can be argued to be broken behaviours, but it'll be hard to convince sites to drop their referrer-based image hotlinking protection and overzealous ones already consider "no referrer" to be a hotlinking attempt. I'd suggest treating it like a bitfield. 0 = Don't send any 1 = Send on clicks 2 = Send on image requests 3 = Send on clicks and image requests That way, it'd possible to work toward a compromise like "Never send referrers on links, send referrers on image requests within the same origin" which could eventually be a Just Works™ default behaviour with sufficient evangelism.

Comment on attachment 726379 [details] [diff] [review] Updated patch to add user controlled referer options Review of attachment 726379 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the quick review, updating with new patch soon. I hope that responding to your review in this way is appropriate, please let me know if I should be doing something else instead. ::: modules/libpref/src/init/all.js @@ +895,5 @@ > > // Headers > pref("network.http.accept.default", "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8"); > + > +// Headers allowing granular control of referers Done @@ +900,2 @@ > pref("network.http.sendRefererHeader", 2); // 0=don't send any, 1=send only on clicks, 2=send on image requests as well > +pref("network.http.refererHeaderSource", 0); // 0=real referer, 1=spoofed referer Done @@ +900,4 @@ > pref("network.http.sendRefererHeader", 2); // 0=don't send any, 1=send only on clicks, 2=send on image requests as well > +pref("network.http.refererHeaderSource", 0); // 0=real referer, 1=spoofed referer > +pref("network.http.refererHeaderPartial", 0); > +pref("network.http.refererHeaderPolicy", 0); Added values. Left cross-origin policy as an int since we may want to expand the set of options here. Regarding the relation to meta referer implementation, I'm not sure I completely understand your second question and last analogy, but will try to respond. I think user prefs should always take precedence over meta referer tags, and users should have fine-grained control regardless of meta referer policies. Because of that, I think it's sensible to flesh out the user prefs first. My sense is that the meta referer implementation could share some code and there may be more opportunities to more deeply integrate the two efforts, but that we can better evaluate that after this patch lands. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +854,5 @@ > + int userReferrerLevel = gHttpHandler->ReferrerLevel(); > + > + // 0: use real referrer > + // 1: spoof with URI of the current request > + int userReferrerSource = gHttpHandler->ReferrerSource(); Done @@ +864,5 @@ > + int userPartialReferrerLevel = gHttpHandler->PartialReferrerLevel(); > + > + // 0: send referer no matter what > + // 1: send referer ONLY when hosts match > + int userReferrerPolicy = gHttpHandler->ReferrerPolicy(); We will probably want more options here so I'd prefer to leave it as an int, at least until the end of the code review process. If the patch is blessed otherwise and it is important to not have int options for a bool, I can switch it to a bool, and the back to an int in a later patch if needed. @@ +977,4 @@ > // (2) keep a reference to it after returning from this function > // > // Use CloneIgnoringRef to strip away any fragment per RFC 2616 section 14.36 > + Done @@ +993,5 @@ > + // check policy for sending ref only when hosts match > + if (userReferrerPolicy == 1 && currenthost != referrerhost) > + return NS_OK; > + > + // send spoofed referrer Correct @@ +1009,5 @@ > > nsAutoCString spec; > + > + // check how much referer to send > + if (userPartialReferrerLevel == 0) { Done @@ +1029,5 @@ > + if (queryStart != -1) { > + int32_t queryLen = path.Length() - queryStart; > + path.Cut(queryStart, queryLen); > + } > + spec = prepath + path; Done

Stephan, I agree with your ideas for changing sendRefererHeader. However, I'm hesitant to mess with that functionality without knowing the dependencies and if anything will break. For that reason I think doing it as a separate patch makes sense so that we can roll it back separately if needed.

Dan: In principle, your response technique is fine, but you should include in comments (>) the requests you're responding to, not just the code. You can also just omit the sections you agree with and say "implemented requested things". :) Shortcut. Regarding the note about "we will probably want more options here"... what else would we want? Could you just build them into this patch?

Ah yes, didn't realize your comments weren't going to show up, apologies :/ As far as the options for the refererXOrigin policy, right now we have an option not to send when hosts do not match. But the user might also want to send when the base domain is the same, even if hosts are different. This could be accomplished using more fine-grained URI parsing tools (e.g. https://developer.mozilla.org/en-US/docs/Code_snippets/URI_parsing). I think getting the right policies here might require some user feedback about what breaks, but I'll try to at least put in the base domain option for now and will remove the review flag until that's done. There may need to be future patches to expand this pref and possibly make it more sophisticated -- I'll need to give it more thought, but hope that the hosts and base-domain options are at least a good starting point.

Comment on attachment 729112 [details] [diff] [review] patch to add user-controlled options for sending referer Review of attachment 729112 [details] [diff] [review]: ----------------------------------------------------------------- This mostly looks fine, with nits. The only thing preventing me from +r is that I'm concerned about pref("network.http.referer.trimmingPolicy", 0); // 0=full,1=scheme+host+port+path,2=scheme+host+port,3=host RFC 2616 section 14.36 says the referrer should be "Referer" ":" ( absoluteURI | relativeURI ) dumping just "www.yahoo.com" in there would seem to legal only if we interpret it as a relative URI, which seems... broken. Is there some use case for 'host' without scheme that I'm missing here? Are we confident servers aren't going to just be confused by this? ::: modules/libpref/src/init/all.js @@ +900,2 @@ > pref("network.http.sendRefererHeader", 2); // 0=don't send any, 1=send only on clicks, 2=send on image requests as well > +pref("network.http.referer.spoofSource", false); // false=real referer, true=spoofed referer "true=spoofed (use target URI as referrer)" When comments get long might want to consider putting them on their own lines above the pref they refer to. But our usual rules about 80-width columns seems relaxed with prefs files. @@ +900,3 @@ > pref("network.http.sendRefererHeader", 2); // 0=don't send any, 1=send only on clicks, 2=send on image requests as well > +pref("network.http.referer.spoofSource", false); // false=real referer, true=spoofed referer > +pref("network.http.referer.trimmingPolicy", 0); // 0=full,1=scheme+host+port+path,2=scheme+host+port,3=host as mentioned I'm skeptical about option 3 here. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +850,5 @@ > > + // 0: never send referer > + // 1: send referer for direct user action > + // 2: always send referer > + int userReferrerLevel = gHttpHandler->ReferrerLevel(); make this an unsigned type: else I'm getting a warning (and we treat warnings as errors in necko) where you compare it with referrerLevel, below. @@ +871,2 @@ > if (!referrer) > return NS_OK; put the !referrer check above the code you added to fetch the new prefs. @@ +885,5 @@ > bool match; > > + // Get the charset of the original URI so we can pass it to our fixed up URI. > + nsAutoCString charset; > + referrer->GetOriginCharset(charset); why did you move charset from declared in if (wyciwyc) {..} to function scope? You don't use it anywhere else. No point cluttering 200+ line function with more function-scope variables. Move back into if scope. @@ +915,3 @@ > // Replace |referrer| with a URI without wyciwyg://123/. > rv = NS_NewURI(getter_AddRefs(referrerGrip), > Substring(path, slashIndex + 1, pathLength - slashIndex - 1), favor: please remove "gopher" from referrerWhiteList. Looks like we missed that when we removed support for gopher. I don't want to open a whole bug just to remove that one line. @@ +989,5 @@ > + rv = clone->GetAsciiHost(referrerHost); > + if (NS_FAILED(rv)) return rv; > + > + // check policy for sending ref only when hosts match > + if (userReferrerXOriginPolicy == 2 && currentHost != referrerHost) our String classes are so poorly documented that I had to poke around to figure out if operator != is implemented. Replace this with !currentDomain.Equals(referrerDomain) so no one else has to do that again :) It's what we do elsewhere in file (like in SecureXSiteReferrer check a few lines higher). @@ +995,5 @@ > + > + nsAutoCString currentDomain = currentHost; > + nsAutoCString referrerDomain = referrerHost; > + uint32_t extraDomains; > + nsCOMPtr<nsIEffectiveTLDService> eTLDService = do_GetService(NS_EFFECTIVETLDSERVICE_CONTRACTID); no lines longer than 80 chars, please. @@ +1003,5 @@ > + rv = eTLDService->GetBaseDomain(clone, extraDomains, referrerDomain); > + if (NS_FAILED(rv)) return rv; > + } > + > + // check policy for sending only when main domain matches. s/main/top level/ @@ +1005,5 @@ > + } > + > + // check policy for sending only when main domain matches. > + // this falls back on using host if eTLDService does not work > + if (userReferrerXOriginPolicy == 1 && currentDomain != referrerDomain) We don't want to go through the expense of computing the eTLD domains unless we're actually going to use them. So please wrap up all the code from the currentDomain declaration to here within an "if (userReferrerXOriginPolicy == 1) {...}" block. also replace != with !Equals here when you do test @@ +1031,5 @@ > + // scheme+host+port+path > + nsAutoCString prepath, path; > + rv = clone->GetPrePath(prepath); > + if (NS_FAILED(rv)) return rv; > + extra whitespace on blank line here @@ +1061,5 @@ > + // full URI > + rv = clone->GetAsciiSpec(spec); > + if (NS_FAILED(rv)) return rv; > + break; > + } trailing whitespace after } and after (spec); @@ +1066,4 @@ > > // finally, remember the referrer URI and set the Referer header. > mReferrer = clone; > + no need to add extra line here. ::: netwerk/test/unit/test_bug822869.js @@ +24,5 @@ > + > + var server_uri = "http://bar.examplesite.com/path2"; > + var server_uri_2 = "http://bar.example.com/anotherpath"; > + var referer_uri = "http://foo.example.com/path"; > + var referer_uri_2 = "http://bar.examplesite.com/path3?q=blah"; Add a "#anchor" to one or more of these so we make sure we lop it off in all cases: var referer_uri_nofrag = "http://foo.example.com/path"; var referrer_uri = referer_uri_nofrag + "#anchor"; Also, if you don't mind adding a few more test cases it would be great to have an https:// in the mix so we have test coverage there. You don't need to double the # of tests for it-just a few. @@ +27,5 @@ > + var referer_uri = "http://foo.example.com/path"; > + var referer_uri_2 = "http://bar.examplesite.com/path3?q=blah"; > + > + // tests for sendRefererHeader > + prefs.setIntPref("network.http.sendRefererHeader", 0); It would be great to have an e10s version of this test, too (see the files in the necko/test/unit_ipc directory), but I'm not sure if we're allowed to write prefs in the child process. I seem to remember child pref access being read-only. I'll ask someone. @@ +65,5 @@ > + prefs.setIntPref("network.http.referer.trimmingPolicy", 1); > + prefs.setBoolPref("network.http.referer.spoofSource", true); > + prefs.setIntPref("network.http.referer.XOriginPolicy", 2); > + do_check_eq(getTestReferrer(dest_uri, combo_referer_uri), "http://blah.foo.com:9999/spoofedpath"); > + do_check_null(getTestReferrer(dest_uri, "http://gah.foo.com/anotherpath")); thanks for all the test coverage--makes it much easier to feel good about reviewing this.

Comment on attachment 729112 [details] [diff] [review] patch to add user-controlled options for sending referer Patrick: I want to make sure you buy into the basic concept here. A peek at the all.js file is probably all you need to get the idea.

I've been using RefControl and I have only one domain to add that has not been already cited on this thread: * developer.mozilla.org => See bug 819289 (starting at comment 8, before that, we were just trying to understand the issue). Note that the issue only occurs when you're a logged-in user. David Walsh suggests that it might be a default Django policy which in turn could mean that a lot of sites will be affected, probably only logged-in users (so harder to detect & report). This might threaten the work on this bug (sorry for chiming in that late :-s).

Comment on attachment 729112 [details] [diff] [review] patch to add user-controlled options for sending referer I'm ok with this, mostly because sstamm is ok with it. In general pref'd off features aren't worth a lot but it looks like this could lay the groundwork for the meta referrer/csp approach and that would be a very good thing. If anyone still harbors concerns that this patch could make those things more difficult - please speak up!

> It would be great to have an e10s version of this test, too Turns out we can't set prefs in the child yet, so no need to try to get the test here working in the child for now. If we can get an answer on whether "Referer: host" is valid or not, and fix the nit in my review, we're good to go here.

(In reply to Jason Duell (:jduell) from comment #35) > > It would be great to have an e10s version of this test, too > > Turns out we can't set prefs in the child yet, so no need to try to get the > test here working in the child for now. > > If we can get an answer on whether "Referer: host" is valid or not, and fix > the nit in my review, we're good to go here. You can set the pref in the parent and surround pref-setting in the original test file with try/catch.

Thanks for the review! Working through your nits, and will recompile, re-test and then post the updated patch. As far as the question of host only, I am happy to remove this option since I don't think it offers a significant privacy win over scheme+host+port. That is, unless Sid wants to chime in to suggest otherwise, since that was one of the options in the referer roadmap document. Would you like me to try the e10s test as suggested by jdm? Or leave it out?

Sid, are you OK with removing the 'host'-only option for .trimmingPolicy, per comment 31? re: e10s test: > You can set the pref in the parent The problem is that the test here sets the prefs many times to test different configs. We *should* be able to rewrite it so that each pref change happens in the parent and then only the tests with those settings are run, and then the parent gets a callback to tell it to modify the next setting, etc. I wrote the functions in head.js to support that model, but I don't know of any tests that actually use it (I.e. we'd use an initial call to do_load_child_test_harness(), plus a call to send the getTestReferrer() and other boilerplate to the child, and then a bunch of calls to sendCommand('run_test_3', prepare_test4), where prepare_test4 runs on the parent, changes pref settings, then calls the next sendCommand()). See testing/xpcshell/head.js for the docs on these functions. This is a bit much to ask for, but Dan, if you want to give that a whirl, go for it. Otherwise, just give me an updated patch and I'll take a stab at doing the e10s-ification for the test. Oh, and please rename the test file something like "test_referrer.js". I'm trying to lead us away from the undescriptive "test_bug34354.js" naming convention.

Yeah, removing host-only makes sense since the RFC wants a URI.

Thanks, Sid! Jason, I was hoping to have time to tackle the e10s refactoring, but I am traveling and already pressed for time, so hope it's OK if in the interest of speed and efficiency, I pass that back to you. I have addressed all of your nits, taken out the "host-only" option, added a few https tests. (It'd be great to respond inline to your comments but last time I tried that it posted to the thread a big ugly mess, so just replying all at once here -- let me know if there is a better way in the future). I also changed some unrelated >80 char lines in the SetReferrer function in HttpBaseChannel.cpp.

Comment on attachment 739265 [details] [diff] [review] add user-controlled referer options Review of attachment 739265 [details] [diff] [review]: ----------------------------------------------------------------- > added a few https tests. But then you took away all the tests! :) The test file isn't in the latest patch--somehow the hg add must have gotten lost. Put up new version, please.

Dan, also merge this patch on top of yours (with your patch applies, just "patch -p1 <patchfile; hg qref" is the fastest way) before you resubmit--this just fixes a few minor nits I had. That and the test file and then I'll tweak it to do some e10s test coverage. Thanks.

Argh, forgot to hg add after renaming the file. Here it is again, with your patch applied.

Hi Jason, it's been quite some time since my last edit. Have you started with the e10s integration? I can try to put it back on my queue. I really think it's important to get this bug through, so let's keep communicating and figure out a plan. Thanks.

Pinging this thread again. If it makes sense for me to help with e10s, I will do so, but need to know whether to prioritize this on my end. If you are able to do it, please let me know. Thanks.

Let's make this request for information in comment 47 explicit.

Bumping this again. Jason, are you based in the bay area? Maybe we can schedule a time to meet in San Francisco and get this out the door?

Dan, so sorry about the delay. I started working on this and I'm running into a (new?) bug with the original (non-e10s) test in the patch. When I run test_referrer.js I'm getting ###!!! ASSERTION: aAdditionalParts should can't be negative and different from -1: 'aAdditionalParts == -1', file /home/jj/hg/c/t/netwerk/dns/nsEffectiveTLDService.cpp, line 300 If you can figure out why that's happening that would be great and I'll write up the e10s test. I can also take a look at the error if you have trouble figuring it out.

Thanks. I'm pretty sure that wasn't there before -- I will plan to build FF tonight and look at this tomorrow to reproduce and fix.

I'm unable to reproduce. After pulling, updating, applying the patch from attachment 779844 [details] [diff] [review], today, clobbering and building via "./mach build" without any special build options set, the xpcshell test passes (see attachment for the output). Should I be compiling with special flags? Can you try a fresh pull, update and build and see if it works for you?

Dan, what platform are you running on? I'm on Linux x86-64. I just tried a build from scratch on mozilla-central and I still get failures--somewhat randomly it's one of either TEST-UNEXPECTED-FAIL | xpcshell/head.js | [Exception... "Component returned failure code: 0x804b0050 (NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) [nsIHttpChannel.referrer]" nsresult: "0x804b0050 (NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS)" location: "JS frame :: /home/jj/hg/c/proj/browser/_tests/xpcshell/netwerk/test/unit/test_referrer.js :: getTestReferrer :: line 12" data: no] Or ###!!! ASSERTION: aAdditionalParts should can't be negative and different from -1: 'aAdditionalParts == -1', file /home/jj/hg/c/t/netwerk/dns/nsEffectiveTLDService.cpp, line 300 If you can't see these I'll try to debug on my end.

Hi Jason, apologies for the delay, was on vacation and at conferences. I'm also on Linux x86-64, Ubuntu 12.04. I haven't been able to reproduce. I can try re-building again from scratch, but perhaps it will be better to get a third person to try and build to see if they have this problem. I'm happy to meet in person to work on this; I think limiting referers is of increased importance given that this is a huge information leak that has been highlighted as a target by intelligence agencies.

(In reply to Dan Auerbach from comment #54) > I think limiting referers is of increased > importance given that this is a huge information leak that has been > highlighted as a target by intelligence agencies. Knowing for sure that governments are doing things everybody was just pretty sure they were doing a decade ago doesn't change much of anything here. While recent events may bring privacy issues like this to the forefront of our minds, I don't think there's anything here that's actually applicable in any major way. This is just yet another feature that sounded good at the time it was added but is a privacy mess that never got cleaned up. :/

OK I just rebuilt from scratch, and had to make a couple of minor changes to the files to get the test to pass: 1. Move location of "[test_referrer.js]" in xpcshell.ini 2. Remove const statements for Components.* e.g.: "const Cc = Components.classes;" I was not able to reproduce your error above. Please try to run the tests with the patch I will attach after making this comment. Also, if it is useful, before I was able to get the test to pass, it failed with the following error (printed below). I was unable to find what xpcshell return codes meant. I also was unable to figure out how to make the output more useful or verbose. Have any pointers for either of these? I was able to get the tests to psas with a crude comment-out-binary-search on the test_referrer.js file, but it'd be nice to not have to do this. dan@dan-X1:~/mercurial/mozilla-central$ ./mach xpcshell-test netwerk/test/unit/test_referrer.js From _tests: Kept 11493 existing; Added/updated 0; Removed 0 files and 0 directories. 0:09.03 INFO | Running tests sequentially. 0:09.08 TEST-INFO | profile dir is /tmp/firefox/xpcshellprofile 0:09.08 TEST-INFO | /home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/test_referrer.js | full command: ['/home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/bin/xpcshell', '-g', '/home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/bin', '-a', '/home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/bin', '-r', '/home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/bin/components/httpd.manifest', '-m', '-n', '-s', '-e', 'const _HTTPD_JS_PATH = "/home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/bin/components/httpd.js";', '-e', 'const _HEAD_JS_PATH = "/home/dan/mercurial/mozilla-central/testing/xpcshell/head.js";', '-e', 'const _TESTING_MODULES_DIR = "/home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/modules/";', '-f', '/home/dan/mercurial/mozilla-central/testing/xpcshell/head.js', '-p', '/tmp/tmpnPKVr4', '-e', 'const _SERVER_ADDR = "localhost"', '-e', u'const _HEAD_FILES = ["/home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/head_channels.js", "/home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/head_cache.js", "/home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/head_cache2.js"];', '-e', 'const _TAIL_FILES = [];', '-e', u'const _TEST_FILE = ["/home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/test_referrer.js"];', '-e', '_execute_test(); quit(0);'] 0:09.08 TEST-INFO | /home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/test_referrer.js | current directory: u'/home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit' 0:09.08 TEST-INFO | /home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/test_referrer.js | environment: ['XPCSHELL_TEST_TEMP_DIR=/tmp/tmp0lhbxC', 'XPCOM_DEBUG_BREAK=stack-and-abort', 'MOZ_CRASHREPORTER=1', 'NS_TRACE_MALLOC_DISABLE_STACKS=1', 'LD_LIBRARY_PATH=/home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/bin', 'XPCSHELL_TEST_PROFILE_DIR=/tmp/firefox/xpcshellprofile', 'MOZ_CRASHREPORTER_NO_REPORT=1'] 0:10.11 TEST-UNEXPECTED-FAIL | /home/dan/mercurial/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/test_referrer.js | test failed (with xpcshell return code: 3) 0:10.11 >>>>>>> 0:10.11 <<<<<<< 0:10.12 INFO | Result summary: 0:10.12 INFO | Passed: 0 0:10.12 INFO | Failed: 1 0:10.12 INFO | Todo: 0 0:10.12 INFO | Retried: 0

Ping. An ETA for when this can be looked at would be great. If you're still having build problems, it'd be great to pull someone else in from the Mozilla side to confirm.

OK, this is failing on try too, so it's not just me. https://tbpl.mozilla.org/?tree=Try&rev=b2ece4762c7c All failing with NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS. Also a windows build fail, but that might be transient--I'll try it again once I debug the error. (Dan, I'm taking this to debug since I can repro it and it's probably not code you're familiar with.)

Great. I'd love to get more involved with Firefox patches and contributions, so if there's anything I can learn to help fix this, that'd be great. It looks to me like on the Try server, the Linux builds/tests pass but the OS X and Windows builds have issues. Besides running my own vms, is there a way to efficiently build on these other platforms that uses Mozilla's infrastructure? Is is possible to get access to the Try server? Also, this looks potentially relevant: https://groups.google.com/forum/#!msg/python-tornado/9Bdnr_vuJvM/O-dHKunrtlUJ Let me know if I can help, but I'll leave this on your plate for now. It'd be great to finish this off soon, if possible.

Dan: you're welcome to eyeball the code to figure out why we might be getting NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS. My hunch is that running a debugger will be the easiest way to trace to the issue, and that'll be hard w/o a box in front of you that manifests the bug. But you might be able to figure it out from eyeballing things if you want to give that a try.

Definitely you should take this on if you can, Jason. I'll put it on my todo list to try to reproduce in an OS X vm, at which point I can dig in more. But I may not get to that right way. Thanks!

(In reply to Dan Auerbach from comment #60) > > is there a way > to efficiently build on these other platforms that uses Mozilla's > infrastructure? Is is possible to get access to the Try server? Hi Dan, as an aside, yes, you can get access to the Try Server with level 1 commit access. http://www.mozilla.org/hacking/commit-access-policy/ and http://www.mozilla.org/hacking/committer/ have the info on what you need to do to get level 1 access. https://wiki.mozilla.org/ReleaseEngineering/TryServer has info on using try.

Dan, This looks like the issue--your patch didn't initialize extraDomains. Presumably on your box it luckily wound up as '0' (or at least wasn't negative), so no crash. Please verify that the value should be 0. That's my read of what you're doing in the patch (i.e. you want to compare top-level domains, like bbc.co.uk, not www.bbc.co.uk).

Try run: https://tbpl.mozilla.org/?tree=Try&rev=084b7ed434d6 (with patch from bug 934805 too)

new try build with better fix for bug 934805 https://tbpl.mozilla.org/?tree=Try&rev=14398251a9c4

That is the correct initialization for extraDomains. Thanks for catching it, and apologies for the delay. Please let me know if there is a particular build process or lint to use in the future to be sure to catch these.

And the build worked and the test passed for me too.

Comment on attachment 827148 [details] [diff] [review] 822869-fixbug Review of attachment 827148 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

This is ready to land whenever inbound is open again.

is there intention to make one of the new options the default? perhaps we need to allow a composite option which balances usability and privacy such as: send full URI if base domains match trim to host otherwise

Thanks a lot for fixing this! =)

Compare bug 55477 In addition to similar options as there, it added a "fake" referer sends the current host that instead of third party hosts as referer, which makes it more compatible than no referer.

Sorry, the last sentence wasn't clear. One way is: - if source and target hosts match: send full referer - if they don't, i.e. it's a cross-origin request: send a fake referer, namely the current host's root, instead of no referer. This keeps certain hosts happy. In fact, I think that would be the best default for most users, even. I'm not sure the current prefs allow this particular combination, do they?

Eyal already filed a new bug, bug 970092 about the default. I suggest that we discuss what the defaults should be on the dev-privacy mailing list, not in bug 970092 and definitely not in this *RESOLVED* bug. See https://groups.google.com/forum/#!msg/mozilla.dev.privacy/wmPzPCdzIU8/Vrugn8XquL4J for an earlier attempt at that discussion. That message also includes a link to the project page for the overall project. You can subscribe to dev-privacy here: https://lists.mozilla.org/listinfo/dev-privacy