Skip to content

[ticket/17665] Add automatic admin notifications on security updates - #7012

Merged
marc1706 merged 17 commits into
phpbb:3.3.xfrom
marc1706:ticket/17665
Sep 17, 2026
Merged

marc1706 merged 17 commits into
phpbb:3.3.xfrom
marc1706:ticket/17665

Conversation

@marc1706

@marc1706 marc1706 commented Jul 27, 2026

Copy link
Copy Markdown
Member

Checklist:

  • Correct branch: master for new features; 3.3.x for fixes
  • Tests pass
  • Code follows coding guidelines: master and 3.3.x
  • Commit follows commit message format

Tracker ticket:

https://tracker.phpbb.com/browse/PHPBB-17665

@marc1706 marc1706 added this to the 3.3.18 milestone Jul 27, 2026
@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown

The attempt to merge branch 3.3.x into master has completed after considering the changes in this PR.

  • Merge result: Conflict ❌

A separate PR will be needed to merge 3.3.x into master.

@ECYaz

ECYaz commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Some findings from looking into the red CI, verified locally against this branch (phpunit 9.6.35, MySQL 8 strict mode, PostgreSQL 16):

1. The whole matrix is red from one failure. migrations_check_config_added_test fails because the two new config options are missing from phpBB/install/schemas/schema_data.sql, and fail-fast then cancels the other jobs. Since the cron rewrites version_check_last_cron on every run, it should also be dynamic like the other cron timestamps (cron_lock, the *_last_gc ones), otherwise each run purges the config cache:

INSERT INTO phpbb_config (config_name, config_value) VALUES ('version_check_interval', '60');
INSERT INTO phpbb_config (config_name, config_value, is_dynamic) VALUES ('version_check_last_cron', '0', 1);

with the matching third argument in the migration:

['config.add', ['version_check_last_cron', 0, true]],

2. The generated item_id does not fit the column on any supported DBMS. In version_check::notify_admins():

'item_id' => (int) sprintf('%u', crc32($template . $update_data['current'])),

notifications.item_id is UINT, which is mediumint (max 16777215) on MySQL and INT4 (max 2147483647) on PostgreSQL. Nearly every crc32 value exceeds mediumint, so the insert fails with "Out of range value" under MySQL strict mode, and about half also exceed INT4 on PostgreSQL. The very next security notification on this branch would produce crc32('update_security' . '3.3.18') = 3943736323, which I verified fails the insert on both MySQL 8 and PostgreSQL 16. Masking the hash to 24 bits keeps it inside every mapping, including Oracle's number(8):

'item_id' => crc32($template . $update_data['current']) & 0xFFFFFF,

The hardcoded 1089886753 in test_run_with_updates becomes 16144929 with the mask. The tests do not catch this today because they only use small fixed ids.

3. The migration has no depends_on(), so the migrator is free to run it at any point in the tree:

public static function depends_on(): array
{
    return ['\phpbb\db\migration\data\v33x\v3317'];
}

4. Failed checks retry on every cron trigger. run() only sets version_check_last_cron on success, and get_update_on_branch(true) bypasses the versioncheck cache, so while the version server is unreachable every eligible cron trigger performs a blocking remote fetch. Setting the timestamp in the catch block as well avoids hammering both ends (test_run_with_exception would then assert greater than 0 instead of 0). There is also a leftover // @todo in that catch.

Minor: $db and $user are injected into the cron task but not used.

@marc1706
marc1706 requested a review from Derky August 17, 2026 18:30
Comment thread phpBB/install/schemas/schema_data.sql
Comment thread phpBB/language/en/email/update_urgent.txt Outdated
Comment thread phpBB/language/en/email/update_maintenance.txt Outdated
Comment thread phpBB/language/en/email/update_maintenance.txt
Comment thread phpBB/language/en/email/update_maintenance.txt Outdated
Comment thread phpBB/phpbb/notification/type/update_maintenance.php Outdated
Comment thread phpBB/phpbb/notification/type/update_maintenance.php
Comment thread tests/cron/version_check_test.php
Comment thread phpBB/install/schemas/schema_data.sql
Comment thread phpBB/language/en/email/update_maintenance.txt
Comment thread phpBB/language/en/common.php Outdated
Comment thread tests/cron/version_check_test.php
@Derky

Derky commented Sep 6, 2026

Copy link
Copy Markdown
Member

I'm having troubles testing this, current feedback

  • Editing theversion_check_interval doesn't seem to help. Tried it on one minute and deleted cache multiple times.
  • It only returns a notifcation, no mail by default
  • After enabling an email notification it still doesn't send out a mail
  • The content of the notification is not escaped, I can add HTML to the version that gets parsed

@marc1706

marc1706 commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

Updated the PR. Cron is working fine now and the default admin will receive the notification via mail by default. When running the migration, all admins with appropriate permissions should get the same setting. I also double checked and the version check data gets verified against the schema and no HTML can be added to inject HTML.

@marc1706
marc1706 requested a lite review from Copilot September 17, 2026 10:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Adds automatic cron-based phpBB update notifications for administrators, including maintenance, security, and critical update classifications.

Changes:

  • Adds version-checking cron task and version metadata for security/critical releases.
  • Registers an administrator notification type with default email subscriptions.
  • Adds migration, language/email templates, installation data, and automated tests.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/functional/update_maintenance_test.php Tests rendered update notification titles.
tests/cron/version_check_test.php Tests cron scheduling and update classification.
phpBB/phpbb/version_helper.php Extends the version response schema.
phpBB/phpbb/notification/type/update_maintenance.php Implements administrator update notifications.
phpBB/phpbb/db/migration/data/v33x/add_version_check_cron.php Adds cron configuration and default subscriptions.
phpBB/phpbb/cron/task/core/version_check.php Checks for updates and sends notifications.
phpBB/language/en/ucp.php Adds the notification type label.
phpBB/language/en/email/update_security.txt Adds the security update email template.
phpBB/language/en/email/update_maintenance.txt Adds the maintenance update email template.
phpBB/language/en/email/update_critical.txt Adds the critical update email template.
phpBB/language/en/common.php Adds update notification title strings.
phpBB/language/en/acp/common.php Adds the version-check failure log message.
phpBB/install/schemas/schema_data.sql Adds fresh-install cron configuration and admin email subscription data.
phpBB/config/default/container/services_notification.yml Registers the update notification service.
phpBB/config/default/container/services_cron.yml Registers the version-check cron task.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread phpBB/phpbb/cron/task/core/version_check.php
Comment thread phpBB/phpbb/db/migration/data/v33x/add_version_check_cron.php
Comment thread phpBB/phpbb/db/migration/data/v33x/add_version_check_cron.php
Comment thread phpBB/phpbb/db/migration/data/v33x/add_version_check_cron.php Outdated
@marc1706
marc1706 merged commit d8d7d0a into phpbb:3.3.x Sep 17, 2026
3 of 39 checks passed
@marc1706
marc1706 deleted the ticket/17665 branch September 17, 2026 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants