Re-enable ServiceWorker initiated request blocking

A bug in the chrome.declarativeNetRequest API[1] meant that extensions could
block requests initiated by other extensions. This cross-extension request
blocking wasn't supposed to be possible[2], and led to lots of issues for users
with multiple browser extensions installed (for example, see this Privacy Badger
bug report[3]).

These cross-extension requests weren't associated with a tabId, and so could not
be differentiated from other such requests - including ServiceWorker initiated
requests. As a workaround, to avoid breaking the other extensions our users
might have installed, we stopped blocking any requests with no associated tabId.

Since then, we have fixed the bug in Chrome[4], and that fix was release with
Chrome 128. So let's re-enable ServiceWorker initiated request blocking again
and increase the minimum supported Chrome version to 128.

1 - https://crbug.com/40896400
2 - w3c/webextensions#369
3 - EFForg/privacybadger#2968
4 - https://chromiumdash.appspot.com/commit/486d638e6977dad73bd207dc914df9319afac152

browsers/chrome/manifest.json

@@ -3,7 +3,7 @@
     "description": "__MSG_appDesc__",
     "default_locale": "en",
     "version": "2024.10.16",
-    "minimum_chrome_version": "121.0",
+    "minimum_chrome_version": "128.0",
     "icons": {
         "16": "img/icon_16.png",
         "48": "img/icon_48.png",

integration-test/request-blocking.spec.js

@@ -105,47 +105,6 @@ test.describe('Test request blocking', () => {
         await page.close()
     })
 
-    test('serviceworkerInitiatedRequests exceptions should disable service worker blocking', async ({ page, backgroundPage, context, backgroundNetworkContext, manifestVersion }) => {
-        await overridePrivacyConfig(backgroundNetworkContext, 'serviceworker-blocking.json')
-        await forExtensionLoaded(context)
-        await forAllConfiguration(backgroundPage)
-        if (manifestVersion === 3) {
-            await forDynamicDNRRulesLoaded(backgroundPage)
-        }
-        await backgroundPage.evaluate(async (domain) => {
-            /** @type {import('../shared/js/background/components/resource-loader').default} */
-            const configLoader = globalThis.components.tds.config
-            await configLoader.modify((config) => {
-                config.features.serviceworkerInitiatedRequests.exceptions.push({
-                    domain,
-                    reason: 'test'
-                })
-                return config
-            })
-        }, testHost)
-        const [, pageRequests] = await runRequestBlockingTest(page)
-
-        // Verify that no logged requests were allowed.
-        for (const { url, method, type, status } of pageRequests) {
-            const description = `URL: ${url}, Method: ${method}, Type: ${type}`
-            expect(status, description).toEqual('blocked')
-        }
-
-        // Check that the test page itself agrees that no requests were
-        // allowed.
-        const pageResults = await page.evaluate(
-            () => results.results // eslint-disable-line no-undef
-        )
-        for (const { id, category, status } of pageResults) {
-            const description = `ID: ${id}, Category: ${category}`
-            if (id === 'serviceworker-fetch') {
-                expect(status, description).toEqual('loaded')
-            } else {
-                expect(status, description).not.toEqual('loaded')
-            }
-        }
-    })
-
     test('Blocking should not run on localhost', async ({ page, backgroundPage, context, manifestVersion, backgroundNetworkContext }) => {
         await overridePrivacyConfig(backgroundNetworkContext, 'serviceworker-blocking.json')
         await forExtensionLoaded(context)
@@ -213,11 +172,6 @@ test.describe('Test request blocking', () => {
             if (['video', 'websocket'].includes(id)) {
                 continue
             }
-            // serviceworker-fetch: allowlist does not work in MV3
-            // https://app.asana.com/0/892838074342800/1204515863331825/f
-            if (manifestVersion === 3 && id === 'serviceworker-fetch') {
-                continue
-            }
             const description = `ID: ${id}, Category: ${category}`
             expect(status, description).toEqual('loaded')
         }

packages/ddg2dnr/lib/rulePriorities.js

@@ -5,7 +5,6 @@
 //       there is not a better place to put them.
 
 exports.AD_ATTRIBUTION_POLICY_PRIORITY = 30000
-exports.SERVICE_WORKER_INITIATED_ALLOWING_PRIORITY = 1000000
 exports.USER_ALLOWLISTED_PRIORITY = 1000000
 exports.ATB_PARAM_PRIORITY = 2000000
 exports.NEWTAB_TRACKER_STATS_REDIRECT_PRIORITY = 2000000

packages/ddg2dnr/test/rulePriorities.js

@@ -37,7 +37,6 @@ const {
 
 const {
     AD_ATTRIBUTION_POLICY_PRIORITY,
-    SERVICE_WORKER_INITIATED_ALLOWING_PRIORITY,
     USER_ALLOWLISTED_PRIORITY,
     ATB_PARAM_PRIORITY,
     NEWTAB_TRACKER_STATS_REDIRECT_PRIORITY
@@ -57,7 +56,6 @@ describe('Rule Priorities', () => {
         assert.equal(TRACKING_PARAM_PRIORITY, 40000)
         assert.equal(COOKIE_PRIORITY, 40000)
         assert.equal(UNPROTECTED_TEMPORARY_ALLOWLIST_PRIORITY, 1000000)
-        assert.equal(SERVICE_WORKER_INITIATED_ALLOWING_PRIORITY, 1000000)
         assert.equal(USER_ALLOWLISTED_PRIORITY, 1000000)
         assert.equal(ATB_PARAM_PRIORITY, 2000000)
         assert.equal(NEWTAB_TRACKER_STATS_REDIRECT_PRIORITY, 2000000)
@@ -67,12 +65,10 @@ describe('Rule Priorities', () => {
         // ATB is higher than allowlist
         assert.ok(ATB_PARAM_PRIORITY > UNPROTECTED_TEMPORARY_ALLOWLIST_PRIORITY)
         assert.ok(ATB_PARAM_PRIORITY > USER_ALLOWLISTED_PRIORITY)
-        assert.ok(ATB_PARAM_PRIORITY > SERVICE_WORKER_INITIATED_ALLOWING_PRIORITY)
 
         // NEWTAB_TRACKER_STATS is higher than allowlist
         assert.ok(NEWTAB_TRACKER_STATS_REDIRECT_PRIORITY > UNPROTECTED_TEMPORARY_ALLOWLIST_PRIORITY)
         assert.ok(NEWTAB_TRACKER_STATS_REDIRECT_PRIORITY > USER_ALLOWLISTED_PRIORITY)
-        assert.ok(NEWTAB_TRACKER_STATS_REDIRECT_PRIORITY > SERVICE_WORKER_INITIATED_ALLOWING_PRIORITY)
 
         // Ceiling priorities should always be higher than baseline.
         assert.ok(TRACKER_BLOCKING_BASELINE_PRIORITY < TRACKER_BLOCKING_CEILING_PRIORITY)
@@ -86,7 +82,6 @@ describe('Rule Priorities', () => {
         assert.ok(SMARTER_ENCRYPTION_PRIORITY < GPC_HEADER_PRIORITY)
         assert.ok(SMARTER_ENCRYPTION_PRIORITY < TRACKING_PARAM_PRIORITY)
         assert.ok(SMARTER_ENCRYPTION_PRIORITY < UNPROTECTED_TEMPORARY_ALLOWLIST_PRIORITY)
-        assert.ok(SMARTER_ENCRYPTION_PRIORITY < SERVICE_WORKER_INITIATED_ALLOWING_PRIORITY)
         assert.ok(SMARTER_ENCRYPTION_PRIORITY < USER_ALLOWLISTED_PRIORITY)
         assert.ok(SMARTER_ENCRYPTION_PRIORITY < NEWTAB_TRACKER_STATS_REDIRECT_PRIORITY)
 
@@ -99,7 +94,6 @@ describe('Rule Priorities', () => {
         assert.ok(TRACKER_ALLOWLIST_CEILING_PRIORITY < GPC_HEADER_PRIORITY)
         assert.ok(TRACKER_ALLOWLIST_CEILING_PRIORITY < TRACKING_PARAM_PRIORITY)
         assert.ok(TRACKER_ALLOWLIST_CEILING_PRIORITY < UNPROTECTED_TEMPORARY_ALLOWLIST_PRIORITY)
-        assert.ok(TRACKER_ALLOWLIST_CEILING_PRIORITY < SERVICE_WORKER_INITIATED_ALLOWING_PRIORITY)
         assert.ok(TRACKER_ALLOWLIST_CEILING_PRIORITY < USER_ALLOWLISTED_PRIORITY)
 
         // Ad attribution allowlisting and the contentBlocking allowlist should
@@ -112,7 +106,6 @@ describe('Rule Priorities', () => {
         assert.ok(AD_ATTRIBUTION_POLICY_PRIORITY < GPC_HEADER_PRIORITY)
         assert.ok(AD_ATTRIBUTION_POLICY_PRIORITY < TRACKING_PARAM_PRIORITY)
         assert.ok(AD_ATTRIBUTION_POLICY_PRIORITY < UNPROTECTED_TEMPORARY_ALLOWLIST_PRIORITY)
-        assert.ok(AD_ATTRIBUTION_POLICY_PRIORITY < SERVICE_WORKER_INITIATED_ALLOWING_PRIORITY)
         assert.ok(AD_ATTRIBUTION_POLICY_PRIORITY < USER_ALLOWLISTED_PRIORITY)
 
         // Tracking parameter protection, AMP protection  and GPC should take
@@ -125,13 +118,10 @@ describe('Rule Priorities', () => {
         assert.ok(TRACKING_PARAM_PRIORITY > TRACKER_ALLOWLIST_CEILING_PRIORITY)
         assert.ok(TRACKING_PARAM_PRIORITY > CONTENT_BLOCKING_ALLOWLIST_PRIORITY)
         assert.ok(TRACKING_PARAM_PRIORITY < UNPROTECTED_TEMPORARY_ALLOWLIST_PRIORITY)
-        assert.ok(TRACKING_PARAM_PRIORITY < SERVICE_WORKER_INITIATED_ALLOWING_PRIORITY)
         assert.ok(TRACKING_PARAM_PRIORITY < USER_ALLOWLISTED_PRIORITY)
 
-        // The unprotectedTemporary allowlist, exception for ServiceWorker
-        // initiated requests and the user allowlist should take priority over
-        // everything else.
-        assert.equal(UNPROTECTED_TEMPORARY_ALLOWLIST_PRIORITY, SERVICE_WORKER_INITIATED_ALLOWING_PRIORITY)
+        // The unprotectedTemporary allowlist and the user allowlist should take
+        // priority over everything else.
         assert.equal(UNPROTECTED_TEMPORARY_ALLOWLIST_PRIORITY, USER_ALLOWLISTED_PRIORITY)
         assert.ok(USER_ALLOWLISTED_PRIORITY > TRACKER_BLOCKING_CEILING_PRIORITY)
         assert.ok(USER_ALLOWLISTED_PRIORITY > TRACKER_ALLOWLIST_CEILING_PRIORITY)

shared/js/background/dnr-config-rulesets.js

@@ -4,9 +4,6 @@ import tdsStorage from './storage/tds'
 import trackers from './trackers'
 import { isFeatureEnabled } from './utils'
 import { ensureGPCHeaderRule } from './dnr-gpc'
-import {
-    ensureServiceWorkerInitiatedRequestExceptions
-} from './dnr-service-worker-initiated'
 import { getDenylistedDomains } from './dnr-user-allowlist'
 import { findExistingDynamicRule, SETTING_PREFIX, ruleIdRangeByConfigName } from './dnr-utils'
 import {
@@ -292,7 +289,6 @@ export async function onConfigUpdate (configName, etag, configValue) {
         } else if (configName === 'config') {
             await updateExtensionConfigRules(etag, configValue)
             await ensureGPCHeaderRule(configValue)
-            await ensureServiceWorkerInitiatedRequestExceptions(configValue)
         }
         // combined rules (cookie blocking)
         await updateCombinedConfigBlocklistRules()

unit-test/background/declarative-net-request.js

@@ -13,9 +13,6 @@ import tabManager from '../../shared/js/background/tab-manager'
 import tdsStorage from '../../shared/js/background/storage/tds'
 import trackers from '../../shared/js/background/trackers'
 import { ensureGPCHeaderRule } from '../../shared/js/background/dnr-gpc'
-import {
-    ensureServiceWorkerInitiatedRequestExceptions
-} from '../../shared/js/background/dnr-service-worker-initiated'
 import {
     onConfigUpdate
 } from '../../shared/js/background/dnr-config-rulesets'
@@ -31,7 +28,6 @@ import {
     SETTING_PREFIX
 } from '../../shared/js/background/dnr-utils'
 import {
-    SERVICE_WORKER_INITIATED_ALLOWING_PRIORITY,
     USER_ALLOWLISTED_PRIORITY
 } from '@duckduckgo/ddg2dnr/lib/rulePriorities'
 import { GPC_HEADER_PRIORITY } from '@duckduckgo/ddg2dnr/lib/gpc'
@@ -691,69 +687,6 @@ describe('declarativeNetRequest', () => {
         })
     })
 
-    it('ServiceWorker initiated request allowing', async () => {
-        const ruleId = SERVICE_WORKER_INITIATED_ALLOWING_RULE_ID
-        const rulePriority = SERVICE_WORKER_INITIATED_ALLOWING_PRIORITY
-        const rule = {
-            id: ruleId,
-            priority: rulePriority,
-            action: { type: 'allow' },
-            condition: { tabIds: [-1], initiatorDomains: ['example.com', 'google.com', 'suntrust.com'] }
-        }
-        const tempUnprotected = config.unprotectedTemporary
-
-        // The rule won't exist initially.
-        expect(updateSessionRulesObserver.calls.count()).toEqual(0)
-        expect(sessionRulesByRuleId.has(ruleId)).toBeFalse()
-
-        // if there are no serviceworker exceptions, or unprotected sites, no rule should be created
-        config.features.serviceworkerInitiatedRequests = {
-            state: 'enabled',
-            exceptions: []
-        }
-        config.unprotectedTemporary = []
-        await ensureServiceWorkerInitiatedRequestExceptions(config)
-        expect(updateSessionRulesObserver.calls.count()).toEqual(1)
-        expect(sessionRulesByRuleId.has(ruleId)).toBeFalse()
-
-        // If completely disabled, a rule should be created with no domain
-        // exceptions.
-        config.features.serviceworkerInitiatedRequests = {
-            state: 'disabled',
-            exceptions: [{ domain: 'example.com', reason: '' }]
-        }
-        config.unprotectedTemporary = tempUnprotected
-        await ensureServiceWorkerInitiatedRequestExceptions(config)
-        expect(updateSessionRulesObserver.calls.count()).toEqual(2)
-        expect(sessionRulesByRuleId.has(ruleId)).toBeTrue()
-        expect(sessionRulesByRuleId.get(ruleId)?.condition?.initiatorDomains).toBeUndefined()
-
-        // But if enabled, domain exceptions should be used.
-        config.features.serviceworkerInitiatedRequests.state = 'enabled'
-        await ensureServiceWorkerInitiatedRequestExceptions(config)
-        expect(updateSessionRulesObserver.calls.count()).toEqual(3)
-        expect(sessionRulesByRuleId.has(ruleId)).toBeTrue()
-        expect(sessionRulesByRuleId.get(ruleId)).toEqual(rule)
-        expect(sessionRulesByRuleId.get(ruleId)?.condition?.initiatorDomains[0]).toEqual('example.com')
-
-        // If ensureServiceWorkerInitiatedRequestException is called again, the
-        // rule should just be recreated.
-        await ensureServiceWorkerInitiatedRequestExceptions(config)
-        expect(updateSessionRulesObserver.calls.count()).toEqual(4)
-        expect(sessionRulesByRuleId.has(ruleId)).toBeTrue()
-        expect(sessionRulesByRuleId.get(ruleId)).toEqual(rule)
-
-        // If enabled and there are no domain exceptions, the rule should be
-        // removed.
-        config.features.serviceworkerInitiatedRequests.exceptions = []
-        config.unprotectedTemporary = []
-        await ensureServiceWorkerInitiatedRequestExceptions(config)
-        expect(updateSessionRulesObserver.calls.count()).toEqual(5)
-        expect(sessionRulesByRuleId.has(ruleId)).toBeFalse()
-
-        config.unprotectedTemporary = tempUnprotected
-    })
-
     it('GPC header redirection', async () => {
         const ruleId = GPC_HEADER_RULE_ID
         const rulePriority = GPC_HEADER_PRIORITY