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.

Comments

keith.smith’s picture

Status: Needs review » Needs work

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.

mohanjith’s picture

Status: Needs work » Needs review
StatusFileSize
new1.61 KB

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.

cburschka’s picture

Status: Needs review » Needs work

There are a few points that still need to be changed.

1.) Do what ever to is vague and confusing. I guess what you are doing is Reset the cached client IP address.

2.) The description of the $force parameter is indented one space too much - compare it with the line below @return.

3.) $force is not descriptive. As far as I know, other functions with a resetting parameter name it $reset or $refresh.

4.) The ip_address function 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 of ip_address(TRUE);.

5.) true should be capitalized according to code-style, I think.

cburschka’s picture

Status: Needs work » Needs review
StatusFileSize
new1.58 KB

Here is a revised patch that corrects the above points.

Also,

  static $ip_address = NULL;
 
  if ($reset) {
    $ip_address = NULL;
  }

  if (!isset($ip_address)) {

can be rewritten to

  static $ip_address = NULL;
 
  if (!isset($ip_address) || $reset) {
paddy_deburca’s picture

The definition of the function still refers to $force. Should this be now $reset ?

function ip_address($reset = FALSE) { ... }

Paddy.

cburschka’s picture

StatusFileSize
new1.58 KB

Right.

pancho’s picture

StatusFileSize
new1.79 KB

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.

pancho’s picture

StatusFileSize
new1.96 KB

Another version of #7, reflecting the contributions #2-#6.

heine’s picture

Status: Needs review » Needs work

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.

killes@www.drop.org’s picture

Status: Needs work » Closed (won't fix)

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.

neilotoole’s picture

I ran into the issue as well, but killes is correct in stating that it's bogus. For the benefit of anybody else who might be scratching their heads: the reason I ran into this issue is because I set the 'reverse_proxy' and 'reverse_proxy_addresses' variables in the database (it was difficult to get access to the file system on the server). However, since the first invocation of ip_address() occurs in the bootstrap before the call to variable_init() (which actually reads the contents of the variables db table), the ip_address() function doesn't have access to any of the database variables. It only has access to values set on the global $conf object, which you can configure in.... your settings.php file.

In short, you can only configure Reverse Proxy settings from settings.php, NOT via the variables database table.