Closed Bug 1233895 Opened 4 years ago Closed 4 years ago

fix up the Feeds.jsm to use the default user context

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: huseby, Assigned: huseby)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId][OA])

Attachments

(1 file, 3 obsolete files)

Fix up the call to createCodebasePrincipal in browser/modules/Feeds.jsm so that it uses the default user context.
Attached patch Bug_1233895.patch (obsolete) — Splinter Review
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)
Component: Security → DOM
Product: Firefox → Core
Whiteboard: [userContextId]
Attachment #8700242 - Flags: review?(jonas)
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)
Attached patch Bug_1233895.patch (obsolete) — Splinter Review
Bobby, here's my proposed fix for this.
Attachment #8700242 - Attachment is obsolete: true
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)
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)
Comment on attachment 8715034 [details] [diff] [review]
Bug_1233895.patch

this patch is still good to go.
Attachment #8715034 - Flags: review?(jonas)
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.
Attached patch Bug_1233895.patch (obsolete) — Splinter Review
already r+ by :sicking.  just adding a comment to the patch that he requested.
Attachment #8715034 - Attachment is obsolete: true
Attachment #8726349 - Flags: review+
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
Looks clean, time to land it.
Keywords: checkin-needed
Can we get a more descriptive commit message for this patch?
Flags: needinfo?(huseby)
r+ by :sicking.  updating the commit message.
Attachment #8726349 - Attachment is obsolete: true
Flags: needinfo?(huseby)
Attachment #8733133 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9e8a70380957
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Whiteboard: [userContextId] → [userContextId][OA]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.