1530132 - Whitelisted site allows all cookies requested by site to be saved when blocking all cookies

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, defect)

Description

[nevsjs]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.119 Safari/537.36

Steps to reproduce:

Versions tested: 65.0.1, 67.0a1

1. Create new profile
2. In 'Privacy & Security', select custom option to block all cookies
3. Set site 'https://microsoft.com' to 'Allow' in 'Manage Permissions...'
4. Visit site 'https://microsoft.com/'
5. Open privacy options again and open 'Manage Data...' to view sites with saved data

Actual results:

Cookies are saved for websites not in whitelist. eg. In the case above, cookies from live.com & bing.com (plus others) are saved when these have not been explicitly allowed.

Expected results:

Only the cookies from microsoft.com should've been saved. This was the behavior of the 'block all cookies' setting prior to version 65.0.

Comment 1

[Junior [inactive]]

It seems by design. ni? ehsan to confirm

Comment 2

[(no longer active)]

Hi David,

Is it possible to get a regression range on this bug to see which changeset regressed it please? Thanks!

Comment 3

[Alice0775 White]

I can reproduce the issue on Firefox65 and Nightly67.0a1 windows10 as well.

Regression Window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=591fefb0f99c9d50325fe8e4b151aab7e1ba8691&tochange=bec76d96700206177d42ca13e789519bc7eb313a

Regressed By:
bec76d967002 Ehsan Akhgari — Bug 1502240 - Ensure that Content Blocking allow list is applied to all cookie policies r=baku

Comment 4

[(no longer active)]

Thanks a lot for the regression range, Alice!

So we start here: https://searchfox.org/mozilla-central/rev/2a6f3dde00801374d3b2a704232de54a132af389/netwerk/cookie/nsCookieService.cpp#2087 and try to do an anti-tracking check. That results in us taking this early return path from this branch https://searchfox.org/mozilla-central/rev/2a6f3dde00801374d3b2a704232de54a132af389/toolkit/components/antitracking/AntiTrackingCommon.cpp#1336 because there is a cookie permission for the top-level domain (microsoft.com).

Then we get to this check: https://searchfox.org/mozilla-central/rev/2a6f3dde00801374d3b2a704232de54a132af389/netwerk/cookie/nsCookieService.cpp#4046. But here we have the wrong information. We accept the cookie but we should actually reject it because the result of the anti-tracking check should be a failure.

I think this shows a bug in the anti-tracking backend... It seems to me that we shouldn't be calling CheckCookiePermissionForPrincipal() on the top-level principal, since that means performing a cookie permission check on the origin of the top-level page rather than the channel performing the load. Andrea, what do you think?

Comment 5

[David Olah]

Thank you Alice!

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Ehsan is this something you might work on? I can keep tracking it into the 66 release. Since it is a privacy issue we could potentially consider including a fix in late beta or even in a dot release.

Comment 7

[(no longer active)]

I'm waiting on Andrea to respond to my question in comment 4, I'm not yet sure how to proceed here... Let me email him to remind him.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: Bug 1530132 - Consider the cookie permission for the current request, ignoring the top-level one, r?ehsan

Comment 9

[Andrea Marchesini [:baku]]

This regression was introduced by bug 1480780. I think it's resoneable to consider just the current cookie permission and not the top-level one.

Comment 10

[(no longer active)]

Great, yes, this is exactly what I had in mind too. Thanks for confirming.

I actually posted your patch to try yesterday and it's green. :-)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9912c7022822c8efdf4f1c72e8c90a7396c681b2

Comment 11

[(no longer active)]

I'm gonna land this on inbound to expedite things. Can you please ask for approval for beta?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7619b9307278
Consider the cookie permission for the current request, ignoring the top-level one; r=ehsan

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 9049088 [details]
Bug 1530132 - Consider the cookie permission for the current request, ignoring the top-level one, r?ehsan

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1480780
• User impact if declined: More cookies are stored then what we should if the user has custom cookie permissions
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: There is a STR on the bug descritpion
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch removes a cookie permission check. That was introduced wrongly and it's about Anti-Tracking only. Low risk.
• String changes made/needed: none

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12d0f0ba867a
Consider the cookie permission for the current request, ignoring the top-level one, r=Ehsan

Comment 15

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/7619b9307278

Comment 16

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/12d0f0ba867a

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9049088 [details]
Bug 1530132 - Consider the cookie permission for the current request, ignoring the top-level one, r?ehsan

I'd like this in the RC, and it's the last minute,
so let's uplift now and verify after.

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9374f09f9b17

Comment 19

[Oana Botisan, Desktop QA]

I managed to reproduce the issue using an older version on Nightly (2019-02-23) on Windows 10 x63.
I verified the fix using latest Nightly on macOS 10.13 and Ubuntu 18.04 x64 and the bug is not reproducing.

HOwever on Windows 10 x64 I can still reproduce the issue using the latest Nightly 67.0a1.

Comment 20

[(no longer active)]

This isn't a platform specific regression but rather an intermittent one. I'll file another bug on it.

Comment 21

[(no longer active)]

BTW that's a regression from bug 1525245 not this one, so it doesn't affect beta.

Comment 22

[(no longer active)]

Bug 1535380 is the said bug.

Comment 23

[Oana Botisan, Desktop QA]

I will take out the qe-verify + flag from this bug and I will follow the Bug 1535380.