diff options
Diffstat (limited to 'actionpack')
| -rw-r--r-- | actionpack/CHANGELOG.md | 20 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/metal/request_forgery_protection.rb | 65 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/inspector.rb | 25 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/mapper.rb | 42 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/route_set.rb | 13 | ||||
| -rw-r--r-- | actionpack/test/controller/request_forgery_protection_test.rb | 172 | ||||
| -rw-r--r-- | actionpack/test/dispatch/routing/inspector_test.rb | 38 | ||||
| -rw-r--r-- | actionpack/test/dispatch/routing_test.rb | 21 | ||||
| -rw-r--r-- | actionpack/test/dispatch/ssl_test.rb | 2 |
9 files changed, 356 insertions, 42 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index ac767652b8..b93ae8f8ff 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,23 @@ +* More explicit error message when running `rake routes`. `CONTROLLER` argument + can now be supplied in different ways: + `Rails::WelcomeController`, `Rails::Welcome`, `rails/welcome` + + Fixes #22918 + + *Edouard Chin* + +* Allow `ActionController::Parameters` instances as an argument to URL + helper methods. An `ArgumentError` will be raised if the passed parameters + are not secure. + + Fixes #22832 + + *Prathamesh Sonpatki* + +* Add option for per-form CSRF tokens. + + *Greg Ose & Ben Toews* + * Add tests and documentation for `ActionController::Renderers::use_renderers`. *Benjamin Fleischer* diff --git a/actionpack/lib/action_controller/metal/request_forgery_protection.rb b/actionpack/lib/action_controller/metal/request_forgery_protection.rb index 26c4550f89..91b3403ad5 100644 --- a/actionpack/lib/action_controller/metal/request_forgery_protection.rb +++ b/actionpack/lib/action_controller/metal/request_forgery_protection.rb @@ -81,6 +81,10 @@ module ActionController #:nodoc: config_accessor :forgery_protection_origin_check self.forgery_protection_origin_check = false + # Controls whether form-action/method specific CSRF tokens are used. + config_accessor :per_form_csrf_tokens + self.per_form_csrf_tokens = false + helper_method :form_authenticity_token helper_method :protect_against_forgery? end @@ -277,16 +281,25 @@ module ActionController #:nodoc: end # Sets the token value for the current session. - def form_authenticity_token - masked_authenticity_token(session) + def form_authenticity_token(form_options: {}) + masked_authenticity_token(session, form_options: form_options) end # Creates a masked version of the authenticity token that varies # on each request. The masking is used to mitigate SSL attacks # like BREACH. - def masked_authenticity_token(session) + def masked_authenticity_token(session, form_options: {}) + action, method = form_options.values_at(:action, :method) + + raw_token = if per_form_csrf_tokens && action && method + action_path = normalize_action_path(action) + per_form_csrf_token(session, action_path, method) + else + real_csrf_token(session) + end + one_time_pad = SecureRandom.random_bytes(AUTHENTICITY_TOKEN_LENGTH) - encrypted_csrf_token = xor_byte_strings(one_time_pad, real_csrf_token(session)) + encrypted_csrf_token = xor_byte_strings(one_time_pad, raw_token) masked_token = one_time_pad + encrypted_csrf_token Base64.strict_encode64(masked_token) end @@ -316,28 +329,54 @@ module ActionController #:nodoc: compare_with_real_token masked_token, session elsif masked_token.length == AUTHENTICITY_TOKEN_LENGTH * 2 - # Split the token into the one-time pad and the encrypted - # value and decrypt it - one_time_pad = masked_token[0...AUTHENTICITY_TOKEN_LENGTH] - encrypted_csrf_token = masked_token[AUTHENTICITY_TOKEN_LENGTH..-1] - csrf_token = xor_byte_strings(one_time_pad, encrypted_csrf_token) - - compare_with_real_token csrf_token, session + csrf_token = unmask_token(masked_token) + compare_with_real_token(csrf_token, session) || + valid_per_form_csrf_token?(csrf_token, session) else false # Token is malformed end end + def unmask_token(masked_token) + # Split the token into the one-time pad and the encrypted + # value and decrypt it + one_time_pad = masked_token[0...AUTHENTICITY_TOKEN_LENGTH] + encrypted_csrf_token = masked_token[AUTHENTICITY_TOKEN_LENGTH..-1] + xor_byte_strings(one_time_pad, encrypted_csrf_token) + end + def compare_with_real_token(token, session) ActiveSupport::SecurityUtils.secure_compare(token, real_csrf_token(session)) end + def valid_per_form_csrf_token?(token, session) + if per_form_csrf_tokens + correct_token = per_form_csrf_token( + session, + normalize_action_path(request.fullpath), + request.request_method + ) + + ActiveSupport::SecurityUtils.secure_compare(token, correct_token) + else + false + end + end + def real_csrf_token(session) session[:_csrf_token] ||= SecureRandom.base64(AUTHENTICITY_TOKEN_LENGTH) Base64.strict_decode64(session[:_csrf_token]) end + def per_form_csrf_token(session, action_path, method) + OpenSSL::HMAC.digest( + OpenSSL::Digest::SHA256.new, + real_csrf_token(session), + [action_path, method.downcase].join("#") + ) + end + def xor_byte_strings(s1, s2) s1.bytes.zip(s2.bytes).map { |(c1,c2)| c1 ^ c2 }.pack('c*') end @@ -362,5 +401,9 @@ module ActionController #:nodoc: true end end + + def normalize_action_path(action_path) + action_path.split('?').first.to_s.chomp('/') + end end end diff --git a/actionpack/lib/action_dispatch/routing/inspector.rb b/actionpack/lib/action_dispatch/routing/inspector.rb index f3a5268d2e..69e6dd5215 100644 --- a/actionpack/lib/action_dispatch/routing/inspector.rb +++ b/actionpack/lib/action_dispatch/routing/inspector.rb @@ -65,7 +65,7 @@ module ActionDispatch routes = collect_routes(routes_to_display) if routes.none? - formatter.no_routes + formatter.no_routes(collect_routes(@routes), filter) return formatter.result end @@ -84,7 +84,8 @@ module ActionDispatch def filter_routes(filter) if filter - @routes.select { |route| route.defaults[:controller] == filter } + filter_name = filter.underscore.sub(/_controller$/, '') + @routes.select { |route| route.defaults[:controller] == filter_name } else @routes end @@ -136,17 +137,27 @@ module ActionDispatch @buffer << draw_header(routes) end - def no_routes - @buffer << <<-MESSAGE.strip_heredoc + def no_routes(routes, filter) + @buffer << + if routes.none? + <<-MESSAGE.strip_heredoc You don't have any routes defined! Please add some routes in config/routes.rb. - - For more information about routes, see the Rails guide: http://guides.rubyonrails.org/routing.html. MESSAGE + elsif missing_controller?(filter) + "The controller #{filter} does not exist!" + else + "No routes were found for this controller" + end + @buffer << "For more information about routes, see the Rails guide: http://guides.rubyonrails.org/routing.html." end private + def missing_controller?(controller_name) + [ controller_name.camelize, "#{controller_name.camelize}Controller" ].none?(&:safe_constantize) + end + def draw_section(routes) header_lengths = ['Prefix', 'Verb', 'URI Pattern'].map(&:length) name_width, verb_width, path_width = widths(routes).zip(header_lengths).map(&:max) @@ -187,7 +198,7 @@ module ActionDispatch def header(routes) end - def no_routes + def no_routes(*) @buffer << <<-MESSAGE.strip_heredoc <p>You don't have any routes defined!</p> <ul> diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 18cd205bad..522012063d 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -387,24 +387,6 @@ module ActionDispatch end module Base - # You can specify what Rails should route "/" to with the root method: - # - # root to: 'pages#main' - # - # For options, see +match+, as +root+ uses it internally. - # - # You can also pass a string which will expand - # - # root 'pages#main' - # - # You should put the root route at the top of <tt>config/routes.rb</tt>, - # because this means it will be matched first. As this is the most popular route - # of most Rails applications, this is beneficial. - def root(options = {}) - name = has_named_route?(:root) ? nil : :root - match '/', { as: name, via: :get }.merge!(options) - end - # Matches a url pattern to one or more routes. # # You should not use the +match+ method in your router @@ -1689,7 +1671,20 @@ to this: @set.add_route(mapping, ast, as, anchor) end - def root(path, options={}) + # You can specify what Rails should route "/" to with the root method: + # + # root to: 'pages#main' + # + # For options, see +match+, as +root+ uses it internally. + # + # You can also pass a string which will expand + # + # root 'pages#main' + # + # You should put the root route at the top of <tt>config/routes.rb</tt>, + # because this means it will be matched first. As this is the most popular route + # of most Rails applications, this is beneficial. + def root(path, options = {}) if path.is_a?(String) options[:to] = path elsif path.is_a?(Hash) and options.empty? @@ -1701,11 +1696,11 @@ to this: if @scope.resources? with_scope_level(:root) do path_scope(parent_resource.path) do - super(options) + match_root_route(options) end end else - super(options) + match_root_route(options) end end @@ -1900,6 +1895,11 @@ to this: ensure @scope = @scope.parent end + + def match_root_route(options) + name = has_named_route?(:root) ? nil : :root + match '/', { :as => name, :via => :get }.merge!(options) + end end # Routing Concerns allow you to declare common routes that can be reused diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 2bd2e53252..846b5fa1fc 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -281,8 +281,17 @@ module ActionDispatch helper = UrlHelper.create(route, opts, route_key, url_strategy) mod.module_eval do define_method(name) do |*args| - options = nil - options = args.pop if args.last.is_a? Hash + last = args.last + options = case last + when Hash + args.pop + when ActionController::Parameters + if last.permitted? + args.pop.to_h + else + raise ArgumentError, "Generating an URL from non sanitized request parameters is insecure!" + end + end helper.call self, args, options end end diff --git a/actionpack/test/controller/request_forgery_protection_test.rb b/actionpack/test/controller/request_forgery_protection_test.rb index 87a8ed3dc9..1984ad8825 100644 --- a/actionpack/test/controller/request_forgery_protection_test.rb +++ b/actionpack/test/controller/request_forgery_protection_test.rb @@ -128,6 +128,23 @@ class CustomAuthenticityParamController < RequestForgeryProtectionControllerUsin end end +class PerFormTokensController < ActionController::Base + protect_from_forgery :with => :exception + self.per_form_csrf_tokens = true + + def index + render inline: "<%= form_tag (params[:form_path] || '/per_form_tokens/post_one'), method: (params[:form_method] || :post) %>" + end + + def post_one + render plain: '' + end + + def post_two + render plain: '' + end +end + # common test methods module RequestForgeryProtectionTests def setup @@ -623,3 +640,158 @@ class CustomAuthenticityParamControllerTest < ActionController::TestCase end end end + +class PerFormTokensControllerTest < ActionController::TestCase + def test_per_form_token_is_same_size_as_global_token + get :index + expected = ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH + actual = @controller.send(:per_form_csrf_token, session, '/path', 'post').size + assert_equal expected, actual + end + + def test_accepts_token_for_correct_path_and_method + get :index + + form_token = nil + assert_select 'input[name=custom_authenticity_token]' do |elts| + form_token = elts.first['value'] + assert_not_nil form_token + end + + actual = @controller.send(:unmask_token, Base64.strict_decode64(form_token)) + expected = @controller.send(:per_form_csrf_token, session, '/per_form_tokens/post_one', 'post') + assert_equal expected, actual + + # This is required because PATH_INFO isn't reset between requests. + @request.env['PATH_INFO'] = '/per_form_tokens/post_one' + assert_nothing_raised do + post :post_one, params: {custom_authenticity_token: form_token} + end + assert_response :success + end + + def test_rejects_token_for_incorrect_path + get :index + + form_token = nil + assert_select 'input[name=custom_authenticity_token]' do |elts| + form_token = elts.first['value'] + assert_not_nil form_token + end + + actual = @controller.send(:unmask_token, Base64.strict_decode64(form_token)) + expected = @controller.send(:per_form_csrf_token, session, '/per_form_tokens/post_one', 'post') + assert_equal expected, actual + + # This is required because PATH_INFO isn't reset between requests. + @request.env['PATH_INFO'] = '/per_form_tokens/post_two' + assert_raises(ActionController::InvalidAuthenticityToken) do + post :post_two, params: {custom_authenticity_token: form_token} + end + end + + def test_rejects_token_for_incorrect_method + get :index + + form_token = nil + assert_select 'input[name=custom_authenticity_token]' do |elts| + form_token = elts.first['value'] + assert_not_nil form_token + end + + actual = @controller.send(:unmask_token, Base64.strict_decode64(form_token)) + expected = @controller.send(:per_form_csrf_token, session, '/per_form_tokens/post_one', 'post') + assert_equal expected, actual + + # This is required because PATH_INFO isn't reset between requests. + @request.env['PATH_INFO'] = '/per_form_tokens/post_one' + assert_raises(ActionController::InvalidAuthenticityToken) do + patch :post_one, params: {custom_authenticity_token: form_token} + end + end + + def test_accepts_global_csrf_token + get :index + + token = @controller.send(:form_authenticity_token) + + # This is required because PATH_INFO isn't reset between requests. + @request.env['PATH_INFO'] = '/per_form_tokens/post_one' + assert_nothing_raised do + post :post_one, params: {custom_authenticity_token: token} + end + assert_response :success + end + + def test_ignores_params + get :index, params: {form_path: '/per_form_tokens/post_one?foo=bar'} + + form_token = nil + assert_select 'input[name=custom_authenticity_token]' do |elts| + form_token = elts.first['value'] + assert_not_nil form_token + end + + actual = @controller.send(:unmask_token, Base64.strict_decode64(form_token)) + expected = @controller.send(:per_form_csrf_token, session, '/per_form_tokens/post_one', 'post') + assert_equal expected, actual + + # This is required because PATH_INFO isn't reset between requests. + @request.env['PATH_INFO'] = '/per_form_tokens/post_one?foo=baz' + assert_nothing_raised do + post :post_one, params: {custom_authenticity_token: form_token, baz: 'foo'} + end + assert_response :success + end + + def test_ignores_trailing_slash_during_generation + get :index, params: {form_path: '/per_form_tokens/post_one/'} + + form_token = nil + assert_select 'input[name=custom_authenticity_token]' do |elts| + form_token = elts.first['value'] + assert_not_nil form_token + end + + # This is required because PATH_INFO isn't reset between requests. + @request.env['PATH_INFO'] = '/per_form_tokens/post_one' + assert_nothing_raised do + post :post_one, params: {custom_authenticity_token: form_token} + end + assert_response :success + end + + def test_ignores_trailing_slash_during_validation + get :index + + form_token = nil + assert_select 'input[name=custom_authenticity_token]' do |elts| + form_token = elts.first['value'] + assert_not_nil form_token + end + + # This is required because PATH_INFO isn't reset between requests. + @request.env['PATH_INFO'] = '/per_form_tokens/post_one/' + assert_nothing_raised do + post :post_one, params: {custom_authenticity_token: form_token} + end + assert_response :success + end + + def test_method_is_case_insensitive + get :index, params: {form_method: "POST"} + + form_token = nil + assert_select 'input[name=custom_authenticity_token]' do |elts| + form_token = elts.first['value'] + assert_not_nil form_token + end + + # This is required because PATH_INFO isn't reset between requests. + @request.env['PATH_INFO'] = '/per_form_tokens/post_one/' + assert_nothing_raised do + post :post_one, params: {custom_authenticity_token: form_token} + end + assert_response :success + end +end diff --git a/actionpack/test/dispatch/routing/inspector_test.rb b/actionpack/test/dispatch/routing/inspector_test.rb index a17d07c40b..7382c267c7 100644 --- a/actionpack/test/dispatch/routing/inspector_test.rb +++ b/actionpack/test/dispatch/routing/inspector_test.rb @@ -7,6 +7,9 @@ class MountedRackApp end end +class Rails::DummyController +end + module ActionDispatch module Routing class RoutesInspectorTest < ActiveSupport::TestCase @@ -331,6 +334,41 @@ module ActionDispatch " cart GET /cart(.:format) cart#show" ], output end + + def test_routes_with_undefined_filter + output = draw(:filter => 'Rails::MissingController') do + get 'photos/:id' => 'photos#show', :id => /[A-Z]\d{5}/ + end + + assert_equal [ + "The controller Rails::MissingController does not exist!", + "For more information about routes, see the Rails guide: http://guides.rubyonrails.org/routing.html." + ], output + end + + def test_no_routes_matched_filter + output = draw(:filter => 'rails/dummy') do + get 'photos/:id' => 'photos#show', :id => /[A-Z]\d{5}/ + end + + assert_equal [ + "No routes were found for this controller", + "For more information about routes, see the Rails guide: http://guides.rubyonrails.org/routing.html." + ], output + end + + def test_no_routes_were_defined + output = draw(:filter => 'Rails::DummyController') { } + + assert_equal [ + "You don't have any routes defined!", + "", + "Please add some routes in config/routes.rb.", + "", + "For more information about routes, see the Rails guide: http://guides.rubyonrails.org/routing.html." + ], output + end + end end end diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index 82222a141c..62d65ec5c0 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -3578,6 +3578,27 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest assert_equal 'HEAD', @response.body end + def test_passing_action_parameters_to_url_helpers_raises_error_if_parameters_are_not_permitted + draw do + root :to => 'projects#index' + end + params = ActionController::Parameters.new(id: '1') + + assert_raises ArgumentError do + root_path(params) + end + end + + def test_passing_action_parameters_to_url_helpers_is_allowed_if_parameters_are_permitted + draw do + root :to => 'projects#index' + end + params = ActionController::Parameters.new(id: '1') + params.permit! + + assert_equal '/?id=1', root_path(params) + end + private def draw(&block) diff --git a/actionpack/test/dispatch/ssl_test.rb b/actionpack/test/dispatch/ssl_test.rb index 11251c86e7..c66a0e6a7a 100644 --- a/actionpack/test/dispatch/ssl_test.rb +++ b/actionpack/test/dispatch/ssl_test.rb @@ -199,7 +199,7 @@ class SecureCookiesTest < SSLTest def test_cookies_as_not_secure_with_secure_cookies_disabled get headers: { 'Set-Cookie' => DEFAULT }, ssl_options: { secure_cookies: false } - assert_cookies *DEFAULT.split("\n") + assert_cookies(*DEFAULT.split("\n")) end def test_no_cookies |