1524674 - subdomain settings override cookie permissions for domain when cleared on shutdown

Status: RESOLVED FIXED

(Toolkit :: Data Sanitization, defect, P1)

Description

[[:philipp]]

after the update to firefox 65 there have been a handful of reports from users, who have set the preference "Delete cookies and site data when Firefox is closed" but have exceptions for sites they want to stay logged-into over multiple sessions, that firefox started clearing some cookies from domains that were set to "allow" after closing the app.

user astral on irc found out that's the case in particular when there are stricter permissions ("allow for session") for a subdomain than for the top level domain ("allow") - in this setup the cookies from the top level domain get cleared after the session as well.

str in a new profile:

• tick "Delete cookies and site data when Firefox is closed" in about:preferences#privacy
• under manage permissions set https://www.mozilla.org to "Allow for Session" and https://mozilla.org to "Allow"
• visit https://www.mozilla.org
• take a look under "Manage Data", there is now be a cookie present from www.mozilla.org as well as mozilla.org
• restart the browser and take a look in "Manage Data" again - the cookie from mozilla.org is gone, though it should have been kept according to the settings.

this looks related to the work in bug 1503217...

Comment 1

[[:philipp]]

addition: the problem is even reproducible without having set "Delete cookies and site data when Firefox is closed" in the str above.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: Bug 1524674 - Cleanup site data with custom permissions per subdomains - debug messages, r=johannh

Comment 3

[Andrea Marchesini [:baku]]

Attachment: Bug 1524674 - Cleanup site data with custom permissions per subdomains - check permissions, r=johannh

Depends on D18496

Comment 4

[Andrea Marchesini [:baku]]

Attachment: Bug 1524674 - Cleanup site data with custom permissions per subdomains - Tests, r?johannh

Depends on D18497

Comment 5

[Andrea Marchesini [:baku]]

I wrote a fix for this issue but I don't know if we should allow custom permissions for subdomains.
Would be cleaner to have permissions for eTLD+1 domains? I'm asking this because cookies are deleted and stored using basename as key. This means that cookies are wrongly deleted (see the test in patch 3).

Comment 6

[Johann Hofmann [:johannh]]

(In reply to Andrea Marchesini [:baku] from comment #5)

I wrote a fix for this issue but I don't know if we should allow custom permissions for subdomains.
Would be cleaner to have permissions for eTLD+1 domains? I'm asking this because cookies are deleted and stored using basename as key. This means that cookies are wrongly deleted (see the test in patch 3).

If we are instructed to clear cookies from sub.example.com then it is the right course of action to also clear cookies for example.com, because those might contain information that sub.example.com is accessing. The "wrong" deletion of those is a minor inconvenience for folks with complicated whitelist rules while the consequence of not deleting them may be a privacy risk to other users.

I agree that limiting this permission to eTLD+1 would kind of resolve that issue, though in practice a lot of people already have these permissions for sub-domains so we can never really stop supporting it. Ideally we could issue a warning in the UI if two rules conflict with each other and just keep things as they are. Might be worth filing a bug on it.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47c9a1bb89cc
Cleanup site data with custom permissions per subdomains - debug messages, r=johannh
https://hg.mozilla.org/integration/autoland/rev/48d3ec8e7ec6
Cleanup site data with custom permissions per subdomains - check permissions, r=johannh
https://hg.mozilla.org/integration/autoland/rev/7d4ae80c96b2
Cleanup site data with custom permissions per subdomains - Tests, r=johannh

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/47c9a1bb89cc
https://hg.mozilla.org/mozilla-central/rev/48d3ec8e7ec6
https://hg.mozilla.org/mozilla-central/rev/7d4ae80c96b2

Comment 9

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

baku, do you want to request beta uplift?

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 9041027 [details]
Bug 1524674 - Cleanup site data with custom permissions per subdomains - debug messages, r=johannh

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1503217

User impact if declined

If the user has custom cookie permissions for domain and its subdomains, Sanitizer.jsm is not able to delete cookies correctly.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

Yes

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)

The real fix is just 1 line where we check the existence of cookie permissions for subdomains instead of delete all directly.
The other 2 patches are tests and logging to be sure that we don't delete more or less than what we should. Low risk patches.

String changes made/needed

Comment 11

[[:philipp]]

an affected user at https://support.mozilla.org/questions/1249037#answer-1194210 has verified that the issue is solved in the current nightly build.

Comment 12

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

Comment on attachment 9041027 [details]
Bug 1524674 - Cleanup site data with custom permissions per subdomains - debug messages, r=johannh

With user verification of the fix, let's take this for beta 6.

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bf4fec8933af
https://hg.mozilla.org/releases/mozilla-beta/rev/ea1d5311b285
https://hg.mozilla.org/releases/mozilla-beta/rev/6e1b209f1fd8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

I'm a bit nervous about taking this in next week's dot release. Is there any sort of exploratory testing QA could do around this to help find any possible regressions?

Comment 15

[Andrea Marchesini [:baku]]

I'm a bit nervous about taking this in next week's dot release. Is there any sort of exploratory testing QA could do around this to help find any possible regressions?

Sorry for the delay. This feature is covered by tests, but if we want to QA it, there is an STR.
Note that the test cannot be done just using cookies, because nsCookieService doesn't deal correctly with cookies set and removed on a domain and one of its sub domain. Better to use indexedDB, localStorage, or any other storage API.

It's important to know that this bug is about a corner-case, reproducible just by advanced users.

Comment 16

[Thomas Mayer]

I have third party cookies blocked and I let FF delete all cookies between two usages.

After getting the 65.0 Update (via ubuntu 18.04 packages), a log in at a major insurance company is not working any more.

This bug has already been classified as a defect which was introduced in FF 65. It even got fixed a year ago for 66+ but presumably not for 65.

@RyanVM

Why is this a wontfix for FF65?

I guess the regression is in the wild now (via FF65) and it should also be fixed for the current stable version - which most people use.

Comment 17

[[:philipp]]

firefox 65 was superseded by later versions containing the fix on march 19, 2019 and remains unsupported since then - the current stable/supported version of the browser is firefox 75.

Comment 18

[Thomas Mayer]

Thanks for the response. Indeed. I have FF 75, not 65. Sorry for the confusion.

Then I assume that the web site in question requires third party cookies for some - bad - reasons.