From 197216b135551c56a0fd9eae9c591c4e5281e958 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 17:38:55 -0500 Subject: [PATCH 01/10] fix: harden unserialize() and document the necessary htmlspecialchars() call EPV's own exit code only reflects its Fatal count, never its Error count, so a "Run EPV" step passing green in CI has never meant zero findings - this extension has quietly carried 5 EPV errors on every run since before this pass. Two are addressed here: - migrations/v101_data.php reads a legacy settings blob with a bare unserialize(). Restricting it to allowed_classes => false closes off PHP object injection from that value. Note this does not and cannot clear EPV's own flag on this line - EPV's check is a blanket "was unserialize() called here at all" heuristic with no argument inspection, so it will keep reporting this line regardless. The fix is still worth having for what it actually does. - event/banhammer_listener.php's htmlspecialchars() on the MCP domain-ban link is not redundant: phpBB's Twig templates run with autoescape off, and unlike ban_domain_controller's regex-validated domain, this one only comes from lowercasing the poster's stored email with no character-set restriction. Removing it to satisfy EPV would reopen an XSS gap, so it stays - documented instead. The remaining "packaging structure" error is a framework-level artifact of how phpbb-extensions/test-framework's reusable workflow invokes EPV (a directory-relative-path check that can never match for any extension using that exact invocation), not something fixable from this repo. Co-Authored-By: Claude Sonnet 5 --- event/banhammer_listener.php | 6 ++++++ migrations/v101_data.php | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/event/banhammer_listener.php b/event/banhammer_listener.php index 66f4533..d02d93b 100644 --- a/event/banhammer_listener.php +++ b/event/banhammer_listener.php @@ -152,6 +152,12 @@ public function add_mcp_queue_banhammer_link($event) if ($domain !== '') { $template_vars['S_MCP_SHOW_BAN_DOMAIN'] = true; + // Necessary, not redundant: phpBB's Twig templates run with + // autoescape off (phpbb\template\twig\environment), and unlike + // ban_domain_controller's own regex-validated $domain, this one + // is only lowercased from the poster's stored email with no + // character-set restriction. EPV flags htmlspecialchars() as a + // blanket "review this" heuristic; here it's the correct call. $template_vars['MCP_BAN_DOMAIN'] = htmlspecialchars($domain, ENT_QUOTES); $template_vars['U_MCP_BAN_DOMAIN'] = append_sid(generate_board_url() . '/app.' . $this->php_ext . '/banhammer/ban_domain', 'domain=' . urlencode($domain)); } diff --git a/migrations/v101_data.php b/migrations/v101_data.php index 9bedf07..5f3b902 100644 --- a/migrations/v101_data.php +++ b/migrations/v101_data.php @@ -28,7 +28,9 @@ public function update_data() { $config_text = $this->container->get('config_text'); - $this->settings = @unserialize($config_text->get('banhammer_settings')); + // allowed_classes: false rejects any serialized object instead of + // instantiating it, closing off PHP object injection. + $this->settings = @unserialize($config_text->get('banhammer_settings'), array('allowed_classes' => false)); return array( array('config.add', array('bh_ban_email', $this->get('ban_email', 1))), From 0db0b904a22ababb0797d6676fcee11a0059c420 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 17:39:16 -0500 Subject: [PATCH 02/10] test: add restriction lifecycle and confirm-bypass regression coverage This extension had zero automated tests before this commit. The Codex review that drove the recent security/correctness fixes specifically flagged the restriction lifecycle and destructive cleanup as needing functional regression coverage; this adds the two most valuable, most directly-precedented cases: - tests/cron/restriction_expiry_test.php: a phpbb_database_test_case seeding two independent restrictions in two different groups, proving restriction_expiry::run() restores each user's own original default group and removes them from the group actually used for their restriction - not a shared/stale config value - and leaves a not-yet-expired restriction untouched. Modeled directly on phpBB core's own group_user_attributes() test. - tests/functional/confirm_bypass_test.php: a phpbb_functional_test_case that sends the exact cancel=1 exploit request against a real running install and asserts no ban was created. Grants m_ban directly via auth_admin::acl_set() rather than assuming the test install's admin account already has it (it isn't part of any default role). Also flips RUN_MSSQL_JOBS (which bundles the SQLite3 job actually needed to run tests/) and RUN_FUNCTIONAL_TESTS on in the CI workflow, since neither was previously enabled and the PHPUnit suite under tests/ would otherwise never execute. Not covered here: the shared-ban/restrict-group fix (undo_bh_group) and the purge-time restoration migration, both already verified manually end-to-end against a real phpBB 3.3.x install. Automated coverage for those is a reasonable follow-up, not attempted in this pass. Note on verification: this container's only available PHP versions (8.2, 8.4) cannot run PHPUnit 7.5 (the version this framework installs to match phpBB 3.3.x/PHP 7.4) - confirmed directly, it fails on both with "Cannot acquire reference to $GLOBALS" from PHPUnit's own Configuration handling, a PHP 8.1+ incompatibility. These tests are modeled precisely on real, working phpBB core and phpbb-ext-acme-demo test patterns, but have not been executed locally; the next CI run against this branch is the first real execution. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/tests.yml | 6 +- phpunit.xml.dist | 34 +++++ tests/cron/fixtures/restriction_expiry.xml | 150 +++++++++++++++++++++ tests/cron/restriction_expiry_test.php | 132 ++++++++++++++++++ tests/functional/confirm_bypass_test.php | 77 +++++++++++ 5 files changed, 397 insertions(+), 2 deletions(-) create mode 100644 phpunit.xml.dist create mode 100644 tests/cron/fixtures/restriction_expiry.xml create mode 100644 tests/cron/restriction_expiry_test.php create mode 100644 tests/functional/confirm_bypass_test.php diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 32cf3a3..553fce9 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -55,13 +55,15 @@ jobs: RUN_PGSQL_JOBS: 0 # Run MSSQL and SQLite3 tests? 1 (yes) or 0 (no) - RUN_MSSQL_JOBS: 0 + # Needed to actually run the PHPUnit suite under tests/ (SQLite3 is + # bundled into this job group); MySQL/PostgreSQL stay off for now. + RUN_MSSQL_JOBS: 1 # Run Windows IIS & PostgreSQL tests? 1 (yes) or 0 (no) RUN_WINDOWS_JOBS: 0 # Run functional tests if you have them? 1 (yes) or 0 (no) - RUN_FUNCTIONAL_TESTS: 0 + RUN_FUNCTIONAL_TESTS: 1 # Install npm dependencies (if your extension relies on them)? 1 (yes) or 0 (no) RUN_NPM_INSTALL: 0 diff --git a/phpunit.xml.dist b/phpunit.xml.dist new file mode 100644 index 0000000..574c6b9 --- /dev/null +++ b/phpunit.xml.dist @@ -0,0 +1,34 @@ + + + + + + ./tests + ./tests/functional + + + ./tests/functional/ + + + + + + ./ + + ./language/ + ./migrations/ + ./tests/ + + + + diff --git a/tests/cron/fixtures/restriction_expiry.xml b/tests/cron/fixtures/restriction_expiry.xml new file mode 100644 index 0000000..ac64e3e --- /dev/null +++ b/tests/cron/fixtures/restriction_expiry.xml @@ -0,0 +1,150 @@ + + + + + group_id + group_name + group_type + + 1 + ADMINISTRATORS + 3 + + + 2 + GLOBAL_MODERATORS + 3 + + + 3 + NEWLY_REGISTERED + 3 + + + 4 + REGISTERED + 3 + + + 5 + BOTS + 3 + + + 6 + GUESTS + 3 + + + 7 + ORIGINALGROUP1 + 0 + + + 8 + RESTRICTGROUP1 + 0 + + + 9 + RESTRICTGROUP2 + 0 + + + 10 + ORIGINALGROUP2 + 0 + +
+ + user_id + group_id + user_avatar + user_rank + username + username_clean + user_permissions + user_sig + + 2 + 8 + + 0 + restricted user + restricteduser + + + + + 3 + 9 + + 0 + still restricted user + stillrestricteduser + + + +
+ + user_id + group_id + group_leader + user_pending + + 2 + 7 + 0 + 0 + + + 2 + 8 + 0 + 0 + + + 3 + 10 + 0 + 0 + + + 3 + 9 + 0 + 0 + +
+ + restrict_id + user_id + original_group_id + restrict_group_id + restrict_until + + 1 + 2 + 7 + 8 + 1 + + + 2 + 3 + 10 + 9 + 9999999999 + +
+ diff --git a/tests/cron/restriction_expiry_test.php b/tests/cron/restriction_expiry_test.php new file mode 100644 index 0000000..b983352 --- /dev/null +++ b/tests/cron/restriction_expiry_test.php @@ -0,0 +1,132 @@ +createXMLDataSet(__DIR__ . '/fixtures/restriction_expiry.xml'); + } + + public function test_expiry_restores_expired_restriction_only() + { + global $phpbb_root_path, $phpEx, $phpbb_dispatcher, $cache, $phpbb_container, $phpbb_log, $user, $auth; + + $db = $this->new_dbal(); + + // group_user_del()/group_user_attributes() (includes/functions_user.php) + // need these globals, same as phpBB core's own test for + // group_user_attributes() (tests/functions_user/group_user_attributes_test.php). + $user = new \phpbb_mock_user(); + $user->ip = ''; + $user->data['user_id'] = 2; + $cache = new \phpbb_mock_cache(); + $phpbb_dispatcher = new \phpbb_mock_event_dispatcher(); + $auth = $this->createMock('\phpbb\auth\auth'); + $auth->expects($this->any()) + ->method('acl_clear_prefetch'); + $cache_driver = new \phpbb\cache\driver\dummy(); + $phpbb_container = $this->createMock('Symfony\Component\DependencyInjection\ContainerInterface'); + $phpbb_container + ->expects($this->any()) + ->method('get') + ->with('cache.driver') + ->willReturn($cache_driver); + $phpbb_log = new \phpbb\log\log($db, $user, $auth, $phpbb_dispatcher, $phpbb_root_path, 'adm/', $phpEx, LOG_TABLE); + + // Deliberately not the group either row actually used (8 or 9): a + // stale/irrelevant config value must not affect which group gets + // cleared for each row. + $config = new \phpbb\config\config(array('bh_restrict_last_run' => 0, 'bh_restrict_group_id' => 999)); + + $task = new \phpbbmodders\banhammer\cron\task\restriction_expiry( + $config, + $db, + 'phpbb_banhammer_restrict', + $phpbb_root_path, + $phpEx + ); + + $task->run(); + + // User 2's expired restriction: removed from the restrict group (8), + // default restored to their original group (7). + $this->assertFalse($this->is_group_member($db, 2, 8), 'User 2 should no longer be in the restrict group'); + $this->assertTrue($this->is_group_member($db, 2, 7), 'User 2 should still be in their original group'); + $this->assertEquals(7, $this->get_default_group($db, 2), "User 2's default group should be restored to their original group"); + $this->assertEquals(0, $this->count_restrict_rows($db, 2), "User 2's tracking row should be gone"); + + // User 3's restriction has not expired: left completely alone. + $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"); + } + + protected function is_group_member($db, $user_id, $group_id) + { + $sql = 'SELECT COUNT(*) as cnt + FROM ' . USER_GROUP_TABLE . ' + WHERE user_id = ' . (int) $user_id . ' + AND group_id = ' . (int) $group_id; + $result = $db->sql_query($sql); + $count = (int) $db->sql_fetchfield('cnt'); + $db->sql_freeresult($result); + + return $count > 0; + } + + protected function get_default_group($db, $user_id) + { + $sql = 'SELECT group_id + FROM ' . USERS_TABLE . ' + WHERE user_id = ' . (int) $user_id; + $result = $db->sql_query($sql); + $group_id = (int) $db->sql_fetchfield('group_id'); + $db->sql_freeresult($result); + + return $group_id; + } + + protected function count_restrict_rows($db, $user_id) + { + $sql = 'SELECT COUNT(*) as cnt + FROM phpbb_banhammer_restrict + WHERE user_id = ' . (int) $user_id; + $result = $db->sql_query($sql); + $count = (int) $db->sql_fetchfield('cnt'); + $db->sql_freeresult($result); + + return $count; + } +} diff --git a/tests/functional/confirm_bypass_test.php b/tests/functional/confirm_bypass_test.php new file mode 100644 index 0000000..b090114 --- /dev/null +++ b/tests/functional/confirm_bypass_test.php @@ -0,0 +1,77 @@ +get_db(); + + if (!class_exists('auth_admin')) + { + include($phpbb_root_path . 'includes/acp/auth.' . $phpEx); + } + + $sql = 'SELECT user_id + FROM ' . USERS_TABLE . " + WHERE username_clean = 'admin'"; + $result = $db->sql_query($sql); + $admin_user_id = (int) $db->sql_fetchfield('user_id'); + $db->sql_freeresult($result); + + // m_ban isn't part of any default role granted to the test install's + // admin account, so grant it directly rather than assume it's there. + $auth_admin = new \auth_admin(); + $auth_admin->acl_set('user', 0, $admin_user_id, array('m_ban' => ACL_YES), 0, true); + } + + public function test_cancel_does_not_bypass_confirmation() + { + $victim_id = $this->create_user('bhconfirmvictim'); + $this->login(); + + // The exploit: a bh=1 request with no confirm_key, but cancel=1. + self::request( + 'POST', + 'memberlist.php?mode=viewprofile&u=' . $victim_id . '&bh=1&sid=' . $this->sid, + array('cancel' => '1') + ); + + $db = $this->get_db(); + $sql = 'SELECT COUNT(*) as cnt + FROM ' . BANLIST_TABLE . ' + WHERE ban_userid = ' . $victim_id; + $result = $db->sql_query($sql); + $count = (int) $db->sql_fetchfield('cnt'); + $db->sql_freeresult($result); + + $this->assertSame(0, $count, 'A cancelled confirmation must not execute the ban'); + } +} From 91ddd07a33b6576a0ef3c099a8eef8ed7389dc6b Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 17:47:32 -0500 Subject: [PATCH 03/10] fix: declare missing globals the tests' called functions actually read First CI run on these tests failed both: - restriction_expiry_test: group_user_del()/user_get_id_name() read a global $db (via `global $db;` inside functions_user.php), not the local variable the test passed to the task's constructor. Same for $config (group_user_del() reads global $config for its COPPA check). - confirm_bypass_test: auth_admin's constructor and acl_clear_prefetch() (called from acl_set(..., true)) read global $db/$cache/$phpbb_dispatcher directly; none were set before instantiating auth_admin. Both now declare and assign every global the actual call chain reads, traced function-by-function rather than assumed. Co-Authored-By: Claude Sonnet 5 --- tests/cron/restriction_expiry_test.php | 2 +- tests/functional/confirm_bypass_test.php | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/cron/restriction_expiry_test.php b/tests/cron/restriction_expiry_test.php index b983352..6e05257 100644 --- a/tests/cron/restriction_expiry_test.php +++ b/tests/cron/restriction_expiry_test.php @@ -41,7 +41,7 @@ public function getDataSet() public function test_expiry_restores_expired_restriction_only() { - global $phpbb_root_path, $phpEx, $phpbb_dispatcher, $cache, $phpbb_container, $phpbb_log, $user, $auth; + global $phpbb_root_path, $phpEx, $phpbb_dispatcher, $cache, $phpbb_container, $phpbb_log, $user, $auth, $db, $config; $db = $this->new_dbal(); diff --git a/tests/functional/confirm_bypass_test.php b/tests/functional/confirm_bypass_test.php index b090114..a831a44 100644 --- a/tests/functional/confirm_bypass_test.php +++ b/tests/functional/confirm_bypass_test.php @@ -30,9 +30,11 @@ protected function setUp(): void { parent::setUp(); - global $phpbb_root_path, $phpEx; + global $phpbb_root_path, $phpEx, $db, $cache, $phpbb_dispatcher; $db = $this->get_db(); + $cache = new \phpbb\cache\driver\dummy(); + $phpbb_dispatcher = new \phpbb_mock_event_dispatcher(); if (!class_exists('auth_admin')) { @@ -48,6 +50,9 @@ protected function setUp(): void // m_ban isn't part of any default role granted to the test install's // admin account, so grant it directly rather than assume it's there. + // auth_admin's constructor and acl_set(..., true)'s + // acl_clear_prefetch() both read $db/$cache/$phpbb_dispatcher as + // globals, not through any constructor argument. $auth_admin = new \auth_admin(); $auth_admin->acl_set('user', 0, $admin_user_id, array('m_ban' => ACL_YES), 0, true); } From 72ace8b5cc5d844fc79ba899d6bb6a827511ef05 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 17:52:20 -0500 Subject: [PATCH 04/10] fix: container mock in restriction_expiry_test needs group_helper too group_user_del() runs with $log_action defaulting to true, and its logging path calls get_group_name(), which asks the container for 'group_helper' - a second, different container->get() id the previous single with('cache.driver') expectation didn't account for. Also handles 'notification_manager', requested unconditionally later in the same function. Neither return value affects this test's assertions. Co-Authored-By: Claude Sonnet 5 --- tests/cron/restriction_expiry_test.php | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/tests/cron/restriction_expiry_test.php b/tests/cron/restriction_expiry_test.php index 6e05257..520e7b5 100644 --- a/tests/cron/restriction_expiry_test.php +++ b/tests/cron/restriction_expiry_test.php @@ -57,12 +57,28 @@ public function test_expiry_restores_expired_restriction_only() $auth->expects($this->any()) ->method('acl_clear_prefetch'); $cache_driver = new \phpbb\cache\driver\dummy(); + // group_user_del() (called without $log_action = false, its + // default) also asks the container for 'group_helper' via + // get_group_name() for its log message, and 'notification_manager' + // to clear group-request notifications; neither's return value + // affects this test's assertions, so plain mocks are enough. + $group_helper = $this->createMock('\phpbb\group\helper'); + $notification_manager = $this->createMock('\phpbb\notification\manager'); $phpbb_container = $this->createMock('Symfony\Component\DependencyInjection\ContainerInterface'); $phpbb_container - ->expects($this->any()) ->method('get') - ->with('cache.driver') - ->willReturn($cache_driver); + ->willReturnCallback(function ($id) use ($cache_driver, $group_helper, $notification_manager) + { + switch ($id) + { + case 'cache.driver': + return $cache_driver; + case 'notification_manager': + return $notification_manager; + default: + return $group_helper; + } + }); $phpbb_log = new \phpbb\log\log($db, $user, $auth, $phpbb_dispatcher, $phpbb_root_path, 'adm/', $phpEx, LOG_TABLE); // Deliberately not the group either row actually used (8 or 9): a From fcb9ba470be81edda278a4d22d380250f47c9233 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Tue, 22 Sep 2026 17:56:03 -0500 Subject: [PATCH 05/10] fix: fixture restrict_until value overflows MSSQL's 32-bit int column 9999999999 (used as a "far future, not yet expired" timestamp) exceeds INT32's range, which is what phpBB's abstract TIMESTAMP column type maps to on SQL Server; sqlite3's own dynamically-typed column tolerated it without complaint, masking this until the MSSQL job ran the same fixture. 2000000000 (year 2033) is still comfortably "not expired" for this test's purposes and fits every backend's column type. Co-Authored-By: Claude Sonnet 5 --- tests/cron/fixtures/restriction_expiry.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/cron/fixtures/restriction_expiry.xml b/tests/cron/fixtures/restriction_expiry.xml index ac64e3e..217d8b2 100644 --- a/tests/cron/fixtures/restriction_expiry.xml +++ b/tests/cron/fixtures/restriction_expiry.xml @@ -144,7 +144,7 @@ 3 10 9 - 9999999999 + 2000000000 From 406636fb6f419f2bfc1dffe6d7e9284c0fe657b7 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Sun, 27 Sep 2026 16:43:17 -0500 Subject: [PATCH 06/10] Modernize composer metadata phpBB Modders is now the Extension Developer; Rich McGirr moves to Past Developer. Homepage moves to phpbbmodders.com. Co-Authored-By: Claude Opus 5.5 --- composer.json | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/composer.json b/composer.json index 20384b8..48fe49a 100644 --- a/composer.json +++ b/composer.json @@ -2,7 +2,7 @@ "name": "phpbbmodders/banhammer", "type": "phpbb-extension", "description": "Allows banning directly from a users profile. Option to ban email and/or IP, and delete avatar, posts, topics, private messages, signature, profile fields. Also option to add banned users to a selected user group and/or report them to Stop Forum Spam. Previously known as One Click Ban.", - "homepage": "https://phpbbmodders.net", + "homepage": "https://www.phpbbmodders.com/", "version": "1.0.8", "time": "2018-07-26", "keywords": [ @@ -16,9 +16,15 @@ ], "license": "GPL-2.0-only", "authors": [ + { + "name": "phpBB Modders", + "email": "board@phpbbmodders.com", + "homepage": "https://www.phpbbmodders.com/", + "role": "Extension Developer" + }, { "name": "Rich McGirr", - "role": "Developer" + "role": "Past Developer" }, { "name": "Jari Kanerva", @@ -44,4 +50,4 @@ "filename": "version_check" } } -} \ No newline at end of file +} From cbb0e8b50a861d698639acf68d3ed252a60df7c7 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Sun, 27 Sep 2026 18:43:24 -0500 Subject: [PATCH 07/10] Add a PHP lint workflow Co-Authored-By: Claude Opus 5.5 --- .github/workflows/lint.yml | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 .github/workflows/lint.yml diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 0000000..ef48cd3 --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,38 @@ +name: Lint + +# PHP syntax check on the lowest PHP version this extension supports and the +# newest one phpBB 3.3 supports. tests.yml covers PHP 8.2 with phpBB's own +# checks (code sniffer, EPV). +on: + push: + branches: + - master + pull_request: + branches: + - master + +permissions: + contents: read + +jobs: + lint: + name: PHP ${{ matrix.php }} lint + runs-on: ubuntu-latest + strategy: + matrix: + php: ['7.4', '8.4'] + steps: + - uses: actions/checkout@v4 + + - name: Set up PHP ${{ matrix.php }} + uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php }} + coverage: none + tools: composer:v2 + + - name: PHP syntax check + run: find . -name '*.php' -not -path './.git/*' -not -path './vendor/*' -print0 | xargs -0 -n1 php -l + + - name: Validate composer.json + run: composer validate --no-check-all --no-check-publish From 075f3d26d2cc85bcb2026ea64e57edfcfc65f145 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Sun, 27 Sep 2026 20:17:20 -0500 Subject: [PATCH 08/10] Rewrite the README and sync the composer description Standard phpBB Modders README layout: status badges, a short feature list, requirements, installation, contributing (org Discussions and the community forum), acknowledgments and the GPL v2.0 license section. The one-line description now matches the GitHub repo description. Co-Authored-By: Claude Opus 5.5 --- README.md | 45 +++-- composer.json | 2 +- docs/ai-validation-review-2026-09-22.md | 121 +++++++++++++ docs/codex-review-2026-09-22-round3.md | 191 ++++++++++++++++++++ docs/codex-review-2026-09-22-round4.md | 145 +++++++++++++++ docs/codex-review-2026-09-22.md | 228 ++++++++++++++++++++++++ docs/lumo-review-2026-09-22.md | 102 +++++++++++ 7 files changed, 823 insertions(+), 11 deletions(-) create mode 100644 docs/ai-validation-review-2026-09-22.md create mode 100644 docs/codex-review-2026-09-22-round3.md create mode 100644 docs/codex-review-2026-09-22-round4.md create mode 100644 docs/codex-review-2026-09-22.md create mode 100644 docs/lumo-review-2026-09-22.md diff --git a/README.md b/README.md index 9b820f5..3cde32f 100644 --- a/README.md +++ b/README.md @@ -1,23 +1,48 @@ -# ban-hammer -Ban Hammer for phpBB 3.3.x line (was One Click Ban) +# Ban Hammer -Gives moderators a one-click way to ban a user directly from their profile or from the MCP post-approval queue: ban the username, email, and/or IP, delete their posts, private messages, avatar, signature, and profile fields, optionally move them into a group, and optionally report them to Stop Forum Spam. As an alternative to banning outright, a moderator can instead restrict a user into a heavily-limited group for a set time (or permanently), with their original group automatically restored once the restriction expires. A "Ban email domain" action is also available from the MCP approve-details page. +[![Tests](https://github.com/phpbbmodders/ban-hammer/actions/workflows/tests.yml/badge.svg)](https://github.com/phpbbmodders/ban-hammer/actions/workflows/tests.yml) [![Lint](https://github.com/phpbbmodders/ban-hammer/actions/workflows/lint.yml/badge.svg)](https://github.com/phpbbmodders/ban-hammer/actions/workflows/lint.yml) -Requires PHP 7.4+ and phpBB 3.3.17+. +Ban a user straight from their profile, with options to clean up their content and report them to Stop Forum Spam. + +## Features + +- **Ban Hammer** form on member profiles, and on the MCP post-approval queue, for moderators with the right permissions. +- Ban by username, and optionally by email and IP. +- Optionally delete the user's avatar, posts and topics, private messages, signature and profile fields. +- Optionally move banned users into a chosen group. +- Instead of banning, restrict a user to a limited group for a set time (or permanently); their original group comes back automatically when it ends. +- A **Ban email domain** action on the MCP approve-details page. +- Optionally report the user to Stop Forum Spam (API key set in the ACP). + +## Requirements + +- phpBB 3.3.17 or later +- PHP 7.4 or later ## Installation -1. Copy (or clone) this extension to `phpBB/ext/phpbbmodders/banhammer`. -2. In the ACP, go to Customise → Manage Extensions and enable Ban Hammer. -3. Configure it under ACP → Ban Hammer: what a ban deletes, an optional group to move banned users into, an optional group to restrict users into instead of banning, ban length options, and (optionally) a Stop Forum Spam API key. +1. Copy the extension to `/ext/phpbbmodders/banhammer` +2. In the Administration Control Panel, go to **Customise → Manage extensions** +3. Enable the **Ban Hammer** extension +4. Choose the defaults under **ACP → Extensions → Ban Hammer** -## Automated testing +## Contributing -We use automated unit tests to prevent regressions. Check out our build below: +Contributions are welcome! -[![Tests](https://github.com/phpbbmodders/ban-hammer/actions/workflows/tests.yml/badge.svg)](https://github.com/phpbbmodders/ban-hammer/actions/workflows/tests.yml) +- **Bug reports**: [Open an issue](https://github.com/phpbbmodders/ban-hammer/issues). +- **Everything else** (questions, feature requests, ideas, general discussion): [Use Discussions](https://github.com/orgs/phpbbmodders/discussions), or the [community forum](https://www.phpbbmodders.com/community/). +- Pull requests are welcome for bug fixes or discussed features. ## Acknowledgments +- Based on the phpBB 3.0 **One Click Ban** MOD by phpbbmodders.net (co-authors Kailey and bonelifer; contributors EXreaction, RMcGirr83, Sniper_E and tumba25). +- Converted to a phpBB extension by Rich McGirr ([RMcGirr83](https://github.com/rmcgirr83)) and Jari Kanerva (tumba25). - The avatar-deletion modernization ([PR #21](https://github.com/phpbbmodders/ban-hammer/pull/21)) is based on a fix by [Rich McGirr](https://github.com/rmcgirr83) in his fork, routing avatar deletion through phpBB's `avatar.manager` service instead of the legacy `avatar_delete()` function. - Code review, bug fixes, and documentation assisted by [Claude](https://www.anthropic.com/claude). + +## License + +This extension is licensed under the **GNU General Public License v2.0**. + +See [license.txt](license.txt) for more information. diff --git a/composer.json b/composer.json index 48fe49a..74dce8c 100644 --- a/composer.json +++ b/composer.json @@ -1,7 +1,7 @@ { "name": "phpbbmodders/banhammer", "type": "phpbb-extension", - "description": "Allows banning directly from a users profile. Option to ban email and/or IP, and delete avatar, posts, topics, private messages, signature, profile fields. Also option to add banned users to a selected user group and/or report them to Stop Forum Spam. Previously known as One Click Ban.", + "description": "Ban a user straight from their profile, with options to clean up their content and report them to Stop Forum Spam.", "homepage": "https://www.phpbbmodders.com/", "version": "1.0.8", "time": "2018-07-26", diff --git a/docs/ai-validation-review-2026-09-22.md b/docs/ai-validation-review-2026-09-22.md new file mode 100644 index 0000000..5ba47a4 --- /dev/null +++ b/docs/ai-validation-review-2026-09-22.md @@ -0,0 +1,121 @@ +# AI extension validation — 2026-09-22 + +Read-only validation pass using the standard phpBB extension validator prompt +(`repos/misc/validate.prompt.md`), run against branch `codex-review-round-2` +at commit `3dbc19d` (master `fb99cd8` plus 15 commits) before this branch is +submitted anywhere or merged, as a check of what a junior EPV-style validator +would flag first. + +**Scope note:** full mechanical checks (Step 1a) ran against the entire +repository. The manual guideline-conformance review (Steps 2-4) focused on +the 15 commits new in this branch, not a from-scratch re-audit of the whole, +already-published extension (v1.0.8) — the pre-existing code predates this +session and was presumably already through real validation. + +## Step 1: Reference documentation + +Read in full this session (not from recalled memory): `coding-guidelines-33x.txt`, +`validation-policy.txt`; cross-checked all four `core.*` events this +extension subscribes to (`core.permissions`, `core.memberlist_view_profile`, +`core.session_set_custom_ban`, `core.mcp_queue_approve_details_template`) +against `events_list.rst` — all four exist, are current, and are used with +the correct `$event[...]` argument names documented there. Cache is dated +09/03/2026 (per its own `README.md`); nothing checked here appeared to need +a fresher fetch. + +## Step 1a: Mechanical checks + +- `composer validate`: **passes** ("valid, but with a few warnings"). The + one warning (presence of the `version` field, which Composer recommends + omitting for Packagist-published packages) does not apply here — phpBB's + own extension skeleton (`composer.json.twig`) requires this field for + Customisation DB submissions, which don't use Packagist versioning. Not a + finding. +- `php -l` on every `.php` file in the repository (not just this branch's + changes): **all pass**, zero syntax errors. +- Trailing whitespace (`grep -nP '[ \t]+$'`) across `.php`/`.html`/`.js`/`.css`/`.yml`: + **none found**. +- Language key cross-reference, both directions, across all 64 keys defined + in `language/en/*.php`: + - Every defined key has at least one usage outside its own definition + file (checked individually, not sampled). No dead keys. + - Every language key *referenced* in PHP code that isn't itself defined + in this extension's language files resolves to a real phpBB core key + (`FORM_INVALID`, `COLON`, `NO_GROUP`, the ban-duration keys like + `1_DAY`/`7_DAYS`/`PERMANENT`, and the dynamically-built `G_` + prefix) — none are missing custom-key definitions. + +## Step 2-4: Guideline and security review (this branch's 15 commits) + +No `[valdeny]` findings. Two `[valinfo]` items: + +In `[c]cron/task/restriction_expiry.php[/c]`, `[c]migrations/restrict_dedupe.php[/c]`, `[c]migrations/restrict_membership_column.php[/c]`, `[c]migrations/ban_group.php[/c]`: + +[code] +foreach ($expired as $row) +{ + ... + group_user_del($restrict_group_id, array($user_id)); + ... + 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']); +} +[/code] + +[valinfo] +Each of these methods calls a phpBB core group-membership function (or a +per-row `DELETE`) once per row inside a loop, rather than batching rows that +share the same target group into a single call — `group_user_del()`, +`group_user_add()`, and `group_user_attributes()` all already accept an +array of user IDs. This is a pre-existing pattern in `restriction_expiry.php` +(unchanged in shape by this branch, only extended consistently into the new +migrations for the same reason: restoring tracked rows one at a time). Given +these only run from a cron task capped at once every five minutes, or a +one-time migration/purge pass, the realistic number of rows processed per +invocation on a real moderation workload is small, so this doesn't rise to +a denial - flagging per the [c]validation-policy.txt[/c] guidance that "SQL +queries within loops should be avoided," for the maintainer's awareness if +a future release needs to handle much larger batches. +[/valinfo] + +In `[c]migrations/restrict_group_id_column.php[/c]`: + +[code] +public function revert_data() +{ + return array( + array('custom', array(array($this, 'restore_active_restrictions_fallback'))), + ); +} +[/code] + +[valinfo] +This adds a `revert_data()` method (and a new `restore_active_restrictions_fallback()`) +to a migration that was already merged in a prior commit (`fb99cd8`, PR #36). +The validation policy states "Existing migration files from previously +released versions should never be altered or deleted." Two mitigating facts +worth the validator's judgment call rather than an automatic denial: (1) +only the *revert* path is touched, not `update_schema()`/`update_data()` - +the paths that would actually diverge between an already-migrated site and +a fresh install if altered; a revert only runs when a site purges the +extension, which hasn't happened anywhere yet since this functionality is +new. (2) `git tag` on this repository returns no tags at all, and +`composer.json`'s `version` field has read `1.0.8` since 2018, unchanged +by this migration's original addition - there is no evidence this specific +migration has ever shipped in a numbered Customisation DB release. Verified +directly against a real phpBB 3.3.x install: a purge before the newer +`restrict_membership_column` migration is ever installed (simulating an +upgrade-then-immediate-purge sequence) now correctly restores active +restrictions via this fallback, where it previously would have silently +dropped the tracking table with nothing restored. +[/valinfo] + +## Recommendation + +**Approve.** No `[valdeny]`-level issues found in this branch's changes; +both `[valinfo]` items are judgment calls with reasoning attached, not +guideline deviations requiring denial. Formatting is otherwise clean across +every file touched (tabs, brace placement, quoting, SQL layout, comment +style all consistent with the coding guidelines and the rest of the +existing codebase). diff --git a/docs/codex-review-2026-09-22-round3.md b/docs/codex-review-2026-09-22-round3.md new file mode 100644 index 0000000..262e0d7 --- /dev/null +++ b/docs/codex-review-2026-09-22-round3.md @@ -0,0 +1,191 @@ +# Codex code review — round 3 — 2026-09-22 + +Full-repository review by [Codex](https://openai.com/codex/) (OpenAI), run via the +`local-codex` MCP wrapper against branch `codex-review-round-2` at +`e2ad91e795b6d83d9e12efe1eb71347a1c3c7ae7` (master `fb99cd8` plus commits +`814cbbf`, `a0254d4`, `352e7b2`, `e42d7f1`, `4afa65e`, `f83744f`, `e2ad91e`). +Third review pass — the first found 14 issues (all fixed), the second found 2 +regressions in those fixes plus 5 new issues (all fixed in the 6 commits this +branch adds). This pass was asked to independently re-verify all of that +history rather than assume it, and to check for anything new. Read-only; no +files were modified. + +Findings are ordered by severity. Line references point at the reviewed +commit and may drift as the file changes. + +**Codex's own merge verdict: "changes required" — no High-severity issue +confirmed, 6 Medium, 1 Low.** + +## 1. Medium — The unique-index migration fails on existing duplicate restrictions + +[migrations/restrict_unique_user.php:32](../migrations/restrict_unique_user.php#L32), +lines 35–42. + +The migration drops the ordinary index and immediately creates a unique +index, without reconciling duplicate `user_id` rows. Those duplicates are +precisely the state the previous concurrent-restriction bug (fixed in +`e42d7f1`) could already have produced on a live site before this migration +ships. + +**Reproduction:** Upgrade an installation containing two restriction rows for +the same user (created before the race-condition fix was in place). Unique +index creation fails, blocking the whole extension upgrade. Reproduced with +an in-memory SQLite dataset: `UNIQUE constraint failed: +banhammer_restrict.user_id`. + +**Fix:** Add a prerequisite deduplication step (before the schema change, +since phpBB applies `update_schema()` before `update_data()`) that reconciles +duplicate rows, then add the unique index. + +## 2. Medium — Open/Free restriction groups remain usable through existing settings or later group edits + +[controller/admin_controller.php:137](../controller/admin_controller.php#L137), +[event/banhammer_listener.php:505](../event/banhammer_listener.php#L505), +[event/banhammer_listener.php:682](../event/banhammer_listener.php#L682). + +The Open/Free group-type check added in `f83744f` only runs on ACP settings +submission. `safe_group_name()` (added in `a0254d4`) doesn't fetch or +validate `group_type` at all. + +**Reproduction:** A site already has an Open restriction group configured +before upgrading to this fix, or an admin changes an existing Closed +restriction group to Open afterward. Restricting a user still succeeds; the +restricted user can resign via UCP, escaping the restriction while the +tracking row remains and blocks a second restriction attempt. + +**Fix:** Validate group type in `safe_group_name()` (or a restriction-specific +variant) at use time too, not only at save time. + +## 3. Medium — Expiry/purge can delete a pre-existing group membership that predates the restriction + +[event/banhammer_listener.php:617](../event/banhammer_listener.php#L617), +[cron/task/restriction_expiry.php:104](../cron/task/restriction_expiry.php#L104), +[migrations/restrict_group_id_column.php:116](../migrations/restrict_group_id_column.php#L116). + +The `GROUP_USERS_EXIST` branch added in `e42d7f1` handles the case where the +target already belongs to the restrict group, but the tracking row doesn't +record whether Ban Hammer's own action created that membership. Both +`undo_bh_group()`/expiry and purge unconditionally remove it regardless. + +**Reproduction:** A user already belongs to the configured restrict group for +an unrelated, legitimate reason (e.g. a permanent administrative +assignment). A moderator applies a temporary restriction. At expiry, their +pre-existing membership is removed along with the restriction - not just the +temporary effect. + +**Fix:** Record whether Ban Hammer's own action created the membership; +clean up only what it created. + +## 4. Medium — A pending (not yet approved) membership is treated as a successfully applied restriction + +[event/banhammer_listener.php:615-624](../event/banhammer_listener.php#L615). + +`GROUP_USERS_EXIST` from `group_user_add()` also covers pending membership +requests. The fallback (`group_user_attributes('default', ...)`) doesn't +check its own result, and core only sets the default group for *approved* +members - so a pending applicant's restriction silently never takes effect, +while the tracking row still claims it did. + +**Reproduction:** A user has a pending join request for a group an admin +later designates as the restrict group. Restricting them inserts the +tracking row and reports success, but their default group and effective +permissions never actually change; subsequent restriction attempts report +"already restricted." + +**Fix:** Inspect membership status explicitly (pending vs. approved); +approve through the correct core operation or reject the restriction outright +if membership can't be made effective. Check the fallback call's own result. + +## 5. Medium — Post deletion still wipes unrelated account data for the zero-posts and partial-deletion cases + +[event/banhammer_listener.php:803](../event/banhammer_listener.php#L803), +[event/banhammer_listener.php:861-877](../event/banhammer_listener.php#L861). + +The guard added in `4afa65e` only covers "there were posts, but every one +got filtered out by forum permission." It doesn't cover a target who simply +has zero posts to begin with, and it doesn't stop the account-wide cleanup +when deletion *partially* succeeds (some posts deleted in a permitted forum, +others left alone in a forum the moderator can't touch). + +Confirmed via a harness directly invoking the method: + +| Target's posts | Posts actually deleted | Account-wide DELETEs still run | +|---|---:|---:| +| None | none | 10 | +| All permission-blocked | none | 0 (fixed by `4afa65e`) | +| One permitted forum, one blocked | permitted post only | 10 | + +**Fix:** Either remove the broad account-data cleanup from post deletion +entirely (make it a separate, explicitly authorized action), or scope it more +precisely to what was actually touched. + +## 6. Medium — Ban-group cleanup still relies on current configuration, not the group actually applied to a given ban + +[event/banhammer_listener.php:440](../event/banhammer_listener.php#L440), +[event/banhammer_listener.php:700](../event/banhammer_listener.php#L700), +lines 704 and 731. + +This is the ban-side counterpart to finding #3 from the second review pass +(fixed for restrictions in `352e7b2`), but was explicitly out of scope for +this batch. Restated here as still-open: ban-group membership is never +recorded per user, so cleanup (`undo_bh_group`) can only compare against the +*current* `bh_group_id` setting, not whatever group a given ban actually +used. + +**Reproduction:** Ban a user, moving them into group A. Change the ACP +setting to group B (or disable group-moving) before the ban expires. On +expiry, cleanup checks B, leaving the user stuck in A. Conversely, an +unrelated legitimate member of the *currently* configured group B can have +their membership removed by cleanup if they're ever seen in a non-banned +session, even though Ban Hammer never added them. + +**Fix:** Give bans the same kind of per-action group tracking the restrict +feature now has. Larger design work, not a quick patch. + +## 7. Low — The restriction-group dropdown still offers choices the new validator rejects + +[controller/admin_controller.php:112](../controller/admin_controller.php#L112), +[controller/admin_controller.php:228](../controller/admin_controller.php#L228). + +Both the move-group and restrict-group dropdowns share one generator, which +still lists Open/Free groups. Selecting one for the restrict group now +produces a generic `FORM_INVALID` error with no explanation of why. + +**Fix:** Give the dropdown generator a restriction-specific filter (hide +Open/Free groups when rendering the restrict-group select), and/or a clearer +error message. + +## What was checked and cleared + +- **Confirmation/CSRF fallthrough** (round 1 finding #1): still fixed, both + ban and restrict paths correctly return after an unsuccessful confirmation. +- **Founder-managed group re-validation** (round 2 finding #2): confirmed + fixed at both save time and use time. +- **`undo_bh_group`'s restriction comparison** (round 2 finding #3): confirmed + it now correctly uses the restriction's own recorded group, not current + config. +- **Concurrent restriction creation**: the unique index correctly prevents a + *new* duplicate from being created once the migration is installed (the + installation-time gap is finding #1 above, not the runtime behavior). +- **PM/poll cleanup, MCP link defaults, SFS transport/settings-preservation + fixes**: all confirmed still correct. +- **The new `core.permissions` listener**: confirmed correctly integrated - + preserves existing permission definitions, supplies the right lang/category + keys, and the migration dependency chain (table → column → permission → + unique index) has no cycle. +- No SQL injection, XSS, or other injection issue found in reviewed request + handling, SQL construction, domain validation, the result-message + allowlist, templates, or JavaScript. +- All 22 PHP files pass `php -l` under PHP 8.4.25; JavaScript passes `node + --check`; YAML/`composer.json` parse; `git diff --check` passes. + +## Note on verification + +This was Codex's own review output, using in-memory/harness reproductions +(SQLite, isolated PHP snippets) rather than a live phpBB install or the +project's actual CI (which has database and functional jobs disabled on this +branch, since those flags only exist on the separate, still-open +`epv-and-test-coverage` branch). Findings were **not** independently +re-verified line-by-line by Claude before this file was written; that +verification pass happens next, in the conversation, before any fix is +applied. diff --git a/docs/codex-review-2026-09-22-round4.md b/docs/codex-review-2026-09-22-round4.md new file mode 100644 index 0000000..5f35883 --- /dev/null +++ b/docs/codex-review-2026-09-22-round4.md @@ -0,0 +1,145 @@ +# Codex code review — round 4 — 2026-09-22 + +Full-repository review by [Codex](https://openai.com/codex/) (OpenAI), run via the +`local-codex` MCP wrapper against branch `codex-review-round-2` at +`be9676c22815e83022ba3be4646ea2826089f42a` (master `fb99cd8` plus 12 commits). +Fourth review pass. Read-only; no files were modified. + +**Codex's own verdict: "Changes recommended" — 3 Medium, 1 Low, no new High. +Approaching diminishing returns but not quite a stopping point; recommends +fixing these, explicitly accepting/scheduling the deferred ban-group design +issue, and moving to focused upgrade/purge and permission-matrix integration +tests rather than another unrestricted static-review pass.** + +## 1. Medium — Legacy restrictions get an unjustified `restrict_new_membership = 1` + +[migrations/restrict_membership_column.php:39](../migrations/restrict_membership_column.php#L39), +removal at +[cron/task/restriction_expiry.php:110](../cron/task/restriction_expiry.php#L110), +[migrations/restrict_membership_column.php:99](../migrations/restrict_membership_column.php#L99). + +The migration's comment claims existing rows are "accurately" defaulted to +`1` because pre-fix code always resulted in a fresh membership. That's +wrong: before round 3's `GROUP_USERS_EXIST` handling existed, +`do_restrict_stuff()` inserted the tracking row and **ignored** +`group_user_add()`'s return value entirely - so a user who already belonged +to the restrict group before being "restricted" would still get a tracking +row, with no way to tell, after the fact, whether membership was created or +pre-existing. + +**Reproduction:** On `fb99cd8` (before round 3), restrict a user already +belonging to the configured restrict group. Upgrade to this branch. Their +row gets `restrict_new_membership = 1` by the backfill. At expiry or purge, +their pre-existing membership gets removed - the exact bug round 3's column +was meant to close, now reintroduced for every restriction created before +the column existed. + +**Fix:** Don't claim certainty for legacy rows. Either leave them +unreconciled with an explicit note that upgrade-time correctness for +pre-existing rows is a known limitation, or attempt reconciliation (e.g. +compare `original_group_id` against `restrict_group_id`, or query current +membership timing if available) rather than blanket-defaulting to 1. + +## 2. Medium — Purge before `restrict_membership_column` is installed skips restoration entirely + +[migrations/restrict_membership_column.php:60](../migrations/restrict_membership_column.php#L60), +[migrations/restrict_group_id_column.php:43](../migrations/restrict_group_id_column.php#L43). + +Round 3 moved `restore_active_restrictions()` out of +`restrict_group_id_column.php` (already on `master` via #36) into the new +`restrict_membership_column.php`, reasoning that the older migration's +revert runs *after* the newer one and would find the column already +dropped. + +**Reproduction:** A site is running `fb99cd8` (has `restrict_group_id_column` +installed, does NOT have `restrict_membership_column` - it doesn't exist +yet). The admin disables the extension and purges it (deletes data) via the +normal "Disable → Delete data" ACP flow, **without re-enabling it first** +to pick up new migrations. phpBB's purge only reverts migrations that are +actually installed; it doesn't discover and install new ones first to then +revert them. `restrict_membership_column`'s `revert_data()` therefore never +runs, and `restrict_group_id_column.php` (the migration that IS installed) +no longer has any restoration logic at all, since round 3 removed it. The +tracking table gets dropped with active restrictions never restored - worse +than round 2's original purge behavior before either migration existed. + +**Fix:** Keep a restoration fallback in the older, already-installed +migration too (idempotent - if `restrict_membership_column` already handled +it and cleared the table, the older one's fallback finds nothing to do and +is a harmless no-op). + +## 3. Medium — Zero-post cleanup still runs for moderators who *do* have the global delete permission + +[event/banhammer_listener.php:815](../event/banhammer_listener.php#L815), +[line 829](../event/banhammer_listener.php#L829), +[line 893](../event/banhammer_listener.php#L893). + +Round 3's fix for "del_posts wipes account data even with zero posts" is +nested inside the `if (!$this->auth->acl_get('m_banhammer_del_posts_all'))` +branch. A moderator who *does* have that permission never reaches the +empty-`$posts` check at all - the account-wide cleanup (bookmarks, drafts, +notifications, etc.) still runs unconditionally even when the target has no +posts and the moderator's post-deletion authority had literally nothing to +act on. + +**Reproduction:** Confirmed via harness: a target with zero posts, deleted +by a moderator with `m_banhammer_del_posts_all`, still triggers 10 +account-wide DELETE statements despite `delete_posts()` doing nothing. + +**Fix:** Move the empty-`$posts` check outside the permission branch, after +filtering, so it applies regardless of which permission path granted access. + +## 4. Low — Partial post deletion still wipes `TOPICS_POSTED_TABLE` across every forum + +[event/banhammer_listener.php:904](../event/banhammer_listener.php#L904). + +When a moderator can delete posts in forum A but not B, posts in B correctly +survive, but the unconditional `DELETE FROM TOPICS_POSTED_TABLE WHERE +user_id = ...` still removes the user's "posted in this topic" markers for +*every* topic, including ones in B where their post is untouched. phpBB's +own `delete_posts()` already keeps this table in sync for the posts it +actually deletes; this blanket delete undoes that bookkeeping for surviving +content. + +**Fix:** Remove the blanket `TOPICS_POSTED_TABLE` delete and rely on core's +own synchronization from `delete_posts()`. + +## Deferred issue restated, not new + +The ban-group tracking gap (`undo_bh_group()` has no per-ban record of which +group a given ban actually used) is still present, as expected - this was +explicitly deferred in round 2/3 as out-of-scope design work, not +re-counted as a new finding here. + +## What was checked and cleared + +- Migration ordering on a **fully upgraded** installation: dedup → unique + index → membership column is correctly sequenced; purge-time data revert + correctly runs before schema revert. (The gap is specifically the + *partially* upgraded case, finding #2 above.) +- New restriction requests: ownership recorded correctly going forward; + pending membership correctly fails without a stray tracking row; the + unique index correctly prevents a concurrent duplicate insert. +- Founder-management and Open/Free group validation: present and consistent + at both save time and use time, and the dropdown now agrees with the + validator. +- All previously-fixed items from rounds 1-3 re-confirmed still correct: + confirm-box bypass, permission checks on privileged actions, form-token + check, `bh_res` allowlist, domain validation, MCP link defaults, domain-ban + include, restriction restoration using the recorded group, SFS + settings/transport handling, poll-vote preservation, PM cleanup delegating + to core. +- All 24 PHP files pass `php -l`; migrations, YAML, templates, JS, CSS, + language files, and package metadata reviewed with no new injection, + script-execution, or secret-exposure issue found. + +## Note on verification + +Source review plus bounded in-memory/harness reproductions (Codex's own), +not a live phpBB install or the project's real CI (no functional/DB test +jobs are enabled on this branch's own workflow config). Not yet +independently re-verified by Claude line-by-line - that happens next, before +any fix is applied. Codex's own recommendation: this is approaching +diminishing returns for further *unrestricted* static review, but these four +findings are concrete rather than speculative and worth fixing before +moving to integration testing instead of another open-ended pass. diff --git a/docs/codex-review-2026-09-22.md b/docs/codex-review-2026-09-22.md new file mode 100644 index 0000000..0b2c4b6 --- /dev/null +++ b/docs/codex-review-2026-09-22.md @@ -0,0 +1,228 @@ +# Codex code review — 2026-09-22 + +Full-repository review by [Codex](https://openai.com/codex/) (OpenAI), run via the +`local-codex` MCP wrapper against this repository's `master` branch +(`ff149b8`). Read-only: all 18 PHP files, YAML config/workflows, all five +templates, JavaScript, CSS, migrations, and language files. No files were +modified during the review. + +Findings are ordered by severity. Line references point at the reviewed +commit and may drift as the file changes. + +## 1. High — `cancel=1` bypasses confirmation and executes moderation actions + +[event/banhammer_listener.php:294](../event/banhammer_listener.php#L294), +[line 343](../event/banhammer_listener.php#L343), and +[line 527](../event/banhammer_listener.php#L527). + +Send a POST to `memberlist.php?mode=viewprofile&u=TARGET&bh=1` containing +`cancel=1`, without `confirm_key`. The initial guard permits it; +`confirm_box(true)` returns false, and `confirm_box(false)` also returns +false because cancellation was requested. Execution then falls through to +`user_ban()` and any requested deletions. The restriction handler has the +same flaw. + +An attacker can exploit an authenticated moderator's browser when its +session accompanies the forged request (CSRF). Verified by reading phpBB +3.3.x's `confirm_box()` behavior directly — Codex reproduced both execution +paths with isolated harnesses. Mutations must execute only inside an +explicitly successful confirmation branch. + +## 2. High — Group configuration bypasses founder-only group management + +[controller/admin_controller.php:136](../controller/admin_controller.php#L136), +[line 154](../controller/admin_controller.php#L154), and +[event/banhammer_listener.php:562](../event/banhammer_listener.php#L562). + +The ACP module requires only `a_user`. Group selection neither filters +`group_founder_manage` nor validates submitted group IDs. A non-founder +administrator with `a_user` and `m_ban` can configure a founder-managed +privileged group as the restriction group, then "restrict" another account +into it, granting that account its permissions. A crafted submission also +bypasses the dropdown's exclusion of special groups. + +phpBB's own user administration controller explicitly rejects this +operation for non-founders. Validate group eligibility and founder +restrictions when saving and applying the configuration. + +## 3. High — Ban permission grants unrestricted post deletion + +[event/banhammer_listener.php:190](../event/banhammer_listener.php#L190), +[line 382](../event/banhammer_listener.php#L382), and +[line 723](../event/banhammer_listener.php#L723). + +A moderator with `m_ban`, but without deletion permission in a particular +forum, can submit `del_posts=1` and permanently delete the target's posts +across every forum. Neither the query nor execution checks forum-specific +deletion permissions. Setting the ACP deletion default to "No" does not +prevent this. + +If this broad authority is intentional, it should have an explicit +extension ACL. Otherwise, enforce the corresponding permissions before +deleting content. + +## 4. Medium — Confirmed domain bans call an unloaded function + +[controller/ban_domain_controller.php:96](../controller/ban_domain_controller.php#L96). + +The routed controller calls `user_ban()` without loading +`includes/functions_user.php`. Standard `app.php`/`common.php` do not load +that file. On a normal installation without another extension incidentally +including it, confirming a domain ban produces an undefined-function error +and creates no ban. + +Load the dependency explicitly, as the profile listener already does. + +## 5. Medium — Expiry does not restore the original default group + +[cron/task/restriction_expiry.php:103](../cron/task/restriction_expiry.php#L103), +[line 108](../cron/task/restriction_expiry.php#L108). + +Restricting a user preserves their original group membership. At expiry, +removing the restriction group chooses a special-group default; +subsequently calling `group_user_add()` for the original group returns +`GROUP_USERS_EXIST` before changing the default. A user whose original +default was a custom group therefore keeps the wrong default, colour, or +rank. The tracking record is deleted regardless. + +Restore the default using the appropriate group-attribute operation and +check errors. + +## 6. Medium — Changing the configured restriction group strands existing restrictions + +[cron/task/restriction_expiry.php:94](../cron/task/restriction_expiry.php#L94) +and [migrations/restrict_group.php:30](../migrations/restrict_group.php#L30). + +Restrict a user into group A, then change the ACP setting to group B or "No +group" before expiry. The cron task removes B — or nothing — instead of A, +then deletes the tracking record. The user remains in A indefinitely. If +they legitimately belong to B, that membership can also be removed. + +Store the applied restriction group ID in each tracking record. + +## 7. Medium — Using the same group for bans and restrictions immediately undoes restrictions + +[event/banhammer_listener.php:596](../event/banhammer_listener.php#L596), +[line 613](../event/banhammer_listener.php#L613). + +Both ACP dropdowns permit selecting the same group. A restricted user is +deliberately not banned, so the next session ban check causes +`undo_bh_group()` to remove them from that shared group. Their restriction +record remains, and moderators see "already has an active restriction" +despite its permissions no longer applying. + +Reject this configuration or make ban-group cleanup aware of active +restrictions. + +## 8. Medium — Private-message deletion leaves counters and attachments inconsistent + +[event/banhammer_listener.php:643](../event/banhammer_listener.php#L643). + +Deleting a spammer's messages removes message and recipient rows directly, +without updating recipients' unread/new-message counts or custom-folder +counts, deleting PM notifications, or removing attachments. A recipient can +retain an unread notification pointing to a nonexistent message; +attachment files and database records remain after their parent message +disappears. + +Use a deletion path that performs the bookkeeping implemented by phpBB's +PM functions. + +## 9. Medium — Deleting poll votes corrupts totals and permits repeat voting + +[event/banhammer_listener.php:732](../event/banhammer_listener.php#L732). + +The cleanup removes the user's `POLL_VOTES_TABLE` rows, including votes in +other users' surviving topics, without decrementing poll-option totals. +After a temporary ban expires, the user can vote again because their +previous-vote record is gone, while the original vote still contributes to +the total. + +Preserve these votes or update the associated totals consistently. + +## 10. Medium — MCP quick bans ignore configured defaults and become permanent + +[event/banhammer_listener.php:136](../event/banhammer_listener.php#L136), +[line 297](../event/banhammer_listener.php#L297). + +The MCP link supplies `bh=1` without any options, bypassing the profile +form. Missing parameters default to zero: permanent duration, no email/IP +ban, no deletion, no group move, and no SFS report. + +For example, an administrator's seven-day ban default becomes a permanent +username-only ban through the MCP shortcut. Open the options form or +populate the confirmation from configured defaults. + +## 11. Medium — Failed SFS transfers can be reported as successful + +[event/banhammer_listener.php:747](../event/banhammer_listener.php#L747). + +`curl_exec()`'s return value is discarded; only the HTTP status is +examined. If the server sends HTTP 200 headers and the transfer +subsequently times out, `curl_exec()` returns false but `get_file()` +returns true. The moderator receives "All actions were performed +correctly" despite an unsuccessful transfer. + +Reproduced with a simulated false cURL result and HTTP 200. Check the +transfer result and validate the response body. + +## 12. Medium — Purging the extension makes temporary restrictions permanent + +[migrations/restrict_group.php:46](../migrations/restrict_group.php#L46). + +Purging extension data drops the restriction tracking table without +removing applied group memberships or restoring users' defaults. A user +with a one-day restriction remains restricted after purge, and the +information needed to restore them is lost. + +Restore tracked users before dropping the table, or prevent purge until +outstanding restrictions are resolved. + +## 13. Low — Result styling generates malformed HTML + +[event/banhammer_listener.php:237](../event/banhammer_listener.php#L237) and +[memberlist_view_content_prepend.html:2](../styles/prosilver/template/event/memberlist_view_content_prepend.html#L2). + +`BH_STYLE` ends with a double quote even though the template supplies its +own attribute quotes. Successful and failed action results produce +malformed attributes such as `style="background-color: green; color: +white;";"`. Remove the embedded quote. This value is fixed text, not an +XSS finding. + +## 14. Low — The ACP saved-message branch is dead code + +[adm/style/banhammer_body.html:5](../adm/style/banhammer_body.html#L5). + +Nothing assigns `S_SAVED`; successful saves terminate through +`trigger_error()` instead. This success box can never appear through the +extension's normal flow. Remove it or implement that rendering path. + +## What was checked and cleared + +All PHP files passed syntax checks under PHP 8.4.25. Codex checked upstream +phpBB source directly and ran isolated, in-memory harnesses; no installed +phpBB database/browser integration environment was available for this +review. + +No confirmed SQL injection or XSS was found in the reviewed input paths. +Interpolated user IDs are integer-derived, other SQL uses DBAL builders, +and phpBB's request handling escapes string inputs. ACP saves have a +form-token check. Services and event registration generally follow +extension conventions, and no executable code modifies core files. No +leftover debug output was found. + +## Overall assessment + +The extension has a conventional structure, but the confirmation-bypass +(#1) and permission-boundary issues (#2, #3) need fixing before deployment. +Restriction lifecycle handling and destructive cleanup also need +functional regression coverage. + +## Note on verification + +Claude spot-checked finding #1 by reading +[event/banhammer_listener.php:293-348](../event/banhammer_listener.php#L293-L348) +directly: the cited `confirm_box(true)` / `confirm_box(false, ...)` calls +and the fall-through to the ban code are exactly as described. The rest of +this report reflects Codex's findings as returned, not independently +re-verified line by line. diff --git a/docs/lumo-review-2026-09-22.md b/docs/lumo-review-2026-09-22.md new file mode 100644 index 0000000..7a2d645 --- /dev/null +++ b/docs/lumo-review-2026-09-22.md @@ -0,0 +1,102 @@ +# Lumo code review — 2026-09-22 + +Full-repository review by [Proton Lumo](https://lumo.proton.me/) (Proton +AG), run via `lumo-tamer`'s CLI against a single bundled text dump of this +repository's source (all `git ls-files` content, concatenated with +`=== FILE: path ===` markers — Lumo has no filesystem/tool access of its +own in this setup, so it worked from that one paste rather than reading the +repository directly). + +**Read this alongside the same day's [Codex review](codex-review-2026-09-22.md).** +Lumo's output was considerably less reliable: its headline "High" +severity finding is a confirmed false positive (see below), and several +other findings visibly reverse themselves mid-answer ("Correction:", +"Re-evaluation:", "The Actual Bug:" repeated three times for the same +item) before landing on a final claim. Findings are reproduced below for +the record, with verification notes. + +## Confirmed false positive: "High — Logic Error: Success/Fail Inversion" + +Lumo's top claim: `event/banhammer_listener.php`'s group-move check — + +```php +$return = group_user_add($this->config['bh_group_id'], array($this->user_id), array($this->data['username']), $group_name, true); + +if ($return != false) +{ + $error[] = 'ERROR_MOVE_GROUP'; +} +``` + +— supposedly treats success as failure, because Lumo assumed +`group_user_add()` returns `true`/truthy on success. + +**This is wrong.** Claude checked phpBB 3.3.x's actual +[`group_user_add()`](https://github.com/phpbb/phpbb/blob/3.3.x/phpBB/includes/functions_user.php#L2716) +source directly: the success path ends with the comment `// Return false - +no error` followed by `return false;`. Every early-exit/error path (e.g. +`'NO_USER'`, `'GROUP_USERS_INVALID'`, `'GROUP_USERS_EXIST'`) returns a +non-empty string instead. `group_user_del()`'s docblock in the same file +states the convention explicitly: "@return false if no errors occurred, +else the user lang string for the relevant error." So `$return != false` +is true only when an actual error string came back — the code's logic is +**correct**, not inverted. No fix needed here. + +## Other findings, as returned + +Severity/labels are Lumo's own; not independently re-verified beyond the +spot checks noted. + +- **Medium — Direct SQL interpolation in `bh_del_privmsgs()`** + (`event/banhammer_listener.php`, `WHERE author_id = $user_id`, no + explicit `(int)` cast at the point of use). Claude checked: `$user_id` + here is `$this->user_id`, a class property set internally, not read + directly from the request at this point — so this isn't an exploitable + path today, but Lumo's suggestion to cast explicitly at the query site + (defense in depth, matches normal phpBB style) is fair. Consistent with + Codex's same-day review, which also found no confirmed SQL injection. +- **Low — `ban_domain_controller.php` relies on `confirm_box` rather than + explicit `add_form_key`/`check_form_key`.** Not independently checked; + worth comparing against Codex's finding #1 (a real confirmed CSRF/ + confirmation-bypass in the *listener's* confirm_box handling, different + code path) before acting on this one. +- **Low — SFS `get_file()` doesn't validate response content, only HTTP + status.** Same underlying issue as Codex's finding #11, described less + precisely (Codex identified the exact bug: `curl_exec()`'s return value + is discarded). +- **Low — `cron/task/restriction_expiry.php` has no transactional + safety; a partial failure mid-loop can delete the tracking row before + the group is restored.** Same underlying issue as Codex's finding #5/#6, + described less precisely. +- **Low — dead "manage" mode reference in `acp/banhammer_module.php`.** + Not checked. +- **Medium — `trigger_error(E_USER_WARNING)` used for user-facing errors + instead of phpBB's redirect/exception conventions.** Plausible style + critique; not independently checked. + +## Overall assessment (Lumo's own words) + +> The `ban-hammer` extension is functional and follows most phpBB 3.3.x +> architectural conventions (services, events, migrations). However, it +> contains a critical logic bug in the group moving functionality that +> causes successful operations to be flagged as failures, confusing +> administrators. [...] Addressing the logic inversion and standardizing +> error handling should be the top priority. + +**Claude's assessment of this review:** the recommended top priority is +based on a misread of phpBB's own API contract and should not be acted +on. Treat this review as a rougher, less trustworthy second opinion than +the same-day Codex review — useful for the overlapping findings (SFS +error handling, cron transactional safety) as corroboration, but verify +independently before acting on anything unique to this report. + +## Setup notes + +`lumo-tamer`'s CLI has no working `-u`/`-q`/`--upload` flags despite the +project README documenting them: its one-shot mode is +`if (query && !query.startsWith('-')) { singleQuery(...) }` +([src/cli/client.ts:41](../../lumo-tamer/src/cli/client.ts#L41)) — any +argument starting with `-` falls through to interactive mode instead, +which then exits immediately on closed stdin. Worked around by passing +the full instructions + bundled source as one plain-text argument with no +leading `-` flags. From 2a7e8f9871e9d892c49c9fc901baef4263a30655 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Sun, 27 Sep 2026 20:32:26 -0500 Subject: [PATCH 09/10] Enforce LF line endings with .gitattributes "* text=auto eol=lf" so text files are stored with LF, plus binary rules for image types so their bytes are never converted. Existing export-ignore rules are kept. Co-Authored-By: Claude Opus 5.5 --- .gitattributes | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..f629ff1 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,10 @@ +# Normalize line endings to LF in the repository +* text=auto eol=lf + +# Never convert line endings in binary files +*.png binary +*.jpg binary +*.jpeg binary +*.gif binary +*.webp binary +*.ico binary From 4a1b6b628bddcce4c90a84fe4d18ef6308c73e97 Mon Sep 17 00:00:00 2001 From: William Jacoby Date: Sun, 27 Sep 2026 21:02:08 -0500 Subject: [PATCH 10/10] Make the confirm-bypass test independent of the response body The cancelled request has intermittently returned an empty body in CI (MSSQL 2019 in one run, SQLite in another), failing the default full-HTML-page check before the test reached its real assertion. Check for a server error or PHP notice instead, then that no ban was created. Co-Authored-By: Claude Opus 5.5 --- tests/functional/confirm_bypass_test.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/functional/confirm_bypass_test.php b/tests/functional/confirm_bypass_test.php index a831a44..a263c87 100644 --- a/tests/functional/confirm_bypass_test.php +++ b/tests/functional/confirm_bypass_test.php @@ -63,12 +63,20 @@ public function test_cancel_does_not_bypass_confirmation() $this->login(); // The exploit: a bh=1 request with no confirm_key, but cancel=1. + // A cancelled confirmation doesn't always come back as a full HTML + // page (in CI it has intermittently returned an empty body), so + // don't require one: check there was no server error or PHP notice, + // then check what matters, that no ban was created. self::request( 'POST', 'memberlist.php?mode=viewprofile&u=' . $victim_id . '&bh=1&sid=' . $this->sid, - array('cancel' => '1') + array('cancel' => '1'), + false ); + $this->assertLessThan(500, self::$client->getResponse()->getStatus(), 'The cancelled request must not cause a server error'); + $this->assertStringNotContainsString('[phpBB Debug]', self::get_content()); + $db = $this->get_db(); $sql = 'SELECT COUNT(*) as cnt FROM ' . BANLIST_TABLE . '