Keep the CSP nonce cookie off public cacheable responses

The nonce generator runs for every response that carries the policy, so
the nonce_id cookie was set on QR codes, logos, avatars and disk-served
blobs — all public, shared-cacheable. Those endpoints now opt out of the
policy, so no cache can replay one client's identifier and nonce.

Dockerfile

@@ -66,6 +66,9 @@ ENV HTTP_WRITE_TIMEOUT=300
 COPY --from=build --chown=rails:rails /usr/local/bundle /usr/local/bundle
 COPY --from=build --chown=rails:rails /rails /rails
 
+# Install ONCE backup/restore hooks
+COPY --chmod=755 hooks /hooks
+
 # Set version and revision
 ARG APP_VERSION
 ENV APP_VERSION=$APP_VERSION

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

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

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

app/controllers/accounts/logos_controller.rb

@@ -2,6 +2,7 @@ class Accounts::LogosController < ApplicationController
   include ActiveStorage::Streaming, ActionView::Helpers::AssetUrlHelper
 
   allow_unauthenticated_access only: :show
+  content_security_policy false, only: :show
   before_action :ensure_can_administer, only: :destroy
 
   def show

app/controllers/qr_code_controller.rb

@@ -1,5 +1,6 @@
 class QrCodeController < ApplicationController
   allow_unauthenticated_access
+  content_security_policy false
 
   def show
     url = Base64.urlsafe_decode64(params[:id])

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

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

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

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

app/controllers/users/avatars_controller.rb

@@ -1,6 +1,8 @@
 class Users::AvatarsController < ApplicationController
   include ActiveStorage::Streaming
 
+  content_security_policy false, only: :show
+
   rescue_from(ActiveSupport::MessageVerifier::InvalidSignature) { head :not_found }
 
   def show

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

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

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 %>
 

bin/release

@@ -15,6 +15,7 @@ include SSHKit::DSL
 ARCH = "linux/amd64,linux/arm64"
 BUILDER_NAME = "campfire-builder"
 GIT_SHA = `git rev-parse HEAD`.strip
+MANUAL_NOTES = ARGV.delete("--notes")
 VERSION = ARGV[0]
 
 DOCKER_IMAGE_NAME = "registry.once.com/campfire"
@@ -28,7 +29,7 @@ EXPORT_REMOTE_USER = "app"
 EXPORT_DEPLOY_HOSTS = %w[ once-store-app-101 once-store-app-102 ]
 EXPORT_DEPLOY_LOCATION = "/local/app/product_releases/"
 
-abort "You must specify a version, e.g. `bin/release 1.0.0`" unless VERSION
+abort "You must specify a version, e.g. `bin/release 1.0.0` (pass --notes to write release notes manually)" unless VERSION
 
 current_branch = `git rev-parse --abbrev-ref HEAD`.strip
 if current_branch != "main"
@@ -57,19 +58,25 @@ on(:local) do
 end
 
 announcing "Creating GitHub release v#{VERSION}..." do
-  notes_file = "tmp/release_notes.md"
-
   on(:local) do
-    execute :mkdir, "-p", "tmp"
+    if MANUAL_NOTES
+      notes_file = "tmp/release_notes.md"
 
-    File.write(notes_file, "# What's Changed\n\n- \n")
-    system ENV["EDITOR"] || "vim", notes_file
+      execute :mkdir, "-p", "tmp"
 
-    unless system "gh", "release", "create", "v#{VERSION}", "--title", "v#{VERSION}", "--notes-file", notes_file
-      abort "Failed to create release; check that the version doesn't already exist"
+      File.write(notes_file, "# What's Changed\n\n- \n")
+      system ENV["EDITOR"] || "vim", notes_file
+
+      unless system "gh", "release", "create", "v#{VERSION}", "--title", "v#{VERSION}", "--notes-file", notes_file
+        abort "Failed to create release; check that the version doesn't already exist"
+      end
+
+      execute :rm, notes_file
+    else
+      unless system "gh", "release", "create", "v#{VERSION}", "--title", "v#{VERSION}", "--generate-notes"
+        abort "Failed to create release; check that the version doesn't already exist"
+      end
     end
-
-    execute :rm, notes_file
   end
 end
 

config/initializers/active_storage.rb

@@ -1,4 +1,6 @@
 ActiveSupport.on_load(:active_storage_blob) do
+  ActiveStorage::DiskController.content_security_policy false, only: :show
+
   ActiveStorage::DiskController.after_action only: :show do
     response.set_header("Cache-Control", "max-age=3600, public")
   end

config/initializers/content_security_policy.rb

@@ -1,25 +1,120 @@
 # Be sure to restart your server when you modify this file.
 
-# Define an application-wide content security policy.
+# Baseline application-wide Content-Security-Policy, deployed in Report-Only
+# mode: browsers evaluate the policy and report violations (once a report
+# endpoint is wired up) but enforce nothing, so rendering cannot break.
+# Tune the policy against observed violations, then flip
+# `content_security_policy_report_only` off to enforce it.
+#
 # See the Securing Rails Applications Guide for more information:
 # https://guides.rubyonrails.org/security.html#content-security-policy-header
 
-# Rails.application.configure do
-#   config.content_security_policy do |policy|
-#     policy.default_src :self, :https
-#     policy.font_src    :self, :https, :data
-#     policy.img_src     :self, :https, :data
-#     policy.object_src  :none
-#     policy.script_src  :self, :https
-#     policy.style_src   :self, :https
-#     # Specify URI for violation reports
-#     # policy.report_uri "/csp-violation-report-endpoint"
-#   end
+# Session-stable nonce for permitted inline scripts (the importmap JSON + shim,
+# auto-nonced by importmap-rails).
 #
-#   # Generate session nonces for permitted importmap and inline scripts
-#   config.content_security_policy_nonce_generator = ->(request) { request.session.id.to_s }
-#   config.content_security_policy_nonce_directives = %w(script-src)
+# The nonce is stable across a session's requests so Turbo snapshot restores
+# don't replay a stale nonce and trip CSP. It's the HMAC of a stable cookie
+# value keyed by the server secret: the cookie is client-settable, but the
+# client can't predict the resulting nonce without knowing secret_key_base.
 #
-#   # Report violations without enforcing the policy.
-#   # config.content_security_policy_report_only = true
-# end
+# Campfire sets no per-session verification cookie, so the lightweight
+# nonce_id cookie — set on first visit, present for every session including
+# unauthenticated ones — is the sole identifier.
+#
+# The generator runs for every response that carries the policy, so the cookie
+# write would otherwise land on public, shared-cacheable responses (QR codes,
+# logos, avatars, disk-served blobs) where a cache could replay one client's
+# identifier and nonce to others. Those endpoints opt out of the policy with
+# `content_security_policy false`; the nonce is only meaningful on documents.
+module CSP
+  module Nonce
+    COOKIE = "campfire_csp_nonce_id"
+
+    def self.generate(request)
+      hmac(nonce_id(request))
+    end
+
+    def self.hmac(identifier)
+      OpenSSL::HMAC.hexdigest("SHA256", Rails.application.secret_key_base, identifier)
+    end
+
+    # Read or initialize a stable nonce identifier cookie.
+    def self.nonce_id(request)
+      request.cookies[COOKIE] || set_nonce_id(request)
+    end
+
+    def self.set_nonce_id(request)
+      value = SecureRandom.base64(16)
+      request.cookie_jar[COOKIE] = { value: value, httponly: true, same_site: :lax }
+      value
+    end
+  end
+
+  # Per-install CSP extras.
+  #
+  # Campfire is a ONCE product: each customer self-hosts it on their own domain
+  # and an admin may embed or connect to external hosts — video/embed providers,
+  # image CDNs, analytics, form or webhook endpoints — that vary per install and
+  # are unknown at build time. `:self` already tracks this install's own origin;
+  # these ENV knobs let an admin allow additional hosts without editing this file
+  # (and without which enforcement would break their legitimate integrations).
+  #
+  # Each is a comma-, semicolon-, or whitespace-separated list of CSP source
+  # expressions, e.g.
+  #
+  #   CSP_EXTRA_FRAME_SRC="https://www.youtube.com https://player.vimeo.com"
+  #
+  # Leave them unset (the default) to keep each directive at :self only.
+  EXTRA_ENV = {
+    script_src:  "CSP_EXTRA_SCRIPT_SRC",
+    style_src:   "CSP_EXTRA_STYLE_SRC",
+    img_src:     "CSP_EXTRA_IMG_SRC",
+    connect_src: "CSP_EXTRA_CONNECT_SRC",
+    frame_src:   "CSP_EXTRA_FRAME_SRC",
+    form_action: "CSP_EXTRA_FORM_ACTION"
+  }.freeze
+
+  # Parse one ENV knob into a list of extra host sources.
+  #
+  # Semicolons are tokenized like commas/whitespace: because the nonce forces a
+  # per-request policy build, a stray `;` in a value (a plausible operator paste,
+  # e.g. "https://youtube.com; https://vimeo.com") would otherwise land inside a
+  # single source token and make Rails raise InvalidDirectiveError on every
+  # request — a site-wide 500 even in report-only mode. Splitting on `;` yields
+  # valid tokens instead; Rails still validates each one, so no injection is
+  # introduced (a token with an embedded space still fails validation).
+  def self.extra(directive)
+    ENV[EXTRA_ENV.fetch(directive)].to_s.split(/[,;\s]+/).reject(&:blank?)
+  end
+
+  # Build the baseline policy. Kept as a reusable method so it can be exercised
+  # in isolation by tests as well as at boot.
+  def self.apply(policy)
+    policy.default_src     :self
+    policy.script_src      :self, *extra(:script_src)  # nonce auto-appended via nonce_directives below
+    # unsafe_inline retained: many style="…" attributes across the message
+    # composer and ActionText content can't be nonced yet.
+    policy.style_src       :self, :unsafe_inline, *extra(:style_src)
+    # frame_src / img_src / connect_src start at :self plus any per-install extras
+    # and get tuned against violation reports during the report-only window.
+    policy.img_src         :self, :data, :blob, *extra(:img_src)
+    policy.connect_src     :self, *extra(:connect_src)
+    policy.frame_src       :self, *extra(:frame_src)
+    policy.frame_ancestors :self
+    policy.base_uri        :self
+    policy.form_action     :self, *extra(:form_action)
+    policy.object_src      :none
+    # Specify URI for violation reports once a report sink is available
+    # policy.report_uri "/csp-violation-report-endpoint"
+  end
+end
+
+Rails.application.configure do
+  config.content_security_policy { |policy| CSP.apply(policy) }
+
+  config.content_security_policy_nonce_generator = ->(request) { CSP::Nonce.generate(request) }
+  config.content_security_policy_nonce_directives = %w[ script-src ]
+
+  # Report violations without enforcing the policy.
+  config.content_security_policy_report_only = true
+end

config/initializers/turbo_streams_authorization.rb

@@ -0,0 +1,3 @@
+Rails.application.config.to_prepare do
+  Turbo::StreamsChannel.prepend RoomStreamsAreAuthorized
+end

hooks/post-restore

@@ -0,0 +1,12 @@
+#!/bin/bash
+set -euo pipefail
+
+db_name="${RAILS_ENV:-production}.sqlite3"
+snapshot="/rails/storage/backups/$db_name"
+db="/rails/storage/db/$db_name"
+
+if [ -f "$snapshot" ]; then
+  mkdir -p "$(dirname "$db")"
+  cp "$snapshot" "$db"
+  rm -f "$db-wal" "$db-shm"
+fi

hooks/pre-backup

@@ -0,0 +1,2 @@
+#!/bin/bash
+exec /rails/script/admin/prepare-backup

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

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

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
 

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 ] })

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

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

test/integration/csp_nonce_test.rb

@@ -0,0 +1,128 @@
+require "test_helper"
+
+class CspNonceTest < ActionDispatch::IntegrationTest
+  test "policy is delivered Report-Only, not enforced" do
+    sign_in :david
+    get root_url
+    follow_redirect!  # root redirects to the last room
+
+    assert_response :success
+    assert response.headers["Content-Security-Policy-Report-Only"].present?,
+      "Expected a Report-Only CSP header"
+    assert_nil response.headers["Content-Security-Policy"],
+      "Policy must not be enforced yet"
+  end
+
+  test "nonce is stable across requests so Turbo restores don't trip CSP" do
+    sign_in :david
+
+    get root_url
+    nonce1 = report_only_nonce
+
+    get root_url
+    nonce2 = report_only_nonce
+
+    assert nonce1.present?, "Expected a nonce in the Report-Only CSP header"
+    assert_equal nonce1, nonce2, "Nonce must be stable across requests"
+  end
+
+  test "client-set identifier still yields an unpredictable HMAC nonce" do
+    fake_id = "attacker-controlled-value"
+    cookies[CSP::Nonce::COOKIE] = fake_id
+
+    get root_url
+    nonce = report_only_nonce
+
+    assert_equal CSP::Nonce.hmac(fake_id), nonce,
+      "Nonce must be HMAC-SHA256 of the identifier keyed by secret_key_base"
+  end
+
+  test "importmap script tag carries the nonce" do
+    sign_in :david
+    get root_url
+    follow_redirect!  # root redirects to the last room
+
+    assert_response :success
+    nonce = report_only_nonce
+    assert_select "script[type='importmap'][nonce=?]", nonce
+  end
+
+  test "a per-install ENV extra host is appended to its directive" do
+    with_env "CSP_EXTRA_FRAME_SRC" => "https://player.vimeo.com https://www.youtube.com",
+             "CSP_EXTRA_IMG_SRC"   => "https://cdn.example.test" do
+      header = ActionDispatch::ContentSecurityPolicy.new { |p| CSP.apply(p) }.build
+
+      assert_match %r{frame-src[^;]*\bhttps://player\.vimeo\.com\b}, header
+      assert_match %r{frame-src[^;]*\bhttps://www\.youtube\.com\b}, header
+      assert_match %r{img-src[^;]*\bhttps://cdn\.example\.test\b}, header
+      # :self is preserved alongside the extras.
+      assert_match %r{frame-src 'self'}, header
+    end
+  end
+
+  test "a semicolon-separated ENV extra tokenizes into valid sources without raising" do
+    # A plausible operator paste separates hosts with "; ". Because the nonce
+    # forces a per-request policy build, a semicolon left inside a single source
+    # token would make Rails raise InvalidDirectiveError on every request — a
+    # site-wide 500 even in report-only mode. Splitting on ';' must yield both
+    # hosts as valid tokens and never raise.
+    with_env "CSP_EXTRA_FRAME_SRC" => "https://a.example; https://b.example" do
+      header = nil
+      assert_nothing_raised do
+        header = ActionDispatch::ContentSecurityPolicy.new { |p| CSP.apply(p) }.build
+      end
+
+      assert_match %r{frame-src[^;]*\bhttps://a\.example\b}, header
+      assert_match %r{frame-src[^;]*\bhttps://b\.example\b}, header
+      # Both hosts share the one frame-src directive; the semicolon did not leak
+      # a second directive into the policy.
+      assert_equal 1, header.scan(/(?:^|;\s*)frame-src\b/).size,
+        "Expected exactly one frame-src directive"
+    end
+  end
+
+  test "public cacheable responses carry neither the policy nor the nonce cookie" do
+    sign_in :david
+    blob = ActiveStorage::Blob.create_and_upload!(io: file_fixture("moon.jpg").open, filename: "moon.jpg", content_type: "image/jpeg")
+    get rails_blob_path(blob)
+    disk_blob_url = response.location
+
+    [
+      qr_code_path(Base64.urlsafe_encode64("http://example.com")),
+      account_logo_path,
+      user_avatar_path(users(:kevin).avatar_token),
+      disk_blob_url
+    ].each do |path|
+      cookies.delete CSP::Nonce::COOKIE
+
+      get path
+
+      assert_response :success
+      assert response.cache_control[:public], "#{path} is expected to be publicly cacheable"
+      assert_nil response.headers["Content-Security-Policy-Report-Only"], "#{path} must not carry the policy"
+      assert_nil cookies[CSP::Nonce::COOKIE], "#{path} must not set the nonce cookie"
+    end
+  end
+
+  test "directives default to :self only when no ENV extras are set" do
+    with_env "CSP_EXTRA_FRAME_SRC" => nil, "CSP_EXTRA_IMG_SRC" => nil do
+      header = ActionDispatch::ContentSecurityPolicy.new { |p| CSP.apply(p) }.build
+
+      assert_match %r{frame-src 'self'(;|\z)}, header
+    end
+  end
+
+  private
+    def with_env(vars)
+      original = {}
+      vars.each_key { |k| original[k] = ENV.key?(k) ? ENV[k] : :__unset__ }
+      vars.each { |k, v| v.nil? ? ENV.delete(k) : ENV[k] = v }
+      yield
+    ensure
+      original.each { |k, v| v == :__unset__ ? ENV.delete(k) : ENV[k] = v }
+    end
+
+    def report_only_nonce
+      response.headers["Content-Security-Policy-Report-Only"].to_s[/'nonce-([^']+)'/, 1]
+    end
+end