Opened 12 years ago

Closed 10 years ago

Last modified 10 years ago

#3872 closed (fixed)

Bug in SetRemoteAddrFromForwardedFor middleware

Reported by: Simon Willison Owned by: Grzegorz Ślusarek
Component: Core (Other) Version: master
Severity: Keywords: middleware
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


The middleware contains the following:

# HTTP_X_FORWARDED_FOR can be a comma-separated list of IPs.
# Take just the first one.
real_ip = real_ip.split(",")[0]

I'm pretty sure it should be taking the LAST element in the list, not the first - at least going by Bob Ippolito's description here:

This could be a security issue, as it may allow people to forge an X-Forwarded-For header and provide a fake IP address to Django.

Attachments (2)

SetRemoteAddrFromForwardedFor.diff (665 bytes) - added by Grzegorz Ślusarek 12 years ago.
patch against revision 6213
patch.git.diff (525 bytes) - added by John Moylan 10 years ago.
Patch to use second last IP address in list as real_ip.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 12 years ago by Gary Wilson <gary.wilson@…>

Triage Stage: UnreviewedAccepted

Marking accepted unless someone can prove otherwise.

comment:2 Changed 12 years ago by Grzegorz Ślusarek

Owner: changed from nobody to Grzegorz Ślusarek
Status: newassigned

Changed 12 years ago by Grzegorz Ślusarek

patch against revision 6213

comment:3 Changed 12 years ago by Grzegorz Ślusarek

Has patch: set

comment:4 Changed 12 years ago by Grzegorz Ślusarek

Triage Stage: AcceptedReady for checkin

comment:5 Changed 12 years ago by Adrian Holovaty

Resolution: fixed
Status: assignedclosed

(In [6364]) Fixed #3872 -- Fixed incorrect handling of HTTP_X_FORWARDED_FOR in SetRemoteAddrFromForwardedFor. Thanks, Simon Willison and gregorth

comment:6 Changed 12 years ago by Chris Bennett <chrisrbennett@…>

Resolution: fixed
Status: closedreopened

Wait a minute... client IS the first IP listed.

And I'm looking at request on a machine being proxied by Apache right now:
HTTP_X_FORWARDED_FOR: 66.162.32.x,

Also, some reverse proxies may pass X-Forwarded-For and its capitalization variants in place of HTTP_X_FORWARDED_FOR.

Though, in either case, the origin client is clearly first. Revert?

comment:7 Changed 12 years ago by Jacob

Resolution: fixed
Status: reopenedclosed

(In [6397]) Fixed #3872, which turns out to not have been a bug in the first place, by reverting [6364].

comment:8 Changed 12 years ago by leosoto <leo.soto@…>

comment:9 Changed 10 years ago by John Moylan

Resolution: fixed
Status: closedreopened

Hi, I'm using Apache and Django with Squid as a reverse proxy. Squid appends it's IP to the "END" of the X_FORWARDED_FOR header. The real ip shoudl be the second last in the list.


'HTTP_X_FORWARDED_FOR': ',,', is Squid

In this sort of setup the SetRemoteAddrFromForwardedFor should use that second last IP as the real IP

real_ip = real_ip.split(",")[-2].strip()

and not the current

real_ip = real_ip.split(",")[0].strip()

Having real_ip as the first IP in the list assumes that the client has not gone through multiple proxies.

Changed 10 years ago by John Moylan

Attachment: patch.git.diff added

Patch to use second last IP address in list as real_ip.

comment:10 Changed 10 years ago by Malcolm Tredinnick

Resolution: fixed
Status: reopenedclosed

Your patch doesn't generalise. What if there's another such proxy in the middle. Then you have to look at the third-last address. And so on. There's no standards here and no way to work it out. So we do the best we can and no better. Going to reclose this, since, as indicated by the various changes, there are conflicting setups.

Ultimately, if what is there doesn't work for you, don't use it. If there's some circumstance where Django won't operate with this feature (i.e. it's not ignorable, as opposed to no useful), then that would be a bug and a new ticket would be appropriate.

comment:11 Changed 10 years ago by John Moylan

Maybe the issue is a documentation problem. I had mistakenly assumed that this middleware was for use when you have a django app sitting behind a reverse proxy. In such a settup you have to use the X-Forwarded-For last IP that is before your own reverse proxy ip as the remote-ip.

If you want to try and get the original Client IP - real_ip.split(",")[0].strip() then the current code is probably a good attempt but in real world situations may not be as useful as just getting the last proxy IP address because it will be more prone to issues with proxy anonomizers or caches set up to strip X-Forwarded-For .

Note: See TracTickets for help on using tickets.
Back to Top