REST API: Support ipv6 for `author_ip` in comments endpoint

[dd32]

The comments endpoint currently accepts a author_ip field, however sanitizes it to be an IPv4 address, and as a result rejects IPv6 addresses.

IPv6 is supported out of the box by WordPress and in all other comment APIs, and so should be supported.

[schlessera]

Allow both IPv4 & IPv6 for comment author IP

[schlessera]

I've created a patch to allow for the comment author IP address to be either an IPv4 or an IPv6 address.

To achieve this while staying compatible to both the code that uses the rest_is_ip_address() function as well as the code that uses the format validation ipv4, I've split up rest_is_ip_address() in the following way:

rest_is_ip_address( $ip ) => IPv4 & IPv6
rest_is_ipv4_address( $ipv4 ) => IPv4 only
rest_is_ipv6_address( $ipv6 ) => IPv6 only

I've added corresponding unit tests to make sure IPv6 addresses are correctly addressed.

In the same vein, there are now three different validation formats: ip, ipv4 and ipv6.

The validation format for a comment's author_ip was changed from ipv4 to ip.

Note: As I was not aware of a specific reason why the PHP's built-in filter_var with the flag FILTER_VALIDATE_IP was not used (the IPv4 check used a custom regex), I've implemented each of these functions as such a filter_var() call, which should provide the best performance. filter_var is available in PHP 5.2+.

[joehoyle]

I think we should stick with just rest_is_ip_address, I don't see a need to validation ipv4 / ipv6 separately, doing so will only make it more likely they developers only support one of the formats, when really all code should be written to support both. I'd get rid of rest_is_ipv4_address and rest_is_ipv6_address and just have a single function that can validate either. Same for the schema types.

[schlessera]

Replying to joehoyle:

I'd get rid of rest_is_ipv4_address and rest_is_ipv6_address and just have a single function that can validate either. Same for the schema types.

I agree that all code should ideally support both. However, getting rid of the schema type ipv4 in favor of ip might break existing code that already relies on the schema type ipv4.

[swissspidy]

I agree that all code should ideally support both. However, getting rid of the schema type ipv4 in favor of ip might break existing code that already relies on the schema type ipv4.

What about automatically mapping ipv4 to ip? Only checking IPv4 addresses doesn't make sense, but if the ipv4 is to be kept and one really needs to only validate IPv4, we could also add a new parameter to rest_is_ip_address().

Patch needs to be updated anyway. We can't use filter_var() as the filter extension can be disabled on older PHP versions.

[dd32]

FWIW I don't see the need to to allow validation of only-ipv4, having it just specify ip and checking it's a valid IP seems the best way forward to me. If we can internally map ipv4 to a generic ip check that seems even better.

The only reason I would want a specific IPv4 validator is if the json schema spec followed doesn't allow a generic ip specification and wanted it to be specified as [ 'ipv4', 'ipv6' ].

As for validating IPv6 addresses, that's a super-complex regex AFAIK, we may be able to borrow/call/re-use something from src/wp-includes/Requests/IPv6.php / src/wp-includes/SimplePie/Net/IPv6.php though (I wouldn't worry too much about requiring the IPv6 address is strictly valid though, I'd put the onus on the caller, a simple basic syntax check may be okay here)

[rmccue]

Replying to dd32:

As for validating IPv6 addresses, that's a super-complex regex AFAIK, we may be able to borrow/call/re-use something from src/wp-includes/Requests/IPv6.php

Requests_IPv6::check_ipv6() can be used directly to check this.

[jnylen0]

What about automatically mapping ipv4 to ip

I'm against this - it would mean that ipv4 is lying about what it does, which is unexpected. Also it's a change from behavior supported in previous versions.

I think a better solution is to add ip and deprecate ipv4, and document it as deprecated.

[dd32]

I'm against this - it would mean that ipv4 is lying about what it does, which is unexpected. Also it's a change from behavior supported in previous versions.

I don't see BC as an issue for this. Anything that is currently requiring an IPv4 is probably unintentionally broken - yes, there's the possibility that there exists something out there that requires it, but breaking that one in a million and simplifying it is much more sane.

[jnylen0]

Having a format string that is called ipv4 but does something else related to IP addresses is not sane.

[dd32]

Having a format string that is called ipv4 but does something else related to IP addresses is not sane.

When you phrase it like that, I agree, but validating as ip is better than not validating at all, especially when the intent of setting it to ipv4 is to validate as an IP. Sane validators for the future is a better solution than adding a validator for something that's hopefully soon going to be a thing of the past, ease the transition forward.

[rachelbaker]

Needs a refreshed patch that uses rest_is_ip_address() to also check Requests_IPv6::check_ipv6() for ipv6 addresses.

[danielbachhuber]

Allow both IPv4 and IPv6 for 'ip' schema format

[danielbachhuber]

38818.2.diff changes the 'ipv4' format to a more generic 'ip' format, which supports IPv4 and IPv6

I'm not concerned about backwards compatibility w/r/t changing the format.

[schlessera]

38818.3.diff is similar to 38818.2.diff by @danielbachhuber, but it uses the original regex for IPv4 and Requests_IPv6::check_ipv6() for IPv6.

[joehoyle]

In 39296:

REST API: Change “ipv4” types to “ip” to support ipv6.

Stop presuming IP address are IPv4, instead make the type “ip” to be agnostic of IP version. This fixes requests with ipv6 addresses for comments in core.

Props dd32, schlessera, danielbachhuber.
Fixes #38818.