Getting the HTTP client's IP

Question

Can you please tell me whether this is a good way to get the user's IP (IPv4 or IPv6)? Does someone have a better way to do this? Please take a look at the entire code, the server_params order and the comments.

/* We are looking for ip in server params */
$server_params = array('HTTP_CLIENT_IP', 'HTTP_X_FORWARDED_FOR', 'REMOTE_ADDR');
foreach($server_params as $server_param)
{
 /* If server param exists and his value is a real ip */
 if(isset($_SERVER[$server_param]) && filter_var($_SERVER[$server_param], FILTER_VALIDATE_IP))
 {
 /* If we are here, then means that the ip is whether IPV4 or IPV6 */
 /* This means that we can set user_ip as binary - We will insert this ip into an mysql column - VARBINARY (16) */
 define('user_ip',inet_pton($_SERVER[$server_param]));
 break;
 }
}
/* If the user_ip is not defined or the inet_pton failed (can inet_pton fail if is defined ?) */
if(!defined('user_ip') || user_ip === false) { echo 'Error. Your ip is not valid'; exit(); }

• 200_success: Related Stack Overflow question

Answer 1

Here's what I would change :

• The answer to the question (see 200_success comment) states that the only thing that is hard to spoof is REMOTE_ADDR, so you should put this value first in your array.
• I'm not a big fan of your double test (isset and filter_var). I would like to trust that the implementation of filter_var is a good one and that it does test the emptyness of your variable before going any futher, but I'm not sure. Anyway, I'll just keep the test with filter_var .
• Not a big fan of your define either, why do you want to use a constant ? I would initialize a variable userIP before the foreach to false, so there's only one test to do after the loop.

One thing that worries me a bit is the existence of a case where filter_var would succeed but inet_pton would fail (for the same value). It should not exist, but if it does, you might exit your loop with userIP equal to false, while there is one valid IP in one of the following server variables. So just to be sure, I would add a test on inet_pton before breaking out of the loop.