Skip to content
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

strips x-client-data headers from outgoing requests #2549

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ablanathtanalba
Copy link
Contributor

@ablanathtanalba ablanathtanalba commented Feb 11, 2020

Evidence shows that the x-client-data header in GET requests that Chrome sends could be used for tracking.

This change strips all x-client-data headers from outgoing requests when Privacy Badger is enabled, the user is on Chrome or some Chromium browser, and the option is toggled on.

Copy link
Member

@ghostwords ghostwords left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are two more places where we might want to remove the header: first-party requests when Badger is enabled, and third-party requests that are not yet cookieblocked. So ... we always want to remove x-client-data, unless Privacy Badger is disabled.

src/js/webrequest.js Outdated Show resolved Hide resolved
@ablanathtanalba ablanathtanalba force-pushed the remove-x-client-data-header branch 5 times, most recently from 90f2fda to 0365ff7 Compare February 19, 2020 03:41
@ablanathtanalba ablanathtanalba changed the title strips x-client-data headers from HTTP requests strips x-client-data headers from outgoing requests Feb 19, 2020
@ablanathtanalba

This comment has been minimized.

@ghostwords

This comment has been minimized.

@ablanathtanalba

This comment has been minimized.

@ghostwords
Copy link
Member

ghostwords commented Jun 18, 2020

Some X-Client-Data discussion here and here.

@ghostwords

This comment has been minimized.

@ablanathtanalba ablanathtanalba force-pushed the remove-x-client-data-header branch from 41f30b0 to a3ae9f8 Compare September 30, 2021 00:55
@ablanathtanalba
Copy link
Contributor Author

Refreshed and rebased this branch, including adding a setting in the options page for user's to toggle this on/off. I'm not sure yet what kind of supplementary information that toggle setting might need to let the user know what's going on (a helper tooltip? a link outwards to some credible article that lays out why x-client-data is fishy?)

@@ -129,6 +129,10 @@
"message": "Prevent WebRTC from leaking local IP address",
"description": "Checkbox label on the general settings page"
},
"options_x_client_data_setting": {
"message": "Remove \"x-client-data\" header from outgoing requests",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should phrase this in a way that's intelligible to non-technical people. Take a look at our existing privacy overrides. Something like Disable sending [NON TECHNICAL DESCRIPTION OF VARIATIONS HEADER DATA] to Google ("X-Client-Data header").

And, yes, exactly, we should also have a "learn more" icon that links to somewhere helpful.

src/js/background.js Outdated Show resolved Hide resolved
@@ -168,6 +168,22 @@ function loadOptions() {
});
}

// only show the x-client-data header setting if in Chrome & Chromium browsers
// TODO: more accurate way to determine this is a Chrome or Chromium browser
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

X-Client-Data is not in Chromium, only Chrome, I think.

We could try doing feature detection (always better than guessing based on UA). Something like, on Privacy Badger startup, make a dummy request to a Google domain. This request should get cancelled, but before it does, we'll see the headers and set our internal xClientDataHeaderDetected flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants