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

Block All Cookies in shields doesn't truly block all cookies #729

Closed
srirambv opened this issue Aug 10, 2018 · 8 comments · Fixed by brave/brave-core#589
Closed

Block All Cookies in shields doesn't truly block all cookies #729

srirambv opened this issue Aug 10, 2018 · 8 comments · Fixed by brave/brave-core#589
Assignees
Labels
bug feature/global-settings Settings at browser level independent of shields settings feature/shields The overall Shields feature in Brave. priority/P3 The next thing for us to work on. It'll ride the trains. privacy/tracking Preventing sites from tracking users across the web privacy QA/Yes release/blocking

Comments

@srirambv
Copy link
Contributor

Description

Block All Cookies in shields doesn't truly block all cookies

Steps to Reproduce

  1. Start with a clean profile on 0.53.2 with default global settings
  2. Visit a site and change Cookie setting to Block all Cookies in shields
  3. Wait for the page to reload, click on the connection info, popup shows Cookies (x in use) (x is number of cookies in used, varies from site to site)
  4. Open Settings->Privacy->Content Settings-> Cookies -> toggle to blocked
  5. Reload the page from step 3 and click on the connection info, popup shows Cookies (0 in use)

Actual result:

Chrome vs Brave
block cookies

Expected result:

Shields settings should override the global settings for Cookies.

Reproduces how often:

100% on all sites

Brave version (about:brave info)

0.52.3

Reproducible on current release:

N/A

Website problems only:

  • Does the issue resolve itself when disabling Brave Shields?
  • Is the issue reproducible on the latest version of Chrome?

Additional Information

#402 #226
cc: @bbondy @bridiver @diracdeltas

@srirambv srirambv added bug feature/shields The overall Shields feature in Brave. release/blocking feature/global-settings Settings at browser level independent of shields settings labels Aug 10, 2018
@srirambv srirambv added this to the Releasable builds 0.55.x milestone Aug 10, 2018
@bbondy
Copy link
Member

bbondy commented Sep 10, 2018

Can reproduce on https://www.cnet.com/

@ltilve ltilve self-assigned this Sep 12, 2018
@ltilve
Copy link

ltilve commented Sep 21, 2018

I have been digging into the issue, and on how all the cookies were being blocked and was a bit confused because all the functionality seemed that it should be working as expected.

Once the shield flag was changed and the view was reloaded, there were just a subset of the Cookies shown as (in use) on the connection details. Once the tab was closed and reopened all were blocked as they should. While all the header management for the cookies was seeming to be correctly working, the elements that were not blocked were duplicate, and had been set directly with a direct document.cookie JS call.

So I will be ensuring from the extension instead that it gets cleaned after enabling the Block All Cookies shield flag.

@bbondy
Copy link
Member

bbondy commented Sep 22, 2018

I'm going to move this to 1.x because I think it is working as expected, but it just doesn't clear already existing cookies.

@bbondy bbondy modified the milestones: Releasable builds 0.55.x, 1.x Backlog Sep 22, 2018
@rebron rebron added priority/P3 The next thing for us to work on. It'll ride the trains. and removed release/blocking labels Sep 28, 2018
@diracdeltas
Copy link
Member

I think this should block the releasable builds milestone since it's a privacy bug. Users who select 'block all cookies' from shields would expect it to not send any cookies.

@tildelowengrimm tildelowengrimm added privacy release/blocking privacy/tracking Preventing sites from tracking users across the web labels Sep 28, 2018
@tildelowengrimm tildelowengrimm modified the milestones: 1.x Backlog, 1.0 (0.56.x) Sep 28, 2018
@bbondy
Copy link
Member

bbondy commented Sep 29, 2018

I think Yan wanted in Releasable builds milestone and not 1.0, so bumping it up there.

@bbondy bbondy modified the milestones: 1.0 (0.56.x), Releasable builds 0.55.x Sep 29, 2018
@ltilve
Copy link

ltilve commented Oct 5, 2018

So I have been investigating this a bit more, but haven't been able to get a fix for it. The problem seems to be happening with just the uncleaned cookies set as commented, for the case of etherscan.io on just on cflb.
blocked2
allowed2

However, as commented this is only affecting the tab reload happening called from the extension changing the setting (or manually reloading the tab). If the tab is closed and recreated, the bubble shows correctly just all the blocked cookies.

I had been trying but wasn't able to fix it by manually calling a js script from the extension to cleanup that tab content when the combo option was modified but couldn't manage to have it working. On the other hand, I was still digging on what settings/content/cookie flag could be doing differently, and how that could eventually have different behaviour on TabSpecificContentSettings.

@diracdeltas
Copy link
Member

i think this should not have been auto-closed

@diracdeltas diracdeltas reopened this Oct 8, 2018
@diracdeltas
Copy link
Member

[probably related, but not a blocker] - appears that turning on the 'allow all cookies' switch doesn't allow all cookies either. STR:

  1. go to https://www.whatismybrowser.com/detect/are-third-party-cookies-enabled
  2. it should say 3p cookies are blocked
  3. switch the cookie setting in shields to 'allow all'
  4. the page reloads and still shows 3p cookies blocked

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feature/global-settings Settings at browser level independent of shields settings feature/shields The overall Shields feature in Brave. priority/P3 The next thing for us to work on. It'll ride the trains. privacy/tracking Preventing sites from tracking users across the web privacy QA/Yes release/blocking
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants