Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to verify Origin header in CSRF checks #22263

Merged
merged 1 commit into from Nov 26, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -77,6 +77,10 @@ module RequestForgeryProtection
config_accessor :log_warning_on_csrf_failure
self.log_warning_on_csrf_failure = true

# Controls whether the Origin header is checked in addition to the CSRF token.
config_accessor :forgery_protection_origin_check
self.forgery_protection_origin_check = false

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this might be safely enabled by default without breaking back compat since we're permitting requests from older browsers without an Origin header. When an Origin is provided, it must match. Could see this breaking folks who deploy with a reverse proxy that rewrites the Host header, so every request would be a false-positive CSRF.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it would also break apps that might run on multiple domains and POST between them. I'll defer to you folks' judgement on whether it should be enabled by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could leave it off by default for existing apps, but turn it on by default for new apps by adding an initializer to the generator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an initializer to the generator template in d17cba97dab393756c598af1019657263f101678.

helper_method :form_authenticity_token
helper_method :protect_against_forgery?
end
Expand Down Expand Up @@ -257,8 +261,19 @@ def non_xhr_javascript_response?
# * Does the X-CSRF-Token header match the form_authenticity_token
def verified_request?
!protect_against_forgery? || request.get? || request.head? ||
valid_authenticity_token?(session, form_authenticity_param) ||
valid_authenticity_token?(session, request.headers['X-CSRF-Token'])
(valid_request_origin? && any_authenticity_token_valid?)
end

# Checks if any of the authenticity tokens from the request are valid.
def any_authenticity_token_valid?
request_authenticity_tokens.any? do |token|
valid_authenticity_token?(session, token)
end
end

# Possible authenticity tokens sent in the request.
def request_authenticity_tokens
[form_authenticity_param, request.x_csrf_token]
end

# Sets the token value for the current session.
Expand Down Expand Up @@ -336,5 +351,16 @@ def form_authenticity_param
def protect_against_forgery?
allow_forgery_protection
end

# Checks if the request originated from the same origin by looking at the
# Origin header.
def valid_request_origin?
if forgery_protection_origin_check
# We accept blank origin headers because some user agents don't send it.
request.origin.nil? || request.origin == request.base_url
else
true
end
end
end
end
4 changes: 2 additions & 2 deletions actionpack/lib/action_dispatch/http/request.rb
Expand Up @@ -36,8 +36,8 @@ class Request
HTTP_ACCEPT HTTP_ACCEPT_CHARSET HTTP_ACCEPT_ENCODING
HTTP_ACCEPT_LANGUAGE HTTP_CACHE_CONTROL HTTP_FROM
HTTP_NEGOTIATE HTTP_PRAGMA HTTP_CLIENT_IP
HTTP_X_FORWARDED_FOR HTTP_VERSION
HTTP_X_REQUEST_ID HTTP_X_FORWARDED_HOST
HTTP_X_FORWARDED_FOR HTTP_ORIGIN HTTP_VERSION
HTTP_X_CSRF_TOKEN HTTP_X_REQUEST_ID HTTP_X_FORWARDED_HOST
SERVER_ADDR
].freeze

Expand Down
45 changes: 45 additions & 0 deletions actionpack/test/controller/request_forgery_protection_test.rb
Expand Up @@ -304,6 +304,41 @@ def test_should_allow_put_with_token_in_header
assert_not_blocked { put :index }
end

def test_should_allow_post_with_origin_checking_and_correct_origin
forgery_protection_origin_check do
session[:_csrf_token] = @token
@controller.stub :form_authenticity_token, @token do
assert_not_blocked do
@request.set_header 'HTTP_ORIGIN', 'http://test.host'
post :index, params: { custom_authenticity_token: @token }
end
end
end
end

def test_should_allow_post_with_origin_checking_and_no_origin
forgery_protection_origin_check do
session[:_csrf_token] = @token
@controller.stub :form_authenticity_token, @token do
assert_not_blocked do
post :index, params: { custom_authenticity_token: @token }
end
end
end
end

def test_should_block_post_with_origin_checking_and_wrong_origin
forgery_protection_origin_check do
session[:_csrf_token] = @token
@controller.stub :form_authenticity_token, @token do
assert_blocked do
@request.set_header 'HTTP_ORIGIN', 'http://bad.host'
post :index, params: { custom_authenticity_token: @token }
end
end
end
end

def test_should_warn_on_missing_csrf_token
old_logger = ActionController::Base.logger
logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
Expand Down Expand Up @@ -405,6 +440,16 @@ def assert_cross_origin_blocked
def assert_cross_origin_not_blocked
assert_not_blocked { yield }
end

def forgery_protection_origin_check
old_setting = ActionController::Base.forgery_protection_origin_check
ActionController::Base.forgery_protection_origin_check = true
begin
yield
ensure
ActionController::Base.forgery_protection_origin_check = old_setting
end
end
end

# OK let's get our test on
Expand Down
2 changes: 2 additions & 0 deletions guides/source/configuring.md
Expand Up @@ -345,6 +345,8 @@ The schema dumper adds one additional configuration option:

* `config.action_controller.allow_forgery_protection` enables or disables CSRF protection. By default this is `false` in test mode and `true` in all other modes.

* `config.action_controller.forgery_protection_origin_check` configures whether the HTTP `Origin` header should be checked against the site's origin as an additional CSRF defense.

* `config.action_controller.relative_url_root` can be used to tell Rails that you are [deploying to a subdirectory](configuring.html#deploy-to-a-subdirectory-relative-url-root). The default is `ENV['RAILS_RELATIVE_URL_ROOT']`.

* `config.action_controller.permit_all_parameters` sets all the parameters for mass assignment to be permitted by default. The default value is `false`.
Expand Down
@@ -0,0 +1,4 @@
# Be sure to restart your server when you modify this file.

# Enable origin-checking CSRF mitigation.
Rails.application.config.action_controller.forgery_protection_origin_check = true