Thanks for the detailed response.

> That looks like (if X-Forwarded-For is “valid”) your jumping straight

> to considering the X-Forwarded-For header. You might want to check

> beforehand whether the client ip is a “trusted” proxy. And only if it

> is, you should walk the X-Forwarded-For.

> Also, it sounds a bit like you're going through X-Forwarded-For from

> left to right. Make sure to walk it from right to left, as proxies are

> expected to append (not prepend) the client IP they forward for.

Okay. Assuming we append client_ip to X-Forwarded-For and process the list from right-to-left, do you still think we should explicitly check whether client_ip is a trusted proxy? Because, per the modified algorithm, the client_ip will be the first item to be processed and if it is not a trusted proxy, it will be returned as the result - which is what even the explicit initial check will result in this case as well. Correct?

> The way you combine the looping and if looks like you might be

> skipping across entries that you cannot parse.

> But if you find invalid entries while backtracking the IPs, you're

> probably in parts of the X-Forwarded-For value that you shouldn't trust.

> (X-Forwarded-For can easily be spoofed by clients)

We only check that each entry is a valid IPv4 or IPv6 address and not just a random string. If we find invalid entries, then the only option we have is to continue the search further for a valid IP.

>  list_of_ips = append client_ip to X-Forwarded-For

>   for ip in reverse( list_of_ips )

>   do

>       if ip is not a trusted proxy or the iterator does not have more elements

>       then

>           return ip

>       fi

>   done

> (That should give invalid IP addresses for some requests. That

> seems to be the correct thing from my point of view. But if you rather

> geolocate wrong than not geolocate at all, you can choose to pick the

> last known good IP address instead. I guess that's more a matter of

> taste.)

What would picking the last known good IP address in this context? An IP that matches the IPv4/IPv6 pattern _AND_ is not a trusted proxy?

> Different stakeholders at the WMF use different lists of what kind of

> proxies they consider. So things are not really clear cut.

Compiling a common list - may be union of "General", "MediaWiki" and "Wikipedia Zero" - would be very useful. As Oliver suggests, it would be good to have them in site-wide config files.

Thanks,

Ananth