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

Fix cookie tracker detection #1259

Merged
merged 3 commits into from
Mar 22, 2017
Merged

Conversation

ghostwords
Copy link
Member

Fixes #1245.

@ghostwords ghostwords requested a review from cooperq March 21, 2017 16:24
@ghostwords ghostwords changed the title Fix test cookie tracker detection [WIP] Mar 21, 2017
@ghostwords
Copy link
Member Author

ghostwords commented Mar 21, 2017

@pde @cooperq @cowlicks So this was a Privacy Badger bug! Header names are case-insensitive, and cookie tracking detection code wasn't normalizing header names: f0a0fb9. Meaning Privacy Badger doesn't detect cookie tracking whenever the Cookie/Set-Cookie header is lowercase, which seems to happen consistently for this test in Firefox.

@ghostwords ghostwords mentioned this pull request Mar 21, 2017
4 tasks
@ghostwords
Copy link
Member Author

ghostwords commented Mar 21, 2017

TODOs:

  • Add unit test to cover this bug: 8112fa6
  • Check why the test_cookie_tracker_detection test was passing in Chrome. Answer: Chrome capitalizes HTTP header names, Firefox lowercases.
  • Clarify scope of breakage: It appears that Privacy Badger doesn't learn from cookie tracking in Firefox. Tested with the release-2017.1.26.1 tag in a new profile on Firefox 52.
@cowlicks
Copy link
Contributor

cowlicks commented Mar 22, 2017

Yikes! It would be good to understand how strict browsers are with accepting Set-Cookie headers. And if browsers always send cookies with a Cookie header. Could we be missing header names with whitespace in them? Or headers using an em dash instead of a normal dash like Set–Cookie. Ideally we'd have an internal browser function to validate this since diverging from its behavior could lead to some XSS or header injection.

@cowlicks
Copy link
Contributor

Apparently http header field names are supposed to be case-insensitive. https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

@ghostwords
Copy link
Member Author

ghostwords commented Mar 22, 2017

It seems like header handling changed in Firefox somewhere between versions 52.0a2 and 52.0.1. A fresh install of Privacy Badger from AMO starts blocking after three newspaper sites on 52.0a2, but not on 52.0.1.

Note that you have to manually clear Badger's storage if you reuse the profile, as uninstalling Firefox extensions doesn't perform proper cleanup. Might be easier to use a new profile for each test.

@SwartzCr SwartzCr merged commit 66c12f0 into master Mar 22, 2017
@ghostwords ghostwords deleted the fix-test_cookie_tracker_detection branch March 22, 2017 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants