• You visit www.oscms-summit.org
  • Before you gets redirected to 2007.oscms-summit.org you get a cookie. We will call this cookie G for general as it is general .oscms-summit.org domain. The redirect happens from PHP as a PHP sessionid is generated for and its value is sent in cookie G.
  • The browser performs the redirect to 2007.oscms-summit.org and sends the cookie G along with this request.
  • You type your username and password and click log in. Along the request, cookie G gets sent again.
  • login happens. user table gets update and sess_regenerate runs. Here:
    • a new session id is generated, and I will call this S because it's for 2007.oscms-summit.org which is more specific than the generic one.
    • A setcookie command runs -- this wants to invalidate cookie G which is right, but it does wrong because according to RFC 2109:
      Max-Age=delta-seconds

      Optional. The Max-Age attribute defines the lifetime of the cookie, in seconds. The delta-seconds value is a decimal non-negative integer. After delta-seconds seconds elapse, the client should discard the cookie. A value of zero means the cookie should be discarded immediately.

      we set a negative Max-Age which is undefined This is why the issue is so elusive. And also because you need a redirect to get two cookies.

    • The session gets moved from the old session id to the new. The old session id is stored in cookie G and the new is in cookie S.
  • form API redirects
  • Cookie G is still sent, but that session has just been moved to S, so you are anonymous again. This also adds to elusiveness: the browser might choose S for what it like. As G Max-Age is set to an undefined value, the browser's behaviour here is undefined. So this step is why the crucial setcookie was introduced: we do not want cookie G but alas it got botched.

Solution is trivial, set max-age to 0.

CommentFileSizeAuthor
#12 cookie-domain_0.patch1.47 KBwebchick
#11 cookie-domain.patch1.48 KBwebchick
#8 cookie_domain_1.patch1.02 KBchx
#7 cookie_domain_0.patch1.02 KBchx
#6 cookie_domain.patch1.02 KBchx
sess_finally_we_know.patch641 byteschx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Further evidence that this is the correct usage of the function is in the php documentation of setcookie.

If set to 0, or omitted, the cookie will expire at the end of the session (when the browser closes).

That's the behaviour we are looking for so even if this isn't the end of the almighty bug its an approriate fix to this section of code. That said I think this should fix the problem.

+1

crunchywelch’s picture

crunchywelch CreditAttribution: crunchywelch commented

This works for me reliably on FF and Konquerer, although I was only able to recreate the bug with Konquerer. This is most likely due to FF dealing with the undefined cookie lifetime value one way, and Konquerer another. +1 for this.

Caleb G2’s picture

Caleb G2 CreditAttribution: Caleb G2 commented

+1. Apparently this effectively creates the same condition as setting session.cookie_lifetime to zero, which is what I had done on my sites to get rid of this issues successfully.

If this patch gets in doesn't it make the session.cookie_lifetime setting in settings.php irrelevant?

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Needs work

The old Netscape warns

The path and name must match exactly in order for the expiring cookie to replace the valid cookie. This requirement makes it difficult for anyone but the originator of a cookie to delete a cookie.

I bet that when you are a different domain, you are not taken for the originator. Crell said on IRC: So the problem is that Drupal lets you get anything at all on a domain other than the "real" one. It should be redirecting you before the session starts, htaccess or no.

Let's think on how we can fix that.

BTW. crunchywelch says he saw two cookies for the same domain, so that's fixed by his patch (ie. the setcookie command). But not two domains.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Active

One more: we do not set a negative max-age we are sending an old style cookie 

  Set-Cookie: PHPSESSID=fa6f11eb42dae2289d9462b0242bc59c; expires=Fri, 16 Feb 2007 17:42:00 GMT; path=/

because that's what PHP setcookie command does. This however, does only murk the issue even more.

So. We should check whether the domain we are on is the same as the $base_url if a $base_url is set and if not redirect before session_start. And kindly warn everyone to set $base_url if they do wacky redirects. Or drop automatic base_url altogether.

This needs a debate and actually there is nothing wrong with the setcookie so setting to active.

chx’s picture

chx CreditAttribution: chx commented
Status: Active » Needs review
FileSize
cookie_domain.patch1.02 KB

Actually there is an attempt in Drupal 5 now to set the cookie domain, but it's not doing the right thing, not everyone is on www, as we can see. Those who do complex setups for various subdomains (like drupal.org) should have the knowledge to set things up right (ie. redirect as drupal.org does and comment this code out).

chx’s picture

chx CreditAttribution: chx commented
FileSize
cookie_domain_0.patch1.02 KB

Actually there is an attempt in Drupal 5 now to set the cookie domain, but it's not doing the right thing, not everyone is on www, as we can see. Those who do complex setups for various subdomains (like drupal.org) should have the knowledge to set things up right (ie. redirect as drupal.org does and comment this code out).

chx’s picture

chx CreditAttribution: chx commented
FileSize
cookie_domain_1.patch1.02 KB

I uploaded the wrong patch twice. Congratulations. Even if I read the patch before submit, I do not know which one lands on the machine because there is no preview :(

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Needs work

Neh. I am posting lots, sorry. My fix is wrong, the current guesswork is right but what's needed is a diatribe in settings.php which sums up that you need to set a common session cookie domain . I leave writing rants to others, I did enough here :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

chx: webchick: please add a polished version of "foo.example.com can't clear cookies for .example.com so if you have both then set the session cookie domain to .example.com . Note that the code below does this for www.example.com"
chx: to settings.php above the example code
chx: also check whether 4.7 has this

Just jotting this down so I can get a patch rolled a bit later.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs work » Needs review
FileSize
cookie-domain.patch1.48 KB

The wording for this actually comes from Crell, I just patch-ified it. :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
FileSize
cookie-domain_0.patch1.47 KB

Small fix; we don't put double spaces between sentences.

chx’s picture

chx CreditAttribution: chx commented
Title: Elusive login issue » PHP bug causes elusive login issue
Status: Needs review » Reviewed & tested by the community

Yes, this is the best we can do.

So, to sum up: the problem is that when there are two cookies from two different domains and we want to invalidate, we can't because we are not the originator. This fix covers that problem. Note that the PHP bug linked from session.inc http://bugs.php.net/bug.php?id=32802 is titled General cookie overrides more specific cookie (path) -- well it seems it's not just the (path) but domains as well!

It's not impossible that there are other login problems -- so far we catched two: one, where one domain had two cookies, this is the current

chx’s picture

chx CreditAttribution: chx commented
Title: PHP bug causes elusive login issue » Elusive login issue

Resetting title. I am not 100% it's PHP -- I will try to sniff myself later.

Dries’s picture

Dries CreditAttribution: Dries commented

Committed to CVS HEAD. Thanks.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

So then this should be committed to 5.x as well, right?

Is it still RTBC for that?

chx’s picture

chx CreditAttribution: chx commented

yes

BioALIEN’s picture

BioALIEN CreditAttribution: BioALIEN commented

Can we please have this committed and ready for a 5.2 release?

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

.. I meant 5

Caleb G2’s picture

Caleb G2 CreditAttribution: Caleb G2 commented
Status: Fixed » Reviewed & tested by the community

I hope this is not to presumptuous of me, please don't take my head off if it is, but I'm marking back as rtbc in hopes that it will be committed to the 4.7 branch (which committed the other recent patch involving login issues).

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 5.x-dev » 4.7.x-dev

If you're gonna do that, you should change the version too. :)

jacauc’s picture

jacauc CreditAttribution: jacauc commented

Can I apply the patch at #12 to Drupal 5.1?
Will this become drupal 5.2?

So that, if I apply the patch, I will technically have a 5.2 installation right? no other fixes going into 5.2?

Thanks!
jacauc

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

You can apply the patch in #12 to a 5.1 install, yes, but no, that definitely will not mean you have a 5.2 installation -- 5.2 will encompass all of the changes made to the DRUPAL-5 branch in CVS since the 5.1 release. There are usually dozens of such fixes. See http://drupal.org/node/108345 for an example of all the fixes that went in between RC1 and RC2.

jacauc’s picture

jacauc CreditAttribution: jacauc commented

Thanks! :D

BioALIEN’s picture

BioALIEN CreditAttribution: BioALIEN commented

Just to recap, this patch has been successfully committed to HEAD and 5.x we're just waiting for 4.7.x and then this issue is closed (unless someone wants to port it to 4.6.x).

If you're suffering from this bug - just wait for the next release of the Drupal branch you're using and all will be resolved (hopefully).

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Um. This patch doesn't "fix" anything. It just adds documentation to settings.php and a line to comment out if you're having this problem.

There's no reason to wait for the next release if you're having this problem, just add the ini_set line referenced by the patch.

adrinux’s picture

adrinux CreditAttribution: adrinux commented

Just to confirm this approach fixes a login problem with both 5.1 an 4.7.6 on my home development box. I couldn't get beyond the login page because no cookie was being set. I think the wording in settings.php might need expanded though.

In my case I have several development sites on subdomains:
site1.example.com:8090
site2.example.com:8090
site3.example.com:8090

In this case the actual domain is set via DynDNS, with each sub domain setup as a vhost in Apache, listening on port 8090.
From looking at the output from devel module:
sess_wrte: UPDATE sessions SET uid = 1, cache = 0, hostname = '192.168.0.1', etc

The hostname php is seeing is in fact the LAN IP address of my router, not even the IP of the machine the server is running on. I guess that's the problem with NAT :)

Setting the cookie domain manually, as per this patch, works:
ini_set('session.cookie_domain', '.site1.example.com');
Somehow it seems odd to do it this way, with the dot before the entire domain, but it works adn AFAIK it's to standard.

Hope this is helpful to someone.

Server info:
Apache/2.2.3 (Debian etch) PHP/5.2.0-8

romale’s picture

romale CreditAttribution: romale commented

Solution #28 also work for me. I've drupal 5.1, php 5.2, mysql 5.0 and DynDNS. After drupal installation I had "Access deny".

I added in my /srv/www/htdocs/drupal/sites/default/settings.php:
ini_set('session.cookie_domain', 'my.DynDNShostname.org');

and comment:
if (isset($_SERVER['HTTP_HOST'])) {
$domain = '.'. preg_replace('`^www.`', '', $_SERVER['HTTP_HOST']);
// Per RFC 2109, cookie domains must contain at least one dot other than the
// first. For hosts such as 'localhost', we don't set a cookie domain.
if (count(explode('.', $domain)) > 2) {
ini_set('session.cookie_domain', $domain);
}
}

no any patches.

Is it right solution?

adrinux’s picture

adrinux CreditAttribution: adrinux commented

The patch is further up this page romale! Comment#28 was just application of this patch and manual configuration of the cookie domain - which is exactly the solution the patch describes. My comment#28 was just adding information regarding DynDNS, NAT and routers, something I hadn't seen mentioned elsewhere.

killes@www.drop.org’s picture

killes@www.drop.org CreditAttribution: killes@www.drop.org commented
Status: Reviewed & tested by the community » Fixed

applied to 4.7

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)
terotil’s picture

terotil CreditAttribution: terotil commented

Somehow it seems odd to do it this way, with the dot before the entire domain, but it works adn AFAIK it's to standard.

Cookie RFC says it

An explicitly specified [cookie] domain must always start with a dot.

adrinux might just have this issue too http://drupal.org/node/126340. In some situations port number ends up in cookie domain, which breaks cookies and they will never be sent back to server and so all the cookies fail and thus login fails.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented

Those interested in a follow-up, the most recent patch for #56357 significantly modifies the instructions added to settings.php by this issue.