Don't Raise if Can't Deserialize Session #243

Closed
yfeldblum opened this Issue Sep 30, 2011 · 6 comments

Projects

None yet

2 participants

@yfeldblum

If Rack cannot deserialize the session in development or test, then it should probably raise. But if Rack cannot deserialize the session in production, it should probably clear the session instead of raising. This could be set at use time with an option.

In the move from rails-3.0 to rails-3.1, Rails changed one of the classes that was getting serialized into the session. As a result, people who had gotten a session cookie with the rails-3.0 version of their app and were now trying to visit the rails-3.1 version were seeing exceptions.

@raggi
Member
raggi commented Oct 1, 2011

This is a problem with using middleware. The app isn't able to exhibit sensible control over such issues.

Fundamentally though, this is a forward compatibility bug in rails.

I may consider not raising in 1.4, but the problem is, this could open up another class of issue, whereby there would be a partial pass through of cookie data.

e.g. if you have two cookies, one for the session and one for some other storage, the session would be ignored, but the other would not. I'd prefer not to lead users into making mistakes, and the raise is not entirely invalid.

@yfeldblum

I think the app can be able to exhibit sensible control here by useing the middleware with options specified by the app or by the app's configuration. The app can tell the Rack session cookie middleware either "raise on error" or "nil on error", as long as the middleware takes such an option in its constructor. As to what the default should be - perhaps the default should be "raise on error", with "nil on error" being a documented option in cases like upgrading to rails-3.1. If this were an option to the Rack middleware, Rails could then expose it as a Rails option that the app developer can control. This exposes to the app developer the choice of whether to allow, or how to deal with, the second class of bug.

The real problem with raising is that there is no good way for the application to handle it, and therefore no good way for the application to do anything to help the user. The user can't browse to the site because they have a now-undeserializable session cookie, so there's no way for the site to drop the cookie or issue a new deserializable session cookie so that the user can browse. It's a catch-22.

It's really up to Rack to provide a way so that the app can help the user break out of that catch-22, in case such a circumstance arises.

@raggi
Member
raggi commented Oct 1, 2011

After further consideration I think I agree with you.

If I default the option to raise, then I could potentially add this in 1.3.5.

I will also discuss with some rails folks and see if we can't come up with a good solution for the 3.0 -> 3.1 transition also.

@yfeldblum

Thanks! There will be a lot of Rails users who will appreciate this effort!

@raggi
Member
raggi commented Oct 18, 2011

So rack actually does the right thing here.

I took a look in racks tests, and it does recover from "normally" broken cookies, that is, things with particularly bad data.

The rails problem specifically, is that they changed the parent class of these objects (iirc), and that raises a very specific error, that rack shouldn't really know about.

I would recommend that those changes should be reverted or corrected on the rails side.

@raggi raggi closed this Oct 18, 2011
@raggi
Member
raggi commented Oct 18, 2011

see spec_session_cookie.rb for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment