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

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: huseby, Assigned: huseby)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [userContextId][OA])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

2 years ago
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

2 years ago
Created attachment 8715153 [details] [diff] [review]
Bug_1238177.patch

this still needs some tests to prove that it is correct.
(Assignee)

Comment 2

2 years ago
Created attachment 8722054 [details] [diff] [review]
Bug_1238177.patch

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

2 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 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

2 years ago
Created attachment 8737423 [details] [diff] [review]
Bug_1238177.patch

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 9

2 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

2 years ago
Created attachment 8737539 [details] [diff] [review]
Bug_1238177.patch

r+ by :sicking.  fixed error in unit tests.
Attachment #8737423 - Attachment is obsolete: true
Attachment #8737539 - Flags: review+
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 15

2 years ago
Created attachment 8737904 [details] [diff] [review]
Bug_1238177.patch

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+
(Assignee)

Comment 16

2 years ago
looks good.
Keywords: checkin-needed

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3ce2c626f6ba
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Updated

2 years ago
Whiteboard: [userContextId] → [userContextId][OA]
You need to log in before you can comment on or make changes to this bug.