cookie name is wrong when accessing Drupal from subdir of install

This code in question is a backport from Drupal HEAD, so any issues need to be fixed there first.

Using multisite hosting, there can be many sites with different users and different sessions, for one index.php. This is not an acceptable algorithm for determining what is a site. Maybe conf_path() could be used.

I’ve confirmed that https and http accesses will have different session names. So that does need to be fixed. But…

The problem is that it creates a session name from $base_url, which changes depending on … whether the script is in the site's root directory or in a subdirectory.

That’s the desired behavior. A Drupal site installed at $base_url = http://example.com/ and a site at $base_url = http://example.com/myothersite/ will have different session names. If you want to share session cookies between the 2 separate Drupal installs, you’ll need to configure the sites to use the same user tables and to change the settings.php file’s $cookie_domain variable.

New patch fixes http/https issue for HEAD.

good catch. RTBC for the most recent patch.

Darren, I understand your $base_url problem now. Please see chatroom issue #160068 for the fix.

And I agree with Moshe, good catch on the http/https bug. I want to get this backported to 5.x ASAP!

Drupal, by design, assumes that $_SERVER['PHP_SELF'] will be in the root directory of the Drupal install. You can see that assumption/design in the way $base_url is calculated in conf_init(). If you’d like to argue that point, your patch will need to address how $base_url is created.

I‘m moving the http/https bug fix to issue #160107.

You can’t use PATH_TRANSLATED to get the file path to the current script, because it can be empty. See http://us.php.net/reserved.variables

Also, I don’t think it’s a good idea to calculate the $base_path on each request into Drupal. Since $base_url doesn’t change after installation, it would be much faster if the $base_url was specified in the settings.php file. The best thing would be for the installer to calculate it and add it to settings.php.

Are there any core modules that use AJAX requests in this way?

By default $base_url is not set in $settings, so the patch fixes an edge case (which may or may not be in core), but negatively affects all page requests.

If this is to get into core, one of two things must happen:

1. show an example of a core AJAX request getting a cookie with the wrong name (a critical bug), or
2. in addition to your current patch, update the installation process to save $base_url in settings.php file, so that, by default, the code that your current patch adds to conf_init() isn‘t run. I.e. update drupal_rewrite_settings(). Rather than negatively affecting the default case, this would actually improve the performance of the default case.

On the version number- this code should be the same for Drupal 5.x and 6.x, so the fix will hopefully be the same in both versions. In this situation, we should fix this in Drupal 6.x and then backport to 5.x, as I mentioned in update #1.

On splitting this patch into two parts- yes, this is a perfectly reasonable patch to split. If one chunk of code is ready to go, there is no reason to hold it back by tying it to another chunk of code.

On taking out the protocol, http or https- this looks like a good idea, but it needs a code comment.

On the rest, the directory traversal- I am a bit confused on what exactly this is doing. Could you either
1. Put in more code comments.
2. Summarize a sample configuration which triggers the bug in an update to this issue?

I have another idea for avoiding the directory traversal- use something based on drupal_get_private_key(). This will of course have to be hashed sufficiently to not reveal it. It does provide a unique value for each variables table, and there is almost always only one of those per-site, crazy shared-table installations excluded. I am not sure how this would affect install.php since there is not a database available at that point.

This should be fixed in both Drupal 5.x and 6.x. In this case, the version number of the issue should be the highest available. 6.x has a better chance of getting reviews since more developers are looking at it now. However, arguing this point is pointless if the reasons are ignored and I will leave this at the incorrect version.

Thanks for the explanation, this patch makes sense now.

As mentioned in follow-ups #1 and #2, you can have two URLs pointing to the same Drupal install and have two separate sites which should have separate cookies. $_SERVER['SCRIPT_FILENAME'] is be the same if two Apache virtual hosts point at the same filesystem path. This approach would break the intended behavior of having one cookie per site.

I did further research on my proposed alternative using Drupal's private key. Installation of Drupal doesn't actually seem to use cookies, which makes sense, since we don't have the sessions table either, so I think using Drupal's private key as a seed would be an effective way of ensuring we have one cookie per site.

I’m not sure this is a bug. It seems that the method Darren is describing (bypassing index.php and accessing a module’s file directly) is incompatible with multi-site configurations. For example:

If you have Drupal installed at / but your only configured sites are at /subsite1 and /subsite2 and then accessing /modules/example/ajax-connect.php directly, will mean that the ajax-connect.php file won’t know what subsite or what session cookie it should be using.

Since multi-site configurations are a supported part of the API, and I don’t see any docs to the contrary, this AJAX method can’t be supported. I’m in favor of marking this “by design” or “won't fix.”

tried the patch, and recieved this error:
patching file bootstrap.inc
Hunk #1 succeeded at 253 (offset -5 lines).
Hunk #2 FAILED at 283.
1 out of 2 hunks FAILED -- saving rejects to file bootstrap.inc.rej

how do I fix this?

Darren, of course I read your patch. But it’s immaterial, since it’s the described AJAX method which I’m arguing against being supported in core.

There are lots of possible multi-site configs. One of which is:

1. installing Drupal in the root of web accessible directory ( http://example.com/ )
2. But only configuring Drupal sites in sub-directories ( http://example.com/subsite1/ and http://example.com/subsite2/ ) i.e. http://example.com/ is not Drupal site.

In other words, any request outside of /subsite1 and /subsite2 are NOT handled by Drupal, but instead by real files (if anyone is unfamiliar with how this works, see the default /.htaccess file.) So a request to /modules/example/ajax-request.php gets the REAL file. And a request to /subsite1/modules/example/ajax-request.php won’t work because Drupal isn’t physically installed in /subsite1/.

So there‘s no patch you could add to core that would help the ajax-request.php to know which site config to use.

http://drupal.org/node/160107 has been committed to both 6.x and 5.x, in that order. This patch no longer applies cleanly.

At this point I have spent multiple hours reviewing this patch, including testing it.

There are two design problems with the approach in this patch:

1. The code is not obvious. I do agree that it does seem to get one session name per site, however the code does not readily explain this. At least, an additional comment is required.
2. This potentially generates plenty of calls to file_exists() and adds complex array manipulation. I am concerned about the speed of this approach.

This is why I have offered two separate alternatives which I am not fully convinced won't work and both will be faster since they both are a single function call which get a value from a static variable. Both are calls to existing APIs which are designed to uniquely identify sites.

I didn't get a promise to fix the broken Drupal 5 API,

Sorry, what API do you mean? Something 'that used to work' is not immediately an API.

Setting to 'needs review'

weird. Setting to need review for real.

Subscribing

I hope this gets resolved. A point release probably shouldn't break contrib modules unless unavoidable

Drupals name for stability is on the line here, it's against Drupals own principles to release a minor-version update that fixes bugs but potentially totally breaks 3th party modules!! This cannot be true, please fix.

Drupal current bootstrap API is designed to work from scripts residing in the root of the Drupal install. You can see that both in the way files are included via include("./filepath") and in the way $base_url is calculated.

If a module’s script attempted to startup by using the Drupal bootstrap API, it wouldn’t work. This is true for Drupal 5.0, 5.1, and 5.2.

The chatroom module gets around this limitation by using a custom startup in chatroomread.php. (BTW, there is a module-side patch to get chatroom to work with Drupal 5.2 at http://drupal.org/node/160068)

Yes, the bootstrap API changed in Drupal 5.2, but the inability to use it in a module was true for 5.1 as well.

Could the bootstrap API be changed to allow easy usage by a module? Sure, but that’s a feature request, not a bug report.

This is not going to get changed in 5.x.

Scott and Kees, the change in the API was unavoidable; see issue 56357 for the critical bugs it fixed. Are there modules besides Darren’s chatroom breaking?

just because something unusual happenned to work in drupal5, does not mean it was supported. where does it say that we support scripts run from random subdirs of drupal? i agree with won't fix here. furthermore, the workaround is trivial. just symlink or copy to root, and make some minor tweaks to the script. With such an easy workaround available, I feel that Darren is just (vehemently) making a point and others are free to agree or disagree with him. In any case, the status of this issue doesn't really matter. Noone is working on restoring that capability and such a patch is not likely to garner attention and support.

subscribe