rails/rails — pull request #6034 by willbryant (2012-04-28 10:10:33+00:00)
Title: Fix the Flash middleware loading the session on every request 

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.