Fixed that an ArgumentError is thrown when request.session_options[:id] is read in the following scenario: when the cookie store is used, and the session contains a serialized object of an unloaded class, and no session data accesses have occurred yet. Pushed the stale_session_check responsibility out of the SessionHash and down into the session store, closer to where the deserialization actually occurs. Added some test coverage for this case and others related to deserialization of unloaded types.

[#4938]

Signed-off-by: José Valim <jose.valim@gmail.com>

6 files changed, 113 insertions, 36 deletions

diff --git a/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb b/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb
index bc1d6fab83..08bc80dbc2 100644
--- a/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb
+++ b/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb
@@ -88,9 +88,7 @@ module ActionDispatch
 
         def exists?
           return @exists if instance_variable_defined?(:@exists)
-          stale_session_check! do
-            @exists = @by.send(:exists?, @env)
-          end
+          @exists = @by.send(:exists?, @env)
         end
 
         def loaded?
@@ -115,29 +113,12 @@ module ActionDispatch
           end
 
           def load!
-            stale_session_check! do
-              id, session = @by.send(:load_session, @env)
-              @env[ENV_SESSION_OPTIONS_KEY][:id] = id
-              replace(session.stringify_keys)
-              @loaded = true
-            end
+            id, session = @by.send(:load_session, @env)
+            @env[ENV_SESSION_OPTIONS_KEY][:id] = id
+            replace(session.stringify_keys)
+            @loaded = true
           end
 
-          def stale_session_check!
-            yield
-          rescue ArgumentError => argument_error
-            if argument_error.message =~ %r{undefined class/module ([\w:]*\w)}
-              begin
-                # Note that the regexp does not allow $1 to end with a ':'
-                $1.constantize
-              rescue LoadError, NameError => const_error
-                raise ActionDispatch::Session::SessionRestoreError, "Session contains objects whose class definition isn't available.\nRemember to require the classes for all objects kept in the session.\n(Original exception: #{const_error.message} [#{const_error.class}])\n"
-              end
-              retry
-            else
-              raise
-            end
-          end
       end
 
       DEFAULT_OPTIONS = {
@@ -204,16 +185,20 @@ module ActionDispatch
         end
 
         def load_session(env)
-          sid = current_session_id(env)
-          sid, session = get_session(env, sid)
-          [sid, session]
+          stale_session_check! do
+            sid = current_session_id(env)
+            sid, session = get_session(env, sid)
+            [sid, session]
+          end
         end
 
         def extract_session_id(env)
-          request = ActionDispatch::Request.new(env)
-          sid = request.cookies[@key]
-          sid ||= request.params[@key] unless @cookie_only
-          sid
+          stale_session_check! do
+            request = ActionDispatch::Request.new(env)
+            sid = request.cookies[@key]
+            sid ||= request.params[@key] unless @cookie_only
+            sid
+          end
         end
 
         def current_session_id(env)
@@ -229,6 +214,22 @@ module ActionDispatch
           end
         end
 
+        def stale_session_check!
+          yield
+        rescue ArgumentError => argument_error
+          if argument_error.message =~ %r{undefined class/module ([\w:]*\w)}
+            begin
+              # Note that the regexp does not allow $1 to end with a ':'
+              $1.constantize
+            rescue LoadError, NameError => const_error
+              raise ActionDispatch::Session::SessionRestoreError, "Session contains objects whose class definition isn't available.\nRemember to require the classes for all objects kept in the session.\n(Original exception: #{const_error.message} [#{const_error.class}])\n"
+            end
+            retry
+          else
+            raise
+          end
+        end
+
         def exists?(env)
           current_session_id(env).present?
         end
@@ -245,7 +246,6 @@ module ActionDispatch
         def destroy(env)
           raise '#destroy needs to be implemented.'
         end
-
     end
   end
 end
diff --git a/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb b/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb
index 7ebd532f4a..dce47c63bc 100644
--- a/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb
+++ b/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb
@@ -63,11 +63,13 @@ module ActionDispatch
 
         def unpacked_cookie_data(env)
           env["action_dispatch.request.unsigned_session_cookie"] ||= begin
-            request = ActionDispatch::Request.new(env)
-            if data = request.cookie_jar.signed[@key]
-              data.stringify_keys!
+            stale_session_check! do
+              request = ActionDispatch::Request.new(env)
+              if data = request.cookie_jar.signed[@key]
+                data.stringify_keys!
+              end
+              data || {}
             end
-            data || {}
           end
         end
 
diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb
index 84c5395610..2640e96453 100644
--- a/actionpack/test/abstract_unit.rb
+++ b/actionpack/test/abstract_unit.rb
@@ -202,6 +202,21 @@ class ActionController::IntegrationTest < ActiveSupport::TestCase
     self.class.app = old_app
     silence_warnings { Object.const_set(:SharedTestRoutes, old_routes) }
   end
+
+  def with_autoload_path(path)
+    path = File.join(File.dirname(__FILE__), "fixtures", path)  
+    if ActiveSupport::Dependencies.autoload_paths.include?(path)
+      yield
+    else
+      begin
+        ActiveSupport::Dependencies.autoload_paths << path
+        yield
+      ensure
+        ActiveSupport::Dependencies.autoload_paths.reject! {|p| p == path}
+        ActiveSupport::Dependencies.clear
+      end
+    end
+  end
 end
 
 # Temporary base class
diff --git a/actionpack/test/dispatch/session/cookie_store_test.rb b/actionpack/test/dispatch/session/cookie_store_test.rb
index 6aca22b456..fd63f5ad5e 100644
--- a/actionpack/test/dispatch/session/cookie_store_test.rb
+++ b/actionpack/test/dispatch/session/cookie_store_test.rb
@@ -96,6 +96,31 @@ class CookieStoreTest < ActionController::IntegrationTest
     end
   end
 
+  # {:foo=>#<SessionAutoloadTest::Foo bar:"baz">, :session_id=>"ce8b0752a6ab7c7af3cdb8a80e6b9e46"}
+  SignedSerializedCookie = "BAh7BzoIZm9vbzodU2Vzc2lvbkF1dG9sb2FkVGVzdDo6Rm9vBjoJQGJhciIIYmF6Og9zZXNzaW9uX2lkIiVjZThiMDc1MmE2YWI3YzdhZjNjZGI4YTgwZTZiOWU0Ng==--2bf3af1ae8bd4e52b9ac2099258ace0c380e601c"
+
+  def test_deserializes_unloaded_classes_on_get_id
+    with_test_route_set do
+      with_autoload_path "session_autoload_test" do
+        cookies[SessionKey] = SignedSerializedCookie
+        get '/get_session_id'
+        assert_response :success
+        assert_equal 'id: ce8b0752a6ab7c7af3cdb8a80e6b9e46', response.body, "should auto-load unloaded class"
+      end
+    end
+  end  
+  
+  def test_deserializes_unloaded_classes_on_get_value
+    with_test_route_set do
+      with_autoload_path "session_autoload_test" do 
+        cookies[SessionKey] = SignedSerializedCookie
+        get '/get_session_value'
+        assert_response :success
+        assert_equal 'foo: #<SessionAutoloadTest::Foo bar:"baz">', response.body, "should auto-load unloaded class"
+      end
+    end
+  end
+
   def test_close_raises_when_data_overflows
     with_test_route_set do
       assert_raise(ActionDispatch::Cookies::CookieOverflow) {
@@ -247,4 +272,5 @@ class CookieStoreTest < ActionController::IntegrationTest
         yield
       end
     end
+
 end
diff --git a/actionpack/test/dispatch/session/mem_cache_store_test.rb b/actionpack/test/dispatch/session/mem_cache_store_test.rb
index d388992b98..9bd6f9b8c4 100644
--- a/actionpack/test/dispatch/session/mem_cache_store_test.rb
+++ b/actionpack/test/dispatch/session/mem_cache_store_test.rb
@@ -11,6 +11,11 @@ class MemCacheStoreTest < ActionController::IntegrationTest
       session[:foo] = "bar"
       head :ok
     end
+    
+    def set_serialized_session_value
+      session[:foo] = SessionAutoloadTest::Foo.new
+      head :ok
+    end
 
     def get_session_value
       render :text => "foo: #{session[:foo].inspect}"
@@ -117,6 +122,25 @@ class MemCacheStoreTest < ActionController::IntegrationTest
       end
     end
 
+    def test_deserializes_unloaded_class
+      with_test_route_set do
+        with_autoload_path "session_autoload_test" do
+          get '/set_serialized_session_value'
+          assert_response :success
+          assert cookies['_session_id']
+        end
+        with_autoload_path "session_autoload_test" do
+          get '/get_session_id'
+          assert_response :success
+        end
+        with_autoload_path "session_autoload_test" do
+          get '/get_session_value'
+          assert_response :success
+          assert_equal 'foo: #<SessionAutoloadTest::Foo bar:"baz">', response.body, "should auto-load unloaded class"
+        end
+      end
+    end
+
     def test_doesnt_write_session_cookie_if_session_id_is_already_exists
       with_test_route_set do
         get '/set_session_value'
diff --git a/actionpack/test/fixtures/session_autoload_test/session_autoload_test/foo.rb b/actionpack/test/fixtures/session_autoload_test/session_autoload_test/foo.rb
new file mode 100644
index 0000000000..4ee7a24561
--- /dev/null
+++ b/actionpack/test/fixtures/session_autoload_test/session_autoload_test/foo.rb
@@ -0,0 +1,10 @@
+module SessionAutoloadTest
+  class Foo
+    def initialize(bar='baz')
+      @bar = bar
+    end
+    def inspect
+      "#<#{self.class} bar:#{@bar.inspect}>"
+    end
+  end
+end