Page MenuHomePhabricator

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

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

Details

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>

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Sep 5 2019, 11:26 AM
epriestley requested review of this revision.Sep 5 2019, 11:28 AM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 5 2019, 11:30 AM
This revision was automatically updated to reflect the committed changes.