diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 0ade45a66..89b94f079 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -2,6 +2,7 @@ class ApplicationController < ActionController::Base include Authentication include Authorization include CurrentRequest, CurrentTimezone, SetPlatform + include RequestForgeryProtection include TurboFlash, ViewTransitions include Saas include RoutingHeaders diff --git a/app/controllers/concerns/request_forgery_protection.rb b/app/controllers/concerns/request_forgery_protection.rb new file mode 100644 index 000000000..b3ca6ceed --- /dev/null +++ b/app/controllers/concerns/request_forgery_protection.rb @@ -0,0 +1,54 @@ +module RequestForgeryProtection + extend ActiveSupport::Concern + + included do + after_action :append_set_fetch_site_to_vary_header + end + + private + def append_set_fetch_site_to_vary_header + vary_header = response.headers["Vary"].to_s.split(",").map(&:strip).reject(&:blank?) + response.headers["Vary"] = (vary_header + [ "Sec-Fetch-Site" ]).join(",") + end + + def verified_request? + return true if request.get? || request.head? || !protect_against_forgery? + + origin = valid_request_origin? + token = any_authenticity_token_valid? + sec_fetch_site = safe_fetch_site? + report_on_forgery_protection_results(origin:, token:, sec_fetch_site:) + + origin && token + end + + SAFE_FETCH_SITES = %w[ same-origin same-site ] + + def safe_fetch_site? + SAFE_FETCH_SITES.include?(safe_fetch_site_value) + end + + def safe_fetch_site_value + request.headers["Sec-Fetch-Site"].to_s.downcase + end + + def report_on_forgery_protection_results(origin:, token:, sec_fetch_site:) + unless [ origin, token, sec_fetch_site ].all? + info = { + origin: "#{pass_or_fail_value(origin)} (#{request.origin})", + token: "#{pass_or_fail_value(token)}", + sec_fetch_site: "#{pass_or_fail_value(sec_fetch_site)} (#{safe_fetch_site_value})" + } + + Rails.logger.info "CSRF protection check: " + info.map { it.join(" ") }.join(", ") + + if (origin && token) != sec_fetch_site + Sentry.capture_message "CSRF protection mismatch", level: :info, extra: { info: info } + end + end + end + + def pass_or_fail_value(result) + result ? "pass" : "fail" + end +end diff --git a/test/controllers/concerns/request_forgery_protection_test.rb b/test/controllers/concerns/request_forgery_protection_test.rb new file mode 100644 index 000000000..797c2039a --- /dev/null +++ b/test/controllers/concerns/request_forgery_protection_test.rb @@ -0,0 +1,148 @@ +require "test_helper" + +class RequestForgeryProtectionTest < ActionDispatch::IntegrationTest + setup do + sign_in_as :kevin + + # Forgery protection is disabled in test environment so we need to + # enable it here + @original_allow_forgery_protection = ActionController::Base.allow_forgery_protection + ActionController::Base.allow_forgery_protection = true + end + + teardown do + ActionController::Base.allow_forgery_protection = @original_allow_forgery_protection + end + + test "don't report when Sec-Fetch-Site is same-origin and CSRF token matches" do + assert_log(excludes: "CSRF protection check") do + post boards_path, + params: { board: { name: "Test Board" }, authenticity_token: csrf_token }, + headers: { "Sec-Fetch-Site" => "same-origin" } + end + end + + test "don't report when Sec-Fetch-Site is same-site and CSRF token matches" do + assert_log(excludes: "CSRF protection check") do + post boards_path, + params: { board: { name: "Test Board" }, authenticity_token: csrf_token }, + headers: { "Sec-Fetch-Site" => "same-site" } + end + end + + test "fail and report when token doesn't match, regardless of Sec-Fetch-Site" do + assert_report + + assert_log(includes: [ "CSRF protection check", "sec_fetch_site pass (same-origin)", "token fail" ]) do + assert_no_difference -> { Board.count } do + post boards_path, + params: { board: { name: "Test Board" }, authenticity_token: "invalid-token" }, + headers: { "Sec-Fetch-Site" => "same-origin" } + end + end + end + + test "fail and report when Origin doesn't match, regardless of Sec-Fetch-Site" do + assert_report + + assert_log(includes: [ "CSRF protection check", "sec_fetch_site pass (same-origin)", + "token pass", "origin fail (evil-site.com)" ]) do + assert_no_difference -> { Board.count } do + post boards_path, + params: { board: { name: "Test Board" }, authenticity_token: csrf_token }, + headers: { "Sec-Fetch-Site" => "same-origin", Origin: "evil-site.com" } + end + end + end + + test "succeed and report when Sec-Fetch-Site is cross-site and CSRF token matches" do + assert_report + + assert_log(includes: [ "CSRF protection check", "sec_fetch_site fail (cross-site)", "token pass" ]) do + assert_difference -> { Board.count }, +1 do + post boards_path, + params: { board: { name: "Test Board" }, authenticity_token: csrf_token }, + headers: { "Sec-Fetch-Site" => "cross-site" } + end + end + end + + test "succeed and report when Sec-Fetch-Site is none" do + assert_report + + assert_log(includes: [ "CSRF protection check", "sec_fetch_site fail (none)", "token pass" ]) do + assert_difference -> { Board.count }, +1 do + post boards_path, + params: { board: { name: "Test Board" }, authenticity_token: csrf_token }, + headers: { "Sec-Fetch-Site" => "none" } + end + end + end + + test "succeed and report when Sec-Fetch-Site is missing" do + assert_report + + assert_log(includes: [ "CSRF protection check", "sec_fetch_site fail ()", "token pass" ]) do + assert_difference -> { Board.count }, +1 do + post boards_path, params: { board: { name: "Test Board" }, authenticity_token: csrf_token } + end + end + end + + test "don't report and succeed for GET requests" do + assert_log(excludes: "CSRF protection check") do + get board_url(boards(:writebook)), headers: { "Sec-Fetch-Site" => "cross-site" } + assert_response :success + end + end + + test "GET requests succeed regardless of Sec-Fetch-Site header" do + get board_url(boards(:writebook)), headers: { "Sec-Fetch-Site" => "cross-site" } + + assert_response :success + end + + test "appends Sec-Fetch-Site to existing Vary header" do + get board_url(boards(:writebook)), headers: { "Accept" => "text/html" } + + assert_response :success + vary_values = response.headers["Vary"].split(",").map(&:strip) + assert_includes vary_values, "Sec-Fetch-Site" + end + + test "appends Sec-Fetch-Site to Vary header on POST requests" do + post boards_path, + params: { board: { name: "Test Board" }, authenticity_token: csrf_token }, + headers: { "Sec-Fetch-Site" => "same-origin" } + + assert_response :redirect + assert_includes response.headers["Vary"], "Sec-Fetch-Site" + end + + private + def assert_log(includes: [], excludes: [], &block) + original_logger = Rails.logger + log_output = StringIO.new + Rails.logger = ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(log_output)) + + yield + + Array(includes).each { assert_includes(log_output.string, it) } + Array(excludes).each { assert_not_includes(log_output.string, it) } + ensure + Rails.logger = original_logger + end + + def assert_report + Sentry.expects(:capture_message).with do |message, **kwargs| + message == "CSRF protection mismatch" && kwargs[:level] == :info + end + end + + def csrf_token + @csrf_token ||= begin + get new_board_url + response.body[/name="authenticity_token" value="([^"]+)"/, 1] + end + end +end