Closed Bug 529899 Opened 10 years ago Closed 4 years ago

Session Restore needs to honor "Keep cookies until I close Firefox" in a clean shut-down

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed
blocking2.0 --- -

People

(Reporter: dveditz, Assigned: ttaubert)

References

(Depends on 1 open bug)

Details

(Keywords: privacy)

Attachments

(1 file, 1 obsolete file)

The cookie option "Keep cookies until I close Firefox" internally means "Turn all cookies into session cookies" so Session Restore is currently restoring them. Although we could change the wording to match our internal model, most people who select that more paranoid option really do want the cookies to go away when they quit Firefox. In a clean user-prompted shutdown (i.e. File->Exit or Apple->Quit) we should throw away session data (cookies and sessionStorage; form data might be OK to keep) for users who have selected that cookie option.

Users will still expect full sessions to be restored after a crash or a prompted restart when installing/upgrading--we should not change those cases. Users who don't like that behavior can set the Session Restore privacy_level pref.

The internal pref to check for this behavior is if "network.cookie.lifetimePolicy" has a value of 2.

It would be better to not save the data in the first place than to save it and not restore it.
Flags: wanted-firefox3.6?
Flags: blocking-firefox3.6?
Pretty late in the game here, but if a patch comes through with tests before we freeze, I'd take it.

Does this bug belong here or in preferences?
blocking2.0: --- → ?
Flags: wanted-firefox3.6?
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6-
(In reply to comment #1)
> Does this bug belong here or in preferences?

Here.

How would this apply to the APIs? getBrowserState currently includes the session cookies. I'd assume that would need to remain the case and we would only strip them out at quit? (FWIW, this would make automated testing difficult)
Attached patch WIP (obsolete) — Splinter Review
I was going to take this, but it seems more involved than I realized, and it's not currently near the top of my todo list, but here's the patch that I've come up with.  This patch tries to not save the cookie info if the lifetime policy pref is 2, but tries to make an exception upon restarting the app.  It doesn't work quite well yet, mainly because of two reasons:

1. The restart part actually doesn't work, although I don't have any idea why.
2. It doesn't try to clear the existing cookie data in sessionstore data, which mainly concerns closed windows.

Once these issues are handled, testing this automatically should be possible by opening a page which has cookies, forcing the sessionstore data to be flushed to disk, and then making sure that the data on disk does not include cookies.
Blocks: 443354
No longer blocks: 443354
See Also: → 443354
(In reply to comment #3)
> Created an attachment (id=413465) [details]

This will stop session cookies from being returned in getBrowserState, which might not be what we want (I would say it's not).

There was also some talk on the call today about just adding a new hidden pref for this. I think that was you Lucas? Does that discussion belong here or in one of the other bugs related to this?
There's already a pref Cookies .... Keep until: [ I close Firefox ] .
I suggest to add an entry there [ I close Firefox or it evaporates itself (don't store them) ]. Problem solved, for my use case - that's what I expected it to do all along. Not intrusive for normal users either, because they never go there.
Sorry, dveditz specifically limited this bug to clean shutdowns.
This didn't block 3.6 and doesn't block 4.0. There's always been tension between the paranoid prefs and session restoration. If someone has a good approach here that has a test and doesn't cause other regressions, I'd take it.
blocking2.0: ? → -
At present, Firefox (4.0b7pre) does _not_ honour my privacy settings because of browser.sessionstore.privacy_level.

I have 'Clear history when Minefield closes' set, with the following set to clear:
- Cookies
- Active Logins
- Cache
- Site Preferences
- Offline Website Data

I.e. all tracking data that would be available to websites.

Note that the 'Settings' window for this has the following text: 'When I quit Minefield'.

I also have 'When Minefield starts' set to 'Show my windows and tabs from last time'.

I understand the rationale for the setting of browser.sessionstore.privacy_level to 1 by default; however, the 'Clear history' settings should_absolutely_ override this.

With my requested options:

privacy_level   Data cleared on quit?    Windows and tabs from last time shown?
1               NO                       Yes
2               Yes                      Yes

Note, I didn't check for 'Session restored' (which is ambiguous and not what the startup option refers to).

For some idea of the implications of this: http://samy.pl/evercookie/
What I'd like to see is a pref that allows session cookies and the like to be flushed (I use CS Lite to whitelist login cookies on sites I want to persist between sessions) while still preserving scroll positions so I can restart Firefox without losing my place in a story.
Stephan: Setting browser.sessionstore.privacy_level to 2 will retain your scroll position in tabs. I am unsure what exactly the difference between level 1 and 2 is. Here is an explanation, but it is not clear what exactly is saved: http://blog.zpao.com/post/1099464627/restore-previous-session

I really would like to see the settings available in browser.sessionstore.privacy_level be exposed in the UI as part of the Privacy > History tab in the Prefs dialog if there is a substantial difference between level 1 and 2 except for storing of cookies.
The prefs (privacy_level and privacy_level_deferred) control form data, session cookies, and sessionstorage.

0 = save for HTTP and HTTPS
1 = save for HTTP
2 = don't save anything

Scroll position, window size, etc aren't really considered private in any way, so those are stored regardless of that preference value.
Makes sense. I'll have to re-locate the page in the Mozillazine Knowledge Base that said otherwise and make a correction then.
Duplicate of this bug: 632332
Temporal workaround for me: Use Sesson Manager Addon (https://addons.mozilla.org/de/firefox/addon/session-manager/) to handle session restore. This way (session) cookies are removed on close.
To me "Accept third party cookies until I close Firefox", means "destroy all cookies when I close Firefox". So I was rather surprised to see a bunch of cookies still being stored after a clean shutdown.

I don't have "restore session" checked. What I do have checked is "When firefox starts, show my windows and tabs from last time". No mention of "session" there.
btw, probably the most frustrating aspect of this bug for me is that the ONLY reason I want firefox to restart showing "my windows and tabs from last time" is so that I can close Firefox at the end of the day *specifically to clear my cookies*, and yet come back the next day having the same pages open that I was looking at before I decided to clear out the cookies.

I suppose a better solution for me would be just to click the button that says "clear all cookies", but closing the browser every now and again just feels more natural.
Duplicate of this bug: 401187
Can this bug be prioritized somehow? Current behavior clearly violates the user expectations based on the checkbox language in Privacy prefs. Wouldn't the simplest fix be to expose browser.sessionstore.privacy_level setting in Privacy tab with some sensible text?
Duplicate of this bug: 1139190
Duplicate of this bug: 1214698
Attachment #413465 - Attachment is obsolete: true
Hope you can review this, David. It's a small patch that clears all cookies on a clean shutdown when the user set their cookie prefs to "Keep until I close Firefox".

Cookies are kept when crashing, basically always when we don't shutdown cleanly. We also keep cookies when we restart to apply an update or similar.

We have no way to automatically test this, I don't think we should move this into the SessionWorker because 1) it's a fast operation and 2) we'd have to forward preference values to the worker.

I smoked-tested that with "Keep until I close Firefox" we remove cookies on clean shutdown, and keep them when restarting or crashing. Please note that this only applies to cookies that have been set *after* "Keep until ..." was switched on, because that makes them fake session cookies. If you set cookies before that then they're stored in the permanent cookie store and session store can't do anything about that.
Attachment #8718373 - Flags: review?(dteller)
(In reply to Tim Taubert [:ttaubert] from comment #22)
> Please note that
> this only applies to cookies that have been set *after* "Keep until ..." was
> switched on, because that makes them fake session cookies. If you set
> cookies before that then they're stored in the permanent cookie store and
> session store can't do anything about that.

Good that you're not "fixing" this behavior. It's essential to use "Keep until ..." and still be able to (manually) have some sites' "remember me" working.
Comment on attachment 8718373 [details] [diff] [review]
0001-Bug-529899-Purge-cookies-on-clean-shutdown-with-Keep.patch

Review of attachment 8718373 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

::: browser/components/sessionstore/SessionSaver.jsm
@@ +215,5 @@
> +    // their cookie preferences to "Keep until I close Firefox", then we
> +    // should remove all cookies. Check "resume_session_once" so we keep
> +    // cookies when restarting due to a Firefox update.
> +    if (RunState.isClosing &&
> +        Services.prefs.getIntPref("network.cookie.lifetimePolicy") == 2 &&

Could you replace the magic 2 with `Ci.nsICookieService.ACCEPT_SESSION` ?
Attachment #8718373 - Flags: review?(dteller) → review+
Assignee: nobody → ttaubert
(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #24)
> ::: browser/components/sessionstore/SessionSaver.jsm
> @@ +215,5 @@
> > +    // their cookie preferences to "Keep until I close Firefox", then we
> > +    // should remove all cookies. Check "resume_session_once" so we keep
> > +    // cookies when restarting due to a Firefox update.
> > +    if (RunState.isClosing &&
> > +        Services.prefs.getIntPref("network.cookie.lifetimePolicy") == 2 &&
> 
> Could you replace the magic 2 with `Ci.nsICookieService.ACCEPT_SESSION` ?

Good idea, will do.
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/39742e742b69
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Blocks: 1260360
This recently stopped working, apparently Services.cookies.ACCEPT_SESSION is undefined. Ci.nsICookieService.ACCEPT_SESSION reports the correct value.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please always file new bugs instead of opening old ones.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Depends on: 1296167
You need to log in before you can comment on or make changes to this bug.