Bug: session DatabaseHandler does not set timestamp column properly

[sneakyimp]

Describe the bug
The file system/Session/Handlers/DatabaseHandler.php fails to insert or update correct timestamp values the timestamp column.

CodeIgniter 4 version
4.1.3

Affected module(s)
system/Session/Handlers/DatabaseHandler.php

Expected behavior, and steps to reproduce if appropriate
When you configure your CodeIgniter installation to use the DatabaseHandler for sessions, and you define the ci_sessions database table as described here, then the records insert into the ci_sessions table should be created with timestamp column containing the current timestamp. As you access the site, these records should have the timestamp column properly updated.

This problem is clearly due to the means by which the query is generated. The code attempts this:

			$insertData = [
				'id'         => $sessionID,
				'ip_address' => $this->ipAddress,
				'timestamp'  => 'now()',
				'data'       => $this->platform === 'postgre' ? '\x' . bin2hex($sessionData) : $sessionData,
			];

or this:

		$updateData = [
			'timestamp' => 'now()',
		];

When these arrays are handed to the builder->update() or db->table->insert() methods, the scalar string 'now()' gets escaped, which yields a query like so:

UPDATE `session` SET `timestamp` = 'now()' WHERE `id` = '<some-session-id>'

MySQL ends up setting the timestamp to '0000-00-00 00:00:00', which is clearly incorrect.

This could be remedied by generating a datetime string from PHP's settings like so:

		$updateData = [
			'timestamp' => date('Y-m-d H:i:s')
		];

however this might result in a timestamp that doesn't match the current time on the SQL server (sometimes SQL server time doesn't match PHP server time). OR -- and I don't know for sure -- this might yield a value that doesn't match with the timezone specified in CodeIgniter's config.

Alternatively, the SQL could be manually generated. Or perhaps we could add a parameter or property to the objects in use to prevent the values from being escaped.

Context

• OS: Ubuntu 20.04 LTS
• Web server: Apache 2.4.41
• PHP version: 7.4.3

[MGatner]

Pretty sure this is a duplicate of #4807 and already fixed. Try the latest version of the 4.2 branch and see if you still have issues.

[sneakyimp]

Pretty sure this is a duplicate of #4807 and already fixed. Try the latest version of the 4.2 branch and see if you still have issues.

I've tried both origin/4.2 and the HEAD/develop branch and they both do not correctly update the timestamp column for the recommended database definition in the documentation for the session table:

CREATE TABLE IF NOT EXISTS `ci_sessions` (
        `id` varchar(128) NOT NULL,
        `ip_address` varchar(45) NOT NULL,
        `timestamp` timestamp DEFAULT CURRENT_TIMESTAMP NOT NULL,
        `data` blob NOT NULL,
        KEY `ci_sessions_timestamp` (`timestamp`)
);

Updating the database to try and update to CURRENT_TIMESTAMP also does not work:

CREATE TABLE IF NOT EXISTS `ci_sessions` (
        `id` varchar(128) NOT NULL,
        `ip_address` varchar(45) NOT NULL,
        `timestamp` timestamp DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP NOT NULL,
        `data` blob NOT NULL,
        KEY `ci_sessions_timestamp` (`timestamp`)
);

Unless I'm missing something, I'm pretty sure I pointed out the problem in my OP: the assignment of now() to the timestamp column results in the NOW() function being escaped so it's treated as a string rather than an SQL function.

[sneakyimp]

@MGatner it looks like @michalsn made an attempt to repair the problem but his change isn't in the HEAD branch or 4.2. Also, there may still be a problem with now() being treated as a string (and escaped) in michal's gc routine. I haven't tested it to confirm there's a problem, but I wonder if that line might also fail.

UPDATE: michal's change doesn't appear to be in the develop branch, but I was able to download a ZIP from the 4.2 branch via the browser UI which appears to work.

I'm wondering about a couple of things:

1. shouldn't michal's fix by in the develop branch? It does not appear to be applied there.

2. Forgive me if this is a noobish question, but I cloned the repo via CLI, checked out origin/4.2 branch, and michal's fix is not there:

# select 4.2 branch in github UI in my browser, copy the url and clone it to my workstation:
git clone https://github.com/codeigniter4/CodeIgniter4.git github
cd github
git branch origin/4.2
git checkout origin/4.2
# edit the Session/Handlers/DatabaseHandler.php file and michal's change is not there??

[MGatner]

Glad you figured it out! I'm not sure what happened with your clone but it should definitely be in place on 4.2 - you can see where the PR was pointed at the top of its page: #4876

Our next release will be a content-free restyle of the code to PSR-12 (plus some others) guidelines. Because of this we have been directing all content changes to 4.2 until after this next release.

[sneakyimp]

This bug/issue is definitely a duplicate, so I'm going to close it, but shouldn't that change be applied to the develop branch also? It's a pretty fundamental bug if everybody's session gets wiped every time sessions are garbage collected.

[MGatner]

@sneakyimp Did you read my previous response? TL;DR: No changes to develop now, but all of 4.2 branch will eventually be rolled in.

[sneakyimp]

Did you read my previous response?

I did, and did not realize from reading it that no changes are being made to develop -- which is the default branch when you visit or clone this repo. I expect that will cause a lot of confusion. I'd also point out that clicking download from the CI site gets you 4.1.3 which also has the bug.

Clearly, I am of the opinion that this fix should be deployed ASAP. That said, I'm closing this issue.

[MGatner]

Sorry to be unclear. After v4.1.4 (hopefully soon) we will roll the 4.2 branch into develop and continue from there.

If something merited a hotfix, under Git Flow this would be applied to master, released, then existing branches would be rebased with it. I don't believe this issue merits a hotfix but I'm open to hearing from others. If you feel strongly about it maybe start a forum thread and see what kind of support it gathers.