Conversation
…() 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #36. Two things:
EPV findings. EPV's own exit code only reflects its Fatal count, never Errors, so a green "Run EPV" step in CI has never meant zero findings — this extension has quietly carried 5 EPV errors on every run since before #36. Of those:
unserialize()inmigrations/v101_data.phpnow usesallowed_classes => falseto close off object injection. This does not clear EPV's flag on that line — its check is a blanket "was unserialize() called here" heuristic with no argument inspection — but the fix is worth having regardless.htmlspecialchars()on the MCP domain-ban link is intentionally left as-is and documented: phpBB's Twig templates run withautoescapeoff, and this value has no character-set restriction (unlike the regex-validated domain in the other ban-domain path), so removing it to satisfy EPV would reopen an XSS gap.phpbb-extensions/test-framework's reusable workflow invokes EPV, not something fixable in this repo.First test suite. This extension had zero automated tests. Added:
tests/cron/restriction_expiry_test.php— a real DB-backed test proving restriction expiry restores each user's own original group and clears the group actually used for their restriction, not a shared/stale config value, and leaves a not-yet-expired restriction alone.tests/functional/confirm_bypass_test.php— sends the exactcancel=1exploit request from Security and correctness fixes from a Codex-assisted review #36's first fix against a real running install and asserts no ban was created.RUN_MSSQL_JOBS(bundles the SQLite3 job the suite needs) andRUN_FUNCTIONAL_TESTSon in CI, since neither was previously enabled and these tests would otherwise never run.Not covered: the shared-group
undo_bh_groupfix and the purge-time restoration migration from #36, both already verified manually end-to-end. Reasonable follow-up, not attempted here.Important caveat
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, a PHP 8.1+ incompatibility in PHPUnit 7's own code. These two test files were written carefully, modeled on real working phpBB core andphpbb-ext-acme-demopatterns, but have not been executed anywhere before this PR. This CI run is their first real execution — they may need iteration before they pass.cc @kaileymsnay — you added the current CI testing framework in #31, so flagging this since it changes that workflow's config (
RUN_MSSQL_JOBS,RUN_FUNCTIONAL_TESTS) and adds the first tests that actually run under it.Investigated and written by Claude on behalf of William Jacoby (bonelifer).