Issues with cookie domain behind a reverse proxy

[michael-e]

If Symphony runs behind a reverse proxy, Symphony's server will probably listen to 127.0.0.1. In this case the getDomain function in lib/toolkit/class.session.php will cause issues:

public static function getDomain()
{
    if (isset($_SERVER['HTTP_HOST'])) {
        if (preg_match('/(localhost|127\.0\.0\.1)/', $_SERVER['HTTP_HOST']) || $_SERVER['SERVER_ADDR'] == '127.0.0.1') {
            return null; // prevent problems on local setups
        }
        return preg_replace('/(^www\.|:\d+$)/i', null, $_SERVER['HTTP_HOST']);
    }
    return null;
}

Checking for $_SERVER['SERVER_ADDR'] == '127.0.0.1' is no good idea in this case, because it will prevent Symphony from returning (and properly setting) the correct cookie domain.

I don't remember why exactly we needed that "prevent problems on local setups" logic, but there must have been issues with certain browsers on certain "localhost" setups. Nevertheless, looking forward, I suggest to remove at least the mentioned part of the if clause, because it prevents correct cookies in real-world scenarios, for example Symphony running on Apache behind nginx as frontend server (and SSL terminator, in my case).

(It really took me a while to find out what was causing my issues.)

[nitriques]

I had the same problem a month ago and had to hijack the DOMAIN constant to make it work.

I am not sure how to deal with this, but we sure must find a solution.

[michael-e]

My suggestion is to remove the problematic part || $_SERVER['SERVER_ADDR'] == '127.0.0.1') and see if anyone has problems. :-)

Honestly I don't remember which setups caused issues, and I hardly ever run Symphony locally (most of the time I am working on vServers, automatically rsyncing the local file system). But maybe @brendo knows more.

[nitriques]

Why can't the domain be 'localhost' ?

[michael-e]

I don't remember.

[nitriques]

Let's hope @brendo does.

But if not, I would not mind removing the || $_SERVER['SERVER_ADDR'] == '127.0.0.1') part.

[brendo]

I don't remember either, it's well before my time. It's not something to do with Windows is it?

[nitriques]

I would then do @michael-e's proposed change and see if anybody has problems with it.

[michael-e]

I can send a pull request if @brendo agrees with it.

[brendo]

Is there any other way to detect the reverse proxy? When this information is removed, I assume this will result in a cookie created for the domain 127.0.0.1? Does this trigger weird behaviour, or potentially leak the information out to other domains?

Perhaps no more so then 'null'?

[michael-e]

When this information is removed, I assume this will result in a cookie created for the domain 127.0.0.1?

A proxy should pass the Host header to the backend, so that should not happen. A minimum configuration for nginx, for example, would be:

location /some/path/ {
    proxy_set_header Host $host;
    proxy_set_header X-Real-IP $remote_addr;
    proxy_pass http://localhost:8000;
}

One might attempt to detect a frontend proxy using special headers like X-Real-IP or X-Forwarded-For, but that feels like a bad idea.

So instead I ask: Why do we need the second condition? Are we trying to cover cases where PHP's $_SERVER['HTTP_HOST'] is empty? Then, couldn't we just do the following?

if (!$_SERVER['HTTP_HOST'] || preg_match('/(localhost|127\.0\.0\.1)/', $_SERVER['HTTP_HOST'])) {
    return null; // prevent problems on local setups
}

[nitriques]

I do not want to start yet another debate on where to get server/env values...

But, we deal with potentially dangerous data all the time and need to address those cases ourself (sadly). I'be been doing quite a bit of system programming in C, and almost all system calls can return errors. There are no "safe" values.

That being said, having NO Host header is a violation of the HTTP/1.1 spec, but HTTP/1.0 allows it. HTTP/1.x is ready to be replaced with a new version and I hope it does the same thing

So, I would fix the case where case where PHP's $_SERVER['HTTP_HOST'] is empty? like so:

if (empty($_SERVER['HTTP_HOST'])) {
    header($_SERVER['SERVER_PROTOCOL'].' 400 Bad Request');
    exit;
}

[michael-e]

AFAIK, $_SERVER['HTTP_HOST']will always be set, so you shouldn't need empty to test it. It should be enough to write:

if (!$_SERVER['HTTP_HOST']) {

I agree with your point that any data sent by a client are potentially dangerous, so if we see potential issues we should fix them.

But if HTTP 1.0 allows the Host header to be emtpy, are you really sure that we should treat this as an error? (I am not sure, nor am I sure about the opposite.)

[nitriques]

are you really sure that we should treat this as an error?

The only HTTP/1.0 traffic I get on my servers are weird looking requests, i.e. hackers.

1.1 mandates host to be there and server should reject the request.

Maybe 400 is a bit arsh, maybe 412 Precondition Failed should be used.

I think disabling http/1.0 support would solve a lot of problems. But maybe that's something I need to do server-wise.

[michael-e]

Let's see what @brendo thinks.

[nitriques]

Yup :)

[brendo]

It's quite difficult to know exactly why this was introduced as it's not documented, and therefore, it's not easy to know what repercussions may result from removing it. Because of that, I'm hesitant to make any changes in a point release that we cannot easily verify.

Unfortunately, Cookies & Sessions have a notorious history with Symphony and any ill effects are usually widely and immediately felt. Whatever we do here, it has to be measured and tested carefully so to avoid a string of future point releases attempting to 'fix' it.

The existing code already has logic for dealing (rather crudely) with the lack of the HTTP_HOST header (it'll send null back), so, for the moment let's just remove the SERVER_ADDR condition and test. We should at minimum test this on Apache, nginx, Windows and Mac environments, with a mix of simulating local development and production code.

[michael-e]

I wouldn't mind the point release, because it's just a bugfix in my eyes. The SERVER_ADDR condition causes issues, and we don't remember if it ever did anything good. And if it was useful, it must have been a workaround for a bug in somebody's environment.

No wonder we don't remember, the code must be more than 6 years old. :-)

Normally I am the one to say "test, test test!". But in this case I think it's pretty hard, simply because we don't know which problems to expect.

[brendo]

Normally I am the one to say "test, test test!". But in this case I think it's pretty hard, simply because we don't know which problems to expect.

By test I mean verify it still works in the environments above. I wouldn't be comfortable in removing the condition and pushing the release out without this verification.

[michael-e]

Oh, everything seems to work as before

1. locally on the Mac (OS X 10.8, built-in Apache) and
2. on the server (Apache behind an nginx proxy, as described above).

I'd be happy if others could verify different setups.

[brendo]

Out of curiosity, what does the $_SERVER['REMOTE_ADDR'] value hold on your nginx proxy?

[michael-e]

PHP runs in Apache, and $_SERVER['REMOTE_ADDR'] is 127.0.0.1 there. (Apache only "sees" nginx).

[brendo]

Hmm, I'm looking at it now and what I think the purpose of the code is to prevent problems when you use local hostnames in development, and then the cookie will be shared on the 'live' site.

For example, if you have your development site setup to run as example.net using your hosts file, the $_SERVER['HTTP_HOST'] value will be example.net. If you then actually visit example.net, that cookie will be 'valid'.

There seems to be a lot of advice about never set the domain, and let PHP handle it as this results in more predictable behaviour with subdomains. Our little regex at the moment seems to handle the making the cookie available on all subdomains by stripping the www. and any port information from $_SERVER['HTTP_HOST']. Essentially, we currently force all cookies to be available across all subdomains. I believe if the regex doesn't run, PHP will just set the cookie for the current domain.

Setting the domain to 'www.example.com' will make the cookie available in the www subdomain and higher subdomains. Cookies available to a lower domain, such as 'example.com' will be available to higher subdomains, such as 'www.example.com'. Older browsers still implementing the deprecated » RFC 2109 may require a leading . to match all subdomains.

So now it's a weird one, if we remove the getDomain() function altogether and just return null, it's a breaking change as cookies will/may only work on the exact domain and 'higher' (as per quote). If we remove the SERVER_ADDR condition, developers may suddenly be getting cookies inadvertently set for unexpected domains.

Both scenarios risk breaking changes, and as such, I'm not comfortable pushing into a 2.6.x release.

[michael-e]

Completely removing the function has one disadvantage: We drop a place where we can "hack" things. For a long time I have been using a modified getDomain function, because I didn't like Symphony's behaviour (setting cookies for all subdomains only if you log in from the main domain — I needed this no matter where people log in, let it be a subdomain or the main domain.)

For example, if you have your development site setup to run as example.net using your hosts file

If a developer really does this (instead of using example.dev or similar), he hopefully knows what he does. And honestly, what's the problem with setting a cookie for this domain then? It's simply correct, isn't it?

Symphony shouldn't try to be clever here.

[nitriques]

If a developer really does this (instead of using example.dev or similar), he hopefully knows what he does.

Agreed.

Symphony shouldn't try to be clever here.

Agreed.

But lets hold up for a major release then. 3.x will break things anyways.

[michael-e]

Hmmm.

[michael-e]

Given the nature of the proposed fix (re-reading the above discussion I name it a "maybe breaking bugfix") I suggest to include it in Symphony 2.7.

I can send a PR if desired.

[nitriques]

I can send a PR if desired.

Yes please! I presume you've been running it for some time now ?

[michael-e]

I presume you've been running it for some time now

Yep, sure. It's one of the hacks I always add (i.e. rebase) on top of the current Symphony version.