From 4c2a2c27c177b5c2d5e8245c574f9d788bbfdd80 Mon Sep 17 00:00:00 2001
From: Emelia Smith <ThisIsMissEm@users.noreply.github.com>
Date: Thu, 4 Sep 2025 10:00:44 +0200
Subject: [PATCH] Automatically remove invalid Web::PushSubscriptions (#35987)

---
 app/workers/web/push_notification_worker.rb   |  9 ++++++++
 .../web/push_notification_worker_spec.rb      | 22 +++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/app/workers/web/push_notification_worker.rb b/app/workers/web/push_notification_worker.rb
index 32279a9e74d..f9b1dc4f193 100644
--- a/app/workers/web/push_notification_worker.rb
+++ b/app/workers/web/push_notification_worker.rb
@@ -15,6 +15,15 @@ class Web::PushNotificationWorker
 
     return if @notification.updated_at < TTL.ago
 
+    # Clean up old Web::PushSubscriptions that were added before validation of
+    # the endpoint and keys: #30542, #30540
+    unless @subscription.valid?
+      Rails.logger.debug { "Web::PushSubscription is invalid, removing: #{subscription_id}" }
+      @subscription.destroy!
+
+      return
+    end
+
     # Polymorphically associated activity could have been deleted
     # in the meantime, so we have to double-check before proceeding
     return unless @notification.activity.present? && @subscription.pushable?(@notification)
diff --git a/spec/workers/web/push_notification_worker_spec.rb b/spec/workers/web/push_notification_worker_spec.rb
index d18d6c4d682..7ef4cd4fe51 100644
--- a/spec/workers/web/push_notification_worker_spec.rb
+++ b/spec/workers/web/push_notification_worker_spec.rb
@@ -36,6 +36,9 @@ RSpec.describe Web::PushNotificationWorker do
   let(:std_input) { 'When I grow up, I want to be a watermelon' }
   let(:std_ciphertext) { 'DGv6ra1nlYgDCS1FRnbzlwAAEABBBP4z9KsN6nGRTbVYI_c7VJSPQTBtkgcy27mlmlMoZIIgDll6e3vCYLocInmYWAmS6TlzAC8wEqKK6PBru3jl7A_yl95bQpu6cVPTpK4Mqgkf1CXztLVBSt2Ks3oZwbuwXPXLWyouBWLVWGNWQexSgSxsj_Qulcy4a-fN' }
 
+  # Invalid subscription:
+  let(:invalid_subscription) { Fabricate.build(:web_push_subscription, user_id: user.id, key_p256dh: 'invalid', key_auth: 'invalid', endpoint: endpoint, standard: true, data: { alerts: { notification.type => true } }) }
+
   describe 'perform' do
     around do |example|
       original_private = Rails.configuration.x.vapid.private_key
@@ -83,6 +86,25 @@ RSpec.describe Web::PushNotificationWorker do
     end
     # rubocop:enable RSpec/SubjectStub
 
+    context 'with invalid record that will fail' do
+      before do
+        # Fabricator always runs validation, here we deliberately want to bypass
+        # the validation, simulating an invalid Web::PushSubscription that was
+        # created before PRs #30542, #30540 added validation.
+        invalid_subscription.save(validate: false)
+      end
+
+      it 'removes the record and does not process the request' do
+        expect { subject.perform(invalid_subscription.id, notification.id) }
+          .to_not raise_error
+
+        expect { invalid_subscription.reload }
+          .to raise_error ActiveRecord::RecordNotFound
+
+        expect(a_request(:post, endpoint)).to_not have_been_made
+      end
+    end
+
     def legacy_web_push_endpoint_request
       a_request(
         :post,