1352365 - Remove duplicate PrivacyLevel checks in SessionCookies.jsm

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement)

Description

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1352365-Remove-duplicate-PrivacyLevel-checks-in-.patch

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1352365-Remove-duplicate-PrivacyLevel-checks-in-.patch, v2

Small correction.

Comment 3

[Mike de Boer [:mikedeboer]]

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.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1352365-Remove-duplicate-PrivacyLevel-checks-in-.patch, v3

Added some tests because I don't trust myself :)

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8cf7f1fbb089d8618a7b881ab09c7e54f7a036b

Comment 6

[Mike de Boer [:mikedeboer]]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc68a5077b7
Remove duplicate PrivacyLevel checks in SessionCookies.jsm r=mikedeboer

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ecc68a5077b7