Make it easier to parse "X-Forwarded-For" with one or more load balancers
ClosedPublic
Actions

Authored by epriestley on Sep 5 2019, 11:26 AM.

Details

Reviewers: None
Maniphest Tasks: T13392: Current implementations of "X-Forwarded-For" may take the wrong element
Commits: rPadc2002d2870: Make it easier to parse "X-Forwarded-For" with one or more load balancers

Summary

Fixes T13392. If you have 17 load balancers in sequence, Phabricator will receive requests with at least 17 "X-Forwarded-For" components in the header.

We want to select the 17th-from-last element, since prior elements are not trustworthy.

This currently isn't very easy/obvious, and you have to add a kind of sketchy piece of custom code to preamble.php to do any "X-Forwarded-For" parsing. Make handling this correctly easier.

Test Plan

Ran unit tests.
Configured my local preamble.php to call preamble_trust_x_forwarded_for_header(4), then made /debug/ dump the header and the final value of REMOTE_ADDR.

$ curl http://local.phacility.com/debug/
<pre>

HTTP_X_FORWARDED_FOR =
   FINAL REMOTE_ADDR = 127.0.0.1
</pre>

$ curl -H 'X-Forwarded-For: 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/
<pre>

HTTP_X_FORWARDED_FOR = 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6
   FINAL REMOTE_ADDR = 3.3.3.3
</pre>

$ curl -H 'X-Forwarded-For: 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/
<pre>

HTTP_X_FORWARDED_FOR = 5.5.5.5, 6.6.6.6
   FINAL REMOTE_ADDR = 5.5.5.5
</pre>