Closed Bug 1524674 Opened Last year Closed Last year

subdomain settings override cookie permissions for domain when cleared on shutdown

Categories

(Toolkit :: Data Sanitization, defect, P1)

65 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- unaffected
firefox65 + wontfix
firefox66 + fixed
firefox67 --- fixed

People

(Reporter: philipp, Assigned: baku)

Details

(Keywords: regression)

Attachments

(3 files)

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...

Flags: needinfo?(amarchesini)

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

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).

Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Status: NEW → ASSIGNED
Priority: -- → P1

(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.

Attachment #9041028 - Attachment description: Bug 1524674 - Cleanup site data with custom permissions per subdomains - check permissions, r?johannh → Bug 1524674 - Cleanup site data with custom permissions per subdomains - check permissions, r=johannh
Attachment #9041029 - Attachment description: Bug 1524674 - Cleanup site data with custom permissions per subdomains - Tests, r?johannh → Bug 1524674 - Cleanup site data with custom permissions per subdomains - Tests, r=johannh
Attachment #9041027 - Attachment description: Bug 1524674 - Cleanup site data with custom permissions per subdomains - debug messages, r?johannh → Bug 1524674 - Cleanup site data with custom permissions per subdomains - debug messages, r=johannh
Pushed by amarchesini@mozilla.com:
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

baku, do you want to request beta uplift?

Flags: needinfo?(amarchesini)

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

Flags: needinfo?(amarchesini)
Attachment #9041027 - Flags: approval-mozilla-beta?

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 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.

Attachment #9041027 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9041029 - Attachment description: Bug 1524674 - Cleanup site data with custom permissions per subdomains - Tests, r=johannh → Bug 1524674 - Cleanup site data with custom permissions per subdomains - Tests, r?johannh
Attachment #9041029 - Flags: approval-mozilla-beta+

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?

Flags: needinfo?(amarchesini)

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.

Flags: needinfo?(amarchesini)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.