ip_address function doesn't detect the correct IP
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | won't fix |
In a scenario where Caching mode is set to Normal and the Drupal instance is behind a proxy ip_address function reports the Proxy server's IP address.
Setup:
| Client | --> | Proxy | --> | Origin |
I did not test in the setup where Caching mode is disabled or set to Aggressive.
In the ip_address function, declaring $ip_address as static and then testing whether it is already set passes at all times and thus no attempt is made to find the client's IP.
In the patch I have introduced a new optional parameter $force that will force the function to reset $ip_address and hence calculate the client's IP. In index.php ip_address with $force set to true will be calledl Irrespective of any cache the ip will be re-calculated at every request, hence getting the correct client IP.
| Attachment | Size |
|---|---|
| proxy.patch | 1.59 KB |
#1
I read your blog post at http://blog.mohanjith.net/2008/02/running-drupal-behind-reverse-proxy.html, which had a link to this issue. I don't have a configuration where I can test this, but note that there are a few very minor code style violations which will have to be addressed (namely, indentation and periods at the end of comments). There may be others as well -- see http://drupal.org/coding-standards.
#2
keith.smith thanks for having a look at the patch.
I have read the coding standards documentation again and made the necessary changes to be compliant with the Drupal coding style. Hope I haven't looked over any other changes necessary.
Rerolled the patch with the changes against the latest HEAD.
#3
There are a few points that still need to be changed.
1.)
Do what ever tois vague and confusing. I guess what you are doing isReset the cached client IP address.2.) The description of the
$forceparameter is indented one space too much - compare it with the line below@return.3.)
$forceis not descriptive. As far as I know, other functions with a resetting parameter name it$resetor$refresh.4.) The
ip_addressfunction claims it is resets the static variable because that fixes a problem. Not true - it resets it because it was told to by a parameter. In line with the causality, the explanation of why this is needed should go to the function call ofip_address(TRUE);.5.)
trueshould be capitalized according to code-style, I think.#4
Here is a revised patch that corrects the above points.
Also,
<?php
static $ip_address = NULL;
if ($reset) {
$ip_address = NULL;
}
if (!isset($ip_address)) {
?>
can be rewritten to
<?php
static $ip_address = NULL;
if (!isset($ip_address) || $reset) {
?>
#5
The definition of the function still refers to
$force. Should this be now$reset?function ip_address($reset = FALSE) { ... }Paddy.
#6
Right.
#7
I'm certainly not an expert in reverse proxying, neither can I test this right now.
Still it seems to me the place where $ip_address is forcedly recalculated is not the right one. mohanjith is doing this after a full bootstrap, while in fact the recalculated IP address is already needed in DRUPAL_BOOTSTRAP_ACCESS.
I moved it there, corrected codestyle and improved the documentation.
Tested this with my means, so this certainly doesn't break environments without a reverse proxy. Otherwise, this remains to be tested.
edit: Unfortunately, I've been crossing all the posts 2-6. The idea remains to be valid.
#8
Another version of #7, reflecting the contributions #2-#6.
#9
How is a static cached in between requests?
The only reason I can see is that ip_address is called the first time before the variables (for variable_get) have been loaded.
#10
I think this issue is bogus.
I've been discussing this with chx:
Despite variable_init not beeing called until DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE, the config option for the ip option is already read from settings.php in DRUPAL_BOOTSTRAP_CONFIGURATION in conf_init.
Won't fix.