From 82ff27766d3edc3fb1d0d043841e9c3cd277744f Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Sep 2007 16:50:48 +0000 Subject: [PATCH] Better error messages if you leave out the :secret option for request forgery protection. Closes #9670 [rick] git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@7671 5ecf4fe2-1ee6-0310-87b1-e25e094e27de --- actionpack/CHANGELOG | 2 + .../request_forgery_protection.rb | 6 +- .../request_forgery_protection_test.rb | 119 +++++++++++------- railties/helpers/application.rb | 5 +- .../applications/app/app_generator.rb | 2 +- 5 files changed, 86 insertions(+), 48 deletions(-) diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index 13db84720c..ad7e8ee1f9 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,7 @@ *SVN* +* Better error messages if you leave out the :secret option for request forgery protection. Closes #9670 [rick] + * Allow ability to disable request forgery protection, disable it in test mode by default. Closes #9693 [lifofifo] * Avoid calling is_missing on LoadErrors. Closes #7460. [ntalbott] diff --git a/actionpack/lib/action_controller/request_forgery_protection.rb b/actionpack/lib/action_controller/request_forgery_protection.rb index 3a7eb789c4..035bad7498 100644 --- a/actionpack/lib/action_controller/request_forgery_protection.rb +++ b/actionpack/lib/action_controller/request_forgery_protection.rb @@ -93,8 +93,12 @@ def verifiable_request_format? def form_authenticity_token @form_authenticity_token ||= if request_forgery_protection_options[:secret] authenticity_token_from_session_id - else + elsif session.respond_to?(:dbman) && session.dbman.respond_to?(:generate_digest) authenticity_token_from_cookie_session + elsif session.nil? + raise InvalidAuthenticityToken, "Request Forgery Protection requires a valid session. Use #allow_forgery_protection to disable it, or use a valid session." + else + raise InvalidAuthenticityToken, "No :secret given to the #protect_from_forgery call. Set that or use a session store capable of generating its own keys (Cookie Session Store)." end end diff --git a/actionpack/test/controller/request_forgery_protection_test.rb b/actionpack/test/controller/request_forgery_protection_test.rb index 2166b24aa6..0990d1b0c5 100644 --- a/actionpack/test/controller/request_forgery_protection_test.rb +++ b/actionpack/test/controller/request_forgery_protection_test.rb @@ -5,6 +5,61 @@ map.connect ':controller/:action/:id' end +# simulates cookie session store +class FakeSessionDbMan + def self.generate_digest(data) + Digest::SHA1.hexdigest("secure") + end +end + +# common controller actions +module RequestForgeryProtectionActions + def index + render :inline => "<%= form_tag('/') {} %>" + end + + def show_button + render :inline => "<%= button_to('New', '/') {} %>" + end + + def unsafe + render :text => 'pwn' + end + + def rescue_action(e) raise e end +end + +# sample controllers +class RequestForgeryProtectionController < ActionController::Base + include RequestForgeryProtectionActions + protect_from_forgery :only => :index, :secret => 'abc' +end + +class RequestForgeryProtectionWithoutSecretController < ActionController::Base + include RequestForgeryProtectionActions + protect_from_forgery +end + +# no token is given, assume the cookie store is used +class CsrfCookieMonsterController < ActionController::Base + include RequestForgeryProtectionActions + protect_from_forgery :only => :index +end + +class FreeCookieController < CsrfCookieMonsterController + self.allow_forgery_protection = false + + def index + render :inline => "<%= form_tag('/') {} %>" + end + + def show_button + render :inline => "<%= button_to('New', '/') {} %>" + end +end + +# common test methods + module RequestForgeryProtectionTests def teardown ActionController::Base.request_forgery_protection_token = nil @@ -85,26 +140,7 @@ def test_should_allow_delete_with_xml end end -module RequestForgeryProtectionActions - def index - render :inline => "<%= form_tag('/') {} %>" - end - - def show_button - render :inline => "<%= button_to('New', '/') {} %>" - end - - def unsafe - render :text => 'pwn' - end - - def rescue_action(e) raise e end -end - -class RequestForgeryProtectionController < ActionController::Base - include RequestForgeryProtectionActions - protect_from_forgery :only => :index, :secret => 'abc' -end +# OK let's get our test on class RequestForgeryProtectionControllerTest < Test::Unit::TestCase include RequestForgeryProtectionTests @@ -120,27 +156,22 @@ def session_id() '123' end end end -# no token is given, assume the cookie store is used -class CsrfCookieMonsterController < ActionController::Base - include RequestForgeryProtectionActions - protect_from_forgery :only => :index -end - -class FreeCookieController < CsrfCookieMonsterController - self.allow_forgery_protection = false - - def index - render :inline => "<%= form_tag('/') {} %>" +class RequestForgeryProtectionWithoutSecretControllerTest < Test::Unit::TestCase + def setup + @controller = RequestForgeryProtectionWithoutSecretController.new + @request = ActionController::TestRequest.new + @response = ActionController::TestResponse.new + class << @request.session + def session_id() '123' end + end + @token = OpenSSL::HMAC.hexdigest(OpenSSL::Digest::Digest.new('SHA1'), 'abc', '123') + ActionController::Base.request_forgery_protection_token = :authenticity_token end - def show_button - render :inline => "<%= button_to('New', '/') {} %>" - end -end - -class FakeSessionDbMan - def self.generate_digest(data) - Digest::SHA1.hexdigest("secure") + def test_should_raise_error_without_secret + assert_raises ActionController::InvalidAuthenticityToken do + get :index + end end end @@ -150,18 +181,17 @@ def setup @controller = CsrfCookieMonsterController.new @request = ActionController::TestRequest.new @response = ActionController::TestResponse.new - # simulate a cookie session store - @request.session.instance_variable_set(:@dbman, FakeSessionDbMan) class << @request.session - attr_reader :dbman + attr_accessor :dbman end + # simulate a cookie session store + @request.session.dbman = FakeSessionDbMan @token = Digest::SHA1.hexdigest("secure") ActionController::Base.request_forgery_protection_token = :authenticity_token end end class FreeCookieControllerTest < Test::Unit::TestCase - def setup @controller = FreeCookieController.new @request = ActionController::TestRequest.new @@ -184,5 +214,4 @@ def test_should_allow_all_methods_without_token assert_nothing_raised { send(method, :index)} end end - -end +end \ No newline at end of file diff --git a/railties/helpers/application.rb b/railties/helpers/application.rb index 043488903d..0a72bba2c9 100644 --- a/railties/helpers/application.rb +++ b/railties/helpers/application.rb @@ -3,5 +3,8 @@ class ApplicationController < ActionController::Base helper :all # include all helpers, all the time - protect_from_forgery # See ActionController::RequestForgeryProtection for details + + # See ActionController::RequestForgeryProtection for details + # If you're using the Cookie Session Store you can leave out the :secret + protect_from_forgery :secret => '<%= app_secret %>' end \ No newline at end of file diff --git a/railties/lib/rails_generator/generators/applications/app/app_generator.rb b/railties/lib/rails_generator/generators/applications/app/app_generator.rb index 9e0759a2b5..e78286495c 100644 --- a/railties/lib/rails_generator/generators/applications/app/app_generator.rb +++ b/railties/lib/rails_generator/generators/applications/app/app_generator.rb @@ -43,7 +43,7 @@ def manifest m.file "README", "README" # Application - m.template "helpers/application.rb", "app/controllers/application.rb", :assigns => { :app_name => @app_name } + m.template "helpers/application.rb", "app/controllers/application.rb", :assigns => { :app_name => @app_name, :app_secret => md5.hexdigest } m.template "helpers/application_helper.rb", "app/helpers/application_helper.rb" m.template "helpers/test_helper.rb", "test/test_helper.rb" -- GitLab