Page MenuHomePhabricator

Current implementations of "X-Forwarded-For" may take the wrong element
Closed, ResolvedPublic

Description

See https://discourse.phabricator-community.org/t/example-preamble-snippet-picks-wrong-entry-as-client-ip-from-comma-separated-list/3066.

We may be selecting the wrong element from X-Forwarded-For lists, and getting a good-enough result because it's rare that this list contains more than one element. If the element selection is incorrect, several things should be updated.

Event Timeline

epriestley triaged this task as Normal priority.Aug 29 2019, 2:42 PM
epriestley created this task.

Our behavior appears to be correct when the load balancer is an AWS ELB.

If I make a request with the header X-Forwarded-For: AAAA, BBBB, the host receives a request with the header like X-Forwarded-For: AAAA, BBBB, 1.2.3.4, where 1.2.3.4 is the my actual address.

This is actually consistent with the documentation referenced in the linked thread, I think the reporter actually needs to take the second-to-last element.

So I think we're largely good here, but support could be improved by loading a small library before loading preamble.php and providing something like preamble_trust_x_forwarded_for_http_header(N = 1), where N is the number of load balancers to trust.