fix: point all three scan paths at events that actually exist - #1
Merged
Conversation
K5: composer.json declared a `path` repository pointing at a sibling
checkout of the PHP SDK (`../spamtroll-php-sdk`, pinned to 0.9.2). On any
machine without that sibling directory `composer install` aborts hard
("The `url` supplied for the path repository does not exist"), so neither
the documented install steps nor CI could ever fetch dependencies. The SDK
is published on Packagist, so the block is simply removed and the
constraint raised to `^0.9.3`.
W6: `composer validate --strict` failed on the `version` field, which is
the first step of the QA workflow — PHPStan and PHPUnit were reported as
`skipped` on every run and had never executed. Field dropped.
Also:
- `require.php` raised to `>=8.2` and the CI matrix moved to 8.2/8.3/8.4
to match the project baseline.
- `composer.lock` is now committed so release artefacts are reproducible.
- PHPUnit bumped to ^10.5 (9.6 is EOL and unusable on PHP 8.4);
phpunit.xml.dist migrated to the 10.5 schema (`cacheDirectory`,
`<source>` instead of `<coverage><include>`).
- New `release.yml` workflow builds `spamtroll_phpbb_<version>.zip`
containing `ext/spamtroll/phpbb/` **with** a production `vendor/` tree,
which is what the phpBB Extension Database expects; the job asserts
`vendor/autoload.php` and the SDK are present before zipping.
- README install steps rewritten accordingly (the old
`composer create-project` recipe could not have worked).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
None of the three scanners were reachable in production.
K1 — registration. `core.user_add_modify_data` fires inside user_add()
(includes/functions_user.php:290) with vars user_row, cp_data, sql_ary,
notifications_data. There is no `error`, and phpBB discards any key a
listener adds: dispatcher::trigger_event() returns
get_data_filtered(array_keys($data)) (phpbb/event/dispatcher.php:47),
i.e. array_intersect_key (phpbb/event/data.php:44). The registration was
also already past validation at that point. Moved to
`core.ucp_register_data_after` (includes/ucp/ucp_register.php:329-338,
vars submit/data/cp_data/error), reading $data['username'] and
$data['email']; a non-empty $error aborts the form at :339.
K2 — posts. $post_data['message'] never exists: posting.php:642-645 binds
the text into the parser and unsets the key, and only restores it at
:1807, after the event at :1428. `message_parser` is not in the event's
variable list either. We now read the same request field phpBB reads at
posting.php:941 — $request->variable('message', '', true).
K3 — private messages. `core.ucp_pm_compose_modify_parsed_text` does not
exist anywhere in phpBB 3.3.x, so check_pm() was never invoked; the old
handler also wrote into $message_parser->warn_msg, which
ucp_pm_compose.php:876-888 consumes *before* either parse_* event.
Moved to `core.ucp_pm_compose_modify_parse_before`
(includes/ucp/ucp_pm_compose.php:845-868). The *before* variant, not
*after*, because parse_message::parse() replaces
$message_parser->message with the s9e TextFormatter XML representation
(includes/message_parser.php:1251) — unusable as scanner input. Both
variants expose `error`, and ucp_pm_compose.php:947
(`if (!count($error) && $submit)`) is what stops the PM.
Also:
- errors are appended via offsetSet on the pre-existing `error` key, so
they survive get_data_filtered();
- content is html_entity_decode()d, because phpBB's request layer
htmlspecialchars()es every string variable
(phpbb/request/type_cast_helper.php:46);
- handlers bail out unless `submit` is truthy. posting.php:937 and the
PM equivalent run the whole block on preview and refresh too, where
raising a blocking error is wrong and a scan would needlessly consume
the daily quota.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…structor
K4: opening ACP → Spamtroll Settings threw
`ArgumentCountError: Too few arguments to function
spamtroll\phpbb\acp\main_module::__construct(), 1 passed, 7 expected`.
phpBB does not build ACP modules through the DI container.
includes/functions_module.php:598-600 does
$class_name = $this->p_name;
$this->module = new $class_name($this);
and the file has no reference to $phpbb_container at all, so the
`spamtroll.phpbb.acp.module` service definition was never read by anyone
— dead code, now removed from services.yml.
The constructor is now optional-arg only (it receives the module manager
and ignores it) and main() pulls $config, $language, $request, $template,
$user and $phpbb_container from the global scope, taking the two
extension services out of the container. That is the same pattern phpBB's
own bundled modules use — see includes/acp/acp_extensions.php:41-55.
Those globals are registered by register_compatibility_globals()
(includes/compatibility_globals.php:40,60).
set_dependencies() is kept as a seam so the module stays unit-testable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
W4: cron/task/cleanup_logs.php existed but had no entry in
config/services.yml and no `{ name: cron.task }` tag, so the class was
never instantiated. Consequences: the ACP "Log retention (days)" setting
did nothing, phpbb_spamtroll_log grew unbounded, and personal data (IP,
username, a slice of the content) was retained indefinitely.
Registered following the core pattern
(config/default/container/services_cron.yml), including the
`set_name` call the cron manager needs to address the task by name.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
W1 — the one real fail-open hole in the extension. scanner::safe_log() wraps the write in `catch (\Throwable)`, but phpBB does not throw on SQL errors: \phpbb\db\driver\driver::sql_error() ends in trigger_error($message, E_USER_ERROR) (driver.php:1028-1031), which runs msg_handler() → exit_handler(). No catch block intercepts that; the request dies half way through submitting a post — fail-closed. Two realistic triggers: 1. Missing phpbb_spamtroll_log (extension copied without running the migrations, or a migration that stopped half way). 2. substr($preview, 0, 500) cut by *bytes*. A multibyte character straddling byte 500 leaves a broken sequence, which MySQL in strict mode rejects on a utf8mb4 column with "Incorrect string value" — the same fatal. Any Polish or German post can hit this. Fixes: - INSERT now runs between sql_return_on_error(true)/(false), so sql_error() returns the error array instead of triggering the fatal (the `if (!$this->return_on_error)` guard at driver.php:989) and the existing catch in the scanner sees an ordinary failure. - All string columns are cut with utf8_substr() (phpBB always loads includes/utf/utf_tools.php — common.php:87), falling back to mb_substr() and finally to a byte cut that strips a trailing partial sequence. Character-based truncation is also what the VCHAR columns want. - `symbols` is built by dropping whole entries until the encoded JSON fits, instead of truncating the JSON text into an unparseable document (audit N1). Also W4/second half: cleanup() deletes in batches of 500 via a SELECT log_id → DELETE ... IN() round trip instead of one unbounded DELETE. The first run against a board that has been logging without retention would otherwise hold a very long lock. The round trip keeps it portable; PostgreSQL has no `DELETE ... LIMIT`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
W7: tests/bootstrap.php stubbed \phpbb\event\data with a public $data
array and a set_data() that swapped the whole thing, so adding a brand
new key "worked" and registration_test passed against code that was a
no-op in production.
The stub is now a port of the real class, including get_data_filtered()
— array_intersect_key($this->data, array_flip($keys)),
phpbb/event/data.php:44 — and $data is private, as it is upstream.
tests/support/event_harness.php reproduces
\phpbb\event\dispatcher::trigger_event() end to end
(phpbb/event/dispatcher.php:43-48): build the event from the documented
variable list, run the handler, hand back only get_data_filtered(). Tests
assert on that return value, which is what phpBB actually extract()s.
The harness also carries the three real variable lists, so the fixtures
double as executable documentation — notably that post_data has neither
`message` nor `post_text` at core.posting_modify_submission_errors.
Supporting doubles: fake_request (applies the same htmlspecialchars the
request layer does, phpbb/request/type_cast_helper.php:46),
fake_message_parser, fake_db (records the row, the return_on_error flips,
and throws where phpBB would trigger_error(E_USER_ERROR)), recording_http
(records call count and request bodies).
New/rewritten suites: tests/listener/{registration,post,pm}_test.php,
tests/logger/logger_test.php, tests/acp/main_module_test.php.
Verified as regression tests. Against the pre-fix
event/main_listener.php, service/logger.php and acp/main_module.php the
suite reports 19 failures and 4 errors out of 43; against the fixed tree
it is 43/43 green. Every "was not scanned" test asserts on the recorded
HTTP call count rather than on an exception, so it cannot be satisfied by
the scanner's fail-open catch swallowing a guard.
Also fixed a dead PHPStan ignore: '#given\\.$#i' — NEON single quotes are
literal, so that pattern looked for a backslash and never matched
anything. Replaced with a targeted rule for test doubles standing in for
un-autoloadable phpBB types, which let tests/ come back into the analysis
set instead of being excluded file by file.
PHPStan level 5 reports no errors; `composer validate --strict` passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Also annotates the three scan-path bullets and the cron bullet in the 0.1.0 entry as never having been functional, rather than rewriting a released section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing calls it; main() resolves everything from the global scope the way phpBB's bundled modules do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Problem
The extension was functionally dead: none of the three scan paths ever ran, and the ACP module crashed on open.
core.user_add_modify_data, which has noerrorvariable —data.php:44array_intersect_keysilently dropped the injected key$post_data['message'], which never exists inposting.php;post_textis unset at:645and only restored at:1807, i.e. after the eventcore.ucp_pm_compose_modify_parsed_text, an event that does not exist in phpBB at allfunctions_module.php:600doesnew $class_name($this)with no container, while the constructor required 7 arguments →ArgumentCountErrorcomposer installhard-failed on arepositories: pathentry pointing at../spamtroll-php-sdk, andvendor/was gitignored, so the ZIP shipped without the SDKChanges
All three paths now hook events that exist and expose
error, verified against phpBB 3.3.15:core.ucp_register_data_afterucp_register.php:338if (!count($error))core.posting_modify_submission_errorsposting.php:1428if (!count($error) && $submit)core.ucp_pm_compose_modify_parse_beforeucp_pm_compose.php:860Post content is read from
request->variable('message')rather than the strippedpost_data, and PM content from the parser with a request fallback.Also: ACP dependencies resolved from the container;
cleanup_logsregistered inservices.ymlso retention is no longer fiction; the audit logger no longer truncates mid-UTF-8 (substr→ byte-safe), which used to triggertrigger_error(E_USER_ERROR)on utf8mb4 strict and kill the request — a fail-closed path a Polish post hits easily;repositories: pathdropped in favour of Packagist.Verification
The event stub in the test bootstrap now mirrors phpBB's real
array_intersect_keyfiltering; previously it masked the registration bug.Found by the audit in
spamtroll/docs/audit/plugins/PHPBB.md(K1–K5, W1, W4, W6, W7).🤖 Generated with Claude Code