-
-
Notifications
You must be signed in to change notification settings - Fork 388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spoof Referer header of third party requests #2155
Spoof Referer header of third party requests #2155
Conversation
This would be a nice feature in most cases, but I think it's known to cause issues with some sites/widgets (see gorhill/uMatrix#68). We should definitely have a toggle for this, and perhaps a per-domain toggle as well. |
Relevant Firefox breakage study: https://blog.mozilla.org/data/2018/01/26/improving-privacy-without-breaking-the-web/ |
@@ -135,6 +135,10 @@ | |||
"message": "Replace social widgets", | |||
"description": "Checkbox label on the general settings page" | |||
}, | |||
"options_spoof_referrer_checkbox": { | |||
"message": "Spoof the Referrer header", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be useful to have this text link out to an explanation of what a "referer" header is. Also, should we spell it the English way, or the canonical way? https://en.wikipedia.org/wiki/HTTP_referer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple very minor nits. Looks good otherwise.
} else if (badger.isSpoofReferrerEnabled()) { | ||
if (badger.isPrivacyBadgerEnabled(tabDomain) && badger.isPrivacyBadgerEnabled(requestDomain)) { | ||
// spoof referer header for third party requests | ||
const refererHeader = details.requestHeaders.find(header => header.name === "Referer"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do a case-insensitive check here? header.name.toLowerCase() == "referer"
I know for a fact that when the referrer header is spoofed/wiped out for third-parties, embedded private Vimeo videos which can only be embedded on certain site/domains, can't be reproduced, as Vimeo gets the site/domain from the referrer header. More info here and here. So, on this matter, I would go with what I said some days ago:
With "third-party cookie-blocked domains" being the ones already blocked by PB. |
@popcornenthusiast interesting, thanks for bringing that up. Maybe we should hold off on this for a bit. We do already remove the referer from cookieblocked domains: https://github.com/EFForg/privacybadger/blob/master/src/js/webrequest.js#L229 |
@bcyphers Oh, you're welcome! And...
Didn't know that. That's great! But, see, instead of spoofing/wiping out the referrer header from every third-party request, why not wipe it out only from every cookie-blocked domain (already done, like you said) AND from requests for css and fonts from third-parties? It probably wouldn't break anything, this way. Also, is it possible to wipe it out from preconnect/prefetch look-a-like requests? |
I don't mind these change being made to this PR, or if someone wants to work from this that's fine to. I don't have the time to work on this anymore. |
Vimeo not working when cookieblocked is tracked in #188. |
@popcornenthusiast we could use the resource type of the request to decide whether to send referer. It looks like Vimeo embeds are loaded as the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Left code logic feedback. Still need to consider the overall feature some more.
To recap ... This enforces origin-only Referer values for third-party GET requests, and strips Referer values entirely for third-party PUT requests. Is this right? What's the rationale behind stripping Referer from PUTs?
Do we have more examples of what this feature might break? Do any privacy tools besides uMatrix provide similar functionality?
Edit: Brave does:
Referrer values are modified in the following ways
- cross-origin requests for iframes and sub-resources have their referrer set to be the origin of the requested resource
- cross-origin navigations have no referrer at all
Brave also keeps a list of referrer exceptions. Vimeo appears exempt, for instance.
Edit2: Brave's wiki seems out-of-date: brave/brave-browser#3422
@@ -172,6 +172,23 @@ function onBeforeSendHeaders(details) { | |||
} else { | |||
return {}; | |||
} | |||
|
|||
} else if (badger.isSpoofReferrerEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be an "else if", just an "if", since the "if this isn't a third-party request" block above always exits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move this entire referrer spoofing block down in onBeforeSendHeaders
callback.
We don't want to do this when Badger is disabled, and we already have a check for this later on in the callback (if (!badger.isPrivacyBadgerEnabled(tabDomain)) ...
). We don't want to do this when Badger "cookieblocks" a request (Refer gets stripped there already), or when Badger ends up blocking the request (when the request happens to register as third strike thanks to cookie headers).
We do want this to work in concert with DNT header injection though, so it makes sense for things that alter headers to live next to each other.
@@ -172,6 +172,23 @@ function onBeforeSendHeaders(details) { | |||
} else { | |||
return {}; | |||
} | |||
|
|||
} else if (badger.isSpoofReferrerEnabled()) { | |||
if (badger.isPrivacyBadgerEnabled(tabDomain) && badger.isPrivacyBadgerEnabled(requestDomain)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't check requestDomain
here, right? Privacy Badger's site whitelist is for first-party (site) domains.
if (details.method === "GET") { | ||
// spoof referer value | ||
const requestUrl = new URL(details.url); | ||
refererHeader.value = requestUrl.origin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we already have the origin in the requestDomain
variable defined above.
} else if (badger.isSpoofReferrerEnabled()) { | ||
if (badger.isPrivacyBadgerEnabled(tabDomain) && badger.isPrivacyBadgerEnabled(requestDomain)) { | ||
// spoof referer header for third party requests | ||
const refererHeader = details.requestHeaders.find(header => header.name === "Referer"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: refer_header
(lowercase with underscores for for non-object/non-object-property variables).
refererHeader.value = requestUrl.origin; | ||
} else { | ||
// remove referer header from non-GET request | ||
const refererHeaderIndex = details.requestHeaders.indexOf(refererHeader); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: referer_header_index
That sounds good! Then, instead of not sending the referrer only to css and fonts requests, it would be possible to also don't send it to beacon, ping and speculative requests without breaking anything (I'm still not sure about image, imageset and object). Also, does "speculative" takes care of dns-prefetch requests?
Not sure about this, though. Would not it allow tracking from every iframe embedded objects? Vimeo wouldn't get broken anymore, but then the problem would be with tracking. |
@popcornenthusiast What I'm proposing would only allow referrers for But bringing the conversation back to this pull request: since Vimeo is already on the yellowlist, it seems like users are already suffering the negative consequences of Privacy Badger blocking referers, at least in that case. We can fix that issue separately, but I don't think it should affect how we deal with this PR. I'm in favor of merging this in, then dealing with the broken-site issues as they come in. Based on the Mozilla report, I don't think there should be very many. |
@bcyphers Yes, you're right. When I made my first comment I didn't know that Vimeo (and any other service that works the same way) was already broken, and then I just got carried way. |
Besides breaking some Vimeo videos, removing the referrer header also breaks some (?) |
Closing as per #2155 (comment) |
This PR will spoof the
Referer
header of all third partyGET
requests.For other third party requests, the
Referer
header is removed - this is the same as what uMatrix does.See this comment in uMatrix and the implementation in uMatrix
Fixes #855 except that spoofing is mandatory, there is no option to turn it off.
Should an option be added for this? Is it not needed?