Two sites on the dev server:
1. projects/cvs/drupal
2. projects/drupal_upgrade

Logging into 1 logs me out of 2 (and vice versa) if using the same browser. Commenting out the session_regenerate_id lines in user_login_submit (user.module) as per chx's suggestion fixed the issue. But this is a security feature..

-K

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Zen’s picture

FileSize
773 bytes

Adding a unique session name appears to solve this issue.

-K

Zen’s picture

FileSize
774 bytes

.

fgm’s picture

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.

markus_petrux’s picture

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

ini_set('session.name', 'PHPSESSID');

where PHPSESSID is the default, but you could use something unique for each site.

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:

ini_set('session.cookie_domain', '.example.com');
ini_set('session.cookie_path', '/sub1');
ini_set('session.cookie_domain', '.example.com');
ini_set('session.cookie_path', '/sub2/sub21);
ini_set('session.cookie_domain', '.example.com');
ini_set('session.cookie_path', '/sub2/sub22');

Maybe something like that would work, but then, it may fail for users who doesn't have cokkies correctly secured in their browser settings.

Zen’s picture

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

moshe weitzman’s picture

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.

markus_petrux’s picture

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.

fgm’s picture

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 ?

markus_petrux’s picture

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?

Zen’s picture

FileSize
943 bytes

Thanks Markus. settings.php patch attached.

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

Cheers,
-K

fgm’s picture

Markus,

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

markus_petrux’s picture

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

Here are some notes:

/**
 * For multi-site configurations, you might want to make the session name unique
 * Note that PHPSESSID is the default, so the related cookie could be visible from
 * other sites on the same domain/subdomain.
 */
ini_set('session.name', 'PHPSESSID');

/**
 * The default value for cookie_domain is empty. This makes the
 * browser use the current host name present in the URL.
 * It is recommended to use the same exact domain specified
 * in $base_url. This prevents from sharing the session cookie
 * between subdomains, for privacy and security reasons.
 * However, if you need to share cookies between subdomains,
 * you may need to specify the common part of the domain
 * names. ie. if you need to share cookies between
 * www.example.com and subdomain.example.com, then the
 * correct value for cookie_domain would be .example.com
 */
ini_set('session.cookie_domain', '.example.com');

/**
 * The default value for cookie_path is /
 * For multi-site configurations, you may want to specify
 * the path here. This would take advantage of browser
 * security settings to prevent from getting access to the
 * session cookie between sites. see notes on cookie_domain.
 */
ini_set('session.cookie_path', '/example_path');

/**
 * Set the cookie_secure value to 1, if your site
 * uses HTTPS.
 */
ini_set('session.cookie_secure', 0);

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:

RewriteEngine On
RewriteCond %{HTTP_HOST} ^example\.com$
RewriteRule ^(.*)$ http://www.example.com/$1 [L,R=301]

To avoid the www. part:

RewriteEngine On
RewriteCond %{HTTP_HOST} ^www\.example\.com$
RewriteRule ^(.*)$ http://example.com/$1 [L,R=301]
markus_petrux’s picture

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...

Zen’s picture

Hi Markus,

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

-K

chx’s picture

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.

dan_aka_jack’s picture

ini_set('session.cookie_domain', '.example.com');

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).

markus_petrux’s picture

@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?

markus_petrux’s picture

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

dan_aka_jack’s picture

@markus : thanks for the tip.

markus_petrux’s picture

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.

moshe weitzman’s picture

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.

Tommy Sundstrom’s picture

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

chx’s picture

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

magico’s picture

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

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

greggles’s picture

Version: x.y.z » 6.x-dev
JohnAlbin’s picture

Title: Cannot stay logged in on more than one site on the same server » Cannot log in on more than one site on the same domain
Priority: Normal » Critical
Status: Active » Needs review
FileSize
1.31 KB

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”.

JohnAlbin’s picture

Version: 6.x-dev » 5.1

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.

RobRoy’s picture

Version: 5.1 » 6.x-dev

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.

moshe weitzman’s picture

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

JohnAlbin’s picture

FileSize
1.38 KB

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.

Steven’s picture

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

JohnAlbin’s picture

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.

Steven’s picture

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.

JohnAlbin’s picture

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

JohnAlbin’s picture

Title: Cannot log in on more than one site on the same domain » Login issues with mutiple sites in the same domain (session cookie collision)

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).

JohnAlbin’s picture

FileSize
1.42 KB

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

aether’s picture

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!

JohnAlbin’s picture

FileSize
1.42 KB

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.

dmitrig01’s picture

I'm having the same problem

JohnAlbin’s picture

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.

dalin’s picture

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

Thanks

AmrMostafa’s picture

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

AmrMostafa’s picture

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

JohnAlbin’s picture

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

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

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

Dries’s picture

Status: Reviewed & tested by the community » Needs work

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.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
4.77 KB

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.

Caleb G2’s picture

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.

JohnAlbin’s picture

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.

AmrMostafa’s picture

FileSize
4.78 KB

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.

JohnAlbin’s picture

FileSize
4.82 KB

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.

Dries’s picture

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.

AmrMostafa’s picture

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()

Zen’s picture

Status: Needs review » Needs work

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

AmrMostafa’s picture

FileSize
4.95 KB

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:

preg_replace: 0.494443893433
str manipulation: 0.28924202919

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.

AmrMostafa’s picture

Status: Needs work » Needs review
FileSize
4.95 KB

I uploaded wrong patch, this is the correct one.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.79 KB

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

Dries’s picture

Status: Reviewed & tested by the community » Needs work

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.

moshe weitzman’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.88 KB

I appended a sentence answering the 'Why' question.

Dries’s picture

Version: 6.x-dev » 5.x-dev

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.

forngren’s picture

Nice job fixing this.

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

JohnAlbin’s picture

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).

JohnAlbin’s picture

FileSize
3.99 KB

Patch re-rolled for 5.x.

JohnAlbin’s picture

FileSize
3.99 KB

Sorry, previous was wrong patch file.

riccardoR’s picture

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.

Debug Strict (PHP 5): D:\var\htdocs\drupal_head\includes\bootstrap.inc line 312 - Only variables should be passed by reference

bootstrap.inc line 312 is:

  $cookie_domain = '.'. array_shift(explode(':', $cookie_domain));

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

Thanks,
Riccardo

JohnAlbin’s picture

Version: 5.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Needs work

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

$cookie_domain = explode(':', $cookie_domain);
$cookie_domain = '.' . array_shift($cookie_domain);

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

riccardoR’s picture

Status: Needs work » Needs review
FileSize
790 bytes

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

Zen’s picture

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

owahab’s picture

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.

if (isset($base_url)) {
  $clean_base_url = str_replace('http://', '', $base_url);
  $base_array = explode('/', $clean_base_url);
  $cookie_domain = array_shift($base_array);
  $cookie_path = implode('/', $base_array);
  ini_set('session.cookie_domain', $cookie_domain);
  ini_set('session.cookie_path', '/'.$cookie_path);
}

// too lazy to diff and upload.

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

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

riccardoR’s picture

Title: Login issues with mutiple sites in the same domain (session cookie collision) » Login issues with multiple sites in the same domain (session cookie collision)
FileSize
737 bytes

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

cmseasy’s picture

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

JohnAlbin’s picture

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.

JohnAlbin’s picture

Version: 6.x-dev » 5.x-dev
FileSize
4.03 KB

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.

riccardoR’s picture

@JohnAlbin: many thanks,

Riccardo

Dries’s picture

Status: Reviewed & tested by the community » Needs work

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. :)

Dries’s picture

Status: Needs work » Reviewed & tested by the community

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

drumm’s picture

Status: Reviewed & tested by the community » Needs work

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.

dalin’s picture

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?

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
4.18 KB

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.

drumm’s picture

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?

bonobo’s picture

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

JohnAlbin’s picture

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.

drumm’s picture

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?

JohnAlbin’s picture

FileSize
2.95 KB

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?

clemens.tolboom’s picture

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

JohnAlbin’s picture

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!

clemens.tolboom’s picture

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

Zen’s picture

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.

drumm’s picture

FileSize
827 bytes

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.

JohnAlbin’s picture

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.)

drumm’s picture

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?

JohnAlbin’s picture

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

  if (!$cookie_domain) {
    // If the $cookie_domain is empty, try to use the session.cookie_domain.
    $cookie_domain = ini_get('session.cookie_domain');
  }

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.

drumm’s picture

Status: Needs review » Fixed

Committed #80 to 5.x.

Darren Oh’s picture

Status: Fixed » Active

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?

JohnAlbin’s picture

Status: Active » Fixed

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.

mrtransistor’s picture

Category: bug » support
Priority: Critical » Normal
Status: Fixed » Active

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...

Darren Oh’s picture

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:

http://localhost/~darren/drupal-5/sites/all/modules/chatroom/chatroomread.php

POST /~darren/drupal-5/sites/all/modules/chatroom/chatroomread.php HTTP/1.1
Host: localhost
User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Accept: text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip,deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Keep-Alive: 300
Connection: close
Content-Type: application/x-www-form-urlencoded
X-Requested-With: XMLHttpRequest
Referer: http://localhost/~darren/drupal-5/?q=chatrooms/chat/1
Content-Length: 276
Cookie: SESS943b7ba519ab9c6aa7744f2cdb60b44f=64ef0cabbffbc5781dabdfd9221f0b73
Pragma: no-cache
Cache-Control: no-cache
drupal_base=%2FUsers%2Fdarren%2FSites%2Fdrupal-5&chatroom_base=sites%2Fall%2Fmodules%2Fchatroom&update_count=1&user_base=modules%2Fuser&chat_id=1&last_msg_id=0&timezone=0&timestamp=1184602464&chat_cache_file=%2Ftmp%2Flocalhost%2F_darren%2Fdrupal-5%2Fdrupal_chat_cache%2Fchat_1

HTTP/1.x 200 OK
Date: Mon, 16 Jul 2007 16:14:25 GMT
Server: Apache/1.3.33 (Darwin) PHP/4.4.4
X-Powered-By: PHP/4.4.4
Set-Cookie: SESS0c6b0b1a60a18f1c57a72638c4dde480=5c968c6b68ee2506cba0d0df58721138; expires=Wed, 08 Aug 2007 19:47:46 GMT; path=/
Last-Modified: Mon, 16 Jul 2007 16:14:26 GMT
Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0
Pragma: no-cache
Connection: close
Transfer-Encoding: chunked
Content-Type: text/html
JohnAlbin’s picture

Category: support » bug
Priority: Normal » Critical
Status: Active » Fixed

Mrtransistor, I think you might be experiencing this bug, assuming you are using Drupal 5.1. The bug fix will be included with Drupal 5.2. But please open a separate support request rather than re-using this fixed bug report. Thanks!

Darren Oh’s picture

Status: Fixed » Active

My last comment should not be taken to mean this issue is fixed. If you look at the headers, you can clearly see that the browser sent a cookie header and received a Set-Cookie header. This patch has caused Drupal to create duplicate cookies.

Darren Oh’s picture

Status: Active » Fixed

I'm setting this back to fixed since the problem of sites overwriting each others cookies has been solved. I opened issue 159854 to deal with the problem of multiple session names being used for the same site. A patch is available.

Anonymous’s picture

Status: Fixed » Closed (fixed)
sent222’s picture

Why Different tabs in the same browser(firefox & IE) has same session id ?

Darren Oh’s picture

Because the session is designed to identify the user, not the window or tab. Would you want to have to log in separately in each tab?

fgm’s picture

This is actually a situation I frequently have when developing : need to test the behaviour of a module both for a logged-in session and a not-logged-in session, or a session logged-in with a different set of roles.

The typical solution is to use different browsers.

mariagwyn’s picture

I realize this issue is closed, but I seem to be having a related problem.

I am running drupal 5.5, multisite install, using firefox to go between sites. I have three sites running off the code:
1. mariagwyn.com/home
2. mariagwyn.com/ekbatheon (may move eventually)
3. snq.mariagwyn.com

I changed line 146 in settings.php for sites 1 and 2 to:
$cookie_domain = 'mariagwyn.com';

When I do this, logging in to 1 or 2 logs me out of the other, but logging into 3 has no affect. I also tried to change ll 147 to include '/home' and '/ekbatheon', didn't work.

What am I doing wrong?

Many thanks,
Maria

mrgoltra’s picture

subscribing