IP with port number triggers warnings in WP_Community_Events

[EatonZ]

Hello, I'd like to assume this is probably a duplicate report, but I couldn't find anything via the search, so I thought I would report this anyway to ensure it gets fixed in the next update, as it's very annoying.

After upgrading to the latest WordPress, the following errors appear in my PHP errors log:

[16-Jun-2017 16:53:03 UTC] PHP Warning:  inet_pton(): Unrecognized address 104.xxx.xxx.98:58749 in C:\home\site\wwwroot\wp-admin\includes\class-wp-community-events.php on line 268
[16-Jun-2017 16:53:03 UTC] PHP Warning:  inet_pton(): Unrecognized address 104.xxx.xxx.98:58749 in C:\home\site\wwwroot\wp-admin\includes\class-wp-community-events.php on line 274
[16-Jun-2017 16:53:03 UTC] PHP Warning:  A non-numeric value encountered in C:\home\site\wwwroot\wp-admin\includes\class-wp-community-events.php on line 274
[16-Jun-2017 16:53:03 UTC] PHP Warning:  inet_ntop(): Invalid in_addr value in C:\home\site\wwwroot\wp-admin\includes\class-wp-community-events.php on line 274
[16-Jun-2017 16:53:04 UTC] PHP Warning:  inet_pton(): Unrecognized address 104.xxx.xxx.98:4108 in C:\home\site\wwwroot\wp-admin\includes\class-wp-community-events.php on line 268
[16-Jun-2017 16:53:04 UTC] PHP Warning:  inet_pton(): Unrecognized address 104.xxx.xxx.98:4108 in C:\home\site\wwwroot\wp-admin\includes\class-wp-community-events.php on line 274
[16-Jun-2017 16:53:04 UTC] PHP Warning:  A non-numeric value encountered in C:\home\site\wwwroot\wp-admin\includes\class-wp-community-events.php on line 274
[16-Jun-2017 16:53:04 UTC] PHP Warning:  inet_ntop(): Invalid in_addr value in C:\home\site\wwwroot\wp-admin\includes\class-wp-community-events.php on line 274

I have replaced parts of the IP address with "xxx" because it is my computer's IP address (not the server's).

This error is 100% reproducible by simply loading the admin dashboard page with the WordPress Events and News box shown.

This is new to WordPress 4.8.

PHP version is 7.1.6.
Windows Server 2012 R2/IIS 8.5

[iandunn]

I was able to reproduce this, by adding a port number to the IP. I'm guessing IIS does that by default, or maybe some configurations do it regardless of platform.

The port isn't important for our use case, so we can probably just trim it off.

[jsonm]

Updated code to strip the port before the address is used by inet_ functions.

[jsonm]

My patch won't work with ipv6 addresses actually, I'll need to rework it.

[iandunn]

Alternate approach with tests - not finished

[iandunn]

I worked on this a bit the other day too, 41083.1.diff​ is as far as I got. It needs some more work, though, and I'm not entirely confident that it's the best approach, either. 41083.0.diff​ might be a better approach, but I haven't had time to take a close look.

41083.1.diff​ adds a few unit tests, but I think there might be a logic error in the loop, since one of the failing tests seems to be getting a completely different IP back. I haven't tested it on IIS servers, or those without the filter extension installed.

I used filter_var in get_unsafe_client_ip(), because it felt too fragile to rely on regex, strpos, etc, especially if when trying to use a single statement for both IPv4 and IPv6 addresses. filter_var is the most robust way to differentiate between the two.

The filter extension is disabled on a fraction of servers, though, so the function just returns false in those cases, which prevents IP geolocation. We could instead opt to avoid the anonymization if the extension is disabled, but if we're going to anonymize results, then it seems better to be consistent about it, especially when the determining factors are outside the user's control. Related #40974.

On the other hand, though, we could just remove the IP anonymization entirely, since CDN requests already expose the user's IP to w.org, see #40871 and ​https://wordpress.slack.com/archives/C02RQBWTW/p1496167727833251. If this turns out to be a bigger headache than it initially appears, then that might be the simplest and most maintenance-friendly option.

We'd probably still need to strip the port off, though, so that we're sending valid input to the API.

[iandunn]

Got another report of this, but with an IPv6 address that lacks a port. We'll need to test for this case too.

Warning: inet_pton(): Unrecognized address 2601:601:f00:d1bb:a468:d709:115e:de78 in wp-admin/includes/class-wp-community-events.php on line 268

Warning: inet_pton(): Unrecognized address ffff:ffff:ffff:ffff:0000:0000:0000:0000 in wp-admin/includes/class-wp-community-events.php on line 274

Warning: inet_ntop(): Invalid in_addr value in wp-admin/includes/class-wp-community-events.php on line 274

I wasn't able to reproduce it on Ubuntu 14 w/ Nginx and PHP 5.5.9, though, so it may only happen on certain setups. It looks like PHP can be compiled without IPv6 support. We can use defined( 'AF_INET6' ) to detect that.

[iandunn]

For reproducing the IPv6 issue, the site is hosted on a SiteGround shared "Start Up" hosting plan. In this case, it looks like it's running Linux, Apache, and PHP 5.6.

[photoMaldives]

Hi guys. I'm a WP user, small-scale developer.
Been seeing this error in my WP dashboard for some days (with 'WordPress Events and News' unchecked). A quick Google didn't find much, but brought me here.
On my 10 WP sites, this error is only seen on 3 Siteground-hosted sites (no problem with A2, Bluehost, Vidahost). WP theme is the same for all sites (ET's Divi) and plugins are similar (Wordfence, iThemes, Yoast, etc).

Jun 24, 11:50:36
PHP Warning: inet_ntop(): Invalid in_addr value in /wp-admin/includes/class-wp-community-events.php on line 274
Jun 24, 11:50:36
PHP Warning: inet_pton(): Unrecognized address ffff:ffff:ffff:ffff:0000:0000:0000:0000 in /wp-admin/includes/class-wp-community-events.php on line 274
Jun 24, 11:50:36
PHP Warning: inet_pton(): Unrecognized address for=92.xx.xxx.xx in /wp-admin/includes/class-wp-community-events.php on line 268

Jun 24, 11:46:06
PHP Warning: inet_ntop(): Invalid in_addr value in /wp-admin/includes/class-wp-community-events.php on line 274
Jun 24, 11:46:06
PHP Warning: inet_pton(): Unrecognized address ffff:ffff:ffff:ffff:0000:0000:0000:0000 in /wp-admin/includes/class-wp-community-events.php on line 274
Jun 24, 11:46:06
PHP Warning: inet_pton(): Unrecognized address for=92.xx.xxx.xx in /wp-admin/includes/class-wp-community-events.php on line 268

Log copy/pasted as-is, except '92.xx.xxx.xx'= my IP. The 3 errors seem to repeat every ~4 minutes.
Hope this helps, and good luck. :-)
pM

[DrewAPicture]

Assigning ownership to mark the good-first-bug as "claimed".

[EatonZ]

Another "fix" for this is to put the site behind Cloudflare. If anyone is annoyed by this warning, using Cloudflare will fix it for you.

[swalker1595]

This issue still manifest itself when using localhost.

When localhost resolves to an IPv6 Address, [::1]:80, the error persist.

As I am not an experienced PHP Developer, I have modified the file like so:

<?php
public static function get_unsafe_client_ip()

{
        $client_ip = false;
        // In order of preference, with the best ones for this purpose first.
        $address_headers = array(
                'HTTP_CLIENT_IP',
                'HTTP_X_FORWARDED_FOR',
                'HTTP_X_FORWARDED',
                'HTTP_X_CLUSTER_CLIENT_IP',
                'HTTP_FORWARDED_FOR',
                'HTTP_FORWARDED',
                'REMOTE_ADDR',
        );
        foreach($address_headers as $header) {
                if (array_key_exists($header, $_SERVER)) {
                        /*
                        * HTTP_X_FORWARDED_FOR can contain a chain of
                        comma-separated
                        * addresses. The first one is the original client. It can't
                        be
                        * trusted for authenticity, but we don't need to for this
                        purpose.
                        */
                        $address_chain = explode(',', $_SERVER[$header]);
                        $client_ip = trim($address_chain[0]);
                        if ((strlen($client_ip) < 25) && (stristr($client_ip, ":") !== FALSE)) {
                                $client_ip = substr($client_ip, 0, strpos($client_ip, ":"));
                        }
                        break;
                }
        }
     
        /* CUSTOM MODIFICATION */
        if (empty($client_ip)) {
                $client_ip = '127.0.0.1';
        }
        /* END CUSTOM MODIFICATION */


        // These functions are not available on Windows until PHP 5.3.
        if (function_exists('inet_pton') && function_exists('inet_ntop')) {
                if (4 === strlen(inet_pton($client_ip))) {
                        $netmask = '255.255.255.0'; // ipv4.
                }
                else {
                        $netmask = 'ffff:ffff:ffff:ffff:0000:0000:0000:0000'; //
                        ipv6 .
                }
                $client_ip = inet_ntop(inet_pton($client_ip) & inet_pton($netmask));
        }
        return $client_ip;
}

[jbpaul17]

Punting to 4.9 per today's bug scrub and resulting discussion in #core.

[spaatz]

Replying to photoMaldives:

I am running Divi as well. Newest version as of yesterday. Errors only appear on mobile in the format narrated. Reinstalling wordpress from the updates panel fixed the issue last time it appeared. To the comment that Cloudflare fixes this, I tried to switch to Cloudflare and that is when the problem resurfaced. They may or may not be related.

Yoast SEO as well.

[EatonZ]

Maybe WordFence is a factor in this. If you use it, try setting this to stop the errors:

Before, it was on: "Let Wordfence use the most secure method to get visitor IP addresses. Prevents spoofing and works with most sites."

Maybe anyone who failed to reproduce should install WordFence and see if it causes this.

[woodwardmatt]

Replying to iandunn:

I worked on this a bit the other day too, 41083.1.diff​ is as far as I got. It needs some more work, though, and I'm not entirely confident that it's the best approach, either. 41083.0.diff​ might be a better approach, but I haven't had time to take a close look.

I tested this diff (41083.1.diff​) on a vanilla install of 4.8.1 where the IP4 address (with port) was causing the warnings to be generated. All warnings were suppressed after applying the diff.

[SergeyBiryukov]

#41651 was marked as a duplicate.

[iandunn]

Replying to iandunn:

there might be a logic error in the loop...

Part of the problem is that it's using $_GET instead of $_SERVER, but there's still another problem I haven't tracked down yet.

I used filter_var in get_unsafe_client_ip(), because it felt too fragile to rely on regex, strpos, etc...

​parse_url() almost works for both IPv4 and IPv6 addresses all the way back to 5.2.4, as long as a scheme is prepended (see ​#72811-php). With a little cajoling, it might work for IPv6 addresses that don't have ports. If it can, that'd probably be simpler and more compatible than filter_var().

[iandunn]

Alternative to filter_var

[iandunn]

Nevermind, I missed that parse_url() doesn't handle IPv6 addresses that don't contain port numbers. We could add support for that to wp_parse_url(), but that doesn't seem like a good idea, since I think the intention of functions like those is really just only to smooth out compatibility issues between PHP versions, not extend PHP's functionality. I could be wrong there, though.

filter_var() would be nice because it's the most reliable way to parse IPs, but it's typically avoided in Core because the PHP extension can be disabled. It might be ok in this case if there weren't an alternative, because in this situation we can fallback gracefully, by short-circuiting geolocation and letting the user manually enter an address. I think I found an alternative approach, though.

Since we really only need to know if the input is formatted as an IPv4 address or an IPv6 address, we can use a quick and dirty method, like in 41083.2.diff​.

Another idea would be to update WP_Http::is_ip_address() to support port numbers, but that could turn into a rabbit hole with unintended consequences, especially since it's used outside the class (by the REST API and plugins).

Yet another idea would be using @ to suppress the warnings, but that's pretty hacky, and could unintentionally hide errors that we'd want to fix.

So, I'm left thinking that the approach in 41083.2.diff​ might be the least-bad one. I'm leery of adding so much code just to do something that seems as simple as stripping off a port number, though.

Does anyone see a more elegant solution?

[danieltj]

Whether or not I'll be able to help here I'll take a look at this when I have a spare moment tonight or tomorrow and see if I can see anything.

Personally if this is just a localised issue to this one metabox then I think it's best to change things that only affect the geolocation and not do anything that might alter something outside this issue. Especially considering it's a somewhat unique issue to itself.

[birgire]

Reminds me of the IPv6 support ticket #41722 for db hosts:

[birgire]

Here's a modified regex from the patch by @schlessera in ticket #41722

I changed it to preg_replace and removed the port match.

The regex might not be the way forward here, but a stripped down helper function might look like:

function strip_ip_address_port( $address ) {
    if ( substr_count( $address, ':' ) > 1 ) {                      // IPv6
        $pattern = '#^(?:\[)?([0-9a-fA-F:]+)(?:\]:([\d]+))?#'; 
    } elseif( 3 === substr_count( $address, '.' ) ) {                // IPv4 
        $pattern = '#^([^:]+)(?::([\d]+))?#';
    } else {                                                         // Not valid
        return false;
    }
    return preg_replace( $pattern, '\1', $address );
}

or if I understand the patch by @iandunn correctly, we could try to adjust it to (untested):

protected static function strip_ip_address_port( $ip_address, $address_format ) { 
	if ( 4 === $address_format ) { 
		$port_delimiter = strpos( $ip_address, ':' ); 
 		 
 	    	if ( $port_delimiter ) { 
			$ip_address = substr( $ip_address, 0, $port_delimiter ); 
		} 
 	} elseif ( 6 === $address_format ) { 
	     	$pattern = '#^(?:\[)?([0-9a-fA-F:]+)(?:\]:([\d]+))?#'; 
		$ip_address = preg_replace( $pattern, '\1', $ip_address );
 	} 		 
	return $ip_address; 
} 

but I would have to look further into that.

[danieltj]

I tried to get a fix for this but I came up with nothing, sorry guys.

I think that @iandunn method in v2 diff is on the right track though. More robust anyway.

[tdchodgy]

This is also affecting a site I am working on.

We are running WP from a container inside a Linux WebApp on Azure.

PHP 7.1 and Apache 2.4.10

[iandunn]

Iteration on 2.diff

[iandunn]

No problem @danieltj, thanks for checking!

Thanks for pointing that ticket out @birgire, the patch and test data are helpful here. I think we'll need the format detection to be a separate function from the port-stripping, though, so it can be reused when determining the $netmask.

It's tempting to simplify detect_ip_address_format() into just $address_format = substr_count( $client_ip, ':' ) > 1 ? 6 : 4 in the calling function, but that wouldn't account for invalid input. It's not unheard of for things like 'unknown' to show up in HTTP_X_FORWARDED_FOR or even REMOTE_ADDR (see #41651).

The substr_count() call is better than I came up with, though, because it accounts for IPv6 compatibility mode though (e.g., ::ffff:10.20.30.40). I've updated 41083.3.diff​ and the unit tests to use it.

I'm not sure about using regex in this case. We gain precision, but it's also more fragile and takes extra time for developers working with it to read/understand/test/etc. I think they're the best tool when the corresponding string-manipulation functions would add lots of extra code paths to catch corner cases, etc, but our needs in this case are fairly straight-forward. Are there any cases that the strpos() / substr() approach fails on? If not, then to me it seems like that's simpler and more maintainable.

I'm definitely open to disagreement on that, though. If we do use regex, I think we'll need to iterate on those patterns, since they failed some of the unit tests. There might be some well-tested, semi-canonical ones floating around the web.

[iandunn]

I'd like to get feedback from other committers before moving forward with this, since none of the options we've explored so far seem elegant enough. comment:19 is a good place to start to get up to speed on the issues.

[pento]

41083.diff​ adds support for IPv6 compatibility mode, and a bunch more tests. I also moved everything back into the single method, because all this code is really only being used in this one place.

Unfortunately, any code dealing with IP addresses is going to be inherently inelegant, as IP addresses are a messy problem. Handling all the weird edge cases makes everyone sad.

Unless anyone wants to iterate on this a bit more, I'm fine with committing it.

[birgire]

In 41083.5.diff​ I was wondering about the case where inet_ntop() and inet_pton() are not available.

If it would be better if the WP_Community_Events::get_unsafe_client_ip() method returned false, instead of sending out the actual full IP address?

[pento]

The original behaviour was to return the full IP address, this seems to be intentional, given that the behaviour is documented in the method @return docs. @iandunn or @dd32 may know why it was originally built this way.

[dd32]

While I'm not sure of the back-history of that particular decision, it seems to be erring on the better side of UX. In that scenario I'm fine with it sending the original full IP personally.

That being said, I'm curious if we shouldn't just short-circuit the function for IPv4 addresses and use direct string manipulation, to avoid those kind of issues. (if 4 occurrences of ., strip anything before/after the ip, replace 4th octet with 0).
Parsing IPv6 like that would be far more complicated, and I kind of expect the functions would be more likely available in those scenario's, based on ipv6 being used by more modern installs of PHP.. hopefully :)

[iandunn]

Overall, 41083.diff​ looks good to me at first glance, thanks @pento! I should have some time tomorrow to take a closer look and run through some manual tests.

---

Replying to dd32:

would be better if the method returned false?

The original behaviour was to return the full IP address

it seems to be erring on the better side of UX.

That's correct as far as I can remember. It's best if we can provide automatic locations for as many people as possible, so that the feature instantly works without any effort on their part.

If anyone wants to be fully anonymous in all cases, we anticipated that and released ​a plugin to make that easy for them.

41083.3.diff​ did return false if the address format couldn't be detected, but that was just a sanity check, since in that case we've encountered an unhandled edge case and don't know how to recover.

---

Replying to pento:

I also moved everything back into the single method, because all this code is really only being used in this one place.

I wasn't really concerned with reuse. Mostly it just seemed like the modularity made it much more readable and clear than doing 4 distinct tasks in one large function. I'm happy to defer to your judgement, though.

[iandunn]

41083.diff with string manipulation approach to IPv4 addresses

[iandunn]

just short-circuit the function for IPv4 addresses and use direct string manipulation, to avoid those kind of issues.

That's a good point to bring up. I tested that out in 41083.6.diff​. @dd32, does that look like what you were picturing? (I played around with some alternate flows where there was an actual early return for IPv4 addresses, but they felt less intuitive and readable.)

Overall, the two approaches seem pretty similar to me, in terms of validity. I can imagine people favoring one over the other, but just as a personal preference. I'm not seeing anything that's objectively better about one or the other.

Given that, I'm inclined to go with the "short-circuit" approach in 41083.6.diff​, since it does have the added benefit of maintaining the anonymization for installs running the latest version of WP on PHP 5.2 on Windows (a tiny percent, but not nothing in absolute numbers).

Dion, if that patch looks good to you, and @pento doesn't have any objections, then I'll go ahead and do some final testing and get it committed in time for 4.9-beta4 on Monday.

[iandunn]

Gary said he'd review this soon, and I'll commit or punt it based on that.

[pento]

41083.6.diff​ looks good to me, let's ship it.

[iandunn]

In 42016:

Dashboard: Strip ports from IPs to avoid PHP warnings.

Fixes #41083.
Props pento, iandunn, EatonZ, birgire, dd32.

[EatonZ]

Thanks for all your hard work. I'm surprised it was this complicated to fix!
I've just installed the official 4.9 release and can confirm the warnings are no longer displayed.👍

[eamax]

On my smartphone (Samsung A5 - Chrome app) the warning error appears, I believe it is the same one quoted. Wifi or 4G.
Wordpress 4.9.1
PHP 7

I'm sorry if I reported it in the wrong place.

[iandunn]

I was able to reproduce this. It looks like an edge case that we missed in r42016.

[iandunn]

Unit test for comment:41

[iandunn]

Fix case from comment:41, add more tests, 2 of the new tests still failing

[soulseekah]

Based on 41083.8.diff, fails on bad characters

[soulseekah]

Improved on tests from 41083.8.diff by adding a couple of uppercase inputs to make sure case-insensitive preg_* are used

[soulseekah]

@soulseekah
12 days ago

[iandunn]

In 42968:

Dashboard: Strip more extraneous IP parts to prevent PHP warnings.

This iterates on earlier versions of the code, in order to handle more edge cases. An arbitrary string like or=\" will now be stripped, as well as reachability scopes like %eth0.

Props eamax, soulseekah, iandunn.
Fixes #41083.

[iandunn]

Thanks @soulseekah :)