1238182 - extensions need to use origin attributes correctly

Status: RESOLVED WONTFIX

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

Description

[Dave Huseby [:huseby]]

in the file toolkit/components/extensions/Extension.jsm there is a call to createCodebasePrincipal to create a principal that is later used to call checkLoadURIStrWithPrincipal.

Comment 1

[Dave Huseby [:huseby]]

Bobby,

this is one of the cases where i don't know what the right thing to do is.  i'm not sure if the principal that the extension.jsm creates should be using the default user context (no user context) or should it use a user context id to isolate on user contexts?  i'm not sure what the checkLoadURIStrWithPrincipal does.

i've got Bug 1238177 which will make extension context objects use the userContextId set in the origin attributes in the associated document.  honestly, i don't know what the extension.jsm code does so i can't tell if we need to isolate here too or not.

thoughts?

Comment 2

[Bobby Holley (:bholley)]

Are these WebExtension content scripts? If so, we probably want to give them the same user context as the content they're attaching to.

Comment 3

[Bobby Holley (:bholley)]

Actually, this looks like something else, not sure what. Bill?

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#883

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This is for stuff like extension background pages, browser action popups, etc. It's all loaded from moz-extension: URIs into content docshells. Right now it runs in the chrome process. Eventually it will run in a special extension process.

I don't know what a "user context" is, but I hope this is enough to resolve any questions.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #4)
> This is for stuff like extension background pages, browser action popups,
> etc. It's all loaded from moz-extension: URIs into content docshells. Right
> now it runs in the chrome process. Eventually it will run in a special
> extension process.

Ok. If it's loaded into a content docshell then we probably want the user context of the docshell.

> I don't know what a "user context" is, but I hope this is enough to resolve
> any questions.

An OriginAttribute that allows us to separate cookies etc between different tabs, so that you can be logged into a website with two different accounts in two different tabs.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bobby Holley (busy) from comment #5)
> Ok. If it's loaded into a content docshell then we probably want the user
> context of the docshell.

These are docshells we create specifically for extensions. They're not even part of the DOM. So I'm not sure what that would mean.

This sounds really tricky to get right.

Comment 7

[Tanvi Vyas[:tanvi]]

(In reply to Bill McCloskey (:billm) from comment #6)
> (In reply to Bobby Holley (busy) from comment #5)
> > Ok. If it's loaded into a content docshell then we probably want the user
> > context of the docshell.
> 
> These are docshells we create specifically for extensions. They're not even
> part of the DOM. So I'm not sure what that would mean.
> 
> This sounds really tricky to get right.

Are network requests being made to get content for these extensions?  Are cookies sent with those requests (or can they be set with the responses)?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

These pages can request external images, for example. I suspect Chrome uses the user's normal cookies to access those. They do have some special rules for private browsing.

Are we really going to do this per-tab? If it were per-window, then we could say that the popups in that window should use the cookies of the user owning the window. If it's per-tab, then I'm not sure what we'd do.

There's also a background page that's not displayed to the user. It's not associated with any window. For that, we could perhaps not use cookies at all. That would break compatibility with Chrome, but any extension relying on it is probably doing some sort of tracking anyway.

I'm definitely not an expert on cookies or security, though, so I'm kinda just saying stuff.

Comment 9

[Tim Huang[:timhuang]]

Does there any way that we could know this code is running on the background page or on the content page. If we can know where it is running, we could use contextual id accordingly.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, we can separate the background page case from other cases (although that's not how the code works right now).

Comment 11

[Tim Huang[:timhuang]]

IMO, we should use the default origin attributes here. Because the principal of the Extension APIs which injects into the content window is handled by https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#267 properly. And the principal which is created by createCodeBasePrincipal here is for background page. One web extension only shares one background page with all content windows. Thus, the default origin attributes should be the best option here.

Comment 12

[Tim Huang[:timhuang]]

Attachment: WIP - Fix the Extension.jsm to use the correct origin attributes.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8738479 [details] [diff] [review]
WIP - Fix the Extension.jsm to use the correct origin attributes.

Why are you removing the addonId? That's necessary in order to make [1] work:

https://hg.mozilla.org/mozilla-central/file/494289c72ba3/caps/BasePrincipal.cpp#l593

I also don't understand why you need to explkicitly call createDefaultOriginAttributes(). Passing {} to createCodebasePrincipal should be enough - all non-specified properties are assumed to be of the default value.

Comment 14

[Tim Huang[:timhuang]]

Comment on attachment 8738479 [details] [diff] [review]
WIP - Fix the Extension.jsm to use the correct origin attributes.

According to comment 13 and the changes of ChromeUtils APIs that describe in Bug 1238177, I think the code here is correct.