Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#90 | uniquesession.minimal.patch | 827 bytes | drumm |
#85 | uniquesession_4.patch | 2.95 KB | JohnAlbin |
#80 | uniquesession_3.patch | 4.18 KB | JohnAlbin |
#74 | uniquesession_2.patch | 4.03 KB | JohnAlbin |
#71 | bootstrap_27.patch | 737 bytes | riccardoR |
Comments
Comment #1
Zen CreditAttribution: Zen commentedAdding a unique session name appears to solve this issue.
-K
Comment #2
Zen CreditAttribution: Zen commented.
Comment #3
fgmDoesn'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.
Comment #4
markus_petrux CreditAttribution: markus_petrux commentedMaybe you could try to set a different session.name for each site in its own settings.php file. Something like this:
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:
Maybe something like that would work, but then, it may fail for users who doesn't have cokkies correctly secured in their browser settings.
Comment #5
Zen CreditAttribution: Zen commentedMarkus + 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
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedwhat 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.
Comment #7
markus_petrux CreditAttribution: markus_petrux commentedWhy 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.
Comment #8
fgmThanks 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 ?
Comment #9
markus_petrux CreditAttribution: markus_petrux commentedGlad 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?
Comment #10
Zen CreditAttribution: Zen commentedThanks Markus. settings.php patch attached.
Please review and feel free to amend it if need be..
Cheers,
-K
Comment #11
fgmMarkus,
Yes I would think so. But before you ask, no I don't feel qualified enough to write it :-)
Comment #12
markus_petrux CreditAttribution: markus_petrux commentedI 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:
To avoid the www. part:
Comment #13
markus_petrux CreditAttribution: markus_petrux commentedOh, 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...
Comment #14
Zen CreditAttribution: Zen commentedHi Markus,
Can you please roll up a patch with whatever you recommend and concise comments?
-K
Comment #15
chx CreditAttribution: chx commentedZen, 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.
Comment #16
dan_aka_jack CreditAttribution: dan_aka_jack commentedI 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).
Comment #17
markus_petrux CreditAttribution: markus_petrux commented@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.
Thanks. I agree, this is a doc issue. I hope there is enough info posted in this issue. Any draft already?
Comment #18
markus_petrux CreditAttribution: markus_petrux commentedI have tried to compile some information about PHP session/cookies here:
http://www.phpmix.org/setting_up_php_sessions
Comment #19
dan_aka_jack CreditAttribution: dan_aka_jack commented@markus : thanks for the tip.
Comment #20
markus_petrux CreditAttribution: markus_petrux commentedok, 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.
Comment #21
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #22
Tommy Sundstrom CreditAttribution: Tommy Sundstrom commentedThis may be related: http://drupal.org/node/64054
Comment #23
chx CreditAttribution: chx commentedThis IMO needs addressing , let's decide which patch we would like to see in.
Comment #24
magico CreditAttribution: magico commentedThis can also have something to do with http://drupal.org/node/82662
@chx: what do you think that can be done about this?
Comment #25
gregglesComment #26
JohnAlbinThis 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:
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 thesession.cookie_domain
value concatenated with theconf_path()
value to provide a quick, easily-calculated, and unique session name. And to prevent the session name from being excessively long I’vemd5
’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”.Comment #27
JohnAlbinJust 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.
Comment #28
RobRoy CreditAttribution: RobRoy commentedIt'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.
Comment #29
moshe weitzman CreditAttribution: moshe weitzman commentedi 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
Comment #30
JohnAlbinMoshe, 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 thanmd5($domain.conf_path())
andsession_name()
is faster thanset_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 theconf_init()
line ( sinceconf_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.Comment #31
Steven CreditAttribution: Steven commentedIsn't the cookie path the right way to solve this?
Comment #32
JohnAlbinHi 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.
Comment #33
Steven CreditAttribution: Steven commentedWe 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.
Comment #34
JohnAlbinThe 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
Comment #35
JohnAlbinI 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:…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).
Comment #36
JohnAlbinThe new patch updates some comments and (eep) removes a tab.
Comment #37
aether CreditAttribution: aether commentedJohnAlbin,
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!
Comment #38
JohnAlbinI’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.Comment #39
dmitrig01 CreditAttribution: dmitrig01 commentedI'm having the same problem
Comment #40
JohnAlbinI’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:
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 thecookie_name()
.I would very much like to see some solution to this problem included in 5.2.
Comment #41
dalinJohn your patch in #38 works for me on both Drupal 5 and 4-7.
Thanks
Comment #42
AmrMostafa CreditAttribution: AmrMostafa commentedPlease bare in mind different port numbers if possible: http://drupal.org/node/126340
Comment #43
AmrMostafa CreditAttribution: AmrMostafa commentedPlease bare in mind different port numbers: http://drupal.org/node/126340
Comment #44
JohnAlbinAmr, don’t worry. This patch uses $base_url, so it already addresses port numbers.
Comment #45
moshe weitzman CreditAttribution: moshe weitzman commentedwe've had a couple of people say that this patch works and fixed their ailing site. RTBC.
Comment #46
Dries CreditAttribution: Dries commented1.
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.
Comment #47
JohnAlbinWe’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.Comment #48
Caleb G2 CreditAttribution: Caleb G2 commentedHow 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.Comment #49
JohnAlbinCaleb: 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. Theini_set('session.cookie_domain', …)
function is still used in the background (inconf_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.
Comment #50
AmrMostafa CreditAttribution: AmrMostafa commentedI'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.
Comment #51
JohnAlbinAmr, thanks for testing!
if (!empty($_SERVER['HTTP_HOST']))
is needed for E_ALL compliance. HTTP_HOST is not always set.This new patch adds Amr’s suggestions.
Comment #52
Dries CreditAttribution: Dries commentedThis 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.
Comment #53
AmrMostafa CreditAttribution: AmrMostafa commentedDries, 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()
Comment #54
Zen CreditAttribution: Zen commentedVisual review only:
Thanks for chasing this up.
-K
Comment #55
AmrMostafa CreditAttribution: AmrMostafa commentedHere'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):
alienlabs.no-ip.org
:md5() is slightly faster..
www.alienlabs.no-ip.org:1234/foo/bar
:Here the difference is considerable
Zen,
I think this is excessive. I hope this patch would make this unnecessary.
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.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.
No more preg_
fixed
I agree, but I'd recommend against me writing this, my writing style isn't great
fixed
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.
Comment #56
AmrMostafa CreditAttribution: AmrMostafa commentedI uploaded wrong patch, this is the correct one.
Comment #57
moshe weitzman CreditAttribution: moshe weitzman commentedI 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
Comment #58
Dries CreditAttribution: Dries commentedI 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.
Comment #59
moshe weitzman CreditAttribution: moshe weitzman commentedI appended a sentence answering the 'Why' question.
Comment #60
Dries CreditAttribution: Dries commentedI'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.
Comment #61
forngren CreditAttribution: forngren commentedNice job fixing this.
Please backport. This has been driving me nuts for the latest couple of months.
Comment #62
JohnAlbinI 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).
Comment #63
JohnAlbinPatch re-rolled for 5.x.
Comment #64
JohnAlbinSorry, previous was wrong patch file.
Comment #65
riccardoR CreditAttribution: riccardoR commentedI 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
Comment #66
JohnAlbinIndeed 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.)
Comment #67
riccardoR CreditAttribution: riccardoR commented@JohnAlbin: here is a patch rolled according to your instructions :)
Comment #68
Zen CreditAttribution: Zen commentedThe 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
Comment #69
owahab CreditAttribution: owahab commentedI 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.
// too lazy to diff and upload.
Comment #70
JohnAlbinRiccardo’s patch in #67 works as advertised for D6. Zen, sorry it took me so long to RTBC. Will look at D5 next.
Comment #71
riccardoR CreditAttribution: riccardoR commentedRerolled patch in #67 against HEAD (it applied with offset of 2 lines).
Comment #72
cmseasy CreditAttribution: cmseasy commentedThe 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
Comment #73
JohnAlbinOmar, 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.
Comment #74
JohnAlbinWith 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.
Comment #75
riccardoR CreditAttribution: riccardoR commented@JohnAlbin: many thanks,
Riccardo
Comment #76
Dries CreditAttribution: Dries commentedsites/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. :)
Comment #77
Dries CreditAttribution: Dries commentedNevermind my previous comment. The patch for CVS HEAD was moved to another issue. :)
Comment #78
drummI 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.
Comment #79
dalinTrue 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?
Comment #80
JohnAlbinI 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 thesession.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.
Comment #81
drummGreat, 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?
Comment #82
bonobo CreditAttribution: bonobo commentedWhile 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
Comment #83
JohnAlbinNeil, 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 onini_set
, I’ll re-roll.Comment #84
drummOn 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?Comment #85
JohnAlbinThe 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 throughconf_init()
unchanged.Like this?
Comment #86
clemens.tolboomWhen 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
Comment #87
JohnAlbinThe 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.
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!
Comment #88
clemens.tolboomNow 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
Comment #89
Zen CreditAttribution: Zen commentedPersonally, 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.
Comment #90
drummI 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.
Comment #91
JohnAlbinThe 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 theglobal $cookie_domain
.)Comment #92
drummI 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?
Comment #93
JohnAlbinThe 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.
Comment #94
drummCommitted #80 to 5.x.
Comment #95
Darren OhThis 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?
Comment #96
JohnAlbinI 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.
Comment #97
mrtransistor CreditAttribution: mrtransistor commentedJust 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...
Comment #98
Darren OhThe 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:
Comment #99
JohnAlbinMrtransistor, 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!
Comment #100
Darren OhMy 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.
Comment #101
Darren OhI'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.
Comment #102
(not verified) CreditAttribution: commentedComment #103
sent222 CreditAttribution: sent222 commentedWhy Different tabs in the same browser(firefox & IE) has same session id ?
Comment #104
Darren OhBecause 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?
Comment #105
fgmThis 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.
Comment #106
mariagwyn CreditAttribution: mariagwyn commentedI 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
Comment #107
mrgoltra CreditAttribution: mrgoltra commentedsubscribing