The root cause for this is that MediaWiki's IP class considers these IP addresses valid, but is not able to convert them to hex. I fixed the conversion to hex. See also T62035.

'/(?<=\.)0+(?=([1-9]|0\.))/'

Unnecessary parens inside the (?=).

Also, consider this address: 00192.0.2.000, the extraneous zeros won't be stripped. I'd recommend using '/(?:^|(?<=\.))0+(?=[1-9]|0\.|0$)/' instead.

I concur with @Anomie's regex correction. However, can we refactor out the preg_replace() into a separate function so that we can unit test that more effectively? Or, toward the same aim, can that preg_replace() be moved to the IPv4 handling portion of sanitizeIP()?

Attached for review/comments are two patches:

I think the latter is preferable for maintenance and code review reasons. What say others?

In T97897#1350038, @dpatrick wrote:

T97897_w_Anomie.patch1 KBDownload

which is just the original patch, but with @Anomie's regex correction;

Looks ok. Haven't tested.

T97897_dpatrick_refactor.patch3 KBDownload

is a refactor which pulls out the preg_match() and adds additional test cases.

I'm not liking that we would now have IP::sanitizeIP()/IP::prettifyIP() that mostly ignores IPv4 and IP::standardizeIPv4() that does real cleanup for v4 but returns false for v6. I'd rather see the public behavior of your IP::standardizeIPv4() integrated into IP::sanitizeIP(). Then just use IP::sanitizeIP() inside the IP::toHex() case for IPv4 like it already does for IPv6. Or is there some sort of BC concern that prevents that?

Code-wise I don't care whether you put the logic from IP::standardizeIPv4() directly into IP::sanitizeIP() or split both the IPv4 and IPv6 logic into private functions with IP::sanitizeIP() just checking the type and calling the appropriate one. As for IP::toHex(), since we're touching it to this extent IMO either IP::IPv6ToRawHex() should be merged in or the IPv4 logic should be split out to a private IP::IPv4ToRawHex(), as it is now it's just weird.

In T97897#1353830, @Anomie wrote:

In T97897#1350038, @dpatrick wrote:

T97897_dpatrick_refactor.patch3 KBDownload

is a refactor which pulls out the preg_match() and adds additional test cases.

I'm not liking that we would now have IP::sanitizeIP()/IP::prettifyIP() that mostly ignores IPv4 and IP::standardizeIPv4() that does real cleanup for v4 but returns false for v6. I'd rather see the public behavior of your IP::standardizeIPv4() integrated into IP::sanitizeIP(). Then just use IP::sanitizeIP() inside the IP::toHex() case for IPv4 like it already does for IPv6. Or is there some sort of BC concern that prevents that?

I do have backwards compatibility concerns, yes. That's why i didn't add the code to sanitizeIP() in this patch. Here are some examples:

• [[https://git.wikimedia.org/blob/mediawiki%2Fcore/8af83f4ff833b40ce9497e25ff2df439a0fa18a3/includes%2Fapi%2FApiQueryLogEvents.php#L168|core/includes/api/ApiQueryLogEvents.php:168]] and [[https://git.wikimedia.org/blob/mediawiki%2Fcore/8af83f4ff833b40ce9497e25ff2df439a0fa18a3/includes%2Flogging%2FLogPager.php#L170|core/includes/logging/LogPager.php:170]] call IP::sanitizeIP() when searching logs by user. In theory, anonymous users may have leading zeros in their IP octets. If we change IP::sanitizeIP() to remove leading zeros, then searches may fail to find results.
• [[https://git.wikimedia.org/blob/mediawiki%2Fcore/8af83f4ff833b40ce9497e25ff2df439a0fa18a3/includes%2FWebRequest.php#L1125 |core/includes/WebRequest.php:1125]] determines the current user's originating IP address from the X-Forwarded-For header, then calls IP::sanitizeIP(). The same sort of failure to locate all changes made by a give user may occur if IP::sanitizeIP() is changed.
• [[https://git.wikimedia.org/blob/mediawiki%2Fextensions%2FAbuseFilter/f0b0f2826558528115c1bc7356272b9da95115a2/api%2FApiQueryAbuseLog.php#L117|AbuseFilter/api/ApiQueryAbuseLog.php:117]], same deal as the first two instances.
• [[https://git.wikimedia.org/blob/mediawiki%2Fextensions%2FAbuseFilter/f0b0f2826558528115c1bc7356272b9da95115a2/special%2FSpecialAbuseLog.php#L92|AbuseFilter/special/SpecialAbuseLog.php:92]], again.

I stopped examining grep results there, but there were other calls to IP::sanitizeIP() from the CheckUser, GlobalBlocking, ProofreadPage, and TorBlock extensions. In short, I hated to add another method, but I was concerned about breaking stuff by changing the behavior of IP::sanitizeIP().

Code-wise I don't care whether you put the logic from IP::standardizeIPv4() directly into IP::sanitizeIP() or split both the IPv4 and IPv6 logic into private functions with IP::sanitizeIP() just checking the type and calling the appropriate one. As for IP::toHex(), since we're touching it to this extent IMO either IP::IPv6ToRawHex() should be merged in or the IPv4 logic should be split out to a private IP::IPv4ToRawHex(), as it is now it's just weird.

We can't merge IP::IPv6ToRawHex() into IP::toHex() because it's called from IP::parseCIDR6(). I suppose it makes sense to pull the IPv4 functionality out into a separate IP::IPv4ToRawHex() function, but that function would still need to invoke a separate IP::standardizeIPv4()/IP::removeLeadingIPv4OctetZeros()/whatever in order to meet me original goal of improving testability of the preg_replace().

I'd rather actually look into those calls to see if anything really would break. XFF header parsing, for example, really should be normalizing the zeros to prevent spurious non-matches if some upstream is injecting IPs with the abnormal zeros.

This patch integrates zero trimming into sanitizeIP:

In T97897#1450079, @dpatrick wrote:

This patch integrates zero trimming into sanitizeIP:

T97897_01.patch3 KBDownload

Comment for sanitizeIP should be updated to reflect the change for ipv4. Otherwise I think that addresses @Anomie's comments.

@csteipp, thanks. Updated and rebased:

@Anomie, any more comments?

@Anomie?

I wasn't able to duplicate Vituzzu original issue in my testing, where all newbies were blocked. But I can confirm that after this patch, adding a block with 000 in it is normalized correctly to a 0, and the IP is actually blocked, where before the IP wasn't. Both via the Special page and API. Setting a local block exemption seems to work correctly with the patch as well.

Also, if a trusted proxy adds an IP with a 00 in it, the user is still correctly blocked, although there's a php warning,

Warning: inet_pton(): Unrecognized address 141.00.11.253 in <b>/var/www/wiki/en/vendor/wikimedia/ip-set/src/IPSet.php

I'll go ahead and deploy this in the next day or two.

Swell. Thanks Chris. Is that warning worth mentioning to the IPSet maintainer?

Here is an updated patch

LGTM

In T97897#1818516, @csteipp wrote:

T97897_03.patch4 KBDownload

LGTM

Patch applies cleanly to 1.24 and beyond.

Patch for 1.23: