From 814cbbf2c4a9dbcda2250fc77cc32de01a29f320 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 18:47:05 -0500 Subject: [PATCH 01/16] fix(security): register m_banhammer_del_posts_all with core.permissions 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 --- event/banhammer_listener.php | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index 66f4533..955e6f6 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -105,9 +105,28 @@ static public function getSubscribedEvents() ), 'core.session_set_custom_ban' => 'undo_bh_group', 'core.mcp_queue_approve_details_template' => 'add_mcp_queue_banhammer_link', + 'core.permissions' => 'add_permission', )); } + /** + * Register m_banhammer_del_posts_all with phpBB's permission system. + * Without this, \phpbb\permissions::permission_defined() never + * recognises it, and the ACP permission editor filters it out of + * every screen - the migration-granted permission would work but be + * invisible and unrevokable through the normal UI. + * + * @param \phpbb\event\data $event The event object + * @return void + * @access public + */ + public function add_permission($event) + { + $permissions = $event['permissions']; + $permissions['m_banhammer_del_posts_all'] = array('lang' => 'ACL_M_BANHAMMER_DEL_POSTS_ALL', 'cat' => 'misc'); + $event['permissions'] = $permissions; + } + /** * Add a Ban Hammer link to the MCP post-approval detail page (mcp_post.html), * so a moderator can ban the poster without leaving the approval workflow to From a0254d4dfdfffe42aacc32704eaada8bc1e69c89 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 18:47:29 -0500 Subject: [PATCH 02/16] fix(security): re-validate move/restrict groups at use time, not just 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 --- event/banhammer_listener.php | 58 ++++++++++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 12 deletions(-) diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index 955e6f6..89ca69b 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -270,19 +270,14 @@ public function do_ban_hammer_stuff($event) return; } - if ($this->config['bh_group_id']) - { - // Get group name for banned users, if any. - $sql = 'SELECT group_id, group_name FROM ' . GROUPS_TABLE . ' - WHERE group_id = ' . (int) $this->config['bh_group_id']; - $result = $this->db->sql_query($sql); - $group_name = $this->db->sql_fetchfield('group_name'); - $this->db->sql_freeresult($result); + // Re-validated here, not just trusted from when it was saved in the + // ACP: the group may have been deleted, or made founder-managed, + // since then. + $group_name = $this->safe_group_name($this->config['bh_group_id']); - if (empty($group_name)) - { - $this->config['bh_group_id'] = 0; - } + if ($group_name === '') + { + $this->config['bh_group_id'] = 0; } if (!$this->request->is_set('bh') || ($this->request->is_set('bh') && $this->request->is_set('confirm_key') && !confirm_box(true))) @@ -505,6 +500,13 @@ public function do_restrict_stuff($event) $user_id = (int) $data['user_id']; $restrict_group_id = (int) $this->config['bh_restrict_group_id']; + // Re-validated here, not just trusted from ACP-save time: the group + // may have been deleted, or made founder-managed, since then. + if ($restrict_group_id && $this->safe_group_name($restrict_group_id) === '') + { + $restrict_group_id = 0; + } + if (!$this->auth->acl_get('m_ban') || $data['user_type'] == USER_FOUNDER || $user_id == $this->user->data['user_id'] || !$restrict_group_id) { // Nothing to see here, move on. No group configured in the ACP @@ -624,6 +626,38 @@ protected function active_restriction($user_id) return ($row) ?: null; } + /** + * A configured move/restrict group's name, re-validated at the point + * it's about to be used rather than trusted from ACP-save time: the + * group may have been deleted, or made founder-managed, since then. + * + * @param int $group_id + * @return string Group name, or '' if unset, gone, or founder-managed + * and the acting moderator isn't the founder. + * @access protected + */ + protected function safe_group_name($group_id) + { + if (!$group_id) + { + return ''; + } + + $sql = 'SELECT group_name, group_founder_manage + FROM ' . GROUPS_TABLE . ' + WHERE group_id = ' . (int) $group_id; + $result = $this->db->sql_query($sql); + $row = $this->db->sql_fetchrow($result); + $this->db->sql_freeresult($result); + + if (!$row || ($this->user->data['user_type'] != USER_FOUNDER && $row['group_founder_manage'])) + { + return ''; + } + + return $row['group_name']; + } + // Once a ban is cleared try and remove the user from the banned group set in the ACP of the extension public function undo_bh_group($event) { From 352e7b2b25123e9e3f96823e833735816783f631 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 18:47:47 -0500 Subject: [PATCH 03/16] fix: undo_bh_group compares the restriction's own recorded group 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 --- event/banhammer_listener.php | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index 89ca69b..8b76627 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -616,7 +616,7 @@ public function do_restrict_stuff($event) */ protected function active_restriction($user_id) { - $sql = 'SELECT restrict_id + $sql = 'SELECT restrict_id, restrict_group_id FROM ' . $this->restrict_table . ' WHERE user_id = ' . (int) $user_id; $result = $this->db->sql_query_limit($sql, 1); @@ -673,11 +673,16 @@ public function undo_bh_group($event) if ($group_id) { - // The ban and restrict groups can be configured to be the - // same group. A restricted (not banned) user deliberately - // sits in it, so leave their membership alone while the - // restriction is still active instead of undoing it here. - if ((int) $this->config['bh_restrict_group_id'] === (int) $this->config['bh_group_id'] && $this->active_restriction($this->user->data['user_id']) !== null) + // A restricted (not banned) user deliberately sits in + // whatever group their own restriction actually used, so + // leave that membership alone rather than undoing it here - + // checked against the restriction's own recorded group, not + // the current ACP settings, which may have changed since + // (comparing current settings would stop protecting an + // already-active restriction the moment either is edited). + $restriction = $this->active_restriction($this->user->data['user_id']); + + if ($restriction !== null && (int) $restriction['restrict_group_id'] === (int) $this->config['bh_group_id']) { return; } From e42d7f1c425d7fe359a5ff1d6923adae055bd84a Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 18:48:07 -0500 Subject: [PATCH 04/16] fix: check group_user_add() result and guard against concurrent restrictions 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 --- event/banhammer_listener.php | 40 ++++++++++++++++++- migrations/restrict_unique_user.php | 59 +++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 migrations/restrict_unique_user.php diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index 8b76627..e147190 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -575,7 +575,7 @@ public function do_restrict_stuff($event) return; } - if (!function_exists('group_user_add')) + if (!function_exists('group_user_add') || !function_exists('group_user_attributes')) { include($this->root_path . 'includes/functions_user.' . $this->php_ext); } @@ -593,9 +593,45 @@ public function do_restrict_stuff($event) 'restrict_until' => $restrict_until, ); $sql = 'INSERT INTO ' . $this->restrict_table . ' ' . $this->db->sql_build_array('INSERT', $sql_ary); + + // Two moderators confirming a restriction on the same user at + // nearly the same moment could both pass the active_restriction() + // check above before either commits. The unique index on user_id + // (see the restrict_unique_user migration) turns the loser's + // insert into a caught error here instead of a second, silently + // conflicting tracking row. + $this->db->sql_return_on_error(true); $this->db->sql_query($sql); + $insert_failed = (bool) $this->db->get_sql_error_triggered(); + $this->db->sql_return_on_error(false); + + if ($insert_failed) + { + $this->template->assign_var('RESTRICT_MESSAGE', $this->user->lang['BH_ALREADY_RESTRICTED']); + + return; + } - group_user_add($restrict_group_id, array($user_id), false, false, true); + $result = group_user_add($restrict_group_id, array($user_id), false, false, true); + + if ($result === 'GROUP_USERS_EXIST') + { + // Already a member for some unrelated reason: group_user_add() + // returns before setting the default group in that case (see + // the same fix in restriction_expiry's restore path), so set + // it directly instead. + group_user_attributes('default', $restrict_group_id, array($user_id)); + } + else if ($result !== false) + { + // NO_USER / GROUP_USERS_INVALID: the group action never took + // effect (shouldn't happen - $user_id comes from the profile + // this event fired for). Don't leave a tracking row behind for + // a restriction that isn't actually in place. + $this->db->sql_query('DELETE FROM ' . $this->restrict_table . ' WHERE user_id = ' . $user_id); + + return; + } $args = array( 'mode' => 'viewprofile', diff --git a/migrations/restrict_unique_user.php b/migrations/restrict_unique_user.php new file mode 100644 index 0000000..a0f74e0 --- /dev/null +++ b/migrations/restrict_unique_user.php @@ -0,0 +1,59 @@ +db_tools->sql_unique_index_exists($this->table_prefix . 'banhammer_restrict', 'user_id'); + } + + static public function depends_on() + { + return array('\phpbbmodders\banhammer\migrations\permission_del_posts_all'); + } + + public function update_schema() + { + return array( + 'drop_keys' => array( + $this->table_prefix . 'banhammer_restrict' => array('user_id'), + ), + 'add_unique_index' => array( + $this->table_prefix . 'banhammer_restrict' => array( + 'user_id' => array('user_id'), + ), + ), + ); + } + + public function revert_schema() + { + return array( + 'drop_keys' => array( + $this->table_prefix . 'banhammer_restrict' => array('user_id'), + ), + 'add_index' => array( + $this->table_prefix . 'banhammer_restrict' => array( + 'user_id' => array('user_id'), + ), + ), + ); + } +} From 4afa65ead5793254422bbcd187c835e3bf483147 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 18:48:25 -0500 Subject: [PATCH 05/16] fix: stop wiping unrelated account data when del_posts has no effect 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 --- event/banhammer_listener.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index e147190..ed5ae67 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -786,6 +786,8 @@ private function bh_del_posts() // posts in forums the acting moderator could delete in anyway. if (!$this->auth->acl_get('m_banhammer_del_posts_all')) { + $had_posts = !empty($posts); + foreach ($posts as $post_id => $post_row) { if (!$this->auth->acl_get('m_delete', (int) $post_row['forum_id'])) @@ -797,6 +799,15 @@ private function bh_del_posts() unset($posts[$post_id]); } } + + if ($had_posts && empty($posts)) + { + // There were posts to consider, but no permission to touch + // any of them: stop here rather than still wiping unrelated + // account data (bookmarks, drafts, notifications, ...) + // below for an action that had no actual effect on posts. + return; + } } // And now handle the reports. From f83744f296e3a3a71172be825922a9c4a774d12d Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 18:48:47 -0500 Subject: [PATCH 06/16] fix: reject self-service restrict groups and preserve the SFS key without 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 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 --- controller/admin_controller.php | 34 +++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/controller/admin_controller.php b/controller/admin_controller.php index 7c7e4a0..4f7d7e6 100644 --- a/controller/admin_controller.php +++ b/controller/admin_controller.php @@ -130,7 +130,11 @@ protected function set_options() $restrict_group = $this->request->variable('restrict_group', 0); $this->validate_group($move_group); - $this->validate_group($restrict_group); + // Open/Free groups let a member resign from them unilaterally via + // UCP (see includes/ucp/ucp_groups.php), which would let a + // restricted user simply leave the restriction. A banned user + // can't log in to do the same, so this only applies here. + $this->validate_group($restrict_group, true); $this->config->set('bh_ban_email', $this->request->variable('ban_email', 0)); $this->config->set('bh_ban_ip', $this->request->variable('ban_ip', 0)); @@ -141,37 +145,51 @@ protected function set_options() $this->config->set('bh_del_signature', $this->request->variable('del_signature', 0)); $this->config->set('bh_group_id', $move_group); $this->config->set('bh_restrict_group_id', $restrict_group); - $this->config->set('bh_sfs_api_key', $this->request->variable('sfs_api_key', '', true)); - $this->config->set('bh_sfs_allow_http', $this->request->variable('sfs_allow_http', 0)); + + if (function_exists('curl_init')) + { + // Both fields are hidden from the form when cURL isn't + // available (see the template), so they're absent from the + // submitted data - writing them unconditionally would silently + // blank the stored SFS key on any unrelated settings save. + $this->config->set('bh_sfs_api_key', $this->request->variable('sfs_api_key', '', true)); + $this->config->set('bh_sfs_allow_http', $this->request->variable('sfs_allow_http', 0)); + } + $this->config->set('bh_ban_time', $this->request->variable('ban_time', 0)); } /** * Reject a submitted group id that isn't a valid choice: one of the * special groups excluded from the dropdown (get_groups() only hides - * them client-side, a crafted submission can still send their id), or a + * them client-side, a crafted submission can still send their id), a * founder-managed group the current admin isn't allowed to assign users - * into (phpBB's own acp_users.php enforces the same rule). + * into (phpBB's own acp_users.php enforces the same rule), or - when + * $reject_self_service is set - an Open/Free group. * * @param int $group_id + * @param bool $reject_self_service * @return void * @access private */ - private function validate_group($group_id) + private function validate_group($group_id, $reject_self_service = false) { if (!$group_id) { return; } - $sql = 'SELECT group_name, group_founder_manage + $sql = 'SELECT group_name, group_type, group_founder_manage FROM ' . GROUPS_TABLE . ' WHERE group_id = ' . (int) $group_id; $result = $this->db->sql_query($sql); $row = $this->db->sql_fetchrow($result); $this->db->sql_freeresult($result); - if (!$row || in_array($row['group_name'], $this->ignored_groups(), true) || ($this->user->data['user_type'] != USER_FOUNDER && $row['group_founder_manage'])) + if (!$row + || in_array($row['group_name'], $this->ignored_groups(), true) + || ($this->user->data['user_type'] != USER_FOUNDER && $row['group_founder_manage']) + || ($reject_self_service && ($row['group_type'] == GROUP_OPEN || $row['group_type'] == GROUP_FREE))) { trigger_error($this->user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); } From e2ad91e795b6d83d9e12efe1eb71347a1c3c7ae7 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 18:55:38 -0500 Subject: [PATCH 07/16] chore: ignore local review write-ups 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 --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 14ff796..5ce93d4 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,3 @@ vendor output*.txt +*-review-*.md From a1641fa7ea3233f1984072addf92a61bc63fbc6b Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 19:13:09 -0500 Subject: [PATCH 08/16] fix: reject Open/Free restrict groups at use time too, not just save 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 --- event/banhammer_listener.php | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index ed5ae67..f281789 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -501,8 +501,10 @@ public function do_restrict_stuff($event) $restrict_group_id = (int) $this->config['bh_restrict_group_id']; // Re-validated here, not just trusted from ACP-save time: the group - // may have been deleted, or made founder-managed, since then. - if ($restrict_group_id && $this->safe_group_name($restrict_group_id) === '') + // may have been deleted, made founder-managed, or turned into an + // Open/Free group (which would let the restricted user just resign + // via UCP - see includes/ucp/ucp_groups.php) since then. + if ($restrict_group_id && $this->safe_group_name($restrict_group_id, true) === '') { $restrict_group_id = 0; } @@ -665,28 +667,34 @@ protected function active_restriction($user_id) /** * A configured move/restrict group's name, re-validated at the point * it's about to be used rather than trusted from ACP-save time: the - * group may have been deleted, or made founder-managed, since then. + * group may have been deleted, made founder-managed, or (when + * $reject_self_service is set) turned into an Open/Free group, since + * then. * * @param int $group_id - * @return string Group name, or '' if unset, gone, or founder-managed - * and the acting moderator isn't the founder. + * @param bool $reject_self_service + * @return string Group name, or '' if unset, gone, founder-managed and + * the acting moderator isn't the founder, or (when + * $reject_self_service is set) Open/Free. * @access protected */ - protected function safe_group_name($group_id) + protected function safe_group_name($group_id, $reject_self_service = false) { if (!$group_id) { return ''; } - $sql = 'SELECT group_name, group_founder_manage + $sql = 'SELECT group_name, group_type, group_founder_manage FROM ' . GROUPS_TABLE . ' WHERE group_id = ' . (int) $group_id; $result = $this->db->sql_query($sql); $row = $this->db->sql_fetchrow($result); $this->db->sql_freeresult($result); - if (!$row || ($this->user->data['user_type'] != USER_FOUNDER && $row['group_founder_manage'])) + if (!$row + || ($this->user->data['user_type'] != USER_FOUNDER && $row['group_founder_manage']) + || ($reject_self_service && ($row['group_type'] == GROUP_OPEN || $row['group_type'] == GROUP_FREE))) { return ''; } From 1c7d648e0462793e8a61144f13feb5600448a043 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 19:13:33 -0500 Subject: [PATCH 09/16] fix: track whether a restriction created its own group membership 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 --- cron/task/restriction_expiry.php | 10 +- event/banhammer_listener.php | 36 +++++-- migrations/restrict_group_id_column.php | 48 --------- migrations/restrict_membership_column.php | 113 ++++++++++++++++++++++ 4 files changed, 149 insertions(+), 58 deletions(-) create mode 100644 migrations/restrict_membership_column.php diff --git a/cron/task/restriction_expiry.php b/cron/task/restriction_expiry.php index 06a7733..bc0124d 100644 --- a/cron/task/restriction_expiry.php +++ b/cron/task/restriction_expiry.php @@ -67,7 +67,7 @@ public function run() { $this->config->set('bh_restrict_last_run', time(), false); - $sql = 'SELECT restrict_id, user_id, original_group_id, restrict_group_id + $sql = 'SELECT restrict_id, user_id, original_group_id, restrict_group_id, restrict_new_membership FROM ' . $this->restrict_table . ' WHERE restrict_until > 0 AND restrict_until <= ' . time(); @@ -101,7 +101,13 @@ public function run() // user in whatever group they were actually restricted into. $restrict_group_id = (int) $row['restrict_group_id']; - if ($restrict_group_id) + // Only remove group membership the restriction itself created. + // If the user already belonged to the restrict group for an + // unrelated reason (do_restrict_stuff() found GROUP_USERS_EXIST + // and recorded restrict_new_membership = 0), that membership + // predates this restriction and isn't the restriction's to take + // away - only the default-group change and the tracking row are. + if ($restrict_group_id && $row['restrict_new_membership']) { group_user_del($restrict_group_id, array($user_id)); } diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index f281789..696e5da 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -593,6 +593,9 @@ public function do_restrict_stuff($event) 'original_group_id' => $original_group_id, 'restrict_group_id' => $restrict_group_id, 'restrict_until' => $restrict_until, + // Corrected below to 0 if group_user_add() finds the user + // already a member; default of 1 matches the common case. + 'restrict_new_membership' => 1, ); $sql = 'INSERT INTO ' . $this->restrict_table . ' ' . $this->db->sql_build_array('INSERT', $sql_ary); @@ -615,21 +618,38 @@ public function do_restrict_stuff($event) } $result = group_user_add($restrict_group_id, array($user_id), false, false, true); + $group_action_failed = ($result !== false); if ($result === 'GROUP_USERS_EXIST') { // Already a member for some unrelated reason: group_user_add() // returns before setting the default group in that case (see - // the same fix in restriction_expiry's restore path), so set - // it directly instead. - group_user_attributes('default', $restrict_group_id, array($user_id)); + // the same fix in restriction_expiry's restore path), so set it + // directly instead - but only approved members can be made a + // default group (see group_user_attributes()'s 'default' case); + // a pending join request returns NO_USERS and changes nothing, + // which is still a failure to actually apply the restriction. + $group_action_failed = (group_user_attributes('default', $restrict_group_id, array($user_id)) !== false); + + if (!$group_action_failed) + { + // The membership predates this restriction; don't let + // expiry/purge remove it later on this restriction's + // account - only the default-group change and the + // tracking row itself belong to it. + $this->db->sql_query('UPDATE ' . $this->restrict_table . ' + SET restrict_new_membership = 0 + WHERE user_id = ' . $user_id); + } } - else if ($result !== false) + + if ($group_action_failed) { - // NO_USER / GROUP_USERS_INVALID: the group action never took - // effect (shouldn't happen - $user_id comes from the profile - // this event fired for). Don't leave a tracking row behind for - // a restriction that isn't actually in place. + // The group action never took effect - pending membership, + // or NO_USER/GROUP_USERS_INVALID (shouldn't happen; $user_id + // comes from the profile this event fired for). Don't leave a + // tracking row behind for a restriction that isn't actually in + // place. $this->db->sql_query('DELETE FROM ' . $this->restrict_table . ' WHERE user_id = ' . $user_id); return; diff --git a/migrations/restrict_group_id_column.php b/migrations/restrict_group_id_column.php index f1d3c2e..c181d18 100644 --- a/migrations/restrict_group_id_column.php +++ b/migrations/restrict_group_id_column.php @@ -61,18 +61,6 @@ public function update_data() ); } - public function revert_data() - { - return array( - // A purge is about to drop this tracking (this column, then the - // whole table once restrict_group.php itself reverts next). - // Restore anyone still actively restricted now, while the data - // needed to do it correctly is still here, rather than - // stranding them mid-restriction with no way back. - array('custom', array(array($this, 'restore_active_restrictions'))), - ); - } - /** * @return void * @access public @@ -91,40 +79,4 @@ public function backfill_restrict_group_id() WHERE restrict_group_id = 0'; $this->sql_query($sql); } - - /** - * @return void - * @access public - */ - public function restore_active_restrictions() - { - if (!function_exists('group_user_del') || !function_exists('group_user_attributes')) - { - include($this->phpbb_root_path . 'includes/functions_user.' . $this->php_ext); - } - - $sql = 'SELECT user_id, original_group_id, restrict_group_id - FROM ' . $this->table_prefix . 'banhammer_restrict'; - $result = $this->db->sql_query($sql); - - while ($row = $this->db->sql_fetchrow($result)) - { - $user_id = (int) $row['user_id']; - $restrict_group_id = (int) $row['restrict_group_id']; - $original_group_id = (int) $row['original_group_id']; - - if ($restrict_group_id) - { - group_user_del($restrict_group_id, array($user_id)); - } - - if ($original_group_id) - { - group_user_attributes('default', $original_group_id, array($user_id)); - } - } - $this->db->sql_freeresult($result); - - $this->sql_query('DELETE FROM ' . $this->table_prefix . 'banhammer_restrict'); - } } diff --git a/migrations/restrict_membership_column.php b/migrations/restrict_membership_column.php new file mode 100644 index 0000000..1fe3388 --- /dev/null +++ b/migrations/restrict_membership_column.php @@ -0,0 +1,113 @@ +db_tools->sql_column_exists($this->table_prefix . 'banhammer_restrict', 'restrict_new_membership'); + } + + static public function depends_on() + { + return array('\phpbbmodders\banhammer\migrations\restrict_unique_user'); + } + + public function update_schema() + { + return array( + 'add_columns' => array( + $this->table_prefix . 'banhammer_restrict' => array( + // Existing rows predate this column and were all + // created before GROUP_USERS_EXIST was even handled, so + // they always resulted in a fresh membership - default + // of 1 (true) is accurate for them, not just a filler. + 'restrict_new_membership' => array('BOOL', 1), + ), + ), + ); + } + + public function revert_schema() + { + return array( + 'drop_columns' => array( + $this->table_prefix . 'banhammer_restrict' => array( + 'restrict_new_membership', + ), + ), + ); + } + + public function revert_data() + { + return array( + // A purge is about to drop this tracking (this column, then the + // whole table once earlier migrations revert next - migrations + // revert newest-first, so this is the last point the full, + // per-row data is still available). Restore anyone still + // actively restricted now, rather than stranding them + // mid-restriction with no way back. This lives here rather than + // in the older restrict_group_id_column migration because that + // one reverts after this one, by which point + // restrict_new_membership would already be gone. + array('custom', array(array($this, 'restore_active_restrictions'))), + ); + } + + /** + * @return void + * @access public + */ + public function restore_active_restrictions() + { + if (!function_exists('group_user_del') || !function_exists('group_user_attributes')) + { + include($this->phpbb_root_path . 'includes/functions_user.' . $this->php_ext); + } + + $sql = 'SELECT user_id, original_group_id, restrict_group_id, restrict_new_membership + FROM ' . $this->table_prefix . 'banhammer_restrict'; + $result = $this->db->sql_query($sql); + + while ($row = $this->db->sql_fetchrow($result)) + { + $user_id = (int) $row['user_id']; + $restrict_group_id = (int) $row['restrict_group_id']; + $original_group_id = (int) $row['original_group_id']; + + // Only remove membership this restriction actually created - + // see restrict_new_membership's own docs above. + if ($restrict_group_id && $row['restrict_new_membership']) + { + group_user_del($restrict_group_id, array($user_id)); + } + + if ($original_group_id) + { + group_user_attributes('default', $original_group_id, array($user_id)); + } + } + $this->db->sql_freeresult($result); + + $this->sql_query('DELETE FROM ' . $this->table_prefix . 'banhammer_restrict'); + } +} From eee5fff11de8b37dd9115414247b4394d3dd2415 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 19:13:53 -0500 Subject: [PATCH 10/16] fix: stop del_posts cleanup when the target simply has no posts 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 --- event/banhammer_listener.php | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index 696e5da..3dba657 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -814,8 +814,6 @@ private function bh_del_posts() // posts in forums the acting moderator could delete in anyway. if (!$this->auth->acl_get('m_banhammer_del_posts_all')) { - $had_posts = !empty($posts); - foreach ($posts as $post_id => $post_row) { if (!$this->auth->acl_get('m_delete', (int) $post_row['forum_id'])) @@ -828,12 +826,16 @@ private function bh_del_posts() } } - if ($had_posts && empty($posts)) + if (empty($posts)) { - // There were posts to consider, but no permission to touch - // any of them: stop here rather than still wiping unrelated - // account data (bookmarks, drafts, notifications, ...) - // below for an action that had no actual effect on posts. + // No posts left to delete - either there were none to begin + // with, or permission filtered out all of them. Either way, + // stop here rather than still wiping unrelated account data + // (bookmarks, drafts, notifications, ...) below for a + // del_posts request with no legitimate post-deletion + // authority behind it at all. A moderator with authority + // over at least one of the target's posts still gets the + // full account cleanup, same as always. return; } } From b79967d4012c157f89f1f7a6d26da72814349864 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 19:14:10 -0500 Subject: [PATCH 11/16] fix: deduplicate leftover restriction rows before adding the unique index 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 --- migrations/restrict_dedupe.php | 64 +++++++++++++++++++++++++++++ migrations/restrict_unique_user.php | 6 ++- 2 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 migrations/restrict_dedupe.php diff --git a/migrations/restrict_dedupe.php b/migrations/restrict_dedupe.php new file mode 100644 index 0000000..b0b9876 --- /dev/null +++ b/migrations/restrict_dedupe.php @@ -0,0 +1,64 @@ +table_prefix . 'banhammer_restrict + GROUP BY user_id + HAVING COUNT(*) > 1'; + $result = $this->db->sql_query($sql); + + while ($row = $this->db->sql_fetchrow($result)) + { + $sql = 'DELETE FROM ' . $this->table_prefix . 'banhammer_restrict + WHERE user_id = ' . (int) $row['user_id'] . ' + AND restrict_id <> ' . (int) $row['keep_id']; + $this->sql_query($sql); + } + $this->db->sql_freeresult($result); + } +} diff --git a/migrations/restrict_unique_user.php b/migrations/restrict_unique_user.php index a0f74e0..da6d79b 100644 --- a/migrations/restrict_unique_user.php +++ b/migrations/restrict_unique_user.php @@ -26,7 +26,11 @@ public function effectively_installed() static public function depends_on() { - return array('\phpbbmodders\banhammer\migrations\permission_del_posts_all'); + // restrict_dedupe removes any leftover duplicate user_id rows from + // the concurrent-restriction race this index closes off; without + // running first, creating a unique index over pre-existing + // duplicates would fail outright. + return array('\phpbbmodders\banhammer\migrations\restrict_dedupe'); } public function update_schema() From be9676c22815e83022ba3be4646ea2826089f42a Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 19:14:26 -0500 Subject: [PATCH 12/16] fix: hide Open/Free groups from the restrict-group dropdown 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 --- controller/admin_controller.php | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/controller/admin_controller.php b/controller/admin_controller.php index 4f7d7e6..955659e 100644 --- a/controller/admin_controller.php +++ b/controller/admin_controller.php @@ -109,7 +109,7 @@ public function display_options() 'DEL_PROFILE' => (!empty($this->config['bh_del_profile'])) ? true : false, 'DEL_SIGNATURE' => (!empty($this->config['bh_del_signature'])) ? true : false, 'MOVE_GROUP' => $this->get_groups($this->request->variable('move_group', $this->config['bh_group_id'])), - 'RESTRICT_GROUP' => $this->get_groups($this->request->variable('restrict_group', $this->config['bh_restrict_group_id'])), + 'RESTRICT_GROUP' => $this->get_groups($this->request->variable('restrict_group', $this->config['bh_restrict_group_id']), true), 'SFS_ALLOW_HTTP' => (!empty($this->config['bh_sfs_allow_http'])) ? true : false, 'SFS_API_KEY' => (!empty($this->config['bh_sfs_api_key'])) ? $this->config['bh_sfs_api_key'] : '', 'SFS_CURL' => (function_exists('curl_init')) ? true : false, @@ -208,8 +208,14 @@ private function ignored_groups() /** * function to return groups that are allowed + * + * @param int $group_selected + * @param bool $reject_self_service Hide Open/Free groups too (see + * validate_group()); used for the + * restrict-group dropdown, not the + * ban move-to-group one. */ - private function get_groups($group_selected) + private function get_groups($group_selected, $reject_self_service = false) { $this->user->add_lang('acp/groups'); @@ -234,6 +240,13 @@ private function get_groups($group_selected) continue; } + // Open/Free groups let a member resign unilaterally via UCP, + // which would let a restricted user just leave the restriction. + if ($reject_self_service && ($row['group_type'] == GROUP_OPEN || $row['group_type'] == GROUP_FREE)) + { + continue; + } + $selected = ($row['group_id'] == $group_selected) ? ' selected="selected"' : ''; $group_name = ($row['group_type'] == GROUP_SPECIAL) ? $this->user->lang['G_' . $row['group_name']] : $row['group_name']; $s_group_options .= ""; From aa5e8796755354cf86254f1cf877468c2ad350ea Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 19:33:22 -0500 Subject: [PATCH 13/16] fix: correct legacy-row default and add a purge fallback for restrict_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 --- migrations/restrict_group_id_column.php | 64 +++++++++++++++++++++++ migrations/restrict_membership_column.php | 16 ++++-- 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/migrations/restrict_group_id_column.php b/migrations/restrict_group_id_column.php index c181d18..843da2d 100644 --- a/migrations/restrict_group_id_column.php +++ b/migrations/restrict_group_id_column.php @@ -61,6 +61,24 @@ public function update_data() ); } + public function revert_data() + { + return array( + // Normally a no-op: restrict_membership_column's own + // revert_data() already restores every active restriction and + // empties this table before this migration's own revert runs + // (migrations revert newest-first). This is only a safety net + // for a purge that happens before restrict_membership_column + // was ever installed - e.g. the extension's files were + // upgraded to include it, then the extension was disabled and + // purged without being re-enabled first to actually run it. + // phpBB's migrator only reverts migrations recorded as + // installed, so a never-installed migration's revert_data() + // simply never runs. + array('custom', array(array($this, 'restore_active_restrictions_fallback'))), + ); + } + /** * @return void * @access public @@ -79,4 +97,50 @@ public function backfill_restrict_group_id() WHERE restrict_group_id = 0'; $this->sql_query($sql); } + + /** + * @return void + * @access public + */ + public function restore_active_restrictions_fallback() + { + if (!function_exists('group_user_del') || !function_exists('group_user_attributes')) + { + include($this->phpbb_root_path . 'includes/functions_user.' . $this->php_ext); + } + + // restrict_new_membership may or may not exist depending on + // whether restrict_membership_column ran - see revert_data() above. + $has_membership_column = $this->db_tools->sql_column_exists($this->table_prefix . 'banhammer_restrict', 'restrict_new_membership'); + + $sql = 'SELECT user_id, original_group_id, restrict_group_id' + . ($has_membership_column ? ', restrict_new_membership' : '') . ' + FROM ' . $this->table_prefix . 'banhammer_restrict'; + $result = $this->db->sql_query($sql); + + while ($row = $this->db->sql_fetchrow($result)) + { + $user_id = (int) $row['user_id']; + $restrict_group_id = (int) $row['restrict_group_id']; + $original_group_id = (int) $row['original_group_id']; + + // Without restrict_new_membership there's no way to tell + // whether this restriction created the membership or found it + // pre-existing; conservatively leave it alone rather than risk + // stripping a membership this restriction never granted (same + // reasoning as restrict_membership_column's own default). + if ($restrict_group_id && $has_membership_column && $row['restrict_new_membership']) + { + group_user_del($restrict_group_id, array($user_id)); + } + + if ($original_group_id) + { + group_user_attributes('default', $original_group_id, array($user_id)); + } + } + $this->db->sql_freeresult($result); + + $this->sql_query('DELETE FROM ' . $this->table_prefix . 'banhammer_restrict'); + } } diff --git a/migrations/restrict_membership_column.php b/migrations/restrict_membership_column.php index 1fe3388..1f9e17f 100644 --- a/migrations/restrict_membership_column.php +++ b/migrations/restrict_membership_column.php @@ -36,11 +36,17 @@ public function update_schema() return array( 'add_columns' => array( $this->table_prefix . 'banhammer_restrict' => array( - // Existing rows predate this column and were all - // created before GROUP_USERS_EXIST was even handled, so - // they always resulted in a fresh membership - default - // of 1 (true) is accurate for them, not just a filler. - 'restrict_new_membership' => array('BOOL', 1), + // Existing rows predate this column. Before this fix's + // own group_user_add() result check existed, the + // tracking row was inserted unconditionally regardless + // of whether the user was already a member - so a + // legacy row could represent either case, and there's + // no way to tell which after the fact. Default to 0 + // (don't remove membership): leaving a genuinely + // ban-hammer-created membership in place a little too + // long is a much smaller problem than stripping a + // membership this restriction never granted. + 'restrict_new_membership' => array('BOOL', 0), ), ), ); From cd7e2d1c91a9b778fe2d3709bbcfb73c6c182ef2 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 19:33:41 -0500 Subject: [PATCH 14/16] fix: apply the empty-posts guard unconditionally, stop wiping topics_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 --- event/banhammer_listener.php | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index 3dba657..d5d1895 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -825,19 +825,19 @@ private function bh_del_posts() unset($posts[$post_id]); } } + } - if (empty($posts)) - { - // No posts left to delete - either there were none to begin - // with, or permission filtered out all of them. Either way, - // stop here rather than still wiping unrelated account data - // (bookmarks, drafts, notifications, ...) below for a - // del_posts request with no legitimate post-deletion - // authority behind it at all. A moderator with authority - // over at least one of the target's posts still gets the - // full account cleanup, same as always. - return; - } + if (empty($posts)) + { + // No posts to delete - either there were none to begin with, or + // permission filtered out all of them. Either way, stop here + // rather than still wiping unrelated account data (bookmarks, + // drafts, notifications, ...) below for a del_posts request + // that has nothing to actually act on, regardless of which + // permission granted access. A moderator whose authority + // covers at least one of the target's posts still gets the + // full account cleanup, same as always. + return; } // And now handle the reports. @@ -901,7 +901,12 @@ private function bh_del_posts() // user_delete() (which doesn't touch POLL_VOTES_TABLE either): // removing them here without decrementing poll_option_total would // corrupt the poll's totals and let the user vote again later. - $this->db->sql_query('DELETE FROM ' . TOPICS_POSTED_TABLE . " WHERE user_id = $user_id"); + // TOPICS_POSTED_TABLE is also deliberately left alone: delete_posts() + // above already calls update_posted_info() to rebuild it correctly + // for the topics it actually touched. A blanket delete here would + // also wipe the "posted in this topic" marker for topics where the + // user's post survived (not deletable in this pass), which + // delete_posts() never touched and has no reason to be wrong. $this->db->sql_query('DELETE FROM ' . TOPICS_TRACK_TABLE . " WHERE user_id = $user_id"); $this->db->sql_query('DELETE FROM ' . TOPICS_WATCH_TABLE . " WHERE user_id = $user_id"); $this->db->sql_query('DELETE FROM ' . USER_NOTIFICATIONS_TABLE . " WHERE user_id = $user_id"); From 3dbc19d39935342f2349f081ed4ce837847ae025 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 20:12:04 -0500 Subject: [PATCH 15/16] fix: give bans per-action group tracking, same as restrictions 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 --- config/services.yml | 1 + event/banhammer_listener.php | 140 +++++++++++++++++++++------- migrations/ban_group.php | 173 +++++++++++++++++++++++++++++++++++ 3 files changed, 281 insertions(+), 33 deletions(-) create mode 100644 migrations/ban_group.php diff --git a/config/services.yml b/config/services.yml index de8e65b..3545d57 100644 --- a/config/services.yml +++ b/config/services.yml @@ -14,6 +14,7 @@ services: - '%core.php_ext%' - '@service_container' - '%core.table_prefix%banhammer_restrict' + - '%core.table_prefix%banhammer_ban_group' tags: - { name: event.listener } phpbbmodders.banhammer.admin.controller: diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index d5d1895..35c0989 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -67,6 +67,9 @@ class banhammer_listener implements EventSubscriberInterface /** @var string */ protected $restrict_table; + /** @var string */ + protected $ban_group_table; + public function __construct( \phpbb\auth\auth $auth, \phpbb\cache\driver\driver_interface $cache, @@ -79,7 +82,8 @@ public function __construct( $root_path, $phpExt, ContainerInterface $container, - $restrict_table + $restrict_table, + $ban_group_table ) { $this->auth = $auth; @@ -94,6 +98,7 @@ public function __construct( $this->php_ext = $phpExt; $this->container = $container; $this->restrict_table = $restrict_table; + $this->ban_group_table = $ban_group_table; } static public function getSubscribedEvents() @@ -437,12 +442,47 @@ public function do_ban_hammer_stuff($event) if ($this->request->variable('move_group', 0) && !empty($group_name)) { - $return = group_user_add($this->config['bh_group_id'], array($this->user_id), array($this->data['username']), $group_name, true); + $move_group_id = (int) $this->config['bh_group_id']; + $return = group_user_add($move_group_id, array($this->user_id), array($this->data['username']), $group_name, true); + $move_new_membership = 1; - if ($return != false) + if ($return === 'GROUP_USERS_EXIST') + { + // Already a member for some unrelated reason: group_user_add() + // returns before setting the default group in that case, so + // set it directly instead (same fix as do_restrict_stuff()'s + // equivalent case). + $return = group_user_attributes('default', $move_group_id, array($this->user_id)); + $move_new_membership = 0; + } + + if ($return !== false) { $error[] = 'ERROR_MOVE_GROUP'; } + else + { + // Record which group this ban actually moved them into (and + // whether that membership was newly created or pre-existing), + // and their default group beforehand, so undo_bh_group() can + // clean up precisely this action later instead of comparing + // against the *current* ACP setting. + $sql_ary = array( + 'user_id' => $this->user_id, + 'original_group_id' => (int) $this->data['group_id'], + 'move_group_id' => $move_group_id, + 'move_new_membership' => $move_new_membership, + ); + $sql = 'INSERT INTO ' . $this->ban_group_table . ' ' . $this->db->sql_build_array('INSERT', $sql_ary); + + // A second ban+move on the same already-banned user is + // blocked well before this point (see the banned-user check + // above), but guard the unique index anyway rather than let + // a genuine race surface as an uncaught SQL error. + $this->db->sql_return_on_error(true); + $this->db->sql_query($sql); + $this->db->sql_return_on_error(false); + } } if ($this->request->variable('sfs_report', 0) && !empty($this->config['bh_sfs_api_key']) && $curl_exists) @@ -684,6 +724,25 @@ protected function active_restriction($user_id) return ($row) ?: null; } + /** + * The group-move tracking row for a user's currently active ban, if any. + * + * @param int $user_id + * @return array|null The tracking row, or null when there is none. + * @access protected + */ + protected function active_ban_group($user_id) + { + $sql = 'SELECT ban_id, original_group_id, move_group_id, move_new_membership + FROM ' . $this->ban_group_table . ' + WHERE user_id = ' . (int) $user_id; + $result = $this->db->sql_query_limit($sql, 1); + $row = $this->db->sql_fetchrow($result); + $this->db->sql_freeresult($result); + + return ($row) ?: null; + } + /** * A configured move/restrict group's name, re-validated at the point * it's about to be used rather than trusted from ACP-save time: the @@ -722,43 +781,58 @@ protected function safe_group_name($group_id, $reject_self_service = false) return $row['group_name']; } - // Once a ban is cleared try and remove the user from the banned group set in the ACP of the extension + /** + * Once a ban is cleared, undo whatever group move that specific ban + * actually made - not "the group currently configured in the ACP", + * which may have changed since, or matter to some other, unrelated + * group membership entirely. + * + * @param \phpbb\event\data $event The event object + * @return void + * @access public + */ public function undo_bh_group($event) { - if (!empty($this->config['bh_group_id']) && !$event['banned'] && $this->user->data['user_type'] != USER_IGNORE) + if ($event['banned'] || $this->user->data['user_type'] == USER_IGNORE) { - // determine if the user is in the ban hammer group set in the ACP - $sql = 'SELECT group_id FROM ' . USER_GROUP_TABLE . ' - WHERE group_id = ' . (int) $this->config['bh_group_id'] . ' - AND user_id = ' . (int) $this->user->data['user_id']; - $result = $this->db->sql_query($sql); - $group_id = $this->db->sql_fetchfield('group_id'); - $this->db->sql_freeresult($result); + return; + } - if ($group_id) - { - // A restricted (not banned) user deliberately sits in - // whatever group their own restriction actually used, so - // leave that membership alone rather than undoing it here - - // checked against the restriction's own recorded group, not - // the current ACP settings, which may have changed since - // (comparing current settings would stop protecting an - // already-active restriction the moment either is edited). - $restriction = $this->active_restriction($this->user->data['user_id']); - - if ($restriction !== null && (int) $restriction['restrict_group_id'] === (int) $this->config['bh_group_id']) - { - return; - } + $ban_group = $this->active_ban_group($this->user->data['user_id']); - // Remove the user from the banned group set in the ACP - if (!function_exists('group_user_del')) - { - include($this->root_path . 'includes/functions_user.' . $this->php_ext); - } - group_user_del($this->config['bh_group_id'], array($this->user->data['user_id'])); + if ($ban_group === null) + { + return; + } + + if (!function_exists('group_user_del') || !function_exists('group_user_attributes')) + { + include($this->root_path . 'includes/functions_user.' . $this->php_ext); + } + + $move_group_id = (int) $ban_group['move_group_id']; + + if ($move_group_id && $ban_group['move_new_membership']) + { + // A restriction can be configured to use the same group as a + // ban's move-to group. If this user also has an active + // restriction recorded against this exact group, leave their + // membership alone - it's still needed for the restriction, + // checked against its own recorded group, not current config. + $restriction = $this->active_restriction($this->user->data['user_id']); + + if ($restriction === null || (int) $restriction['restrict_group_id'] !== $move_group_id) + { + group_user_del($move_group_id, array($this->user->data['user_id'])); } } + + if ($ban_group['original_group_id']) + { + group_user_attributes('default', (int) $ban_group['original_group_id'], array($this->user->data['user_id'])); + } + + $this->db->sql_query('DELETE FROM ' . $this->ban_group_table . ' WHERE ban_id = ' . (int) $ban_group['ban_id']); } private function bh_del_privmsgs() diff --git a/migrations/ban_group.php b/migrations/ban_group.php new file mode 100644 index 0000000..cb03918 --- /dev/null +++ b/migrations/ban_group.php @@ -0,0 +1,173 @@ +db_tools->sql_table_exists($this->table_prefix . 'banhammer_ban_group'); + } + + static public function depends_on() + { + return array('\phpbbmodders\banhammer\migrations\restrict_membership_column'); + } + + public function update_schema() + { + return array( + 'add_tables' => array( + $this->table_prefix . 'banhammer_ban_group' => array( + 'COLUMNS' => array( + 'ban_id' => array('UINT', null, 'auto_increment'), + 'user_id' => array('UINT', 0), + 'original_group_id' => array('UINT', 0), + 'move_group_id' => array('UINT', 0), + 'move_new_membership' => array('BOOL', 0), + ), + 'PRIMARY_KEY' => 'ban_id', + 'KEYS' => array( + 'user_id' => array('UNIQUE', 'user_id'), + ), + ), + ), + ); + } + + public function revert_schema() + { + return array( + 'drop_tables' => array( + $this->table_prefix . 'banhammer_ban_group', + ), + ); + } + + public function update_data() + { + return array( + // A user already banned-and-moved under the old code has no + // tracking row (the table didn't exist yet), so undo_bh_group() + // would never clean them up once unbanned. Back-fill one for + // anyone currently banned and a member of the currently + // configured move-to group. + array('custom', array(array($this, 'backfill_active_bans'))), + ); + } + + /** + * @return void + * @access public + */ + public function backfill_active_bans() + { + $move_group_id = (int) $this->config['bh_group_id']; + + if (!$move_group_id) + { + return; + } + + // There's no record of what these users' default group was before + // being banned (original_group_id = 0, so undo_bh_group() won't + // attempt to restore one), and no way to tell whether the ban + // itself put them in this group or they already belonged for an + // unrelated reason - conservatively assume the latter + // (move_new_membership = 0), same reasoning as + // restrict_membership_column's own legacy default: leaving a + // genuinely ban-created membership in place is a much smaller + // problem than stripping one the ban never granted. + $sql = 'SELECT DISTINCT ug.user_id + FROM ' . USER_GROUP_TABLE . ' ug, ' . BANLIST_TABLE . ' b + WHERE ug.group_id = ' . $move_group_id . ' + AND b.ban_userid = ug.user_id + AND b.ban_exclude = 0 + AND (b.ban_end = 0 OR b.ban_end > ' . time() . ')'; + $result = $this->db->sql_query($sql); + + $sql_ary = array(); + while ($row = $this->db->sql_fetchrow($result)) + { + $sql_ary[] = array( + 'user_id' => (int) $row['user_id'], + 'original_group_id' => 0, + 'move_group_id' => $move_group_id, + 'move_new_membership' => 0, + ); + } + $this->db->sql_freeresult($result); + + if (!empty($sql_ary)) + { + $this->db->sql_multi_insert($this->table_prefix . 'banhammer_ban_group', $sql_ary); + } + } + + public function revert_data() + { + return array( + // A purge is about to drop this tracking. Restore anyone still + // actively tracked now, rather than stranding them in whatever + // group a ban moved them into with no way back. + array('custom', array(array($this, 'restore_active_ban_groups'))), + ); + } + + /** + * @return void + * @access public + */ + public function restore_active_ban_groups() + { + if (!function_exists('group_user_del') || !function_exists('group_user_attributes')) + { + include($this->phpbb_root_path . 'includes/functions_user.' . $this->php_ext); + } + + $sql = 'SELECT user_id, original_group_id, move_group_id, move_new_membership + FROM ' . $this->table_prefix . 'banhammer_ban_group'; + $result = $this->db->sql_query($sql); + + while ($row = $this->db->sql_fetchrow($result)) + { + $user_id = (int) $row['user_id']; + $move_group_id = (int) $row['move_group_id']; + $original_group_id = (int) $row['original_group_id']; + + if ($move_group_id && $row['move_new_membership']) + { + group_user_del($move_group_id, array($user_id)); + } + + if ($original_group_id) + { + group_user_attributes('default', $original_group_id, array($user_id)); + } + } + $this->db->sql_freeresult($result); + + $this->sql_query('DELETE FROM ' . $this->table_prefix . 'banhammer_ban_group'); + } +} From 6cf891ad4aeb392cb8853cbe45cb167291a39670 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Sun, 27 Sep 2026 21:10:16 -0500 Subject: [PATCH 16/16] Update the restriction expiry test for restrict_new_membership #37's fixture predates the restrict_new_membership column this branch adds, so both rows defaulted to 0 and the cron task would no longer remove user 2 from the restrict group, failing the test. Mark users 2 and 3 as new memberships, and add a case for the new behaviour: a user who was already in the restrict group keeps that membership when the restriction expires, while the default group is restored and the tracking row removed. Co-Authored-By: Claude Opus 5.5 --- tests/cron/fixtures/restriction_expiry.xml | 41 ++++++++++++++++++++++ tests/cron/restriction_expiry_test.php | 8 +++++ 2 files changed, 49 insertions(+) diff --git a/tests/cron/fixtures/restriction_expiry.xml b/tests/cron/fixtures/restriction_expiry.xml index 217d8b2..de28762 100644 --- a/tests/cron/fixtures/restriction_expiry.xml +++ b/tests/cron/fixtures/restriction_expiry.xml @@ -10,6 +10,14 @@ original group, 7. - user 3: restriction NOT expired (restrict_until far in the future) - must be left completely alone. + - user 4: restriction expired, but user 4 was already a member of + the restrict group (8) before being restricted + (restrict_new_membership = 0) - that membership must be kept; + only the default group is restored (to 7) and the tracking row + removed. + + Users 2 and 3 were added to their restrict group by the + restriction itself, so restrict_new_membership = 1. --> group_id @@ -95,6 +103,16 @@ + + 4 + 8 + + 0 + existing member user + existingmemberuser + + +
user_id @@ -125,6 +143,18 @@ 00 + + 4 + 7 + 0 + 0 + + + 4 + 8 + 0 + 0 +
restrict_id @@ -132,12 +162,14 @@ original_group_idrestrict_group_idrestrict_until + restrict_new_membership 1 2 7 8 1 + 1 2 @@ -145,6 +177,15 @@ 10 9 2000000000 + 1 + + + 3 + 4 + 7 + 8 + 1 + 0
diff --git a/tests/cron/restriction_expiry_test.php b/tests/cron/restriction_expiry_test.php index 520e7b5..f4125a6 100644 --- a/tests/cron/restriction_expiry_test.php +++ b/tests/cron/restriction_expiry_test.php @@ -107,6 +107,14 @@ public function test_expiry_restores_expired_restriction_only() $this->assertTrue($this->is_group_member($db, 3, 9), 'User 3 should still be in their restrict group'); $this->assertEquals(9, $this->get_default_group($db, 3), "User 3's default group should be unchanged"); $this->assertEquals(1, $this->count_restrict_rows($db, 3), "User 3's tracking row should remain"); + + // User 4's restriction expired, but they were already in the + // restrict group beforehand (restrict_new_membership = 0): keep that + // membership, restore only the default group and drop the row. + $this->assertTrue($this->is_group_member($db, 4, 8), 'User 4 should keep their pre-existing restrict group membership'); + $this->assertTrue($this->is_group_member($db, 4, 7), 'User 4 should still be in their original group'); + $this->assertEquals(7, $this->get_default_group($db, 4), "User 4's default group should be restored to their original group"); + $this->assertEquals(0, $this->count_restrict_rows($db, 4), "User 4's tracking row should be gone"); } protected function is_group_member($db, $user_id, $group_id)