Security and correctness fixes from a Codex-assisted review - #36
Merged
Merged
Conversation
A forged cancel=1 POST (no confirm_key) made confirm_box(true) fail, then confirm_box(false, ...) return false without rendering anything instead of exiting, letting execution fall straight through to the ban or restriction code below - CSRF-exploitable against an authenticated moderator's session. Both call sites now return immediately after confirm_box(false, ...), matching phpBB core's own confirm_box() idiom (e.g. includes/mcp/mcp_ban.php), where the gated action only ever runs inside a successful confirm_box(true) branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
set_options() stored the submitted move_group/restrict_group id with no validation, and the dropdown only hid special/founder-managed groups client-side. A non-founder admin with a_user and m_ban could configure a founder-managed privileged group as the move/restrict target, then grant another account its permissions by moving them into it - phpBB's own acp_users.php explicitly rejects this same operation for non-founders. set_options() now rejects a submitted group that's a special group or founder-managed (unless the current user is the founder), and get_groups() also hides founder-managed groups from non-founders in the dropdown, matching acp_users.php's own display logic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bh_del_posts() deleted every post a banned user ever made across every forum, gated only on m_ban. A moderator with m_ban but no delete permission in a particular forum could still wipe the target's posts there, and setting the ACP deletion default to "No" didn't prevent it either since del_posts=1 could still be submitted directly. Adds a new, independently-grantable permission, m_banhammer_del_posts_all, for the cross-forum authority; without it, bh_del_posts() now only deletes posts in forums the acting moderator already has m_delete in. The new migration copies the permission onto anyone who currently holds m_ban (via ROLE_MOD_FULL and any direct grants), so existing installs keep today's behavior unchanged, but going forward it's an explicit, auditable grant rather than a hidden side effect of m_ban. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…roller ban_domain_controller is routed through app.php, which doesn't load includes/functions_user.php by default (unlike common.php's request flow, which the profile-page ban listener already accounts for). Without another extension incidentally loading it first, confirming a domain ban hit an undefined-function error on user_ban() and created no ban. root_path/php_ext are now injected the same way the listener and cron task already get them, and the controller includes functions_user.php on demand before calling user_ban(), matching the guard used everywhere else in this extension. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Four related bugs in the "restrict instead of ban" feature:
- Expiry didn't restore the original default group: group_user_add()
sees the user as already a member (they were never removed from their
original group while restricted) and returns GROUP_USERS_EXIST before
reaching the code that sets the default group. Now calls
group_user_attributes('default', ...) directly, which is what
group_user_add() would have called internally.
- Changing the ACP restrict group while a restriction was still active
stranded the user: the cron task read the *current* config value for
every expiring row instead of what was actually applied. A new
restrict_group_id column records the group used at restriction time,
written by do_restrict_stuff() and read by the cron task per row, with
a migration that backfills existing rows from the current setting as
the best available guess.
- Configuring the same group for both bans and restrictions caused the
ban-expiry group cleanup (undo_bh_group) to immediately undo an active
restriction on its next session check, since it can't tell a
restricted-not-banned user from a leftover ban-group member. It now
checks active_restriction() first when the two settings match.
- Purging the extension dropped the restriction-tracking table without
restoring anyone still actively restricted, stranding them in the
restrict group permanently with no way back. The new migration's
revert_data() now restores every still-tracked user (group and
default) before its own revert_schema() drops the column, and before
restrict_group.php's own revert drops the table next.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bh_del_privmsgs() hand-deleted PM rows without updating recipients' unread/new counts, custom-folder counts, or attachments, and could leave recipients with unread notifications pointing at a message that no longer exists. It now delegates to phpbb_delete_users_pms(), the same bulk cleanup phpBB's own account deletion uses, and only handles the banned user's own folder/rule rows directly (their own account settings, which don't affect anyone else). bh_del_posts() also deleted every poll vote the user ever cast, in any topic, without decrementing poll_option_total - corrupting poll totals and letting the user vote again after a temporary ban expired, since their prior-vote record was gone. phpBB's own user_delete() doesn't touch POLL_VOTES_TABLE at all for the same reason; this now matches that and leaves votes alone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The MCP post-approval "Ban Hammer" link went straight to memberlist.php?...&bh=1 with no other parameters, which skips the profile page's options form entirely and lands directly on the confirmation step with every option defaulted to zero: permanent duration, no email/IP ban, no deletions, no group move, no SFS report. An admin's configured seven-day-ban default, for example, silently became a permanent username-only ban through this shortcut. The link now points at the plain profile page instead, which shows the real options form reflecting the ACP-configured defaults, same as visiting the profile directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
get_file() discarded curl_exec()'s return value and only inspected the HTTP status code. If the server sent 200 headers and the transfer then timed out, curl_exec() returns false but the recorded HTTP code still reads 200, so the moderator was told "All actions were performed correctly" despite the Stop Forum Spam report never actually completing. Now treats a false return from curl_exec() as a failure too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
BH_STYLE ended with a stray embedded double quote even though the template already supplies its own attribute quotes, producing malformed HTML like style="background-color: green; color: white;";" on both success and failure results. The value is fixed text either way, not a templating bug, so this isn't an XSS finding - just broken markup. Also removed the ACP template's S_SAVED success box: nothing ever assigns S_SAVED, since a successful save goes through trigger_error() like every other success message in this extension, so the block was unreachable dead code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
@kaileymsnay A little late, but could you still look over this? |
This was referenced Sep 22, 2026
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
Nine focused fixes addressing 14 findings from a Codex-assisted security review of this extension (auth/CSRF, permission boundaries, restriction lifecycle, and several data-hygiene bugs). Each commit is scoped to one concern; see individual commit messages for the full reasoning and the exact phpBB core behavior each fix was verified against.
cancel=1POST let both the ban and restrict actions execute without ever passing through a real confirmation, exploitable via CSRF against an authenticated moderator.del_postslet any moderator withm_bandelete a user's posts in forums they have no delete permission in. Added a new, independently-grantable permission (m_banhammer_del_posts_all) for that broader authority; a migration copies it onto everyone who currently hasm_ban, so existing behavior is unchanged today, but it's an explicit, auditable grant going forward.Test plan
php -lon every changed/added PHP fileextension:purge, confirming the user is correctly restored to their original default group and removed from the restrict group before the tracking table is droppedm_banhammer_del_posts_allpermission is created and copied ontoROLE_MOD_FULL(which carriesm_ban), and cleanly removed on purgeInvestigated and written by Claude on behalf of William Jacoby (bonelifer).