Closed
Bug 1233895
Opened 8 years ago
Closed 8 years ago
fix up the Feeds.jsm to use the default user context
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: huseby, Assigned: huseby)
References
Details
(Whiteboard: [userContextId][OA])
Attachments
(1 file, 3 obsolete files)
1.32 KB,
patch
|
huseby
:
review+
|
Details | Diff | Splinter Review |
Fix up the call to createCodebasePrincipal in browser/modules/Feeds.jsm so that it uses the default user context.
Assignee | ||
Comment 1•8 years ago
|
||
This fixes the Feeds.jsm to use the default context origin attributes. We aren't going to isolate subscribed feeds in the first iteration.
Attachment #8700242 -
Flags: review?(jonas)
Assignee | ||
Updated•8 years ago
|
Component: Security → DOM
Product: Firefox → Core
Assignee | ||
Updated•8 years ago
|
Whiteboard: [userContextId]
Assignee | ||
Updated•8 years ago
|
Attachment #8700242 -
Flags: review?(jonas)
Assignee | ||
Comment 2•8 years ago
|
||
Bobby, In this file I'm trying to fix up the code below. I need to pass the aPrincipal.originAttributes to the function on line 88. BUT, the comment on line 85 suggests that I may need to do something special with aPrincipal.originAttributes since I may be dealing with CPOWs. I thought we were killing all of the CPOWs. > 85 // re-create the principal as it may be a CPOW. > 86 let principalURI = BrowserUtils.makeURIFromCPOW(aPrincipal.URI); > 87 let principalToCheck = > 88 Services.scriptSecurityManager.createCodebasePrincipal(principalURI, {}); > 89 try { > 90 BrowserUtils.urlSecurityCheck(aLink.href, principalToCheck, > 91 Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL); My proposed fixed is to just change line 88 to the following: > 88 Services.scriptSecurityManager.createCodebasePrincipal(principalURI, aPrincipal.originAttributes); Will that work?
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 3•8 years ago
|
||
Bobby, here's my proposed fix for this.
Assignee | ||
Updated•8 years ago
|
Attachment #8700242 -
Attachment is obsolete: true
Comment 4•8 years ago
|
||
Looks like the CPOW handling was added in may, in bug 1144493. Given that CPOWs are now banned in browser code, I'm guessing that we should be fine here. So I'd say we should just use aPrincipal directly. mconley, does that sound right?
Flags: needinfo?(bobbyholley) → needinfo?(mconley)
Comment 5•8 years ago
|
||
Note that it's _unsafe_ CPOW usage that has been banned in browser code. This means that if the content process is blocked on the parent, then CPOW access is still possible from the parent (this is "safe" CPOW usage). A quick look at the callers of isValidFeed, I see content.js and ContentLinkHandler.jsm, both of which should be running in the content process for any cases where you'd have the chance of hitting CPOWs, and both of which seem to be passing the right things directly (the document principals). A look at the blame of that file, it appears that these bits were added in order to make Page Info limp along using CPOWs. Page Info, since that time, has been modified to use messages, so I doubt this case applies any longer. So I think your change makes sense. If you want to be super-cautious, you could throw if Cu.isCrossProcessWrapper returns true for the aPrincipal, but I really can't see how you'd hit that situation.
Flags: needinfo?(mconley)
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8715034 [details] [diff] [review] Bug_1233895.patch this patch is still good to go.
Attachment #8715034 -
Flags: review?(jonas)
Attachment #8715034 -
Flags: review?(jonas) → review+
Maybe add a comment saying that once this can't be a CPOW any more, that we should simply use "aPrincipal" rather than create a new principal.
Assignee | ||
Comment 8•8 years ago
|
||
already r+ by :sicking. just adding a comment to the patch that he requested.
Attachment #8715034 -
Attachment is obsolete: true
Attachment #8726349 -
Flags: review+
Assignee | ||
Comment 9•8 years ago
|
||
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55d4f26a7a08
Assignee | ||
Comment 10•8 years ago
|
||
lot's of reds in windows 7. let's try it one more time with the latest code: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe6afa5d6211
Assignee | ||
Comment 11•8 years ago
|
||
another try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6790227c342
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c0b42d8790c
Can we get a more descriptive commit message for this patch?
Flags: needinfo?(huseby)
Assignee | ||
Comment 15•8 years ago
|
||
r+ by :sicking. updating the commit message.
Attachment #8726349 -
Attachment is obsolete: true
Flags: needinfo?(huseby)
Attachment #8733133 -
Flags: review+
Comment 16•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9e8a70380957
Keywords: checkin-needed
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9e8a70380957
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Updated•8 years ago
|
Whiteboard: [userContextId] → [userContextId][OA]
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•