Skip to content

Commit b12de4b

Browse files
iMattProiMattPro
authored
Merge pull request #127 from iMattPro/invalid-endpoint
Clean up permanently removed endpoints
2 parents a971fdb + 049e7fb commit b12de4b

File tree

2 files changed

+130
-3
lines changed

2 files changed

+130
-3
lines changed

‎notification/method/webpush.php‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -281,9 +281,11 @@ protected function notify_using_webpush(): void
281281
{
282282
if (!$report->isSuccess())
283283
{
284-
// Fill array of endpoints to remove if subscription has expired
285-
// Library checks for 404/410; we also check for 401 (Unauthorized)
286-
if ($report->isSubscriptionExpired() || $this->is_subscription_unauthorized($report))
284+
// Fill array of endpoints to remove if subscription has expired or is permanently gone.
285+
// Library checks for 404/410; we also check for 401 (Unauthorized) and endpoints
286+
// using the .invalid TLD (e.g. permanently-removed.invalid), which per RFC 6761 are
287+
// guaranteed to never resolve and are used as a sentinel for dead subscriptions.
288+
if ($report->isSubscriptionExpired() || $this->is_subscription_unauthorized($report) || $this->is_endpoint_permanently_removed($report->getEndpoint()))
287289
{
288290
$expired_endpoints[] = $report->getEndpoint();
289291
}
@@ -512,4 +514,22 @@ protected function is_subscription_unauthorized(\Minishlink\WebPush\MessageSentR
512514
$response = $report->getResponse();
513515
return $response && $response->getStatusCode() === 401;
514516
}
517+
518+
/**
519+
* Check if a push endpoint uses the .invalid TLD, meaning it can never resolve.
520+
*
521+
* Per RFC 6761, the .invalid TLD is reserved and guaranteed to never resolve in DNS.
522+
* It is commonly used as a sentinel value for dead/permanently-removed push subscriptions
523+
* (e.g. permanently-removed.invalid) where the push service has indicated the endpoint
524+
* is gone but no HTTP response was returned (e.g. cURL error 6: could not resolve host).
525+
*
526+
* @param string $endpoint
527+
*
528+
* @return bool True if the endpoint host ends with .invalid
529+
*/
530+
protected function is_endpoint_permanently_removed(string $endpoint): bool
531+
{
532+
$host = parse_url($endpoint, PHP_URL_HOST);
533+
return $host !== null && substr($host, -strlen('.invalid')) === '.invalid';
534+
}
515535
}

‎tests/notification/notification_method_webpush_test.php‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -602,6 +602,47 @@ public function test_expired_subscriptions_deleted($notification_type, $post_dat
602602
}
603603
}
604604

605+
/**
606+
* @dataProvider data_notification_webpush
607+
*/
608+
public function test_permanently_removed_subscriptions_deleted($notification_type, $post_data, $expected_users): void
609+
{
610+
// Skip test if no expected users
611+
if (empty($expected_users))
612+
{
613+
$this->assertTrue(true);
614+
return;
615+
}
616+
617+
// Insert a permanently-removed.invalid subscription for the first user.
618+
// This simulates a dead subscription whose endpoint can never resolve (RFC 6761).
619+
$first_user_id = array_key_first($expected_users);
620+
$dead_endpoint = 'https://permanently-removed.invalid/fcm/send/test_dead_subscription';
621+
$this->insert_subscription_for_user($first_user_id, $dead_endpoint);
622+
623+
$this->assertEquals(1, $this->get_subscription_count(), 'Expected 1 subscription before notification');
624+
625+
$post_data = array_merge([
626+
'post_time' => 1349413322,
627+
'poster_id' => 1,
628+
'topic_title' => '',
629+
'post_subject' => '',
630+
'post_username' => '',
631+
'forum_name' => '',
632+
], $post_data);
633+
634+
// Send notifications — should trigger cleanup of the permanently-removed subscription
635+
$this->notifications->add_notifications($notification_type, $post_data);
636+
637+
// The dead subscription should have been silently deleted
638+
$this->assertEquals(0, $this->get_subscription_count(), 'Expected permanently-removed subscription to be deleted');
639+
640+
// Verify no admin log was written — unlike real delivery failures (which log errors),
641+
// permanently-removed endpoints should be silently cleaned up without noise.
642+
$admin_logs = $this->log->get_logs('admin');
643+
$this->assertEmpty($admin_logs, 'Expected no admin log entry for a permanently-removed subscription');
644+
}
645+
605646
public function test_get_type(): void
606647
{
607648
$this->assertEquals('notification.method.phpbb.wpn.webpush', $this->notification_method_webpush->get_type());
@@ -688,6 +729,51 @@ protected function createMockRequest(): \Psr\Http\Message\RequestInterface
688729
return $request;
689730
}
690731

732+
/**
733+
* Test is_endpoint_permanently_removed method
734+
*/
735+
public function test_is_endpoint_permanently_removed(): void
736+
{
737+
$reflection = new \ReflectionMethod($this->notification_method_webpush, 'is_endpoint_permanently_removed');
738+
$reflection->setAccessible(true);
739+
740+
// .invalid TLD sentinel — should return true
741+
$this->assertTrue(
742+
$reflection->invoke($this->notification_method_webpush, 'https://permanently-removed.invalid/fcm/send/abc123'),
743+
'Expected permanently-removed.invalid to be treated as permanently removed'
744+
);
745+
746+
// Any .invalid host — should return true
747+
$this->assertTrue(
748+
$reflection->invoke($this->notification_method_webpush, 'https://some-other.invalid/push/endpoint'),
749+
'Expected any .invalid host to be treated as permanently removed'
750+
);
751+
752+
// Valid FCM endpoint — should return false
753+
$this->assertFalse(
754+
$reflection->invoke($this->notification_method_webpush, 'https://fcm.googleapis.com/fcm/send/abc123'),
755+
'Expected valid FCM endpoint to not be treated as permanently removed'
756+
);
757+
758+
// Valid Mozilla endpoint — should return false
759+
$this->assertFalse(
760+
$reflection->invoke($this->notification_method_webpush, 'https://updates.push.services.mozilla.com/push/v1/abc123'),
761+
'Expected valid Mozilla endpoint to not be treated as permanently removed'
762+
);
763+
764+
// Subdomain spoofing attempt (host ends in .invalid.attacker.com, not .invalid) — should return false
765+
$this->assertFalse(
766+
$reflection->invoke($this->notification_method_webpush, 'https://permanently-removed.invalid.attacker.com/push'),
767+
'Expected .invalid.attacker.com to not be treated as permanently removed'
768+
);
769+
770+
// Empty/invalid URL — should return false
771+
$this->assertFalse(
772+
$reflection->invoke($this->notification_method_webpush, 'not_a_url'),
773+
'Expected unparseable URL to not be treated as permanently removed'
774+
);
775+
}
776+
691777
/**
692778
* @dataProvider data_notification_webpush
693779
*/
@@ -905,6 +991,27 @@ protected function get_all_subscriptions(): array
905991
return $sql_ary;
906992
}
907993

994+
/**
995+
* Create a real subscription via the push testing service for the given user, then overwrite
996+
* its endpoint with the specified value. This gives a subscription with valid encryption keys
997+
* (required for payload encryption) but an endpoint that will never resolve — used for testing
998+
* dead/sentinel endpoints such as permanently-removed.invalid.
999+
*/
1000+
protected function insert_subscription_for_user(int $user_id, string $endpoint): void
1001+
{
1002+
// Get a real subscription from the push testing service so the p256dh/auth keys are
1003+
// valid base64url-encoded EC keys that the library can actually encrypt against.
1004+
$subscription_data = $this->create_subscription_for_user($user_id);
1005+
1006+
// Overwrite the endpoint to the dead one we want to test with.
1007+
$push_subscriptions_table = $this->container->getParameter('tables.phpbb.wpn.push_subscriptions');
1008+
$sql = 'UPDATE ' . $push_subscriptions_table . "
1009+
SET endpoint = '" . $this->db->sql_escape($endpoint) . "'
1010+
WHERE user_id = " . (int) $user_id . "
1011+
AND endpoint = '" . $this->db->sql_escape($subscription_data['endpoint']) . "'";
1012+
$this->db->sql_query($sql);
1013+
}
1014+
9081015
/**
9091016
* @depends test_get_subscription
9101017
*/

0 commit comments

Comments
 (0)