diff --git a/app/controllers/concerns/request_forgery_protection.rb b/app/controllers/concerns/request_forgery_protection.rb index c8481beb3..fdff7ba8f 100644 --- a/app/controllers/concerns/request_forgery_protection.rb +++ b/app/controllers/concerns/request_forgery_protection.rb @@ -2,31 +2,19 @@ module RequestForgeryProtection extend ActiveSupport::Concern included do - after_action :append_sec_fetch_site_to_vary_header + protect_from_forgery using: :header_only, with: :exception end private - def append_sec_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(",") + def verified_via_header_only? + super || allowed_api_request? || allowed_insecure_context_request? end - def verified_request? - request.get? || request.head? || !protect_against_forgery? || - (valid_request_origin? && safe_fetch_site?) + def allowed_api_request? + sec_fetch_site_value.nil? && request.format.json? end - SAFE_FETCH_SITES = %w[ same-origin same-site ] - - def safe_fetch_site? - SAFE_FETCH_SITES.include?(sec_fetch_site_value) || (sec_fetch_site_value.nil? && api_request?) - end - - def api_request? - request.format.json? - end - - def sec_fetch_site_value - request.headers["Sec-Fetch-Site"].to_s.downcase.presence + def allowed_insecure_context_request? + sec_fetch_site_value.nil? && !request.ssl? && !Rails.configuration.force_ssl end end diff --git a/test/controllers/concerns/request_forgery_protection_test.rb b/test/controllers/concerns/request_forgery_protection_test.rb index 233c1e361..db1a84e63 100644 --- a/test/controllers/concerns/request_forgery_protection_test.rb +++ b/test/controllers/concerns/request_forgery_protection_test.rb @@ -6,83 +6,16 @@ class RequestForgeryProtectionTest < ActionDispatch::IntegrationTest @original_allow_forgery_protection = ActionController::Base.allow_forgery_protection ActionController::Base.allow_forgery_protection = true + + @original_force_ssl = Rails.configuration.force_ssl end teardown do ActionController::Base.allow_forgery_protection = @original_allow_forgery_protection + Rails.configuration.force_ssl = @original_force_ssl end - test "fails if Sec-Fetch-Site is cross-site" 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 "succeeds with same-origin Sec-Fetch-Site" 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 "succeeds with same-site Sec-Fetch-Site" do - assert_difference -> { Board.count }, +1 do - post boards_path, - params: { board: { name: "Test Board" } }, - headers: { "Sec-Fetch-Site" => "same-site" } - end - - assert_response :redirect - end - - test "fails with none Sec-Fetch-Site" 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 "fails when Sec-Fetch-Site header is missing" do - assert_no_difference -> { Board.count } do - post boards_path, params: { board: { name: "Test Board" } } - end - - assert_response :unprocessable_entity - end - - test "GET requests succeed regardless of Sec-Fetch-Site header" do - get board_path(boards(:writebook)), headers: { "Sec-Fetch-Site" => "cross-site" } - - assert_response :success - end - - test "appends Sec-Fetch-Site to Vary header on GET requests" do - get board_path(boards(:writebook)) - - assert_response :success - 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" } }, - headers: { "Sec-Fetch-Site" => "same-origin" } - - assert_response :redirect - assert_includes response.headers["Vary"], "Sec-Fetch-Site" - end - - test "JSON request succeeds with missing Sec-Fetch-Site" do + test "JSON request succeeds with missing Sec-Fetch-Site header" do assert_difference -> { Board.count }, +1 do post boards_path, params: { board: { name: "Test Board" } }, @@ -92,22 +25,37 @@ class RequestForgeryProtectionTest < ActionDispatch::IntegrationTest assert_response :created end - test "JSON request fails with cross-site Sec-Fetch-Site" do + test "HTTP request succeeds with missing Sec-Fetch-Site header when force_ssl is disabled" do + Rails.configuration.force_ssl = false + + assert_difference -> { Board.count }, +1 do + post boards_path, + params: { board: { name: "Test Board" } } + end + + assert_response :redirect + end + + test "HTTP request fails with missing Sec-Fetch-Site header when force_ssl is enabled" do + Rails.configuration.force_ssl = true + assert_no_difference -> { Board.count } do post boards_path, - params: { board: { name: "Test Board" } }, - headers: { "Sec-Fetch-Site" => "cross-site" }, - as: :json + params: { board: { name: "Test Board" } } end assert_response :unprocessable_entity end - private - def csrf_token - @csrf_token ||= begin - get new_board_path - response.body[/name="authenticity_token" value="([^"]+)"/, 1] - end + test "HTTPS request fails with missing Sec-Fetch-Site header" do + Rails.configuration.force_ssl = false + + assert_no_difference -> { Board.count } do + post boards_path, + params: { board: { name: "Test Board" } }, + headers: { "X-Forwarded-Proto" => "https" } end + + assert_response :unprocessable_entity + end end