Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tracking: migrate cookie lifetime and sanitize cookie/offlineApps #1441

Closed
aleyvo opened this issue May 3, 2022 · 35 comments
Closed

tracking: migrate cookie lifetime and sanitize cookie/offlineApps #1441

aleyvo opened this issue May 3, 2022 · 35 comments

Comments

@aleyvo
Copy link

aleyvo commented May 3, 2022

Just a follow-up to the "keep cookie + site data exceptions on close" functionality (issues #1256 and #1291.)

Considering that:

  • privacy.sanitize.sanitizeOnShutdown is not "All or Nothing" anymore, i.e. privacy.clearOnShutdown.cookies and privacy.clearOnShutdown.offlineApps now respect website exceptions as of FF99 (1681701, code);

  • network.cookie.lifetimePolicy is soon/not so soon to be deprecated (1681493, 1764761).

Would it be the right time to update section 2800, deprecating network.cookie.lifetimePolicy and setting both privacy.clearOnShutdown.cookies and privacy.clearOnShutdown.offlineApps to true? Functionality wise, thing should continue to be the same. It would be more of a future-proof measure.

@Thorin-Oakenpants
Copy link
Contributor

Thorin-Oakenpants commented May 3, 2022

The only issue I have is timing

  • there is a migration planned
    • i.e if users have lifetime policy = 2, then enable sanitize on close and check cookies and offlineApps - and some logic to uncheck the other options IF sanitize on close was disabled
    • IDK what happens if the prefs are modified - e.g. us
    • and for subsequent starts the user.js will reset them

In other words I want to avoid the migration code getting triggered. So as long as we can switch at least one release before the pref is deprecated, then that's what I want to do

My best guess is that they will want to get this done in ESR102 at the latest. For now it is not in FF101 and we can safely keep using the current setup.

I would rather not pull the trigger on changing until I know it's production ready. There's a bit to digest, and I would want to know it's all working as intended. For example, 101 introduced some sanitizing speed perfs, which caused a regression with containers and subdomains (which was fixed). What I'm saying is I want to let the bugs manifest if there are any. And there is still code to be changed where lifetime policy is used.

We can afford to wait. The only benefit I see is that it would then allow service workers, but since we've lived without those for the last four five+ years, I don't think another release or two matters

We can keep this issue open as the ToDo and for tracking (although I am already tracking it)

@Thorin-Oakenpants Thorin-Oakenpants changed the title discussion: keep cookie + site data exceptions on close (pref deprecation) tracking: migrate cookie lifetime and sanitize cookie/offlineApps May 3, 2022
@necaran
Copy link

necaran commented May 5, 2022

I think I found the difference between network.cookie.lifetimePolicy and privacy.sanitize.sanitizeOnShutdown way. Say I have a rule to always allow https://accounts.google.com (btw, you need only cookies set by accounts.google.com to keep Google logged in).

  • Case 1: network.cookie.lifetimePolicy = 2.
    Visit www.google.com and inspect devtools > storage. Cookies set by www.google.com are session-only (Expires / Max Age column). If you quit the browser and start again. Cookies from www.google.com will be gone but those from accounts.google.com are still there.

  • Case 2: network.cookie.lifetimePolicy = 0, privacy.sanitize.sanitizeOnShutdown = true and privacy.clearOnShutdown.cookies = true.
    Visit www.google.com and inspect devtools > storage. Cookies set by www.google.com are not session-only and have proper expiration dates. If you quit the browser and start again. Cookies from both www.google.com and accounts.google.com are still there.

TL;DR:
In case 1, Google dark theme setting is not remembered, but in case 2 it is remembered.

@Thorin-Oakenpants
Copy link
Contributor

Case 2 - We are going to be making clearOnShutdown true for both cookies (cookies) and offlineApps (idb, service worker cache, local storage, quota manager stuff). IDK if that changes the google dark theme being remembered, it depends on where it is stored and it's OA (origin attributes)

Anyway, when ready, we need to test this without using dFPI exceptions .. because, well fuck .... have a read

  • https://github.com/privacyguides/privacyguides.org/discussions/941#discussioncomment-2674048 along with subsequent/previous comments

Anyway, personally, I have uBO in with no 3rd party by default, and I only have five site-data exceptions, and only one of those sites can be used as a SSO, so ... The Almighty knows I am out of this mess, but he's pretty sure you're fucked :)

@necaran
Copy link

necaran commented May 5, 2022

I do have offlineApps checked. The logic is that when allowing https://accounts.google.com, a cookie of domain .google.com (as shown by devtools > storage) will not be cleared if not session-only, regardless of the subdomain by which it was added. If I modify a cookie's domain to www.google.com, it will be cleared on shutdown. (BTW, by doing this to cookie name NID resets the dark theme setting of www.google.com)

@Thorin-Oakenpants
Copy link
Contributor

this stuff can get confusing fast, and tests need to also check subdomains and probably scheme. AFAIK shit should be eTLD+1 (and i think scheme)

@Thorin-Oakenpants
Copy link
Contributor

this stuff probably intersects with #1293 and the longer I can ignore it all, the better TBH

@aleyvo
Copy link
Author

aleyvo commented May 5, 2022

Anyway, when ready, we need to test this without using dFPI exceptions .. because, well fuck .... have a read

* `https://github.com/privacyguides/privacyguides.org/discussions/941#discussioncomment-2674048` along with subsequent/previous comments

Wait... So website exceptions don't even know that cookie partitions are a thing? I feel like I've been carefully feeding the monkey with small amounts of cookies each day, only to know that it had access to my whole jar of cookies since the beginning.

Have a bunch of site exceptions. Seems like you're right, Mr. Stephen. I'm sure to be f*cked.

@Thorin-Oakenpants
Copy link
Contributor

right .. here we go .. patchs underway bitches, early in the 102 nightly cycle

@Thorin-Oakenpants
Copy link
Contributor

The UI settings changing is important as well for the user.js

@necaran
Copy link

necaran commented May 6, 2022

patchs underway

When network.cookie.lifetimePolicy = 2, cookies are handled in a two-phase manner.

  • Pahse 1: All cookies, unless permanently allowed, are marked as session-only when added.
  • Phase 2: On shutdown, cookies that are session-only or not in the exception list are purged.

If migrated to privacy.sanitize.sanitizeOnShutdown, there won't be a phase 1 (as of Firefox 100) and thus the situation above. Will this be covered in the future?

@Thorin-Oakenpants
Copy link
Contributor

I can't follow that.

If you migrate from lifetime = 2 then "phase" 1 won't happen (unless permanently set as Allow for session) because the default behavior changed.

And thus, "phase 2" also won't happen, because you aren't setting cookies to be session only. Except, session cookies (such as bank ones etc) have always been cleared on close, that's what the "session" part means, so in technically, phase 2 has always happened


This is a two stepper, don't think too hard about it

  • lifetime pref changed to default / deprecated: all cookies are now kept by default: does not affect your existing Allow cookies
  • 2x sanitize on close prefs: as long as both prefs respect the site exception, then we're good
    • side note: this actually reduces some sanitizing as before I could keep the "cookie + site data" exceptions but bulk-clear all "site data"

101 is basically done and dusted, and looks like 102 will "migrate" existing users, so at this point we have almost two months to work it out

@aleyvo
Copy link
Author

aleyvo commented May 13, 2022

and at this point I'm really concerned, because as I'm trying to word the notes/description in 0801, I realized that all the defaults for cache, downloads, formdata, history are true, and the code I think is going to change those to false (so users don't get their shit wiped when only asking to remove cookies and site data) - but I think it also respects the current settings if 0801 is enabled - which makes no sense except in the migration, because that's what the UI will now toggle

Migration Code

The migration code will only do its thing if network.cookie.lifetimePolicy is 2. If this is the case, then 2801 will be enabled and *.cookies, *.offlineApps, and *.cache will be set to true and network.cookie.lifetimePolicy will be wiped. But before enabling 2801, the code will check if it has already been enabled - if it has NOT, then *.history, *.downloads, *.formdata, *.sessions, and *.siteSettings will be set to false to avoid unintentional wiping of user data when flipping 2801.

If lifetimePolicy is not 2, then the migration won't be triggered. If we were to deprecate this lifetimePolicy pref in arkenfox's user.js, and then to run prefsCleaner, it seems like we could avoid this migration altogether.

The migration code can be seen here.

UI Code

The UI code is a little trickier, but here is my understanding of it:

  • Scenario 1: If ANY of the prefs 2801, *.cookies, *.offlineApps, OR *.cache is set to false in prefs.js, then "Delete cookies and site data when Firefox is closed" will be shown as unchecked when about:preferences is loaded. In this case, whenever the user checks this option, 2801, *.cookies, *.offlineApps, and *.cache will ALL be set to true in prefs.js. But before enabling 2801, the code will check if it has already been enabled - if it has NOT, then *.history, *.downloads, *.formdata, *.sessions, and *.siteSettings will be set to false to avoid unintentional wiping of user data when flipping 2801.

  • Scenario 2: If prefs 2801, *.cookies, *.offlineApps, AND *.cache are ALL set to true in prefs.js, then "Delete cookies and site data when Firefox is closed" will be shown as checked when about:preferences is loaded. In this case, whenever the user unchecks this option, *.cookies, *.offlineApps, and *.cache will be set to false in prefs.js. And 2801 will only be set to false if all of the other clearOnShutdown.* prefs are also set to false.

Whenever the user changes ANY of the prefs 2801, *.cookies, *.offlineApps, OR *.cache in about:preferences, if this were to change the status of the "Delete cookies and site data when Firefox is closed" checkbox, then Scenario 1 or Scenario 2 would be reapplied, as if the user was clicking directly at the "Delete cookies and site data when Firefox is closed" checkbox.

Regarding arkenfox, if we were to set *.cookies and *.offlineApps to true in user.js, and considering that 2801 and *.cache are already set to true, then "Delete cookies and site data when Firefox is closed" would be shown as checked in about:preferences. But both scenarios, 1 and 2, would only happen with explicit user interaction. In other words, no flipping of prefs set by arkenfox should happen automatically. So arkenfox should be fine.

The UI code can be seen here.

@aleyvo
Copy link
Author

aleyvo commented May 13, 2022

and we need to fit the cpd ones in - we will need to test that the cookie + offlineApps respect exceptions

As of FF 99, clearOnShutdown.cookies and clearOnShutdown.offlineApps do respect website exceptions, but cpd.cookies and cpd.offlineApps do not. I believe this behavior will be kept the same when FF 102 comes out, because it would be the only way to clear all website data at once, even from exceptions, if the user wanted to.

But I think arkenfox should keep cpd.cookies and cpd.offlineApps to false, to preserve user data in an eventual cpd cleaning. After all, if the user really wants to clear cookies and offlineApps exceptions when using cpd, he is just two checkboxes away from that - no pain at all.

@Thorin-Oakenpants
Copy link
Contributor

Thorin-Oakenpants commented May 13, 2022

i'm not "worried" about the migration code, I'm worried that toggling the UI causes odd behavior, but now I've had a sleep I think the logic might be ok

because it would be the only way to clear all website data at once, even from exceptions, if the user wanted to

That's what manage data + clear data is for. cpd should follow the same logic and those two prefs respect exceptions

@aleyvo
Copy link
Author

aleyvo commented May 14, 2022

You're right, Thorin. I had forgotten about manage data + clear data. They're really the guaranteed way to clear exceptions, selectively or all at once. My previous reasoning is plain wrong.

But even having it, Mozilla seems to be trying to push cpd not to respect exceptions. There's a unit test ensuring this behavior, at least for cookies. Maybe they thought it would be easier for end users? I don't know. Time will tell how this evolves.

@Thorin-Oakenpants
Copy link
Contributor

That was back in Feb. Lets just see what happens. One other reason this is a bit icky, is that I think the same codebase is used for forget about this site, and if they wanted to have cpd behave the same as clearonshutdown, then they might have to add more engineering so it knows if you are using forget about this site or ctrl-shift-del - and maybe that's not worth it in their eyes

personally, I can see confusion if cpd behavior differs

@Thorin-Oakenpants
Copy link
Contributor

I hope you all realize that this was pushed back to FF103 at best (they may back port it to ESR at some point, IDK - I hope they do, so everyone is in sync)

@Thorin-Oakenpants
Copy link
Contributor

fuck, I think the migration is in 102

@Thorin-Oakenpants
Copy link
Contributor

^ yup: https://old.reddit.com/r/firefox/comments/vn68tr/firefox_is_unchecking_delete_cookies_and_site/

oh well, what a messy UI and consistency until it gets resolved

@Thorin-Oakenpants
Copy link
Contributor

aa7d7c2

I also merged the old 2802 pref into 2810's and renumbered 2812+ -> 2820's (and updated wiki reference numbers)

@Thorin-Oakenpants
Copy link
Contributor

Thorin-Oakenpants commented Jun 29, 2022

now I'm losing all cookies every few sessions - my user.js is the same as the v102 one in PR

  • edit: it's happened twice: the first I expected e.g. from reports from LW, the second I did not. will monitor

@aleyvo
Copy link
Author

aleyvo commented Jun 29, 2022

I've been using these changes of arkenfox-102 PR for a week (done by myself, in FF 101), and I have not experienced this issue yet. Maybe it's a bug introduced by FF 102?

@Thorin-Oakenpants
Copy link
Contributor

IDK why it happened again, or even the first time (can't be assed) - I modified my user.js before updating to 102 (to avoid the migration and am 75% sure I reset the pref (or maybe I decided to let the migration do it for kicks). Anyway, doesn't seem to be happening now.

I did subsequently check the lifetime pref UI in cookies and site data, and then restarted, but I fail to see how that does anything besides trigger another migration .. so ¯\_(ツ)_/¯

@Thorin-Oakenpants
Copy link
Contributor

So the "delete cookies and site data" checkbox in 102 still changes the lifetime pref, which then proceeds to uncheck itself and migrate again on next start - this is changed in 103 - https://bugzilla.mozilla.org/show_bug.cgi?id=1681495

The other one is every time you play with that checkbox, it resets to deleting cache - see comments https://old.reddit.com/r/firefox/comments/vn68tr/firefox_is_unchecking_delete_cookies_and_site/

I was going to try and shoehorn a warning not to touch that UI setting, but I ended up moving the pref to deprecated, and it's only 4 more weeks until it's semi-resolved. We should keep an eye on updating the pref info for that checkbox

@aleyvo
Copy link
Author

aleyvo commented Jun 29, 2022

This weird issue has just happened to me. There was an electrical outage here... when I reopened everything, all my cookies and local storage were gone, just like Thanos. I'm still using FF 101, with arkenfox-102 PR.

@Thorin-Oakenpants
Copy link
Contributor

^ that's a bug that has been fixed in an upcoming release - I'll report back with the bugzilla for ya

for me it's now all stable as fuck, been testing and opening and closing shit for the last 5 hours

@Thorin-Oakenpants
Copy link
Contributor

^ https://bugzilla.mozilla.org/show_bug.cgi?id=1771024 - fixed in 103

@Thorin-Oakenpants
Copy link
Contributor

Thorin-Oakenpants commented Jun 29, 2022

just like Thanos

stop over-reacting ... Thanos' snappening was only half :)

@Thorin-Oakenpants
Copy link
Contributor

Well, fuck .. there it goes for the third time

@aleyvo
Copy link
Author

aleyvo commented Jul 1, 2022

Another electrical outage and another Thanos' snapping. I'll stick to FF101 and lifetimePolicy until FF103 comes out. July 26th seems so far away in the future :(

@aleyvo
Copy link
Author

aleyvo commented Jul 1, 2022

If I kill FF process, it also happens. 1771024 says that FF101 is unaffected, but that's the version I'm using. I don't get it.

Edit: Damn, my FF was updated to 102 and I hadn't noticed it. So it seems 1771024 is really the culprit in my case.

@opusforlife2
Copy link

I'd like to confirm once because of the seemingly complicated mess here.

I use AF without overriding any cookie-related prefs at all. Everything has the AF default value. No cookie or site data exceptions either. Will it be fine to update to Firefox 102 with the 102 AF? Or should I stay on 101 and wait until 103 like aleyvo intended to above?

@Thorin-Oakenpants
Copy link
Contributor

Aleyvo's issue is with unexpected shutdowns, which was always the case with sanitize onShutdown - so he's going to stick with making all cookies session-only (lifetime pref) and not onShutdown those (cookies, offlineApps). You can't do that in 102 (make all cookies session cookies)

AF v102 will out soon. There's no need to wait IMO. It's working (albeit godamn, there's something fishy about losing them all periodically with a graceful shutdown - maybe that's it, sometimes it doesn't record a graceful shutdown, IDK - this is just my anecdata - it should be solid as fuck - it's just the UI is a mess IMO).

@opusforlife2
Copy link

Thanks. Yeah, I don't need to retain any data or fiddle with the UI; I just want that the data that is supposed to be erased, actually is erased.

@Thorin-Oakenpants
Copy link
Contributor

Thorin-Oakenpants commented Jul 6, 2022

and .. fuck .. a 4th time ... fuck and a 5th time and I am exiting the app cleanly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants