Cron hook incorrectly deletes all session api records [#1140064]

The patch committed for #785292: Incompatible with Pressflow breaks the intended behaviour of session_api_cron(). Before the patch, the session_id stored in the session_api table was created with session_id() and matched the sid stored in the sessions table so that the left join in the following query would correctly identify "outdated sids":

  $result = db_query("SELECT sap.sid FROM {session_api} sap LEFT JOIN {sessions} s ON (sap.session_id = s.sid) WHERE s.sid IS NULL");

The patch changes the way session_id's in session_api are created so that they do not match sids in the session table. This means that the query above returns all sids stored in the session_api table which are then all deleted.

This bug can be demonstrated with the flag module. As an anonymous user, bookmark some content. Run cron and the anonymous user's bookmarks will have been deleted.

Comment	File	Size	Author
#2	cron_session_id_bug-1140064-2.patch	3.55 KB	chaps2

Comments

Comment #1

chaps2 commented 27 April 2011 at 15:20

I'm not using the 7.x version of Session API but it looks like the same bug was introduced there as well.

Comment #2

chaps2 commented 27 April 2011 at 17:48

Status:

Active

» Needs review

Status	File	Size
new	cron_session_id_bug-1140064-2.patch	3.55 KB

Here's a patch that fixes this bug by adding a column to the session_api table to store the PHP session id. #785292: Incompatible with Pressflow should continue to work as designed but I haven't tested with Pressflow.

Comment #3

rbayliss commented 28 April 2011 at 03:38

Please see #1058960: Use an expiration logic when clearing sessions on cron. I don't think we need a new DB column for the core session ID, as it was being kept in session_api.session_id before #785292: Incompatible with Pressflow.

Comment #4

chaps2 commented 28 April 2011 at 07:40

I added a new column for the session_id() in order to maintain the Pressflow compatibility added in #785292: Incompatible with Pressflow. If you read through that issue, especially @quicksketch's comment 5 (#785292-5: Incompatible with Pressflow), the rationale behind not storing session_id() in the session_api_id cookie is well justified and in which case there needs to be a mapping in the database from the session_api_id to sesssion_id().

I've raised this as a separate issue because it's a critical bug. The fix should not include any new functionality.

Comment #5

rbayliss commented 28 April 2011 at 14:32

I agree this is a critical bug. My point is this:

This module used the session_id() for that column before the Pressflow patch, which is why we have the left over LEFT JOIN's to the sessions table.
Why not use this logic:

$session_id = session_id();
$session_id = empty($session_id) ? md5(ip_address() . time() . drupal_get_private_key()) : $session_id;

to determine the session ID, write it to the session_api.session_id column as before? That way if the user has a session, we use it for the cookie and the database column. If not, make one up and use that.

I don't think this is new functionality, more of a revision of the Pressflow patch and it fixes the session clearing for authenticated users without breaking any of the caching mechanisms that I can see. Agreed that the anonymous expiration issue is probably best kept separate as it does add new functionality.

Comment #6

chaps2 commented 3 May 2011 at 11:43

@rbayliss - That would certainly work and I agree that there is merit in not adding another database column. On the other hand I'm not sure that having two types of session id stored in the same column is 'correct': the only way to differentiate between a session_id() derived session id and the session api one is via the sessions table join which may not be desirable.

I'm happy to roll a patch with the code in #5 but it would be good to have some direction/input from the module maintainer(s) first.