Cookie store and object instances in session behavior when instance class no longer exists

Today a colleague was playing in another branch trying the ruby-saml gem to play with SAML. When he was back to the master branch all requests failed for apparently no reason. This is related to this (it was trying to instantiate some OneLogin class which doesn't exist in master):

http://devblog.songkick.com/2012/10/24/get-your-objects-out-of-my-session/

I just think that it's too bad on Rails part to simply crash when it's unable to deserialize some value from the cookie. I noticed Rack would simply ignore them and return nil in such cases:

https://github.com/rack/rack/blob/1.6.4/lib/rack/session/cookie.rb#L66

Why not doing the same in Rails?

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/cookies.rb#L420

I ended up doing something like this in my application:

Rails.application.config.action_dispatch.cookies_serializer = :marshal

module RailsSerializerPatch
   protected

   def deserialize(name, value)
     super
   rescue
     puts "Failed to deserialize session value for #{name}: #{value}"
     nil
   end
end

ActionDispatch::Cookies::EncryptedCookieJar.prepend RailsSerializerPatch
ActionDispatch::Cookies::SignedCookieJar.prepend RailsSerializerPatch

I tried to prepend to SerializedCookieJars module but it didn't work and I have no idea why, but anyway...

Maybe it would be even better to delete that key from the session whenever such error happens.

Shouldn't Rails better handle such deserialization problems without crashing the whole application on every request? For example, even if I enable Devise sign-out through get requests the application would crash (respond with 500) before having the opportunity to clear up any cookies...

Or maybe Rails could simply remove all cookies when an error happens due to deserialization raising. Or at least make the desired behavior more easily configurable. What do you think?

Today a colleague was playing in another branch trying the ruby-saml gem to play with SAML. When he was back to the master branch all requests failed for apparently no reason. This is related to this (it was trying to instantiate some OneLogin class which doesn't exist in master):

http://devblog.songkick.com/2012/10/24/get-your-objects-out-of-my-session/

I just think that it's too bad on Rails part to simply crash when it's unable to deserialize some value from the cookie. I noticed Rack would simply ignore them and return nil in such cases:

https://github.com/rack/rack/blob/1.6.4/lib/rack/session/cookie.rb#L66

Why not doing the same in Rails?

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/cookies.rb#L420

I ended up doing something like this in my application:

Rails.application.config.action_dispatch.cookies_serializer = :marshal

module RailsSerializerPatch
protected

def deserialize(name, value)
   super
rescue
   puts "Failed to deserialize session value for #{name}: #{value}"
   nil
end
end

ActionDispatch::Cookies::EncryptedCookieJar.prepend RailsSerializerPatch
ActionDispatch::Cookies::SignedCookieJar.prepend RailsSerializerPatch

I tried to prepend to SerializedCookieJars module but it didn't work and I have no idea why, but anyway...

Maybe it would be even better to delete that key from the session whenever such error happens.

IMO this isn’t a good default behavior. Silently deleting the key - and muttering to STDOUT is pretty close to “silent” - means that if this issue is hit in production unexpectedly it is basically invisible.

Shouldn't Rails better handle such deserialization problems without crashing the whole application on every request? For example, even if I enable Devise sign-out through get requests the application would crash (respond with 500) before having the opportunity to clear up any cookies…

In the general case, if the session can’t be cleanly deserialized there’s very little that can be done. A specific application can make the call to "muddle through”, but that would be an unsafe default for some applications.

Or maybe Rails could simply remove all cookies when an error happens due to deserialization raising. Or at least make the desired behavior more easily configurable. What do you think?

+1 to making it configurable.

—Matt Jones

Today a colleague was playing in another branch trying the ruby-saml gem to play with SAML. When he was back to the master branch all requests failed for apparently no reason. This is related to this (it was trying to instantiate some OneLogin class which doesn't exist in master):

http://devblog.songkick.com/2012/10/24/get-your-objects-out-of-my-session/

I just think that it's too bad on Rails part to simply crash when it's unable to deserialize some value from the cookie. I noticed Rack would simply ignore them and return nil in such cases:

https://github.com/rack/rack/blob/1.6.4/lib/rack/session/cookie.rb#L66

Why not doing the same in Rails?

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/cookies.rb#L420

I ended up doing something like this in my application:

Rails.application.config.action_dispatch.cookies_serializer = :marshal

module RailsSerializerPatch
  protected

  def deserialize(name, value)
    super
  rescue
    puts "Failed to deserialize session value for #{name}: #{value}"
    nil
  end
end

ActionDispatch::Cookies::EncryptedCookieJar.prepend RailsSerializerPatch
ActionDispatch::Cookies::SignedCookieJar.prepend RailsSerializerPatch

I tried to prepend to SerializedCookieJars module but it didn't work and I have no idea why, but anyway...

Maybe it would be even better to delete that key from the session whenever such error happens.

IMO this isn’t a good default behavior. Silently deleting the key - and muttering to STDOUT is pretty close to “silent” - means that if this issue is hit in production unexpectedly it is basically invisible.

I agree that even though I'm okay with this behavior for my application, Rails should provide a better default.

But I don't think the current behavior is a good default either. I think it's much worse than silently ignore values it can't deserialize.

Currently, the user would no longer be able to use the application or even to reauthenticate or clean up his cookies if some middleware will try to load all values, like Devise does... It will simply always raise 500 for the user on all requests until the user manually clear up his cookies. This is much worse than silently ignoring values that can't be deserialized.

Shouldn't Rails better handle such deserialization problems without crashing the whole application on every request? For example, even if I enable Devise sign-out through get requests the application would crash (respond with 500) before having the opportunity to clear up any cookies…

In the general case, if the session can’t be cleanly deserialized there’s very little that can be done.

I wouldn't say that, and I've even provided some suggestions. If silently ignoring such values is not desired, than maybe Rails should simply render an error page, so that it gets noticed and then clear up the current session and force the user to re-authenticate rather than failing forever. This would be certainly much better than the current behavior.

  A specific application can make the call to "muddle through”, but that would be an unsafe default for some applications.

Or maybe Rails could simply remove all cookies when an error happens due to deserialization raising. Or at least make the desired behavior more easily configurable. What do you think?

+1 to making it configurable.

—Matt Jones

I may try to get some time to work on this once I know what would be the desired options bundled with Rails. Maybe something like:

Rails.application.config.action_dispatch.on_cookies_serializer_load_error = option # please suggest a better option name

Where option could be :error_once_and_clear_cookies (maybe the default), :silently_ignore, :raise (current behavior) or maybe a hash like: {clear_cookies_and_redirect_to: '/'}

Sounds good to you?

Best,
Rodrigo.