Improvement of the IPv4 address format check

[ka2]

For the current "is_ip_address()" in the function of out of range, such as "256.256.256.256" IP so will also be judged as IPv4, it is increases the usefulness of the function you have to return false for out of range of IP.

[ka2]

I fixed file.

[GaryJ]

Hello ka2, welcome to Trac. Thank you for your ticket and patch.

A couple of things about your patch to consider:

• Only the last hunk is important here - all the rest are un-related whitespace fixes. Try and keep to fixes just in the immediate function you're changing - in this case, adding the non-optional braces for the if that you're amending.
• This is going to need unit tests to show that 999.1.1.1 and other similarly broken IPv4 address potentials are no longer valid. If unit tests for correct IP addresses, these should be added too.

[johnbillion]

This method doesn't validate an IP address, it just checks whether the passed value looks like an IP address as opposed to a domain name. From the method's description:

This does not verify if the IP is a valid IP, only that it appears to be an IP address.

This method is used in WP_Http_Streams::verify_ssl_certificate() to determine whether the host is an IP address or a domain name. If an invalid IP address is passed to an HTTP request, then WP_Http::is_ip_address() must still return true otherwise the host will incorrectly be treated as a domain name. Note that either way, the HTTP request will fail due to the invalid IP address.

I think this is a wontfix.

[chriscct7]

I actually like this enhancement. It ensures that non-valid IP addresses won't be validated as such, so if plugins rely on this to check an IP address, it can be safely used as part of a validation routine. Refreshed patch from proper directory.

[swissspidy]

34538.patch​ still includes unneeded whitespace changes.

This method doesn't validate an IP address, it just checks whether the passed value looks like an IP address as opposed to a domain name.

Agree on this, though the second regex seems to check for real IPv6 addresses. See ​http://home.deds.nl/~aeron/regex/

If an invalid IP address is passed to an HTTP request, then WP_Http::is_ip_address() must still return true otherwise the host will incorrectly be treated as a domain name. Note that either way, the HTTP request will fail due to the invalid IP address.

What exactly would change for an HTTP request with this proposed patch?

[ka2]

Thank you for your consideration various about my patch.

As you say, I also think that there is no benefit to the existing HTTP request processing even if the verification of the IP address by my patch was stricter.
However, I think that this function is useable from a place other than the core, as for example plugins. Also I think that it will become very user-friendly feature if "WP_Http::is_ip_address()" will be able to strictly the IP validation.

As another approach, I thought that divides the verification processing by adding a toggle of strict verification to the second argument of the function. But it just would simply become a redundant function.

I suggested to the ticket because I thought that might be there are happy to some people in the case that became to be able to complete IP address verification in this function.

Thank you,

[ka2]

I've created a unit test.
Test code contents are as follows:

	/**
	 * @dataProvider ip_address_testcases
	 */
	function test_is_ip_address( $maybe_ip, $expected ) {
		$actual = WP_Http::is_ip_address( $maybe_ip );
		$this->assertEquals( $expected, $actual );
	}

	function ip_address_testcases() {
		// 0: The IP Address, 1: The expected resulting
		return array(
			// Valid IPv4 address
			// See: http://tools.ietf.org/html/rfc5737
			array( '0.0.0.0', 4 ),
			array( '127.0.0.1', 4 ), // The loopback address
			array( '192.0.2.0', 4 ), // RFC 5737 - IPv4 Address Blocks Reserved for Documentation
			array( '198.51.100.0', 4 ), // RFC 5737 - IPv4 Address Blocks Reserved for Documentation
			array( '203.0.113.0', 4 ), // RFC 5737 - IPv4 Address Blocks Reserved for Documentation
			array( '255.255.255.255', 4 ),
			
			// Invalid IPv4 address
			array( '256.255.255.255', false ), // The first octet is out of range
			array( '255.256.255.255', false ), // The second octet is out of range
			array( '255.255.256.255', false ), // The third octet is out of range
			array( '255.255.255.256', false ), // The fourth octet is out of range
			array( '999.999.999.999', false ), // All octet is out of range
			array( '2000.1.1.1', false ),
			array( '1.2000.1.1', false ),
			array( '1.1.2000.1', false ),
			array( '1.1.1.2000', false ),
			array( '2000.2000.2000.2000', false ),
			
			// Valid IPv6 address
			// See: http://tools.ietf.org/html/rfc4291
			array( '0:0:0:0:0:0:0:0', 6 ), // The unspecified address
			array( '::', 6 ), // The unspecified address (in compressed form)
			array( '0:0:0:0:0:0:0:1', 6 ), // The loopback address
			array( '::1', 6 ), // The loopback address (in compressed form)
			array( '2001:db8::', 6 ), // RFC 3849 - IPv6 Address Prefix Reserved for Documentation
			array( '2001:0db8:0000:0000:0000:0000:dead:beaf', 6 ),
			array( '2001:db8:0:0:0:0:dead:beaf', 6 ),
			array( '2001:db8::dead:beaf', 6 ),
			array( 'ABCD:EF01:2345:6789:ABCD:EF01:2345:6789', 6 ), // RFC 4291 - Example
			array( '2001:DB8:0:0:8:800:200C:417A', 6 ), // RFC 4291 - Example of unicast address
			array( '2001:DB8::8:800:200C:417A', 6 ), // RFC 4291 - Example of unicast address (in compressed form)
			array( 'FF01:0:0:0:0:0:0:101', 6 ), // RFC 4291 - Example of multicast address
			array( 'FF01::101', 6 ), // RFC 4291 - Example of multicast address (in compressed form)
			array( '0:0:0:0:0:0:13.1.68.3', 6 ), // RFC 4291 - Example of an alternative form with a mixed environment of IPv4 and IPv6 nodes
			array( '::13.1.68.3', 6 ), // RFC 4291 - Example of an alternative form with a mixed environment of IPv4 and IPv6 nodes (in compressed form)
			array( '0:0:0:0:0:FFFF:129.144.52.38', 6 ), // RFC 4291 - Example of an alternative form with a mixed environment of IPv4 and IPv6 nodes
			array( '::FFFF:129.144.52.38', 6 ), // RFC 4291 - Example of an alternative form with a mixed environment of IPv4 and IPv6 nodes (in compressed form)
			
			// Invalid IPv6 address
			// See: https://github.com/richb-intermapper/IPv6-Regex
			array( '2001:DB8:0:0:8:800:200C:417A:221', false ), // unicast full
			array( 'FF01::101::2', false ), // multicast compressed
			array( '02001:0000:1234:0000:0000:C1C0:ABCD:0876', false ), // extra 0 not allowed
			array( '2001:0000:1234:0000:00001:C1C0:ABCD:0876', false ), // extra 0 not allowed
			array( '2001:0000:1234:0000:0000:C1C0:ABCD:0876  0', false ), // junk after valid address
			array( '2001:0000:1234: 0000:0000:C1C0:ABCD:0876', false ), // internal space
			array( '3ffe:0b00:0000:0001:0000:0000:000a', false ), // Segments is less than 8
			array( 'FF02:0000:0000:0000:0000:0000:0000:0000:0001', false ), // Segments is over 8
			array( '3ffe:b00::1::a', false ), // Compressed double
			array( '::1111:2222:3333:4444:5555:6666::', false ), // Compressed double
			array( '1::5:400.2.3.4', false ), // Embedded ipv4 address is out of range
			array( '1::5:192.168.0.256', false ), // Embedded ipv4 address is out of range
			array( '2001:1:1:1:1:1:255Z255X255Y255', false ), // garbage instead of "." in IPv4
			array( '::ffff:192x168.1.26', false ), // ditto
			array( '::ffff:2.3.4', false ), // has ipv4 octet lost
			array( '1.2.3.4:1111:2222:3333:4444::5555', false ), // Aeron
			array( ':', false ),
			array( ':::', false ),
			
			// other
			array( '123', false ),
			array( 'ldkfj', false ),
			array( '.', false ),
			array( '..', false ),
			array( '...', false ),
			
		);
	}

Then, results of unit test execution are as follows:

# phpunit --group="http"
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 5.0.8 by Sebastian Bergmann and contributors.

................................................................ 64 / 89 ( 71%)
.........................                                        89 / 89 (100%)

Time: 16.41 seconds, Memory: 74.75Mb

OK (89 tests, 89 assertions)

I will attach a patch file that unit testing is included.

In addition, I have been corresponding of parts leakage in white space refactoring.

Thank you,

[ka2]

It is a patch that contains the unit test.

[chriscct7]

The new patch is good but the whitespace changes at the top of it need to be removed from it for it to be merged into core.

[ka2]

I recreated a patch with removed of the whitespace changes.

Thank you,