Web extension url filters "port" property skips other checks on match
Categories
(WebExtensions :: Untriaged, defect, P3)
Tracking
(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.
Comment 1•3 years ago
|
||
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.
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;
}
Comment 3•3 years ago
•
|
||
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.
Updated•3 years ago
|
Comment 4•3 years ago
|
||
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 !
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.
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".
Comment 8•3 years ago
|
||
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.
Updated•3 years ago
|
Assignee | ||
Comment 10•3 years ago
|
||
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. ^-^'
Comment 11•3 years ago
|
||
(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. ^-^'
You need to wait for review :)
Assignee | ||
Comment 12•3 years ago
|
||
Depends on D138935
Assignee | ||
Comment 13•3 years ago
|
||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 14•3 years ago
|
||
Comment 15•3 years ago
|
||
bugherder |
Updated•3 years ago
|
Comment 16•3 years ago
|
||
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.
Description
•