Fixed in git, thanks for the catch.

Close, but no cigar. The_ipIsUnique function will return false at all 
times. See attached patch.

Changes have been made in Git for this ticket:

Add method to check validity/uniqueness of IP address before using.
Bug: 10857

  2 files changed, 42 insertions(+), 3 deletions(-)
http://git.horde.org/horde-git/-/commit/c79abee4a9349b6fb0035a5d180b6a64a69f6d29

Changes have been made in Git for this ticket:

Bug: 10857 Actually use the ip address we pass into the method.
Signed-off-by: Michael J Rubinsky <mrubinsk@horde.org>

  1 files changed, 1 insertions(+), 1 deletions(-)
http://git.horde.org/horde-git/-/commit/a8e5006d4eb19ec694004b6cf51911ad25f54e0c

I agree. I already rewrote this to not depend on an external package:

     protected function _getLocationByIp($ip)
     {
         if (empty($ip)) {
             throw new Horde_Service_Weather_Exception('Error, trying 
to lookup empty IP.');
         }
         // Check if the provided IP is from a reserved range of IPv4 addresses
         // that should never appear on the public internet (see RFC3330) and
         // for which no geolookup will ever be possible. In this 
case, fallback
         // to the server IP as a last resort (which is better than 
nothing, but
         // not much).
         if ((sscanf($ip, "%d.%d.%d.%d", $a, $b, $c, $d) === 4) &&
            (($a === 10) ||                          // 10/8       - 
Private-Use Networks
             ($a === 127) ||                         // 127/8      - Loopback
             ($a === 169 && $b === 254) ||           // 169.254/16 - Link Local
             ($a === 172 && $b >= 16 && $b <= 31) || // 172.16/12  - 
Private-Use Networks
             ($a === 192 && $b === 0 && $c === 2) || // 192.0.2/24 - Test-Net
             ($a === 192 && $b === 168))) {          // 192.168/16 - 
Private-Use Networks
             Horde::logMessage('Using server IP instead of client IP 
[' . $ip . '] (not globally unique)');
             return $this->_makeRequest(self::API_URL . '/api/' . 
$this->_apiKey
                 . '/geolookup/q/autoip.json');
         } else {
             return $this->_makeRequest(self::API_URL . '/api/' . 
$this->_apiKey
                 . '/geolookup/q/autoip.json?geo_ip=' . $ip);
         }
     }

This indeed is simple enough. Besides the fact that Net_CheckIP2 is 
broken when it comes to checking the Link Local addresses and it also 
doesn't include the Loopback address. I'll prepare a patch later today.

I'd actually rather not add another, even optional, external 
dependency for something so simple. The code to check the validity of 
the IP address is pretty straightforward, so I'll just add this check 
directly to the Service_Weather package.

This is for the TImeobjects calls, obviously - for the Block, we use 
the Block's location setting first.

This is why the IP look-up is a last resort, after both the 
Identities' location pref, and Turba's My Contact is checked.

I know the 'require_once' in the patches is against the Horde coding 
standards and should really be done through the Horde_Autoloader. 
Unfortunately, I'm not that fluent in PHP that I'm able to figure out 
how to do this... :-(

The attached patch will check the IP address that is passed in 
_getLocationByIp. If the address is found to be local, use the server 
IP address to determine the location, otherwise use the client IP 
address. Most of the time, client and server will be at the same 
location when the connection does is local. This is not perfect, but 
it is the best we can do.

For me the geolookup based on client/server IP is not very useful by 
the way. If a result is found, many times the location is so far off, 
that it is not useable anyway (in The Netherlands, many lookups result 
in a location near the AMS-IX, which is *way* off in many parts of the 
country).

I wouldn't be too heartbroken if the WeatherUnderground driver 
wouldn't use this at all, the locations are too far off to be useful 
here.

Hmmm, I just noticed that the IP can only be from RFC1918 or RFC3330 
address space when the client and the (web)server are connected 
locally. This means that in this case we don't have to give up, but 
rather should use the (web)server public IP instead.

Rough example of how to weed out RFC1918 and RFC3330 IP addresses. 
This probably could (should) be improved to not bluntly require 
Net/CheckIP2, but rather use it when it is present and skip the check 
if it isn't.

When searching for the location by IP, the WeatherUnderground driver 
omits passing the client IP that is passed in _getLocationByIp. 
Although this is valid (in terms of the Wunderground API), this will 
cause the lookup to be based on the server IP, which might not be 
where the client is located. The attached patch fixes this.

Another thing that pops up after applying this patch, is that the 
driver will happily attempt to do geolookups for IP addresses from 
reserved (RFC1918) or ZeroConf (RFC3330) address space that are known 
to fail. Although this will be dealt with cleanly by Wunderground, we 
could save the cost of looking this up by checking this beforehand 
(through Net/CheckIP2 or someting similar). But this would probably 
need to be done in timeobjects/lib/Driver/Weather.php, where this 
might be handled for all drivers.