Closed
Bug 1505071
Opened 6 years ago
Closed 5 years ago
Cleanup cookies should ignore OriginAttributes
Categories
(Toolkit :: Data Sanitization, enhancement)
Toolkit
Data Sanitization
Tracking
()
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
References
Details
Attachments
(1 file, 1 obsolete file)
26.80 KB,
patch
|
johannh
:
review+
|
Details | Diff | Splinter Review |
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•6 years ago
|
||
Attachment #9022988 -
Flags: review?(jhofmann)
Comment 2•6 years ago
|
||
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•6 years ago
|
||
Attachment #9022988 -
Attachment is obsolete: true
Attachment #9023224 -
Flags: review?(jhofmann)
Comment 4•5 years ago
|
||
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•5 years 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.
Assignee | ||
Comment 6•5 years ago
|
||
https://searchfox.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#211-213 I was wrong
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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c7fd93c8dd6c
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment 9•5 years ago
|
||
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)
Comment 11•5 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•