Page MenuHomePhabricator
Create Task
Maniphest T248237

IPUtils >=2.0.0 test failures on MW core
Closed, ResolvedPublic
Actions

Description

17:28:53 There were 2 errors:
17:28:53 
17:28:53 1) WebRequestTest::testGetIP with data set #2 ('12.0.0.1', array('abcd:0001:002:03:4:555:6666:7777', '12.0.0.1, abcd:0001:002:03:4:...6:7777'), array('ABCD:1:2:3:4:555:6666:7777'), array(), false, 'IPv6 normalisation')
17:28:53 === Logs generated by test case
17:28:53 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
17:28:53 [localisation] [debug] LocalisationCache using store LCStoreNull []
17:28:53 [localisation] [debug] LocalisationCache using store LCStoreNull []
17:28:53 ===
17:28:53 MWException: Unable to determine IP.
17:28:53 
17:28:53 /workspace/src/includes/WebRequest.php:1260
17:28:53 /workspace/src/tests/phpunit/includes/WebRequestTest.php:413
17:28:53 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:418
17:28:53 /workspace/src/tests/phpunit/phpunit.php:71
17:28:53 /workspace/src/maintenance/doMaintenance.php:99
17:28:53 /workspace/src/tests/phpunit/phpunit.php:130
17:28:53 
17:28:53 2) WebRequestTest::testGetIP with data set #13 ('12.0.0.1', array('abcd:0001:002:03:4:555:6666:7777', '12.0.0.1, abcd:0001:002:03:4:...6:7777'), array('ABCD:1:2:3::/64'), array(), false, 'IPv6 CIDR')
17:28:53 === Logs generated by test case
17:28:53 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
17:28:53 [localisation] [debug] LocalisationCache using store LCStoreNull []
17:28:53 [localisation] [debug] LocalisationCache using store LCStoreNull []
17:28:53 ===
17:28:53 MWException: Unable to determine IP.
17:28:53 
17:28:53 /workspace/src/includes/WebRequest.php:1260
17:28:53 /workspace/src/tests/phpunit/includes/WebRequestTest.php:413
17:28:53 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:418
17:28:53 /workspace/src/tests/phpunit/phpunit.php:71
17:28:53 /workspace/src/maintenance/doMaintenance.php:99
17:28:53 /workspace/src/tests/phpunit/phpunit.php:130
17:28:53 
17:28:53 --
17:28:53 
17:28:53 There were 2 failures:
17:28:53 
17:28:53 1) WebRequestTest::testGetIP with data set #1 ('::1', array('::1'), array(), array(), false, 'Simple IPv6')
17:28:53 === Logs generated by test case
17:28:53 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
17:28:53 [localisation] [debug] LocalisationCache using store LCStoreNull []
17:28:53 [localisation] [debug] LocalisationCache using store LCStoreNull []
17:28:53 [wfDebug] [debug] IP: 127.0.0.1 {"private":false}
17:28:53 ===
17:28:53 Simple IPv6
17:28:53 Failed asserting that two strings are equal.
17:28:53 --- Expected
17:28:53 +++ Actual
17:28:53 @@ @@
17:28:53 -'::1'
17:28:53 +'127.0.0.1'
17:28:53 
17:28:53 /workspace/src/tests/phpunit/includes/WebRequestTest.php:414
17:28:53 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:418
17:28:53 /workspace/src/tests/phpunit/phpunit.php:71
17:28:53 /workspace/src/maintenance/doMaintenance.php:99
17:28:53 /workspace/src/tests/phpunit/phpunit.php:130
17:28:53 
17:28:53 2) HTMLRestrictionsFieldTest::testForm with data set #2 ('1.2.3.4\n::/0', array('1.2.3.4', '::/0'))
17:28:53 === Logs generated by test case
17:28:53 [wfDebug] [debug] ContextSource::getContext (OOUIHTMLForm): called and $context is null. Using RequestContext::getMain() for sanity {"private":false}
17:28:53 [GlobalTitleFail] [info] RequestContext::getTitle called with no title set. {"exception":{}}
17:28:53 [GlobalTitleFail] [info] MessageCache::parse called with no title set. {"exception":{}}
17:28:53 ===
17:28:53 Failed asserting that false matches expected true.
17:28:53 
17:28:53 /workspace/src/tests/phpunit/includes/htmlform/HTMLRestrictionsFieldTest.php:45
17:28:53 /workspace/src/tests/phpunit/phpunit.php:71
17:28:53 /workspace/src/maintenance/doMaintenance.php:99
17:28:53 /workspace/src/tests/phpunit/phpunit.php:130

Details

SubjectRepoBranchLines +/-
Canonicalise ::1 -> 127.0.0.1mediawiki/extensions/CheckUsermaster+9 -5
Update wikimedia/ip-utils from 1.0.0 to 3.0.1mediawiki/coremaster+31 -4
Fix up some issues with 2.0.0 canonicalize()mediawiki/libs/IPUtilsmaster+8 -14
Document that WebRequest::getRawIP() can also return nullmediawiki/coremaster+1 -1
Customize query in gerrit

Related Objects
Search...

Event Timeline

Reedy created this task.Mar 21 2020, 5:44 PM
Restricted Application added subscribers: Liuxinyu970226, Aklapper. · View Herald TranscriptMar 21 2020, 5:44 PM
Reedy added a comment.Mar 21 2020, 6:36 PM
17:28:53 Simple IPv6
17:28:53 Failed asserting that two strings are equal.
17:28:53 --- Expected
17:28:53 +++ Actual
17:28:53 @@ @@
17:28:53 -'::1'
17:28:53 +'127.0.0.1'
17:28:53 
17:28:53 /workspace/src/tests/phpunit/includes/WebRequestTest.php:414
17:28:53 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:418
17:28:53 /workspace/src/tests/phpunit/phpunit.php:71
17:28:53 /workspace/src/maintenance/doMaintenance.php:99
17:28:53 /workspace/src/tests/phpunit/phpunit.php:130
17:28:53

So this one is fun. The library always (even when it was in MW core) had a '::1' -> '127.0.0.1' mapping - https://github.com/wikimedia/mediawiki-libs-IPUtils/blame/b4a377d/src/IPUtils.php#L755-L759

		// IPv6 loopback address
		$m = [];
		if ( preg_match( '/^0*' . self::RE_IPV6_GAP . '1$/', $addr, $m ) ) {
			return '127.0.0.1';
		}

Until I made a change in https://github.com/wikimedia/mediawiki-libs-IPUtils/commit/620acad5de58a54b6bf825a423b1508efb586756 which stopped allowing valid IPv6 shortcutting and going out of canonicalize without any change

This code was obviously in MW core for a while before, and while this ::1 -> 127.0.0.1 mapping was there, it wasn't met, as ::1 is a valid IP

(1.0.0)

$ php maintenance/eval.php 
> var_dump( \Wikimedia\IPUtils::isValid( '::1' ) );
/var/www/wiki/mediawiki/core/maintenance/eval.php(78) : eval()'d code:1:
bool(true)

> var_dump( \Wikimedia\IPUtils::canonicalize( '::1' ) );
/var/www/wiki/mediawiki/core/maintenance/eval.php(78) : eval()'d code:1:
string(3) "::1"

2.0.0

$ php maintenance/eval.php 
> var_dump( \Wikimedia\IPUtils::isValid( '::1' ) );
/var/www/wiki/mediawiki/core/maintenance/eval.php(78) : eval()'d code:1:
bool(true)

> var_dump( \Wikimedia\IPUtils::canonicalize( '::1' ) );
/var/www/wiki/mediawiki/core/maintenance/eval.php(78) : eval()'d code:1:
string(9) "127.0.0.1"

So it's a question of which is right... :)

Reedy added a comment.Mar 21 2020, 6:45 PM

If we revert https://github.com/wikimedia/mediawiki-libs-IPUtils/blame/b4a377d/src/IPUtils.php#L743 from isValidIPv4 back to isValid that fixes 3 of the issues (but will break some of the tests in IPUtils).

HTMLRestrictionsFieldTest::testForm is still broken though... Lets see why

Reedy added a comment.Mar 21 2020, 7:26 PM

So for HTMLRestrictionsFieldTest::testForm

1.0.0

$ mwscript eval.php enwiki
> var_dump( \Wikimedia\IPUtils::isIPAddress( '::/0' ) );
bool(true)

2.0.0

$ php /var/www/wiki/mediawiki/core/maintenance/eval.php 
> var_dump( \Wikimedia\IPUtils::isIPAddress( '::/0' ) );
/var/www/wiki/mediawiki/core/maintenance/eval.php(78) : eval()'d code:1:
bool(false)

But ::/0 isn't a valid IPv6 address... So of course it gets kicked out. I guess that means for this failure, we should change that IP to something else

Reedy added a comment.EditedMar 21 2020, 7:44 PM

And to continue the confusion of the ::1 -> 127.0.0.1...

The regex seems to be looking for stuff like 0000000000:0:0::1... Which the library considers invalid (ie it doesn't pass isValid), but then would return 127.0.0.1

Why should 0000000000:0:0::1 become 127.0.0.1, but not ::1?

Of course it says

	 * @return string|null Valid dotted quad IPv4 address or null

It just seems odd that we change odd looking IPv6 to an IPv4 if it's not a normal representation, even if functionally equivalent....

Oh well

gerritbot added a comment.Mar 21 2020, 7:47 PM

Change 582528 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/libs/IPUtils@master] Fix up some issues with 2.0.0 canonicalize()

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

gerritbot added a project: Patch-For-Review.Mar 21 2020, 7:47 PM
gerritbot added a comment.Mar 21 2020, 8:55 PM

Change 582537 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] Document that WebRequest::getRawIP() can also return null

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

gerritbot added a comment.Mar 21 2020, 9:24 PM

Change 582537 merged by jenkins-bot:
[mediawiki/core@master] Document that WebRequest::getRawIP() can also return null

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

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.25; 2020-03-24).Mar 21 2020, 10:00 PM
Reedy mentioned this in T247160: Add range support to resetAuthenticationThrottle.php.May 6 2020, 8:57 PM
Reedy mentioned this in T247212: Make new IPUtils release.Jul 15 2020, 2:30 PM
Reedy mentioned this in T218074: ip_in_range should accept explicit range notation.Sep 16 2020, 2:42 PM
Daimona subscribed.Sep 16 2020, 2:43 PM
Reedy added a parent task: T247212: Make new IPUtils release.Sep 20 2020, 4:07 PM
Reedy added a project: Platform Engineering.Sep 20 2020, 4:10 PM

Tagging Platform Engineering for some help with the way forward to get it unblocked...

Technically might be Platform Team Workboards (External Code Reviews), but it's potentially not just CR needed...

daniel edited projects, added Platform Team Workboards (Clinic Duty Team); removed Platform Engineering.Sep 21 2020, 12:33 PM
daniel moved this task from Inbox to Waiting for Review on the Platform Team Workboards (Clinic Duty Team) board.
tstarling subscribed.Nov 9 2020, 9:37 PM

This behaviour of converting ::1 to 127.0.0.1 was added in 2006 in r17989. IP::canonicalize() was added there to be used for a very specific purpose -- to assist in parsing XFF headers -- and the author believed that converting ::1 to 127.0.0.1 would assist in matching trusted proxies. It arguably should have been in the caller to start with. It doesn't really make sense to preserve this behaviour in a general-purpose library. I would suggest removing the behaviour, updating the test, and also adding a special case to WebRequest::getIP() for backwards compatibility.

Reedy added a comment.Nov 9 2020, 10:06 PM
In T248237#6614927, @tstarling wrote:

This behaviour of converting ::1 to 127.0.0.1 was added in 2006 in r17989. IP::canonicalize() was added there to be used for a very specific purpose -- to assist in parsing XFF headers -- and the author believed that converting ::1 to 127.0.0.1 would assist in matching trusted proxies. It arguably should have been in the caller to start with. It doesn't really make sense to preserve this behaviour in a general-purpose library. I would suggest removing the behaviour, updating the test, and also adding a special case to WebRequest::getIP() for backwards compatibility.

Thanks Tim.

Can you please CR https://gerrit.wikimedia.org/r/c/mediawiki/libs/IPUtils/+/582528 to get this going again?

When that's merged, it looks time to do a new release (3.0.0 maybe?) so we can bring the library into MW, and then update the related code as appropriate

https://github.com/wikimedia/ip-utils/compare/2.0.0...master

gerritbot added a comment.Nov 10 2020, 4:11 AM

Change 582528 merged by jenkins-bot:
[mediawiki/libs/IPUtils@master] Fix up some issues with 2.0.0 canonicalize()

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

Reedy mentioned this in rMLIP8d8aeb9396e4: Fix up some issues with 2.0.0 canonicalize().Nov 10 2020, 4:43 AM
Maintenance_bot removed a project: Patch-For-Review.Nov 10 2020, 5:10 AM
gerritbot added a comment.Nov 10 2020, 12:07 PM

Change 640398 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] [WIP] Update wikimedia/ip-utils from 1.0.0 to 3.0.0

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

gerritbot added a project: Patch-For-Review.Nov 10 2020, 12:07 PM
Reedy added a parent task: T218074: ip_in_range should accept explicit range notation.Nov 10 2020, 12:20 PM
Reedy removed a parent task: T218074: ip_in_range should accept explicit range notation.
gerritbot added a comment.Nov 10 2020, 1:33 PM

Change 640412 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/CheckUser@master] Canonicalise ::1 -> 127.0.0.1

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

• AMooney moved this task from Waiting for Review to Blocked Externally on the Platform Team Workboards (Clinic Duty Team) board.Nov 17 2020, 2:33 PM
• AMooney subscribed.

Let us know when this is ready for more review.

Reedy renamed this task from IPUtils 2.0.0 test failures on MW core to IPUtils >=2.0.0 test failures on MW core.Jan 26 2021, 3:06 AM
Reedy claimed this task.Jan 26 2021, 11:22 PM
gerritbot added a comment.Jan 27 2021, 12:00 AM

Change 640398 merged by jenkins-bot:
[mediawiki/core@master] Update wikimedia/ip-utils from 1.0.0 to 3.0.1

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

Jdforrester-WMF closed this task as Resolved.Jan 27 2021, 12:01 AM
ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.29; 2021-02-02).Jan 27 2021, 1:00 AM
gerritbot added a comment.Jan 27 2021, 1:09 AM

Change 640412 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] Canonicalise ::1 -> 127.0.0.1

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

Krinkle edited projects, added mediawiki-libs-IPUtils; removed archived-IPUtils.Jan 3 2024, 3:05 PM
Maintenance_bot removed a project: Patch-For-Review.Jan 3 2024, 3:31 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