Cleanup cookies should ignore OriginAttributes

RESOLVED FIXED in Firefox 65

Status

()

enhancement
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

7 months ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=1503217#c8

It's not possible to set permissions per containers. We should ignore OriginAttributes when comparing principals here:

https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/browser/modules/Sanitizer.jsm#791-793
Assignee

Comment 1

7 months ago
Posted patch cleanup.patch (obsolete) — Splinter Review
Attachment #9022988 - Flags: review?(jhofmann)
Comment on attachment 9022988 [details] [diff] [review]
cleanup.patch

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

Sorry for missing that in my review...

This looks good to me, but can we have a test case for this?
Attachment #9022988 - Flags: review?(jhofmann) → review-
Assignee

Comment 3

7 months ago
Posted patch cleanup.patchSplinter Review
Attachment #9022988 - Attachment is obsolete: true
Attachment #9023224 - Flags: review?(jhofmann)
Comment on attachment 9023224 [details] [diff] [review]
cleanup.patch

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

Sorry that this took so long, part of me suspects that this could be rewritten to cut down on the complexity but that's really not a reason to hold this off any longer.

Thanks for the great work!

::: browser/modules/Sanitizer.jsm
@@ +762,5 @@
>    hosts.forEach(host => {
>      // Cookies and permissions are handled by origin/host. Doesn't matter if we
>      // use http: or https: schema here.
>      principals.push(
>        Services.scriptSecurityManager.createCodebasePrincipalFromOrigin("https://" + host));

I just noticed that this actually doesn't make sense, we should add principals for both https and http (since both can be in the permission manager), but we already fixed that by doing the hasRootDomain check in maybeSanitizeSessionPrincipals, so it probably doesn't matter.

@@ +785,5 @@
>  
>  function cookiesAllowedForDomainOrSubDomain(principal) {
>    // If we have the 'cookie' permission for this principal, let's return
>    // immediately.
> +  let p = Services.perms.testPermission(principal.URI, "cookie");

I don't understand this change, is it to strip OAs? Those are stripped in the permission manager anyway. I would prefer if we used permission manager APIs with principal unless it can't be avoided, since we want to turn off nsIURI support in the permission manager at some point.
Attachment #9023224 - Flags: review?(jhofmann) → review+
Assignee

Comment 5

7 months ago
> I don't understand this change, is it to strip OAs? Those are stripped in

No it doesn't. We use the origin, which contains the OA.

Comment 7

7 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7fd93c8dd6c
Cleanup cookies should ignore OriginAttributes on shutdown, r=johannh

Comment 8

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c7fd93c8dd6c
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65

Updated

7 months ago
Depends on: 1507171
Hi Andrea, I want to verify this issue and I want to know if the https://bugzilla.mozilla.org/show_bug.cgi?id=1503217#c8 will be enough to verify it, or there are some other steps? Thanks
Flags: needinfo?(amarchesini)
Assignee

Comment 10

7 months ago
Yes, it is. Thanks.
Flags: needinfo?(amarchesini)
I did the exact steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1503217#c8 on Windows 10 and Mac OS X 10.12 with FF Nightly 65.0a1(2018-11-15) and after I quit the browser and open again the cookies.sqlite file, the data for slack.com is still there.
Depends on: 1524200
You need to log in before you can comment on or make changes to this bug.