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

[Rodrigo Rosenfeld Rosas]

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?

[Matt Jones]

On Jul 15, 2015, at 11:06 AM, Rodrigo Rosenfeld Rosas <rr.r...@gmail.com> wrote:

> 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

[Rodrigo Rosenfeld Rosas]

On 16-07-2015 09:34, Matt Jones wrote:
> On Jul 15, 2015, at 11:06 AM, Rodrigo Rosenfeld Rosas <rr.r...@gmail.com> wrote:
>
>> 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.