Closed Bug 1352365 Opened 3 years ago Closed 3 years ago

Remove duplicate PrivacyLevel checks in SessionCookies.jsm

Categories

(Firefox :: Session Restore, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: ttaubert, Assigned: ttaubert)

Details

Attachments

(1 file, 2 obsolete files)

PrivacyLevel checks currently allow to disable storing secure cookies and any cookies belonging to an HTTPS host, or completely disable storing cookies. We call PrivacyLevel.canSave() for every host found in the shistory of a given window's tabs. We then call it again for every cookie when retrieving all cookies stored for a given host.

The two different privacy checks exist because in the past an HTTP site could send a secure cookie too. Since Firefox 52 this isn’t possible anymore, only HTTPS sites can send secure cookies. So as soon as nsICookie.isSecure=true we know the site was loaded over TLS.

That means there are the following scenarios:

[PRIVACY_LEVEL=NONE] (default)
We store all cookies.

[PRIVACY_LEVEL=FULL]
We store no cookies at all.

[PRIVACY_LEVEL=ENCRYPTED]
HTTP site sends cookie: Store.
HTTP site sends secure cookie: Can't happen since Fx52
HTTPS site sends cookie: Store. The site is HTTPS but we should store the cookie anyway because the "Secure" directive is missing. That means the site wants us to send it for HTTP requests too.
HTTPS site sends secure cookie: Don't store.

This allows us to simplify the code and remove the per-host PrivacyLevel checks. Checking nsICookie.isSecure is enough to tell whether we want to keep a cookie or not.
Small correction.
Attachment #8853349 - Attachment is obsolete: true
Attachment #8853349 - Flags: review?(mdeboer)
Attachment #8853352 - Flags: review?(mdeboer)
Comment on attachment 8853352 [details] [diff] [review]
0001-Bug-1352365-Remove-duplicate-PrivacyLevel-checks-in-.patch, v2

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

Nice! Thanks for the attached explainer brief; very helpful.
Attachment #8853352 - Flags: review?(mdeboer) → review+
Added some tests because I don't trust myself :)
Attachment #8853352 - Attachment is obsolete: true
Attachment #8853356 - Flags: review?(mdeboer)
Comment on attachment 8853356 [details] [diff] [review]
0001-Bug-1352365-Remove-duplicate-PrivacyLevel-checks-in-.patch, v3

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

Thanks for the test! Now it's even better :)
Attachment #8853356 - Flags: review?(mdeboer) → review+
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc68a5077b7
Remove duplicate PrivacyLevel checks in SessionCookies.jsm r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/ecc68a5077b7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.