1903789 - Can't distinguish service worker requests from requests for sidebar loads

Status: NEW

(WebExtensions :: Request Handling, defect)

Description

[:Gijs (he/him)]

After some discussion with Luca, it would appear from bug 1901242 and some trial and error that it isn't possible for an extension with a requestFilter like this:

let requestFilter = {
  tabId: -1,
  types: ["main_frame"],
  urls: ["http://*/*", "https://*/*"],
};

using onBeforeSendHeaders to distinguish if a request relates to a service worker vs. to a load from the sidebar.

Here's example request info from google meet:

{
  "requestId": "224",
  "url": "https://meet.google.com/<meetingid>",
  "originUrl": "https://meet.google.com/<meetingid>",
  "documentUrl": "https://meet.google.com/<meetingid>",
  "method": "GET",
  "type": "main_frame",
  "timeStamp": 1718890100990,
  "tabId": -1,
  "frameId": 0,
  "parentFrameId": -1,
  "incognito": false,
  "thirdParty": false,
  "cookieStoreId": "firefox-default",
  "requestHeaders": [
    {
      "name": "Host",
      "value": "meet.google.com"
    },
    {
      "name": "User-Agent",
      "value": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:129.0) Gecko/20100101 Firefox/129.0"
    },
    {
      "name": "Accept",
      "value": "text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/png,image/svg+xml,*/*;q=0.8"
    },
    {
      "name": "Accept-Language",
      "value": "en-GB,en;q=0.5"
    },
    {
      "name": "Accept-Encoding",
      "value": "gzip, deflate, br, zstd"
    },
    {
      "name": "Upgrade-Insecure-Requests",
      "value": "1"
    },
    {
      "name": "Service-Worker-Navigation-Preload",
      "value": "true"
    },
    {
      "name": "Referer",
      "value": "https://meet.app.goo.gl/"
    },
    {
      "name": "Alt-Used",
      "value": "meet.google.com"
    },
    {
      "name": "Connection",
      "value": "keep-alive"
    },
    {
      "name": "Cookie",
      "value": "redacted"
    },
    {
      "name": "Sec-Fetch-Dest",
      "value": "document"
    },
    {
      "name": "Sec-Fetch-Mode",
      "value": "navigate"
    },
    {
      "name": "Sec-Fetch-Site",
      "value": "same-origin"
    }
  ],
  "proxyInfo": null,
  "ip": null,
  "urlClassification": {
    "firstParty": [
      "tracking_content",
      "any_strict_tracking"
    ],
    "thirdParty": []
  },
  "requestSize": 0,
  "responseSize": 0
}

There is a "name": "Service-Worker-Navigation-Preload", header, which I suppose does clarify that this is a service worker request. But of course, the add-on probably wants to intercept service worker requests for the sidebar - and it still can't tell if this is a service worker request for the sidebar or for the tabs area (as tabId is -1).

I'm not sure what a solution looks like here, or what other browsers do. Perhaps tabId should be >=0 ? Perhaps there should be a serviceWorker: true property? Perhaps there should be some other way to more clearly say "this request relates to a sidebar/panel load"?

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

While looking a bit more into what details we have already available from inside WebRequest.sys.mjs that would help to tell apart main_frame requests that have tabId set to -1 because in a panel instead of a tab from requests that have tabId set to -1 because they are being handled by a service worker, I noticed that we do support windowId as a webRequest request filter property but we don't include the windowId in the details of the request that we provide to the webRequest API event listeners.

In Chrome the windowId isn't included in the request details neither, which may have been the main reason for not having included in WebRequest, but Chrome doesn't support it neither in the request filter.

And so that seems to be kind of an inconsistency for our webRequest implementation given that unlike Chrome we support filtering the request events by windowId.

Adding that to the webRequest API may help with tell apart requests orignated from a sidebar or popup extension panel from requests handled by service workers (for which both tabId and windowId will be expected to be set to -1, while for a sidebar we would expect tabId to be -1 but windowId to be set to the windowId associated in the other WebExtensions API to the window where the sidebar panel is hosted) and the addition should be doable with just a few small changes (likely 90% changes on the test side to cover it), and so it may be worth considering it.

e.g. something like the following should be enough to have the property exposed to the extensions (and missing mainly the changes on the test side):

diff --git a/toolkit/components/extensions/schemas/web_request.json b/toolkit/components/extensions/schemas/web_request.json
--- a/toolkit/components/extensions/schemas/web_request.json
+++ b/toolkit/components/extensions/schemas/web_request.json
@@ -574,6 +574,10 @@
                 "type": "integer",
                 "description": "The ID of the tab in which the request takes place. Set to -1 if the request isn't related to a tab."
               },
+              "windowId": {
+                "type": "integer",
+                "description": "The ID of the window in which the request takes place. Set to -1 if the request isn't related to a tab."
+              },
               "type": {
                 "$ref": "ResourceType",
                 "description": "How the requested resource will be used."
diff --git a/toolkit/components/extensions/webrequest/WebRequest.sys.mjs b/toolkit/components/extensions/webrequest/WebRequest.sys.mjs
--- a/toolkit/components/extensions/webrequest/WebRequest.sys.mjs
+++ b/toolkit/components/extensions/webrequest/WebRequest.sys.mjs
@@ -352,6 +352,7 @@ function serializeRequestData(eventName)
     type: this.type,
     timeStamp: Date.now(),
     tabId: this.tabId,
+    windowId: this.windowId,
     frameId: this.frameId,
     parentFrameId: this.parentFrameId,
     incognito: this.incognito,
@@ -848,6 +849,7 @@ HttpObserverManager = {
       documentUrl: channel.documentURL || undefined,
 
       tabId: this.getBrowserData(channel).tabId,
+      windowId: this.getBrowserData(channel).windowId,
       frameId: channel.frameId,
       parentFrameId: channel.parentFrameId,

With the windowId exposed, then a change on the side-view extension side like the following may be enough to prevent bug 1901242 from being hit:

diff --git a/addon/background.js b/addon/background.js
index 27d63e1..2e4559c 100644
--- a/addon/background.js
+++ b/addon/background.js
@@ -188,6 +188,14 @@ async function dismissRecentTab(tab_index) {
 
 // Add a mobile header to outgoing requests
 browser.webRequest.onBeforeSendHeaders.addListener(function (info) {
+  if (info.windowId === -1) {
+    // Ignore main_frame requests with windowId set to -1,
+    // to prevent this listener from handling main_frame requests
+    // that are being handled by a service worker (for which tabId
+    // and windowId are expected to be both set to -1).
+    return;
+  }
+
   let hostname = (new URL(info.url)).hostname;
   if (desktopHostnames[hostname]) {
     return {};

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

I briefly discussed about this issue with the rest of the team as part of our weekly bugzilla triage, and we determined that our preferred path would be to include in webRequest event details a documentId property, which is meant to be a per-document unique id to include in multiple APIs to make it easier to correlate WebExtensions events and API methods results related to the same document.

I'm linking this bug as depending on the bugzilla issue tracking implement documentId across all the extension API that should expose it: Bug 1891478

We could also consider to use this bug to track adding documentId to the webRequest API events as a sub-task part of Bug 1891478 work.