From 5c5c82b27a0cb44e1cb2a8037e620d648dc89af6 Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Mon, 3 Aug 2026 14:55:04 -0700
Subject: [PATCH 1/3] Scope room lookup to the type each controller administers

Rooms::DirectsController relaxes ensure_can_administer to true, because every
participant in a direct room may administer it. set_room was inherited unscoped,
though, so that relaxation applied to any room the caller was merely a member of:
DELETE /rooms/directs/<id> destroyed open and closed rooms and all their messages.

The same unscoped lookup let a direct room be loaded by the opens and closeds
controllers, where force_room_type promoted it. Promoting a DM to open grants every
user on the account membership and republishes the whole conversation, including the
other participant's messages; converting it to closed lets the initiator revise who
is in it and lock the other participant out.

Each controller now narrows room_scope to the types it may act on. Opens and closeds
keep reach into each other, since converting between them is a feature. Neither can
reach a direct room, and directs can only reach directs.

Room also refuses to change type away from Rooms::Direct, so the invariant holds for
any future caller of becomes! rather than only these two controllers.
---
 app/controllers/rooms/closeds_controller.rb   |  6 ++++
 app/controllers/rooms/directs_controller.rb   |  7 ++++-
 app/controllers/rooms/opens_controller.rb     |  6 ++++
 app/controllers/rooms_controller.rb           |  8 ++++-
 app/models/room.rb                            | 11 +++++++
 .../rooms/closeds_controller_test.rb          | 12 ++++++++
 .../rooms/directs_controller_test.rb          | 30 +++++++++++++++++++
 .../rooms/opens_controller_test.rb            | 19 ++++++++++++
 8 files changed, 97 insertions(+), 2 deletions(-)

diff --git a/app/controllers/rooms/closeds_controller.rb b/app/controllers/rooms/closeds_controller.rb
index 0e8bdf7..c0d01d4 100644
--- a/app/controllers/rooms/closeds_controller.rb
+++ b/app/controllers/rooms/closeds_controller.rb
@@ -42,6 +42,12 @@ class Rooms::ClosedsController < RoomsController
       @room = @room.becomes!(Rooms::Closed)
     end
 
+    # Open and closed rooms convert into each other, so both are in reach here. Direct
+    # rooms never are: converting one would let its creator revise who's in it.
+    def room_scope
+      Current.user.rooms.without_directs
+    end
+
     def grantees
       User.where(id: grantee_ids)
     end
diff --git a/app/controllers/rooms/directs_controller.rb b/app/controllers/rooms/directs_controller.rb
index dc31090..d9b1ec4 100644
--- a/app/controllers/rooms/directs_controller.rb
+++ b/app/controllers/rooms/directs_controller.rb
@@ -29,8 +29,13 @@ class Rooms::DirectsController < RoomsController
       end
     end
 
-    # All users in a direct room can administer it
+    # All users in a direct room can administer it. Only direct rooms, though: this
+    # relaxation is why room_scope below has to keep every other type out of reach.
     def ensure_can_administer
       true
     end
+
+    def room_scope
+      Current.user.rooms.directs
+    end
 end
diff --git a/app/controllers/rooms/opens_controller.rb b/app/controllers/rooms/opens_controller.rb
index c2bda9e..2605d22 100644
--- a/app/controllers/rooms/opens_controller.rb
+++ b/app/controllers/rooms/opens_controller.rb
@@ -40,6 +40,12 @@ class Rooms::OpensController < RoomsController
       @room = @room.becomes!(Rooms::Open)
     end
 
+    # Open and closed rooms convert into each other, so both are in reach here. Direct
+    # rooms never are: promoting one would republish its history to the whole account.
+    def room_scope
+      Current.user.rooms.without_directs
+    end
+
     def broadcast_create_room(room)
       broadcast_prepend_to :rooms, target: :shared_rooms, partial: "users/sidebars/rooms/shared", locals: { room: room }
     end
diff --git a/app/controllers/rooms_controller.rb b/app/controllers/rooms_controller.rb
index 2c308c8..1f1d28e 100644
--- a/app/controllers/rooms_controller.rb
+++ b/app/controllers/rooms_controller.rb
@@ -20,13 +20,19 @@ class RoomsController < ApplicationController
 
   private
     def set_room
-      if room = Current.user.rooms.find_by(id: params[:room_id] || params[:id])
+      if room = room_scope.find_by(id: params[:room_id] || params[:id])
         @room = room
       else
         redirect_to root_url, alert: "Room not found or inaccessible"
       end
     end
 
+    # Subclasses narrow this to the room types they're allowed to act on, so that one
+    # room namespace can't be used to reach another's rooms.
+    def room_scope
+      Current.user.rooms
+    end
+
     def ensure_can_administer
       head :forbidden unless Current.user.can_administer?(@room)
     end
diff --git a/app/models/room.rb b/app/models/room.rb
index a9d97ac..20865fc 100644
--- a/app/models/room.rb
+++ b/app/models/room.rb
@@ -22,6 +22,8 @@ class Room < ApplicationRecord
 
   belongs_to :creator, class_name: "User", default: -> { Current.user }
 
+  validate :direct_rooms_keep_their_type, on: :update
+
   scope :opens,           -> { where(type: "Rooms::Open") }
   scope :closeds,         -> { where(type: "Rooms::Closed") }
   scope :directs,         -> { where(type: "Rooms::Direct") }
@@ -65,6 +67,15 @@ class Room < ApplicationRecord
   end
 
   private
+    # Open and closed rooms convert into each other freely. A direct room can't become
+    # either: its participants agreed to a private conversation, not to one whose
+    # audience someone else gets to widen afterwards.
+    def direct_rooms_keep_their_type
+      if type_changed? && type_was == "Rooms::Direct"
+        errors.add :type, "can't be changed for a direct room"
+      end
+    end
+
     def unread_memberships(message)
       memberships.visible.disconnected.where.not(user: message.creator).update_all(unread_at: message.created_at, updated_at: Time.current)
     end
diff --git a/test/controllers/rooms/closeds_controller_test.rb b/test/controllers/rooms/closeds_controller_test.rb
index d8976ac..504745d 100644
--- a/test/controllers/rooms/closeds_controller_test.rb
+++ b/test/controllers/rooms/closeds_controller_test.rb
@@ -66,6 +66,18 @@ class Rooms::ClosedsControllerTest < ActionDispatch::IntegrationTest
     assert rooms(:designers).reload.name, "Designers"
   end
 
+  test "a direct room can't be converted to closed and have its participants revised" do
+    sign_in :kevin
+    direct = rooms(:bender_and_kevin)
+
+    put rooms_closed_url(direct), params: {
+      room: { name: "Watercooler" }, user_ids: [ users(:kevin).id, users(:jz).id ]
+    }
+
+    assert_equal "Rooms::Direct", Room.find(direct.id).type
+    assert_equal [ users(:bender).id, users(:kevin).id ].sort, Room.find(direct.id).user_ids.sort
+  end
+
   test "remove yourself" do
     assert_difference -> { users(:david).rooms.count }, -1 do
       put rooms_closed_url(rooms(:designers), params: { room: { name: "Designers" }, user_ids: [ users(:jason).id, users(:jz).id ] })
diff --git a/test/controllers/rooms/directs_controller_test.rb b/test/controllers/rooms/directs_controller_test.rb
index 18aeb7d..a1d7b06 100644
--- a/test/controllers/rooms/directs_controller_test.rb
+++ b/test/controllers/rooms/directs_controller_test.rb
@@ -29,4 +29,34 @@ class Rooms::DirectsControllerTest < ActionDispatch::IntegrationTest
       assert_redirected_to root_url
     end
   end
+
+  test "destroy can't reach a closed room the member didn't create" do
+    sign_in :kevin
+
+    assert_no_difference -> { Room.count } do
+      delete rooms_direct_url(rooms(:designers))
+    end
+
+    assert rooms(:designers).reload.persisted?
+  end
+
+  test "destroy can't reach an open room the member didn't create" do
+    sign_in :kevin
+
+    assert_no_difference -> { Room.count } do
+      delete rooms_direct_url(rooms(:hq))
+    end
+
+    assert rooms(:hq).reload.persisted?
+  end
+
+  test "destroy can't reach a room the member isn't in at all" do
+    sign_in :jz
+
+    assert_no_difference -> { Room.count } do
+      delete rooms_direct_url(rooms(:david_and_kevin))
+    end
+
+    assert rooms(:david_and_kevin).reload.persisted?
+  end
 end
diff --git a/test/controllers/rooms/opens_controller_test.rb b/test/controllers/rooms/opens_controller_test.rb
index 0f91294..680c2e1 100644
--- a/test/controllers/rooms/opens_controller_test.rb
+++ b/test/controllers/rooms/opens_controller_test.rb
@@ -57,4 +57,23 @@ class Rooms::OpensControllerTest < ActionDispatch::IntegrationTest
     put rooms_open_url(rooms(:designers)), params: { room: { name: "Doesn't matter" } }
     assert_equal rooms(:designers).memberships.count, User.count
   end
+
+  test "a direct room can't be promoted to open by its creator" do
+    sign_in :kevin
+    direct = rooms(:bender_and_kevin)
+
+    put rooms_open_url(direct), params: { room: { name: "Watercooler" } }
+
+    assert_equal "Rooms::Direct", Room.find(direct.id).type
+    assert_equal [ users(:bender).id, users(:kevin).id ].sort, Room.find(direct.id).user_ids.sort
+  end
+
+  test "a direct room can't be promoted to open by an administrator either" do
+    direct = rooms(:david_and_kevin)
+
+    put rooms_open_url(direct), params: { room: { name: "Watercooler" } }
+
+    assert_equal "Rooms::Direct", Room.find(direct.id).type
+    assert_equal [ users(:david).id, users(:kevin).id ].sort, Room.find(direct.id).user_ids.sort
+  end
 end

From ee378092206e1b203c780b55d75b47f5f4a9334a Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Mon, 3 Aug 2026 14:55:12 -0700
Subject: [PATCH 2/3] Authorize the room message stream at subscribe time

Message content is delivered over turbo streams, which ran on the stock
Turbo::StreamsChannel. That channel verifies the signature on the stream name and
nothing else. The name carries no expiry and no binding to a user, so one read off
the page while a member kept working after the membership was revoked.

Revocation made this worse rather than better. Membership#after_destroy_commit
disconnects the user with reconnect: true, and the client replays its subscriptions
on the new socket: RoomChannel re-checks membership and rejects, while the turbo
subscription re-verified only the signature and was accepted.

RoomMessagesChannel re-checks membership on every subscribe, deriving the room from
the verified stream name so there is no parameter to point elsewhere. Since the
subscriber names the channel it wants, the stock channel would otherwise be a way
around that check, so it now turns these stream names away and this is the only door.
---
 .../concerns/room_streams_are_authorized.rb   | 13 +++
 app/channels/room_messages_channel.rb         | 55 ++++++++++++
 app/views/rooms/show.html.erb                 |  2 +-
 .../turbo_streams_authorization.rb            |  3 +
 test/channels/room_messages_channel_test.rb   | 86 +++++++++++++++++++
 5 files changed, 158 insertions(+), 1 deletion(-)
 create mode 100644 app/channels/concerns/room_streams_are_authorized.rb
 create mode 100644 app/channels/room_messages_channel.rb
 create mode 100644 config/initializers/turbo_streams_authorization.rb
 create mode 100644 test/channels/room_messages_channel_test.rb

diff --git a/app/channels/concerns/room_streams_are_authorized.rb b/app/channels/concerns/room_streams_are_authorized.rb
new file mode 100644
index 0000000..ad45270
--- /dev/null
+++ b/app/channels/concerns/room_streams_are_authorized.rb
@@ -0,0 +1,13 @@
+# Prepended onto Turbo::StreamsChannel. The subscriber names the channel it wants, so
+# authorizing room messages only in RoomMessagesChannel would leave the stock channel as
+# a way around it: same signed stream name, no membership check. Turn those names away
+# here and RoomMessagesChannel becomes the only door.
+module RoomStreamsAreAuthorized
+  def subscribed
+    if RoomMessagesChannel.guarded_stream?(verified_stream_name_from_params)
+      reject
+    else
+      super
+    end
+  end
+end
diff --git a/app/channels/room_messages_channel.rb b/app/channels/room_messages_channel.rb
new file mode 100644
index 0000000..1575a4f
--- /dev/null
+++ b/app/channels/room_messages_channel.rb
@@ -0,0 +1,55 @@
+# Authorizes the room message stream when the subscription is made, so that revoking a
+# membership actually stops delivery.
+#
+# Turbo's stock channel verifies only the signature on the stream name. That name carries
+# no expiry and no binding to a user, so one harvested while a member keeps working after
+# the membership is gone. Reconnecting makes it worse rather than better: revoking a
+# membership disconnects the user with reconnect: true, and the client then replays its
+# subscriptions on the fresh socket.
+#
+# The room is derived from the verified stream name rather than taken as a parameter, so
+# there's nothing for a subscriber to point somewhere else. The subscriber also doesn't
+# get to choose the channel: Turbo::StreamsChannel turns these stream names away, so this
+# is the only way onto them. See config/initializers/turbo_streams_authorization.rb.
+class RoomMessagesChannel < ApplicationCable::Channel
+  extend  Turbo::Streams::StreamName
+  include Turbo::Streams::StreamName::ClassMethods
+
+  STREAM_SUFFIX = "messages"
+
+  class << self
+    # True for the stream names this channel exists to guard, whoever is asking.
+    def guarded_stream?(stream_name)
+      stream_name.to_s.split(":", 2).second == STREAM_SUFFIX
+    end
+
+    def subscribable_room(user, stream_name)
+      gid_param, suffix = stream_name.to_s.split(":", 2)
+
+      if suffix == STREAM_SUFFIX && room = room_from(gid_param)
+        user.rooms.find_by(id: room.id)
+      end
+    end
+
+    private
+      def room_from(gid_param)
+        GlobalID::Locator.locate gid_param, only: Room
+      rescue ActiveRecord::RecordNotFound
+        nil
+      end
+  end
+
+  def subscribed
+    if stream_name = authorized_stream_name
+      stream_from stream_name
+    else
+      reject
+    end
+  end
+
+  private
+    def authorized_stream_name
+      stream_name = verified_stream_name_from_params
+      stream_name if stream_name.present? && self.class.subscribable_room(current_user, stream_name)
+    end
+end
diff --git a/app/views/rooms/show.html.erb b/app/views/rooms/show.html.erb
index 8f6bd07..ef76ecd 100644
--- a/app/views/rooms/show.html.erb
+++ b/app/views/rooms/show.html.erb
@@ -17,7 +17,7 @@
     <%= render partial: "messages/message", collection: @messages, cached: true %>
   <% end %>
 
-  <%= turbo_stream_from @room, :messages %>
+  <%= turbo_stream_from @room, :messages, channel: "RoomMessagesChannel" %>
   <%= button_to_jump_to_newest_message %>
 <% end %>
 
diff --git a/config/initializers/turbo_streams_authorization.rb b/config/initializers/turbo_streams_authorization.rb
new file mode 100644
index 0000000..9837ea6
--- /dev/null
+++ b/config/initializers/turbo_streams_authorization.rb
@@ -0,0 +1,3 @@
+Rails.application.config.to_prepare do
+  Turbo::StreamsChannel.prepend RoomStreamsAreAuthorized
+end
diff --git a/test/channels/room_messages_channel_test.rb b/test/channels/room_messages_channel_test.rb
new file mode 100644
index 0000000..9c2d112
--- /dev/null
+++ b/test/channels/room_messages_channel_test.rb
@@ -0,0 +1,86 @@
+require "test_helper"
+
+class RoomMessagesChannelTest < ActionCable::Channel::TestCase
+  tests RoomMessagesChannel
+
+  setup do
+    @room = rooms(:designers)
+    @signed_stream_name = Turbo::StreamsChannel.signed_stream_name [ @room, :messages ]
+  end
+
+  test "a member may subscribe to a room's message stream" do
+    stub_connection(current_user: users(:kevin))
+
+    subscribe signed_stream_name: @signed_stream_name
+
+    assert subscription.confirmed?
+    assert_has_stream Turbo.signed_stream_verifier.verified(@signed_stream_name)
+  end
+
+  test "a user who was never a member may not subscribe" do
+    stub_connection(current_user: users(:bender))
+
+    subscribe signed_stream_name: @signed_stream_name
+
+    assert subscription.rejected?
+  end
+
+  test "a revoked member may not re-subscribe with a stream name harvested while a member" do
+    stub_connection(current_user: users(:kevin))
+
+    subscribe signed_stream_name: @signed_stream_name
+    assert subscription.confirmed?, "kevin must start out able to subscribe"
+
+    @room.memberships.revoke_from users(:kevin)
+
+    subscribe signed_stream_name: @signed_stream_name
+
+    assert subscription.rejected?
+  end
+
+  test "an unsigned stream name is rejected" do
+    stub_connection(current_user: users(:kevin))
+
+    subscribe signed_stream_name: Turbo.signed_stream_verifier.verified(@signed_stream_name)
+
+    assert subscription.rejected?
+  end
+
+  test "a missing stream name is rejected" do
+    stub_connection(current_user: users(:kevin))
+
+    subscribe
+
+    assert subscription.rejected?
+  end
+
+  test "a validly signed stream name for another room the user isn't in is rejected" do
+    stub_connection(current_user: users(:bender))
+
+    subscribe signed_stream_name: Turbo::StreamsChannel.signed_stream_name([ rooms(:hq), :messages ])
+
+    assert subscription.rejected?
+  end
+end
+
+class RoomMessagesViaStockTurboChannelTest < ActionCable::Channel::TestCase
+  tests Turbo::StreamsChannel
+
+  # The subscriber picks the channel, so the stock channel has to turn these names away
+  # too. Otherwise a revoked member just names Turbo::StreamsChannel instead.
+  test "the stock turbo channel refuses to serve a room message stream" do
+    stub_connection(current_user: users(:kevin))
+
+    subscribe signed_stream_name: Turbo::StreamsChannel.signed_stream_name([ rooms(:designers), :messages ])
+
+    assert subscription.rejected?
+  end
+
+  test "the stock turbo channel still serves the room list stream" do
+    stub_connection(current_user: users(:kevin))
+
+    subscribe signed_stream_name: Turbo::StreamsChannel.signed_stream_name([ :rooms ])
+
+    assert subscription.confirmed?
+  end
+end

From 3b509f55caa13215b8983093d2016b623a04c9f5 Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Mon, 3 Aug 2026 14:55:19 -0700
Subject: [PATCH 3/3] Scope the unread rooms stream per user

UnreadRoomsChannel streamed from a hardcoded global name, and every message
published its room id to it. Any authenticated user, including one with no
memberships at all, could subscribe and watch which rooms were active and exactly
when, across every closed room and direct conversation on the account. The browser
filters ids it doesn't recognise, but that happens after delivery.

Its sibling ReadRoomsChannel is already scoped per user; this mirrors it, and
message creation fans the notice out to the room's members instead of broadcasting
it to everyone. No message content was exposed either way, only the timing.
---
 app/channels/unread_rooms_channel.rb         |  9 +++-
 app/models/message/broadcasts.rb             | 11 ++++-
 test/channels/unread_rooms_channel_test.rb   | 47 ++++++++++++++++++++
 test/controllers/messages_controller_test.rb | 19 ++++++--
 4 files changed, 81 insertions(+), 5 deletions(-)
 create mode 100644 test/channels/unread_rooms_channel_test.rb

diff --git a/app/channels/unread_rooms_channel.rb b/app/channels/unread_rooms_channel.rb
index f7fcc08..a3f6cc0 100644
--- a/app/channels/unread_rooms_channel.rb
+++ b/app/channels/unread_rooms_channel.rb
@@ -1,5 +1,12 @@
 class UnreadRoomsChannel < ApplicationCable::Channel
+  # Scoped per user, like ReadRoomsChannel. A single global stream would tell every
+  # authenticated connection when any room on the account is active, including closed
+  # rooms and direct conversations they're not part of.
+  def self.stream_name_for(user_id)
+    "user_#{user_id}_unreads"
+  end
+
   def subscribed
-    stream_from "unread_rooms"
+    stream_from self.class.stream_name_for(current_user.id)
   end
 end
diff --git a/app/models/message/broadcasts.rb b/app/models/message/broadcasts.rb
index 1f909dc..4623f2a 100644
--- a/app/models/message/broadcasts.rb
+++ b/app/models/message/broadcasts.rb
@@ -1,10 +1,19 @@
 module Message::Broadcasts
   def broadcast_create
     broadcast_append_to room, :messages, target: [ room, :messages ]
-    ActionCable.server.broadcast("unread_rooms", { roomId: room.id })
+    broadcast_unread_room
   end
 
   def broadcast_remove
     broadcast_remove_to room, :messages
   end
+
+  private
+    # Fanned out to the room's members rather than published on one global stream, so
+    # that the timing of activity in a room only reaches people who are in it.
+    def broadcast_unread_room
+      room.memberships.pluck(:user_id).each do |user_id|
+        ActionCable.server.broadcast UnreadRoomsChannel.stream_name_for(user_id), { roomId: room.id }
+      end
+    end
 end
diff --git a/test/channels/unread_rooms_channel_test.rb b/test/channels/unread_rooms_channel_test.rb
new file mode 100644
index 0000000..4444eb1
--- /dev/null
+++ b/test/channels/unread_rooms_channel_test.rb
@@ -0,0 +1,47 @@
+require "test_helper"
+
+class UnreadRoomsChannelTest < ActionCable::Channel::TestCase
+  test "streams only the subscriber's own unread stream" do
+    stub_connection(current_user: users(:jz))
+
+    subscribe
+
+    assert subscription.confirmed?
+    assert_has_stream "user_#{users(:jz).id}_unreads"
+    assert_not_includes subscription.streams, "unread_rooms"
+  end
+
+  test "an outsider is not told about activity in a room they can't see" do
+    direct = rooms(:bender_and_kevin)
+    assert_not direct.users.include?(users(:jz)), "jz must be an outsider for this test to mean anything"
+
+    broadcasts = capture_unread_broadcasts_for(users(:jz)) do
+      direct.messages.create!(body: "Private", creator: users(:kevin), client_message_id: "outsider").broadcast_create
+    end
+
+    assert_empty broadcasts
+  end
+
+  test "a member is told about activity in their own room" do
+    direct = rooms(:bender_and_kevin)
+
+    broadcasts = capture_unread_broadcasts_for(users(:kevin)) do
+      direct.messages.create!(body: "Private", creator: users(:bender), client_message_id: "member").broadcast_create
+    end
+
+    assert_equal [ direct.id ], broadcasts.collect { |broadcast| broadcast["roomId"] }
+  end
+
+  private
+    def capture_unread_broadcasts_for(user)
+      stub_connection(current_user: user)
+      subscribe
+
+      stream = subscription.streams.sole
+      before = ActionCable.server.pubsub.broadcasts(stream).size
+
+      yield
+
+      ActionCable.server.pubsub.broadcasts(stream).drop(before).collect { |broadcast| JSON.parse(broadcast) }
+    end
+end
diff --git a/test/controllers/messages_controller_test.rb b/test/controllers/messages_controller_test.rb
index 1f556bf..8088694 100644
--- a/test/controllers/messages_controller_test.rb
+++ b/test/controllers/messages_controller_test.rb
@@ -57,9 +57,22 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
     end
   end
 
-  test "creating a message broadcasts unread room" do
-    assert_broadcasts "unread_rooms", 1 do
-      post room_messages_url(@room, format: :turbo_stream), params: { message: { body: "New one", client_message_id: 999 } }
+  test "creating a message broadcasts unread room to each member" do
+    @room.users.each do |member|
+      assert_broadcasts UnreadRoomsChannel.stream_name_for(member.id), 1 do
+        post room_messages_url(@room, format: :turbo_stream), params: { message: { body: "New one #{member.id}", client_message_id: member.id } }
+      end
+    end
+  end
+
+  test "creating a message doesn't broadcast unread room to non-members" do
+    outsiders = User.where.not(id: @room.users.map(&:id))
+    assert outsiders.any?, "need someone outside the room for this test to mean anything"
+
+    outsiders.each do |outsider|
+      assert_no_broadcasts UnreadRoomsChannel.stream_name_for(outsider.id) do
+        post room_messages_url(@room, format: :turbo_stream), params: { message: { body: "New one", client_message_id: 999 } }
+      end
     end
   end