In "includes/bootstrap.inc", function "ip_address":
Instead of
array_pop(explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']))
shouldn't it be
trim(array_pop(explode(',', $_SERVER['HTTP_X_FORWARDED_FOR'])))
because when the XFF header contains multiple addresses they are separated by ", ".
Same for D7.

CommentFileSizeAuthor
#4 309586_XFF_spaces.patch1.19 KBmr.baileys
#2 309586_XFF_spaces.patch685 bytesmr.baileys

Comments

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Version: 6.x-dev » 7.x-dev

No RFC, but according to wikipedia:

The general format of the header is:

X-Forwarded-For: client1, proxy1, proxy2

where the value is a comma+space separated list of IP addresses, the left-most being the farthest downstream client, and each successive proxy that passed the request adding the IP address where it received the request from. In this example, the request passed proxy1, proxy2 and proxy3 (proxy3 appears as remote address of the request).

So it indeed looks as if we should either explode using ', ' instead of ',' or wrap the array_pop in trim as per your suggestion.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Active » Needs review
Issue tags: +Quick fix, +XFF
StatusFileSize
new 309586_XFF_spaces.patch685 bytes

Patch.

dries’s picture

dries commented

Patch looks good. Maybe extend the code comment a bit? That wouldn't hurt.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
StatusFileSize
new 309586_XFF_spaces.patch1.19 KB

Rewrote the comment to provide some extra documentation/clarification.

dries’s picture

dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

teastburn85’s picture

teastburn85 commented

Why does ip_address() use the rightmost entry from X-Forwarded-For header? Isn't the purpose of ip_address() to get the end-user/client's IP address? The documentation for X-Forwarded-For on wikipedia seems to state that the client's IP is the first mentioned IP (leftmost) and any other servers that touch the request append their IPs to the end of the list (rightmost). Am I misunderstanding how X-Forwarded-For is used in this situation?

Selecting the rightmost IP is breaking our Drupal setup where our servers are behind a CDN that passes the client's IP as the first IP in the X-Forwarded-For header.

Also posted this question on http://drupal.org/node/258397 since they are newest issues I could find.

Thanks in advance