add cookieStoreId to webRequest APIs

RESOLVED FIXED in Firefox 68

Status

enhancement
P1
normal
RESOLVED FIXED
2 years ago
Last month

People

(Reporter: jkt, Assigned: mixedpuppy)

Tracking

(Blocks 3 bugs, {dev-doc-needed})

unspecified
mozilla68
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

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

Updated

2 years ago

Updated

2 years ago
Priority: -- → P5

Comment 2

Last year
Semi-related: If the request can be expanded to include 'incognito' as well, it would benefit the container implementation.
Comment hidden (mozreview-request)

Comment 4

Last year
mozreview-review
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]
Reporter

Comment 5

Last year
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
Attachment #8966639 - Flags: feedback?(kmaglione+bmo)
Attachment #8966639 - Flags: feedback?(aswan)
Assignee

Comment 6

Last year
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.
Attachment #8966639 - Flags: feedback+
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.
Attachment #8966639 - Flags: feedback?(aswan) → feedback+

Comment 8

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

Comment 9

Last year
> 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.
Reporter

Updated

Last year
Assignee: nobody → jkt

Comment 10

Last year
mozreview-review
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...
Attachment #8966639 - Flags: feedback?(kmaglione+bmo) → feedback+
Comment hidden (mozreview-request)
Assignee

Comment 12

Last year
mozreview-review
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 || {}
Attachment #8966639 - Flags: review?(mixedpuppy)
Assignee

Comment 13

Last year
mozreview-review-reply
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.
Reporter

Comment 14

Last year
> 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.
Reporter

Comment 15

Last year
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
Flags: needinfo?(tom)
Flags: needinfo?(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.
> 
> :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.
Flags: needinfo?(tom) → needinfo?(ckerschb)
(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
Flags: needinfo?(mixedpuppy)
Summary: Consider providing cookieStoreId in webRequest APIs → add cookieStoreId to webRequest APIs
Reporter

Comment 18

Last year
Ah I understand the issue now, I didn't realise there were multiple code paths in. Thanks.
Flags: needinfo?(ckerschb)

Updated

Last year
Product: Toolkit → WebExtensions
Assignee

Comment 19

11 months ago
Just curious if you were going to continue this patch.
Flags: needinfo?(jkt)
Assignee

Updated

11 months ago
Blocks: 1460738
Reporter

Comment 20

11 months ago
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. :)
Flags: needinfo?(jkt)
Assignee: jkt → nobody

Updated

2 months ago
See Also: → 1545163
Assignee

Updated

2 months ago
Blocks: 1545411
Assignee

Updated

2 months ago
Attachment #8966639 - Attachment is obsolete: true
Assignee

Updated

2 months ago
Assignee: nobody → mixedpuppy
Priority: P5 → P1

Comment 24

2 months ago
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e64b96d4af3d
add cookiestoreid to proxy and webrequest details r=robwu
Assignee

Updated

2 months ago
Keywords: dev-doc-needed

Comment 25

2 months ago
bugherder
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Comment 26

Last month

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!

Flags: needinfo?(mixedpuppy)
Assignee

Updated

Last month
Flags: needinfo?(mixedpuppy) → qe-verify-
You need to log in before you can comment on or make changes to this bug.