diff --git a/app/controllers/concerns/request_forgery_protection.rb b/app/controllers/concerns/request_forgery_protection.rb index ddc47065f..8ede51fc7 100644 --- a/app/controllers/concerns/request_forgery_protection.rb +++ b/app/controllers/concerns/request_forgery_protection.rb @@ -12,14 +12,7 @@ module RequestForgeryProtection 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 + super || safe_fetch_site? end SAFE_FETCH_SITES = %w[ same-origin same-site ] @@ -31,20 +24,4 @@ module RequestForgeryProtection def sec_fetch_site_value request.headers["Sec-Fetch-Site"].to_s.downcase end - - def report_on_forgery_protection_results(origin:, token:, sec_fetch_site:) - results = { origin:, token:, sec_fetch_site: } - - unless results.values.all? - info = results.transform_values { it ? "pass" : "fail" } - info[:origin] += " (#{request.origin})" - info[:sec_fetch_site] += " (#{sec_fetch_site_value})" - - Rails.logger.info "CSRF protection check: " + info.map { it.join(" ") }.join(", ") - - if Fizzy.saas? && (origin && token) != sec_fetch_site - Sentry.capture_message "CSRF protection mismatch", level: :info, extra: { info: info } - end - end - end end diff --git a/test/controllers/concerns/request_forgery_protection_test.rb b/test/controllers/concerns/request_forgery_protection_test.rb index 8ac5327c2..3fd250de4 100644 --- a/test/controllers/concerns/request_forgery_protection_test.rb +++ b/test/controllers/concerns/request_forgery_protection_test.rb @@ -4,8 +4,6 @@ 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 @@ -14,105 +12,90 @@ class RequestForgeryProtectionTest < ActionDispatch::IntegrationTest 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 + test "succeeds when CSRF token is valid" 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 + + assert_response :redirect + end + + test "succeeds with same-origin Sec-Fetch-Site even without CSRF token" do + assert_difference -> { Board.count }, +1 do + post boards_path, + params: { board: { name: "Test Board" } }, headers: { "Sec-Fetch-Site" => "same-origin" } end + + assert_response :redirect end - test "don't report when Sec-Fetch-Site is same-site and CSRF token matches" do - assert_log(excludes: "CSRF protection check") do + test "succeeds with same-site Sec-Fetch-Site even without CSRF token" do + assert_difference -> { Board.count }, +1 do post boards_path, - params: { board: { name: "Test Board" }, authenticity_token: csrf_token }, + params: { board: { name: "Test Board" } }, headers: { "Sec-Fetch-Site" => "same-site" } end + + assert_response :redirect 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 + test "fails with cross-site Sec-Fetch-Site and no CSRF token" do + assert_no_difference -> { Board.count } do + post boards_path, + params: { board: { name: "Test Board" } }, + headers: { "Sec-Fetch-Site" => "cross-site" } end + + assert_response :unprocessable_entity 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 + test "fails with none Sec-Fetch-Site and no CSRF token" do + assert_no_difference -> { Board.count } do + post boards_path, + params: { board: { name: "Test Board" } }, + headers: { "Sec-Fetch-Site" => "none" } end + + assert_response :unprocessable_entity 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 + test "fails when Sec-Fetch-Site header is missing and no CSRF token" do + assert_no_difference -> { Board.count } do + post boards_path, params: { board: { name: "Test Board" } } end + + assert_response :unprocessable_entity 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 + test "fails with invalid CSRF token and cross-site Sec-Fetch-Site" do + assert_no_difference -> { Board.count } do + post boards_path, + params: { board: { name: "Test Board" }, authenticity_token: "invalid-token" }, + headers: { "Sec-Fetch-Site" => "cross-site" } 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 + assert_response :unprocessable_entity end test "GET requests succeed regardless of Sec-Fetch-Site header" do - get board_url(boards(:writebook)), headers: { "Sec-Fetch-Site" => "cross-site" } + get board_path(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" } + test "appends Sec-Fetch-Site to Vary header on GET requests" do + get board_path(boards(:writebook)) assert_response :success - vary_values = response.headers["Vary"].split(",").map(&:strip) - assert_includes vary_values, "Sec-Fetch-Site" + assert_includes response.headers["Vary"], "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 }, + params: { board: { name: "Test Board" } }, headers: { "Sec-Fetch-Site" => "same-origin" } assert_response :redirect @@ -120,30 +103,9 @@ class RequestForgeryProtectionTest < ActionDispatch::IntegrationTest 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 - if Fizzy.saas? - Sentry.expects(:capture_message).with do |message, **kwargs| - message == "CSRF protection mismatch" && kwargs[:level] == :info - end - end - end - def csrf_token @csrf_token ||= begin - get new_board_url + get new_board_path response.body[/name="authenticity_token" value="([^"]+)"/, 1] end end