From d3a28001fab5cbd2cf8c325541a8a4aa0fa91294 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Wed, 20 May 2026 17:20:43 -0700 Subject: [PATCH 1/5] Guard against empty endpoint --- ucp/controller/webpush.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ucp/controller/webpush.php b/ucp/controller/webpush.php index e66a53f..d3b802e 100644 --- a/ucp/controller/webpush.php +++ b/ucp/controller/webpush.php @@ -458,7 +458,7 @@ public function unsubscribe(symfony_request $symfony_request): JsonResponse $data = json_sanitizer::decode($symfony_request->get('data', '')); - $endpoint = $data['endpoint']; + $endpoint = is_string($data['endpoint'] ?? null) ? $data['endpoint'] : ''; $sql = 'DELETE FROM ' . $this->push_subscriptions_table . ' WHERE user_id = ' . (int) $this->user->id() . " From 64941f2709674d84ea8363a64b4c86fbc411923c Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Wed, 20 May 2026 17:21:15 -0700 Subject: [PATCH 2/5] Add user id to push token map to prevent overwrites --- notification/method/webpush.php | 4 +- .../notification_method_webpush_test.php | 55 ++++++++++++++++++- 2 files changed, 54 insertions(+), 5 deletions(-) diff --git a/notification/method/webpush.php b/notification/method/webpush.php index c538efe..c1e994e 100644 --- a/notification/method/webpush.php +++ b/notification/method/webpush.php @@ -158,7 +158,7 @@ public function notify() ]; $data = self::clean_data($data); $insert_buffer->insert($data); - $this->push_token_map[$notification->notification_type_id][$notification->item_id] = $data['push_token']; + $this->push_token_map[$notification->notification_type_id][$notification->item_id][$notification->user_id] = $data['push_token']; } $insert_buffer->flush(); @@ -239,7 +239,7 @@ protected function notify_using_webpush(): void 'type_id' => $notification->notification_type_id, 'user_id' => $notification->user_id, 'version' => $this->config['assets_version'], - 'token' => hash('sha256', $user['user_form_salt'] . $this->push_token_map[$notification->notification_type_id][$notification->item_id]), + 'token' => hash('sha256', $user['user_form_salt'] . $this->push_token_map[$notification->notification_type_id][$notification->item_id][$notification->user_id]), ]; $json_data = json_encode($data); diff --git a/tests/notification/notification_method_webpush_test.php b/tests/notification/notification_method_webpush_test.php index dbefe7a..69e8042 100644 --- a/tests/notification/notification_method_webpush_test.php +++ b/tests/notification/notification_method_webpush_test.php @@ -19,9 +19,6 @@ require_once __DIR__ . '/../../../../../../tests/notification/base.php'; require_once __DIR__ . '/../../vendor/autoload.php'; // load the composer dependencies for this extension -/** - * @group slow - */ class notification_method_webpush_test extends \phpbb_tests_notification_base { /** @var string[] VAPID keys for testing purposes */ @@ -648,6 +645,58 @@ public function test_get_type(): void $this->assertEquals('notification.method.phpbb.wpn.webpush', $this->notification_method_webpush->get_type()); } + public function test_push_token_map_is_per_user(): void + { + // Verifies that when multiple users are notified about the same item, + // each user's push token is stored and used independently. + // Previously the map was keyed [type_id][item_id], so the last user's + // token overwrote all others, making every other user's token invalid. + $subscription_info = []; + $expected_users = [2 => ['user_id' => '2'], 3 => ['user_id' => '3'], 4 => ['user_id' => '4']]; + foreach ($expected_users as $user_id => $user_data) + { + $subscription_info[$user_id] = $this->create_subscription_for_user($user_id); + } + + $post_data = [ + 'post_time' => 1349413322, + 'poster_id' => 1, + 'topic_title' => '', + 'post_subject' => '', + 'post_username' => '', + 'forum_name' => '', + 'forum_id' => '1', + 'post_id' => '2', + 'topic_id' => '1', + ]; + + $this->notifications->add_notifications('notification.type.post', $post_data); + + // Fetch the stored rows only for users we created subscriptions for + $webpush_table = $this->container->getParameter('tables.phpbb.wpn.notification_push'); + $sql = 'SELECT user_id, push_token FROM ' . $webpush_table . ' WHERE ' . $this->db->sql_in_set('user_id', array_keys($expected_users)) . ' ORDER BY user_id ASC'; + $result = $this->db->sql_query($sql); + $rows = $this->db->sql_fetchrowset($result); + $this->db->sql_freeresult($result); + + $this->assertCount(count($expected_users), $rows, 'Each expected user must have a notification row'); + $tokens = array_column($rows, 'push_token'); + $this->assertEquals(count($tokens), count(array_unique($tokens)), 'Each user must have a unique push_token'); + + // Verify each message payload contains the token hashed with that specific user's salt + foreach ($rows as $row) + { + $user_id = (int) $row['user_id']; + $client_hash = basename($subscription_info[$user_id]['endpoint']); + $messages = $this->get_messages_for_subscription($client_hash); + $this->assertNotEmpty($messages); + $payload = json_decode($messages[0], true); + $user = $this->user_loader->get_user($user_id); + $expected_token = hash('sha256', $user['user_form_salt'] . $row['push_token']); + $this->assertEquals($expected_token, $payload['token'], 'Token in push payload must match hash of that user\'s salt + their own push_token'); + } + } + public function test_get_ucp_template_data_uses_millisecond_expiration_time(): void { $this->user->data['user_id'] = 2; From 250397664ee5c6330b508c43ca15eedc10ab2487 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Thu, 21 May 2026 22:54:21 -0700 Subject: [PATCH 3/5] Fix migration issues on MSSQL --- migrations/handle_subscriptions.php | 35 ++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/migrations/handle_subscriptions.php b/migrations/handle_subscriptions.php index f3e63b9..34c842e 100644 --- a/migrations/handle_subscriptions.php +++ b/migrations/handle_subscriptions.php @@ -89,17 +89,32 @@ public function copy_subscription_tables() $wpn_notification_push_table = $this->table_prefix . 'wpn_notification_push'; $wpn_push_subscriptions_table = $this->table_prefix . 'wpn_push_subscriptions'; - /* - * If webpush notification method exists in phpBB core, - * copy all subscriptions data over the corresponding core tables. - */ - foreach ([ - $core_notification_push_table => $wpn_notification_push_table, - $core_push_subscriptions_table => $wpn_push_subscriptions_table - ] as $core_table => $ext_table) + // Copy push table data + $sql = 'INSERT INTO ' . $core_notification_push_table . ' + (notification_type_id, item_id, item_parent_id, user_id, push_data, notification_time, push_token) + SELECT notification_type_id, item_id, item_parent_id, user_id, push_data, notification_time, push_token + FROM ' . $wpn_notification_push_table; + $this->db->sql_query($sql); + + // Turn on identity insert on mssql to be able to insert into + // identity columns (e.g. id) + if (strpos($this->db->get_sql_layer(), 'mssql') !== false) + { + $sql = 'SET IDENTITY_INSERT ' . $core_push_subscriptions_table . ' ON'; + $this->db->sql_query($sql); + } + + // Copy subscription table data + $sql = 'INSERT INTO ' . $core_push_subscriptions_table . ' + (subscription_id, user_id, endpoint, expiration_time, p256dh, auth) + SELECT subscription_id, user_id, endpoint, expiration_time, p256dh, auth + FROM ' . $wpn_push_subscriptions_table; + $this->db->sql_query($sql); + + // Disable identity insert on mssql again + if (strpos($this->db->get_sql_layer(), 'mssql') !== false) { - $sql = 'INSERT INTO ' . $core_table . ' - SELECT * FROM ' . $ext_table; + $sql = 'SET IDENTITY_INSERT ' . $core_push_subscriptions_table . ' OFF'; $this->db->sql_query($sql); } } From d37c3896ede60ae9e650cf66e23fa1381e520825 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Fri, 22 May 2026 07:58:30 -0700 Subject: [PATCH 4/5] Empty notification queue when there are no valid users --- notification/method/webpush.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/notification/method/webpush.php b/notification/method/webpush.php index c1e994e..be78011 100644 --- a/notification/method/webpush.php +++ b/notification/method/webpush.php @@ -196,8 +196,11 @@ protected function notify_using_webpush(): void // Load all the users we need $notify_users = array_diff($user_ids, $banned_users); + + // If we have no users (e.g. all recipients are banned) empty queue and exit if (empty($notify_users)) { + $this->empty_queue(); return; } From b734245300933bcc8677c9bd026abf1850d5caa7 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Fri, 22 May 2026 07:58:56 -0700 Subject: [PATCH 5/5] Potential bug guards --- notification/method/webpush.php | 2 +- styles/all/template/webpush.js | 22 ++++++++++++++++------ ucp/controller/webpush.php | 11 ++++++++--- 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/notification/method/webpush.php b/notification/method/webpush.php index be78011..42e2f69 100644 --- a/notification/method/webpush.php +++ b/notification/method/webpush.php @@ -534,6 +534,6 @@ protected function is_subscription_unauthorized(\Minishlink\WebPush\MessageSentR protected function is_endpoint_permanently_removed(string $endpoint): bool { $host = parse_url($endpoint, PHP_URL_HOST); - return $host !== null && substr($host, -strlen('.invalid')) === '.invalid'; + return is_string($host) && substr($host, -strlen('.invalid')) === '.invalid'; } } diff --git a/styles/all/template/webpush.js b/styles/all/template/webpush.js index 955c31c..a6a9f4b 100644 --- a/styles/all/template/webpush.js +++ b/styles/all/template/webpush.js @@ -479,19 +479,29 @@ function PhpbbWebpush() { }, body: getFormData({ endpoint: subscription.endpoint }), }) - .then(() => { - loadingIndicator.fadeOut(phpbb.alertTime); - return subscription.unsubscribe(); - }) - .then(unsubscribed => { + .then(async(response) => { + let data = null; + try { + data = await response.json(); + } catch (e) { + // Ignore JSON parsing failures and fall back below. + } + if (!response.ok || !data || !data.success) { + throw new Error(data && data.message ? data.message : 'Unsubscribe failed.'); + } + + const unsubscribed = await subscription.unsubscribe(); + if (unsubscribed) { removeStoredSubscription(subscription.endpoint); setSubscriptionState(false); } }) .catch(error => { + phpbb.alert(ajaxErrorTitle, error.message || error); + }) + .finally(() => { loadingIndicator.fadeOut(phpbb.alertTime); - phpbb.alert(ajaxErrorTitle, error); }); } diff --git a/ucp/controller/webpush.php b/ucp/controller/webpush.php index d3b802e..9dc7f28 100644 --- a/ucp/controller/webpush.php +++ b/ucp/controller/webpush.php @@ -304,7 +304,7 @@ public function is_valid_endpoint(string $endpoint): bool { $host = parse_url($endpoint, PHP_URL_HOST); - if (empty($host)) + if (!is_string($host) || empty($host)) { return false; } @@ -409,8 +409,13 @@ public function subscribe(symfony_request $symfony_request): JsonResponse */ protected function get_subscription_write_data(array $data): array { - $p256dh = is_string($data['keys']['p256dh'] ?? null) ? $data['keys']['p256dh'] : ''; - $auth = is_string($data['keys']['auth'] ?? null) ? $data['keys']['auth'] : ''; + $keys = $data['keys'] ?? []; + if (!is_array($keys)) + { + $keys = []; + } + $p256dh = is_string($keys['p256dh'] ?? null) ? $keys['p256dh'] : ''; + $auth = is_string($keys['auth'] ?? null) ? $keys['auth'] : ''; if ($p256dh === '' || $auth === '') {