Issue 6543 - Match requests without tabId/frameId in their originating context

lib/requestBlocker.js

Index: lib/requestBlocker.js
===================================================================
--- a/lib/requestBlocker.js
+++ b/lib/requestBlocker.js
@@ -73,20 +73,54 @@
   yield "CSP";
 }());
 
-function onBeforeRequestAsync(tabId, url, type, docDomain,
-                              thirdParty, sitekey,
-                              specificOnly, filter)
+function getRelatedTabIds(details)
 {
-  let tabIds = tabId != -1 ? [tabId] : [];
+  // This is the common case, the request is associated with a single tab.
+  // If tabId is -1, its not (e.g. the request was sent by
+  // a Service/Shared Worker) and we have to identify the related tabs.
+  if (details.tabId != -1)
+    return Promise.resolve([details.tabId]);
 
-  if (filter)
-    FilterNotifier.emit("filter.hitCount", filter, 0, 0, tabIds);
+  // Firefox >=48 provides "originUrl" indicating the URL of the tab
+  // that caused this request. In case of Service/Shared Worker,
+  // this is URL of the tab that caused the worker to be spawned.
+  if (details.originUrl)

[kzar, 2018/04/03 12:49:30]

Nit: Please use braces since the body spans multiple lines. (same below)

[Sebastian Noack, 2018/04/04 01:04:45]

No longer relevant (the block has only one line now, same below).

+    return browser.tabs.query({url: details.originUrl}).then(
+      tabs => tabs.map(tab => tab.id)
+    );
 
-  devtools.logRequest(
-    tabIds, url, type, docDomain,
-    thirdParty, sitekey,
-    specificOnly, filter
-  );
+  // Chromium >=63 provides "intiator" which is equivalent to "originUrl" on
+  // Firefox except that its not a full URL but just an origin (proto + host).
+  // So we have to find each tab with an URL of the same origin.
+  if (details.initiator)
+    return browser.tabs.query({}).then(tabs =>

[kzar, 2018/04/03 12:49:30]

Couldn't we pass something like `details.initiator + "*" ` for the url part of
the query? I figure we could avoid filtering the URLs manually later that way.

https://developer.chrome.com/extensions/tabs#method-query
https://developer.chrome.com/extensions/match_patterns

[Sebastian Noack, 2018/04/04 01:04:45]

In fact we can. I wasn't aware that tab.query() accepts URL patterns. Done.

[kzar, 2018/04/05 10:31:51]

Cool, I assumed I was wrong somehow :p. This looks much cleaner now.

+    {
+      let tabIds = [];
+      for (let tab of tabs)
+      {
+        if (new URL(tab.url).origin == details.initiator)
+          tabIds.push(tab.id);

[kzar, 2018/04/03 12:49:30]

Wouldn't this mean we sometimes log a request for a tab incorrectly, say if the
user has two tabs open for the same domain?

[Sebastian Noack, 2018/04/04 01:04:45]

This usually means that the request was sent by a Service Worker controlling
multiple tabs, in which case logging the request for all those tabs is correct.
Theoretically we can get false positives, e.g. if the request was sent by a
Service Worker controlling https://example.com/foo while there is another tab
for https://example.com/bar which isn't controlled by that Service Worker.
However, by default ServiceWorkers have a domain-wide scope and limiting the
scope requires to jump through some hoops. Also given the limited information,
Chrome provides us with, this seems to be the best we can do. Also FWIW,
currently, those requests are logged to all tabs. So we don't degrade here.

[kzar, 2018/04/05 10:31:51]

Acknowledged.

+      }
+      return tabIds;
+    });
+
+  return Promise.resolve([]);
+}
+
+function logRequest(details, url, type, docDomain, thirdParty,
+                    sitekey, specificOnly, filter)
+{
+  getRelatedTabIds(details).then(tabIds =>
+  {
+    if (filter)
+      FilterNotifier.emit("filter.hitCount", filter, 0, 0, tabIds);
+
+    devtools.logRequest(
+      tabIds, url, type, docDomain,
+      thirdParty, sitekey,
+      specificOnly, filter
+    );
+  });
 }
 
 browser.webRequest.onBeforeRequest.addListener(details =>
@@ -105,55 +139,57 @@
       url.protocol != "ws:" && url.protocol != "wss:")
     return;
 
-  // Firefox (only) allows to intercept requests sent by the browser
-  // and other extensions. We don't want to block these.
+  let originUrl = null;
   if (details.originUrl)
   {
-    let originUrl = new URL(details.originUrl);
+    originUrl = new URL(details.originUrl);
+
+    // Firefox (only) allows to intercept requests sent by the browser
+    // and other extensions. We don't want to block these.
     if (originUrl.protocol == "chrome:" ||
         originUrl.protocol == "moz-extension:")
       return;
   }
-
-  let frame = ext.getFrame(
-    details.tabId,
-    // We are looking for the frame that contains the element which
-    // has triggered this request. For most requests (e.g. images) we
-    // can just use the request's frame ID, but for subdocument requests
-    // (e.g. iframes) we must instead use the request's parent frame ID.
-    details.type == "sub_frame" ? details.parentFrameId : details.frameId
-  );
+  // Fallback to "initiator" on Chrome >=63. It doesn't include the
+  // path (unlike "originUrl" on Firefox), but is still good enough
+  // (in case the tab/frame is unknown) for the $domain filter option
+  // and most document exception rules which only match the domain part.
+  else if (details.initiator)
+    originUrl = new URL(details.initiator);
 
-  let docDomain = null;
-  let sitekey = null;
-  let thirdParty = false;
-  let specificOnly = false;
-
-  if (frame)
+  let page = null;
+  let frame = null;
+  if (details.tabId != -1)
   {
-    let page = new ext.Page({id: details.tabId});
+    page = new ext.Page({id: details.tabId});
+    frame = ext.getFrame(
+      details.tabId,
+      // We are looking for the frame that contains the element which
+      // has triggered this request. For most requests (e.g. images) we
+      // can just use the request's frame ID, but for subdocument requests
+      // (e.g. iframes) we must instead use the request's parent frame ID.
+      details.type == "sub_frame" ? details.parentFrameId : details.frameId
+    );
+  }
 
-    if (checkWhitelisted(page, frame))
-      return;
-
-    docDomain = extractHostFromFrame(frame);
-    sitekey = getKey(page, frame);
-    thirdParty = isThirdParty(url, docDomain);
-    specificOnly = !!checkWhitelisted(page, frame,
-                                      RegExpFilter.typeMap.GENERICBLOCK);
-  }
+  if (checkWhitelisted(page, frame, originUrl))
+    return;
 
   let urlString = stringifyURL(url);
   let type = resourceTypes.get(details.type) || "OTHER";
+  let docDomain = extractHostFromFrame(frame, originUrl);
+  let thirdParty = isThirdParty(url, docDomain);
+  let sitekey = getKey(page, frame, originUrl);
+  let specificOnly = !!checkWhitelisted(page, frame, originUrl,
+                                        RegExpFilter.typeMap.GENERICBLOCK);
+
   let filter = defaultMatcher.matchesAny(
     urlString, RegExpFilter.typeMap[type],
     docDomain, thirdParty, sitekey, specificOnly
   );
 
-  setTimeout(onBeforeRequestAsync, 0, details.tabId, urlString,
-                                      type, docDomain,
-                                      thirdParty, sitekey,
-                                      specificOnly, filter);
+  logRequest(details, urlString, type, docDomain,
+             thirdParty, sitekey, specificOnly, filter);
 
   if (filter instanceof BlockingFilter)
     return {cancel: true};
@@ -170,7 +206,7 @@
   let blocked = false;
 
   let specificOnly = checkWhitelisted(
-    sender.page, sender.frame,
+    sender.page, sender.frame, null,
     RegExpFilter.typeMap.GENERICBLOCK
   );