From 93718f7408d43d4fc9f0572cf9f81837da87c718 Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Mon, 19 Oct 2026 12:39:03 -0700
Subject: [PATCH] Remove push subscriptions on a password change, and post them
 again

A push subscription belongs to the user, not a session, so a device
signed out by a password change kept getting notifications. Changing
the password now removes every subscription along with the sessions,
in the same transaction.

A browser still signed in keeps its own subscription in its service
worker, so the notifications controller posts it once per session: the
bell names the session by a digest of its id, never its token, and the
browser remembers the last one it posted for. That post never
unsubscribes the browser when the server refuses it.

A request that authenticated just before the change could put a
subscription back after it. The create action now checks the session
still exists and saves in one immediate transaction, after validating
the endpoint outside it.

From #371.

(cherry picked from commit 6cab9240978ec6d305d5f043bacda1da800d0d12)
---
 .../users/push_subscriptions_controller.rb    | 27 ++++---
 .../users/push_subscriptions_helper.rb        |  7 ++
 .../controllers/notifications_controller.js   | 21 +++++-
 app/models/user.rb                            |  6 +-
 app/views/rooms/involvements/_bell.html.erb   |  3 +-
 .../users/profiles_controller_test.rb         | 14 +++-
 .../push_subscriptions_controller_test.rb     | 48 +++++++++++++
 test/models/user_test.rb                      | 20 +++++-
 test/system/push_subscription_resync_test.rb  | 71 +++++++++++++++++++
 9 files changed, 201 insertions(+), 16 deletions(-)
 create mode 100644 app/helpers/users/push_subscriptions_helper.rb
 create mode 100644 test/system/push_subscription_resync_test.rb

diff --git a/app/controllers/users/push_subscriptions_controller.rb b/app/controllers/users/push_subscriptions_controller.rb
index 8e263b23..e4e46524 100644
--- a/app/controllers/users/push_subscriptions_controller.rb
+++ b/app/controllers/users/push_subscriptions_controller.rb
@@ -5,17 +5,14 @@ class Users::PushSubscriptionsController < ApplicationController
   end
 
   def create
-    if subscription = @push_subscriptions.find_by(push_subscription_params)
-      # Existing endpoints must pass current validations
-      if subscription.valid?
-        subscription.touch
-        head :ok
-      else
-        head :unprocessable_entity
-      end
+    subscription = @push_subscriptions.find_or_initialize_by(push_subscription_params)
+    subscription.user_agent = request.user_agent if subscription.new_record?
+
+    # Existing endpoints must pass current validations too
+    if subscription.valid?
+      head save_while_signed_in(subscription) ? :ok : :unauthorized
     else
-      subscription = @push_subscriptions.create push_subscription_params.merge(user_agent: request.user_agent)
-      head subscription.persisted? ? :ok : :unprocessable_entity
+      head :unprocessable_entity
     end
   end
 
@@ -29,6 +26,16 @@ class Users::PushSubscriptionsController < ApplicationController
       @push_subscriptions = Current.user.push_subscriptions
     end
 
+    # A password change ends every session and subscription. A request that authenticated just before it mustn't
+    # put one back, so the session is checked under the same write lock as the save. Validation, which looks up
+    # the endpoint's address, runs before taking the lock.
+    def save_while_signed_in(subscription)
+      Push::Subscription.transaction do
+        Session.exists?(Current.session.id) &&
+          (subscription.new_record? ? subscription.save!(validate: false) : subscription.touch)
+      end
+    end
+
     def push_subscription_params
       params.require(:push_subscription).permit(:endpoint, :p256dh_key, :auth_key)
     end
diff --git a/app/helpers/users/push_subscriptions_helper.rb b/app/helpers/users/push_subscriptions_helper.rb
new file mode 100644
index 00000000..57c681f9
--- /dev/null
+++ b/app/helpers/users/push_subscriptions_helper.rb
@@ -0,0 +1,7 @@
+module Users::PushSubscriptionsHelper
+  # Names the session without revealing its token, so the browser can tell when it's in a new one. The creation
+  # time keeps an id reused by a new database from looking like the same session.
+  def push_subscription_session_marker
+    Digest::SHA256.hexdigest("push-subscription-session-#{Current.session.id}-#{Current.session.created_at.to_f}") if Current.session
+  end
+end
diff --git a/app/javascript/controllers/notifications_controller.js b/app/javascript/controllers/notifications_controller.js
index 3ce53a5c..b3698a31 100644
--- a/app/javascript/controllers/notifications_controller.js
+++ b/app/javascript/controllers/notifications_controller.js
@@ -4,8 +4,10 @@ import { pageIsTurboPreview } from "helpers/turbo_helpers"
 import { onNextEventLoopTick } from "helpers/timing_helpers"
 import { getCookie, setCookie } from "lib/cookie"
 
+const PUSH_SUBSCRIPTION_SYNCED_KEY = "push-subscription-synced-session"
+
 export default class extends Controller {
-  static values = { subscriptionsUrl: String }
+  static values = { subscriptionsUrl: String, session: String }
   static targets = [ "notAllowedNotice", "bell", "details" ]
   static classes = [ "attention" ]
 
@@ -25,6 +27,8 @@ export default class extends Controller {
           this.#showBellAlert()
         }
       }
+
+      this.#resyncPushSubscriptionOncePerSession()
     }
   }
 
@@ -109,6 +113,21 @@ export default class extends Controller {
     if (!response.ok) subscription.unsubscribe()
   }
 
+  // A password change removes the account's subscriptions, the one this browser holds included. Post it again
+  // once per session, so a browser that's still signed in keeps getting notifications. A failure here leaves the
+  // browser's own subscription alone: the next session tries again.
+  async #resyncPushSubscriptionOncePerSession() {
+    if (this.#allowed && this.sessionValue && localStorage.getItem(PUSH_SUBSCRIPTION_SYNCED_KEY) !== this.sessionValue) {
+      const registration = await this.#serviceWorkerRegistration
+      const subscription = await registration?.pushManager?.getSubscription()
+
+      if (Notification.permission === "granted" && subscription) {
+        await post(this.subscriptionsUrlValue, { body: this.#extractJsonPayloadAsString(subscription), responseKind: "turbo-stream" })
+        localStorage.setItem(PUSH_SUBSCRIPTION_SYNCED_KEY, this.sessionValue)
+      }
+    }
+  }
+
   async #requestPermissionAndSubscribe(registration) {
     const permission = await Notification.requestPermission()
     if (permission === "granted") this.#subscribe(registration)
diff --git a/app/models/user.rb b/app/models/user.rb
index 716088e8..07f4bd03 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -60,10 +60,12 @@ class User < ApplicationRecord
     end
 
     # A new password is how someone shuts out whoever else got into their account, so it ends every session, copies
-    # of the changing browser's cookie included. Connections close once that commits, registered on the transaction
-    # because a later save in it would replace the saved change an after_commit condition reads.
+    # of the changing browser's cookie included, and every push subscription; browsers still signed in post theirs
+    # again. Connections close once that commits, registered on the transaction because a later save in it would
+    # replace the saved change an after_commit condition reads.
     def sign_out_all_sessions
       sessions.delete_all
+      push_subscriptions.delete_all
       self.class.current_transaction.after_commit { disconnect_signed_out_sessions }
     end
 
diff --git a/app/views/rooms/involvements/_bell.html.erb b/app/views/rooms/involvements/_bell.html.erb
index d9fa32c5..266bdfb0 100644
--- a/app/views/rooms/involvements/_bell.html.erb
+++ b/app/views/rooms/involvements/_bell.html.erb
@@ -1,6 +1,7 @@
 <span>
   <span class="button_to_change_notifying"
-      data-controller="notifications" data-notifications-subscriptions-url-value="<%= user_push_subscriptions_path %>" data-notifications-attention-class="btn--pulsing">
+      data-controller="notifications" data-notifications-subscriptions-url-value="<%= user_push_subscriptions_path %>"
+      data-notifications-session-value="<%= push_subscription_session_marker %>" data-notifications-attention-class="btn--pulsing">
     <%= turbo_frame_tag dom_id(room, :involvement), data: {
           controller: "turbo-frame", action: "notifications:ready@window->turbo-frame#load", turbo_frame_url_param: room_involvement_path(room) } do %>
       <button class="btn" data-action="click->notifications#attemptToSubscribe" data-notifications-target="bell">
diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb
index 4e93520c..3d82162a 100644
--- a/test/controllers/users/profiles_controller_test.rb
+++ b/test/controllers/users/profiles_controller_test.rb
@@ -136,6 +136,18 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest
     assert_equal "Dave", users(:david).reload.name
   end
 
+  test "changing the password removes the other devices' push subscriptions, and this browser can register its own again" do
+    stub_web_push_dns_resolution
+    endpoint = push_subscriptions(:david_chrome).attributes.slice("endpoint", "p256dh_key", "auth_key")
+
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
+    assert_empty users(:david).push_subscriptions
+
+    post user_push_subscriptions_url, params: { push_subscription: endpoint }
+    assert_response :ok
+    assert_equal [ endpoint["endpoint"] ], users(:david).push_subscriptions.pluck(:endpoint)
+  end
+
   test "updates are limited to the current user" do
     put user_profile_url(users(:jason)), params: { user: { name: "John Doe" } }
 
@@ -148,7 +160,7 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest
       other_device.post session_url, params: { email_address: users(:david).email_address, password: "secret123456" }
       ActionCable.server.expects(:remote_connections).never
 
-      assert_no_difference -> { Session.count } do
+      assert_no_changes -> { [ Session.count, Push::Subscription.count ] } do
         put user_profile_url, params: params
       end
 
diff --git a/test/controllers/users/push_subscriptions_controller_test.rb b/test/controllers/users/push_subscriptions_controller_test.rb
index e69fc4a2..dd0bcd4e 100644
--- a/test/controllers/users/push_subscriptions_controller_test.rb
+++ b/test/controllers/users/push_subscriptions_controller_test.rb
@@ -74,4 +74,52 @@ class Users::PushSubscriptionsControllerTest < ActionDispatch::IntegrationTest
       assert_redirected_to user_push_subscriptions_url
     end
   end
+
+  test "a signed-out session can't register a subscription" do
+    Session.delete_all
+
+    assert_no_difference -> { Push::Subscription.count } do
+      post user_push_subscriptions_url, params: { push_subscription: new_subscription_params }
+    end
+
+    assert_redirected_to new_session_url
+  end
+
+  test "a session signed out while its request is in flight doesn't register a subscription" do
+    Push::Subscription.any_instance.stubs(:valid?).with { Session.delete_all; true }.returns(true)
+
+    assert_no_difference -> { Push::Subscription.count } do
+      post user_push_subscriptions_url, params: { push_subscription: new_subscription_params }
+    end
+
+    assert_response :unauthorized
+  end
+
+  test "a session signed out while its request is in flight doesn't keep an existing subscription alive" do
+    subscription = push_subscriptions(:david_chrome)
+    Push::Subscription.any_instance.stubs(:valid?).with { Session.delete_all; true }.returns(true)
+
+    assert_no_changes -> { subscription.reload.updated_at } do
+      post user_push_subscriptions_url, params: { push_subscription: subscription.attributes.slice("endpoint", "p256dh_key", "auth_key") }
+    end
+
+    assert_response :unauthorized
+  end
+
+  test "a room names its session for the browser to post its subscription again in a new one" do
+    get room_url(rooms(:watercooler))
+    first_marker = css_select("[data-notifications-session-value]").first["data-notifications-session-value"]
+
+    assert_match(/\A\h{64}\z/, first_marker)
+    assert_not_includes response.body, users(:david).sessions.order(:id).last.token
+
+    sign_in :david
+    get room_url(rooms(:watercooler))
+    assert_not_equal first_marker, css_select("[data-notifications-session-value]").first["data-notifications-session-value"]
+  end
+
+  private
+    def new_subscription_params
+      { "endpoint" => "https://fcm.googleapis.com/fcm/send/abc123", "p256dh_key" => "123", "auth_key" => "456" }
+    end
 end
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 8dbe0ea4..5cd445a8 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -80,10 +80,28 @@ class UserTest < ActiveSupport::TestCase
     end
   end
 
+  test "changing the password removes every push subscription of the user" do
+    assert users(:david).push_subscriptions.any?
+
+    users(:david).update!(password: "a new secret123456")
+
+    assert_empty users(:david).push_subscriptions.reload
+    assert users(:jason).push_subscriptions.any?
+  end
+
+  test "a password change that rolls back keeps push subscriptions" do
+    assert_no_changes -> { users(:david).push_subscriptions.count } do
+      User.transaction do
+        users(:david).update!(password: "a new secret123456")
+        raise ActiveRecord::Rollback
+      end
+    end
+  end
+
   test "changing other details keeps every session" do
     ActionCable.server.expects(:remote_connections).never
 
-    assert_no_changes -> { users(:david).sessions.count } do
+    assert_no_changes -> { [ users(:david).sessions.count, users(:david).push_subscriptions.count ] } do
       users(:david).update!(name: "Dave", email_address: "dave@37signals.com")
     end
   end
diff --git a/test/system/push_subscription_resync_test.rb b/test/system/push_subscription_resync_test.rb
new file mode 100644
index 00000000..fab36825
--- /dev/null
+++ b/test/system/push_subscription_resync_test.rb
@@ -0,0 +1,71 @@
+require "application_system_test_case"
+
+class PushSubscriptionResyncTest < ApplicationSystemTestCase
+  ENDPOINT = "https://fcm.googleapis.com/fcm/send/system-test"
+
+  setup do
+    stub_web_push_dns_resolution
+    fake_a_browser_push_subscription
+  end
+
+  teardown do
+    page.execute_script("localStorage.clear()") if page.current_url.start_with?("http")
+  end
+
+  test "a browser posts its subscription again once per session, so it survives a password change" do
+    sign_in "david@37signals.com"
+    assert_subscription_registered
+    first_sync = subscription.updated_at
+
+    join_room rooms(:hq)
+    assert_equal first_sync, subscription.reload.updated_at, "posted again in the same session"
+
+    change_password
+    assert_nil Push::Subscription.find_by(endpoint: ENDPOINT)
+
+    join_room rooms(:hq)
+    assert_subscription_registered
+    assert_not page.evaluate_script("localStorage.getItem('unsubscribed-for-test')")
+  end
+
+  test "a subscription the server refuses stays in the browser" do
+    stub_dns_resolution("169.254.169.254")
+
+    sign_in "david@37signals.com"
+    Timeout.timeout(5) { sleep 0.1 until page.evaluate_script("localStorage.getItem('push-subscription-synced-session')") }
+    join_room rooms(:hq)
+
+    assert_not Push::Subscription.exists?(endpoint: ENDPOINT)
+    assert_not page.evaluate_script("localStorage.getItem('unsubscribed-for-test')")
+  end
+
+  private
+    # Headless Chrome has no push service, so stand in for the subscription the service worker would hold.
+    def fake_a_browser_push_subscription
+      page.driver.browser.execute_cdp "Page.addScriptToEvaluateOnNewDocument", source: <<~JS
+        Object.defineProperty(Notification, "permission", { get: () => "granted" })
+        const subscription = {
+          toJSON: () => ({ endpoint: "#{ENDPOINT}", keys: { p256dh: "p256dh-key", auth: "auth-key" } }),
+          unsubscribe: async () => { localStorage.setItem("unsubscribed-for-test", "true") }
+        }
+        navigator.serviceWorker.getRegistration = async () => ({ pushManager: { getSubscription: async () => subscription } })
+      JS
+    end
+
+    def change_password
+      visit user_profile_url
+      find("input[name='user[password_challenge]']").fill_in with: "secret123456"
+      find("input[name='user[password]']").fill_in with: "a new secret123456"
+      find("input[name='user[password]']").send_keys :enter
+      assert_selector ".flash", text: "✓"
+    end
+
+    def assert_subscription_registered
+      assert_selector "[data-controller~=notifications]", visible: :all
+      Timeout.timeout(5) { sleep 0.1 until Push::Subscription.exists?(endpoint: ENDPOINT, user: users(:david)) }
+    end
+
+    def subscription
+      Push::Subscription.find_by!(endpoint: ENDPOINT)
+    end
+end