Skip to content

[3.0] Read cached bans from the right place in the session - #9313

Open
albertlast wants to merge 1 commit into
SimpleMachines:release-3.0from
albertlast:fix/ban-cache-session-key
Open

[3.0] Read cached bans from the right place in the session#9313
albertlast wants to merge 1 commit into
SimpleMachines:release-3.0from
albertlast:fix/ban-cache-session-key

Conversation

@albertlast

Copy link
Copy Markdown
Collaborator

Note

This change was produced by an LLM. The code, the commit message and this
description were all written by Claude (Anthropic), driven by @albertlast. It has
not yet had human code review.

Everything stated below was verified by actually running it against a fresh
PostgreSQL install of this branch, rather than only reasoned about. Even so,
please review it as untrusted work: the diagnosis may be right while the fix is
not what SMF would prefer stylistically or architecturally.

Description

Security::checkBans() stores the restrictions that applied under $_SESSION['ban'],
by merging them in alongside the metadata:

$_SESSION['ban'] = array_merge(
    ['last_checked' => time(), 'id_member' => ..., 'ip' => ..., 'ip2' => ..., 'email' => ...],
    $bans,
);

The cached path read them back from $_SESSION directly, one level too high, so it
never found them:

$bans[$restriction] = $_SESSION[$restriction];

That is four warnings on every request that takes the cached path:

Undefined array key "cannot_access" in Sources/Security.php:396
Undefined array key "cannot_login" in Sources/Security.php:396
Undefined array key "cannot_post" in Sources/Security.php:396
Undefined array key "cannot_register" in Sources/Security.php:396

and an array of nulls returned to the caller, so a banned member stops being
recognised as banned until the ban is rechecked against the database.

This reads from $_SESSION['ban'], and only copies restrictions that are actually
present, since checkBans() only stores the ones that applied. It also restores
expire_time, which the cached path dropped even though User::enforceBans() uses
it to tell the member when the ban ends.

How this was verified

PostgreSQL 17 / PHP 8.4. Before: four warnings logged per request taking the cached
path. After: eight consecutive guest requests, 0 errors logged, all HTTP 200.
Re-checked with a real ban in place to confirm the cached path still returns the ban.

Relationship to other PRs

Same subsystem as #9310, #9311 and #9312, but an independent mechanism — this one is
observable without any ban existing, as log spam on ordinary requests. Merges cleanly
alongside them.

Also merges cleanly onto #9302, which touches Sources/Security.php at lines 24, 676
and 1037; this change is at ~396.

Issues References (Fixes|Related|Closes)

  1. Related: [3.0] Only enforce bans that actually exist #9310, [3.0] Clear the topic and board statics instead of unsetting them #9311, [3.0] Pass logBan() its row in the structure insert() expects #9312

Security::checkBans() stores the restrictions that applied under
$_SESSION['ban'], by merging them in alongside last_checked, id_member,
ip, ip2 and email. The cached path read them back from $_SESSION
directly instead, one level too high, so it never found them:

  Undefined array key "cannot_access" in Sources/Security.php:396
  Undefined array key "cannot_login" in Sources/Security.php:396
  Undefined array key "cannot_post" in Sources/Security.php:396
  Undefined array key "cannot_register" in Sources/Security.php:396

That is four warnings on every request that took the cached path, and a
returned array of nulls, so a banned member stopped being recognised as
banned until the ban was rechecked against the database.

Reads from $_SESSION['ban'], and only copies restrictions that are
present, since checkBans() only stores the ones that applied. Also
restores expire_time, which the cached path dropped even though
User::enforceBans() uses it to tell the member when the ban ends.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
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.

1 participant