Re: [Rails-core] Cookie store and object instances in session behavior when instance class no longer exists
On 16-07-2015 09:34, Matt Jones wrote:
On Jul 15, 2015, at 11:06 AM, Rodrigo Rosenfeld Rosas rr.ro...@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.
--
You received this message because you are subscribed to the Google Groups Ruby on
Rails: Core group.
To unsubscribe from this group and stop receiving emails from it, send an email
to rubyonrails-core+unsubscr...@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] Cookie store and object instances in session behavior when instance class no longer exists
On Jul 15, 2015, at 11:06 AM, Rodrigo Rosenfeld Rosas rr.ro...@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
--
You received this message because you are subscribed to the Google Groups Ruby
on Rails: Core group.
To unsubscribe from this group and stop receiving emails from it, send an email
to rubyonrails-core+unsubscr...@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: Message signed with OpenPGP using GPGMail
[Rails-core] 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?
--
You received this message because you are subscribed to the Google Groups Ruby on
Rails: Core group.
To unsubscribe from this group and stop receiving emails from it, send an email
to rubyonrails-core+unsubscr...@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.
