Closed Bug 1238177 Opened 4 years ago Closed 4 years ago

extension content needs to use the correct user context id origin attribute

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, 4 obsolete files)

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.
Attached patch Bug_1238177.patch (obsolete) — Splinter Review
this still needs some tests to prove that it is correct.
Attached patch Bug_1238177.patch (obsolete) — Splinter Review
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)
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 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+
Attached patch Bug_1238177.patch (obsolete) — Splinter Review
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+
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
Attached patch Bug_1238177.patch (obsolete) — Splinter Review
r+ by :sicking.  fixed error in unit tests.
Attachment #8737423 - Attachment is obsolete: true
Attachment #8737539 - Flags: review+
Keywords: checkin-needed
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)
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+
looks good.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3ce2c626f6ba
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.