From 25f7c030e4ea440ea6c2a84c92118299753392d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 18 May 2010 01:43:06 +0200 Subject: [PATCH] Simplify cookie_store by simply relying on cookies.signed. --- .../lib/action_dispatch/middleware/cookies.rb | 32 +++++- .../middleware/session/cookie_store.rb | 107 +++--------------- actionpack/test/abstract_unit.rb | 1 + actionpack/test/controller/cookie_test.rb | 55 ++++++++- actionpack/test/controller/flash_test.rb | 11 +- .../dispatch/session/cookie_store_test.rb | 66 ++++------- .../lib/rails/application/configuration.rb | 8 +- .../test/application/configuration_test.rb | 4 +- railties/test/application/middleware_test.rb | 5 +- 9 files changed, 138 insertions(+), 151 deletions(-) diff --git a/actionpack/lib/action_dispatch/middleware/cookies.rb b/actionpack/lib/action_dispatch/middleware/cookies.rb index 42ab1d1ebb383..1e49a307ed8ce 100644 --- a/actionpack/lib/action_dispatch/middleware/cookies.rb +++ b/actionpack/lib/action_dispatch/middleware/cookies.rb @@ -52,6 +52,9 @@ def cookie_jar # * :httponly - Whether this cookie is accessible via scripting or # only HTTP. Defaults to +false+. class Cookies + # Raised when storing more than 4K of session data. + class CookieOverflow < StandardError; end + class CookieJar < Hash #:nodoc: def self.build(request) secret = request.env["action_dispatch.secret_token"] @@ -166,8 +169,11 @@ def method_missing(method, *arguments, &block) end class SignedCookieJar < CookieJar #:nodoc: + MAX_COOKIE_SIZE = 4096 # Cookies can typically store 4096 bytes. + SECRET_MIN_LENGTH = 30 # Characters + def initialize(parent_jar, secret) - raise "You must set config.secret_token in your app's config" if secret.blank? + ensure_secret_secure(secret) @parent_jar = parent_jar @verifier = ActiveSupport::MessageVerifier.new(secret) end @@ -176,6 +182,8 @@ def [](name) if signed_message = @parent_jar[name] @verifier.verify(signed_message) end + rescue ActiveSupport::MessageVerifier::InvalidSignature + nil end def []=(key, options) @@ -186,12 +194,34 @@ def []=(key, options) options = { :value => @verifier.generate(options) } end + raise CookieOverflow if options[:value].size > MAX_COOKIE_SIZE @parent_jar[key] = options end def method_missing(method, *arguments, &block) @parent_jar.send(method, *arguments, &block) end + + protected + + # To prevent users from using something insecure like "Password" we make sure that the + # secret they've provided is at least 30 characters in length. + def ensure_secret_secure(secret) + if secret.blank? + raise ArgumentError, "A secret is required to generate an " + + "integrity hash for cookie session data. Use " + + "config.secret_token = \"some secret phrase of at " + + "least #{SECRET_MIN_LENGTH} characters\"" + + "in config/application.rb" + end + + if secret.length < SECRET_MIN_LENGTH + raise ArgumentError, "Secret should be something secure, " + + "like \"#{ActiveSupport::SecureRandom.hex(16)}\". The value you " + + "provided, \"#{secret}\", is shorter than the minimum length " + + "of #{SECRET_MIN_LENGTH} characters" + end + end end def initialize(app) diff --git a/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb b/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb index 7114f420038cd..0c1712bf84a52 100644 --- a/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb +++ b/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb @@ -1,3 +1,4 @@ +require 'action_dispatch/middleware/cookies' require 'active_support/core_ext/hash/keys' require 'active_support/core_ext/object/blank' @@ -39,10 +40,6 @@ module Session # # Note that changing digest or secret invalidates all existing sessions! class CookieStore - # Cookies can typically store 4096 bytes. - MAX = 4096 - SECRET_MIN_LENGTH = 30 # characters - DEFAULT_OPTIONS = { :key => '_session_id', :domain => nil, @@ -54,7 +51,7 @@ class CookieStore class OptionsHash < Hash def initialize(by, env, default_options) @session_data = env[CookieStore::ENV_SESSION_KEY] - default_options.each { |key, value| self[key] = value } + merge!(default_options) end def [](key) @@ -64,14 +61,12 @@ def [](key) ENV_SESSION_KEY = "rack.session".freeze ENV_SESSION_OPTIONS_KEY = "rack.session.options".freeze - HTTP_SET_COOKIE = "Set-Cookie".freeze - - # Raised when storing more than 4K of session data. - class CookieOverflow < StandardError; end def initialize(app, options = {}) # Process legacy CGI options + # TODO Refactor and deprecate me options = options.symbolize_keys + if options.has_key?(:session_path) options[:path] = options.delete(:session_path) end @@ -88,15 +83,7 @@ def initialize(app, options = {}) ensure_session_key(options[:key]) @key = options.delete(:key).freeze - # The secret option is required. - ensure_secret_secure(options[:secret]) - @secret = options.delete(:secret).freeze - - @digest = options.delete(:digest) || 'SHA1' - @verifier = verifier_for(@secret, @digest) - @default_options = DEFAULT_OPTIONS.merge(options).freeze - freeze end @@ -111,66 +98,32 @@ def call(env) if !session_data.is_a?(AbstractStore::SessionHash) || session_data.send(:loaded?) || options[:expire_after] session_data.send(:load!) if session_data.is_a?(AbstractStore::SessionHash) && !session_data.send(:loaded?) - session_data = marshal(session_data.to_hash) - - raise CookieOverflow if session_data.size > MAX + session_data = persistent_session_id!(session_data.to_hash) - cookie = Hash.new - cookie[:value] = session_data + cookie = { :value => session_data } unless options[:expire_after].nil? - cookie[:expires] = Time.now + options[:expire_after] + cookie[:expires] = Time.now + options.delete(:expire_after) end - cookie = build_cookie(@key, cookie.merge(options)) - unless headers[HTTP_SET_COOKIE].blank? - headers[HTTP_SET_COOKIE] << "\n#{cookie}" - else - headers[HTTP_SET_COOKIE] = cookie - end + request = ActionDispatch::Request.new(env) + request.cookie_jar.signed[@key] = cookie.merge!(options) end [status, headers, body] end private - # Should be in Rack::Utils soon - def build_cookie(key, value) - case value - when Hash - domain = "; domain=" + value[:domain] if value[:domain] - path = "; path=" + value[:path] if value[:path] - # According to RFC 2109, we need dashes here. - # N.B.: cgi.rb uses spaces... - expires = "; expires=" + value[:expires].clone.gmtime. - strftime("%a, %d-%b-%Y %H:%M:%S GMT") if value[:expires] - secure = "; secure" if value[:secure] - httponly = "; HttpOnly" if value[:httponly] - value = value[:value] - end - value = [value] unless Array === value - cookie = Rack::Utils.escape(key) + "=" + - value.map { |v| Rack::Utils.escape(v) }.join("&") + - "#{domain}#{path}#{expires}#{secure}#{httponly}" - end def load_session(env) - request = Rack::Request.new(env) - session_data = request.cookies[@key] - data = unmarshal(session_data) || persistent_session_id!({}) + request = ActionDispatch::Request.new(env) + data = request.cookie_jar.signed[@key] + data = persistent_session_id!(data || {}) data.stringify_keys! [data["session_id"], data] end - # Marshal a session hash into safe cookie data. Include an integrity hash. - def marshal(session) - @verifier.generate(persistent_session_id!(session)) - end - - # Unmarshal cookie data to a hash and verify its integrity. - def unmarshal(cookie) - persistent_session_id!(@verifier.verify(cookie)) if cookie - rescue ActiveSupport::MessageVerifier::InvalidSignature - nil + def generate_sid + ActiveSupport::SecureRandom.hex(16) end def ensure_session_key(key) @@ -182,38 +135,6 @@ def ensure_session_key(key) end end - # To prevent users from using something insecure like "Password" we make sure that the - # secret they've provided is at least 30 characters in length. - def ensure_secret_secure(secret) - # There's no way we can do this check if they've provided a proc for the - # secret. - return true if secret.is_a?(Proc) - - if secret.blank? - raise ArgumentError, "A secret is required to generate an " + - "integrity hash for cookie session data. Use " + - "config.secret_token = \"some secret phrase of at " + - "least #{SECRET_MIN_LENGTH} characters\"" + - "in config/application.rb" - end - - if secret.length < SECRET_MIN_LENGTH - raise ArgumentError, "Secret should be something secure, " + - "like \"#{ActiveSupport::SecureRandom.hex(16)}\". The value you " + - "provided, \"#{secret}\", is shorter than the minimum length " + - "of #{SECRET_MIN_LENGTH} characters" - end - end - - def verifier_for(secret, digest) - key = secret.respond_to?(:call) ? secret.call : secret - ActiveSupport::MessageVerifier.new(key, digest) - end - - def generate_sid - ActiveSupport::SecureRandom.hex(16) - end - def persistent_session_id!(data) (data ||= {}).merge!(inject_persistent_session_id(data)) end diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index 479e62b23d8a9..d2e5d2e9655f2 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -162,6 +162,7 @@ def self.build_app(routes = nil) middleware.use "ActionDispatch::Cookies" middleware.use "ActionDispatch::Flash" middleware.use "ActionDispatch::Head" + yield(middleware) if block_given? end end diff --git a/actionpack/test/controller/cookie_test.rb b/actionpack/test/controller/cookie_test.rb index 4971866e7ced8..f65eda5c69e9b 100644 --- a/actionpack/test/controller/cookie_test.rb +++ b/actionpack/test/controller/cookie_test.rb @@ -58,6 +58,17 @@ def set_signed_cookie head :ok end + def raise_data_overflow + cookies.signed[:foo] = 'bye!' * 1024 + head :ok + end + + def tampered_cookies + cookies[:tampered] = "BAh7BjoIZm9vIghiYXI%3D--123456780" + cookies.signed[:tampered] + head :ok + end + def set_permanent_signed_cookie cookies.permanent.signed[:remember_me] = 100 head :ok @@ -74,7 +85,7 @@ def delete_and_set_cookie def setup super - @request.env["action_dispatch.secret_token"] = "thisISverySECRET123" + @request.env["action_dispatch.secret_token"] = "b3c631c314c0bbca50c1b2843150fe33" @request.host = "www.nextangle.com" end @@ -163,6 +174,48 @@ def test_delete_and_set_cookie assert_equal({"user_name" => "david"}, @response.cookies) end + def test_raise_data_overflow + assert_raise(ActionDispatch::Cookies::CookieOverflow) do + get :raise_data_overflow + end + end + + def test_tampered_cookies + assert_nothing_raised do + get :tampered_cookies + assert_response :success + end + end + + def test_raises_argument_error_if_missing_secret + assert_raise(ArgumentError, nil.inspect) { + @request.env["action_dispatch.secret_token"] = nil + get :set_signed_cookie + } + + assert_raise(ArgumentError, ''.inspect) { + @request.env["action_dispatch.secret_token"] = "" + get :set_signed_cookie + } + end + + def test_raises_argument_error_if_secret_is_probably_insecure + assert_raise(ArgumentError, "password".inspect) { + @request.env["action_dispatch.secret_token"] = "password" + get :set_signed_cookie + } + + assert_raise(ArgumentError, "secret".inspect) { + @request.env["action_dispatch.secret_token"] = "secret" + get :set_signed_cookie + } + + assert_raise(ArgumentError, "12345678901234567890123456789".inspect) { + @request.env["action_dispatch.secret_token"] = "12345678901234567890123456789" + get :set_signed_cookie + } + end + private def assert_cookie_header(expected) header = @response.headers["Set-Cookie"] diff --git a/actionpack/test/controller/flash_test.rb b/actionpack/test/controller/flash_test.rb index c662ce264bcc8..01c8fd90a510f 100644 --- a/actionpack/test/controller/flash_test.rb +++ b/actionpack/test/controller/flash_test.rb @@ -237,10 +237,19 @@ def test_flash end private + + # Overwrite get to send SessionSecret in env hash + def get(path, parameters = nil, env = {}) + env["action_dispatch.secret_token"] ||= SessionSecret + super + end + def with_test_route_set with_routing do |set| set.draw do |map| - match ':action', :to => ActionDispatch::Session::CookieStore.new(FlashIntegrationTest::TestController, :key => FlashIntegrationTest::SessionKey, :secret => FlashIntegrationTest::SessionSecret) + match ':action', :to => ActionDispatch::Session::CookieStore.new( + FlashIntegrationTest::TestController, :key => SessionKey, :secret => SessionSecret + ) end yield end diff --git a/actionpack/test/dispatch/session/cookie_store_test.rb b/actionpack/test/dispatch/session/cookie_store_test.rb index d2c1758af1c82..a6cdbf8032d75 100644 --- a/actionpack/test/dispatch/session/cookie_store_test.rb +++ b/actionpack/test/dispatch/session/cookie_store_test.rb @@ -55,42 +55,13 @@ def test_raises_argument_error_if_missing_session_key } end - def test_raises_argument_error_if_missing_secret - assert_raise(ArgumentError, nil.inspect) { - ActionDispatch::Session::CookieStore.new(nil, - :key => SessionKey, :secret => nil) - } - - assert_raise(ArgumentError, ''.inspect) { - ActionDispatch::Session::CookieStore.new(nil, - :key => SessionKey, :secret => '') - } - end - - def test_raises_argument_error_if_secret_is_probably_insecure - assert_raise(ArgumentError, "password".inspect) { - ActionDispatch::Session::CookieStore.new(nil, - :key => SessionKey, :secret => "password") - } - - assert_raise(ArgumentError, "secret".inspect) { - ActionDispatch::Session::CookieStore.new(nil, - :key => SessionKey, :secret => "secret") - } - - assert_raise(ArgumentError, "12345678901234567890123456789".inspect) { - ActionDispatch::Session::CookieStore.new(nil, - :key => SessionKey, :secret => "12345678901234567890123456789") - } - end - def test_setting_session_value with_test_route_set do get '/set_session_value' assert_response :success assert_equal "_myapp_session=#{response.body}; path=/; HttpOnly", headers['Set-Cookie'] - end + end end def test_getting_session_value @@ -99,7 +70,7 @@ def test_getting_session_value get '/get_session_value' assert_response :success assert_equal 'foo: "bar"', response.body - end + end end def test_getting_session_id @@ -127,7 +98,7 @@ def test_disregards_tampered_sessions def test_close_raises_when_data_overflows with_test_route_set do - assert_raise(ActionDispatch::Session::CookieStore::CookieOverflow) { + assert_raise(ActionDispatch::Cookies::CookieOverflow) { get '/raise_data_overflow' } end @@ -209,30 +180,33 @@ def test_session_store_with_expire_after get '/no_session_access' assert_response :success - # Mystery bug that came up in 2.3 as well. What is this trying to test?! - # assert_equal "_myapp_session=#{cookie_body}; path=/; expires=#{expected_expiry}; HttpOnly", - # headers['Set-Cookie'] + assert_equal "_myapp_session=#{cookie_body}; path=/; expires=#{expected_expiry}; HttpOnly", + headers['Set-Cookie'] end end private + + # Overwrite get to send SessionSecret in env hash + def get(path, parameters = nil, env = {}) + env["action_dispatch.secret_token"] ||= SessionSecret + super + end + def with_test_route_set(options = {}) with_routing do |set| set.draw do |map| match ':action', :to => ::CookieStoreTest::TestController end - options = {:key => SessionKey, :secret => SessionSecret}.merge(options) - @app = ActionDispatch::Session::CookieStore.new(set, options) + + options = { :key => SessionKey, :secret => SessionSecret }.merge!(options) + + @app = self.class.build_app(set) do |middleware| + middleware.use ActionDispatch::Session::CookieStore, options + middleware.delete "ActionDispatch::ShowExceptions" + end + yield end end - - def unmarshal_session(cookie_string) - session = Rack::Utils.parse_query(cookie_string, ';,').inject({}) {|h,(k,v)| - h[k] = Array === v ? v.first : v - h - }[SessionKey] - verifier = ActiveSupport::MessageVerifier.new(SessionSecret, 'SHA1') - verifier.verify(session) - end end diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index 8afe423973369..1b8af370f765e 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -11,7 +11,8 @@ class Configuration < ::Rails::Engine::Configuration :encoding, :consider_all_requests_local, :dependency_loading, :filter_parameters, :log_level, :logger, :metals, :plugins, :preload_frameworks, :reload_engines, :reload_plugins, - :secret_token, :serve_static_assets, :time_zone, :whiny_nils + :secret_token, :serve_static_assets, :session_options, + :time_zone, :whiny_nils def initialize(*) super @@ -138,11 +139,6 @@ def session_store(*args) end end - def session_options - return @session_options unless @session_store == :cookie_store - @session_options.merge(:secret => @secret_token) - end - protected def default_middleware_stack diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index c08bd2ef2231f..9928ee2c52aea 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -196,7 +196,7 @@ def teardown test "config.secret_token is sent in env" do make_basic_app do |app| - app.config.secret_token = 'ThisIsASECRET123' + app.config.secret_token = 'b3c631c314c0bbca50c1b2843150fe33' app.config.session_store :disabled end @@ -208,7 +208,7 @@ def index end get "/" - assert_equal 'ThisIsASECRET123', last_response.body + assert_equal 'b3c631c314c0bbca50c1b2843150fe33', last_response.body end test "protect from forgery is the default in a new app" do diff --git a/railties/test/application/middleware_test.rb b/railties/test/application/middleware_test.rb index d08f04bddbdfc..617525bf78230 100644 --- a/railties/test/application/middleware_test.rb +++ b/railties/test/application/middleware_test.rb @@ -175,7 +175,10 @@ def middleware def remote_ip(env = {}) remote_ip = nil - env = Rack::MockRequest.env_for("/").merge(env).merge('action_dispatch.show_exceptions' => false) + env = Rack::MockRequest.env_for("/").merge(env).merge!( + 'action_dispatch.show_exceptions' => false, + 'action_dispatch.secret_token' => 'b3c631c314c0bbca50c1b2843150fe33' + ) endpoint = Proc.new do |e| remote_ip = ActionDispatch::Request.new(e).remote_ip