Skip to content

fix: correct IP cast bug in update_session causing login failure#528

Merged
darknoon29 merged 1 commit intodevelopfrom
fix/session-update-ip-cast
Apr 3, 2026
Merged

fix: correct IP cast bug in update_session causing login failure#528
darknoon29 merged 1 commit intodevelopfrom
fix/session-update-ip-cast

Conversation

@darknoon29
Copy link
Copy Markdown
Member

Problem

After resetting a user's password directly in the database, login was failing with "Unidentified visitor" even though authentication logs showed the password verification succeeded.

Root Cause

In Sessions_Model::update_session(), the $user_ip parameter was cast to bool before being used in the SQL WHERE clause:

$user_ip = (bool)$user_ip;  // "82.67.34.73" → true
// ...
$request .= " and `session_ip` = '" . $user_ip . "'";
// → WHERE session_ip = '1'  ← never matches the real IP

This caused the UPDATE to match zero rows, leaving user_id = 0 in the session. The subsequent SELECT in select_user_data_session() then found the session row but with no linked user → "Unidentified visitor".

Fix

Removed the erroneous (bool) cast. The IP string is now properly escaped with sql_escape_string() only when it is actually provided (i.e. not false).

if ($user_ip !== false) {
    $user_ip = $this->db->sql_escape_string($user_ip);
    $request .= " and `session_ip` = '" . $user_ip . "'";
}

Files Changed

  • model/Sessions_Model.phpupdate_session()

The user_ip parameter was cast to bool before being used in the SQL
WHERE clause, causing the UPDATE to filter on session_ip = '1' instead
of the actual IP address. As a result, no row was updated, user_id
remained 0, and the subsequent session lookup returned no authenticated
user (Unidentified visitor).

Fix: remove the erroneous bool cast and escape the IP string only
when it is actually provided (not false).
Copilot AI review requested due to automatic review settings April 3, 2026 07:08
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Apr 3, 2026

@darknoon29 darknoon29 merged commit ef029bf into develop Apr 3, 2026
5 checks passed
@darknoon29 darknoon29 deleted the fix/session-update-ip-cast branch April 3, 2026 07:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a login failure where sessions were not being updated due to an incorrect boolean cast of the IP address in Sessions_Model::update_session(), preventing the UPDATE ... WHERE session_ip = ... clause from matching the real session row.

Changes:

  • Removed the erroneous (bool) cast on $user_ip in update_session().
  • Updated the IP filter logic to only append the session_ip predicate when $user_ip is explicitly provided (!== false), and to escape it before use.

Comment thread model/Sessions_Model.php
Comment on lines 61 to 65
$request = "UPDATE " . TABLE_SESSIONS . " SET `user_id` = " . $user_id . ", `session_lastvisit` = " . $lastvisit . " WHERE `id` = '" . $cookie_id . "'";
if ($user_ip) {
if ($user_ip !== false) {
$user_ip = $this->db->sql_escape_string($user_ip);
$request .= " and `session_ip` = '" . $user_ip . "'";
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add PHPUnit coverage for this bugfix (e.g., assert that when an IP string is provided, update_session() builds the UPDATE with session_ip = '<escaped ip>' and does not coerce it to a boolean). The repo already has unit tests for other models with mocked DBs, but there is no regression test ensuring session updates match the correct IP, so this could silently break again.

Copilot generated this review using guidance from repository custom instructions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants