From ba6d8ac366d41a0f36f6e0afee541029d3c6d3a5 Mon Sep 17 00:00:00 2001 From: Rob Wu Date: Wed, 12 Apr 2023 00:13:20 +0000 Subject: [PATCH] Bug 1825824 - Allow DNR to match navigation requests without permission for initiator r=rpl Differential Revision: https://phabricator.services.mozilla.com/D174895 --- .../extensions/ExtensionDNR.sys.mjs | 18 ++- .../test/mochitest/mochitest-common.ini | 1 + .../test_ext_dnr_other_extensions.html | 113 ++++++++++++++++++ .../xpcshell/test_ext_dnr_allowAllRequests.js | 50 +++++--- .../xpcshell/test_ext_dnr_testMatchOutcome.js | 81 ++++++++++++- 5 files changed, 241 insertions(+), 22 deletions(-) create mode 100644 toolkit/components/extensions/test/mochitest/test_ext_dnr_other_extensions.html diff --git a/toolkit/components/extensions/ExtensionDNR.sys.mjs b/toolkit/components/extensions/ExtensionDNR.sys.mjs index 5be99753b562..6bb098fc78cc 100644 --- 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) { diff --git a/toolkit/components/extensions/test/mochitest/mochitest-common.ini b/toolkit/components/extensions/test/mochitest/mochitest-common.ini index 907dfc9dae63..8812ea83c975 100644 --- 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 = diff --git a/toolkit/components/extensions/test/mochitest/test_ext_dnr_other_extensions.html b/toolkit/components/extensions/test/mochitest/test_ext_dnr_other_extensions.html new file mode 100644 index 000000000000..d3074b3dec6e --- /dev/null +++ b/toolkit/components/extensions/test/mochitest/test_ext_dnr_other_extensions.html @@ -0,0 +1,113 @@ + + + + + DNR and tabs.create from other extension + + + + + + + + + diff --git a/toolkit/components/extensions/test/xpcshell/test_ext_dnr_allowAllRequests.js b/toolkit/components/extensions/test/xpcshell/test_ext_dnr_allowAllRequests.js index 878d1c6cca3f..5df9fd5e99e1 100644 --- 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(); diff --git a/toolkit/components/extensions/test/xpcshell/test_ext_dnr_testMatchOutcome.js b/toolkit/components/extensions/test/xpcshell/test_ext_dnr_testMatchOutcome.js index ced600313bc2..d9cc523d99a6 100644 --- 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(); }, });