Opened 9 years ago
Last modified 8 years ago
#34538 reviewing enhancement
Improvement of the IPv4 address format check
Reported by: | ka2 | Owned by: | chriscct7 |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 4.4 |
Component: | HTTP API | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
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.
Attachments (4)
Change History (13)
#1
@
9 years ago
- Keywords has-patch needs-unit-tests added
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.
#2
@
9 years ago
- Keywords close added
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.
#3
@
9 years ago
- Keywords close removed
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.
#4
@
9 years ago
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?
#5
@
9 years ago
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,
#6
@
9 years ago
- Keywords has-unit-tests added; needs-unit-tests removed
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,
I fixed file.