Closed Bug 1505071 Opened 2 years ago Closed 2 years ago

Cleanup cookies should ignore OriginAttributes

Categories

(Toolkit :: Data Sanitization, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Attached 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-
Attached 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+
> 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.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7fd93c8dd6c
Cleanup cookies should ignore OriginAttributes on shutdown, r=johannh
https://hg.mozilla.org/mozilla-central/rev/c7fd93c8dd6c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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)
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.