Skip to content

[3.0] Load permissions before applying bans in User::enforceBans() - #9319

Merged
Sesquipedalian merged 2 commits into
SimpleMachines:release-3.0from
albertlast:fix/enforce-bans-permissions-not-loaded
Jul 29, 2026
Merged

[3.0] Load permissions before applying bans in User::enforceBans()#9319
Sesquipedalian merged 2 commits into
SimpleMachines:release-3.0from
albertlast:fix/enforce-bans-permissions-not-loaded

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 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

Logging in as any non-admin member dies with a fatal error:

Typed property SMF\User::$permission_sets must not be accessed before initialization

Forum::load() establishes the order the rest of the code relies on — permissions
first, bans second:

// Load the current user's permissions.
User::$me->loadPermissions();
...
// Check if the user should be disallowed access.
User::$me->enforceBans();

Login2::DoLogin() breaks that order. User::setMe() swaps User::$me for a
freshly loaded instance representing the member who just logged in, and nothing has
called loadPermissions() on that new instance:

User::setMe($this->member->id);
...
// Are you banned?
User::$me->enforceBans(true);

enforceBans() ends by applying bans and warnings to the permission sets:

// Fix up the banning permissions.
foreach ($this->permission_sets as $set) {
    $set->applyBansAndWarnings();
}

$permission_sets is a typed property with no default, so reading it on that
instance throws. Admins never hit this, because enforceBans() returns early for
them — which is why a forum looks healthy until the first ordinary member tries to
log in.

The deprecated banPermissions() compatibility function in Subs-Compat.php already
guards this exact case:

if (!isset(SMF\User::$me->permission_sets)) {
    SMF\User::$me->loadPermissions();
}

This gives User::enforceBans() the same guard.

Note that $permission_sets deliberately keeps no default value. banPermissions()
above depends on the uninitialised state to decide whether to load, and defaulting
the property to [] would paper over the fatal while silently skipping the
bans-and-warnings pass instead of loading the sets.

How this was verified

PostgreSQL 17 / PHP 8.4.23.

The baseline was not plain release-3.0: on that branch a login cannot get far
enough to reach this code, because it dies first in the path fixed by #9310. The
baseline used was release-3.0 plus the seven pending fixes #9310#9317, which is
the first state in which a login reaches DoLogin().

  • Before: POST ?action=login2 as an ordinary member → HTTP 500, and the message
    above recorded in the error log. The login cookie is set before the fatal, so the
    member ends up in the confusing state of being logged in on the next request while
    the login itself reported an error.
  • After: HTTP 302 to the forum, session established, index.php returns
    HTTP 200 as that member. Repeated three times from clean sessions, all 302.
  • Not simply disabling enforcement: inserted a real cannot_access ban on the same
    member and confirmed the forum still refuses them — index.php returns HTTP 403
    with "you are banned from using this forum".

One caveat found while checking that last point: for a banned member, the login
request itself then fails with a different, pre-existing error —
SMF\Actions\Login2::load(): Return value must be of type SMF\Actions\Logout, SMF\Actions\Login2 returned,
from the Logout::call() in the force-logout branch of enforceBans(). That is
unrelated to this patch: it reproduces identically on the baseline without it, and it
happens earlier in the method than the code touched here. Mentioned only so it is not
mistaken for a side effect.

php -l clean.

Relationship to other PRs

Touches Sources/User.php inside enforceBans(), at the end of the method (~2236).
#9310 touches ~2103 and #9311 touches ~2190 in the same method, so all three are
independent hunks; verified to merge cleanly alongside them.

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, [3.0] Read cached bans from the right place in the session #9313

User::$me->enforceBans() is called immediately after a successful login,
at which point User::setMe() has replaced User::$me with a freshly loaded
User instance whose permissions have not been loaded yet. Iterating over
$this->permission_sets then fataled with "Typed property
SMF\User::$permission_sets must not be accessed before initialization".

Admins never hit this because enforceBans() returns early for them.

Mirrors the guard that the deprecated banPermissions() compat function
already has.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread Sources/User.php Outdated
@Sesquipedalian
Sesquipedalian merged commit 0730e55 into SimpleMachines:release-3.0 Jul 29, 2026
4 checks passed
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