Bug 1825824 - Allow DNR to match navigation requests without permission for initiator r=rpl

Differential Revision: https://phabricator.services.mozilla.com/D174895
2023-04-12 00:13:20 +00:00 · 2023-04-12 00:13:20 +00:00 · ba6d8ac366
--- a/toolkit/components/extensions/ExtensionDNR.sys.mjs
+++ b/toolkit/components/extensions/ExtensionDNR.sys.mjs
@ -1414,10 +1414,20 @@ class RequestDetails {

  canExtensionModify(extension) {
    const policy = extension.policy;
-    return (
-      (!this.initiatorURI || policy.canAccessURI(this.initiatorURI)) &&
-      policy.canAccessURI(this.requestURI)
-    );
+    if (!policy.canAccessURI(this.requestURI)) {
+      return false;
+    }
+    if (
+      this.initiatorURI &&
+      this.type !== "main_frame" &&
+      this.type !== "sub_frame" &&
+      !policy.canAccessURI(this.initiatorURI)
+    ) {
+      // Host permissions for the initiator is required except for navigation
+      // requests: https://bugzilla.mozilla.org/show_bug.cgi?id=1825824#c2
+      return false;
+    }
+    return true;
  }

  #domainFromURI(uri) {
--- a/toolkit/components/extensions/test/mochitest/mochitest-common.ini
+++ b/toolkit/components/extensions/test/mochitest/mochitest-common.ini
@ -156,6 +156,7 @@ skip-if =
 skip-if = os == 'android' # Bug 1513544 Android does not support multiple windows.
 [test_ext_cookies_permissions_bad.html]
 [test_ext_cookies_permissions_good.html]
+[test_ext_dnr_other_extensions.html]
 [test_ext_dnr_tabIds.html]
 [test_ext_dnr_upgradeScheme.html]
 skip-if =
--- a/toolkit/components/extensions/test/mochitest/test_ext_dnr_other_extensions.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_dnr_other_extensions.html
@ -0,0 +1,113 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>DNR and tabs.create from other extension</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/ExtensionTestUtils.js"></script>
+  <script type="text/javascript" src="head.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css">
+</head>
+<body>
+<script>
+"use strict";
+
+
+// While most DNR tests are xpcshell tests, this one is a mochitest because the
+// tabs.create API does not work in a xpcshell test.
+
+add_setup(async () => {
+  await SpecialPowers.pushPrefEnv({
+    set: [
+      ["extensions.manifestV3.enabled", true],
+      ["extensions.dnr.enabled", true],
+    ],
+  });
+});
+
+
+add_task(async function tabs_create_can_be_redirected_by_other_dnr_extension() {
+  let dnrExtension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      permissions: ["declarativeNetRequestWithHostAccess"],
+      // redirect action requires host permissions:
+      host_permissions: ["*://example.com/*"],
+    },
+    async background() {
+      await browser.declarativeNetRequest.updateSessionRules({
+        addRules: [
+          {
+            id: 1,
+            condition: {
+              resourceTypes: ["main_frame"],
+              urlFilter: "?dnr_redir_me_pls",
+            },
+            action: {
+              type: "redirect",
+              redirect: {
+                transform: {
+                  query: "?dnr_redir_target"
+                },
+              },
+            },
+          },
+        ],
+      });
+      browser.test.sendMessage("dnr_registered");
+    },
+  });
+  await dnrExtension.startup();
+  await dnrExtension.awaitMessage("dnr_registered");
+
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      permissions: ["webNavigation"],
+    },
+    async background() {
+      async function createTabAndGetFinalUrl(url) {
+        let navigationDonePromise = new Promise(resolve => {
+          browser.webNavigation.onDOMContentLoaded.addListener(
+            function listener(details) {
+              browser.webNavigation.onDOMContentLoaded.removeListener(listener);
+              resolve(details);
+            },
+            // All input URLs and redirection targets match this URL filter:
+            { url: [{ queryPrefix: "dnr_redir_" }] }
+          );
+        });
+        const tab = await browser.tabs.create({ url });
+        browser.test.log(`Waiting for navigation done, starting from ${url}`);
+        const result = await navigationDonePromise;
+        browser.test.assertEq(
+          tab.id,
+          result.tabId,
+          `Observed load completion for navigation tab with initial URL ${url}`
+        );
+        await browser.tabs.remove(tab.id);
+        return result.url;
+      }
+
+      browser.test.assertEq(
+        "https://example.com/?dnr_redir_target",
+        await createTabAndGetFinalUrl("https://example.com/?dnr_redir_me_pls"),
+        "DNR rule from other extension should have redirected the navigation"
+      );
+
+      browser.test.assertEq(
+        "https://example.org/?dnr_redir_me_pls",
+        await createTabAndGetFinalUrl("https://example.org/?dnr_redir_me_pls"),
+        "DNR redirect ignored for URLs without host permission"
+      );
+      browser.test.sendMessage("done");
+    }
+  });
+  await extension.startup();
+  await extension.awaitMessage("done");
+
+  await dnrExtension.unload();
+  await extension.unload();
+});
+
+</script>
+</body>
+</html>
--- a/toolkit/components/extensions/test/xpcshell/test_ext_dnr_allowAllRequests.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_dnr_allowAllRequests.js
@ -526,6 +526,9 @@ add_task(async function allowAllRequests_initiatorDomains_dnrWithHostAccess() {
    {
      id: 1,
      condition: {
+        // This test shows that it does not matter whether initiatorDomains is
+        // in host_permissions; it only matters if the frame's URL is matched
+        // by host_permissions.
        initiatorDomains: ["example.net"], // Not in host_permissions.
        resourceTypes: ["sub_frame"],
      },
@ -539,26 +542,45 @@ add_task(async function allowAllRequests_initiatorDomains_dnrWithHostAccess() {
  ];

  const extension = await loadExtensionWithDNRRules(rules, {
-    host_permissions: ["*://example.com/*", "*://example.org/*"],
+    host_permissions: ["*://example.org/*"],
    permissions: ["declarativeNetRequestWithHostAccess"],
  });

+  const testCanFetch = async () => {
+    // example.org is in host_permissions above so "xmlhttprequest" rule is
+    // always expected to match this, unless "allowAllRequests" applied.
+    // If "allowAllRequests" applies, then expectedResult: "fetchAllowed".
+    // If "allowAllRequests" did not apply, then expectedError: FETCH_BLOCKED.
+    return (await fetch("http://example.org/allowed")).text();
+  };
+
  await testLoadInFrame({
-    description: "sub_frame loaded by initiator not in host_permissions",
+    description:
+      "frame URL in host_permissions despite initiator not in host_permissions",
    domains: ["example.com", "example.net", "example.org"],
-    jsForFrame: async () => {
-      try {
-        await (await fetch("http://example.com/allowed")).text();
-        return true; // Result if the allowAllRequests rule applied.
-      } catch (e) {
-        return false; // Result if the allowAllRequests rule did not apply.
-      }
-    },
+    jsForFrame: testCanFetch,
    // The "xmlhttprequest" block rule applies because the request URL
-    // (example.com) and initiator (example.org) are part of host_permissions.
-    // The "allowAllRequests" rule does not apply, because "example.net" is not
-    // part of host_permissions.
-    expectedResult: false,
+    // (example.org) and initiator (example.org) are part of host_permissions.
+    //
+    // The "allowAllRequests" rule applies and overrides the block because the
+    // "example.org" frame has "example.net" as initiator (as specified in the
+    // initiatorDomains DNR rule). Despite the lack of host_permissions for
+    // "example.net", the DNR rule is matched because navigation requests do
+    // not require host permissions.
+    expectedResult: "fetchAllowed",
+  });
+
+  await testLoadInFrame({
+    description: "frame URL and initiator not in host_permissions",
+    domains: ["example.net", "example.com", "example.org"],
+    jsForFrame: testCanFetch,
+    // The "xmlhttprequest" block rule applies because the request URL
+    // (example.org) and initiator (example.org) are part of host_permissions.
+    //
+    // The "allowAllRequests" rule does not apply because it would only apply
+    // to the "example.com" frame (that frame has "example.net" as initiator),
+    // but the DNR extension does not have host permissions for example.com.
+    expectedError: FETCH_BLOCKED,
  });

  await extension.unload();
--- a/toolkit/components/extensions/test/xpcshell/test_ext_dnr_testMatchOutcome.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_dnr_testMatchOutcome.js
@ -47,6 +47,33 @@ function makeDnrTestUtils() {
      description
    );
  }
+  async function testCanMatchAnyBlock({ matchedRequests, nonMatchedRequests }) {
+    await dnr.updateSessionRules({
+      addRules: [
+        {
+          // A rule that is supposed to match everything.
+          id: 1,
+          condition: { excludedResourceTypes: [] },
+          action: { type: "block" },
+        },
+      ],
+    });
+    for (let request of matchedRequests) {
+      await testMatchesRequest(
+        request,
+        [1],
+        `${JSON.stringify(request)} - should match wildcard DNR block rule`
+      );
+    }
+    for (let request of nonMatchedRequests) {
+      await testMatchesRequest(
+        request,
+        [],
+        `${JSON.stringify(request)} - should not match any DNR rule`
+      );
+    }
+    await dnr.updateSessionRules({ removeRuleIds: [1] });
+  }
  async function testCanUseAction(type, canUse) {
    await dnr.updateSessionRules({ addRules: [makeDummyRule(1, type)] });
    await testMatchesRequest(
@ -61,6 +88,7 @@ function makeDnrTestUtils() {
    makeDummyRequest,
    makeDummyRule,
    testMatchesRequest,
+    testCanMatchAnyBlock,
    testCanUseAction,
  });
  return dnrTestUtils;
@ -345,7 +373,7 @@ add_task(async function rule_priority_and_action_type_precedence() {
 add_task(async function declarativeNetRequest_and_host_permissions() {
  await runAsDNRExtension({
    background: async dnrTestUtils => {
-      const { testCanUseAction } = dnrTestUtils;
+      const { testCanUseAction, testCanMatchAnyBlock } = dnrTestUtils;

      // Unlocked by declarativeNetRequest permission:
      await testCanUseAction("allow", true);
@ -356,6 +384,19 @@ add_task(async function declarativeNetRequest_and_host_permissions() {
      await testCanUseAction("redirect", true);
      await testCanUseAction("modifyHeaders", true);

+      const url = "https://example.com/";
+      await testCanMatchAnyBlock({
+        matchedRequests: [
+          { url, type: "other" },
+          { url, type: "main_frame" },
+          { url, type: "sub_frame" },
+          { url, initiator: url, type: "other" },
+          { url, initiator: url, type: "main_frame" },
+          { url, initiator: url, type: "sub_frame" },
+        ],
+        nonMatchedRequests: [],
+      });
+
      browser.test.notifyPass();
    },
  });
@ -367,7 +408,7 @@ add_task(async function declarativeNetRequest_permission_only() {
      host_permissions: [],
    },
    background: async dnrTestUtils => {
-      const { testCanUseAction } = dnrTestUtils;
+      const { testCanUseAction, testCanMatchAnyBlock } = dnrTestUtils;

      // Unlocked by declarativeNetRequest permission:
      await testCanUseAction("allow", true);
@ -378,6 +419,19 @@ add_task(async function declarativeNetRequest_permission_only() {
      await testCanUseAction("redirect", false);
      await testCanUseAction("modifyHeaders", false);

+      const url = "https://example.com/";
+      await testCanMatchAnyBlock({
+        matchedRequests: [
+          { url, type: "other" },
+          { url, type: "main_frame" },
+          { url, type: "sub_frame" },
+          { url, initiator: url, type: "other" },
+          { url, initiator: url, type: "main_frame" },
+          { url, initiator: url, type: "sub_frame" },
+        ],
+        nonMatchedRequests: [],
+      });
+
      browser.test.notifyPass();
    },
  });
@ -409,7 +463,7 @@ add_task(async function declarativeNetRequestWithHostAccess_only() {
  });
 });

-add_task(async function declarativeNetRequestWithHostAccess_only() {
+add_task(async function declarativeNetRequestWithHostAccess_and_host_perm() {
  await runAsDNRExtension({
    manifest: {
      permissions: [
@ -420,7 +474,7 @@ add_task(async function declarativeNetRequestWithHostAccess_only() {
      host_permissions: ["https://example.com/"],
    },
    background: async dnrTestUtils => {
-      const { testCanUseAction } = dnrTestUtils;
+      const { testCanUseAction, testCanMatchAnyBlock } = dnrTestUtils;

      // declarativeNetRequestWithHostAccess + host permissions allows all:
      await testCanUseAction("allow", true);
@ -430,6 +484,25 @@ add_task(async function declarativeNetRequestWithHostAccess_only() {
      await testCanUseAction("redirect", true);
      await testCanUseAction("modifyHeaders", true);

+      const url = "https://example.com/";
+      const urlNoPerm = "https://example.net/?not_in:host_permissions";
+      await testCanMatchAnyBlock({
+        matchedRequests: [
+          { url, type: "other" },
+          { url, type: "main_frame" },
+          { url, type: "sub_frame" },
+          // Navigations do no require host permissions for initiator.
+          { url, initiator: urlNoPerm, type: "main_frame" },
+          { url, initiator: urlNoPerm, type: "sub_frame" },
+        ],
+        nonMatchedRequests: [
+          // url always requires declarativeNetRequest or host permissions.
+          { url: urlNoPerm, type: "other" },
+          // Non-navigations require host permissions for initiator.
+          { url, initiator: urlNoPerm, type: "other" },
+        ],
+      });
+
      browser.test.notifyPass();
    },
  });