From d64b3558f164467e79a91d43babd50ae6c9ded99 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Fri, 13 Nov 2026 08:47:28 -0800 Subject: [PATCH 1/7] Fix push-click URLs for subfolder boards --- styles/all/template/push_worker.js.twig | 2 +- tests/controller/controller_webpush_test.php | 7 -- tests/javascript/push_worker_test.js | 90 ++++++++++++++++++++ tests/javascript/push_worker_test.php | 29 +++++++ ucp/controller/webpush.php | 1 - 5 files changed, 120 insertions(+), 9 deletions(-) create mode 100644 tests/javascript/push_worker_test.js create mode 100644 tests/javascript/push_worker_test.php diff --git a/styles/all/template/push_worker.js.twig b/styles/all/template/push_worker.js.twig index 3af01a9..542d9bb 100644 --- a/styles/all/template/push_worker.js.twig +++ b/styles/all/template/push_worker.js.twig @@ -88,7 +88,7 @@ self.addEventListener('notificationclick', event => { } try { - const url = new URL(event.notification.data.url, '{{ U_BOARD_URL|e("js") }}/'); + const url = new URL(event.notification.data.url, self.location.href); if (url.protocol === 'http:' || url.protocol === 'https:') { event.waitUntil(self.clients.openWindow(url.href)); } diff --git a/tests/controller/controller_webpush_test.php b/tests/controller/controller_webpush_test.php index ee36a3d..a11b554 100644 --- a/tests/controller/controller_webpush_test.php +++ b/tests/controller/controller_webpush_test.php @@ -327,17 +327,10 @@ public function test_get_user_notification_legacy() */ public function test_worker() { - $this->config['force_server_vars'] = true; - $this->config['server_protocol'] = 'https://'; - $this->config['server_name'] = 'example.com'; - $this->config['server_port'] = 443; - $this->config['script_path'] = '/forum'; - $this->config['cookie_secure'] = true; $this->template->expects(self::once()) ->method('render') ->with('@phpbb_webpushnotifications/push_worker.js.twig', [ 'U_WEBPUSH_GET_NOTIFICATION' => 'test_route', - 'U_BOARD_URL' => 'https://example.com/forum', 'ASSETS_VERSION' => '1.0', ]) ->willReturn('rendered_content'); diff --git a/tests/javascript/push_worker_test.js b/tests/javascript/push_worker_test.js new file mode 100644 index 0000000..3ada31e --- /dev/null +++ b/tests/javascript/push_worker_test.js @@ -0,0 +1,90 @@ +'use strict'; + +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const vm = require('node:vm'); + +const workerSource = fs.readFileSync(process.argv[2], 'utf8'); + +async function clickNotification(notificationUrl, workerUrl) { + const listeners = {}; + const openedUrls = []; + const self = { + location: new URL(workerUrl), + clients: { + claim() {}, + openWindow(url) { + openedUrls.push(url); + return Promise.resolve(null); + }, + }, + registration: { + showNotification() {}, + update() {}, + }, + skipWaiting() {}, + addEventListener(type, listener) { + listeners[type] = listener; + }, + }; + + vm.runInNewContext(workerSource, {console, self, URL}, {filename: process.argv[2]}); + + let pending; + listeners.notificationclick({ + notification: { + data: {url: notificationUrl}, + close() {}, + }, + waitUntil(promise) { + pending = promise; + }, + }); + + if (pending) { + await pending; + } + + return openedUrls; +} + +async function run() { + const subfolderWorker = 'http://localhost:8090/forum/app.php/user/push/worker'; + const notificationUrl = './../../../ucp.php?i=pm&mode=view&p=4'; + + assert.deepEqual( + await clickNotification(notificationUrl, subfolderWorker), + ['http://localhost:8090/forum/ucp.php?i=pm&mode=view&p=4'], + 'relative notification URLs must resolve from worker route on subfolder boards', + ); + assert.deepEqual( + await clickNotification(notificationUrl, 'http://localhost:8090/app.php/user/push/worker'), + ['http://localhost:8090/ucp.php?i=pm&mode=view&p=4'], + 'relative notification URLs must still work on root boards', + ); + assert.deepEqual( + await clickNotification('https://example.com/topic/1', subfolderWorker), + ['https://example.com/topic/1'], + 'absolute HTTPS notification URLs must remain unchanged', + ); + assert.deepEqual( + await clickNotification('http://example.com/topic/1', subfolderWorker), + ['http://example.com/topic/1'], + 'absolute HTTP notification URLs must remain unchanged', + ); + assert.deepEqual( + await clickNotification('javascript:alert(1)', subfolderWorker), + [], + 'non-HTTP notification URLs must be rejected', + ); + assert.deepEqual( + await clickNotification('http://[invalid', subfolderWorker), + [], + 'malformed notification URLs must be ignored', + ); +} + +run().catch(error => { + console.error(error); + process.exitCode = 1; +}); diff --git a/tests/javascript/push_worker_test.php b/tests/javascript/push_worker_test.php new file mode 100644 index 0000000..1678eb3 --- /dev/null +++ b/tests/javascript/push_worker_test.php @@ -0,0 +1,29 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + */ + +namespace phpbb\webpushnotifications\tests\javascript; + +use Symfony\Component\Process\Process; + +class push_worker_test extends \phpbb_test_case +{ + public function test_notification_click_url_resolution(): void + { + $process = new Process([ + 'node', + __DIR__ . '/push_worker_test.js', + __DIR__ . '/../../styles/all/template/push_worker.js.twig', + ]); + $process->setTimeout(10); + $process->run(); + + self::assertSame(0, $process->getExitCode(), $process->getErrorOutput() . $process->getOutput()); + } +} diff --git a/ucp/controller/webpush.php b/ucp/controller/webpush.php index ecae851..61a11a1 100644 --- a/ucp/controller/webpush.php +++ b/ucp/controller/webpush.php @@ -280,7 +280,6 @@ public function worker(): Response { $content = $this->template->render('@phpbb_webpushnotifications/push_worker.js.twig', [ 'U_WEBPUSH_GET_NOTIFICATION' => $this->controller_helper->route('phpbb_webpushnotifications_ucp_push_get_notification_controller'), - 'U_BOARD_URL' => generate_board_url(), 'ASSETS_VERSION' => $this->config['assets_version'], ]); From bf1afb6b000a6326ab5ca21a464ebe6707038807 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Fri, 13 Nov 2026 09:14:22 -0800 Subject: [PATCH 2/7] Prevent Malformed Endpoint Batch Denial --- notification/method/webpush.php | 18 +++++++- subscription/validator.php | 19 ++++++++ tests/controller/controller_webpush_test.php | 3 ++ .../notification_method_webpush_test.php | 45 ++++++++++++++++++- tests/subscription/validator_test.php | 23 ++++++++++ ucp/controller/webpush.php | 10 ++--- 6 files changed, 108 insertions(+), 10 deletions(-) diff --git a/notification/method/webpush.php b/notification/method/webpush.php index e99b535..89bfc3b 100644 --- a/notification/method/webpush.php +++ b/notification/method/webpush.php @@ -211,15 +211,17 @@ protected function notify_using_webpush(): void $user_subscription_map = $this->get_user_subscription_map($notify_users); $invalid_subscriptions = []; - // Validate each stored subscription once so malformed keys cannot interrupt the delivery batch. + // Validate each stored subscription once so malformed data cannot interrupt the delivery batch. foreach ($user_subscription_map as $user_id => $subscriptions) { $valid_subscriptions = []; $user = $this->user_loader->get_user($user_id); foreach ($subscriptions as $subscription) { - if (!is_string($subscription['p256dh'] ?? null) + if (!is_string($subscription['endpoint'] ?? null) + || !is_string($subscription['p256dh'] ?? null) || !is_string($subscription['auth'] ?? null) + || !$this->is_valid_endpoint_url($subscription['endpoint']) || !subscription_validator::is_valid($subscription['p256dh'], $subscription['auth'])) { $invalid_subscriptions[$subscription['subscription_id']] = $user; @@ -449,6 +451,18 @@ protected function get_user_subscription_map(array $notify_users): array return $user_subscription_map; } + /** + * Check whether a stored subscription endpoint is valid for delivery + * Wrapper used here exists only as test seam + * + * @param string $endpoint Push subscription endpoint URL + * @return bool + */ + protected function is_valid_endpoint_url(string $endpoint): bool + { + return subscription_validator::is_valid_endpoint_url($endpoint); + } + /** * Remove subscriptions * diff --git a/subscription/validator.php b/subscription/validator.php index 74f62fd..db5ce53 100644 --- a/subscription/validator.php +++ b/subscription/validator.php @@ -20,6 +20,25 @@ */ class validator { + /** + * Check that an endpoint is a plain HTTPS URL with a well-formed hostname + * + * @param string $endpoint Push subscription endpoint URL + * @return bool + */ + public static function is_valid_endpoint_url(string $endpoint): bool + { + $parts = parse_url($endpoint); + + return is_array($parts) + && strtolower($parts['scheme'] ?? '') === 'https' + && !isset($parts['user']) + && !isset($parts['pass']) + && (!isset($parts['port']) || $parts['port'] === 443) + && is_string($parts['host'] ?? null) + && filter_var($parts['host'], FILTER_VALIDATE_DOMAIN, FILTER_FLAG_HOSTNAME) !== false; + } + /** * Validate Web Push subscription keys * diff --git a/tests/controller/controller_webpush_test.php b/tests/controller/controller_webpush_test.php index a11b554..7d3dc72 100644 --- a/tests/controller/controller_webpush_test.php +++ b/tests/controller/controller_webpush_test.php @@ -579,6 +579,7 @@ public function data_subscribe_invalid_endpoint(): array 'no_host' => ['not_a_url'], 'disallowed_host' => ['https://evil.example.com/push/endpoint'], 'disallowed_subdomain' => ['https://evil.fcm.googleapis.com.attacker.com/push'], + 'malformed_allowed_suffix' => ['https://evil.example.com\\.push.apple.com/x'], 'empty_string' => [''], ]; } @@ -671,6 +672,8 @@ public function data_is_valid_endpoint(): array ['https://evil.example.com/push/endpoint', false], // Invalid: subdomain spoofing ['https://evil.fcm.googleapis.com.attacker.com/push', false], + // Invalid: malformed host with an allowed suffix + ['https://evil.example.com\\.push.apple.com/x', false], // Invalid: not a URL ['not_a_url', false], // Invalid: bare wildcard domain without subdomain diff --git a/tests/notification/notification_method_webpush_test.php b/tests/notification/notification_method_webpush_test.php index 155d70b..902384a 100644 --- a/tests/notification/notification_method_webpush_test.php +++ b/tests/notification/notification_method_webpush_test.php @@ -151,7 +151,7 @@ protected function setUp(): void $phpbb_container->setParameter('tables.phpbb.wpn.notification_push', 'phpbb_wpn_notification_push'); $phpbb_container->setParameter('tables.phpbb.wpn.push_subscriptions', 'phpbb_wpn_push_subscriptions'); - $this->notification_method_webpush = new webpush( + $this->notification_method_webpush = new integration_testable_webpush( $phpbb_container->get('config'), $phpbb_container->get('dbal.conn'), $phpbb_container->get('log'), @@ -787,6 +787,40 @@ public function test_invalid_subscription_does_not_interrupt_valid_delivery(): v $this->db->sql_freeresult($result); } + public function test_malformed_endpoint_does_not_interrupt_other_users_delivery(): void + { + $malformed_subscription = $this->create_subscription_for_user(2); + $valid_subscription = $this->create_subscription_for_user(3); + $push_subscriptions_table = $this->container->getParameter('tables.phpbb.wpn.push_subscriptions'); + $malformed_endpoint = 'https://evil.example.com\\.push.apple.com/x'; + + $sql = 'UPDATE ' . $push_subscriptions_table . " + SET endpoint = '" . $this->db->sql_escape($malformed_endpoint) . "' + WHERE endpoint = '" . $this->db->sql_escape($malformed_subscription['endpoint']) . "'"; + $this->db->sql_query($sql); + + $this->notifications->add_notifications('notification.type.post', [ + 'post_time' => 1349413322, + 'poster_id' => 1, + 'topic_title' => '', + 'post_subject' => '', + 'post_username' => '', + 'forum_name' => '', + 'forum_id' => '1', + 'post_id' => '2', + 'topic_id' => '1', + ]); + + $this->assertNotEmpty($this->get_messages_for_subscription($valid_subscription['clientHash'])); + + $sql = 'SELECT COUNT(*) AS subscription_count + FROM ' . $push_subscriptions_table . " + WHERE endpoint = '" . $this->db->sql_escape($malformed_endpoint) . "'"; + $result = $this->db->sql_query($sql); + $this->assertSame(0, (int) $this->db->sql_fetchfield('subscription_count')); + $this->db->sql_freeresult($result); + } + public function test_get_ucp_template_data_uses_millisecond_expiration_time(): void { $this->user->data['user_id'] = 2; @@ -1332,3 +1366,12 @@ public function test_export_data_with_migration(): void ); } } + +class integration_testable_webpush extends webpush +{ + protected function is_valid_endpoint_url(string $endpoint): bool + { + return strpos($endpoint, 'http://localhost:9012/notify/') === 0 + || parent::is_valid_endpoint_url($endpoint); + } +} diff --git a/tests/subscription/validator_test.php b/tests/subscription/validator_test.php index 65f798a..b3ab13d 100644 --- a/tests/subscription/validator_test.php +++ b/tests/subscription/validator_test.php @@ -56,4 +56,27 @@ public function test_invalid_keys(string $public_key, string $auth_token): void { self::assertFalse(validator::is_valid($public_key, $auth_token)); } + + public function endpoint_url_data(): array + { + return [ + 'allowed exact host' => ['https://fcm.googleapis.com/fcm/send/test', true], + 'allowed wildcard host' => ['https://api.push.apple.com/3/device/test', true], + 'insecure scheme' => ['http://api.push.apple.com/3/device/test', false], + 'nonstandard port' => ['https://api.push.apple.com:8443/3/device/test', false], + 'username' => ['https://user@api.push.apple.com/3/device/test', false], + 'password' => ['https://user:pass@api.push.apple.com/3/device/test', false], + 'malformed wildcard host' => ['https://evil.example.com\\.push.apple.com/x', false], + 'percent-encoded wildcard host' => ['https://evil%2epush.apple.com/x', false], + 'invalid hostname label' => ['https://-evil.push.apple.com/x', false], + ]; + } + + /** + * @dataProvider endpoint_url_data + */ + public function test_valid_endpoint_url(string $endpoint, bool $expected): void + { + self::assertSame($expected, validator::is_valid_endpoint_url($endpoint)); + } } diff --git a/ucp/controller/webpush.php b/ucp/controller/webpush.php index 61a11a1..7ecfd18 100644 --- a/ucp/controller/webpush.php +++ b/ucp/controller/webpush.php @@ -41,6 +41,7 @@ class webpush public const PUSH_SERVICE_WHITELIST = [ 'android.googleapis.com', 'fcm.googleapis.com', + 'jmt17.google.com', 'updates.push.services.mozilla.com', 'updates-autopush.stage.mozaws.net', 'updates-autopush.dev.mozaws.net', @@ -305,17 +306,12 @@ public function worker(): Response */ public function is_valid_endpoint(string $endpoint): bool { - $parts = parse_url($endpoint); - - if (!is_array($parts) - || strtolower($parts['scheme'] ?? '') !== 'https' - || (isset($parts['port']) && $parts['port'] !== 443) - || !is_string($parts['host'] ?? null) - || empty($parts['host'])) + if (!subscription_validator::is_valid_endpoint_url($endpoint)) { return false; } + $parts = parse_url($endpoint); $host = strtolower($parts['host']); if (in_array($host, self::PUSH_SERVICE_WHITELIST, true)) From d631615f0a6f31021d9ff0635a3fa20c984fefc6 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Fri, 13 Nov 2026 09:30:48 -0800 Subject: [PATCH 3/7] Harden VAPID Failure Handling and MCP Log Localisation --- controller/acp_controller.php | 10 +++++ language/en/info_mcp_webpushnotifications.php | 23 +++++++++++ .../en/webpushnotifications_module_acp.php | 1 + language/ru/info_mcp_webpushnotifications.php | 23 +++++++++++ .../ru/webpushnotifications_module_acp.php | 1 + notification/method/webpush.php | 12 +++++- subscription/validator.php | 26 ++++++++++++ tests/controller/controller_acp_test.php | 40 ++++++++++++++----- tests/notification/webpush_branches_test.php | 25 ++++++++++++ tests/subscription/validator_test.php | 12 ++++++ tests/system/simple_test.php | 19 +++++++++ 11 files changed, 180 insertions(+), 12 deletions(-) create mode 100644 language/en/info_mcp_webpushnotifications.php create mode 100644 language/ru/info_mcp_webpushnotifications.php diff --git a/controller/acp_controller.php b/controller/acp_controller.php index 8fceae6..1f611d3 100644 --- a/controller/acp_controller.php +++ b/controller/acp_controller.php @@ -21,6 +21,7 @@ use phpbb\template\template; use phpbb\user; use phpbb\webpushnotifications\ext; +use phpbb\webpushnotifications\subscription\validator as subscription_validator; class acp_controller { @@ -223,6 +224,15 @@ protected function save_settings() { // Validate config values validate_config_vars($display_settings, $config_array, $this->errors); + + if (empty($this->errors) + && !subscription_validator::is_valid_vapid_keys( + $config_array['wpn_webpush_vapid_public'] ?? '', + $config_array['wpn_webpush_vapid_private'] ?? $this->config['wpn_webpush_vapid_private'] + )) + { + $this->errors[] = $this->lang->lang('WEBPUSH_VAPID_KEYS_INVALID'); + } } if ($this->display_errors()) diff --git a/language/en/info_mcp_webpushnotifications.php b/language/en/info_mcp_webpushnotifications.php new file mode 100644 index 0000000..c2c474c --- /dev/null +++ b/language/en/info_mcp_webpushnotifications.php @@ -0,0 +1,23 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + */ + +if (!defined('IN_PHPBB')) +{ + exit; +} + +if (empty($lang) || !is_array($lang)) +{ + $lang = []; +} + +$lang = array_merge($lang, [ + 'LOG_WEBPUSH_SUBSCRIPTION_REMOVED' => 'Removed Web Push subscription:
» %s', +]); diff --git a/language/en/webpushnotifications_module_acp.php b/language/en/webpushnotifications_module_acp.php index 18c732b..7ef3224 100644 --- a/language/en/webpushnotifications_module_acp.php +++ b/language/en/webpushnotifications_module_acp.php @@ -53,6 +53,7 @@ 'WEBPUSH_ROTATE_VAPID_KEYS_CONFIRM'=> 'Saving new keys will invalidate all existing Web Push subscriptions. Users will not receive Web Push notifications until their next visit to the board, when their subscriptions will be recreated.', 'WEBPUSH_VAPID_KEYS_GENERATED'=> 'New key pair generated. Submit this form to activate it.', 'WEBPUSH_VAPID_KEYS_GENERATION_FAILED'=> 'Could not generate a key pair in this browser. Enter a valid pair manually or try a supported browser.', + 'WEBPUSH_VAPID_KEYS_INVALID' => 'The VAPID identification keys are invalid.', 'WEBPUSH_VAPID_ROTATION_WARNING'=> 'Important: Changing either key invalidates all existing Web Push subscriptions.', 'WEBPUSH_VAPID_PUBLIC' => 'Server identification public key', 'WEBPUSH_VAPID_PUBLIC_EXPLAIN' => 'Shared with push services to authenticate messages sent from your board.', diff --git a/language/ru/info_mcp_webpushnotifications.php b/language/ru/info_mcp_webpushnotifications.php new file mode 100644 index 0000000..b6e6cfe --- /dev/null +++ b/language/ru/info_mcp_webpushnotifications.php @@ -0,0 +1,23 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + */ + +if (!defined('IN_PHPBB')) +{ + exit; +} + +if (empty($lang) || !is_array($lang)) +{ + $lang = []; +} + +$lang = array_merge($lang, [ + 'LOG_WEBPUSH_SUBSCRIPTION_REMOVED' => 'Удалена подписка на браузерные push—уведомления:
» %s', +]); diff --git a/language/ru/webpushnotifications_module_acp.php b/language/ru/webpushnotifications_module_acp.php index 734c8f5..a6f911f 100644 --- a/language/ru/webpushnotifications_module_acp.php +++ b/language/ru/webpushnotifications_module_acp.php @@ -53,6 +53,7 @@ 'WEBPUSH_ROTATE_VAPID_KEYS_CONFIRM'=> 'Сохранение новых ключей сделает недействительными все существующие подписки на браузерные push—уведомления. Пользователи не будут получать браузерные push—уведомления до следующего посещения конференции, при котором их подписки будут пересозданы.', 'WEBPUSH_VAPID_KEYS_GENERATED'=> 'Новая пара ключей сгенерирована. Отправьте эту форму, чтобы активировать её.', 'WEBPUSH_VAPID_KEYS_GENERATION_FAILED'=> 'Не удалось сгенерировать пару ключей в этом браузере. Введите корректную пару вручную или используйте поддерживаемый браузер.', + 'WEBPUSH_VAPID_KEYS_INVALID' => 'Ключи идентификации VAPID недействительны.', 'WEBPUSH_VAPID_ROTATION_WARNING'=> 'Важно: изменение любого ключа делает недействительными все существующие подписки на браузерные push—уведомления.', 'WEBPUSH_VAPID_PUBLIC' => 'Публичный ключ идентификации сервера', 'WEBPUSH_VAPID_PUBLIC_EXPLAIN' => 'Передаётся службам push—уведомлений для аутентификации сообщений, отправляемых конференцией.', diff --git a/notification/method/webpush.php b/notification/method/webpush.php index 89bfc3b..c0acee9 100644 --- a/notification/method/webpush.php +++ b/notification/method/webpush.php @@ -241,7 +241,17 @@ protected function notify_using_webpush(): void ], ]; - $web_push = new \Minishlink\WebPush\WebPush($auth); + try + { + $web_push = new \Minishlink\WebPush\WebPush($auth); + } + catch (\Throwable $exception) + { + $this->log->add('critical', ANONYMOUS, '', 'LOG_WEBPUSH_MESSAGE_FAIL', false, [$exception->getMessage()]); + $this->empty_queue(); + $this->remove_invalid_subscriptions($invalid_subscriptions); + return; + } $number_of_notifications = 0; diff --git a/subscription/validator.php b/subscription/validator.php index db5ce53..ffee18b 100644 --- a/subscription/validator.php +++ b/subscription/validator.php @@ -14,12 +14,38 @@ use Brick\Math\BigInteger; use Jose\Component\Core\Util\Ecc\NistCurve; use Minishlink\WebPush\Utils; +use Minishlink\WebPush\VAPID; /** * Web Push subscription key validator */ class validator { + /** + * Validate VAPID identification keys + * + * @param string $public_key VAPID public key + * @param string $private_key VAPID private key + * @return bool + */ + public static function is_valid_vapid_keys(string $public_key, string $private_key): bool + { + try + { + VAPID::validate([ + 'subject' => 'https://example.com', + 'publicKey' => $public_key, + 'privateKey' => $private_key, + ]); + + return true; + } + catch (\Throwable $exception) + { + return false; + } + } + /** * Check that an endpoint is a plain HTTPS URL with a well-formed hostname * diff --git a/tests/controller/controller_acp_test.php b/tests/controller/controller_acp_test.php index b8c478c..85f990d 100644 --- a/tests/controller/controller_acp_test.php +++ b/tests/controller/controller_acp_test.php @@ -16,6 +16,9 @@ class controller_acp_test extends \phpbb_test_case { + private const VALID_VAPID_PUBLIC_KEY = 'BIcGkq1Ncj3a2-J0UW-1A0NETLjvxZzNLiYBiPVMKNjgwmwPi5jyK87VfS4FZn9n7S9pLMQzjV3LmFuOnRSOvmI'; + private const VALID_VAPID_PRIVATE_KEY = 'SrlbBEVgibWmKHYbDPu4Y2XvDWPjeGcc9fC16jq01xU'; + /** @var bool */ public static $valid_form = true; @@ -213,8 +216,8 @@ public function webpush_save_data(): array { $valid = [ 'wpn_webpush_enable' => 1, - 'wpn_webpush_vapid_public' => str_repeat('a', 25), - 'wpn_webpush_vapid_private' => str_repeat('b', 25), + 'wpn_webpush_vapid_public' => self::VALID_VAPID_PUBLIC_KEY, + 'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY, 'wpn_webpush_dropdown_subscribe' => 1, 'wpn_webpush_method_enabled' => 0, 'wpn_webpush_popup_prompt' => 1, @@ -228,8 +231,8 @@ public function webpush_save_data(): array 'ERROR_MSG' => '', ], [ - 'wpn_webpush_vapid_public' => str_repeat('a', 25), - 'wpn_webpush_vapid_private' => str_repeat('b', 25), + 'wpn_webpush_vapid_public' => self::VALID_VAPID_PUBLIC_KEY, + 'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY, 'wpn_webpush_method_enabled' => 0, ], true, @@ -243,8 +246,8 @@ public function webpush_save_data(): array 'ERROR_MSG' => '', ], [ - 'wpn_webpush_vapid_public' => str_repeat('a', 25), - 'wpn_webpush_vapid_private' => 'existing-private-key', + 'wpn_webpush_vapid_public' => self::VALID_VAPID_PUBLIC_KEY, + 'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY, ], true, ], @@ -258,8 +261,23 @@ public function webpush_save_data(): array 'ERROR_MSG' => 'SETTING_TOO_SHORT
SETTING_TOO_SHORT', ], [ - 'wpn_webpush_vapid_public' => 'existing-public-key', - 'wpn_webpush_vapid_private' => 'existing-private-key', + 'wpn_webpush_vapid_public' => self::VALID_VAPID_PUBLIC_KEY, + 'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY, + ], + false, + ], + 'invalid VAPID key encoding is rejected' => [ + array_merge($valid, [ + 'wpn_webpush_vapid_public' => str_repeat('A', 30), + 'wpn_webpush_vapid_private' => str_repeat('B', 30), + ]), + [ + 'S_ERROR' => true, + 'ERROR_MSG' => 'WEBPUSH_VAPID_KEYS_INVALID', + ], + [ + 'wpn_webpush_vapid_public' => self::VALID_VAPID_PUBLIC_KEY, + 'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY, ], false, ], @@ -276,7 +294,7 @@ public function webpush_save_data(): array [ 'wpn_webpush_enable' => 0, 'wpn_webpush_vapid_public' => '', - 'wpn_webpush_vapid_private' => 'existing-private-key', + 'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY, ], true, ], @@ -288,8 +306,8 @@ public function webpush_save_data(): array */ public function test_save_settings_validates_and_persists_webpush_config(array $input, array $error_vars, array $expected_config, $expect_saved): void { - $this->config['wpn_webpush_vapid_public'] = 'existing-public-key'; - $this->config['wpn_webpush_vapid_private'] = 'existing-private-key'; + $this->config['wpn_webpush_vapid_public'] = self::VALID_VAPID_PUBLIC_KEY; + $this->config['wpn_webpush_vapid_private'] = self::VALID_VAPID_PRIVATE_KEY; $this->request->method('variable') ->with('config', ['' => ''], true) diff --git a/tests/notification/webpush_branches_test.php b/tests/notification/webpush_branches_test.php index 034e2f4..372ab70 100644 --- a/tests/notification/webpush_branches_test.php +++ b/tests/notification/webpush_branches_test.php @@ -110,6 +110,31 @@ public function test_oversized_payload_logs_failure_without_removing_subscriptio self::assertSame([], $method->removed_subscriptions); } + public function test_invalid_vapid_config_logs_failure_without_throwing(): void + { + global $config; + + $GLOBALS['phpbb_wpn_test_banned_users'] = []; + $log = $this->createMock(\phpbb\log\log_interface::class); + $log->expects(self::once())->method('add')->with( + 'critical', + ANONYMOUS, + '', + 'LOG_WEBPUSH_MESSAGE_FAIL', + false, + self::callback(static function(array $arguments): bool { + return isset($arguments[0]) && strpos($arguments[0], 'Public key should be 65 bytes long') !== false; + }) + ); + $method = $this->create_method($log); + $config['wpn_webpush_vapid_public'] = str_repeat('A', 30); + $method->add_to_queue($this->create_notification(2)); + + $method->send_queued_notifications(); + + self::assertSame([], $method->get_queue()); + } + private function create_method($log = null, $user_loader = null, $assets_version = 1): testable_webpush { global $config, $phpbb_root_path, $phpEx, $user; diff --git a/tests/subscription/validator_test.php b/tests/subscription/validator_test.php index b3ab13d..2b8ede2 100644 --- a/tests/subscription/validator_test.php +++ b/tests/subscription/validator_test.php @@ -16,8 +16,20 @@ class validator_test extends \phpbb_test_case { + private const VALID_VAPID_PRIVATE_KEY = 'SrlbBEVgibWmKHYbDPu4Y2XvDWPjeGcc9fC16jq01xU'; private const VALID_PUBLIC_KEY = 'BGsX0fLhLEJH-Lzm5WOkQPJ3A32BLeszoPShOUXYmMKWT-NC4v4af5uO5-tKfA-eFivOM1drMV7Oy7ZAaDe_UfU'; private const VALID_AUTH_TOKEN = 'AAAAAAAAAAAAAAAAAAAAAA'; + private const VALID_VAPID_PUBLIC_KEY = 'BIcGkq1Ncj3a2-J0UW-1A0NETLjvxZzNLiYBiPVMKNjgwmwPi5jyK87VfS4FZn9n7S9pLMQzjV3LmFuOnRSOvmI'; + + public function test_valid_vapid_keys(): void + { + self::assertTrue(validator::is_valid_vapid_keys(self::VALID_VAPID_PUBLIC_KEY, self::VALID_VAPID_PRIVATE_KEY)); + } + + public function test_invalid_vapid_keys(): void + { + self::assertFalse(validator::is_valid_vapid_keys(str_repeat('A', 30), str_repeat('B', 30))); + } public function valid_key_data(): array { diff --git a/tests/system/simple_test.php b/tests/system/simple_test.php index 2baf7ea..9784731 100644 --- a/tests/system/simple_test.php +++ b/tests/system/simple_test.php @@ -70,6 +70,25 @@ public function test_decode_entities() { self::assertSame('A&B "quote"', \phpbb\webpushnotifications\ext::decode_entities('A&B "quote"', ENT_QUOTES)); } + + public function mcp_log_language_data(): array + { + return [ + 'English' => ['en', 'Removed Web Push subscription:
» %s'], + 'Russian' => ['ru', 'Удалена подписка на браузерные push—уведомления:
» %s'], + ]; + } + + /** + * @dataProvider mcp_log_language_data + */ + public function test_mcp_log_language(string $language, string $expected): void + { + $lang = []; + include __DIR__ . '/../../language/' . $language . '/info_mcp_webpushnotifications.php'; + + self::assertSame($expected, $lang['LOG_WEBPUSH_SUBSCRIPTION_REMOVED']); + } } class testable_ext extends \phpbb\webpushnotifications\ext From 49166001b10d384d4c91310a3e80e8e6b74e8841 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Fri, 13 Nov 2026 09:36:38 -0800 Subject: [PATCH 4/7] Preserve User Text in Push Notifications --- tests/controller/controller_webpush_test.php | 8 ++++---- ucp/controller/webpush.php | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/controller/controller_webpush_test.php b/tests/controller/controller_webpush_test.php index 7d3dc72..ad6894c 100644 --- a/tests/controller/controller_webpush_test.php +++ b/tests/controller/controller_webpush_test.php @@ -696,8 +696,8 @@ public function test_current_notification_data_is_rendered(): void { $notification = $this->createMock(\phpbb\notification\type\type_interface::class); $notification->method('users_to_query')->willReturn([]); - $notification->method('get_title')->willReturn('A & title'); - $notification->method('get_reference')->willReturn('A "reference"'); + $notification->method('get_title')->willReturn('Private Message from Alice & Bob:'); + $notification->method('get_reference')->willReturn('"She said "hello" & asked about a < b in <div> tags"'); $notification->method('get_url')->willReturn('viewtopic.php?p=1&x=2'); $notification->method('get_avatar')->willReturn(''); $this->notification_manager->expects(self::once()) @@ -712,8 +712,8 @@ public function test_current_notification_data_is_rendered(): void ])), true); self::assertSame('yourdomain.com', $data['heading']); - self::assertSame('A & title', $data['title']); - self::assertSame('A "reference"', $data['text']); + self::assertSame('Private Message from Alice & Bob:', $data['title']); + self::assertSame('"She said "hello" & asked about a < b in
tags"', $data['text']); self::assertSame('viewtopic.php?p=1&x=2', $data['url']); self::assertStringEndsWith('/images/avatar.png', $data['avatar']['src']); } diff --git a/ucp/controller/webpush.php b/ucp/controller/webpush.php index 7ecfd18..3c96aa5 100644 --- a/ucp/controller/webpush.php +++ b/ucp/controller/webpush.php @@ -262,8 +262,8 @@ private function get_notification_data(string $notification_data): string return json_encode([ 'heading' => ext::decode_entities($this->config['sitename'], ENT_QUOTES), - 'title' => strip_tags(ext::decode_entities($notification->get_title())), - 'text' => strip_tags(ext::decode_entities($notification->get_reference())), + 'title' => ext::decode_entities(strip_tags($notification->get_title()), ENT_QUOTES), + 'text' => ext::decode_entities(strip_tags($notification->get_reference()), ENT_QUOTES), 'url' => htmlspecialchars_decode($notification->get_url()), 'avatar' => $this->prepare_avatar($notification->get_avatar()), ]); From 311572c4c258f7f4d5c60a048e0cf9c4e67d69b4 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Fri, 13 Nov 2026 09:58:16 -0800 Subject: [PATCH 5/7] Prevent Cross-Account Automatic Push Reassignment --- styles/all/template/webpush.js | 5 + tests/javascript/webpush_test.js | 227 ++++++++++++++++++++++++++++++ tests/javascript/webpush_test.php | 29 ++++ 3 files changed, 261 insertions(+) create mode 100644 tests/javascript/webpush_test.js create mode 100644 tests/javascript/webpush_test.php diff --git a/styles/all/template/webpush.js b/styles/all/template/webpush.js index 9a4a185..6b5f32f 100644 --- a/styles/all/template/webpush.js +++ b/styles/all/template/webpush.js @@ -140,6 +140,11 @@ function PhpbbWebpush() { return; } + // A subscription this account does not own was made by another member on this browser. + if (!subscriptions.some(stored => stored.endpoint === subscription.endpoint)) { + return; + } + if (shouldRefreshSubscription(subscription)) { await refreshSubscription(registration, subscription); return; diff --git a/tests/javascript/webpush_test.js b/tests/javascript/webpush_test.js new file mode 100644 index 0000000..378853d --- /dev/null +++ b/tests/javascript/webpush_test.js @@ -0,0 +1,227 @@ +'use strict'; + +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const vm = require('node:vm'); + +const webpushSource = fs.readFileSync(process.argv[2], 'utf8'); +const endpoint = 'https://push.example.test/subscription/old'; +const currentVapidKey = 'AQID'; + +function createButton(id) { + const listeners = {}; + return { + id, + disabled: false, + hidden: false, + type: 'button', + classList: { + contains() { + return false; + }, + toggle(name, state) { + if (name === 'hidden') { + this.owner.hidden = state; + } + }, + }, + addEventListener(type, listener) { + listeners[type] = listener; + }, + removeEventListener(type) { + delete listeners[type]; + }, + getAttribute() { + return 'Web Push error'; + }, + listeners, + }; +} + +async function settle() { + await new Promise(resolve => setImmediate(resolve)); + await new Promise(resolve => setImmediate(resolve)); +} + +async function runScenario({stored = [], expirationTime = 0, applicationServerKey = [1, 2, 3], click = false}) { + const calls = {fetch: 0, subscribe: 0, unsubscribe: 0}; + const subscribeButton = createButton('subscribe_webpush'); + const unsubscribeButton = createButton('unsubscribe_webpush'); + subscribeButton.classList.owner = subscribeButton; + unsubscribeButton.classList.owner = unsubscribeButton; + + const popup = { + style: {display: 'none'}, + addEventListener() {}, + }; + const storage = new Map(); + let activeSubscription; + + function makeSubscription(subscriptionEndpoint, key, expires = 0) { + return { + endpoint: subscriptionEndpoint, + expirationTime: expires, + options: {applicationServerKey: Uint8Array.from(key).buffer}, + toJSON() { + return { + endpoint: this.endpoint, + expirationTime: this.expirationTime, + keys: {p256dh: 'public-key', auth: 'auth-token'}, + }; + }, + async unsubscribe() { + calls.unsubscribe++; + if (activeSubscription === this) { + activeSubscription = null; + } + return true; + }, + }; + } + + activeSubscription = makeSubscription(endpoint, applicationServerKey, expirationTime); + const registration = { + pushManager: { + async getSubscription() { + return activeSubscription; + }, + async subscribe() { + calls.subscribe++; + activeSubscription = makeSubscription('https://push.example.test/subscription/new', [1, 2, 3]); + return activeSubscription; + }, + }, + }; + + const document = { + readyState: 'complete', + querySelectorAll(selector) { + return selector.includes('unsubscribe') ? [unsubscribeButton] : [subscribeButton]; + }, + querySelector() { + return null; + }, + getElementById(id) { + return id === 'wpn_popup_prompt' ? popup : null; + }, + addEventListener() {}, + removeEventListener() {}, + }; + const window = { + isSecureContext: true, + PushManager: function PushManager() {}, + atob(value) { + return Buffer.from(value, 'base64').toString('binary'); + }, + btoa(value) { + return Buffer.from(value, 'binary').toString('base64'); + }, + }; + const phpbb = { + alert() {}, + alertTime: 0, + loadingIndicator() { + return {fadeOut() {}}; + }, + }; + const context = { + console, + document, + fetch: async() => { + calls.fetch++; + return { + async json() { + return {success: true, form_tokens: {creation_time: 2, form_token: 'new-token'}}; + }, + }; + }, + FormData: class FormData { + append() {} + }, + localStorage: { + getItem(key) { + return storage.get(key) || null; + }, + setItem(key, value) { + storage.set(key, value); + }, + removeItem(key) { + storage.delete(key); + }, + }, + navigator: { + serviceWorker: { + async register() { + return registration; + }, + async getRegistration() { + return registration; + }, + }, + }, + Notification: { + permission: 'granted', + async requestPermission() { + return 'granted'; + }, + }, + phpbb, + phpbbWebpushOptions: { + serviceWorkerUrl: '/push-worker', + subscribeUrl: '/subscribe', + unsubscribeUrl: '/unsubscribe', + togglePopupUrl: '/toggle-popup', + ajaxErrorTitle: 'Error', + unsubscribeError: 'Unsubscribe error', + vapidPublicKey: currentVapidKey, + formTokens: {creationTime: 1, formToken: 'token'}, + subscriptions: stored.map(storedEndpoint => ({endpoint: storedEndpoint, expirationTime: 0})), + }, + setTimeout(callback) { + callback(); + }, + Uint8Array, + window, + }; + + vm.runInNewContext(webpushSource, context, {filename: process.argv[2]}); + await settle(); + + if (click) { + await subscribeButton.listeners.click({ + currentTarget: subscribeButton, + preventDefault() {}, + stopPropagation() {}, + }); + await settle(); + } + + return {calls, popup, subscribeButton, unsubscribeButton}; +} + +async function run() { + const foreign = await runScenario({stored: []}); + assert.deepEqual(foreign.calls, {fetch: 0, subscribe: 0, unsubscribe: 0}); + assert.equal(foreign.subscribeButton.hidden, false, 'foreign subscription must leave subscribe control visible'); + assert.equal(foreign.unsubscribeButton.hidden, true, 'foreign subscription must not appear owned'); + assert.equal(foreign.popup.style.display, 'flex', 'foreign subscription must use normal opt-in prompt'); + + const owned = await runScenario({stored: [endpoint]}); + assert.deepEqual(owned.calls, {fetch: 0, subscribe: 0, unsubscribe: 0}); + assert.equal(owned.subscribeButton.hidden, true, 'owned valid subscription must appear subscribed'); + assert.equal(owned.unsubscribeButton.hidden, false, 'owned valid subscription must allow unsubscribe'); + + const staleKey = await runScenario({stored: [endpoint], applicationServerKey: [4, 5, 6]}); + assert.deepEqual(staleKey.calls, {fetch: 1, subscribe: 1, unsubscribe: 1}, 'owned stale VAPID subscription must refresh'); + + const expired = await runScenario({stored: [endpoint], expirationTime: Date.now() - 1000}); + assert.deepEqual(expired.calls, {fetch: 1, subscribe: 1, unsubscribe: 1}, 'owned expired subscription must refresh'); + + const explicit = await runScenario({stored: [], click: true}); + assert.deepEqual(explicit.calls, {fetch: 1, subscribe: 1, unsubscribe: 1}, 'explicit opt-in may replace foreign subscription'); +} + +run().catch(error => { + console.error(error); + process.exitCode = 1; +}); diff --git a/tests/javascript/webpush_test.php b/tests/javascript/webpush_test.php new file mode 100644 index 0000000..9a12584 --- /dev/null +++ b/tests/javascript/webpush_test.php @@ -0,0 +1,29 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + */ + +namespace phpbb\webpushnotifications\tests\javascript; + +use Symfony\Component\Process\Process; + +class webpush_test extends \phpbb_test_case +{ + public function test_subscription_ownership_and_refresh_behaviour(): void + { + $process = new Process([ + 'node', + __DIR__ . '/webpush_test.js', + __DIR__ . '/../../styles/all/template/webpush.js', + ]); + $process->setTimeout(10); + $process->run(); + + self::assertSame(0, $process->getExitCode(), $process->getErrorOutput() . $process->getOutput()); + } +} From 322719f76bd6b3284b132f30f4a1cc4b6b05c29b Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Fri, 13 Nov 2026 10:56:50 -0800 Subject: [PATCH 6/7] Fixes --- notification/method/webpush.php | 19 +++++------ subscription/validator.php | 23 +++++++++++-- tests/controller/controller_acp_test.php | 16 +++++++++ tests/controller/controller_webpush_test.php | 34 ++++++++++++++++++- tests/controller/fixtures/webpush.xml | 2 +- .../notification_method_webpush_test.php | 20 +++++------ tests/subscription/validator_test.php | 18 ++++++++++ ucp/controller/webpush.php | 12 +++++-- 8 files changed, 116 insertions(+), 28 deletions(-) diff --git a/notification/method/webpush.php b/notification/method/webpush.php index c0acee9..264ef0d 100644 --- a/notification/method/webpush.php +++ b/notification/method/webpush.php @@ -312,10 +312,10 @@ protected function notify_using_webpush(): void if (!$report->isSuccess()) { // Fill array of endpoints to remove if subscription has expired or is permanently gone. - // Library checks for 404/410; we also check for 401/403 auth failures and endpoints + // Library checks for 404/410; we also check endpoints // using the .invalid TLD (e.g. permanently-removed.invalid), which per RFC 6761 are // guaranteed to never resolve and are used as a sentinel for dead subscriptions. - if ($report->isSubscriptionExpired() || $this->is_subscription_unauthorized($report) || $this->is_endpoint_permanently_removed($report->getEndpoint())) + if ($this->should_remove_subscription($report)) { $expired_endpoints[] = $report->getEndpoint(); } @@ -564,20 +564,19 @@ protected function set_endpoint_padding(\Minishlink\WebPush\WebPush $web_push, s } /** - * Check if subscription push failed with a permanent authorization error + * Check if a failed subscription should be removed * - * 401/403 indicate the push service no longer accepts this subscription, - * typically due to revoked credentials, rotated VAPID keys, or the - * subscription no longer being valid for the current credentials. + * Authentication failures are not proof that a subscription expired. In + * particular, a VAPID key rotation returns 401/403 until the browser can + * recreate its subscription, so those rows must remain as ownership records. * * @param \Minishlink\WebPush\MessageSentReport $report * - * @return bool True if subscription returned 401 Unauthorized or 403 Forbidden + * @return bool True if the subscription is expired or permanently gone */ - protected function is_subscription_unauthorized(\Minishlink\WebPush\MessageSentReport $report): bool + protected function should_remove_subscription(\Minishlink\WebPush\MessageSentReport $report): bool { - $response = $report->getResponse(); - return $response && in_array($response->getStatusCode(), [401, 403], true); + return $report->isSubscriptionExpired() || $this->is_endpoint_permanently_removed($report->getEndpoint()); } /** diff --git a/subscription/validator.php b/subscription/validator.php index ffee18b..f57a4dd 100644 --- a/subscription/validator.php +++ b/subscription/validator.php @@ -12,7 +12,9 @@ use Base64Url\Base64Url; use Brick\Math\BigInteger; +use Jose\Component\Core\JWK; use Jose\Component\Core\Util\Ecc\NistCurve; +use Jose\Component\Signature\Algorithm\ES256; use Minishlink\WebPush\Utils; use Minishlink\WebPush\VAPID; @@ -32,13 +34,30 @@ public static function is_valid_vapid_keys(string $public_key, string $private_k { try { - VAPID::validate([ + $vapid = VAPID::validate([ 'subject' => 'https://example.com', 'publicKey' => $public_key, 'privateKey' => $private_key, ]); - return true; + if (!self::is_valid($public_key, Base64Url::encode(substr($vapid['privateKey'], 0, 16)))) + { + return false; + } + + [$x, $y] = Utils::unserializePublicKey($vapid['publicKey']); + $key = new JWK([ + 'kty' => 'EC', + 'crv' => 'P-256', + 'x' => Base64Url::encode($x), + 'y' => Base64Url::encode($y), + 'd' => Base64Url::encode($vapid['privateKey']), + ]); + $algorithm = new ES256(); + $input = 'phpBB VAPID key pair validation'; + $signature = $algorithm->sign($key, $input); + + return $algorithm->verify($key, $input, $signature); } catch (\Throwable $exception) { diff --git a/tests/controller/controller_acp_test.php b/tests/controller/controller_acp_test.php index 85f990d..ff38c1f 100644 --- a/tests/controller/controller_acp_test.php +++ b/tests/controller/controller_acp_test.php @@ -18,6 +18,7 @@ class controller_acp_test extends \phpbb_test_case { private const VALID_VAPID_PUBLIC_KEY = 'BIcGkq1Ncj3a2-J0UW-1A0NETLjvxZzNLiYBiPVMKNjgwmwPi5jyK87VfS4FZn9n7S9pLMQzjV3LmFuOnRSOvmI'; private const VALID_VAPID_PRIVATE_KEY = 'SrlbBEVgibWmKHYbDPu4Y2XvDWPjeGcc9fC16jq01xU'; + private const OTHER_VAPID_PUBLIC_KEY = 'BDnYSJHVZBxq834LqDGr893IfazEez7q-jYH2QBNlT0ji2C9UwGosiqz8Dp_ZN23lqAngBZyRjXVWF4ZLA8X2zI'; /** @var bool */ public static $valid_form = true; @@ -251,6 +252,21 @@ public function webpush_save_data(): array ], true, ], + 'changed public key requires private key' => [ + array_merge($valid, [ + 'wpn_webpush_vapid_public' => self::OTHER_VAPID_PUBLIC_KEY, + 'wpn_webpush_vapid_private' => '', + ]), + [ + 'S_ERROR' => true, + 'ERROR_MSG' => 'WEBPUSH_VAPID_KEYS_INVALID', + ], + [ + 'wpn_webpush_vapid_public' => self::VALID_VAPID_PUBLIC_KEY, + 'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY, + ], + false, + ], 'invalid enabled VAPID keys are rejected' => [ array_merge($valid, [ 'wpn_webpush_vapid_public' => 'short', diff --git a/tests/controller/controller_webpush_test.php b/tests/controller/controller_webpush_test.php index ad6894c..db09f4d 100644 --- a/tests/controller/controller_webpush_test.php +++ b/tests/controller/controller_webpush_test.php @@ -68,6 +68,12 @@ protected function setUp(): void $this->config = new \phpbb\config\config([ 'allow_nocensors' => false, 'sitename' => 'yourdomain.com', + 'force_server_vars' => true, + 'server_protocol' => 'http://', + 'server_name' => 'localhost', + 'server_port' => 80, + 'script_path' => '/phpBB', + 'cookie_secure' => false, ]); $config = $this->config; $this->controller_helper = $this->createMock(\phpbb\controller\helper::class); @@ -151,7 +157,7 @@ private function getExpectedResponse(): array 'heading' => 'yourdomain.com', 'title' => 'Quoted by Guest in:', 'text' => '"Welcome to phpBB"', - 'url' => 'phpBB/viewtopic.php?p=1#p1', + 'url' => 'http://localhost/phpBB/viewtopic.php?p=1#p1', 'avatar' => ['src' => ''], ]; } @@ -320,6 +326,32 @@ public function test_get_user_notification_legacy() $this->assertEquals($this->getExpectedResponse(), $response_data); } + public function legacy_notification_url_data(): array + { + return [ + 'board relative' => ['./viewtopic.php?p=1', 'http://localhost/phpBB/viewtopic.php?p=1'], + 'leading slash' => ['/viewtopic.php?p=1', 'http://localhost/phpBB/viewtopic.php?p=1'], + 'absolute HTTPS' => ['https://example.com/viewtopic.php?p=1', 'https://example.com/viewtopic.php?p=1'], + 'absolute HTTP' => ['http://example.com/viewtopic.php?p=1', 'http://example.com/viewtopic.php?p=1'], + 'unsafe scheme unchanged for worker rejection' => ['javascript:alert(1)', 'javascript:alert(1)'], + ]; + } + + /** + * @dataProvider legacy_notification_url_data + */ + public function test_legacy_notification_urls_are_normalized(string $url, string $expected): void + { + $method = new ReflectionMethod($this->controller, 'get_notification_data'); + $method->setAccessible(true); + $data = json_decode($method->invoke($this->controller, json_encode([ + 'heading' => 'Board', + 'url' => $url, + ])), true); + + self::assertSame($expected, $data['url']); + } + /** * @throws SyntaxError * @throws RuntimeError diff --git a/tests/controller/fixtures/webpush.xml b/tests/controller/fixtures/webpush.xml index f9f67fe..7f025a6 100644 --- a/tests/controller/fixtures/webpush.xml +++ b/tests/controller/fixtures/webpush.xml @@ -25,7 +25,7 @@ 2 - + 488c17afe4f18714c235b395e21b78df1c3d78bf1e13d0633ed9425d2eebf967 2 4 diff --git a/tests/notification/notification_method_webpush_test.php b/tests/notification/notification_method_webpush_test.php index 902384a..501b04c 100644 --- a/tests/notification/notification_method_webpush_test.php +++ b/tests/notification/notification_method_webpush_test.php @@ -879,36 +879,34 @@ public function test_get_ucp_template_data_separates_webpush_availability_from_d } /** - * Test is_subscription_unauthorized method with various HTTP status codes + * Authentication failures retain subscription ownership while expired subscriptions are removed */ - public function test_is_subscription_unauthorized(): void + public function test_should_remove_subscription(): void { - $reflection = new \ReflectionMethod($this->notification_method_webpush, 'is_subscription_unauthorized'); + $reflection = new \ReflectionMethod($this->notification_method_webpush, 'should_remove_subscription'); $reflection->setAccessible(true); - // Test 401 status (should return true) + // 401/403 identify VAPID authentication failures, not expired endpoints. $response_401 = $this->createMockResponse(401); $request_401 = $this->createMockRequest(); $report_401 = new \Minishlink\WebPush\MessageSentReport($request_401, $response_401, false, 'Unauthorized'); - $this->assertTrue($reflection->invoke($this->notification_method_webpush, $report_401), 'Expected 401 to be treated as unauthorized'); + $this->assertFalse($reflection->invoke($this->notification_method_webpush, $report_401), 'Expected 401 subscription ownership to be retained'); - // Test 403 status (should return true for invalid VAPID/subscription mismatches) $response_403 = $this->createMockResponse(403); $request_403 = $this->createMockRequest(); $report_403 = new \Minishlink\WebPush\MessageSentReport($request_403, $response_403, false, 'Forbidden'); - $this->assertTrue($reflection->invoke($this->notification_method_webpush, $report_403), 'Expected 403 to be treated as a permanent authorization failure'); + $this->assertFalse($reflection->invoke($this->notification_method_webpush, $report_403), 'Expected 403 subscription ownership to be retained'); - // Test 404 status (should return false, handled by isSubscriptionExpired) + // 404/410 identify expired endpoints and remain eligible for cleanup. $response_404 = $this->createMockResponse(404); $request_404 = $this->createMockRequest(); $report_404 = new \Minishlink\WebPush\MessageSentReport($request_404, $response_404, false, 'Not Found'); - $this->assertFalse($reflection->invoke($this->notification_method_webpush, $report_404), 'Expected 404 to not be treated as unauthorized'); + $this->assertTrue($reflection->invoke($this->notification_method_webpush, $report_404), 'Expected 404 subscription to be removed'); - // Test 410 status (should return false, handled by isSubscriptionExpired) $response_410 = $this->createMockResponse(410); $request_410 = $this->createMockRequest(); $report_410 = new \Minishlink\WebPush\MessageSentReport($request_410, $response_410, false, 'Gone'); - $this->assertFalse($reflection->invoke($this->notification_method_webpush, $report_410), 'Expected 410 to not be treated as unauthorized'); + $this->assertTrue($reflection->invoke($this->notification_method_webpush, $report_410), 'Expected 410 subscription to be removed'); // Test 429 status (should return false, temporary error) $response_429 = $this->createMockResponse(429); diff --git a/tests/subscription/validator_test.php b/tests/subscription/validator_test.php index 2b8ede2..c6d570e 100644 --- a/tests/subscription/validator_test.php +++ b/tests/subscription/validator_test.php @@ -10,6 +10,7 @@ namespace phpbb\webpushnotifications\tests\subscription; +use Base64Url\Base64Url; use phpbb\webpushnotifications\subscription\validator; require_once __DIR__ . '/../../vendor/autoload.php'; @@ -31,6 +32,23 @@ public function test_invalid_vapid_keys(): void self::assertFalse(validator::is_valid_vapid_keys(str_repeat('A', 30), str_repeat('B', 30))); } + public function invalid_vapid_key_data(): array + { + return [ + 'mismatched pair' => [self::VALID_VAPID_PUBLIC_KEY, 'IE5OYlmfWsMbBU1lzvr0bxrxVAXIteSkAnwGlZIhmRk'], + 'off-curve public point' => [Base64Url::encode("\x04" . str_repeat("\x00", 64)), self::VALID_VAPID_PRIVATE_KEY], + 'invalid public encoding' => [str_repeat('*', 87), self::VALID_VAPID_PRIVATE_KEY], + ]; + } + + /** + * @dataProvider invalid_vapid_key_data + */ + public function test_invalid_vapid_public_keys(string $public_key, string $private_key): void + { + self::assertFalse(validator::is_valid_vapid_keys($public_key, $private_key)); + } + public function valid_key_data(): array { return [ diff --git a/ucp/controller/webpush.php b/ucp/controller/webpush.php index 3c96aa5..0e2ffa1 100644 --- a/ucp/controller/webpush.php +++ b/ucp/controller/webpush.php @@ -248,10 +248,16 @@ private function get_notification_data(string $notification_data): string { $row_data = json_decode($notification_data, true); - // Old notification data is pre-parsed and just needs to be returned + // Old notification data is pre-parsed, but its URL is relative to the board root. if (isset($row_data['heading'])) { - return $notification_data; + if (is_string($row_data['url'] ?? null) && parse_url($row_data['url'], PHP_URL_SCHEME) === null) + { + $relative_url = ltrim($row_data['url'], '/'); + $row_data['url'] = $this->get_board_url() . preg_replace('#^(?:(?:\.\.?)/)+#', '', $relative_url); + } + + return json_encode($row_data); } // Get notification from row_data @@ -531,7 +537,7 @@ protected function prepare_avatar($avatar): array } /** - * Returns the board url (and caches it in the function) + * Returns the board URL * * @return string the generated board url */ From cf8c257e3890cfd0282dfbdf1f8565f6db7c143f Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Fri, 13 Nov 2026 14:20:43 -0800 Subject: [PATCH 7/7] Fixes --- tests/controller/controller_webpush_test.php | 19 +++++++++++++--- ucp/controller/webpush.php | 24 ++++++++++++++++---- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/tests/controller/controller_webpush_test.php b/tests/controller/controller_webpush_test.php index db09f4d..5d0b7a7 100644 --- a/tests/controller/controller_webpush_test.php +++ b/tests/controller/controller_webpush_test.php @@ -724,13 +724,26 @@ public function test_is_valid_endpoint(string $endpoint, bool $expected): void $this->assertEquals($expected, $this->controller->is_valid_endpoint($endpoint)); } - public function test_current_notification_data_is_rendered(): void + public function current_notification_url_data(): array + { + return [ + 'board relative' => ['viewtopic.php?p=1&x=2', 'http://localhost/phpBB/viewtopic.php?p=1&x=2'], + 'route relative' => ['./../../../ucp.php?i=pm&p=4', 'http://localhost/phpBB/ucp.php?i=pm&p=4'], + 'absolute HTTPS' => ['https://example.com/topic/1', 'https://example.com/topic/1'], + 'unsafe scheme unchanged for worker rejection' => ['javascript:alert(1)', 'javascript:alert(1)'], + ]; + } + + /** + * @dataProvider current_notification_url_data + */ + public function test_current_notification_data_is_rendered(string $url, string $expected_url): void { $notification = $this->createMock(\phpbb\notification\type\type_interface::class); $notification->method('users_to_query')->willReturn([]); $notification->method('get_title')->willReturn('Private Message from Alice & Bob:'); $notification->method('get_reference')->willReturn('"She said "hello" & asked about a < b in <div> tags"'); - $notification->method('get_url')->willReturn('viewtopic.php?p=1&x=2'); + $notification->method('get_url')->willReturn($url); $notification->method('get_avatar')->willReturn(''); $this->notification_manager->expects(self::once()) ->method('get_item_type_class') @@ -746,7 +759,7 @@ public function test_current_notification_data_is_rendered(): void self::assertSame('yourdomain.com', $data['heading']); self::assertSame('Private Message from Alice & Bob:', $data['title']); self::assertSame('"She said "hello" & asked about a < b in
tags"', $data['text']); - self::assertSame('viewtopic.php?p=1&x=2', $data['url']); + self::assertSame($expected_url, $data['url']); self::assertStringEndsWith('/images/avatar.png', $data['avatar']['src']); } diff --git a/ucp/controller/webpush.php b/ucp/controller/webpush.php index 0e2ffa1..ebb7bc8 100644 --- a/ucp/controller/webpush.php +++ b/ucp/controller/webpush.php @@ -251,10 +251,9 @@ private function get_notification_data(string $notification_data): string // Old notification data is pre-parsed, but its URL is relative to the board root. if (isset($row_data['heading'])) { - if (is_string($row_data['url'] ?? null) && parse_url($row_data['url'], PHP_URL_SCHEME) === null) + if (is_string($row_data['url'] ?? null)) { - $relative_url = ltrim($row_data['url'], '/'); - $row_data['url'] = $this->get_board_url() . preg_replace('#^(?:(?:\.\.?)/)+#', '', $relative_url); + $row_data['url'] = $this->normalize_notification_url($row_data['url']); } return json_encode($row_data); @@ -270,11 +269,28 @@ private function get_notification_data(string $notification_data): string 'heading' => ext::decode_entities($this->config['sitename'], ENT_QUOTES), 'title' => ext::decode_entities(strip_tags($notification->get_title()), ENT_QUOTES), 'text' => ext::decode_entities(strip_tags($notification->get_reference()), ENT_QUOTES), - 'url' => htmlspecialchars_decode($notification->get_url()), + 'url' => $this->normalize_notification_url(htmlspecialchars_decode($notification->get_url())), 'avatar' => $this->prepare_avatar($notification->get_avatar()), ]); } + /** + * Convert a board-relative notification URL to an absolute URL + * + * @param string $url Notification URL + * @return string + */ + protected function normalize_notification_url(string $url): string + { + if (parse_url($url, PHP_URL_SCHEME) !== null) + { + return $url; + } + + $relative_url = ltrim($url, '/'); + return $this->get_board_url() . preg_replace('#^(?:(?:\.\.?)/)+#', '', $relative_url); + } + /** * Handle request to push worker javascript *