[drupal-devel] [feature] Replace $_SERVER["REMOTE_ADDR"] with a function which understands X-Forwarded-For

[Steven]

Issue status update for http://drupal.org/node/20471

 Project:      Drupal
 Version:      4.6.0
 Component:    base system
 Category:     feature requests
 Priority:     normal
 Assigned to:  degerrit
 Reported by:  degerrit
 Updated by:   Steven
 Status:       patch

In Drupal we tend to store the original information in the db, escaping
or formatting on output as needed. All places where the hostname is
output should be okay after some recent changes.




Steven



Previous comments:
------------------------------------------------------------------------

April 14, 2005 - 01:42 : degerrit

In several modules, replace $_SERVER["REMOTE_ADDR"] with a function
which understands X-Forwarded-For.


Proxy servers often (sometimes, always?) insert "X-Forwarded-For"
headers which contains the "real" client IP address. I think it is
useful to log the client IP address in that situation.


This little snippet works for me, and I'd suggest something similar be
made into a function?


$headers = apache_request_headers();
if (array_key_exists('X-Forwarded-For', $headers)){
  $hostname=$headers['X-Forwarded-For'] . ' via ' .
$_SERVER["REMOTE_ADDR"];
} else {
  $hostname=$_SERVER["REMOTE_ADDR"];
}


I can make a patch proposal if people like the idea.




------------------------------------------------------------------------

May 21, 2005 - 23:44 : degerrit

Attachment: http://drupal.org/files/issues/remote_addr.diff (6.07 KB)

Here's my proposal for showing user IP addresses behind proxy servers,
especially in log files. It adds a function remote_addr() and replaces
all the uses of $_SERVER["REMOTE_ADDR"] with that function.


This is my first patch for Drupal, don't be too harsh one me :-)


A few notes:
- I patched session.inc because $_SERVER["REMOTE_ADDR"] is also called
in session.inc and common.inc is not available at that time it seems
- maybe I replaced too vigorously in common.inc and sessions.inc, it
works for me though
- maybe it should be renamed http_remote_addr or apache_remote_addr




------------------------------------------------------------------------

May 22, 2005 - 10:37 : Dries

Couple comments:



* Drupal supports non-Apache webservers: apache_request_headers() might
not always be available.
* Does this patch need input checking?  Depends on how/when/where the
remote address data is used.  Fact is that we don't want people to
inject XSS-stuff through HTTP headers.  Make sure to double-check this.
* I'd rename the function to remote_address().  We usually don't
abbreviate words like 'address'.
* The word 'via' can't be translated.
* We write
}
else { and not 


} else {* Just thinking up loud.  Is it easy to fake or spoof
X-Forwarded-For-headers?  If it is, it would be easy to trick Drupal
(eg. in the statistics.module's log pages we group by remote address). 
For the watchdog it doesn't matter, but for the statistics module it
does.  Maybe we need a toggle so remote_address() can be told not to
return the 'true IP address'.



------------------------------------------------------------------------

May 25, 2005 - 06:37 : Steven

X-Forwarded-For can be spoofed without any trouble. Just connect and
send the fake header. I don't think we should pay attention to it. If
we do, we need to also store the original IP address.


Perhaps we could do "real.ip.#.# (forwarded.ip.#.#)"




------------------------------------------------------------------------

May 25, 2005 - 06:39 : Steven

Sorry didn't look at the patch carefully enough. Yes, we need XSS
protection: in practice, we always strore remote_addr() in the hostname
column in the db. Wherever that column is displayed, it needs to be
check_plain()ed.




------------------------------------------------------------------------

May 27, 2005 - 00:58 : degerrit

I will re-write the patch with Dries' suggestions, I haven't had much
time this week. I found some interesting examples of scripts doing
roughly the same, none of them seem to do input checking (which I
hadn't thought of either, but is obviously very needed!). Good thing
there are people like Dries around, eh! 


There's also another possible header: HTTP_CLIENT_IP, I've never seen
it but then I don't log all my clients' headers either ;-)



* http://be2.php.net/getenv (the 2nd comment at this time)
* http://php.planetmirror.com/manual/en/function.getallheaders.php
* http://ip-to-country.webhosting.info/node/view/212
*
http://dev.wp-plugins.org/file/wp-contact-form/trunk/wp-contactform.php?rev=1209