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):
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.
