Closed
Bug 1238177
Opened 8 years ago
Closed 8 years ago
extension content needs to use the correct user context id origin attribute
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, 4 obsolete files)
8.62 KB,
patch
|
huseby
:
review+
|
Details | Diff | Splinter Review |
in the file toolkit/components/extensions/ExtensionContent.jsm there's code in the ExtensionContext constructor that creates a codebase principal for the extension. It has the associated document which will have the correct userContextId origin attribute. Here's the code:
> 237 if (ssm.isSystemPrincipal(contentPrincipal)) {
> 238 // Make sure we don't hand out the system principal by accident.
> 239 prin = Cc["@mozilla.org/nullprincipal;1"].createInstance(Ci.nsIPrincipal);
> 240 } else {
> 241 let extensionPrincipal = ssm.createCodebasePrincipal(this.extension.baseURI, {addonId: extensionId});
> 242 prin = [contentPrincipal, extensionPrincipal];
> 243 }
the solution is to change the system principal case so that the created null principal has the user context id set on its origin attributes. in the else case, we just need to pass the user context id to the call to createCodePrincipal.
Assignee | ||
Comment 1•8 years ago
|
||
this still needs some tests to prove that it is correct.
Assignee | ||
Comment 2•8 years ago
|
||
updated for the new API.
Attachment #8715153 -
Attachment is obsolete: true
Attachment #8722054 -
Flags: review?(jonas)
Comment on attachment 8722054 [details] [diff] [review] Bug_1238177.patch Review of attachment 8722054 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/extensions/ExtensionContent.jsm @@ +263,5 @@ > + // copy origin attributes from the content window origin attributes to > + // preserve the user context id. overwrite the addonId. > + let attrs = ChromeUtils.originAttributesFromDict(contentPrincipal.originAttributes); > + attrs.addonId = extensionId; > + let extensionPrincipal = ssm.createCodebasePrincipal(this.extension.baseURI, attrs); Someone else should probably review this part. I don't feel like I know what 'contentPrincipal' refers to here to know if it's the right thing to use. Maybe we can look through together to see if it looks correct. @@ +270,5 @@ > > if (ssm.isSystemPrincipal(contentPrincipal)) { > // Make sure we don't hand out the system principal by accident. > + // also make sure that the null principal has the right origin attributes > + prin = ssm.createNullPrincipal(attrs); Does it really matter what originattributes a nullprincipal has? I'm somewhat surprised that we even have a function with this signature. Can a nullprincipal access any APIs which uses originattributes? It can't use IndexedDB/localStorage as far as I know. Can it make network requests which use cookies?
Comment on attachment 8722054 [details] [diff] [review] Bug_1238177.patch Review of attachment 8722054 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/extensions/ExtensionContent.jsm @@ +263,5 @@ > + // copy origin attributes from the content window origin attributes to > + // preserve the user context id. overwrite the addonId. > + let attrs = ChromeUtils.originAttributesFromDict(contentPrincipal.originAttributes); > + attrs.addonId = extensionId; > + let extensionPrincipal = ssm.createCodebasePrincipal(this.extension.baseURI, attrs); But it's probably better to have bholley or someone that knows addons review this part. @@ +270,5 @@ > > if (ssm.isSystemPrincipal(contentPrincipal)) { > // Make sure we don't hand out the system principal by accident. > + // also make sure that the null principal has the right origin attributes > + prin = ssm.createNullPrincipal(attrs); Per discussions with bholley, we should still try to set the originAttributes on nullprincipals where we can. So this part is good.
Attachment #8722054 -
Flags: review?(jonas)
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8722054 [details] [diff] [review] Bug_1238177.patch Bobby, Can you give me a quick review? This patch propagates the window origin attributes to the principal created for the scope in which extension content scripts can run. This is necessary to get the correct user context set for the scope so caching, cookies, etc will be properly isolated by userContextId. --dave
Attachment #8722054 -
Flags: review?(bobbyholley)
Comment 6•8 years ago
|
||
Comment on attachment 8722054 [details] [diff] [review] Bug_1238177.patch Review of attachment 8722054 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/extensions/ExtensionContent.jsm @@ +261,5 @@ > let ssm = Services.scriptSecurityManager; > > + // copy origin attributes from the content window origin attributes to > + // preserve the user context id. overwrite the addonId. > + let attrs = ChromeUtils.originAttributesFromDict(contentPrincipal.originAttributes); Per IRC conversation, there's no reason to call originAttributesFromDict. I also think that method should be renamed to something like originAttributesWithDefaultValues or somesuch, since we've generally considered it fine to pass around origin attributes as dictionary objects with only the non-default values enumerated (since all C++ APIs will fill in the default values on the way in). Maybe file a followup for that?
Attachment #8722054 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 7•8 years ago
|
||
already r+ by :bholley, this updates the patch with the last changes he wanted and adds a better commit message.
Attachment #8722054 -
Attachment is obsolete: true
Attachment #8737423 -
Flags: review+
Assignee | ||
Comment 8•8 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dc515ad49db
Assignee | ||
Comment 9•8 years ago
|
||
lots of red in that last push. one more time to be sure it is us: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49b4f3eabccb
Assignee | ||
Comment 10•8 years ago
|
||
r+ by :sicking. fixed error in unit tests.
Attachment #8737423 -
Attachment is obsolete: true
Attachment #8737539 -
Flags: review+
Assignee | ||
Comment 11•8 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=676d8cd74310
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0519406b6e57
Keywords: checkin-needed
Comment 13•8 years ago
|
||
backed out for test failures like TEST-UNEXPECTED-ERROR | toolkit/components/extensions/ExtensionContent.jsm:301:17 | "ChromeUtils" is not defined. (no-undef) (https://treeherder.mozilla.org/logviewer.html#?job_id=25059442&repo=mozilla-inbound)
Flags: needinfo?(huseby)
Comment 14•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/e86362173ca4
Assignee | ||
Comment 15•8 years ago
|
||
removed reference to ChromeUtils, that shouldn't have been in there. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a02d735cae96
Attachment #8737539 -
Attachment is obsolete: true
Flags: needinfo?(huseby)
Attachment #8737904 -
Flags: review+
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ce2c626f6ba
Keywords: checkin-needed
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3ce2c626f6ba
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
•