MatchPattern does not support IPv6 literals

VERIFIED FIXED in Firefox 67

Status

defect
VERIFIED FIXED
3 months ago
2 months ago

People

(Reporter: robwu, Assigned: violet.bugreport, Mentored)

Tracking

64 Branch
mozilla67

Firefox Tracking Flags

(firefox67 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

3 months ago

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 ).

Assignee

Comment 1

3 months ago
Posted patch MatchPattern.patch (obsolete) — Splinter Review

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.

Flags: needinfo?(rob)
Reporter

Comment 2

3 months ago

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

Assignee: nobody → violet.bugreport
Mentor: rob
Status: NEW → ASSIGNED
Flags: needinfo?(rob)
Assignee

Comment 3

3 months ago

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.

Assignee

Comment 4

3 months ago

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!

Flags: needinfo?(rob)
Reporter

Comment 5

3 months ago

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.

Flags: needinfo?(rob)
Assignee

Updated

3 months ago
Keywords: checkin-needed

Comment 6

3 months ago

Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/325cacd86079
Drop bracket for IPv6 URL in mDomain of MatchPattern. r=robwu,mixedpuppy

Keywords: checkin-needed
Reporter

Comment 7

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

This patch was re-uploaded (with tests) as https://phabricator.services.mozilla.com/D20603
Attachment #9045233 - Attachment is obsolete: true

Comment 8

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

2 months ago

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.

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