Navigate explicitly in custom_styles XSS system test

The test relied on the post-login landing page implicitly, so the
custom_styles <link> coverage could silently evaporate if that landing
changes. Visit root_url explicitly (matching the writebook sibling) to pin
the page under test. Assertions are unchanged and still non-vacuous.

app/controllers/accounts/custom_styles_controller.rb

@@ -1,5 +1,14 @@
 class Accounts::CustomStylesController < ApplicationController
-  before_action :ensure_can_administer, :set_account
+  allow_unauthenticated_access only: :show
+  before_action :ensure_can_administer, :set_account, except: :show
+
+  def show
+    if stale?(etag: Current.account)
+      expires_in 5.minutes, public: true, stale_while_revalidate: 1.week
+
+      render plain: Current.account&.custom_styles.to_s, content_type: "text/css"
+    end
+  end
 
   def edit
   end

app/controllers/accounts/logos_controller.rb

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

app/controllers/users/avatars_controller.rb

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

app/helpers/application_helper.rb

@@ -12,12 +12,6 @@ module ApplicationHelper
     end
   end
 
-  def custom_styles_tag
-    if custom_styles = Current.account&.custom_styles
-      tag.style(custom_styles.to_s.html_safe, data: { turbo_track: "reload" })
-    end
-  end
-
   def body_classes
     [ @body_class, admin_body_class, account_logo_body_class ].compact.join(" ")
   end

app/views/layouts/application.html.erb

@@ -22,7 +22,7 @@
     <%= tag.link rel: "apple-touch-icon", href: fresh_account_logo_path %>
 
     <%= stylesheet_link_tag :all, "data-turbo-track": "reload" %>
-    <%= custom_styles_tag %>
+    <%= tag.link rel: "stylesheet", href: fresh_custom_styles_path, data: { turbo_track: "reload" } %>
 
     <%= javascript_importmap_tags %>
 

config/initializers/active_storage.rb

@@ -1,6 +1,4 @@
 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,120 +1,25 @@
 # Be sure to restart your server when you modify this file.
 
-# 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.
-#
+# Define an application-wide content security policy.
 # See the Securing Rails Applications Guide for more information:
 # https://guides.rubyonrails.org/security.html#content-security-policy-header
 
-# Session-stable nonce for permitted inline scripts (the importmap JSON + shim,
-# auto-nonced by importmap-rails).
+# 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
 #
-# 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.
+#   # 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)
 #
-# 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
+#   # Report violations without enforcing the policy.
+#   # config.content_security_policy_report_only = true
+# end

config/routes.rb

@@ -21,7 +21,7 @@ Rails.application.routes.draw do
 
       resource :join_code, only: :create
       resource :logo, only: %i[ show destroy ]
-      resource :custom_styles, only: %i[ edit update ]
+      resource :custom_styles, only: %i[ show edit update ]
     end
   end
 
@@ -29,6 +29,10 @@ Rails.application.routes.draw do
     route_for :account_logo, v: Current.account&.updated_at&.to_fs(:number), size: options[:size]
   end
 
+  direct :fresh_custom_styles do |options|
+    route_for :account_custom_styles, v: Current.account&.updated_at&.to_fs(:epoch)
+  end
+
   get "join/:join_code", to: "users#new", as: :join
   post "join/:join_code", to: "users#create"
 

test/controllers/accounts/custom_styles_controller_test.rb

@@ -5,6 +5,34 @@ class Accounts::CustomStylesControllerTest < ActionDispatch::IntegrationTest
     sign_in :david
   end
 
+  test "show serves custom styles as plain CSS" do
+    accounts(:signal).update! custom_styles: ":root { --color-text: red; }"
+
+    get account_custom_styles_url
+    assert_response :ok
+    assert_equal "text/css", @response.media_type
+    assert_equal ":root { --color-text: red; }", @response.body
+  end
+
+  test "show is accessible without authentication" do
+    accounts(:signal).update! custom_styles: ":root { --color-text: red; }"
+    reset!
+
+    get account_custom_styles_url
+    assert_response :ok
+    assert_equal "text/css", @response.media_type
+  end
+
+  test "show serves markup verbatim as inert CSS text, never HTML" do
+    payload = "</style><script>alert(1)</script>"
+    accounts(:signal).update! custom_styles: payload
+
+    get account_custom_styles_url
+    assert_response :ok
+    assert_equal "text/css", @response.media_type
+    assert_equal payload, @response.body
+  end
+
   test "edit" do
     get edit_account_custom_styles_url
     assert_response :ok

test/integration/csp_nonce_test.rb

@@ -1,128 +0,0 @@
-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

test/system/custom_styles_xss_test.rb

@@ -0,0 +1,44 @@
+require "application_system_test_case"
+
+class CustomStylesXssTest < ApplicationSystemTestCase
+  # A stored payload that breaks out of an inline <style> context and executes
+  # as HTML. Rendered inline (the old behavior) this <script> runs and sets the
+  # flag; delivered as an external text/css stylesheet it is inert text.
+  PAYLOAD = %(</style><script>window.__xss_fired = true</script>)
+
+  setup do
+    accounts(:signal).update!(custom_styles: PAYLOAD)
+    sign_in "jz@37signals.com"
+  end
+
+  test "custom styles payload loads as a stylesheet and never executes" do
+    # Navigate to the page under test explicitly (matching the writebook
+    # sibling) rather than leaning on the post-login landing page — otherwise
+    # this coverage could silently evaporate if that landing changes.
+    visit root_url
+
+    # (a) custom styles arrive via an external stylesheet link, not inline markup
+    assert_selector "link[rel='stylesheet'][href*='custom_styles']", visible: false
+
+    # (b) the payload's <script> never executed — no breakout from CSS context
+    assert_nil evaluate_script("window.__xss_fired")
+
+    # (b') and it injected no live <script> element into the document
+    assert_no_selector "script", text: "__xss_fired", visible: false
+
+    # (c) the browser fetched the payload verbatim as text/css, so it is styled,
+    #     never parsed as HTML
+    fetched = page.evaluate_async_script(<<~JS)
+      var done = arguments[arguments.length - 1];
+      var href = document.querySelector("link[rel='stylesheet'][href*='custom_styles']").href;
+      fetch(href).then(function(r) {
+        return r.text().then(function(body) {
+          done({ type: r.headers.get("content-type"), body: body });
+        });
+      });
+    JS
+
+    assert_includes fetched["type"], "text/css"
+    assert_includes fetched["body"], PAYLOAD
+  end
+end