From 0f5d0b2b6e3b79fe47ee7b32dbccb75d6ea663a7 Mon Sep 17 00:00:00 2001 From: GPT on behalf of DHH <2741+dhh@users.noreply.github.com> Date: Thu, 8 Oct 2026 09:05:10 +0200 Subject: [PATCH] Use header-only forgery protection and cache complete responses --- README.md | 10 ++ app/controllers/concerns/authentication.rb | 2 +- app/controllers/concerns/cached_responses.rb | 48 +++++----- app/controllers/messages_controller.rb | 3 +- app/helpers/application_helper.rb | 5 + app/javascript/models/file_uploader.js | 1 - app/views/layouts/application.html.erb | 1 - .../active_storage_authentication.rb | 1 + config/initializers/session_store.rb | 2 +- test/controllers/cached_responses_test.rb | 51 +++++++---- test/controllers/fetch_metadata_test.rb | 91 +++++++++++++++++++ test/performance/chatter.js | 8 +- 12 files changed, 169 insertions(+), 54 deletions(-) create mode 100644 test/controllers/fetch_metadata_test.rb diff --git a/README.md b/README.md index 1d4bfb6..ba38172 100644 --- a/README.md +++ b/README.md @@ -80,3 +80,13 @@ Please see our [development guide](docs/development.md) for how to get Campfire ## Security See [SECURITY.md](SECURITY.md) for how to report a vulnerability and a description of our trust model. + +## Request protection + +Browser writes use Rails' `Sec-Fetch-Site` header-only forgery protection and its +`Origin` check. HTTPS requires modern browser metadata; plain HTTP retains the +missing-header fallback with the existing `SameSite=Lax` cookies. Authenticated +bot APIs and signed disk-upload capabilities keep their existing exemptions. +Forms contain no CSRF tokens. Authenticated page caches reuse complete HTML and +gzip bodies while checking current sessions, permissions and SQLite changes. +Existing installation cookies remain valid, including old token-bearing cookies. diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index e609d33..4a1d70c 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -7,7 +7,7 @@ module Authentication before_action :deny_bots helper_method :signed_in? - protect_from_forgery with: :exception, unless: -> { authenticated_by.bot_key? } + protect_from_forgery using: :header_only, with: :exception, unless: -> { authenticated_by.bot_key? } end class_methods do diff --git a/app/controllers/concerns/cached_responses.rb b/app/controllers/concerns/cached_responses.rb index da4eb1f..f25cf6a 100644 --- a/app/controllers/concerns/cached_responses.rb +++ b/app/controllers/concerns/cached_responses.rb @@ -1,8 +1,9 @@ +require "zlib" + module CachedResponses extend ActiveSupport::Concern - CACHE_HEADERS = %w[ content-type cache-control etag last-modified vary ].freeze - CSRF_TAG = /]*\bname="csrf-token"[^>]*>|]*\bname="authenticity_token"[^>]*>/ + CACHE_HEADERS = %w[ content-type content-encoding cache-control etag last-modified vary ].freeze included do prepend_before_action :capture_response_cache_version @@ -27,8 +28,7 @@ module CachedResponses def combined_fragment_cache_key(key) @fragment_cache_namespace ||= [ @response_cache_version, request.base_url, request.script_name, request.format.to_s, I18n.locale, - Current.user&.id, (Digest::SHA256.hexdigest(Current.session.token) if Current.session), - (Digest::SHA256.hexdigest(real_csrf_token) if request.format.html? || request.format.turbo_stream?) + Current.user&.id, (Digest::SHA256.hexdigest(Current.session.token) if Current.session) ].freeze super([ @fragment_cache_namespace, key ]) end @@ -43,8 +43,10 @@ module CachedResponses # Register after room authorization, but before presentation queries. def cache_read_response if cacheable_read_request? - token = form_authenticity_token - key = response_cache_key + encoding = Rack::Utils.select_best_encoding(%w[ gzip identity ], Rack::Utils.q_values(request.headers["Accept-Encoding"])) + return yield unless encoding + + key = response_cache_key(encoding) original_session = session.to_hash.deep_dup return yield if key.bytesize > ResponseCache::MAX_KEY_BYTES @@ -57,14 +59,15 @@ module CachedResponses if !entry && ResponseCache.instance.version == @response_cache_version yield rendered = true - cache_completed_response(key, original_session) + entry = cache_completed_response(key, original_session, encoding) end end end if entry response.headers.merge!(entry[:headers]) - self.response_body = entry[:body].gsub(entry[:marker], token) + response.headers.delete("Content-Length") + self.response_body = entry[:body] elsif !rendered # A queued request retains its pre-auth snapshot. If it has expired, # render outside the stripe instead of blocking the next generation. @@ -75,14 +78,15 @@ module CachedResponses end end - def cache_completed_response(key, original_session) - if response.status == 200 && response.media_type == "text/html" && session.to_hash == original_session - marker = "campfire-csrf-#{SecureRandom.hex(32)}" - # Replace only framework token attributes, never a matching token in - # message text. Postprocessing also leaves fragment caches untouched. - body = csrf_neutral_body(response.body, marker) + def cache_completed_response(key, original_session, encoding) + if response.status == 200 && response.media_type == "text/html" && session.to_hash == original_session && !response.headers["Content-Encoding"] + body = encoding == "gzip" ? Zlib.gzip(response.body) : response.body + response.headers["Content-Encoding"] = "gzip" if encoding == "gzip" + response.headers["Vary"] = (response.headers["Vary"].to_s.split(/,\s*/) | [ "Accept-Encoding" ]).join(", ") headers = response.headers.slice(*CACHE_HEADERS).to_h.freeze - ResponseCache.instance.write(key, @response_cache_version, { body: body.freeze, marker: marker.freeze, headers: headers }.freeze) + entry = { body: body.freeze, headers: headers }.freeze + ResponseCache.instance.write(key, @response_cache_version, entry) + entry end end @@ -94,21 +98,13 @@ module CachedResponses !ActiveRecord::Base.connection.transaction_open? end - def response_cache_key + def response_cache_key(encoding) ActiveSupport::JSON.encode([ - controller_path, request.fullpath, request.base_url, request.user_agent, + controller_path, request.fullpath, request.base_url, request.user_agent, encoding, request.headers["Accept"], request.headers["Turbo-Frame"], I18n.locale, - # Tokens are hydrated per request, including clients that replay an old - # cookie. Their raw CSRF secret does not select a presentation variant. + # Old token-bearing installation cookies remain valid without affecting HTML. Current.user.id, Current.session.token, session.to_hash.except("_csrf_token"), cookies.to_h.except("_campfire_session", "session_token") ]) end - - def csrf_neutral_body(body, marker) - body.gsub(CSRF_TAG) do |tag| - attribute = tag.start_with?(" { diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 23a66c5..7e2f73c 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -9,7 +9,6 @@ - <%= csrf_meta_tags %> <%= csp_meta_tag %> <%= current_user_meta_tags %> <%= script_aware_action_cable_meta_tag %> diff --git a/config/initializers/active_storage_authentication.rb b/config/initializers/active_storage_authentication.rb index 40b1a7e..31e414e 100644 --- a/config/initializers/active_storage_authentication.rb +++ b/config/initializers/active_storage_authentication.rb @@ -7,6 +7,7 @@ # can read the public login page. Require a valid Campfire session before an # anonymous caller can allocate a Blob or persist bytes to disk. Rails.application.config.to_prepare do + ActiveStorage::BaseController.forgery_protection_verification_strategy = :header_only ActiveStorage::DirectUploadsController.include ActiveStorageAuthentication ActiveStorage::DirectUploadsController.before_action :require_active_storage_authentication diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb index 28e0d6b..ae2fddc 100644 --- a/config/initializers/session_store.rb +++ b/config/initializers/session_store.rb @@ -1,4 +1,4 @@ Rails.application.config.session_store :cookie_store, key: "_campfire_session", - # Persist session cookie as permament so re-opened browser windows maintain a CSRF token + # Preserve the existing installation's persistent browser-session cookie. expire_after: 20.years diff --git a/test/controllers/cached_responses_test.rb b/test/controllers/cached_responses_test.rb index b481454..8968aa0 100644 --- a/test/controllers/cached_responses_test.rb +++ b/test/controllers/cached_responses_test.rb @@ -14,7 +14,7 @@ class CachedResponsesTest < ActionDispatch::IntegrationTest Rails.cache = ActiveSupport::Cache::MemoryStore.new ActionController::Base.perform_caching = true @room = rooms(:watercooler) - # Establish last-room and CSRF cookies before checking reuse. + # Establish last-room cookies before checking reuse. 2.times { get room_url(@room) } ResponseCache.instance.clear end @@ -48,37 +48,54 @@ class CachedResponsesTest < ActionDispatch::IntegrationTest assert_response :success end - test "clients without a persisted CSRF session still reuse token-neutral HTML" do + test "clients without token state reuse complete HTML and post without tokens" do cookies["_campfire_session"] = @login_cookie get room_url(@room) - first = css_select('meta[name="csrf-token"]').first["content"] + first = response.body + assert_select 'meta[name="csrf-token"]', count: 0 + assert_select 'input[name="authenticity_token"]', count: 0 cookies["_campfire_session"] = @login_cookie ResponseCache.instance.expects(:write).never get room_url(@room) - second = css_select('meta[name="csrf-token"]').first["content"] assert_response :success - assert_not_equal first, second - assert_no_match /campfire-csrf-/, response.body + assert_equal first, response.body post room_messages_url(@room, format: :turbo_stream), params: { - authenticity_token: second, message: { body: "fresh replay token works", client_message_id: "cache-replay-token" } } + message: { body: "tokenless replay works", client_message_id: "cache-replay" } }, + headers: { "Sec-Fetch-Site" => "same-origin", "Origin" => "http://once.campfire.test" } assert_response :success end - test "cached tokens stay fresh and literal token-like text survives" do - get room_url(@room) - literal = css_select('meta[name="csrf-token"]').first["content"] + test "literal token-like text survives complete page reuse" do + literal = "campfire-csrf-literal authenticity_token csrf-token" @room.messages.create!(creator: users(:david), body: "literal #{literal}") get room_url(@room) - first = css_select('meta[name="csrf-token"]').first["content"] + first = response.body get room_url(@room) - second = css_select('meta[name="csrf-token"]').first["content"] - assert_not_equal first, second + assert_equal first, response.body assert_includes response.body, "literal #{literal}" - assert_no_match /campfire-csrf-/, response.body + end - post room_messages_url(@room, format: :turbo_stream), params: { - authenticity_token: second, message: { body: "cached token works", client_message_id: "cache-token" } } - assert_response :success + test "gzip bytes are reused and identity negotiation stays separate" do + require "stringio" + get room_url(@room), headers: { "Accept-Encoding" => "gzip" } + assert_equal "gzip", response.headers["Content-Encoding"] + encoded = response.body + decoded = Zlib::GzipReader.new(StringIO.new(encoded)).read + assert_includes decoded, " "gzip" } + assert_equal encoded, response.body + get room_url(@room) + assert_nil response.headers["Content-Encoding"] + assert_equal decoded, response.body + get room_url(@room), headers: { "Accept-Encoding" => "gzip;q=0, identity;q=1" } + assert_nil response.headers["Content-Encoding"] + assert_equal decoded, response.body + get room_url(@room), headers: { "Accept-Encoding" => "*;q=0" } + assert_nil response.headers["Content-Encoding"] end test "local and foreign commits invalidate pages and nested fragments" do diff --git a/test/controllers/fetch_metadata_test.rb b/test/controllers/fetch_metadata_test.rb new file mode 100644 index 0000000..0eae80c --- /dev/null +++ b/test/controllers/fetch_metadata_test.rb @@ -0,0 +1,91 @@ +require "test_helper" + +class FetchMetadataTest < ActionDispatch::IntegrationTest + setup do + host! "once.campfire.test" + @previous_exceptions = Rails.application.env_config["action_dispatch.show_exceptions"] + Rails.application.env_config["action_dispatch.show_exceptions"] = :rescuable + @previous_forgery = ActionController::Base.allow_forgery_protection + ActionController::Base.allow_forgery_protection = true + end + + teardown do + ActionController::Base.allow_forgery_protection = @previous_forgery + Rails.application.env_config["action_dispatch.show_exceptions"] = @previous_exceptions + end + + test "login and authenticated forms have no token fields" do + get new_session_url + assert_response :success + assert_select 'meta[name="csrf-token"]', count: 0 + assert_select 'input[name="authenticity_token"]', count: 0 + sign_in :david + get room_url(rooms(:watercooler)) + assert_response :success + assert_select 'meta[name="csrf-token"]', count: 0 + assert_select 'input[name="authenticity_token"]', count: 0 + end + + test "HTTPS login accepts browser metadata without a token" do + https! + %w[ same-origin same-site ].each do |site| + reset! + host! "once.campfire.test" + https! + post session_url, params: credentials, headers: { + "Sec-Fetch-Site" => site, "Origin" => "https://once.campfire.test" } + assert_response :redirect + assert cookies["session_token"].present? + end + end + + test "HTTPS writes reject missing metadata even with a legacy token parameter" do + https! + assert_no_difference "Session.count" do + post session_url, params: credentials.merge(authenticity_token: "old token"), headers: { "Origin" => "https://once.campfire.test" } + end + assert_response :unprocessable_entity + end + + test "cross-site none and malformed metadata cannot sign in over HTTP or HTTPS" do + [ false, true ].each do |tls| + https! tls + %w[ cross-site none garbage ].each do |site| + assert_no_difference "Session.count" do + post session_url, params: credentials, headers: { "Sec-Fetch-Site" => site } + end + assert_response :unprocessable_entity + end + end + end + + test "provided foreign and null origins fail even with same-site metadata" do + %w[ https://foreign.example null ].each do |origin| + assert_no_difference "Session.count" do + post session_url, params: credentials, headers: { "Sec-Fetch-Site" => "same-site", "Origin" => origin } + end + assert_response :unprocessable_entity + end + end + + test "plain HTTP retains the missing-header fallback" do + post session_url, params: credentials, headers: { "Origin" => "http://once.campfire.test" } + assert_response :redirect + assert cookies["session_token"].present? + end + + test "cross-site reads remain available and method-overridden writes stay protected" do + get new_session_url, headers: { "Sec-Fetch-Site" => "cross-site" } + assert_response :success + sign_in :david + assert_no_difference "Room.count" do + post room_path(rooms(:watercooler)), params: { _method: "delete" }, headers: { "Sec-Fetch-Site" => "cross-site" } + end + assert_response :unprocessable_entity + end + + private + def credentials + { email_address: users(:david).email_address, password: "secret123456" } + end +end diff --git a/test/performance/chatter.js b/test/performance/chatter.js index 5bca31e..c5862c9 100644 --- a/test/performance/chatter.js +++ b/test/performance/chatter.js @@ -87,18 +87,16 @@ export function sockets() { export function messages() { const cookie = `session_token=${dummyCookies[0][0]}`; - const response = http.get(`http://${host}${port}/rooms/1`, { headers: { "Cookie": cookie }, responseType: "text" }); - const csrfToken = response.body.match(/