Skip to content

Commit

Permalink
Change the forgery token implementation to just be a simple random st…
Browse files Browse the repository at this point in the history 
…ring.

This deprecates the use of :secret and :digest which were only needed when we were hashing session ids.
  • Loading branch information
@NZKoz
NZKoz committed Nov 23, 2008
1 parent ed7549d commit 9fdb15e
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 127 deletions.
46 changes: 7 additions & 39 deletions actionpack/lib/action_controller/request_forgery_protection.rb
View file
Expand Up @@ -5,16 +5,14 @@ class InvalidAuthenticityToken < ActionControllerError #:nodoc:
module RequestForgeryProtection
def self.included(base)
base.class_eval do
class_inheritable_accessor :request_forgery_protection_options
self.request_forgery_protection_options = {}
helper_method :form_authenticity_token
helper_method :protect_against_forgery?
end
base.extend(ClassMethods)
end

# Protecting controller actions from CSRF attacks by ensuring that all forms are coming from the current web application, not a
# forged link from another site, is done by embedding a token based on the session (which an attacker wouldn't know) in all
# forged link from another site, is done by embedding a token based on a random string stored in the session (which an attacker wouldn't know) in all
# forms and Ajax requests generated by Rails and then verifying the authenticity of that token in the controller. Only
# HTML/JavaScript requests are checked, so this will not protect your XML API (presumably you'll have a different authentication
# scheme there anyway). Also, GET requests are not protected as these should be idempotent anyway.
Expand Down Expand Up @@ -57,26 +55,21 @@ module ClassMethods
# Example:
#
# class FooController < ApplicationController
# # uses the cookie session store (then you don't need a separate :secret)
# protect_from_forgery :except => :index
#
# # uses one of the other session stores that uses a session_id value.
# protect_from_forgery :secret => 'my-little-pony', :except => :index
#
# # you can disable csrf protection on controller-by-controller basis:
# skip_before_filter :verify_authenticity_token
# end
#
# Valid Options:
#
# * <tt>:only/:except</tt> - Passed to the <tt>before_filter</tt> call. Set which actions are verified.
# * <tt>:secret</tt> - Custom salt used to generate the <tt>form_authenticity_token</tt>.
# Leave this off if you are using the cookie session store.
# * <tt>:digest</tt> - Message digest used for hashing. Defaults to 'SHA1'.
def protect_from_forgery(options = {})
self.request_forgery_protection_token ||= :authenticity_token
before_filter :verify_authenticity_token, :only => options.delete(:only), :except => options.delete(:except)
request_forgery_protection_options.update(options)
if options[:secret] || options[:digest]
ActiveSupport::Deprecation.warn("protect_from_forgery only takes :only and :except options now. :digest and :secret have no effect", caller)
end
end
end

Expand All @@ -90,7 +83,7 @@ def verify_authenticity_token
#
# * is the format restricted? By default, only HTML and AJAX requests are checked.
# * is it a GET request? Gets should be safe and idempotent
# * Does the form_authenticity_token match the given _token value from the params?
# * Does the form_authenticity_token match the given token value from the params?
def verified_request?
!protect_against_forgery? ||
request.method == :get ||
Expand All @@ -105,34 +98,9 @@ def verifiable_request_format?
# Sets the token value for the current session. Pass a <tt>:secret</tt> option
# in +protect_from_forgery+ to add a custom salt to the hash.
def form_authenticity_token
@form_authenticity_token ||= if !session.respond_to?(:session_id)
raise InvalidAuthenticityToken, "Request Forgery Protection requires a valid session. Use #allow_forgery_protection to disable it, or use a valid session."
elsif request_forgery_protection_options[:secret]
authenticity_token_from_session_id
elsif session.respond_to?(:dbman) && session.dbman.respond_to?(:generate_digest)
authenticity_token_from_cookie_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

# Generates a unique digest using the session_id and the CSRF secret.
def authenticity_token_from_session_id
key = if request_forgery_protection_options[:secret].respond_to?(:call)
request_forgery_protection_options[:secret].call(@session)
else
request_forgery_protection_options[:secret]
end
digest = request_forgery_protection_options[:digest] ||= 'SHA1'
OpenSSL::HMAC.hexdigest(OpenSSL::Digest::Digest.new(digest), key.to_s, session.session_id.to_s)
end

# No secret was given, so assume this is a cookie session store.
def authenticity_token_from_cookie_session
session[:csrf_id] ||= CGI::Session.generate_unique_id
session.dbman.generate_digest(session[:csrf_id])
session[:_csrf_token] ||= ActiveSupport::SecureRandom.base64(32)
end

def protect_against_forgery?
allow_forgery_protection && request_forgery_protection_token
end
Expand Down
3 changes: 2 additions & 1 deletion actionpack/test/controller/render_test.rb
View file
Expand Up @@ -883,12 +883,13 @@ def test_render_xml_with_partial
end

def test_enum_rjs_test
ActiveSupport::SecureRandom.stubs(:base64).returns("asdf")
get :enum_rjs_test
body = %{
$$(".product").each(function(value, index) {
new Effect.Highlight(element,{});
new Effect.Highlight(value,{});
Sortable.create(value, {onUpdate:function(){new Ajax.Request('/test/order', {asynchronous:true, evalScripts:true, parameters:Sortable.serialize(value)})}});
Sortable.create(value, {onUpdate:function(){new Ajax.Request('/test/order', {asynchronous:true, evalScripts:true, parameters:Sortable.serialize(value) + '&authenticity_token=' + encodeURIComponent('asdf')})}});
new Draggable(value, {});
});
}.gsub(/^ /, '').strip
Expand Down
93 changes: 6 additions & 87 deletions actionpack/test/controller/request_forgery_protection_test.rb
View file
Expand Up @@ -5,13 +5,6 @@
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
Expand All @@ -35,30 +28,11 @@ def rescue_action(e) raise e 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

# sessions are turned off
class SessionOffController < ActionController::Base
protect_from_forgery :secret => 'foobar'
session :off
def rescue_action(e) raise e end
include RequestForgeryProtectionActions
end

class FreeCookieController < CsrfCookieMonsterController
class FreeCookieController < RequestForgeryProtectionController
self.allow_forgery_protection = false

def index
Expand Down Expand Up @@ -237,45 +211,9 @@ def setup
@request = ActionController::TestRequest.new
@request.format = :html
@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
end
@token = "cf50faa3fe97702ca1ae"

class RequestForgeryProtectionWithoutSecretControllerTest < ActionController::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 test_should_raise_error_without_secret
# assert_raises ActionController::InvalidAuthenticityToken do
# get :index
# end
# end
end

class CsrfCookieMonsterControllerTest < ActionController::TestCase
include RequestForgeryProtectionTests
def setup
@controller = CsrfCookieMonsterController.new
@request = ActionController::TestRequest.new
@response = ActionController::TestResponse.new
class << @request.session
attr_accessor :dbman
end
# simulate a cookie session store
@request.session.dbman = FakeSessionDbMan
@token = Digest::SHA1.hexdigest("secure")
ActiveSupport::SecureRandom.stubs(:base64).returns(@token)
ActionController::Base.request_forgery_protection_token = :authenticity_token
end
end
Expand All @@ -285,7 +223,9 @@ def setup
@controller = FreeCookieController.new
@request = ActionController::TestRequest.new
@response = ActionController::TestResponse.new
@token = OpenSSL::HMAC.hexdigest(OpenSSL::Digest::Digest.new('SHA1'), 'abc', '123')
@token = "cf50faa3fe97702ca1ae"

ActiveSupport::SecureRandom.stubs(:base64).returns(@token)
end

def test_should_not_render_form_with_token_tag
Expand All @@ -304,24 +244,3 @@ def test_should_allow_all_methods_without_token
end
end
end

class SessionOffControllerTest < ActionController::TestCase
def setup
@controller = SessionOffController.new
@request = ActionController::TestRequest.new
@response = ActionController::TestResponse.new
@token = OpenSSL::HMAC.hexdigest(OpenSSL::Digest::Digest.new('SHA1'), 'abc', '123')
end

# TODO: Rewrite this test.
# This test was passing but for the wrong reason.
# Sessions aren't really being turned off, so an exception was raised
# because sessions weren't on - not because the token didn't match.
#
# def test_should_raise_correct_exception
# @request.session = {} # session(:off) doesn't appear to work with controller tests
# assert_raises(ActionController::InvalidAuthenticityToken) do
# post :index, :authenticity_token => @token, :format => :html
# end
# end
end

0 comments on commit 9fdb15e

Please sign in to comment.