Fix the Flash middleware loading the session on every request #6034

Merged
merged 1 commit into from Apr 30, 2012

Projects

None yet

4 participants

@willbryant
Contributor

So once upon a time, Rails always loaded the session unless you explicitly used session :off. Loading itself is not so important but loading the session also triggers writing a session cookie back on the response, so obviously it is very important to turn session cookies off on shared resources, particularly if you're using cache control to make them publicly cacheable. So you always knew to do session :off when you had a controller sending that kind of header.

When sessions were changed to be lazily loaded in the 2.x days, session :off was removed since it was no longer required.

But in the refactoring of the flash code from the controller stack into the middleware stack in the 3.x series, the code was not just moved over directly - the commit (ead93c5) also rewrote a couple of the methods in a way that meant that the middleware now ALWAYS loads the session in order to clear flashes, and so ALWAYS writes back a session cookie. Because Rails no longer has session :off, and middleware can't be skipped on a per-controller basis without building an entire new stack, this is not good.

It gets much worse though. Tests/specs don't really run the real flash or session code - they have to set aup a fake context one way or another so IIRC they never did - and the test session setup code does not have the same bug. So tests written to confirm that the controllers don't set session cookies are falsely passing.

The really bad thing came in Rails 3.1, which turned on Rack::Cache by default, even for existing apps. Unlike most caches, Rack::Cache - at that time (I'm told they've put in a patch since) - would cache any responses that you mark cacheable even if they have cookies, and will cache and serve the cookie.

Every other HTTP cache I've seen will either ignore the cacheable headers because there's a cookie, or strip the cookie. (Of course, it is programmer error to have cacheable headers and a cookie header, but if the programmer error is in the framework and that's an unannounced change that tests don't reveal, this can get into production, and has.)

So by Rails 3.1 we had a silent behavior change to always send session cookies even on responses marked cacheable, false negatives from tests designed to check session cookies were not being sent, and a default infrastructure change that meant that other people's sessions cookies were sent out on cacheable objects even though that's clearly crazy, even on HTTPS.

This caused us a very expensive privacy breach because all customers ended up with session of customer who first happened to hit the shareable resources, since all future responses came from Rack::Cache instead of the actual session.

(In case you're wondering why we didn't hit this as soon as we went to the 3.0 version that had the middleware change, we're using HTTPS - so the only cache that could possible see the response is one on our servers, which means only Rack::Cache. But as I say, any other server-side caching software I've ever used would not have done this anyway.)

Although Rack::Cache may now be more sane, we still need to fix the regression in Rails that caused sessions to be always sent, because it's simply not supposed to work that way, not to mention a liability.

I patched this for 3.1: https://github.com/willbryant/rails/tree/flash_must_not_load_session_on_every_request_3-1-stable
The same patch rebased for 3.2: https://github.com/willbryant/rails/tree/flash_must_not_load_session_on_every_request_3-2-stable
And this pull request is the same patch rebased against master: https://github.com/willbryant/rails/tree/flash_must_not_load_session_on_every_request

We have been using this patch in production on 3.1 for a while now and have had no issues. Essentially it just sweeps the flash the first time it is requested, rather than before every request irrespective of whether it uses flash.

This is a behavior change, but only back to the behavior we had before the middleware refactor (ead93c5). As far as I can see from the commit message (just "Move Flash into middleware") that was not supposed to be a behavior change - I think Josh may have just tidied up and not noticed the implication.

FWIW, aside from not having the above problem, the original behavior is in any case much more useful. To me it makes no sense to be clearing the flash if you haven't referenced the flash: for example, if you have various "whole page" actions which will render the flash in the page layout, but you also have an ajaxy autocompleter search action which only returns json autocomplete results, it makes no sense to have the ajaxy autocompleter search action clear the flash.

So with this patch we go back to the old behavior - when you look at the flash, it will be swept, otherwise, it won't. The session cookie will therefore not be loaded to sweep the flash unless you use the flash.

Although it's just a straight rebase, it'd probably be fair to say that the original commit message might no longer need reference Rack::Cache for master if their patch has gone in since. I haven't looked into whether it's present in the Rack::Cache versions pulled in to the 3.2 or 3.1 branches now, but it wasn't at the time I wrote the patch.

@willbryant willbryant fix the Flash middleware loading the session on every request (very d…
…angerous especially with Rack::Cache), it should only be loaded when the flash method is called
5638adf
@josevalim
Member

/cc @jeremy

We already had this discussion on the core room many times. Some prefer the flash to always be swept, even if not accessed. That said, I would like to propose a compromise. Let's merge this feature in, so the middleware is as smart as possible and if one wants to always read the flash, we can have a method in the controller that defines a filter that will enforce the swap:

def swap_flash_on_every_request
  before_filter { flash }
end

This will also fix issues people are having with flash expiring on ajax requests or images rendered from the server, so it is a very welcome addition imho.

Finally, if someone has a flash check on their layout/application, it will be enough to ensure it will be swept. So it likely won't affect most apps.

@drogus
Member
drogus commented Apr 28, 2012

+1 to all @josevalim said, but I think that it would be nice to also add some tests here. You could add them to railties/application/, so we will not run into this again. If you need any help with this I will be happy to help.

@jeremy
Member
jeremy commented Apr 29, 2012

Agreed @josevalim. swap_flash_on_every_request probably isn't even needed.

This is nasty. +1 to fixing in 3-2-stable even.

@josevalim
Member

Ok, let's merge this! @drogus can you push a test later?

@willbryant
Contributor

Thanks guys. Hoping we can get it into 3-1-stable too to be on the safe side since it's almost a security issue, but will understand if we're only maintaining one version back as usual.

José, I see that a while back you contributed a test which almost caught this bug - test_just_using_flash_does_not_stream_a_cookie_back. Frustratingly, although the test is useful, the buggy code didn't fail this test because it only triggered the session cookie on the response if the request had both a session cookie and a flash key in it:

      if (session = env['rack.session']) && (flash = session['flash'])
        flash.sweep
      end

(obviously users will have this if they have hit other actions first, even if this request doesn't use the session.)

So I am trying to adapt that test to set the session cookie up properly but struggling a bit. @dragus, I'd love some help with this, thanks!

Basically all I want to add to the above test is request.session['flash'] = {:foo => :bar}. But this being a framework test rather than a controller test, request.session isn't set up, nor is request.cookie_jar to set request.cookie_jar.signed[SessionKey] directly.

Any ideas? I'd really like to avoid coping in all the session cookie code because it feels to me like that would be a very brittle test that would stop testing anything useful when the session code is changed.

@drogus drogus was assigned Apr 29, 2012
@drogus drogus added a commit to drogus/rails that referenced this pull request Apr 30, 2012
@drogus drogus Failing test for #6034 e2f15ed
@drogus
Member
drogus commented Apr 30, 2012

@willbryant since this is a problem with middleware I think that more reliable place to test this is in railties, which test entire rails stack, I've pushed a test that was failing without your patch and fixed after applying it. Basically it sets the flash and tries to make another request without fetching it, which should not trigger setting cookie session.

@josevalim
Member

@drogus since you added the test, can you please merge your test and then merge this pull request? It is in your hands now. :)

@drogus
Member
drogus commented Apr 30, 2012

@josevalim sure, just wanted to wait for some feedback. I will push this and apply to previous branches.

@drogus drogus merged commit 88b5f94 into rails:master Apr 30, 2012
@drogus drogus added a commit that referenced this pull request Apr 30, 2012
@drogus drogus Failing test for #6034 e2b9709
@josevalim josevalim merged commit 5638adf into rails:master Apr 30, 2012
@drogus drogus added a commit that referenced this pull request Apr 30, 2012
@drogus drogus Failing test for #6034 2b2983d
@drogus drogus added a commit that referenced this pull request Apr 30, 2012
@drogus drogus Failing test for #6034 e23e684
@willbryant
Contributor

Thanks @drogus, that looks great.

@rafaelfranca rafaelfranca added a commit to rafaelfranca/omg-rails that referenced this pull request Jun 4, 2012
@rafaelfranca rafaelfranca Add CHANGELOG entry for #6034 [ci skip] 5197aaf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment