Page MenuHomePhabricator

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

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

Event Timeline

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

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

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

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

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

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

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

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

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

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.

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

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

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

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 added a subscriber: AMooney.

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

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

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

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