Closed Bug 1675018 Opened 3 years ago Closed 3 years ago

When clearing history, avoid clearing user interaction permissions for sites that still have cookies

Categories

(Toolkit :: Data Sanitization, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
85 Branch
Tracking Status
firefox82 --- disabled
firefox83 --- disabled
firefox84 --- wontfix
firefox85 --- verified
firefox86 --- verified

People

(Reporter: johannh, Assigned: johannh)

References

Details

Attachments

(5 files)

This is the proper fix to bug 1672394.

Instead of strictly clearing interaction permissions together with history, for each permission we confirm that there are no cookies or site data in store for that origin, and only then clear it. That would still be fine privacy-wise since the user had cookies/site data around anyway, so keeping the permission doesn't reveal more. And it should fix this issue.

It's definitely a lot more complex to implement and expensive to run, so we keep on making bug 1524200 worse, but it's probably worth it.

Component: Privacy: Anti-Tracking → Data Sanitization
Product: Core → Toolkit

This is done in preparation to using the module on Android in order to exclude
certain principals from getting their user interaction permissions cleared.

This function is a helper for clearing all storageAccessAPI permissions
that were modified after a certain date. We need to get them and filter
by principal to clear them.

Depends on D96638

This is a helper function for clear history functionality that needs to ensure that
storageAccessAPI permissions that would mirror history are also deleted without
clearing permissions that keep cookies and site data alive.

Depends on D96639

This uses the new deleteStorageAccessForClearingHistory API in Sanitizer to avoid
clearing all storage access API permissions and thus all cookies and site data when
clearing only history.

Depends on D96640

This uses the new deleteStorageAccessForClearingHistory API in GeckoView to avoid
clearing all storage access API permissions and thus causing purging to clear all cookies
and site data when clearing only history.

Depends on D96641

I'll land this next week after the merge to respect soft freeze, but we're interested in uplifting this to early beta.

Attachment #9187028 - Attachment description: Bug 1675018 - Part 3 - Implement ClearDataService::DeleteStorageAccessForClearingHistory. r=timhuang → Bug 1675018 - Part 3 - Implement ClearDataService::DeleteUserInteractionForClearingHistory. r=timhuang
Attachment #9187029 - Attachment description: Bug 1675018 - Part 4 - Use deleteStorageAccessForClearingHistory in Sanitizer.jsm. r=mak,timhuang! → Bug 1675018 - Part 4 - Use deleteUserInteractionForClearingHistory in Sanitizer.jsm. r=mak,timhuang!
Attachment #9187030 - Attachment description: Bug 1675018 - Part 5 - Use deleteStorageAccessForClearingHistory in GeckoView. r=agi,esawin → Bug 1675018 - Part 5 - Use deleteUserInteractionForClearingHistory in GeckoView. r=agi,esawin
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e09ce61e374
Part 1 - Move PrincipalsCollector into its own module. r=mak
https://hg.mozilla.org/integration/autoland/rev/ee836c7f63d4
Part 2 - Add PermissionManager::GetAllByTypeSince. r=timhuang
https://hg.mozilla.org/integration/autoland/rev/34fd482121f2
Part 3 - Implement ClearDataService::DeleteUserInteractionForClearingHistory. r=timhuang
https://hg.mozilla.org/integration/autoland/rev/7fe621a805eb
Part 4 - Use deleteUserInteractionForClearingHistory in Sanitizer.jsm. r=timhuang
https://hg.mozilla.org/integration/autoland/rev/81e7fe361388
Part 5 - Use deleteUserInteractionForClearingHistory in GeckoView. r=agi

Huh, black Python formatter...

Flags: needinfo?(jhofmann)
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e1d21415f6b
Part 1 - Move PrincipalsCollector into its own module. r=mak
https://hg.mozilla.org/integration/autoland/rev/7e58927653ca
Part 2 - Add PermissionManager::GetAllByTypeSince. r=timhuang
https://hg.mozilla.org/integration/autoland/rev/667e91c1e320
Part 3 - Implement ClearDataService::DeleteUserInteractionForClearingHistory. r=timhuang
https://hg.mozilla.org/integration/autoland/rev/316ac9494453
Part 4 - Use deleteUserInteractionForClearingHistory in Sanitizer.jsm. r=timhuang
https://hg.mozilla.org/integration/autoland/rev/44c945afc31b
Part 5 - Use deleteUserInteractionForClearingHistory in GeckoView. r=agi

Comment on attachment 9187026 [details]
Bug 1675018 - Part 1 - Move PrincipalsCollector into its own module. r=mak

Beta/Release Uplift Approval Request

  • User impact if declined: Users are logged out of popular sites daily when using clear history on shutdown. We already have a hacky workaround that prevents purging from running for these users, but that's also not great. This is a better solution (I hope).
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce:
  • Log into Google, Twitter and/or Facebook
  • in about:preferences, enable "Clear History when Nightly closes" and in the "Settings.." sub-menu only enable only Browsing & Download History
  • Restart your browser, which will clear your history
  • Go to about:preferences and turn off "Clear History when Nightly closes" again (this is needed to work around our short-term fix for v83)
  • In the browser toolbox, run
await Components.classes["@mozilla.org/purge-tracker-service;1"].getService(Components.interfaces.nsIPurgeTrackerService).purgeTrackingCookieJars()
  • Visit Google, Twitter and/or Facebook again. The bug is that you're logged out now. You should have stayed logged in because you only wanted to clear history, not cookies.
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): This is a relatively complex patch that changes how we clear history, but it's well tested and we've waited with uplifting until the beginning of the beta cycle on purpose.
  • String changes made/needed: None
Attachment #9187026 - Flags: approval-mozilla-beta?
Attachment #9187027 - Flags: approval-mozilla-beta?
Attachment #9187028 - Flags: approval-mozilla-beta?
Attachment #9187029 - Flags: approval-mozilla-beta?
Attachment #9187030 - Flags: approval-mozilla-beta?

(In reply to Johann Hofmann [:johannh] from comment #12)

  • User impact if declined: Users are logged out of popular sites daily when using clear history on shutdown. We already have a hacky workaround that prevents purging from running for these users, but that's also not great. This is a better solution (I hope).

Given the workaround (in bug 1672394) can't we just let this one ride the trains?

Flags: needinfo?(jhofmann)

Sorry for the PTO-induced delay, I would still prefer to get this into Beta since it also solves the case for people who are clearing history manually, which bug 1672394 doesn't cover.

Flags: needinfo?(jhofmann)

Comment on attachment 9187026 [details]
Bug 1675018 - Part 1 - Move PrincipalsCollector into its own module. r=mak

Given that this hasn't gotten verified by QA yet, is pretty complex, and we're down to our final 2 betas of the cycle, I think it's best to let this ride 85 at this point.

Attachment #9187026 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9187027 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9187028 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9187029 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9187030 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Hello! Reproduced the issue with Firefox 84.0a1 (20201103213933) on Windows 10x64 and steps from comment 12.
The issue is verified fixed with Firefox 85.0b3 (20201217185930) and 86.0a1 (20201218095607) on Windows 10x64, macOS 10.12 and Ubuntu 18.04. After following the steps from comment 12 and revisiting Facebook, Twitter, and Google I was still logged in.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: