Using Rack HEAD causes CookieStore security warnings
#7372

Assignees

Labels

Projects

Milestone

17 participants

Rack::Session::Cookie accepts a :secret option in its initializer. The initializer stores that secret in an instance variable and uses it in the #set_session method to HMAC sign the data.

Rack HEAD added security warnings for when the Rack::Session::Cookie middleware is initialized without a secret.

This is causing the warning to show up because of the way Rails uses Rack::Session::Cookie:

Rails uses a subclass of Rack::Session::Cookie called ActionDispatch::Session::CookieStore that overwrites #set_session and #set_cookie to do its own signed cookie implementation using ActionDispatch::Cookies::SignedCookieJar (using Rack env "action_dispatch.secret_token" instead of the @secret instance variable). Since Rails doesn't need the @secret instance variable, it initializes ActionDispatch::Session::CookieStore without providing a :secret option.

To avoid the warning, we should just not use Rack::Session::Cookie. We're overriding a lot of the functionality for Rack::Session::Cookie anyway, to the point that its most important functionality (cookie signing) is not being used at all.

Can we get someone in Rails core who's familiar with Rack integration to chime in? What's our philosophy behind integration with Rack? Do we try to use as many standard Rack middleware as possible, or is that not a priority?

@dlee I also suggest you start a discussion on the mailing list. The bug tracker is primarily used to track bugs and pull requests with attached code (for new features). For discussions the mailing list is better suited.

@steveklabnik should we close this ticket in favor of a post on the core-list?

yeah, we need to avoid using the rack session subclass as this warning isn't relevant for us. It doesn't seem to give us much behaviour that we don't then turn around and re-implement.

A pull request for that would be a good start.

@homakov we intend to ship an encrypted cookie store with 4.0, and also start using the key derivation code rather than the raw secret, perhaps we can look into that once we've removed the rack subclassing?

@NZKoz good idea! anyone is on it or is it just future plan? I see more pros of 'encrypted' than in 'signed'. It will allow to develop database-less(stateless) applications - developer is sure that data from session is not readable/writable, each user is just carrying his own DB.
One thing - performance. Doesn't it look slower to decrypt every session everytime.. gotta check

Looks like the discussion here (that I wasn't yet aware of) makes #8792 not the appropriate fix for #8789. If I am up to the task of refactoring to what it seems the consensus here is (moving away from subclassing rack's session stuff), would that be a desirable pull request?

It looks like we're going to have to clone a lot of functionality from rack to do this. Seems like it might be better to maintain the subclassing. I'll happily do the work if everyone disagrees, but as the person doing it, it's starting to feel like a bad idea.

For now, I'll just keep going and get a pull request up so there's something explicit for people to look at, and see what I mean.

Maybe it would be nice at this point to team up with the rack team to figure out how it'd be possible to move all this custom functionality you're mentioning into rack itself and have Rails have nothing to do with the details of session and cookie implementation? Is there functionality in the whole rails cookie implementation that would not benefit rack apps in general?

I'm in favor of discussing with rack people as well. Seems like a good course of action. I'll dig into this more tomorrow and build up a list of duplications/points of tight coupling, since that will be useful whichever way we decide to go.

While @spastorino committed a workaround in cb3181e to suppress the message for now, I think the better fix is to subclass from Rack::Session::Abstract::ID instead of Rack::Session::Cookie, as we are providing our own cookie implementation in rails.

I am having some issues with Rack, just bounced through 3-4 issues and landed here but god its so fucking hard to actually understand all the content here I am grateful more capable people are working on it. Since I cant really contribute much understanding myself I decided to thank you guys instead.

@spastorino, @steveklabnik, I really appreciate how fast you guys are to offer help. Just to clear the air, turns out I was the problem, Rack was having a problem with me not the other way around. My profile would make it obvious that I am not half as seasoned as a developer compared to you guy, but since you guys are so helpful I will contact you if I run into a real problem about rails. I do not have that much of a web presence yet, that will hopefully change in the future, if you guys also think I can help you with anything, please reach out.

I am still having this incredibly annoying message that are destroying my spec output. I am on rails 3.2.13 and I thought this should have been back ported am I wrong? I can't use rack 1.4.1 since rails requires 1.4.5. Suggestions?

=> Booting Puma
=> Rails 3.2.13 application starting in development on http://0.0.0.0:3000
=> Call with -d to detach
=> Ctrl-C to shutdown server
Using github credentials for localhost
SECURITY WARNING: No secret option provided to Rack::Session::Cookie.
This poses a security threat. It is strongly recommended that you
provide a secret to prevent exploits that may be possible from crafted
cookies. This will not be supported in future versions of Rack, and
future versions will even invalidate your existing user cookies.
Called from: /Users/drnic/.rvm/gems/ruby-1.9.3-p392/gems/actionpack-3.2.13/lib/action_dispatch/middleware/stack.rb:43:in `new'.