Bug: Session Library

[gmeister2]

Describe the bug Timestamp of session data, saved in database, is always set to 0000-00-00 00:00:00

CodeIgniter 4 version CI 4.1.3

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

• write() (line 201 & line 222, 'timestamp' => 'now()')

Expected behavior, and steps to reproduce if appropriate When inserting values into the session table (default 'ci_sessions'), the timestamp is always set to 0000-00-00 00:00:00. The session table is set up as described in: https://codeigniter.com/user_guide/libraries/sessions.html#databasehandler-driver

The frameworks internal query builder composed this query (gives a timestamp of 0000-00-00 00:00:00): INSERT INTO ci_sessions (id, ip_address, timestamp, data) VALUES ('123456789012345678901234567890', '123.12.0.0', 'now()', '');

If I do a manual insert into the database table, I get a correct result (e.g. timestamp of 2021-06-09 09:56:12): INSERT INTO ci_sessions (id, ip_address, timestamp, data) VALUES ('123456789012345678901234567890', '123.12.0.0', now(), '');

The difference being: 'now()' -> now()

My main concern is that the garbage collector (gc() on line 318) will delete all session data.

Additional question: Am I overlooking something? If not, how would I be able to do a temporary fix for this.

Context

• PHP version 7.4.20
• MySQL 5.7.34

[kenjis]

Am I overlooking something? If not, how would I be able to do a temporary fix for this.

How about removing the timestamp line? https://github.com/codeigniter4/CodeIgniter4/blob/995c51f383844bc44a607026ea6ab85b06c7e87e/system/Session/Handlers/DatabaseHandler.php#L201

[gmeister2]

Thanks for your suggestion.

These might also work (on line 201 & 222):

change 'timestamp' => 'now()',

to 'timestamp' => date('Y-m-d H:i:s'), or (keeping the 'now()' with postgre) 'timestamp' => $this->platform === 'postgre' ? 'now()' : date('Y-m-d H:i:s'),

Both work for me (my db system: MySQL 5.7.34).

Can it be confirmed that this is an actual bug? And if so, which solution is needed for either MySQL and/or Postgres.

[paulbalandan]

to 'timestamp' => date('Y-m-d H:i:s'), or (keeping the 'now()' with postgre) 'timestamp' => $this->platform === 'postgre' ? 'now()' : date('Y-m-d H:i:s'),

Are you saying Postgres can parse the string 'now()' into its own function NOW()? AFAIK, MySQLi also has a NOW() function.

[gmeister2]

What I wanted to communicate is that:

• In my system MySQL doesn't seem to parse the 'now()' into NOW() and this might be a bug.
• I have no experience with Postgres and have no real suggestion for it's specific implementation. 'PR #2256 from pstef/ci_sessions-types' does seem written with Postgres in mind, that is why I left the 'now()' there.

[kenjis]

@gmeister2 The workaround was not enough. We must take care of not only insert but also update. https://github.com/codeigniter4/CodeIgniter4/blob/995c51f383844bc44a607026ea6ab85b06c7e87e/system/Session/Handlers/DatabaseHandler.php#L222

[kenjis]

In my system MySQL doesn't seem to parse the 'now()' into NOW() and this might be a bug.

'now()' is string. It is not now(). It is not a bug of MySQL.

[gmeister2]

'now()' is string. It is not now(). It is not a bug of MySQL.

I am not suggesting this is a bug in MySQL. This 'issue thread' is about reporting a bug in the session library, i.e. 'now()' on line 201 & 222 not getting set in the query builder as now().

This might be a possible solution (on line 201 & 222): 'timestamp' => date('Y-m-d H:i:s'),

[paulbalandan]

Is Postgres NOW() returning a localised timestamp? Or based on UTC? If the former we can use date(), otherwise we use gmdate().

[gmeister2]

I think that Postgres NOW() returns a localized timestamp with a timezone indication.

From the docs (https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-CURRENT):

SELECT CURRENT_TIMESTAMP; Result: 2019-12-23 14:39:53.662522-05

transaction_timestamp() is equivalent to CURRENT_TIMESTAMP, but is named to clearly reflect what it returns. ... now() is a traditional PostgreSQL equivalent to transaction_timestamp().

When inserting this date into a 'timestamptz' column it will get converted to UTC. From docs:

timestamptz is accepted as an abbreviation for timestamp with time zone

For timestamp with time zone, the internally stored value is always in UTC (Universal Coordinated Time, traditionally known as Greenwich Mean Time, GMT).

From CI docs (https://codeigniter.com/user_guide/libraries/sessions.html#databasehandler-driver):

CREATE TABLE "ci_sessions" ( "id" varchar(128) NOT NULL, "ip_address" inet NOT NULL, "timestamp" timestamptz DEFAULT CURRENT_TIMESTAMP NOT NULL, "data" bytea DEFAULT '' NOT NULL );

CREATE INDEX "ci_sessions_timestamp" ON "ci_sessions" ("timestamp");

Might the solution be something like this? 'timestamp' => $this->platform === 'postgre' ? gmdate('Y-m-d H:i:s') : date('Y-m-d H:i:s'),

[paulbalandan]

Wait.

"timestamp" timestamptz DEFAULT CURRENT_TIMESTAMP NOT NULL,

If the timestamp has a default value, then we can just remove the lines updating timestamp as it will be updated anyway with CURRENT_TIMESTAMP when inserted/updated to the database. And CURRENT_TIMESTAMP is equivalent with the NOW(), right?

[kenjis]

Just a question, why was it changed to timestamp instead of int in CI4? It seems int is more portable.

[kenjis]

I've found the commit to change to timestamp: https://github.com/codeigniter4/CodeIgniter4/pull/2256/commits/529b5ad593f52e358eaebf04bd1c5c8301fb8207 in #2256

It was introduced in v4.1.2. It seems it is a BC break change, but there is no documentation. https://codeigniter4.github.io/CodeIgniter4/installation/upgrade_412.html

[gmeister2]

@paulbalandan : The current implementation (for MySQL) will only take CURRENT_TIMESTAMP on a new insert, when no value is specified: `timestamp` timestamp DEFAULT CURRENT_TIMESTAMP NOT NULL,

This suggestion will also do an update of the field (MySQL, see: https://dev.mysql.com/doc/refman/5.7/en/timestamp-initialization.html): `timestamp` timestamp DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP NOT NULL,

For Postgres there seems not do be a direct alternative for the above suggestion, this link suggests using a trigger: https://stackoverflow.com/questions/1035980/update-timestamp-when-row-is-updated-in-postgresql

@kenjis :

It seems int is more portable.

It seems it is a BC break change, but there is no documentation.

Good points.

[MGatner]

It was introduced in v4.1.2. It seems it is a BC break change

@kenjis That commit you referenced is from 2 years ago.

[gmeister2]

That commit you referenced is from 2 years ago.

@MGatner Correct, but it is only recently merged into develop. (MGatner merged commit ad8aa07 into codeigniter4:develop on Feb 24)

To me this feels like a real serious bug, i.e. the garbage collector cleaning all sessions at random moments, due to the fact that the timestamp is incorrectly set (see my previous posts in this thread).

Some background: I am eager to try out the Myth Auth authentication library, but currently still use Ion Auth 4, which uses the CI4 session library.

[MGatner]

it is only recently merged into develop

Wow, thanks for pointing that out. Yes that definitely was a mistake and should not have passed. I have no Postgres experience so rely on other team members for those reviews. Maybe @pstef can help out here as well and we can figure out the proper type and implementation.

[MGatner]

Is this possibly related to https://github.com/codeigniter4/CodeIgniter4/pull/4744?

[gmeister2]

Is this possibly related to #4744?

It might be, but I think this bug stands on its own (although ultimately also related to the garbage collector).

There are already some proposed solution in this thread. The point where we seem stuck is the Postgres experience ;-) . The original committer (@pstef) did seem to have Postgres in mind though.

[pstef]

I changed the type to timestamp with time zone because it's the designated (as per SQL) type for timestamps, which is what this column stores. Not only does it validate values according to correct values of a timestamp, but it's also easier to work with (for one of many possible examples, with timestamptz as the type, a query like "show me sessions active within the last 15 minutes" doesn't require conversion from an integer to a timestamp).

Both MySQL and Postgres can generate a timestamp for the current transaction and I would argue that doing so on the DB side is more reliable. When an SQL query says "now", the database knows what "now" is (no time zones involved in saving the value) and how to store it; as opposed to generating the value in PHP code which has to take time zones into considerations.

Therefore my only suggestion is to find a way for this session-handling code to properly use an equivalent of CURRENT_TIMESTAMP in the MySQL case.

[gmeister2]

Just to get a common understanding: both MySQL and Postgres can interpret these constructs: CURRENT_TIMESTAMP and NOW(), so there lies not the problem.

• The problem is the fact that when the actual database insertion happens, the query builder (certainly in the case of MySQL), does not place the function now() in the final query string, but instead the string 'now()'. I don't know how the query builder handles this in the case of Postgres or if Postgres itself maybe can interpret the string 'now()'. [Solution A] , could be: 'timestamp' => $this->platform === 'postgre' ? 'now()' : date('Y-m-d H:i:s'),

• Avoiding the 'now()' string (not the function per se) altogether. [Solution B] , could be: 'timestamp' => date('Y-m-d H:i:s'),

• As @paulbalandan suggested. [Solution C] , could be: Handling the insertion and update of the timestamp in the database table itself (see above comments).

• Or we might use a bit more raw SQL, below a untested/unfinished example. [Solution D] , could be:

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

$sql = 'INSERT INTO ' . $this->table . ' (id, ip_address, timestamp, data) VALUES (:id:, :ip_address:, now(), :data:)';

$this->db->query($sql, $insertData);

Any ideas about above 4 possible solutions, or maybe something else can help us?

[ziostanko]

with composer autoinstall for example on aws eb it is a problem because i need to override DatabaseHandler class with the changes above. Any update on closing this bug? it will be fixed soon?

[MGatner]

I recognize this is a problem but I really need someone "in the know" to take this on. I don't want simply to revert the changes, potentially introducing new/renewed issues. Can anybody claim this issue?

[michalsn]

Resolved via #4876