Merge pull request #126 from iMattPro/security

iMattPro · web-flow · commit a971fdb4845b · 2026-04-04T07:38:01.000-07:00
Security improvement - allow only known push services
diff --git a/event/listener.php b/event/listener.php
@@ -82,16 +82,33 @@ public function __construct(config $config, controller_helper $controller_helper
 	public static function getSubscribedEvents()
 	{
 		return [
-			'core.page_header_after'			=> [['load_template_data'], ['pwa_manifest']],
-			'core.ucp_display_module_before'	=> 'load_language',
-			'core.acp_main_notice'				=> 'compatibility_notice',
+			'core.user_setup'						=> 'load_language_on_setup',
+			'core.page_header_after'				=> [['load_template_data'], ['pwa_manifest']],
+			'core.ucp_display_module_before'		=> 'load_language',
+			'core.acp_main_notice'					=> 'compatibility_notice',
 			'core.acp_board_config_edit_add'		=> 'acp_pwa_options',
-			'core.acp_board_config_emoji_enabled'=> 'acp_pwa_allow_emoji',
-			'core.validate_config_variable'		=> 'validate_pwa_options',
-			'core.help_manager_add_block_after'	=> 'wpn_faq',
+			'core.acp_board_config_emoji_enabled'	=> 'acp_pwa_allow_emoji',
+			'core.validate_config_variable'			=> 'validate_pwa_options',
+			'core.help_manager_add_block_after'		=> 'wpn_faq',
 		];
 	}
 
+	/**
+	 * Load common language file during user setup
+	 *
+	 * @param	\phpbb\event\data	$event	The event object
+	 * @return	void
+	 */
+	public function load_language_on_setup($event)
+	{
+		$lang_set_ext = $event['lang_set_ext'];
+		$lang_set_ext[] = [
+			'ext_name' => 'phpbb/webpushnotifications',
+			'lang_set' => 'common',
+		];
+		$event['lang_set_ext'] = $lang_set_ext;
+	}
+
 	/**
 	 * Load template data
 	 */
diff --git a/language/en/common.php b/language/en/common.php
@@ -0,0 +1,42 @@
+<?php
+/**
+ *
+ * phpBB Browser Push Notifications. An extension for the phpBB Forum Software package.
+ *
+ * @copyright (c) 2026, phpBB Limited <https://www.phpbb.com>
+ * @license GNU General Public License, version 2 (GPL-2.0)
+ *
+ */
+
+/**
+ * DO NOT CHANGE
+ */
+if (!defined('IN_PHPBB'))
+{
+	exit;
+}
+
+if (empty($lang) || !is_array($lang))
+{
+	$lang = [];
+}
+
+// DEVELOPERS PLEASE NOTE
+//
+// All language files should use UTF-8 as their encoding and the files must not contain a BOM.
+//
+// Placeholders can now contain order information, e.g. instead of
+// 'Page %s of %s' you can (and should) write 'Page %1$s of %2$s', this allows
+// translators to re-order the output of data while ensuring it remains correct
+//
+// You do not need this where single placeholders are used, e.g. 'Message %d' is fine
+// equally where a string contains only two placeholders which are used to wrap text
+// in a url you again do not need to specify an order e.g., 'Click %sHERE%s' is fine
+//
+// Some characters you may want to copy&paste:
+// ’ » “ ” …
+//
+
+$lang = array_merge($lang, [
+	'WEBPUSH_INVALID_ENDPOINT'	=> 'The push notification endpoint is not from a recognised push service.',
+]);
diff --git a/language/ru/common.php b/language/ru/common.php
@@ -0,0 +1,42 @@
+<?php
+/**
+ *
+ * phpBB Browser Push Notifications. An extension for the phpBB Forum Software package.
+ *
+ * @copyright (c) 2026, phpBB Limited <https://www.phpbb.com>
+ * @license GNU General Public License, version 2 (GPL-2.0)
+ *
+ */
+
+/**
+ * DO NOT CHANGE
+ */
+if (!defined('IN_PHPBB'))
+{
+	exit;
+}
+
+if (empty($lang) || !is_array($lang))
+{
+	$lang = [];
+}
+
+// DEVELOPERS PLEASE NOTE
+//
+// All language files should use UTF-8 as their encoding and the files must not contain a BOM.
+//
+// Placeholders can now contain order information, e.g. instead of
+// 'Page %s of %s' you can (and should) write 'Page %1$s of %2$s', this allows
+// translators to re-order the output of data while ensuring it remains correct
+//
+// You do not need this where single placeholders are used, e.g. 'Message %d' is fine
+// equally where a string contains only two placeholders which are used to wrap text
+// in a url you again do not need to specify an order e.g., 'Click %sHERE%s' is fine
+//
+// Some characters you may want to copy&paste:
+// ’ » “ ” …
+//
+
+$lang = array_merge($lang, [
+	'WEBPUSH_INVALID_ENDPOINT'	=> 'Конечная точка push-уведомления не принадлежит известному сервису push-уведомлений.',
+]);
diff --git a/styles/all/template/webpush.js b/styles/all/template/webpush.js
@@ -301,22 +301,35 @@ function PhpbbWebpush() {
 			});
 
 			const loadingIndicator = phpbb.loadingIndicator();
-			fetch(subscribeUrl, {
-				method: 'POST',
-				headers: {
-					'X-Requested-With': 'XMLHttpRequest',
-				},
-				body: getFormData(newSubscription),
-			})
-				.then(response => {
-					loadingIndicator.fadeOut(phpbb.alertTime);
-					return response.json();
-				})
-				.then(handleSubscribe)
-				.catch(error => {
-					loadingIndicator.fadeOut(phpbb.alertTime);
-					phpbb.alert(ajaxErrorTitle, error);
+			try {
+				const response = await fetch(subscribeUrl, {
+					method: 'POST',
+					headers: {
+						'X-Requested-With': 'XMLHttpRequest',
+					},
+					body: getFormData(newSubscription),
 				});
+				const data = await response.json();
+				loadingIndicator.fadeOut(phpbb.alertTime);
+
+				if (data.success) {
+					handleSubscribe(data);
+				} else {
+					// Server rejected the subscription; clean up the browser-side subscription
+					// without awaiting, so a failure here can't bubble to the outer catch and
+					// incorrectly trigger promptDenied or show the wrong error message.
+					newSubscription.unsubscribe().catch(console.error);
+					promptDenied.set();
+					hidePopup(document.getElementById('wpn_popup_prompt'));
+					phpbb.alert(ajaxErrorTitle, data.message || subscribeButton.getAttribute('data-l-unsupported'));
+				}
+			} catch (error) {
+				loadingIndicator.fadeOut(phpbb.alertTime);
+				// Clean up the browser-side subscription so it doesn't become orphaned
+				// when the server request fails (network error, bad JSON, etc.).
+				newSubscription.unsubscribe().catch(console.error);
+				phpbb.alert(ajaxErrorTitle, error.message || subscribeButton.getAttribute('data-l-unsupported'));
+			}
 		} catch (error) {
 			promptDenied.set(); // deny the prompt on error to prevent repeated prompting
 			hidePopup(document.getElementById('wpn_popup_prompt'));
diff --git a/tests/controller/controller_webpush_test.php b/tests/controller/controller_webpush_test.php
@@ -392,7 +392,7 @@ public function test_sub_unsubscribe_success()
 
 		$symfony_request = $this->createMock(\phpbb\symfony_request::class);
 		$symfony_request->method('get')->willReturn(json_encode([
-			'endpoint' => 'test_endpoint',
+			'endpoint' => 'https://fcm.googleapis.com/fcm/send/test_endpoint',
 			'expiration_time' => 0,
 			'keys' => ['p256dh' => 'test_p256dh', 'auth' => 'test_auth']
 		]));
@@ -404,7 +404,7 @@ public function test_sub_unsubscribe_success()
 
 		$this->assertEquals([
 			'user_id' => '2',
-			'endpoint' => 'test_endpoint',
+			'endpoint' => 'https://fcm.googleapis.com/fcm/send/test_endpoint',
 			'p256dh' => 'test_p256dh',
 			'auth' => 'test_auth',
 			'expiration_time' => '0',
@@ -420,6 +420,77 @@ public function test_sub_unsubscribe_success()
 		$this->assertEmpty($this->get_subscription_data());
 	}
 
+	public function data_subscribe_invalid_endpoint(): array
+	{
+		return [
+			'no_host'				=> ['not_a_url'],
+			'disallowed_host'		=> ['https://evil.example.com/push/endpoint'],
+			'disallowed_subdomain'	=> ['https://evil.fcm.googleapis.com.attacker.com/push'],
+			'empty_string'			=> [''],
+		];
+	}
+
+	/**
+	 * @dataProvider data_subscribe_invalid_endpoint
+	 */
+	public function test_subscribe_invalid_endpoint(string $endpoint): void
+	{
+		$this->form_helper->method('check_form_tokens')->willReturn(true);
+		$this->request->method('is_ajax')->willReturn(true);
+		$this->user->data['user_id'] = 2;
+		$this->user->data['is_bot'] = false;
+		$this->user->data['user_type'] = USER_NORMAL;
+
+		$symfony_request = $this->createMock(\phpbb\symfony_request::class);
+		$symfony_request->method('get')->willReturn(json_encode([
+			'endpoint' => $endpoint,
+			'expiration_time' => 0,
+			'keys' => ['p256dh' => 'test_p256dh', 'auth' => 'test_auth']
+		]));
+
+		$this->expectException(http_exception::class);
+		$this->expectExceptionMessage('WEBPUSH_INVALID_ENDPOINT');
+
+		$this->controller->subscribe($symfony_request);
+	}
+
+	public function data_is_valid_endpoint(): array
+	{
+		return [
+			// Exact whitelist matches
+			['https://android.googleapis.com/gcm/send/test', true],
+			['https://fcm.googleapis.com/fcm/send/test', true],
+			['https://updates.push.services.mozilla.com/push/v1/test', true],
+			['https://updates-autopush.stage.mozaws.net/push/v1/test', true],
+			['https://updates-autopush.dev.mozaws.net/push/v1/test', true],
+			// Wildcard *.notify.windows.com
+			['https://am-0.notify.windows.com/throttledthirdparty/test', true],
+			['https://db5.notify.windows.com/push/test', true],
+			// Wildcard *.push.apple.com
+			['https://api.push.apple.com/3/device/test', true],
+			['https://api.development.push.apple.com/3/device/test', true],
+			// Invalid: disallowed host
+			['https://evil.example.com/push/endpoint', false],
+			// Invalid: subdomain spoofing
+			['https://evil.fcm.googleapis.com.attacker.com/push', false],
+			// Invalid: not a URL
+			['not_a_url', false],
+			// Invalid: bare wildcard domain without subdomain
+			['https://notify.windows.com/push', false],
+			['https://push.apple.com/push', false],
+			// Invalid: empty string
+			['', false],
+		];
+	}
+
+	/**
+	 * @dataProvider data_is_valid_endpoint
+	 */
+	public function test_is_valid_endpoint(string $endpoint, bool $expected): void
+	{
+		$this->assertEquals($expected, $this->controller->is_valid_endpoint($endpoint));
+	}
+
 	public function test_toggle_popup_enable_to_disable()
 	{
 		$this->form_helper->method('check_form_tokens')->willReturn(true);
diff --git a/tests/event/listener_test.php b/tests/event/listener_test.php
@@ -141,6 +141,7 @@ public function test_construct()
 	public function test_getSubscribedEvents()
 	{
 		self::assertEquals([
+			'core.user_setup',
 			'core.page_header_after',
 			'core.ucp_display_module_before',
 			'core.acp_main_notice',
@@ -151,6 +152,25 @@ public function test_getSubscribedEvents()
 		], array_keys(\phpbb\webpushnotifications\event\listener::getSubscribedEvents()));
 	}
 
+	public function test_load_language_on_setup($lang_set_ext = [])
+	{
+		$this->set_listener();
+
+		$dispatcher = new \phpbb\event\dispatcher();
+		$dispatcher->addListener('core.user_setup', [$this->listener, 'load_language_on_setup']);
+
+		$event_data = ['lang_set_ext'];
+		$event_data_after = $dispatcher->trigger_event('core.user_setup', compact($event_data));
+		extract($event_data_after);
+
+		self::assertEquals([
+			[
+				'ext_name' => 'phpbb/webpushnotifications',
+				'lang_set' => 'common',
+			]
+		], $lang_set_ext);
+	}
+
 	public function test_load_language()
 	{
 		$this->set_listener();
diff --git a/ucp/controller/webpush.php b/ucp/controller/webpush.php
@@ -36,6 +36,21 @@ class webpush
 	/** @var string UCP form token name */
 	public const FORM_TOKEN_UCP = 'ucp_webpush';
 
+	/** @var array Allowed push service endpoint hosts https://github.com/pushpad/known-push-services */
+	public const PUSH_SERVICE_WHITELIST = [
+		'android.googleapis.com',
+		'fcm.googleapis.com',
+		'updates.push.services.mozilla.com',
+		'updates-autopush.stage.mozaws.net',
+		'updates-autopush.dev.mozaws.net',
+	];
+
+	/** @var array Allowed push service endpoint host wildcard suffixes (e.g. *.notify.windows.com) https://github.com/pushpad/known-push-services */
+	public const PUSH_SERVICE_WILDCARD_SUFFIXES = [
+		'.notify.windows.com',
+		'.push.apple.com',
+	];
+
 	/** @var config */
 	protected $config;
 
@@ -279,6 +294,39 @@ public function worker(): Response
 		return $response;
 	}
 
+	/**
+	 * Check if a push subscription endpoint belongs to an allowed push service
+	 *
+	 * @param string $endpoint The push subscription endpoint URL
+	 * @return bool True if the endpoint host matches a known/allowed push service
+	 */
+	public function is_valid_endpoint(string $endpoint): bool
+	{
+		$host = parse_url($endpoint, PHP_URL_HOST);
+
+		if (empty($host))
+		{
+			return false;
+		}
+
+		$host = strtolower($host);
+
+		if (in_array($host, self::PUSH_SERVICE_WHITELIST, true))
+		{
+			return true;
+		}
+
+		foreach (self::PUSH_SERVICE_WILDCARD_SUFFIXES as $suffix)
+		{
+			if (substr($host, -strlen($suffix)) === $suffix)
+			{
+				return true;
+			}
+		}
+
+		return false;
+	}
+
 	/**
 	 * Check (un)subscribe form for valid link hash
 	 *
@@ -312,6 +360,13 @@ public function subscribe(symfony_request $symfony_request): JsonResponse
 
 		$data = json_sanitizer::decode($symfony_request->get('data', ''));
 
+		$data['endpoint'] = $data['endpoint'] ?? '';
+
+		if (!$this->is_valid_endpoint($data['endpoint']))
+		{
+			throw new http_exception(Response::HTTP_BAD_REQUEST, 'WEBPUSH_INVALID_ENDPOINT');
+		}
+
 		$sql = 'INSERT INTO ' . $this->push_subscriptions_table . ' ' . $this->db->sql_build_array('INSERT', [
 			'user_id'			=> $this->user->id(),
 			'endpoint'			=> $data['endpoint'],
