Switch from report-only to actually using Sec-Fetch-Site for CSRF protection
As a fallback for Rails's token-based mechanism. To use Sec-Fetch-Site exclusively, we'll wait until Rails offers that (when we upstream this).
This commit is contained in:
Rosa Gutierrez
committed by
Rosa Gutierrez
Rosa Gutierrez
parent
6941108cdb
commit
f8a1e0500d
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user