Make sure that cookie sessions use a secret that is at least 30 chars in length. [Koz]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@8184 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

4 files changed, 40 insertions, 4 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index f6d0a8be81..e197e334aa 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Make sure that cookie sessions use a secret that is at least 30 chars in length. [Koz]
+
 * Fixed that partial rendering should look at the type of the first render to determine its own type if no other clues are available (like when using text.plain.erb as the extension in AM) #10130 [java]
 
 * Fixed that has_many :through associations should render as collections too #9051 [mathie/danger]
diff --git a/actionpack/lib/action_controller/session/cookie_store.rb b/actionpack/lib/action_controller/session/cookie_store.rb
index 6de4d88ca0..81092882f7 100644
--- a/actionpack/lib/action_controller/session/cookie_store.rb
+++ b/actionpack/lib/action_controller/session/cookie_store.rb
@@ -53,9 +53,7 @@ class CGI::Session::CookieStore
     end
 
     # The secret option is required.
-    if options['secret'].blank?
-      raise ArgumentError, 'A secret is required to generate an integrity hash for cookie session data. Use config.action_controller.session = { :session_key => "_myapp_session", :secret => "some secret phrase" } in config/environment.rb'
-    end
+    ensure_secret_secure(options['secret'])
 
     # Keep the session and its secret on hand so we can read and write cookies.
     @session, @secret = session, options['secret']
@@ -78,6 +76,22 @@ class CGI::Session::CookieStore
     options['no_cookies'] = true
   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.action_controller.session = { :session_key => "_myapp_session", :secret => "some secret phrase" } in config/environment.rb'
+    end
+
+    if secret.length < 30
+      raise ArgumentError, "Secret should be something secure, like #{CGI::Session.generate_unique_id}.  The value you provided: [#{secret}]"
+    end
+  end
+
   # Restore session data from the cookie.
   def restore
     @original = read_cookie
diff --git a/actionpack/test/controller/session/cookie_store_test.rb b/actionpack/test/controller/session/cookie_store_test.rb
index 0084f35dea..b2655c72d9 100755
--- a/actionpack/test/controller/session/cookie_store_test.rb
+++ b/actionpack/test/controller/session/cookie_store_test.rb
@@ -4,6 +4,19 @@ require 'action_controller/cgi_ext'
 
 require 'stringio'
 
+
+class CGI::Session::CookieStore
+  def ensure_secret_secure_with_test_hax(secret)
+    if secret == CookieStoreTest.default_session_options['secret']
+      return true
+    else
+      ensure_secret_secure_without_test_hax(secret)
+    end
+  end
+  alias_method_chain :ensure_secret_secure, :test_hax
+end
+
+
 # Expose for tests.
 class CGI
   attr_reader :output_cookies, :output_hidden
@@ -49,6 +62,12 @@ class CookieStoreTest < Test::Unit::TestCase
     end
   end
 
+  def test_raises_argument_error_if_secret_is_probably_insecure
+    ["password", "secret", "12345678901234567890123456789"].each do |blank|
+      assert_raise(ArgumentError, blank.inspect) { new_session 'secret' => blank }
+    end
+  end
+
   def test_reconfigures_session_to_omit_id_cookie_and_hidden_field
     new_session do |session|
       assert_equal true, @options['no_hidden']
diff --git a/actionpack/test/controller/session_fixation_test.rb b/actionpack/test/controller/session_fixation_test.rb
index 0b0dce770e..34a7aa2d0d 100644
--- a/actionpack/test/controller/session_fixation_test.rb
+++ b/actionpack/test/controller/session_fixation_test.rb
@@ -1,5 +1,6 @@
 require File.dirname(__FILE__) + '/../abstract_unit'
 
+
 class SessionFixationTest < Test::Unit::TestCase
   class MockCGI < CGI #:nodoc:
     attr_accessor :stdoutput, :env_table
@@ -12,7 +13,7 @@ class SessionFixationTest < Test::Unit::TestCase
   end
 
   class TestController < ActionController::Base
-    session :session_key => '_myapp_session_id', :secret => 'secret', :except => :default_session_key
+    session :session_key => '_myapp_session_id', :secret => CGI::Session.generate_unique_id, :except => :default_session_key
     session :cookie_only => false, :only => :allow_session_fixation
 
     def default_session_key