1643688 - tabs.create on GeckoView can use cookieStoreId without the right permissions or preference

Status: NEW

(GeckoView :: Extensions, defect, P3)

Description

[Rob Wu [:robwu]]

Bug 1622500 introduces support for the "cookieStoreId" parameter, but skips some important checks that are present on desktop Firefox: https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/browser/components/extensions/parent/ext-tabs.js#671-678

The checks are in getUserContextIdForCookieStoreId and enforce several requirements:

• Extension must have "cookies" permission.
• Container tabs must be enabled by preference.
• If "cookieStoreId" is "firefox-private", then it must match the private state of the window. This implicitly enforces the requirement that extensions must have access to that window; but since GV doesn't distinguish between windows and tabs yet, it must separately enforce the private browsing checks.
• Checks whether "cookieStoreId" is a valid (and existing) identifier.

In bug 1622500, the cookieStoreId seems to be given by the GeckoView app (introduced in bug 1501108). This should not be done. The cookieStoreId in the extension API is an abstraction of three kinds of isolation:

• normal browsing "firefox-default"
• private browsing "firefox-private"
• container tabs "firefox-container-<internal container id>"

The current version of the API lacks the necessary checks, and performs the mapping at the wrong level. The identifier for container tabs should be derived from Gecko platform information (OriginAttributes), NOT from GV-specific data.

The "cookieStoreId" is used in other APIs, including cookies and webRequest. In order to have a consistent and functional API, various places where "userContextId" is used should recognize "geckoViewSessionContextId" too.

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

Hey Rob!

I think in general a misunderstanding here is that GeckoView does not do any container management, so we can't really just use the values that gecko knows about. We also allow private browsing containers and that's why we didn't implement firefox-default and firefox-private. Those ids are already special and don't behave the same from normal container IDs, don't really give additional information and are not documented anywhere so I wouldn't expect extensions to use them in a meaningful way, but I'm happy to be proven wrong.

(In reply to Rob Wu [:robwu] from comment #0)

Bug 1622500 introduces support for the "cookieStoreId" parameter, but skips some important checks that are present on desktop Firefox: https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/browser/components/extensions/parent/ext-tabs.js#671-678

The checks are in getUserContextIdForCookieStoreId and enforce several requirements:

• Extension must have "cookies" permission.

good point, this was missed during code review, let's add a check.

• Container tabs must be enabled by preference.

In general apps don't have a way to enable prefs, so this is not really useful for us.

• If "cookieStoreId" is "firefox-private", then it must match the private state of the window. This implicitly enforces the requirement that extensions must have access to that window; but since GV doesn't distinguish between windows and tabs yet, it must separately enforce the private browsing checks.

GeckoView supports having private container tabs, if we did something like that we could not implement it. As a I understand it firefox-private is an implementation detail not documented anywhere so it shouldn't hurt that much? when we get to the full implementation of the API apps will be able to do this if they want to.

• Checks whether "cookieStoreId" is a valid (and existing) identifier.

GeckoView doesn't keep a list of "valid" cookie store IDs, that's entirely to the app to decide.

In bug 1622500, the cookieStoreId seems to be given by the GeckoView app (introduced in bug 1501108). This should not be done. The cookieStoreId in the extension API is an abstraction of three kinds of isolation:

• normal browsing "firefox-default"
• private browsing "firefox-private"
• container tabs "firefox-container-<internal container id>"

This is a leaky abstraction though, isn't it? apps can't really create a "firefox-private" tab in a non-private window. Or a container window in a private window, so they need to be aware that firefox-private and firefox-default are and how they work differently than containers.

The current version of the API lacks the necessary checks, and performs the mapping at the wrong level. The identifier for container tabs should be derived from Gecko platform information (OriginAttributes), NOT from GV-specific data.

we can't do that because the app never sees those identifiers, and the app is responsible for creating/modifying/deleting "contextual identies". We have a mapping though so we should be able to fix the extension APIs to understand us.

The "cookieStoreId" is used in other APIs, including cookies and webRequest. In order to have a consistent and functional API, various places where "userContextId" is used should recognize "geckoViewSessionContextId" too.

yep, good point we need to fix those APIs to use our context ID too.

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Agi Sferro | :agi | ⏰ PST | he/him from comment #1)

good point, this was missed during code review, let's add a check.

opened Bug 1643736

yep, good point we need to fix those APIs to use our context ID too.

Bug 1643740

Comment 3

[Rob Wu [:robwu]]

(In reply to Agi Sferro | :agi | ⏰ PST | he/him from comment #1)

Hey Rob!

I think in general a misunderstanding here is that GeckoView does not do any container management, so we can't really just use the values that gecko knows about. We also allow private browsing containers

Gecko does not do automatic container management either. Containers are created through ContextualIdentityService.create. For the lack of anything external, the IDs are automatically generated based on a counter, but there is no technical reason for it to not be an externally supplied value (e.g. from GV).

userContextId is currently disabled in private windows on desktop, but that is due to UX concerns (bug 1320757). Containers in private mode can be supported. If mobile has a good UX for private/non-private containers, then it would be preferred to support this on desktop as well.

I understand that you'd like GV to allow embedders to manage container tabs, but IMO the responsibility of knowing the canonical set of containers lies with Gecko, not GeckoView. Origin attributes are used in many places, and without ownership of the container identifiers, Gecko will likely accumulate lots of junk (e.g. in storage) in the long run.

I strongly object to using geckoViewSessionContextId for the "container tab" feature, because it is mostly duplication of existing functionality, but from scratch. This leads to bugs and increases the maintenance burden, as well as an extension API that differs on desktop and mobile.

The current direction is to add geckoViewSessionContextId to every API that you are aware of that previously recognized userContextId. There will be gaps. There are many feature requests (default container for bookmarks, container-specific history, container-specific extensions, to name a few) that are independent of platform, whose implementation will become unnecessarily complicated/duplicated by using something other than userContextId.

We also allow private browsing containers and that's why we didn't implement firefox-default and firefox-private. Those ids are already special and don't behave the same from normal container IDs, don't really give additional information and are not documented anywhere so I wouldn't expect extensions to use them in a meaningful way, but I'm happy to be proven wrong.

The "cookieStoreId" concept originated from the cookies API (see cookies.CookieStore type, which in turn originates from Chrome ("private" is Firefox-only though). The design of the extension API is already generic enough to support container tabs in private browsing. The names "firefox-private" and "firefox-default" aren't significant, the concept that they represent is:

• "firefox-default" is the non-container context in normal browsing mode.
• "firefox-private" is the non-container context in private browsing mode.

Every cookie must be associated with a cookie store, and clearly every tab/frame/request is also associated with a cookie store.

The cookieStoreId is just an identifier for the CookieStore concept. The ID does NOT restrictions on the possibility of having private containers.

(In reply to Rob Wu [:robwu] from comment #0)

Bug 1622500 introduces support for the "cookieStoreId" parameter, but skips some important checks that are present on desktop Firefox: https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/browser/components/extensions/parent/ext-tabs.js#671-678

The checks are in getUserContextIdForCookieStoreId and enforce several requirements:

• Extension must have "cookies" permission.

good point, this was missed during code review, let's add a check.

• If "cookieStoreId" is "firefox-private", then it must match the private state of the window. This implicitly enforces the requirement that extensions must have access to that window; but since GV doesn't distinguish between windows and tabs yet, it must separately enforce the private browsing checks.

GeckoView supports having private container tabs, if we did something like that we could not implement it.

I'm referring to enforcing the restriction that extensions without private browsing access should not be able to open private tabs.

• Checks whether "cookieStoreId" is a valid (and existing) identifier.

GeckoView doesn't keep a list of "valid" cookie store IDs, that's entirely to the app to decide.

... and if the app forgets about an ID, Gecko will not be able to recover (clean up data associated with stale cookie stores).

As a I understand it firefox-private is an implementation detail not documented anywhere so it shouldn't hurt that much? when we get to the full implementation of the API apps will be able to do this if they want to.

In bug 1622500, the cookieStoreId seems to be given by the GeckoView app (introduced in bug 1501108). This should not be done. The cookieStoreId in the extension API is an abstraction of three kinds of isolation:

• normal browsing "firefox-default"
• private browsing "firefox-private"
• container tabs "firefox-container-<internal container id>"

This is a leaky abstraction though, isn't it? apps can't really create a "firefox-private" tab in a non-private window. Or a container window in a private window, so they need to be aware that firefox-private and firefox-default are and how they work differently than containers.

Apps don't even need to see "firefox-default" / "firefox-private". They are used in the extension framework to identify the "default" tabs.
Only container tabs need to be passed up. As you can see from the choice of the cookieStoreId format, the exact format of the names are not important; as long as there is an unambiguous way to map cookieStoreId to the "container" concept.

Since containers in private and non-private mode are not sharing any cookies, using different IDs for the cookie stores would be correct. "Containers" may share more state than just cookies (e.g. icons / name / features .... ), which could be solved by allowing contextual identities to inherit (for example - just putting it out there to show that the CookieStore concept can be extended to support many needs).

The current version of the API lacks the necessary checks, and performs the mapping at the wrong level. The identifier for container tabs should be derived from Gecko platform information (OriginAttributes), NOT from GV-specific data.

we can't do that because the app never sees those identifiers, and the app is responsible for creating/modifying/deleting "contextual identies". We have a mapping though so we should be able to fix the extension APIs to understand us.

As explained at the top of my comment, using userContextId does not preclude the app from doing the management of containers. Gecko offers a backend to manage containers, GV can be a client to this backend and interface with the app.

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

Discussed with :snorp end :esawin. There are two parts to this:

firefox-default / firefox-private: will change the GeckoView implementation to match desktop
userContextId: what we want to do is move the desktop-specific bits to browser/ so we can safely use this on GV too. I have to talk to the owners of this code about it yet, but hopefully they won't mind

Comment 5

[[:owlish] 🦉 PST]

still relevant