1391992 - add cookieStoreId to webRequest APIs

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P1)

Description

[Jonathan Kingston [:jkt] he/him]

As requests through service workers don't have a tab there isn't a way to distinguish service worker requests from a container.

Comment 1

[Firefox Bug Husbandry Bot]

Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020

Comment 2

[erosman]

Semi-related: If the request can be expanded to include 'incognito' as well, it would benefit the container implementation.

Comment 3

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1391992 - Add cookieStoreId to webRequest extension API.

Review commit: https://reviewboard.mozilla.org/r/235354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235354/

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8966639 [details]
Bug 1391992 - Add cookieStoreId to webRequest extension API.

https://reviewboard.mozilla.org/r/235354/#review241044


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/parent/ext-webRequest.js:31
(Diff revision 1)
>        }
>  
>        let event = data.serialize(eventName);
>        event.tabId = browserData.tabId;
> +      if (data.originAttributes) {
> +        event.cookieStoreId = getCookieStoreIdForOriginAttributes(data.originAttributes);

Error: 'getcookiestoreidfororiginattributes' is not defined. [eslint: no-undef]

Comment 5

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8966639 [details]
Bug 1391992 - Add cookieStoreId to webRequest extension API.

Hey :aswan, :kmag,

Is there a nicer way to share ext-toolkit.js into WebRequest.jsm without having to handle cookieStoreId in the ext-webRequest.js

I will add a test after you are happy. Assuming this is OK API wise too.

This came up on contain-facebook, given sites like web.whatsapp use a serviceworker so the extension couldn't detect the cookieStoreId.

Thanks

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8966639 [details]
Bug 1391992 - Add cookieStoreId to webRequest extension API.

drive-by.  Don't try to use ext-toolkit in webrequest.jsm, what you did is just fine.  The api looks fine as well.  I'm less clear on eslint stuff, but it looks the same as other globals.

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8966639 [details]
Bug 1391992 - Add cookieStoreId to webRequest extension API.

I agree with Shane that what you did is fine.  The eslint problem is just that you used a different name in .eslintrc, (...ForLoadInfo vs ...ForOriginAttributes).  Fixing it in .eslintrc should clear that up.

Comment 8

[erosman]

re comment 2
Actually, never mind...
I found out it has cookieStoreId: "firefox-private" so that can be used to determine incognito :)

Comment 9

[Jonathan Kingston [:jkt] he/him]

> The eslint problem is just that you used a different name in .eslintrc, (...ForLoadInfo vs ...ForOriginAttributes).

I thought I pushed this :')

> drive-by.  Don't try to use ext-toolkit in webrequest.jsm, what you did is just fine.

Ok great, I'll get cooking up a test then.

Thanks both.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8966639 [details]
Bug 1391992 - Add cookieStoreId to webRequest extension API.

https://reviewboard.mozilla.org/r/235354/#review241138

::: toolkit/components/extensions/parent/ext-toolkit.js:5
(Diff revision 1)
> -/* exported getCookieStoreIdForTab, getCookieStoreIdForContainer,
> -            getContainerForCookieStoreId,
> +/* exported getCookieStoreIdForTab, getCookieStoreIdForOriginAttributes,
> +            getCookieStoreIdForContainer, getContainerForCookieStoreId,

This doesn't need to be added here. In fact, this entire `exported` comment needs to go away...

Comment 11

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8966639 [details]
Bug 1391992 - Add cookieStoreId to webRequest extension API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235354/diff/1-2/

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8966639 [details]
Bug 1391992 - Add cookieStoreId to webRequest extension API.

https://reviewboard.mozilla.org/r/235354/#review242650

::: toolkit/components/extensions/parent/ext-webRequest.js:30
(Diff revision 2)
> +      if (data.originAttributes) {
> +        event.cookieStoreId = getCookieStoreIdForOriginAttributes(data.originAttributes);
> +      }

Unless any of the comment below is applicable, drop the if statement (now the function has defaults) so we always get a cookieStoreId. (also see comment in webrequest.jsm)

Under what circumstances would we not have loadInfo.originAttributes?  Would they ever be absent there, but available from the tab?  

Should we check for attributes on the tab if loadinfo did not have them?

e.g.

if (data.originAttributes) {
  storeId = getCookieStoreIdForOriginAttributes(data.originAttributes);
} else (tabId >= 0) {
  storeId = getCookieStoreIdForTab({incognito: ???}, tabTracker.getTab(tabId));
}

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_cookie_store.html:20
(Diff revision 2)
> +
> +
> +add_task(async function test_cookie_store() {
> +  async function background() {
> +    const TEST_URL = "http://example.org/";
> +    const cookieStores = await browser.contextualIdentities.query({});

Can we also create a custom ci so we test a tab with that?

::: toolkit/modules/addons/WebRequest.jsm:702
(Diff revision 2)
>        windowId: channel.windowId,
>        parentWindowId: channel.parentWindowId,
>  
>        frameAncestors: channel.frameAncestors || undefined,
>  
> +      originAttributes: channel.loadInfo.originAttributes,

originAttributes: channel.loadInfo.originAttributes || {}

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8966639 [details]
Bug 1391992 - Add cookieStoreId to webRequest extension API.

https://reviewboard.mozilla.org/r/235354/#review242650

> Can we also create a custom ci so we test a tab with that?

Thinking about this a bit more, what happens for a request from e.g. a serviceworker in a private window?  What I'm getting at is that we should have the cookieStoreId whenever applicable.

Comment 14

[Jonathan Kingston [:jkt] he/him]

> Under what circumstances would we not have loadInfo.originAttributes?  Would they ever be absent there, but available from the tab?

There were some requests related to Firefox itself that were new that I didn't recognise that were flying past. I think they are tab warming perhaps. It was working for Service Workers, images etc which is my real issue in this bug. Maybe your code sample should work, I will double check as we might want to just fix the OA of the requests without.

> Thinking about this a bit more, what happens for a request from e.g. a serviceworker in a private window?  What I'm getting at is that we should have the cookieStoreId whenever applicable.

We should have all of this, however we should test for each yeah.

Comment 15

[Jonathan Kingston [:jkt] he/him]

I double checked the requests that were causing the issues without the if, they are data: images that look like this:

{"requestId":"fakeRequest-22","url":"data:image/gif;base64,R0lGODlhAQABAID/AMDAwAAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw%3D%3D","originUrl":"https://www.google.co.uk/search?q=sdfsdf&ie=utf-8&oe=utf-8&client=firefox-b-ab&gfe_rd=cr&dcr=0&ei=7sXUWoHQMfDR8gfLwpdQ","documentUrl":"https://www.google.co.uk/search?q=sdfsdf&ie=utf-8&oe=utf-8&client=firefox-b-ab&gfe_rd=cr&dcr=0&ei=7sXUWoHQMfDR8gfLwpdQ","type":"image","timeStamp":1523893753417,"frameId":10737418241,"parentFrameId":-1,"tabId":5}

:mixedpuppy - I'm not exactly sure why this looks different to all other requests. Also I know tjr is mopping up any case where we don't have OA's.

:tjr - do you know if this might be tackled in your changes? This code mentioned is `channel.loadInfo` in toolkit/modules/addons/WebRequest.jsm

Comment 16

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to Jonathan Kingston [:jkt] from comment #15)
> I double checked the requests that were causing the issues without the if,
> they are data: images that look like this:
> 
> {"requestId":"fakeRequest-22","url":"data:image/gif;base64,R0lGODlhAQABAID/
> AMDAwAAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw%3D%3D","originUrl":"https://www.
> google.co.uk/search?q=sdfsdf&ie=utf-8&oe=utf-8&client=firefox-b-
> ab&gfe_rd=cr&dcr=0&ei=7sXUWoHQMfDR8gfLwpdQ","documentUrl":"https://www.
> google.co.uk/search?q=sdfsdf&ie=utf-8&oe=utf-8&client=firefox-b-
> ab&gfe_rd=cr&dcr=0&ei=7sXUWoHQMfDR8gfLwpdQ","type":"image","timeStamp":
> 1523893753417,"frameId":10737418241,"parentFrameId":-1,"tabId":5}
> 
> :mixedpuppy - I'm not exactly sure why this looks different to all other
> requests. Also I know tjr is mopping up any case where we don't have OA's.
> 
> :tjr - do you know if this might be tackled in your changes? This code
> mentioned is `channel.loadInfo` in toolkit/modules/addons/WebRequest.jsm

Nothing I did (it's landed at this point) seems related to this at first glance.  Christoph might have an idea?

Also, a drive by - I'm not sure if this affects First Party Isolation, but that might be something to test for to be sure it's handled correctly.

Comment 17

[Shane Caraveo (:mixedpuppy)]

(In reply to Jonathan Kingston [:jkt] from comment #15)
> I double checked the requests that were causing the issues without the if,
> they are data: images that look like this:
> 
> {"requestId":"fakeRequest-22","url":"data:image/gif;base64,R0lGODlhAQABAID/
> AMDAwAAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw%3D%3D","originUrl":"https://www.
> google.co.uk/search?q=sdfsdf&ie=utf-8&oe=utf-8&client=firefox-b-
> ab&gfe_rd=cr&dcr=0&ei=7sXUWoHQMfDR8gfLwpdQ","documentUrl":"https://www.
> google.co.uk/search?q=sdfsdf&ie=utf-8&oe=utf-8&client=firefox-b-
> ab&gfe_rd=cr&dcr=0&ei=7sXUWoHQMfDR8gfLwpdQ","type":"image","timeStamp":
> 1523893753417,"frameId":10737418241,"parentFrameId":-1,"tabId":5}
> 
> :mixedpuppy - I'm not exactly sure why this looks different to all other
> requests. Also I know tjr is mopping up any case where we don't have OA's.

any non-https? request goes through ContentPolicyManager[1], which listens for messages from WebRequestContent.js.  You'll need to add originAttributes to the data object in receiveMessage.  loadinfo is avialable in WebRequestContent (see shouldLoad) so you can get the origin attributes there, and pass them in the data object[2].

[1] https://searchfox.org/mozilla-central/source/toolkit/modules/addons/WebRequest.jsm#216
[2] https://searchfox.org/mozilla-central/source/toolkit/modules/addons/WebRequestContent.js#183

Comment 18

[Jonathan Kingston [:jkt] he/him]

Ah I understand the issue now, I didn't realise there were multiple code paths in. Thanks.

Comment 19

[Shane Caraveo (:mixedpuppy)]

Just curious if you were going to continue this patch.

Comment 20

[Jonathan Kingston [:jkt] he/him]

If people want to take it off me that would be totally fine. I don't have the cycles at the moment to look at this. :)

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1391992 add cookiestoreid to proxy and webrequest details

Comment 22

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89c68db5d0dd73e34f31f8d1db25436f855bddb6

Comment 23

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca2e07d3caad37fbfe85a5acd807da7b4b22b128

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e64b96d4af3d
add cookiestoreid to proxy and webrequest details r=robwu

Comment 25

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e64b96d4af3d

Comment 26

[CosminB]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!

Comment 27

[Irene Smith]

Added a definition of the new property to proxy.RequestDetails:

cookieStoreId
string. The cookie store ID of the current context.

It is also included in the Release notes.

Comment 28

[ntninja]

(In reply to Irene Smith from comment #27)

Added a definition of the new property to proxy.RequestDetails:

cookieStoreId
string. The cookie store ID of the current context.

It is also included in the Release notes.

Shouldn't it also be documented for all the event details objects on webRequest that this was exposed on? Also, the change should likely be noted in the compat tables of each updated details object.

Thanks for implementing this btw!