Closed Bug 1505071 Opened 3 years ago Closed 3 years ago

Cleanup cookies should ignore OriginAttributes


(Toolkit :: Data Sanitization, enhancement)

Not set



Tracking Status
firefox65 --- fixed


(Reporter: baku, Assigned: baku)




(1 file, 1 obsolete file)


It's not possible to set permissions per containers. We should ignore OriginAttributes when comparing principals here:
Attached patch cleanup.patch (obsolete) — Splinter Review
Attachment #9022988 - Flags: review?(jhofmann)
Comment on attachment 9022988 [details] [diff] [review]

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]

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
Cleanup cookies should ignore OriginAttributes on shutdown, r=johannh
Closed: 3 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 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 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 is still there.
Depends on: 1524200
You need to log in before you can comment on or make changes to this bug.