Closed Bug 1755263 Opened 3 years ago Closed 3 years ago

Web extension url filters "port" property skips other checks on match

Categories

(WebExtensions :: Untriaged, defect, P3)

Firefox 98
defect

Tracking

(firefox97 wontfix, firefox98 wontfix, firefox99 wontfix, firefox100 verified)

VERIFIED FIXED
100 Branch
Tracking Status
firefox97 --- wontfix
firefox98 --- wontfix
firefox99 --- wontfix
firefox100 --- verified

People

(Reporter: seth, Assigned: seth, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:98.0) Gecko/20100101 Firefox/98.0

Steps to reproduce:

Create a web extension that passes the filter argument when registering a new listener, but it must include the ports property.

For example:
https://gist.github.com/SethFalco/f48065a4ebeffb6542395e93a1f9ddf9

You can see the bug here:
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/MatchURLFilters.jsm#97

That should probably be something more like:


      // Return false if none of the ports (or port ranges) is verified
      const portMatch = filter.ports.some(filterPort => {
        if (Array.isArray(filterPort)) {
          let [lower, upper] = filterPort;
          return port >= lower && port <= upper;
        }

        return port === filterPort;
      });
    }

    if (portMatch) {
      return false;
    }

Actual results:

Firefox will forward all navigation events that are on port 80 or 443 regardless of if it matches the other criteria.

Expected results:

Firefox should continue to run through the checks even if the port matches, for example: "https://runescape.com/" shouldn't match the above filter.

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

Product: Firefox → WebExtensions

My bad, I meant:

if (!portMatch) {
  return false;
}

or

// Return false if none of the ports (or port ranges) is verified
if (!ports.some(filterPort => {
  if (Array.isArray(filterPort)) {
    let [lower, upper] = filterPort;
    return port >= lower && port <= upper;
  }

  return port === filterPort;
})) {
  return false;
}
Flags: needinfo?(lgreco)

I looked to the code in MatchURLFilters.jsm and I confirm that the correct behaviors seems to be the one that Seth describes in comment 2 code snippets (where if the url port is not included in the ports range than the url shouldn't match, but matching just the port should not make the url to match if the other criteria are not matched as well).

The test case in https://searchfox.org/mozilla-central/rev/e66593593f3b356901011ea0fcdf9979728e9ae8/toolkit/components/extensions/test/mochitest/test_ext_webnavigation_filters.html#87-118 does not cover this expected behavior properly and should also be updated to fully cover the correct expected behavior when ports are included in the url filter.

Flags: needinfo?(lgreco)
Mentor: lgreco
Severity: -- → S3
Keywords: good-first-bug
Priority: -- → P3

Hello Seth,

I’m from QA and I’m attempting to reproduce the issue in order to confirm it.

Would you be so kind as to attach a minimal test extension to allow me to test this out? If possible also provide some more detailed steps to reproduce so I know what to follow in terms of the results I should obtain.

Thank you !

Flags: needinfo?(seth)

Hey! Yeah sure, I'll attach it immediately.
I'm already working on the patch too for what it's worth, it's my first contribution to Mozilla just had to run through all the Mercurial/Mozilla fun stuff first.

Already have to patch ready, after work today I'll see if I can actually submit it.

Flags: needinfo?(seth)
Attached file ports-bug.zip

Ahh sorry, I forgot to add steps.

With what I gave you, once the extension is loaded, just visit any website except the Never Gonna Give You Up music video on YouTube.
So long as it's on port 80 or 443, it'll redirect you, despite the URL filters indicating it should only match "runescape.com".

Hey Seth and thank you for the quick response !

I reproduced the issue on the latest Nightly ( 99.0a1/20220215210051), Beta (98.0b5/20220215194438) and Release (97.0/20220202182137) under Windows 10 x64 and Ubuntu 16.04 LTS.

With the extension loaded into the browser, I accessed https://www.wow-freakz.com/ which uses port 443. I was immediately redirected to the Rick Astley video on YT, confirming the issue.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → seth
Status: NEW → ASSIGNED

Not sure if I've done this right.
Created a patch and submitted it for review here, but not sure what I'm supposed to do next. ^-^'

https://phabricator.services.mozilla.com/D138935

(In reply to Seth from comment #10)

Created a patch and submitted it for review here, but not sure what I'm supposed to do next. ^-^'

https://phabricator.services.mozilla.com/D138935

You need to wait for review :)

Depends on D138935

Attachment #9264214 - Attachment is obsolete: true
Attachment #9266658 - Attachment is obsolete: true
Attachment #9266660 - Attachment description: Bug 1755263 - Only return if port check fails r?rpl → Bug 1755263 - MatchURLFilters should return only when port filter check fails r?rpl
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/52c2ff5962e6 MatchURLFilters should return only when port filter check fails r=rpl
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 100 Branch

Hello,

Verified the fix on the latest Nightly (100.0a1/20220314214902) under Windows 10 x64 and Ubuntu 16.04 LTS.

As per the STR provided in Comment 7, using the extension from Comment 6, I accessed https://www.wow-freakz.com/ and I no longer got redirected to the Rick Astley video on YT, thus confirming the fix.

For safety I also re-tested the steps on the latest Beta (99.0b3/20220313185831) and the issue is still reproducible on this version, confirming the fix in Nightly.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: