Page MenuHomePhabricator
Create Task
Maniphest T218074

ip_in_range should accept explicit range notation
Closed, ResolvedPublic
Actions

Description

On Logstash, there are several entries for filters failing with

AbuseFilter parser error for filter xxx: Invalid IP range yyy

Examining those filters reveals that all such invalid ranges are explicit ranges, i.e. 1.2.3.4 - 1.2.3.55. AbuseFilter used to accept these ranges in the past, as IP::isInRange can handle them. However, rEABF7fade990d26c79fb6acc100653bcf639f724569a added IP range validation to avoid invalid notations to be used. The downside is that validation is performed with IP::isValidRange, which only checks for ranges in CIDR notation. Given that several filters use explicit notation, and it's a good idea to keep it valid as it's easier to use, we should somehow change the validation to accept these.
AFAICS, the IP class doesn't provide a method to validate explicit ranges, so it's probably worth adding one.

Details

SubjectRepoBranchLines +/-
Allow single IPs in ip_in_rangemediawiki/extensions/AbuseFiltermaster+15 -2
Fix a test for IPUtils 2.0.0mediawiki/extensions/AbuseFiltermaster+1 -1
Add isValidExplicitRange functionmediawiki/libs/IPUtilsmaster+51 -0
IP: Add isValidExplicitRange functionmediawiki/coremaster+52 -0
Customize query in gerrit

Related Objects
Search...

Event Timeline

Daimona created this task.Mar 12 2019, 8:58 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 12 2019, 8:58 AM
Daimona added a project: User-Daimona.Mar 12 2019, 8:58 AM
Huji subscribed.Mar 12 2019, 11:07 PM

Perhaps the IP class should have a method like rangeToCIDR()

Daimona added a comment.Mar 13 2019, 8:14 AM

Well, for the specific goals of this task, we'd need a method like isValidRange but for explicit ranges. Maybe something like isValidExplicitRange.

gerritbot added a comment.Mar 21 2019, 6:46 PM

Change 498201 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] IP: Add isValidExplicitRange function

https://gerrit.wikimedia.org/r/498201

gerritbot added a project: Patch-For-Review.Mar 21 2019, 6:46 PM
gerritbot added a comment.Mar 21 2019, 6:56 PM

Change 498204 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Allow single IPs and explicit ranges in ip_in_range

https://gerrit.wikimedia.org/r/498204

Daimona claimed this task.Mar 21 2019, 6:57 PM
Daimona moved this task from Backlog to Under review on the User-Daimona board.
gerritbot added a comment.Jul 14 2019, 5:33 PM

Change 522989 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/libs/IPUtils@master] Add isValidExplicitRange function

https://gerrit.wikimedia.org/r/522989

gerritbot added a comment.Jul 14 2019, 5:35 PM

Change 498201 abandoned by Daimona Eaytoy:
IP: Add isValidExplicitRange function

Reason:
In favour of I7a14d0b7b611eba4ca3c1710780179fa884b8b3e

https://gerrit.wikimedia.org/r/498201

Reedy added a project: archived-IPUtils.Mar 7 2020, 2:53 PM
gerritbot added a comment.Mar 8 2020, 8:07 PM

Change 522989 abandoned by Daimona Eaytoy:
Add isValidExplicitRange function

Reason:
Done in I39f283dc8986d6971964f239992c33ca839f88eb

https://gerrit.wikimedia.org/r/522989

Reedy subscribed.Mar 8 2020, 8:08 PM

T247163: IPUtils::isValidRange doesn't consider explicit ranges as valid should've unblocked this :)

Daimona added a comment.Mar 8 2020, 8:12 PM
In T218074#5952454, @Reedy wrote:

T247163: IPUtils::isValidRange doesn't consider explicit ranges as valid should've unblocked this :)

Yes, thanks :) The patch is still needed for single IPs, which I should've included in the task description.

Reedy added a comment.Mar 20 2020, 7:16 PM

Trying to use ip-util 2.0.0 in https://gerrit.wikimedia.org/r/#/c/mediawiki/vendor/+/582143/

19:08:02 1) AbuseFilterParserTest::testParser with data set "ip-in-range" ('ip_in_range( '12.34.56.78', '...0/0' )')
19:08:02 AFPUserVisibleException: invalidiprange
19:08:02 
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:1381
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:1160
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:953
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:875
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:817
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:806
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:762
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:752
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:717
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:693
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:664
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:632
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:618
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:544
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:470
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:383
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:364
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:345
19:08:02 /workspace/src/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php:321
19:08:02 /workspace/src/extensions/AbuseFilter/tests/phpunit/unit/AbuseFilterParserTest.php:53
19:08:02 /workspace/src/tests/phpunit/MediaWikiUnitTestCase.php:103
Daimona added a comment.Mar 21 2020, 11:07 AM

The test contains: ip_in_range( '1.1.1.1', '0.0.0.0/0' ), which fails due to the range not being accepted.

gerritbot added a comment.Mar 21 2020, 11:11 AM

Change 582480 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Fix a test for IPUtils 2.0.0

https://gerrit.wikimedia.org/r/582480

Reedy added a comment.Mar 21 2020, 4:50 PM
In T218074#5988859, @Daimona wrote:

The test contains: ip_in_range( '1.1.1.1', '0.0.0.0/0' ), which fails due to the range not being accepted.

AFAIK it should have never been a valid range...

gerritbot added a comment.Mar 21 2020, 5:18 PM

Change 582480 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Fix a test for IPUtils 2.0.0

https://gerrit.wikimedia.org/r/582480

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.25; 2020-03-24).Mar 21 2020, 6:00 PM
Daimona moved this task from Under review to Ideas for overhaul on the User-Daimona board.Aug 20 2020, 12:08 PM
Daimona edited projects, added AbuseFilter (Overhaul-2020); removed User-Daimona, AbuseFilter.Sep 16 2020, 2:40 PM

@Reedy: is the ip-utils upgrade blocked on anything now?

Reedy added a comment.Sep 16 2020, 2:42 PM
In T218074#6466461, @Daimona wrote:

@Reedy: is the ip-utils upgrade blocked on anything now?

T248237: IPUtils >=2.0.0 test failures on MW core

Reedy added a comment.Nov 10 2020, 12:16 PM

T247212: Make new IPUtils release should be moving soon (well, the inclusion into MW core anyway!) due to Tim helping unblock T248237: IPUtils >=2.0.0 test failures on MW core

Gotta wait for Parsoid bumps to come into vendor, and then the core... Next couple of weeks that should've worked its way through at least

Reedy triaged this task as Low priority.Nov 10 2020, 12:20 PM
Reedy added a subtask: T248237: IPUtils >=2.0.0 test failures on MW core.
Reedy edited subtasks, added: T247212: Make new IPUtils release; removed: T248237: IPUtils >=2.0.0 test failures on MW core.
Jdforrester-WMF closed subtask T247212: Make new IPUtils release as Resolved.Jan 27 2021, 12:01 AM
Daimona closed this task as Resolved.Jan 27 2021, 12:23 AM

Done, docs updated (and blocked by my own test filter while doing so...)

gerritbot added a comment.Jan 27 2021, 12:30 AM

Change 498204 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Allow single IPs in ip_in_range

https://gerrit.wikimedia.org/r/498204

Reedy removed a project: Patch-For-Review.Jan 27 2021, 12:35 AM
ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.29; 2021-02-02).Jan 27 2021, 1:00 AM
Jdforrester-WMF added a project: User-notice.Jan 28 2021, 7:39 PM
RhinosF1 subscribed.Jan 28 2021, 8:26 PM
Johan moved this task from To Triage to In current Tech/News draft on the User-notice board.Jan 29 2021, 2:57 PM
BEANS-X2 subscribed.Feb 2 2021, 11:13 AM
Johan moved this task from In current Tech/News draft to Already announced/Archive on the User-notice board.Feb 4 2021, 8:23 AM
Ladsgroup edited projects, added User-notice-archive; removed User-notice.Aug 13 2022, 1:54 PM
Krinkle edited projects, added mediawiki-libs-IPUtils; removed archived-IPUtils.Jan 3 2024, 3:05 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL