1939981 - Bug in WebExtension DNR "initiatorDomains" condition

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect)

Description

[Arkadiy Tetelman]

Steps to reproduce:

Firefox supports WebExtension DNR conditions such as initiatorDomains, excludeInitiatorDomains, requestDomains, and excludeRequestDomains:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/declarativeNetRequest

This logic is implemented here: https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionDNR.sys.mjs

Recently this commit merged to try to optimize the speed of matching rule conditions: https://phabricator.services.mozilla.com/D212691

The way that it works is:

• for a given e.g. initiatorDomain, construct the list of superdomains of it. For instance for "sub.example.com" construct the list ["sub.example.com", "example.com", "com"]
• when evaluating a rule, check that any of the initiatorDomains match against any of the list of superdomains

Unfortunately there is a bug in the list of superdomains due to a shadowed variable:
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionDNR.sys.mjs#1512-1526

In particular we have:

> getAllDomainsWithin('signin.aws.amazon.com')
[ 'signin.aws.amazon.com', 'aws.amazon.com', 'com' ]

Notice that it is missing amazon.com.

This is causing rules to not match in the extension that I'm writing

Actual results:

getAllDomainsWithin('signin.aws.amazon.com') is missing `amazon.com``

Expected results:

getAllDomainsWithin('signin.aws.amazon.com') should contain amazon.com

Comment 1

[Arkadiy Tetelman]

Attachment: Bug 1939981 - Avoid skipping domains in DNR requestDomains/initiatorDomains. r=robwu

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'WebExtensions::Request Handling' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Rob Wu [:robwu]]

This also affects requestDomains, not just initiatorDomains, as it shares the same logic.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8952dd92e3c
Avoid skipping domains in DNR requestDomains/initiatorDomains. r=robwu

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/c8952dd92e3c

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:arkadiyt, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox135 to wontfix.

For more information, please visit BugBot documentation.

Comment 7

[Rob Wu [:robwu]]

I'll request uplifts.

Comment 8

[Rob Wu [:robwu]]

Attachment: Bug 1939981 - Avoid skipping domains in DNR requestDomains/initiatorDomains. r=robwu

Original Revision: https://phabricator.services.mozilla.com/D233223

Comment 9

[Rob Wu [:robwu]]

Attachment: Bug 1939981 - Avoid skipping domains in DNR requestDomains/initiatorDomains. r=robwu

Original Revision: https://phabricator.services.mozilla.com/D233223

Comment 10

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Some extension-specified declarative network rule conditions may fail to apply. E.g. network requests that should be blocked may unexpectedly be allowed.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Not needed; covered by automated unit tests
• Risk associated with taking this patch: Low
• Explanation of risk level: Fixes logical error, with unit tests
• String changes made/needed: None
• Is Android affected?: yes

Comment 11

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: Some extension-specified declarative network rule conditions may fail to apply. E.g. network requests that should be blocked may unexpectedly be allowed.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Not needed; covered by automated unit tests
• Risk associated with taking this patch: Low
• Explanation of risk level: Fixes logical error, with unit tests
• String changes made/needed: None
• Is Android affected?: yes

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/36e17b474807

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/60cb647d82e8