Skip to content

Commit 7edb777

Browse files
iMattProiMattPro
authored
Merge pull request #150 from iMattPro/fixes
Audit Fixes
2 parents 5dabeb3 + cf8c257 commit 7edb777

21 files changed

Lines changed: 822 additions & 67 deletions

‎controller/acp_controller.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
use phpbb\template\template;
2222
use phpbb\user;
2323
use phpbb\webpushnotifications\ext;
24+
use phpbb\webpushnotifications\subscription\validator as subscription_validator;
2425

2526
class acp_controller
2627
{
@@ -223,6 +224,15 @@ protected function save_settings()
223224
{
224225
// Validate config values
225226
validate_config_vars($display_settings, $config_array, $this->errors);
227+
228+
if (empty($this->errors)
229+
&& !subscription_validator::is_valid_vapid_keys(
230+
$config_array['wpn_webpush_vapid_public'] ?? '',
231+
$config_array['wpn_webpush_vapid_private'] ?? $this->config['wpn_webpush_vapid_private']
232+
))
233+
{
234+
$this->errors[] = $this->lang->lang('WEBPUSH_VAPID_KEYS_INVALID');
235+
}
226236
}
227237

228238
if ($this->display_errors())

‎language/en/info_mcp_webpushnotifications.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?php
2+
/**
3+
*
4+
* phpBB Browser Push Notifications. An extension for the phpBB Forum Software package.
5+
*
6+
* @copyright (c) 2026, phpBB Limited <https://www.phpbb.com>
7+
* @license GNU General Public License, version 2 (GPL-2.0)
8+
*
9+
*/
10+
11+
if (!defined('IN_PHPBB'))
12+
{
13+
exit;
14+
}
15+
16+
if (empty($lang) || !is_array($lang))
17+
{
18+
$lang = [];
19+
}
20+
21+
$lang = array_merge($lang, [
22+
'LOG_WEBPUSH_SUBSCRIPTION_REMOVED' => '<strong>Removed Web Push subscription:</strong><br>» %s',
23+
]);

‎language/en/webpushnotifications_module_acp.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@
5353
'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.',
5454
'WEBPUSH_VAPID_KEYS_GENERATED'=> 'New key pair generated. Submit this form to activate it.',
5555
'WEBPUSH_VAPID_KEYS_GENERATION_FAILED'=> 'Could not generate a key pair in this browser. Enter a valid pair manually or try a supported browser.',
56+
'WEBPUSH_VAPID_KEYS_INVALID' => 'The VAPID identification keys are invalid.',
5657
'WEBPUSH_VAPID_ROTATION_WARNING'=> '<strong>Important:</strong> Changing either key invalidates all existing Web Push subscriptions.',
5758
'WEBPUSH_VAPID_PUBLIC' => 'Server identification public key',
5859
'WEBPUSH_VAPID_PUBLIC_EXPLAIN' => 'Shared with push services to authenticate messages sent from your board.',

‎language/ru/info_mcp_webpushnotifications.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?php
2+
/**
3+
*
4+
* phpBB Browser Push Notifications. An extension for the phpBB Forum Software package.
5+
*
6+
* @copyright (c) 2026, phpBB Limited <https://www.phpbb.com>
7+
* @license GNU General Public License, version 2 (GPL-2.0)
8+
*
9+
*/
10+
11+
if (!defined('IN_PHPBB'))
12+
{
13+
exit;
14+
}
15+
16+
if (empty($lang) || !is_array($lang))
17+
{
18+
$lang = [];
19+
}
20+
21+
$lang = array_merge($lang, [
22+
'LOG_WEBPUSH_SUBSCRIPTION_REMOVED' => '<strong>Удалена подписка на браузерные push—уведомления:</strong><br>» %s',
23+
]);

‎language/ru/webpushnotifications_module_acp.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@
5353
'WEBPUSH_ROTATE_VAPID_KEYS_CONFIRM'=> 'Сохранение новых ключей сделает недействительными все существующие подписки на браузерные push—уведомления. Пользователи не будут получать браузерные push—уведомления до следующего посещения конференции, при котором их подписки будут пересозданы.',
5454
'WEBPUSH_VAPID_KEYS_GENERATED'=> 'Новая пара ключей сгенерирована. Отправьте эту форму, чтобы активировать её.',
5555
'WEBPUSH_VAPID_KEYS_GENERATION_FAILED'=> 'Не удалось сгенерировать пару ключей в этом браузере. Введите корректную пару вручную или используйте поддерживаемый браузер.',
56+
'WEBPUSH_VAPID_KEYS_INVALID' => 'Ключи идентификации VAPID недействительны.',
5657
'WEBPUSH_VAPID_ROTATION_WARNING'=> '<strong>Важно:</strong> изменение любого ключа делает недействительными все существующие подписки на браузерные push—уведомления.',
5758
'WEBPUSH_VAPID_PUBLIC' => 'Публичный ключ идентификации сервера',
5859
'WEBPUSH_VAPID_PUBLIC_EXPLAIN' => 'Передаётся службам push—уведомлений для аутентификации сообщений, отправляемых конференцией.',

‎notification/method/webpush.php‎

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -211,15 +211,17 @@ protected function notify_using_webpush(): void
211211
$user_subscription_map = $this->get_user_subscription_map($notify_users);
212212
$invalid_subscriptions = [];
213213

214-
// Validate each stored subscription once so malformed keys cannot interrupt the delivery batch.
214+
// Validate each stored subscription once so malformed data cannot interrupt the delivery batch.
215215
foreach ($user_subscription_map as $user_id => $subscriptions)
216216
{
217217
$valid_subscriptions = [];
218218
$user = $this->user_loader->get_user($user_id);
219219
foreach ($subscriptions as $subscription)
220220
{
221-
if (!is_string($subscription['p256dh'] ?? null)
221+
if (!is_string($subscription['endpoint'] ?? null)
222+
|| !is_string($subscription['p256dh'] ?? null)
222223
|| !is_string($subscription['auth'] ?? null)
224+
|| !$this->is_valid_endpoint_url($subscription['endpoint'])
223225
|| !subscription_validator::is_valid($subscription['p256dh'], $subscription['auth']))
224226
{
225227
$invalid_subscriptions[$subscription['subscription_id']] = $user;
@@ -239,7 +241,17 @@ protected function notify_using_webpush(): void
239241
],
240242
];
241243

242-
$web_push = new \Minishlink\WebPush\WebPush($auth);
244+
try
245+
{
246+
$web_push = new \Minishlink\WebPush\WebPush($auth);
247+
}
248+
catch (\Throwable $exception)
249+
{
250+
$this->log->add('critical', ANONYMOUS, '', 'LOG_WEBPUSH_MESSAGE_FAIL', false, [$exception->getMessage()]);
251+
$this->empty_queue();
252+
$this->remove_invalid_subscriptions($invalid_subscriptions);
253+
return;
254+
}
243255

244256
$number_of_notifications = 0;
245257

@@ -300,10 +312,10 @@ protected function notify_using_webpush(): void
300312
if (!$report->isSuccess())
301313
{
302314
// Fill array of endpoints to remove if subscription has expired or is permanently gone.
303-
// Library checks for 404/410; we also check for 401/403 auth failures and endpoints
315+
// Library checks for 404/410; we also check endpoints
304316
// using the .invalid TLD (e.g. permanently-removed.invalid), which per RFC 6761 are
305317
// guaranteed to never resolve and are used as a sentinel for dead subscriptions.
306-
if ($report->isSubscriptionExpired() || $this->is_subscription_unauthorized($report) || $this->is_endpoint_permanently_removed($report->getEndpoint()))
318+
if ($this->should_remove_subscription($report))
307319
{
308320
$expired_endpoints[] = $report->getEndpoint();
309321
}
@@ -449,6 +461,18 @@ protected function get_user_subscription_map(array $notify_users): array
449461
return $user_subscription_map;
450462
}
451463

464+
/**
465+
* Check whether a stored subscription endpoint is valid for delivery
466+
* Wrapper used here exists only as test seam
467+
*
468+
* @param string $endpoint Push subscription endpoint URL
469+
* @return bool
470+
*/
471+
protected function is_valid_endpoint_url(string $endpoint): bool
472+
{
473+
return subscription_validator::is_valid_endpoint_url($endpoint);
474+
}
475+
452476
/**
453477
* Remove subscriptions
454478
*
@@ -540,20 +564,19 @@ protected function set_endpoint_padding(\Minishlink\WebPush\WebPush $web_push, s
540564
}
541565

542566
/**
543-
* Check if subscription push failed with a permanent authorization error
567+
* Check if a failed subscription should be removed
544568
*
545-
* 401/403 indicate the push service no longer accepts this subscription,
546-
* typically due to revoked credentials, rotated VAPID keys, or the
547-
* subscription no longer being valid for the current credentials.
569+
* Authentication failures are not proof that a subscription expired. In
570+
* particular, a VAPID key rotation returns 401/403 until the browser can
571+
* recreate its subscription, so those rows must remain as ownership records.
548572
*
549573
* @param \Minishlink\WebPush\MessageSentReport $report
550574
*
551-
* @return bool True if subscription returned 401 Unauthorized or 403 Forbidden
575+
* @return bool True if the subscription is expired or permanently gone
552576
*/
553-
protected function is_subscription_unauthorized(\Minishlink\WebPush\MessageSentReport $report): bool
577+
protected function should_remove_subscription(\Minishlink\WebPush\MessageSentReport $report): bool
554578
{
555-
$response = $report->getResponse();
556-
return $response && in_array($response->getStatusCode(), [401, 403], true);
579+
return $report->isSubscriptionExpired() || $this->is_endpoint_permanently_removed($report->getEndpoint());
557580
}
558581

559582
/**

‎styles/all/template/push_worker.js.twig‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ self.addEventListener('notificationclick', event => {
8888
}
8989

9090
try {
91-
const url = new URL(event.notification.data.url, '{{ U_BOARD_URL|e("js") }}/');
91+
const url = new URL(event.notification.data.url, self.location.href);
9292
if (url.protocol === 'http:' || url.protocol === 'https:') {
9393
event.waitUntil(self.clients.openWindow(url.href));
9494
}

‎styles/all/template/webpush.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,11 @@ function PhpbbWebpush() {
140140
return;
141141
}
142142

143+
// A subscription this account does not own was made by another member on this browser.
144+
if (!subscriptions.some(stored => stored.endpoint === subscription.endpoint)) {
145+
return;
146+
}
147+
143148
if (shouldRefreshSubscription(subscription)) {
144149
await refreshSubscription(registration, subscription);
145150
return;

‎subscription/validator.php‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,78 @@
1212

1313
use Base64Url\Base64Url;
1414
use Brick\Math\BigInteger;
15+
use Jose\Component\Core\JWK;
1516
use Jose\Component\Core\Util\Ecc\NistCurve;
17+
use Jose\Component\Signature\Algorithm\ES256;
1618
use Minishlink\WebPush\Utils;
19+
use Minishlink\WebPush\VAPID;
1720

1821
/**
1922
* Web Push subscription key validator
2023
*/
2124
class validator
2225
{
26+
/**
27+
* Validate VAPID identification keys
28+
*
29+
* @param string $public_key VAPID public key
30+
* @param string $private_key VAPID private key
31+
* @return bool
32+
*/
33+
public static function is_valid_vapid_keys(string $public_key, string $private_key): bool
34+
{
35+
try
36+
{
37+
$vapid = VAPID::validate([
38+
'subject' => 'https://example.com',
39+
'publicKey' => $public_key,
40+
'privateKey' => $private_key,
41+
]);
42+
43+
if (!self::is_valid($public_key, Base64Url::encode(substr($vapid['privateKey'], 0, 16))))
44+
{
45+
return false;
46+
}
47+
48+
[$x, $y] = Utils::unserializePublicKey($vapid['publicKey']);
49+
$key = new JWK([
50+
'kty' => 'EC',
51+
'crv' => 'P-256',
52+
'x' => Base64Url::encode($x),
53+
'y' => Base64Url::encode($y),
54+
'd' => Base64Url::encode($vapid['privateKey']),
55+
]);
56+
$algorithm = new ES256();
57+
$input = 'phpBB VAPID key pair validation';
58+
$signature = $algorithm->sign($key, $input);
59+
60+
return $algorithm->verify($key, $input, $signature);
61+
}
62+
catch (\Throwable $exception)
63+
{
64+
return false;
65+
}
66+
}
67+
68+
/**
69+
* Check that an endpoint is a plain HTTPS URL with a well-formed hostname
70+
*
71+
* @param string $endpoint Push subscription endpoint URL
72+
* @return bool
73+
*/
74+
public static function is_valid_endpoint_url(string $endpoint): bool
75+
{
76+
$parts = parse_url($endpoint);
77+
78+
return is_array($parts)
79+
&& strtolower($parts['scheme'] ?? '') === 'https'
80+
&& !isset($parts['user'])
81+
&& !isset($parts['pass'])
82+
&& (!isset($parts['port']) || $parts['port'] === 443)
83+
&& is_string($parts['host'] ?? null)
84+
&& filter_var($parts['host'], FILTER_VALIDATE_DOMAIN, FILTER_FLAG_HOSTNAME) !== false;
85+
}
86+
2387
/**
2488
* Validate Web Push subscription keys
2589
*

‎tests/controller/controller_acp_test.php‎

Lines changed: 45 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@
1616

1717
class controller_acp_test extends \phpbb_test_case
1818
{
19+
private const VALID_VAPID_PUBLIC_KEY = 'BIcGkq1Ncj3a2-J0UW-1A0NETLjvxZzNLiYBiPVMKNjgwmwPi5jyK87VfS4FZn9n7S9pLMQzjV3LmFuOnRSOvmI';
20+
private const VALID_VAPID_PRIVATE_KEY = 'SrlbBEVgibWmKHYbDPu4Y2XvDWPjeGcc9fC16jq01xU';
21+
private const OTHER_VAPID_PUBLIC_KEY = 'BDnYSJHVZBxq834LqDGr893IfazEez7q-jYH2QBNlT0ji2C9UwGosiqz8Dp_ZN23lqAngBZyRjXVWF4ZLA8X2zI';
22+
1923
/** @var bool */
2024
public static $valid_form = true;
2125

@@ -213,8 +217,8 @@ public function webpush_save_data(): array
213217
{
214218
$valid = [
215219
'wpn_webpush_enable' => 1,
216-
'wpn_webpush_vapid_public' => str_repeat('a', 25),
217-
'wpn_webpush_vapid_private' => str_repeat('b', 25),
220+
'wpn_webpush_vapid_public' => self::VALID_VAPID_PUBLIC_KEY,
221+
'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY,
218222
'wpn_webpush_dropdown_subscribe' => 1,
219223
'wpn_webpush_method_enabled' => 0,
220224
'wpn_webpush_popup_prompt' => 1,
@@ -228,8 +232,8 @@ public function webpush_save_data(): array
228232
'ERROR_MSG' => '',
229233
],
230234
[
231-
'wpn_webpush_vapid_public' => str_repeat('a', 25),
232-
'wpn_webpush_vapid_private' => str_repeat('b', 25),
235+
'wpn_webpush_vapid_public' => self::VALID_VAPID_PUBLIC_KEY,
236+
'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY,
233237
'wpn_webpush_method_enabled' => 0,
234238
],
235239
true,
@@ -243,11 +247,26 @@ public function webpush_save_data(): array
243247
'ERROR_MSG' => '',
244248
],
245249
[
246-
'wpn_webpush_vapid_public' => str_repeat('a', 25),
247-
'wpn_webpush_vapid_private' => 'existing-private-key',
250+
'wpn_webpush_vapid_public' => self::VALID_VAPID_PUBLIC_KEY,
251+
'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY,
248252
],
249253
true,
250254
],
255+
'changed public key requires private key' => [
256+
array_merge($valid, [
257+
'wpn_webpush_vapid_public' => self::OTHER_VAPID_PUBLIC_KEY,
258+
'wpn_webpush_vapid_private' => '',
259+
]),
260+
[
261+
'S_ERROR' => true,
262+
'ERROR_MSG' => 'WEBPUSH_VAPID_KEYS_INVALID',
263+
],
264+
[
265+
'wpn_webpush_vapid_public' => self::VALID_VAPID_PUBLIC_KEY,
266+
'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY,
267+
],
268+
false,
269+
],
251270
'invalid enabled VAPID keys are rejected' => [
252271
array_merge($valid, [
253272
'wpn_webpush_vapid_public' => 'short',
@@ -258,8 +277,23 @@ public function webpush_save_data(): array
258277
'ERROR_MSG' => 'SETTING_TOO_SHORT<br>SETTING_TOO_SHORT',
259278
],
260279
[
261-
'wpn_webpush_vapid_public' => 'existing-public-key',
262-
'wpn_webpush_vapid_private' => 'existing-private-key',
280+
'wpn_webpush_vapid_public' => self::VALID_VAPID_PUBLIC_KEY,
281+
'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY,
282+
],
283+
false,
284+
],
285+
'invalid VAPID key encoding is rejected' => [
286+
array_merge($valid, [
287+
'wpn_webpush_vapid_public' => str_repeat('A', 30),
288+
'wpn_webpush_vapid_private' => str_repeat('B', 30),
289+
]),
290+
[
291+
'S_ERROR' => true,
292+
'ERROR_MSG' => 'WEBPUSH_VAPID_KEYS_INVALID',
293+
],
294+
[
295+
'wpn_webpush_vapid_public' => self::VALID_VAPID_PUBLIC_KEY,
296+
'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY,
263297
],
264298
false,
265299
],
@@ -276,7 +310,7 @@ public function webpush_save_data(): array
276310
[
277311
'wpn_webpush_enable' => 0,
278312
'wpn_webpush_vapid_public' => '',
279-
'wpn_webpush_vapid_private' => 'existing-private-key',
313+
'wpn_webpush_vapid_private' => self::VALID_VAPID_PRIVATE_KEY,
280314
],
281315
true,
282316
],
@@ -288,8 +322,8 @@ public function webpush_save_data(): array
288322
*/
289323
public function test_save_settings_validates_and_persists_webpush_config(array $input, array $error_vars, array $expected_config, $expect_saved): void
290324
{
291-
$this->config['wpn_webpush_vapid_public'] = 'existing-public-key';
292-
$this->config['wpn_webpush_vapid_private'] = 'existing-private-key';
325+
$this->config['wpn_webpush_vapid_public'] = self::VALID_VAPID_PUBLIC_KEY;
326+
$this->config['wpn_webpush_vapid_private'] = self::VALID_VAPID_PRIVATE_KEY;
293327

294328
$this->request->method('variable')
295329
->with('config', ['' => ''], true)

0 commit comments

Comments
 (0)