1529230 - MatchPattern does not support IPv6 literals

Status: VERIFIED FIXED

(WebExtensions :: General, defect)

Description

[Rob Wu [:robwu]]

IPv6 literals must be enclosed by brackets (RFC 3986, section 3.2.2). Our current MatchPattern implementation does not support patterns that include IPv6 addresses though.

STR:

Open the global JS console, and run the following snippet:

new MatchPattern('http://[::1]/').matches(Services.io.newURI('http://[::1]/'))
// Expect: true. Actual: false.

I'm expecting true, but get false.

When I remove the brackets (thus turning the pattern into an invalid URL), then I get true:

new MatchPattern('http://::1/').matches(Services.io.newURI('http://[::1]/'))
// Expect: error. Actual: true

Either form is currently accepted as a permission in the manifest.json file, and the http://::1/ permission actually grants access to http://[::1]/. If we want to minimize the risk of introducing backwards-incompatible changes, then we should support both forms.

(this issue was originally reported here: https://bugzilla.mozilla.org/show_bug.cgi?id=1520058#c9 ).

Comment 1

[violet.bugreport]

Attachment: MatchPattern.patch

My patch to this bug is attached.

RFC 3986, section 3.2.2 says IPv6 literal is is the only place where square bracket characters are allowed in the URI syntax.

Thus we can just check the leading and trailing square bracket.

PS. Could you please tell me where to "Open the global JS console"? I couldn't find the place to execute privileged JS code, it would facilitate debugging a lot. Thanks.

Comment 2

[Rob Wu [:robwu]]

To open the global JS console, use Ctrl-Shift-J.
To be able to run privileged code at all in that console, first enable such debugging via "devtools.chrome.enabled=trueatabout:config. Or more easily, visitabout:debugging` and check the "Enable add-ons debugging" checkbox, which also enables the code input at the global JS console.

MatchPattern.cpp is very hot code. Try to minimize unnecessary post-processing of mDomain, e.g. by normalizing the value at MatchPattern::Init: https://searchfox.org/mozilla-central/rev/93905b660fc99a5d52b683690dd26471daca08c8/toolkit/components/extensions/MatchPattern.cpp#327
When you fix the bug, also include a test case for this bug at: https://searchfox.org/mozilla-central/rev/93905b660fc99a5d52b683690dd26471daca08c8/toolkit/components/extensions/test/xpcshell/test_MatchPattern.js

Could you submit your patch via Phabricator instead of attaching it to Bugzilla? The usual patch submission process is explained here (I updated that article a few hours ago): https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Submitting_a_Patch

Comment 3

[violet.bugreport]

Attachment: Bug 1529230 - Drop bracket for IPv6 URL in mDomain of MatchPattern. r=robwu

mDomain in MatchPattern is inconsistent with nsIURI for IPv6 URL, which causes
failure of IPv6 matching. We normalize mDomain by droping the brackets so that
it's consistent with nsIURI.

Comment 4

[violet.bugreport]

Hi Rob,

I've submitted the patch seven days ago, would you mind reviewing?

PS: I'm not quite sure how the review process works here. I've also submitted two patches for some crash/hang bugs a couple of days ago, but it seems no one is reviewing. Will my patches get lost somehow? or is it the normal delay so I don't need to do anything else?

Please give me some advice, thanks!

Comment 5

[Rob Wu [:robwu]]

I've just completed the review, sorry for the delay. I was on PTO for a few days and didn't get to this review yet. Usually you can expect a review within a few (work)days. If you aren't getting responses, you are free to remind the reviewer using needinfo, e-mail or IRC. If that doesn't help, you can also select a different reviewer.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/325cacd86079
Drop bracket for IPv6 URL in mDomain of MatchPattern. r=robwu,mixedpuppy

Comment 7

[Rob Wu [:robwu]]

Comment on attachment 9045233 [details] [diff] [review]
MatchPattern.patch

This patch was re-uploaded (with tests) as https://phabricator.services.mozilla.com/D20603

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/325cacd86079

Comment 9

[Caitlin Neiman [:caitmuenster]]

Thanks so much for the patch, violet.bugreport! 🎉 You contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating a profile on mozillians.org? I'd be happy to vouch for you!

Comment 10

[Madalin Cotetiu]

Verified in Win7x64, Ubuntu 14.0.4 x32, MacOS 10.14.1 on FF67.0b5 (20190325125126)
Both forms:
(1) new MatchPattern('http://[::1]/').matches(Services.io.newURI('http://[::1]/'))
(2) new MatchPattern('http://::1/').matches(Services.io.newURI('http://[::1]/'))
are now accepted, and when the script is run in the console 'true' is returned. Marking bug as verified fixed.