Conversation
phpBB's ACP permission editor filters every screen's permission list
through \phpbb\permissions::permission_defined(), which only recognizes
permissions registered via the static array or a core.permissions event
- not merely anything present in phpbb_acl_options. The migration that
creates and grants this permission never registered it, so it worked
correctly at the ACL-check level but was invisible and unrevokable
through the normal ACP UI. Confirmed via a real container instance:
permission_defined('m_banhammer_del_posts_all') was false before this
listener, true after. Matches the pattern phpBB's own phpbb/pages
extension uses for its custom a_pages permission.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… save admin_controller's founder-group check only ran when a group was selected in the ACP. It never protected the case where a group already configured as the move/restrict target later becomes founder-managed - an admin flipping group_founder_manage on an existing group for unrelated reasons - since do_ban_hammer_stuff()/do_restrict_stuff() never re-checked before actually moving a user into it. A moderator with only m_ban could then still grant a target account founder-managed group membership via that stale configuration. Both now re-validate through a shared safe_group_name() helper immediately before use, treating a deleted or now-founder-managed group exactly like "no group configured" rather than trusting a value only validated when it was saved. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
My earlier fix for the shared ban/restrict group bug compared the *current* bh_restrict_group_id and bh_group_id config values. If either setting is edited while a restriction created under the old matching values is still active, the guard stops applying - even though the user's own tracking row still points at the group now equal to bh_group_id - so cleanup removes them prematurely again, just via a different trigger than the original bug. active_restriction() now also selects restrict_group_id, and undo_bh_group() checks that against bh_group_id directly, independent of whatever bh_restrict_group_id currently is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ictions
Two related bugs in do_restrict_stuff()'s final step:
- The tracking row was inserted before calling group_user_add(), whose
return value was then discarded entirely. If the target happened to
already be a member of the restrict group for an unrelated reason,
group_user_add() returns GROUP_USERS_EXIST before reaching the code
that sets the default group - so the tracking row would claim an
active restriction that never actually changed the user's default
group. Now the insert only happens after checking the result:
GROUP_USERS_EXIST falls back to group_user_attributes('default', ...)
directly (same fix already used in restriction_expiry's restore path);
any other non-false result removes the just-inserted row rather than
leaving a restriction on record that isn't actually in place.
- Two moderators confirming a restriction on the same user within the
same narrow window could both pass the active_restriction() check
before either commits, since checking and inserting were two separate,
unsynchronized steps and the table's user_id index wasn't unique. Both
would insert their own tracking row (typically for the same
bh_restrict_group_id, since it's one shared config value); when the
shorter of the two expires, cron would then remove that shared group
membership out from under the still-active longer restriction.
The new migration replaces the plain user_id index with a unique one.
The insert in do_restrict_stuff() now runs under sql_return_on_error()
and checks get_sql_error_triggered(): the loser of the race gets the
same "already has an active restriction" message instead of a second,
conflicting row (verified directly against phpBB's own DBAL - a
duplicate insert is caught as get_sql_error_triggered() === true, not
an uncaught SQL error).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The forum-permission filter added for the cross-forum deletion fix only gated $posts (and, through it, the reports-closing logic). The unconditional cleanup below - bookmarks, drafts, forum tracking/watch rows, moderator cache, notifications, topics-posted records - still ran regardless. A moderator without m_banhammer_del_posts_all or m_delete in any forum the target posted in could select "delete posts", have zero posts actually deleted, and still wipe all of that unrelated account data as a side effect. Now bails out before any of it when there were posts to consider but none were left after permission filtering, leaving a fully-blocked del_posts request with no effect at all, matching what its own permission denial should mean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…hout cURL Two independent gaps in admin_controller's settings handling: - Nothing stopped an admin from selecting an Open- or Free-type group as the restrict target. phpBB's own UCP lets a member resign from either type unilaterally (includes/ucp/ucp_groups.php only blocks resignation for Closed/Hidden/Special groups) - a restricted user could just leave via UCP and regain their previous permissions while Ban Hammer's own tracking row still shows them as restricted. A banned user can't log in to do the same, so this only applies to the restrict group, not the ban move-to group. validate_group() now takes a $reject_self_service flag, set only for the restrict group. - The SFS API key and "allow HTTP" fields are hidden from the ACP form entirely when cURL isn't available (see the template's <!-- IF SFS_CURL --> guard), so they're simply absent from the submitted data in that case. set_options() wrote them unconditionally regardless, which silently blanked the stored SFS key on any unrelated settings save while cURL was disabled. Both are now only written when cURL is available, matching what the form actually offers to submit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codex/Lumo review docs (e.g. codex-review-2026-09-22.md) are working notes kept locally alongside the repo, not committed - matches the existing untracked docs/codex-review-2026-09-22.md and docs/lumo-review-2026-09-22.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Round 2's founder-managed-group re-check (safe_group_name()) didn't fetch or validate group_type at all, so it never covered the Open/Free self-resignation gap round 2 also fixed - but only at ACP-save time. A site upgrading with an Open restrict group already configured, or an admin changing an existing Closed restrict group to Open afterward, would still let do_restrict_stuff() use it: the restricted user could then resign via UCP (confirmed against includes/ucp/ucp_groups.php, same as the save-time fix) while the tracking row stays behind. safe_group_name() now takes the same $reject_self_service flag validate_group() does, passed true from the restrict-group call site only (a banned user can't log in to self-resign, so this doesn't apply to the ban move-to group). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two related gaps in do_restrict_stuff()'s handling of an already-existing
group membership (group_user_add() returning GROUP_USERS_EXIST):
- The tracking row never recorded whether the restriction itself put the
user in the restrict group, or whether they already belonged to it for
an unrelated, legitimate reason. Both restriction_expiry's expiry path
and the purge migration's restoration unconditionally removed the
membership either way - stripping a membership the restriction never
granted in the first place, and one the user may have had permanent,
independent reasons to keep. A new restrict_new_membership column
records which case applied per restriction; expiry and purge now only
call group_user_del() when it's set. Verified end-to-end against a
real install: a user who already belonged to the restrict group kept
that membership after purge, while a normal restriction's membership
was correctly removed.
- The GROUP_USERS_EXIST fallback (group_user_attributes('default', ...))
didn't check its own result. That function explicitly only sets the
default group for *approved* members (confirmed by reading
functions_user.php directly) - a pending join request returns
NO_USERS and changes nothing, so a restriction targeting a pending
member silently never took effect while still being recorded as
successful. Now checked the same way the outer group_user_add() result
already was, and treated as a failure requiring the tracking row to be
removed rather than left claiming a restriction that isn't in place.
The purge-restoration logic moved from restrict_group_id_column.php (an
earlier migration, whose own revert runs *after* this one - migrations
revert newest-first) to the new restrict_membership_column.php, since it
needs to read restrict_new_membership before it's dropped.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
My earlier fix for the permission-filtered-to-nothing case only guarded against "there were posts, but permission filtered out all of them" - it left the unrelated account-wide cleanup (bookmarks, drafts, notifications, ...) running unconditionally whenever the target had zero posts from the start, regardless of whether the acting moderator had any legitimate post-deletion authority at all. Simplified to gate on the filtered result alone (empty($posts)), which covers both cases the same way: a moderator with no relevant delete authority now gets no effect at all, whether that's because every post was blocked or because there was nothing to delete in the first place. A moderator with authority over at least one of the target's posts still gets the full account cleanup, unchanged from how del_posts has always worked. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ndex restrict_unique_user's unique index closes off a concurrent-restriction race, but that race is exactly what could already have left more than one tracking row for the same user_id on a site upgrading from before the fix. Creating a unique index over pre-existing duplicates fails outright and blocks the whole extension upgrade - reproduced directly: "UNIQUE constraint failed: banhammer_restrict.user_id". phpBB applies a migration's own update_schema() before its update_data(), so the cleanup can't live inside restrict_unique_user itself; it has to run in a migration that completes first. The new restrict_dedupe migration keeps the most recently created row per duplicated user_id and discards the rest - which one "actually won" the original race isn't recoverable at this point, and every real install should have zero duplicates to begin with. Verified the exact SQL directly: it correctly collapses duplicates, and a unique index then creates successfully against the result. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both the move-group and restrict-group selects shared one generator, which only ever hid special/founder-managed groups. Selecting an Open/Free group for the restrict target - still offered in the dropdown - now gets rejected by validate_group() with a generic FORM_INVALID, with no indication of why. get_groups() takes the same $reject_self_service flag validate_group() already has, passed true only for the restrict-group dropdown, so it no longer offers a choice its own validator would reject. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_new_membership Two problems in round 3's restrict_new_membership tracking, both found by re-verifying its own reasoning against the actual pre-fix code history: - The backfill defaulted every existing row to 1 (true), on the claim that pre-fix code "always resulted in a fresh membership". That's wrong: before round 2 added any group_user_add() result checking at all, the tracking row was inserted unconditionally and the result was ignored outright - a user already belonging to the restrict group before being "restricted" still got a tracking row, with no way to tell afterward which case applied. A site upgrading directly from master (which is exactly what happens when this branch merges) could have exactly such rows. Default is now 0 (assume NOT created by this restriction): leaving a genuinely ban-hammer-created membership in place a little longer than ideal is a much smaller problem than stripping one the restriction never granted. - Moving the purge-restoration logic into the new migration assumed a site would always have it installed by the time it purges. phpBB's migrator only reverts migrations that are actually recorded as installed - a purge that happens after the extension's files were upgraded but before the extension was re-enabled (which is what actually runs new migrations) would never install restrict_membership_column at all, so its revert_data() never runs, and restrict_group_id_column (the migration that IS installed) no longer had any restoration logic after round 3 removed it - a purge in that state would silently drop the tracking table with restrictions never restored, worse than before either migration existed. restrict_group_id_column now has its own restoration fallback, which checks whether restrict_new_membership exists and behaves conservatively (never removes membership) when it doesn't. When restrict_membership_column DOES run normally, its own revert already emptied the table first, making this fallback a harmless no-op. Verified both directly: seeded a real install missing restrict_membership_column entirely (deleted its migration record, dropped the column) with an active restriction, and confirmed purge still restores the default group while conservatively leaving group membership alone; a normal full-upgrade purge/enable cycle still works end-to-end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…posted
Two remaining gaps in bh_del_posts(), found by re-checking round 3's own
fixes rather than assuming they covered every path:
- The empty($posts) early return was nested inside the
!acl_get('m_banhammer_del_posts_all') branch, so a moderator who DOES
hold that permission skipped it entirely - a target with zero posts
still triggered the full unrelated account-data cleanup (bookmarks,
drafts, notifications, ...) even though delete_posts() had nothing to
do. Moved the check outside the permission branch so it applies
regardless of which path granted access; behavior for an actual,
non-empty deletion is unchanged either way.
- The unconditional TOPICS_POSTED_TABLE delete removed the user's
"posted in this topic" marker for every topic they've ever posted in,
including ones the permission filter left untouched (their post there
survives, but the marker saying they posted there doesn't). Confirmed
by reading functions_admin.php directly: delete_posts() already calls
update_posted_info() to correctly rebuild this table for the topics it
actually affects, using each topic's remaining live posts to determine
the correct value. The extension's own blanket delete was both
redundant for what delete_posts() already touched and actively wrong
for what it didn't.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
undo_bh_group() has always compared a banned user's group membership against the *current* bh_group_id ACP setting, with no record of what a given ban actually did - the exact same class of bug the restrict feature had (and got fixed, twice, across the last two review rounds), flagged as deliberately deferred until now since fixing it meant the same kind of tracking-table buildout. Changing the ACP setting (or disabling group-moving) while a ban is still active stranded the user in whatever group they were actually moved into; conversely, an unrelated legitimate member of the *currently* configured group could have their membership stripped on their next non-banned session check, even though ban-hammer never added them there. New banhammer_ban_group table, mirroring banhammer_restrict but with the unique index on user_id from the start this time - restrict_group.php's retrofit needed a whole separate dedupe migration to add one after the fact. do_ban_hammer_stuff()'s move_group step now records the group actually used, the user's default group beforehand, and whether group_user_add() created a fresh membership or found the user already there (same GROUP_USERS_EXIST handling as do_restrict_stuff(), including the same default-group-attribute fallback). undo_bh_group() now looks up this per-user tracking instead of comparing against current config, and also correctly restores the original default group on unban - which the old code never did at all, leaving phpBB's own group_user_del() fallback (typically REGISTERED) instead of the user's real prior group. A currently-banned, already-moved user has no tracking row when this migration first runs (the table didn't exist under the old code), so a backfill step creates one - conservatively assuming the membership predates the ban (move_new_membership = 0) and leaving original_group_id unknown (0, so no default-group restore is attempted for these), same reasoning as restrict_membership_column's own legacy-row default. Verified end-to-end against a real install: a currently-banned+moved user backfilled correctly and kept their membership on unban (no tracking to say otherwise); a fresh ban+move recorded correctly and both removed membership and restored the original default group on unban; purge restores an in-progress tracking row before dropping the table. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three more Codex-assisted review passes against
masterafter #36 (each explicitly told to independently re-verify all prior fixes, not just look for new things), plus the ban-side tracking fix that rounds 2/3 both explicitly deferred as out of scope. Every finding across all three rounds was independently re-verified against actual phpBB core source or a real running phpBB 3.3.x install before being acted on — several of Codex's own claims turned out to be false leads and weren't acted on; several others caught real regressions in my own prior fixes.Round 2 — 2 regressions in #36's fixes, 4 new issues
m_banhammer_del_posts_allwithcore.permissions— the ACP permission editor only shows permissions registered that way; the permission worked at the ACL-check level but was invisible/unrevokable in the UI.undo_bh_groupcompares the restriction's own recorded group — the Security and correctness fixes from a Codex-assisted review #36 shared-group fix compared current config, which stopped protecting an active restriction the moment either setting changed.group_user_add()'s result and guard against concurrent restrictions — a failed/no-op group action still got recorded as a successful restriction; two moderators restricting the same user in a narrow window could create conflicting tracking rows (new unique index + result-checking).del_postshas no effect — the Security and correctness fixes from a Codex-assisted review #36 permission filter only gated which posts got touched, not the account-wide cleanup below it.Round 3 — re-verified all of round 2, found 5 more
GROUP_USERS_EXISThandling didn't distinguish "this restriction added the membership" from "the user already had it", so expiry/purge could strip a pre-existing membership.del_postscleanup still ran for a target with zero posts — the round 2 fix only covered "posts existed but were all filtered out."Round 4 — re-verified all of round 3, found 4 more (2 of them genuine errors in my own round 3 reasoning)
restrict_new_membership— round 3's backfill wrongly assumed pre-fix code "always resulted in a fresh membership" (it didn't — the result was ignored entirely before round 2), and moving purge-restoration into the new migration assumed it would always be installed by purge time, which isn't guaranteed if a site upgrades files without re-enabling first.topics_posted— the guard was nested inside one permission branch only; and a blanketTOPICS_POSTED_TABLEdelete undid phpBB's own correct per-topic bookkeeping for posts that survived permission filtering.The deferred issue, now fixed:
undo_bh_groupper-ban group trackingBans never had per-action tracking of which group a ban actually moved someone into — the same class of bug the restrict feature had (and got fixed twice above), explicitly deferred until the tracking pattern was mature. New
banhammer_ban_grouptable (unique index built in from the start this time, learning fromrestrict_group's retrofit pain), a backfill for anyone currently banned-and-moved under the old code, andundo_bh_group()rewritten to use per-user tracking instead of comparing against current config — which also means it now correctly restores the user's actual original default group on unban, something the old code never did at all.Test plan
php -lon every changed/added PHP file after every commitrestrict_membership_columnwas ever installed and confirmed the fallback in the older migration correctly restores the default group while conservatively leaving membership alonerepos/misc/validate.prompt.md) against the full branch:composer validatepasses,php -lclean repo-wide, zero trailing whitespace, all 64 language keys used with no dead keys or missing definitions, all fourcore.*events verified against the current events list. Two non-blocking[valinfo]items noted (queries in loops on a low-volume cron/migration path; touching an already-merged migration's never-yet-executed revert path) — no[valdeny]findings.Not fixed here: nothing remains deliberately deferred.
cc @kaileymsnay — same as #37, flagging since this is another large batch touching the extension's migration chain and permission system.
Investigated and written by Claude on behalf of William Jacoby (bonelifer).