From 83d4025d48103d8b27fbd50c2ab5cc53e53c9048 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 16:41:10 -0500 Subject: [PATCH 1/9] fix(security): stop cancel=1 from bypassing ban/restrict confirmation 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 --- event/banhammer_listener.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index ae8a4c8..4947d0e 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -341,6 +341,11 @@ public function do_ban_hammer_stuff($event) $message .= ($hidden_fields['sfs_report'] && $curl_exists) ? $this->user->lang['BH_SUBMIT_SFS'] . '
' : ''; confirm_box(false, $message, build_hidden_fields($hidden_fields)); + + // confirm_box(false, ...) above only returns instead of exiting + // when the request was actually a cancellation (POST 'cancel'), + // in which case we must not fall through to the ban below. + return; } // We have a user to ban. @@ -538,6 +543,11 @@ public function do_restrict_stuff($event) $message .= ($length) ? '

' . $this->user->lang('BH_RESTRICT_FOR', $length) : '

' . $this->user->lang['BH_RESTRICT_PERM']; confirm_box(false, $message, $hidden_fields); + + // confirm_box(false, ...) above only returns instead of exiting + // when the request was actually a cancellation (POST 'cancel'), + // in which case we must not fall through to the restriction below. + return; } if (!function_exists('group_user_add')) From 94bc0a12aa6eadb131dbf3ecd0acab82794149ed Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 16:41:18 -0500 Subject: [PATCH 2/9] fix(security): validate ACP move/restrict group selections 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 --- controller/admin_controller.php | 63 ++++++++++++++++++++++++++++++--- 1 file changed, 59 insertions(+), 4 deletions(-) diff --git a/controller/admin_controller.php b/controller/admin_controller.php index abcc457..7c7e4a0 100644 --- a/controller/admin_controller.php +++ b/controller/admin_controller.php @@ -126,6 +126,12 @@ public function display_options() */ protected function set_options() { + $move_group = $this->request->variable('move_group', 0); + $restrict_group = $this->request->variable('restrict_group', 0); + + $this->validate_group($move_group); + $this->validate_group($restrict_group); + $this->config->set('bh_ban_email', $this->request->variable('ban_email', 0)); $this->config->set('bh_ban_ip', $this->request->variable('ban_ip', 0)); $this->config->set('bh_del_avatar', $this->request->variable('del_avatar', 0)); @@ -133,13 +139,55 @@ protected function set_options() $this->config->set('bh_del_posts', $this->request->variable('del_posts', 0)); $this->config->set('bh_del_profile', $this->request->variable('del_profile', 0)); $this->config->set('bh_del_signature', $this->request->variable('del_signature', 0)); - $this->config->set('bh_group_id', $this->request->variable('move_group', 0)); - $this->config->set('bh_restrict_group_id', $this->request->variable('restrict_group', 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)); $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 + * founder-managed group the current admin isn't allowed to assign users + * into (phpBB's own acp_users.php enforces the same rule). + * + * @param int $group_id + * @return void + * @access private + */ + private function validate_group($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 || in_array($row['group_name'], $this->ignored_groups(), true) || ($this->user->data['user_type'] != USER_FOUNDER && $row['group_founder_manage'])) + { + trigger_error($this->user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } + } + + /** + * Special groups no admin should be moving/restricting users into. + * + * @return array + * @access private + */ + private function ignored_groups() + { + return array('BOTS', 'GUESTS', 'REGISTERED', 'REGISTERED_COPPA', 'NEWLY_REGISTERED', 'ADMINISTRATORS', 'GLOBAL_MODERATORS'); + } + /** * function to return groups that are allowed */ @@ -149,9 +197,9 @@ private function get_groups($group_selected) // Don't display any of the default groups // highly doubt an admin would want to ban someone into a default group - $ignore_groups = array('BOTS', 'GUESTS', 'REGISTERED', 'REGISTERED_COPPA', 'NEWLY_REGISTERED', 'ADMINISTRATORS', 'GLOBAL_MODERATORS'); + $ignore_groups = $this->ignored_groups(); - $sql = 'SELECT group_name, group_id, group_type + $sql = 'SELECT group_name, group_id, group_type, group_founder_manage FROM ' . GROUPS_TABLE . ' WHERE ' . $this->db->sql_in_set('group_name', $ignore_groups, true) . ' ORDER BY group_name ASC'; @@ -161,6 +209,13 @@ private function get_groups($group_selected) $s_group_options = ""; while ($row = $this->db->sql_fetchrow($result)) { + // Same rule as phpBB's own acp_users.php: don't offer a group a + // non-founder isn't allowed to manage. + if ($this->user->data['user_type'] != USER_FOUNDER && $row['group_founder_manage']) + { + 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 672588f740024d0864ae89e5534313b7e55faa99 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 16:41:41 -0500 Subject: [PATCH 3/9] fix(security): require explicit permission for cross-forum post deletion 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 --- event/banhammer_listener.php | 18 +++++++++ language/en/permissions_banhammer.php | 29 ++++++++++++++ migrations/permission_del_posts_all.php | 53 +++++++++++++++++++++++++ 3 files changed, 100 insertions(+) create mode 100644 language/en/permissions_banhammer.php create mode 100644 migrations/permission_del_posts_all.php diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index 4947d0e..bbe4bd0 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -682,6 +682,24 @@ private function bh_del_posts() } $this->db->sql_freeresult($result); + // m_ban alone doesn't grant delete rights in every forum; only the + // extension's own explicit permission does. Without it, only touch + // posts in forums the acting moderator could delete in anyway. + if (!$this->auth->acl_get('m_banhammer_del_posts_all')) + { + foreach ($posts as $post_id => $post_row) + { + if (!$this->auth->acl_get('m_delete', (int) $post_row['forum_id'])) + { + // Only gates $posts: the report-closing loop below only + // ever reads $topics through a $posts[$post_id] lookup, + // so leaving a stale count here for a topic we're no + // longer touching has no effect. + unset($posts[$post_id]); + } + } + } + // And now handle the reports. $sql = 'SELECT report_id, post_id, report_closed FROM ' . REPORTS_TABLE . ' diff --git a/language/en/permissions_banhammer.php b/language/en/permissions_banhammer.php new file mode 100644 index 0000000..9ecfebc --- /dev/null +++ b/language/en/permissions_banhammer.php @@ -0,0 +1,29 @@ + 'Can delete a banned user\'s posts in every forum via Ban Hammer, not just forums they can otherwise delete posts in', +)); diff --git a/migrations/permission_del_posts_all.php b/migrations/permission_del_posts_all.php new file mode 100644 index 0000000..7961276 --- /dev/null +++ b/migrations/permission_del_posts_all.php @@ -0,0 +1,53 @@ +db->sql_query($sql); + $row = $this->db->sql_fetchrow($result); + $this->db->sql_freeresult($result); + + return (bool) $row; + } + + static public function depends_on() + { + return array('\phpbbmodders\banhammer\migrations\restrict_group_id_column'); + } + + public function update_data() + { + return array( + array('permission.add', array('m_banhammer_del_posts_all', true, 'm_ban')), + ); + } + + public function revert_data() + { + return array( + array('permission.remove', array('m_banhammer_del_posts_all', true)), + ); + } +} From 95ae4bdec757e5f0bb179c4d3f1f25a7140af427 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 16:41:49 -0500 Subject: [PATCH 4/9] fix: load functions_user.php before user_ban() in the domain-ban controller 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 --- config/services.yml | 2 ++ controller/ban_domain_controller.php | 19 ++++++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/config/services.yml b/config/services.yml index 1163fef..de8e65b 100644 --- a/config/services.yml +++ b/config/services.yml @@ -37,6 +37,8 @@ services: - '@language' - '@request' - '@user' + - '%core.root_path%' + - '%core.php_ext%' phpbbmodders.banhammer.cron.restriction_expiry: class: phpbbmodders\banhammer\cron\task\restriction_expiry arguments: diff --git a/controller/ban_domain_controller.php b/controller/ban_domain_controller.php index 446f788..5315deb 100644 --- a/controller/ban_domain_controller.php +++ b/controller/ban_domain_controller.php @@ -35,6 +35,12 @@ class ban_domain_controller /** @var \phpbb\user */ protected $user; + /** @var string phpBB root path */ + protected $root_path; + + /** @var string phpEx */ + protected $php_ext; + /** * Constructor * @@ -42,19 +48,25 @@ class ban_domain_controller * @param \phpbb\language\language $language Language object * @param \phpbb\request\request $request Request object * @param \phpbb\user $user User object + * @param string $root_path phpBB root path + * @param string $php_ext PHP file extension * @access public */ public function __construct( \phpbb\auth\auth $auth, \phpbb\language\language $language, \phpbb\request\request $request, - \phpbb\user $user + \phpbb\user $user, + $root_path, + $php_ext ) { $this->auth = $auth; $this->language = $language; $this->request = $request; $this->user = $user; + $this->root_path = $root_path; + $this->php_ext = $php_ext; } /** @@ -93,6 +105,11 @@ public function handle() { $reason = $this->request->variable('bh_reason', '', true); + if (!function_exists('user_ban')) + { + include($this->root_path . 'includes/functions_user.' . $this->php_ext); + } + $success = user_ban('email', $ban_pattern, 0, '', false, $reason); if (!$success) From 971655ce6d9dd39c23185f73e21e363f2c17ac37 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 16:42:17 -0500 Subject: [PATCH 5/9] fix: correct restriction lifecycle bugs around group changes 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 --- cron/task/restriction_expiry.php | 17 +++- event/banhammer_listener.php | 10 ++ migrations/restrict_group_id_column.php | 130 ++++++++++++++++++++++++ 3 files changed, 152 insertions(+), 5 deletions(-) create mode 100644 migrations/restrict_group_id_column.php diff --git a/cron/task/restriction_expiry.php b/cron/task/restriction_expiry.php index 3ed28e8..06a7733 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 + $sql = 'SELECT restrict_id, user_id, original_group_id, restrict_group_id FROM ' . $this->restrict_table . ' WHERE restrict_until > 0 AND restrict_until <= ' . time(); @@ -86,17 +86,20 @@ public function run() return; } - if (!function_exists('group_user_add') || !function_exists('group_user_del')) + if (!function_exists('group_user_del') || !function_exists('group_user_attributes')) { include($this->root_path . 'includes/functions_user.' . $this->php_ext); } - $restrict_group_id = (int) $this->config['bh_restrict_group_id']; - foreach ($expired as $row) { $user_id = (int) $row['user_id']; $original_group_id = (int) $row['original_group_id']; + // The group actually applied when this restriction was created, + // not the current ACP setting: an admin may have changed it + // since, and removing the wrong (or no) group would strand the + // user in whatever group they were actually restricted into. + $restrict_group_id = (int) $row['restrict_group_id']; if ($restrict_group_id) { @@ -105,7 +108,11 @@ public function run() if ($original_group_id) { - group_user_add($original_group_id, array($user_id), false, false, true); + // Not group_user_add(..., true): the user was never removed + // from their original group while restricted, so it would + // see them as already a member and return GROUP_USERS_EXIST + // before reaching the code that sets the default group. + group_user_attributes('default', $original_group_id, array($user_id)); } $this->db->sql_query('DELETE FROM ' . $this->restrict_table . ' WHERE restrict_id = ' . (int) $row['restrict_id']); diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index bbe4bd0..f888195 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -564,6 +564,7 @@ public function do_restrict_stuff($event) $sql_ary = array( 'user_id' => $user_id, 'original_group_id' => $original_group_id, + 'restrict_group_id' => $restrict_group_id, 'restrict_until' => $restrict_until, ); $sql = 'INSERT INTO ' . $this->restrict_table . ' ' . $this->db->sql_build_array('INSERT', $sql_ary); @@ -615,6 +616,15 @@ 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) + { + return; + } + // Remove the user from the banned group set in the ACP if (!function_exists('group_user_del')) { diff --git a/migrations/restrict_group_id_column.php b/migrations/restrict_group_id_column.php new file mode 100644 index 0000000..f1d3c2e --- /dev/null +++ b/migrations/restrict_group_id_column.php @@ -0,0 +1,130 @@ +db_tools->sql_column_exists($this->table_prefix . 'banhammer_restrict', 'restrict_group_id'); + } + + static public function depends_on() + { + return array('\phpbbmodders\banhammer\migrations\restrict_group'); + } + + public function update_schema() + { + return array( + 'add_columns' => array( + $this->table_prefix . 'banhammer_restrict' => array( + 'restrict_group_id' => array('UINT', 0), + ), + ), + ); + } + + public function revert_schema() + { + return array( + 'drop_columns' => array( + $this->table_prefix . 'banhammer_restrict' => array( + 'restrict_group_id', + ), + ), + ); + } + + public function update_data() + { + return array( + // Rows from before this column existed predate per-row + // tracking; the current ACP setting is the best available + // guess at which group they actually used. + array('custom', array(array($this, 'backfill_restrict_group_id'))), + ); + } + + 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 + */ + public function backfill_restrict_group_id() + { + $restrict_group_id = (int) $this->config['bh_restrict_group_id']; + + if (!$restrict_group_id) + { + return; + } + + $sql = 'UPDATE ' . $this->table_prefix . 'banhammer_restrict + SET restrict_group_id = ' . $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'); + } +} From 0e842f11c5cf00d62a7caadb05659e675231e2c5 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 16:42:57 -0500 Subject: [PATCH 6/9] fix: use phpBB's own cleanup for a banned user's PMs and poll votes 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 --- event/banhammer_listener.php | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index f888195..0ced01c 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -639,31 +639,22 @@ private function bh_del_privmsgs() { $user_id = $this->user_id; - // Get private messages - $sql = 'SELECT msg_id, author_id FROM ' . PRIVMSGS_TABLE . " - WHERE author_id = $user_id"; - $result = $this->db->sql_query($sql); - - $privmsgs_ary = array(); - while ($row = $this->db->sql_fetchrow($result)) - { - $privmsgs_ary[] = $row['msg_id']; - } - $this->db->sql_freeresult($result); - - if (!empty($privmsgs_ary)) + // phpBB's own bulk PM cleanup (used when deleting a user account + // entirely): correctly adjusts recipients' unread/new counts, + // removes attachments and notifications, and anonymizes already- + // delivered sent messages instead of deleting them out from under + // their recipients. A hand-rolled DELETE here previously left all + // of that bookkeeping inconsistent. + if (!function_exists('phpbb_delete_users_pms')) { - // And now close eventual reports. - $sql = 'UPDATE ' . REPORTS_TABLE . ' - SET report_closed = 1 - WHERE ' . $this->db->sql_in_set('pm_id', $privmsgs_ary); - $this->db->sql_query($sql); + include($this->root_path . 'includes/functions_privmsgs.' . $this->php_ext); } + phpbb_delete_users_pms(array($user_id)); - $this->db->sql_query('DELETE FROM ' . PRIVMSGS_TABLE . " WHERE author_id = $user_id"); + // The account itself isn't deleted, only its own folder structure + // and rules, which don't affect any other user. $this->db->sql_query('DELETE FROM ' . PRIVMSGS_FOLDER_TABLE . " WHERE user_id = $user_id"); $this->db->sql_query('DELETE FROM ' . PRIVMSGS_RULES_TABLE . " WHERE user_id = $user_id"); - $this->db->sql_query('DELETE FROM ' . PRIVMSGS_TO_TABLE . " WHERE user_id = $user_id OR author_id = $user_id"); } private function bh_del_posts() @@ -767,7 +758,10 @@ private function bh_del_posts() $this->db->sql_query('DELETE FROM ' . FORUMS_WATCH_TABLE . " WHERE user_id = $user_id"); $this->db->sql_query('DELETE FROM ' . MODERATOR_CACHE_TABLE . " WHERE user_id = $user_id"); $this->db->sql_query('DELETE FROM ' . NOTIFICATIONS_TABLE . " WHERE user_id = $user_id"); - $this->db->sql_query('DELETE FROM ' . POLL_VOTES_TABLE . " WHERE vote_user_id = $user_id"); + // Poll votes are deliberately left alone, same as phpBB's own + // 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"); $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"); From 4b539c20323fd8399172a1ad986460463dd46f8d Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 16:43:11 -0500 Subject: [PATCH 7/9] fix: MCP quick-ban link skips ACP-configured defaults 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 --- event/banhammer_listener.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index 0ced01c..ed465f2 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -133,10 +133,14 @@ public function add_mcp_queue_banhammer_link($event) if ($post_info['user_type'] != USER_FOUNDER && $target_user_id != $this->user->data['user_id'] && $target_user_id > 0) { + // Deliberately not 'bh' => 1: that shortcut jumps straight to the + // confirmation step with none of the ban options set (permanent, + // no email/IP ban, no deletions, no group move, no SFS report), + // silently ignoring the ACP-configured defaults. Link to the + // profile page instead, which shows the real options form. $params = array( 'mode' => 'viewprofile', 'u' => $target_user_id, - 'bh' => 1, ); $template_vars['S_MCP_SHOW_BANHAMMER'] = true; From d5404585c95a3cea88c4bf18a269e6f11688bb0d Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 16:43:24 -0500 Subject: [PATCH 8/9] fix: check curl_exec() result for SFS reports, not just HTTP code 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 --- event/banhammer_listener.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index ed465f2..15b8f0c 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -780,12 +780,14 @@ private function get_file($url) curl_setopt($ch, CURLOPT_URL, $url); curl_setopt($ch, CURLOPT_TIMEOUT, 5); curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 5); - curl_exec($ch); + $response = curl_exec($ch); $httpcode = curl_getinfo($ch, CURLINFO_HTTP_CODE); curl_close($ch); - // if nothing is returned (SFS is down) - if ($httpcode != 200) + // curl_exec() returns false on a transport failure (e.g. the + // connection dropped after headers were already sent), which the + // HTTP code alone would not catch. + if ($response === false || $httpcode != 200) { return false; } From ed337a287e1deb1fd102789d98fa858cee75629c Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 16:43:39 -0500 Subject: [PATCH 9/9] fix: malformed BH_STYLE attribute and dead ACP success-message markup 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 --- adm/style/banhammer_body.html | 4 ---- event/banhammer_listener.php | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/adm/style/banhammer_body.html b/adm/style/banhammer_body.html index d41c5a8..ef0687d 100644 --- a/adm/style/banhammer_body.html +++ b/adm/style/banhammer_body.html @@ -2,10 +2,6 @@

{L_ACP_BH_SETTINGS}

- -
{L_SETTINGS_SAVED}
- -
diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index 15b8f0c..66f4533 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -238,7 +238,7 @@ public function do_ban_hammer_stuff($event) } $this->template->assign_vars(array( - 'BH_STYLE' => (($bh_result == 'success') ? 'green' : '#a92c2c') . '; color: white;"', + 'BH_STYLE' => (($bh_result == 'success') ? 'green' : '#a92c2c') . '; color: white;', 'BH_MESSAGE' => $bh_message, )); }