1510860 - Create an exception to content blocking on a page results in erratic blocking behaviour.

Status: RESOLVED FIXED

(Firefox :: Protections UI, defect)

Description

[Erica Wright [:ewright]]

Attachment: all-cookies-blocked-after-exception.png

STR:
- apply this patch: https://phabricator.services.mozilla.com/D12596
(in order to illustrate the behavior - the behavior is not caused by this patch)

- turn on blocking for "all cookies"
- navigate to a website with cookies like "https://www.ebay.de/"
- add an exception for this site - click "turn off blocking for this site" button in the control panel
- click the arrow next to the cookies section

ER: No cookies should be blocked
AR: cookies "from this site" are being blocked and "tracking cookies" are being allowed through.

this is the result of running await gBrowser.selectedBrowser.getContentBlockingLog():

"{
 \"https://rover.ebay.de\": [[32768, true, 1], [1073741824, true, 1]],
 \"https://pulsar.ebay.de\": [[32768, true, 1], [1073741824, true, 1]],
 \"https://ir.ebaystatic.com\": [[8192, true, 1]],
 \"https://i.ebayimg.com\": [[8192, true, 1]],
 \"https://ocsrest.ebay.de\": [[32768, true, 1], [1073741824, true, 1]],
 \"https://www.ebay.com\": [[8192, true, 1]],
 \"https://signin.ebay.de\": [[32768, true, 1], [1073741824, true, 1]],
 \"https://velocity-files-origin.ebay.com\": [[8192, true, 1]],
 \"https://gha.ebay.de\": [[32768, true, 1], [1073741824, true, 1]],
 \"https://rover.ebay.com\": [[8192, true, 1], [32768, true, 1]],
 \"https://src.ebay-us.com\": [[8192, true, 1]]
}
"

screenshot attached.

This seems to only reliably reproduce with the setting blocking "all cookies".

Comment 1

[(no longer active)]

src.ebay-us.com only has STATE_LOADED_TRACKING_CONTENT.  Why is it marked as blocked then?!

Comment 2

[(no longer active)]

This may be fixed by bug 1515343.

Comment 3

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #2)
> This may be fixed by bug 1515343.

No, it seems unrelated.

Comment 4

[(no longer active)]

This is a regression from bug 1502240 where we made content blocking allow list apply to all cookie policies.

The problem is that these checks in the cookie service are still happening based on isForeign: <https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/netwerk/cookie/nsCookieService.cpp#2001>, which means that we will not be performing the checks for first-party cookies.  This is important since it means that in the case where our cookieBehavior pref tells us to block all cookies, we will not be checking the content blocking allow list to see if we need to exempt a given cookie or not, and we indiscriminately block the cookie, which is the behaviour that is observed after turning off blocking for ebay.de on domains such as rover.ebay.de.

Comment 5

[(no longer active)]

Attachment: Bug 1510860 - Ensure that the cookie service checks the content blocking allow list even for first-party cookies since that's required when we're blocking all cookies;

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a328e80b02c
Ensure that the cookie service checks the content blocking allow list even for first-party cookies since that's required when we're blocking all cookies; r=baku

Comment 7

[Eliza Balazs [:ebalazs_]]

Backed out changeset 3a328e80b02c (Bug 1510860) for failures e.g.: dom/tests/mochitest/general/test_storagePermissionsLimitForeign.html CLOSED TREE

Push with multiple failures, storage issue: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=superseded%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=3a328e80b02cf3c11a36e060380277396e211cd9&selectedJob=218353178

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218353178&repo=autoland&lineNumber=3240
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218355469&repo=autoland&lineNumber=10989
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218355472&repo=autoland&lineNumber=2054
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218351107&repo=autoland&lineNumber=26378

Backout push: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218351107&repo=autoland&lineNumber=26378

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/232000a83412
Backed out changeset 3a328e80b02c for failures e.g.: dom/tests/mochitest/general/test_storagePermissionsLimitForeign.html CLOSED TREE

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e94a166a769c
Ensure that the cookie service checks the content blocking allow list even for first-party cookies since that's required when we're blocking all cookies; r=baku

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/e94a166a769c

Comment 11

[(no longer active)]

Comment on attachment 9032717 [details]
Bug 1510860 - Ensure that the cookie service checks the content blocking allow list even for first-party cookies since that's required when we're blocking all cookies;

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1502240

User impact if declined: See comment 0.  This will most definitely impact other sites too.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Quite low risk.  We were forgetting to do some access checks in the case of first-party checks, but we already perform them in the case of third-party checks, so there isn't really a stability risk associated with running the same code in other cases too, and this code is rigorously tested in automated tests, so I'm not nervous about introducing regressions either.

String changes made/needed: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9032717 [details]
Bug 1510860 - Ensure that the cookie service checks the content blocking allow list even for first-party cookies since that's required when we're blocking all cookies;

[Triage Comment]
Fixes unexpected cookie blocking under some circumstances. Thanks for including tests with this. Approved for 65.0b7.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dd9715b4ca42

Comment 14

[Kanchan Kumari QA]

Does this need manual qa? If yes can you please provide steps to install phabricator patch on Firefox, so that I can verify the fix on latest Beta and Nightly. 

I tried duplicating this issue with comment 0 with strict content blocking option, added exception for "https://www.ebay.de/" by  clicking "turn off blocking for this site" button in the control panel
Version 	65.0a1
Build ID 	20181127100134

I see it's allowing cookies for "https://www.ebay.de/" but don't see the breakdown of tracking cookies.

Comment 15

[(no longer active)]

No, this doesn't need manual QA.  Only bugs marked in our "Privacy Tracking" spreadsheet marked as "QA NEEDED" require manual testing.

Comment 16

[Kanchan Kumari QA]

Based on comment 15 I am removing the qe-verify+ flag. Thanks!