Login issues with multiple sites in the same domain (session cookie collision)

Adding a unique session name appears to solve this issue.

-K

.

Doesn't work for me in a multiple-level configuration. Steps to reproduce:

Configuration 1:
- 1 site at example.com/sub1
- 1 site at example.com/sub2/sub21
This works.

Configuration 2:
- 1 site at example.com/sub2/sub21
- 1 site at example.com/sub2/sub22
This works.

Configuration 3:
- 1 site at example.com/sub1
- 1 site at example.com/sub2/sub21
- 1 site at example.com/sub2/sub22

This doesn't work: logging to sub22 (resp. sub21) logs out of both sub1 and sub21 (respc sub22) and vice-versa. Sites have each their own independent database, so it can't be a prefix issue for the sessions table.

Maybe you could try to set a different session.name for each site in its own settings.php file. Something like this:

Otherwise, cookies visible from different sites will get overridden, if not correctly protected by browser security settings. You could also tweak this by setting different cookie domain/paths with (for configuration 3 above) something like:

Markus + fgm,

Thanks for your input. Can someone please explain why the session_regeneration creates this issue in the first place ? Could this perhaps be fixed there? chx noted that the downside of the above patch could be that admin sessions might be affected during the upcoming upgrade..

Thanks
-K

what most people want is to share their session cookie across sites. i think markus code makes each site distinct. if thats what you want, then no problem.

Why worked? Well, we need to see what session fixation is (I'll try be short as possible).

Session fixation is a vulnerability that allows anyone to fix anything for the session id, and PHP will just use that until it expires (the attcker could take care of keeping the session alive by generating requests at intervals). If you followed a link of the form http://example.com?PHPSESSID=vivalavida, PHP would just use that and when you log in, PHP would still use that session id, now logged in. So the attacker (the one who managed to make you follow that link) could then use that session id to impersonate you. Now, if you are the site admin, the attacker could do anything, even execute PHP code, read your database password, etc.etc.etc. more references

Before chx's session regeneration fix, Drupal had no defense against a session fixation attack. Hence, what was happening is that the first session opened on a Drupal site generated a session id, stored in the browser's cookies. That session id, was reused when the user visited the other sites. Let's say, Drupal let you deploying a session fixation attack.

Now, the session id is regenerated everytime the user logs in. That is, the session id changes and it is updated in the corresponding cookie, the same one that has been shared between several sites. The problem now is that that session id is only known to the database of the site where it was created. When the other sites read that cookie, get a value (a MD5 hash) that is probably unknown to them (different sessions database), hence they see that user as a guest.

No problem, if you try the tips above you can make each site look for its own cookie (the session.name value is the name of the cookie, PHPSESSID by default). Even if those cookies are visible from different sites, each will look at its own. Such a configuration, of course, opens a door to a possible attack, 'cause you could read from one site the other cookies, so you could get access to the other sites. That would be a session hijacking attack. So it would be best to correctly set the other session.cookie parameters, to take advantage of the browser security zones.

Thanks markus. Your configuration of yesterday 14:29 gives just what I needed right now: independent sessions on all sites.

Now if you had an idea to allow several sites like this to share a common session, that would be nice, but AIUI the same cookie would have to be present in the sessions table of each database, which would require additional changes to drupal, would it not ?

Glad it helped, fgm.

Now, IMHO, each site would have to get access to its cookies only, for privacy and security reasons.

In order to fix/close this issue, maybe it would sufice with a handbook page describing how to configure PHP settings related to session/cookies, specially, for multi-sites?

Thanks Markus. settings.php patch attached.

Please review and feel free to amend it if need be..

Cheers,
-K

Markus,

Yes I would think so. But before you ask, no I don't feel qualified enough to write it :-)

I would also add a line for session.cookie_secure, necessary under HTTPS.

Here are some notes:

Another thing one might consider is forcing the domain name to use or not to use the www. part, by means of a simple mod_rewrite trick (don't know for IIS, sorry). This may affect user sessions as well, depending on how PHP session/cookie settings are configured.

To force the www. part:

Oh, I wanted to mention something else...

session.auto_start ought to be disabled, but this is something that needs to be set in .htaccess files, or in httpd.conf or in php.ini. ok, Drupal sets this value in its own provided .htaccess file, however...

1) This makes 2 places where PHP settings related to sessions are defined (.htaccess and settings.php).

2) Many webmasters may have their own .htaccess file before installing Drupal, where they may define a lot more things such as hotlink prevention, additional URL redirections, access control based on IPs, etc. etc. etc., so they may need to merge Drupal provided rules with their own. Well, they may forgot to add the session.auto_start line, and it might be enabled by default in their server. This would lead to unexpected results and confusions.

Just food for thinking...

Hi Markus,

Can you please roll up a patch with whatever you recommend and concise comments?

-K

Zen, first and foremost this is a documentation issue, I have already forwarded the issue to the docs team. Of course we could add this and that to .htaccess but what we need most is a handbook from markus excellent remarks.

I can't get this setting to work for me. My site is http://ukfilm.org and I've tried '.ukfilm.org' and 'ukfilm.org' but both settings prevent users from logging on (the behaviour is as if the user has cookies disabled).

@dan_aka_jack: try without the dot. but if you do not have subdomain, you could just set cookie_domain blank. Clear all cookies and exit all browser windows, then open the browser and try. Also, try with different browsers. It could be a browser issue with the cookies.

Zen, first and foremost this is a documentation issue, I have already forwarded the issue to the docs team. Of course we could add this and that to .htaccess but what we need most is a handbook from markus excellent remarks.

Thanks. I agree, this is a doc issue. I hope there is enough info posted in this issue. Any draft already?

I have tried to compile some information about PHP session/cookies here:
http://www.phpmix.org/setting_up_php_sessions

@markus : thanks for the tip.

ok, here's another issue that is ending up pretty related to this one:
http://drupal.org/node/56634

Though that one introduces a mod_rewrite rules to enforce the domain to match the $base_url.

I am running Zen's original patch on my dev sites and they are no longer logging each other out. Perhaps there are configurations where it would not work but it will help for many sites, no? Maybe a 90% improvement is good enough.

This may be related: http://drupal.org/node/64054

This IMO needs addressing , let's decide which patch we would like to see in.

This can also have something to do with http://drupal.org/node/82662

@chx: what do you think that can be done about this?

This bug is more pernicious than originally thought. There’s a critical bug with an edge-case configuration.

Here’s a not-so-unusual situation:

If a user goes to a PHP-based site at http://example.com (it doesn’t even have to be running Drupal, just some PHP code that uses sessions), by default PHP sets a cookie named PHPSESSID for .example.com.

If that user then goes to a Drupal site at http://intranet.example.com and attempts to login, Drupal sets a cookie named PHPSESSID for .intranet.example.com with authenticated permissions. BUT, the user’s browser sends both the .example.com and the .intranet.example.com cookies back to the intranet Drupal site and when Drupal tries to access the $_COOKIE['PHPSESSID'] value via PHP’s session functions, it gets the .example.com value; the wrong session id!

From the user’s point of view, the user sees no “Unrecognized username or password” message (Drupal does enter a “session opened” watchdog entry), but because Drupal can’t access the correct cookie, the user keeps seeing the login screen. If the .example.com cookie is set to expire in 2037 (some PHP apps do that) and the user doesn’t think to delete cookies for a completely different web server, the user will NEVER be able to log into intranet.example.com!

From the cookie spec:

When sending cookies to a server, all cookies with a more specific path mapping should be sent before cookies with less specific path mappings. For example, a cookie "name1=foo" with a path mapping of "/" should be sent after a cookie "name1=foo2" with a path mapping of "/bar" if they are both to be sent.

Even though both cookies are submitted by the browser, PHP clobbers the value of the more-specific cookie with the less-specific cookie that follows. So there’s no way that Drupal could try to differentiate the two cookies.

I believe this is rightly a PHP bug and have submitted a bug report with PHP. But Drupal has to have a work-around. I verified this affects both 5.1 and 4.7.6.

The only workaround (that I see) is to have a unique session_name() for each Drupal install. I’ve used the session.cookie_domain value concatenated with the conf_path() value to provide a quick, easily-calculated, and unique session name. And to prevent the session name from being excessively long I’ve md5’ed it down to 32 characters; for example, the modest http://www.mydomainname.com/subsite could result in a non-md5’ed session name that is 47 characters long, “mydomainnamecomsites80wwwmydomainnamecomsubsite”.

Just noticed this was set to 6.x-dev.

To reiterate, this is a PHP bug; it affects ALL versions of Drupal, including 5.1 and 4.7.6. The patch above is the work-around which I've tested for Drupal 5.1 and 4.7.6.

It's best to mark any bugs to the latest development version. This ensures the most attention and once it is fixed in 6.x-dev, it will most likely be backported to any previous versions.

i like using a unique cookie name. that prevents plenty of odd behavior as you've seen ... do we really need such a complicated cookie name though? can the md5 be omitted? the $domain? i usually just use $base_url. see http://www.tejasa.com/node/117

Moshe, a complicated session name is not required, just an unique one.

I looked at your solution and did some timing tests: conf_init(); preg_replace("/[^a-z\d]/i", "", $base_url) is faster than md5($domain.conf_path()) and session_name() is faster than set_ini('session.name', 'val').

However, the recursive nature of calling conf_init() from within settings.php makes following the program flow difficult and could lead to some problems if an admin edits the settings.php and puts the $conf = … lines before the conf_init() line ( since conf_init() performs $conf = array();. )

So in this new patch, I've split conf_init() in two, separating the inclusion of settings.php from the clean-up/creation of $base_url. The new patch is faster, but bigger. Please review and discuss.

Isn't the cookie path the right way to solve this?

Hi Steven. No, a cookie path won’t help here. If Drupal is installed in the root directory of http://www.example.com/ and in the root directory of http://other.example.com/, the cookie path has to be the root for both session cookies.

We are mixing two things here. This issue is about running two sites on identical domains (but differing paths). Cookie paths are the right solution here afaik.

The other issue is the PHP cookie specificity bug.

Both need to be solved separately.

The two issues are just manifestations of the same problem: cookie collision. In both cases, the session cookie from one site is over-riding the session cookie for another site. Both would be solved by the above patch.

For the “two sites on identical domains (but differing paths)” half of the issue, cookie paths would indeed fix it, but that fix would be redundant if you include the path in the session_name().

If you would like me to separate the two items, I can update this issue and move the patch over to http://drupal.org/node/82662

I started to split these two items up, but then realized they are, indeed, a single issue.

The cookie spec (http://wp.netscape.com/newsref/std/cookie_spec.html) says that cookies are identified by the triple name/domain/path. And that browsers should send all cookies that match a request URL with the most-specific matches coming before the least-specific matches.

For example, suppose a user requested http://other.example.com/path1; if the browser had these cookies:

• domain=.example.com; path=/; PHPSESSID=sess1
• domain=.example.com; path=/path1; PHPSESSID=sess2
• domain=other.example.com; path=/; PHPSESSID=sess3
• domain=other.example.com; path=/path1; PHPSESSID=sess4

…the browser would send Cookie: PHPSESSID=sess4; PHPSESSID=sess3; PHPSESSID=sess2; PHPSESSID=sess1 (ordered from most-specific to least specific) and PHP would mistakenly setup the session using the sess1 value (the least-specific and wrong session value.)

As can be seen, changing the cookie path, will not fix the issue Steven describes. The only work-around is to give each session cookie an unique name that incorporates the domain and the path (i.e. the $base_url).

The new patch updates some comments and (eep) removes a tab.

JohnAlbin,

Thanks to you and everyone else who has contributed to this patch. I found it the other day. It has solved the login issue I was having with multisites that I had submitted here:

http://drupal.org/node/129329

Thanks again...my project moves forward!

I’m glad the patch helped you, Jeff.

Here’s a very slightly updated patch. It strips the http or https from the beginning of the session name and fixes the redundant /a-zA-Z/i regex I had.

I'm having the same problem

I’ve just finished talking with the PHP developers and it looks like it’s not a bug in PHP, but a bug in the cookie spec.

The cookie spec (RFC 2965 sec 3.3.4) states:

If multiple cookies satisfy the criteria above, they are ordered in the Cookie header such that those with more specific Path attributes precede those with less specific. Ordering with respect to other attributes (e.g., Domain) is unspecified.

It’s that final sentence that is the problem. The browser can put session cookies for domain=other.example.com;path=/ and domain=.example.com;path=/ in any order it feels like.

Indeed I tested IE7, Firefox 2, and Safari 2 and they all put the less-specific .example.com cookie before the other.example.com cookie!

The only solution is to specify an unique cookie domain for each Drupal configuration.

The patch above splits the conf_init() in two parts in order to get at the $base_url clean-up code from within the settings.php file. It looks a little messy, but I can’t think of another way to: 1. allow users to set $base_url (or not) within settings.php, 2. let Drupal to clean up the $base_url value, and 3. allow the settings.php to quickly specify the cookie_name().

I would very much like to see some solution to this problem included in 5.2.

John your patch in #38 works for me on both Drupal 5 and 4-7.

Thanks

Please bare in mind different port numbers if possible: http://drupal.org/node/126340

Please bare in mind different port numbers: http://drupal.org/node/126340

Amr, don’t worry. This patch uses $base_url, so it already addresses port numbers.

we've had a couple of people say that this patch works and fixed their ailing site. RTBC.

1.
+ // Don't call conf_base_url(), if it is called in settings.php.
+ if (!isset($base_root) || !isset($base_path)) {
+ conf_base_url();
+ }

Can't we always call conf_base_url() in settings.php? We could add a note to settings.php ('do not remove or move the following call'). We'd be able to clean up conf_init(), I think.

2. As settings.php is often read by mere mortals that don't know any programming, it is probably a good idea to extend the comments to show a concrete example. Non-technical users might be intimidated by the regex and would probably prefer a copy-pastable alternative.

Should be easy to get this committed if we can massage it a bit more. I think we can squeeze some extra performance out of it, as well as make it a bit more accessible for non-programmers.

Keep up the good work.

We’re trying to do too much in settings.php. My previous patch along with other recent changes to settings.php are making it too cluttered. Also, my previous patch would not work with the helpful hints and suggested code in #117917.

This new patch removes all the regex stuff from settings.php and unifies all the associated session-cookie-related code into conf_init() where it belongs, IMO. It leaves an expanded comment and optional $cookie_domain variable in settings.php, which is more in line with the previous, mere-mortal-friendly settings.php. ;-) Oh, and it also fixes the current issue.

How does this patch play with the recently committed ini_set('session.cookie_domain', '.example.com');?

Are they wholly separate, related, and/or an either/or relationship?

ini_set('session.cookie_domain', '.example.com'); has worked really well for me and many others and went through quite a review process. Just want to be sure things remain integrated and/or don't go backwards.

Caleb: Don’t worry. This patch changes ini_set('session.cookie_domain', '.example.com'); to a simpler $cookie_domain = 'example.com'; syntax. It doesn’t need the awkward period prefix. The ini_set('session.cookie_domain', …) function is still used in the background (in conf_init()) to set the actual cookie domain.

The patch merges all the code in the following issues: #108663, #117917, 127753, 131017, and the patch in #38 above.

I've tested the patch, tried to break it but it seems to be immune to breakage :-).

I thought the $session_name TRUE/FALSE logic was unneeded, so is some if() checks, so I went ahead and removed them and added some comments around. I hope it's a bit simpler now. of course I have tested before/after the changes and it's the same.

+1 for RTBC.

Amr, thanks for testing!

1. if (!empty($_SERVER['HTTP_HOST'])) is needed for E_ALL compliance. HTTP_HOST is not always set.
2. A complicated regex requires a detailed comment, so I’ve reverted that comment.
3. You are correct about the “$session_name TRUE/FALSE” and about one of the if statements.

This new patch adds Amr’s suggestions.

This looks OK to me. The only drawback is that we're executing this code for every page request, which may or maybe not add some unwanted overhead. Feel free to RTBC if others think this approach is best.

Dries, I don't think this patch adds overhead, the only thing that's not plain simple in the code is the couple of preg_replace() calls, one of them existed in settings.php before and this patch moved it inside conf_init()

Visual review only:

• Can't we avoid having to execute a majority of the code on every page request by moving domain (and thereby session name generation) to the installer (and updater)? Both the session name and cookie domain are essentially just constants after all. IIRC, there's a module / patch for wild-card domains; no idea if they will be affected by this ... just a thought.
• According to the current regex, the session names of http://drupal-iceland.org and http://drupaliceland.org will be the same. This might also be an issue with the cookie domain.
• By not specifying a cookie domain for hostnames such as 'localhost', I believe that this will fail for situations like http://drupal.org/node/56357#comment-85309 . Intranets etc. could also fall in this bracket.
• Could you please use a forward slash as a delimiter for the regex? I know that the current regex uses a back tick. But most of Drupal uses a forward slash.
• "an unique" should be "a unique".
• IMO, the comments in settings.php are overly long and the file is generally beginning to take on essay-like proportions. Can the comment be simplified? It seems to me that the $cookie_domain variable is essentially required only when user sessions are to be shared across multi-site installations. A simple one or two liner might be more appropriate.
• Code style: The session_name line needs a space after the concatenation operator.
• Not sure how valid it is now, but please see http://drupal.org/node/56357#comment-86658 for a report where the period prefix seems to have failed.

Thanks for chasing this up.
-K

Here's another patch, gets rid of preg_replace() calls.

I've benchmarked the string manipulation code (look in the patch) that replaces the first preg_replace() call (preg_replace('`(^\.?(www\.|)|[\d:\.]+$)`', '', $cookie_domain)), by running each 50000 times, the results:

I benchmarked md5() which replaces the second preg_replace() call, here are the results (running each call 50000 times):

• with a common simple url alienlabs.no-ip.org:
  
  preg_replace: 0.475145816803
md5: 0.38591003418
  
  md5() is slightly faster..
• with a more complicated url www.alienlabs.no-ip.org:1234/foo/bar:
  
  preg_replace: 0.65002322197
md5: 0.397738933563
  
  Here the difference is considerable

Zen,

Can't we avoid having to execute a majority of the code on every page request by moving domain (and thereby session name generation) to the installer (and updater)? Both the session name and cookie domain are essentially just constants after all. IIRC, there's a module / patch for wild-card domains; no idea if they will be affected by this ... just a thought.

I think this is excessive. I hope this patch would make this unnecessary.

* According to the current regex, the session names of http://drupal-iceland.org and http://drupaliceland.org will be the same. This might also be an issue with the cookie domain.

Doesn't apply to the new patch as md5() will generate two different sessions names for the two domains, but even in the previous patch, that weren't the case because cookie domain would protect the cookies from each other. Actually, the client would never allow such a collision for security. Further more, session_name() doesn't allow - to be used in the session name.

By not specifying a cookie domain for hostnames such as 'localhost', I believe that this will fail for situations like http://drupal.org/node/56357#comment-85309 . Intranets etc. could also fall in this bracket.

The RFC requires that cookie domains must be of second level or more, i.e., must have 2 dots or more, but don't worry, session_name would protect the collision in this case.

Could you please use a forward slash as a delimiter for the regex? I know that the current regex uses a back tick. But most of Drupal uses a forward slash.

No more preg_

"an unique" should be "a unique".

fixed

IMO, the comments in settings.php are overly long and the file is generally beginning to take on essay-like proportions. Can the comment be simplified? It seems to me that the $cookie_domain variable is essentially required only when user sessions are to be shared across multi-site installations. A simple one or two liner might be more appropriate.

I agree, but I'd recommend against me writing this, my writing style isn't great

Code style: The session_name line needs a space after the concatenation operator.

fixed

Not sure how valid it is now, but please see http://drupal.org/node/56357#comment-86658 for a report where the period prefix seems to have failed.

I'm not sure why that would happen, but anyway, that was before the approach this patch takes, it would be great if dan_aka_jack could test it.

I uploaded wrong patch, this is the correct one.

I abbreviated that essay in settings.php and tested the patch. I agree that this code is so fast that we don't need to move it to install time. RTBC

I think Moshe pruned too much documentation. For people that don't know how cookies work across multiple domains, the documentation is no longer helpful.

Let's take Moshe's proposal and add 1 or 2 sentences that explain _why_ you might want to share a cookie domain. This will help people debug login issues and gives them a bit more of a clue.

I appended a sentence answering the 'Why' question.

I've committed this to CVS HEAD. Thanks all.

Not sure if we'll want to backport this. I'm changing the version number so we can discuss this with Neil and the others.

Nice job fixing this.

Please backport. This has been driving me nuts for the latest couple of months.

I agree with Johan. The original reason I provided a patch is because I am experiencing this issue now in 5.1.

I can provide a back-ported patch for 5.x in 8-9 hours (after work).

Patch re-rolled for 5.x.

Sorry, previous was wrong patch file.

I was doing some testing on HEAD while I noticed a "Debug Strict notice" originated by this patch.
Just want to tell you while this issue is still active.

bootstrap.inc line 312 is:

It seems to me that explode() actually doesn't return an array by reference.

Thanks,
Riccardo

Indeed array_shift expects a variable, not just an array. This should be as simple as changing that line to:

Will provide a patch in a few hours (unless someone would like to beat me to it.)

@JohnAlbin: here is a patch rolled according to your instructions :)

The 5 back-port applies cleanly, appears to work as advertised and looks good to go. Drumm tells me that the commit is pending resolution of the Debug Strict notice issue.

-K

I have a totally different approach to solve this:
It needs some work to be strong enough but it might help us all reach a good solution.

I add these lines to settings.php after the domain setting.

Riccardo’s patch in #67 works as advertised for D6. Zen, sorry it took me so long to RTBC. Will look at D5 next.

Rerolled patch in #67 against HEAD (it applied with offset of 2 lines).

The solution in #69 submitted by Omar Abdel-Wahab on May 31, 2007 - 11:41 worked for me in drupal v5.1. Is this a technical good en safe solution?

Martin

Omar, your code is not a “totally different approach.” Using base_url to set the cookie parameters was how several versions of the patch worked before Zen did some well-needed benchmarking in #55 above.

Martin, the patch in #64 above is the 5.x version of the patch that has been tested by lots of people.

With the focus of the 6.x branch on feature-based patches, I took the liberty of moving Riccardo's patch to #150452 - Debug strict notice in bootstrap so we can focus this issue for the 5.x branch.

I’ve applied the patch in #64 above and Riccardo’s patch (the fix for the debug strict notice) and re-rolled.

@JohnAlbin: many thanks,

Riccardo

sites/default/settings.php no longer exists in CVS HEAD so we'll want to re-roll John' last patch.

I looked at the code, and it looks sane. It should be RTBC after a re-roll. Should be straight-forward to get this committed. :)

Nevermind my previous comment. The patch for CVS HEAD was moved to another issue. :)

I don't think this much of a change is acceptable in the stable 5.x branch. Changing how settings.php is configured isn't something to change now.

True it's a big change, but in its current state, without this patch, multi-site drupal is broken right out of the box. Broken to the degree that it's unusable. That's gotta count for something?

I agree with Dave (dalin). But, I think I see what Neil means…

If a user reads the instructions in his 5.1 settings.php file and un-comments and edits the line about cookie domains, ini_set('session.cookie_domain', '.example.com');, the patch would effectively go behind the user’s back and change the session.cookie_domain to something else.

I‘ve updated the patch to add a check to see if the session.cookie_domain is set and to use that value if the new method‘s preferred variable, $cookie_domain, is empty.

I think that extra check should be sufficient to allow users to continue to use the their 5.1 settings.php file with a 5.2 Drupal.

Great, not needing to touch the settings.php file for a 5.x upgrade is the idea.

To further prevent confusion, would it be make sense to only use session.cookie_domain?

While I agree that any change to the stable branch should be minimal at this point, it should also be recognized that most of the people who have set the cookie domain using ini_set('session.cookie_domain', '.example.com'); have done so because the default config didn't work, not because they really understood *why* they were doing it --

Which is all a long way of saying that changing settings.php at this late stage seems to be an acceptable path to address this problem -- for most people (ie, all of those not running multisite), this won't be an issue, but for the people who have been bit by this, having a solid fix in place will be worth manually upgrading the settings.php file

Neil, we could go with a solution that only used ini_set('session.cookie_domain', '.example.com');, but I’m not sure it would avoid confusion. The whole “Remember the leading period on the domain name, even if you wouldn't type it in your browser” was pretty confusing, which is why we are using the simpler $cookie_domain variable in 6.x.

Since you’re the 5.x maintainer, I’ll leave it to your decision. ini_set('session.cookie_domain') or $cookie_comain?

The $cookie_domain patch is available above. If you decide on ini_set, I’ll re-roll.

On second thought, if the user has added ini_set('session.cookie_domain', '.example.com');, we should probably not mess with this setting. By default this is automatically set, so it would be hard to figure out if the cookie_domain setting is default or intentionally set.

What about just doing the session_name() bit, but skipping the new $cookie_domain handling?

If the user has added ini_set('session.cookie_domain', '.example.com');, we should probably not mess with this setting.

The patch won’t modify their setting, if it's set correctly. Your correct that the patch doesn’t know if cookie_domain is automatically or intentionally set, but it doesn’t really matter. conf_init() just sanitizes the cookie domain and then sets it, so a correct cookie_domain from the settings.php file will pass through conf_init() unchanged.

What about just doing the session_name() bit, but skipping the new $cookie_domain handling?

Like this?

When trying uniquesession_4.patch on drupal-5.1.tar.gz I got a reject file for settings.php which is of rev 1.39. Getting version 1.39.2.2 from cvs solved this. This patch works after the mentioned handwork on both a localhost multisite as in the wild. New unique (md5 hashed) cookies are generated.

What is the patch doing for settings.php? Having patched bootstrap.inc and sites/default/settings.php other multisites seems to work without having a patched settings.php! What should have happened?

When will this RTBC? I'm in favour for this patch :-)

Regards, Clemens

The patch only removes a section of code from settings.php that has been moved to bootstrap.inc. If you are upgrading from 5.1, leaving in that duplicate code won’t break anything; it will just be a tiny, tiny bit slower.

Having patched bootstrap.inc and sites/default/settings.php other multisites seems to work without having a patched settings.php!

That is the expected result. The beauty of the patch doing the actual work in bootstrap.inc is that you don’t have to change any of your existing settings.php files!

Now we get for every request a PHP Notice in apache error logging:
PHP Notice:  Undefined variable:  base_url in .../includes/bootstrap.inc on line 238

Personally, I don't see what the fuss is in using the ported patch from HEAD. The user is in any case going to replace his settings.php when he upgrades. He is already aware that he will need to carry over his configuration settings from the old settings.php to the new one. Besides his database configuration, this would include the cookie domain setting and other ini_set changes. Furthermore, the user will now never have to deal with even wondering why there's a big block of hieroglyphics with regexes etc. in his settings.php (how it actually got in there in the first place is bewildering).

IMO, it is safe to assume that admins who mess with the ini_sets in settings.php know what they are doing and will be able to transfer their settings from the old file to the upgraded one. The only time this will actually cause an issue as such would be for CVS upgraders who will be notified of it via a Conflict flag. If need be we can leave a note in the upgrade / install docs.

My 10p.

I meant skipping handling the $cookie_domain setting and only calling session_name(). I have attached a patch for this. The behavior difference between this and the HEAD version is that HEAD will call session_name() using $cookie_domain and this version will use ini_get('session.cookie_domain'). It does remove session cookie collisions and does not change anything else.

Many people simply copy the whole old settings.php and don't bother with manually merging changes. We can not assume much about the contents of settings.php and it should not cause any problems while upgrading. I don't think this will be a problem, but we need to keep it in mind.

I think this, and #74, would be acceptable patches. The decision is, do we want to improve the cookie domain setting in Drupal 5.x.

The only substantive difference I see is that the new code strips port numbers. Otherwise, it is a good code cleanup. However, 5.x is a stable branch with no new functionality; handling of $cookie_domain could be considered new functionality.

The patch in #74 is no good (see my criticism in #80.)

The 5.x handling of cookie_domain has two bugs (126340 and 131017) that were fixed in HEAD by the 6.x version of this issue’s patch; neither IP addresses or port numbers are allowed in cookie_domain. Both of those issues need to be fixed, otherwise no session cookie is created. So your patch in #90 is insufficient.

The patch in #80 is the patch I prefer. It replaces the current #ini_set('session.cookie_domain', '.example.com'); comment with a # $cookie_domain = 'example.com'; comment. You could argue that the $cookie_domain variable is a feature, but I feel it’s simply a fix for the current, extremely-confusing-to-the-average-user “Remember the leading period on the domain name…” comment.

If you think the $cookie_domain is a feature (I’ll leave that determination to you, Neil), then the patch in #85 is the same as #80 (sans the global $cookie_domain.)

I think at this point it is probably best to go with something closer to HEAD since that has had a bit of testing.

The main responsibility is to not break sites with a proper ini_set('session.cookie_domain') in their settings.php since we don't expect people to change it and breaking a site on upgrade looks bad.

Do we think #80 is ready to go?

The patch in #80 adds code that is identical to what is in HEAD with this one exception:

This extra clause will make sure that if a settings.php file isn’t updated (a likely situation), it will use the value set by ini_set('session.cookie_domain', …).

I’m also tested the patch in #80 and am using it on my production, multi-site setup without any issues.

Seeing as I wrote the patch, I won’t mark it RTBC. But I feel that it is commit-worthy.

Committed #80 to 5.x.

This patch immediately caused Chat Room to stop working. To save overhead, Chat Room calls a PHP file that loads only the session phase of Drupal. For some reason, it creates a PHPSESSID cookie. Commenting out session_name('SESS'. md5($session_name)); makes it work again.

It may be unrelated, but I also noticed that without this line the session property of the user object is empty. I'm running this on the localhost domain.

What does it take to get an AJAX process to use the correct cookie?

I suspect this is a configuration issue since chatroom requires 5.x-1.x-dev rather than 5.1.

I’ve emailed Darren privately and, if there is an issue with the patch, will report back here.

Just wanted to add my experience into the mix. While not exactly the same issue as in this main post, I've encountered the "login, access denied, re-login, access denied..." problem. In my case, I only want the login to appear under the drupal/admin directory. I've pinpointed it to restricting access to the login block. No matter how I do it (whether it's restrict the login block to show up only at "admin" and "admin/*" or use php code to check the URI for admin and return true) I always end up at the same problem. Not sure if this problem is related to the one in discussion here...

The PHPSESSID cookie I mentioned in comment #95 may have been left over from before the change. I'm providing the headers which show the duplicate cookie being set to help with the diagnosis: