Skip to content
This repository

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

Merged
merged 1 commit into from almost 2 years ago

4 participants

Will Bryant José Valim Piotr Sarnacki Jeremy Kemper
Will Bryant

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.

Will Bryant 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
José Valim
Owner

/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.

Piotr Sarnacki
Collaborator

+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 Kemper
Owner

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

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

José Valim
Owner

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

Will Bryant

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.

Piotr Sarnacki drogus referenced this pull request from a commit in drogus/rails
Piotr Sarnacki Failing test for #6034 e2f15ed
Piotr Sarnacki
Collaborator

@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.

José Valim
Owner

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

Piotr Sarnacki
Collaborator

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

Piotr Sarnacki drogus merged commit 88b5f94 into from
Piotr Sarnacki drogus closed this
Piotr Sarnacki drogus referenced this pull request from a commit
Piotr Sarnacki Failing test for #6034 e2b9709
José Valim josevalim merged commit 5638adf into from
José Valim josevalim closed this
Piotr Sarnacki drogus referenced this pull request from a commit
Piotr Sarnacki Failing test for #6034 2b2983d
Piotr Sarnacki drogus referenced this pull request from a commit
Piotr Sarnacki Failing test for #6034 e23e684
Will Bryant

Thanks @drogus, that looks great.

Rafael Mendonça França rafaelfranca referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Rafael Mendonça França rafaelfranca referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Rafael Mendonça França rafaelfranca referenced this pull request from a commit in rafaelfranca/rails
Rafael Mendonça França 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

Showing 1 unique commit by 1 author.

Apr 28, 2012
Will Bryant 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
This page is out of date. Refresh to see the latest.
1  actionpack/lib/action_controller/test_case.rb
@@ -473,7 +473,6 @@ def process(action, http_method = 'GET', *args)
473 473
 
474 474
         @request.session = ActionController::TestSession.new(session) if session
475 475
         @request.session["flash"] = @request.flash.update(flash || {})
476  
-        @request.session["flash"].sweep
477 476
 
478 477
         @controller.request = @request
479 478
         build_request_uri(action, parameters)
9  actionpack/lib/action_dispatch/middleware/flash.rb
@@ -4,7 +4,7 @@ class Request
4 4
     # read a notice you put there or <tt>flash["notice"] = "hello"</tt>
5 5
     # to put a new one.
6 6
     def flash
7  
-      @env[Flash::KEY] ||= (session["flash"] || Flash::FlashHash.new)
  7
+      @env[Flash::KEY] ||= (session["flash"] || Flash::FlashHash.new).tap(&:sweep)
8 8
     end
9 9
   end
10 10
 
@@ -217,10 +217,6 @@ def initialize(app)
217 217
     end
218 218
 
219 219
     def call(env)
220  
-      if (session = env['rack.session']) && (flash = session['flash'])
221  
-        flash.sweep
222  
-      end
223  
-
224 220
       @app.call(env)
225 221
     ensure
226 222
       session    = env['rack.session'] || {}
@@ -237,7 +233,8 @@ def call(env)
237 233
         env[KEY] = new_hash
238 234
       end
239 235
 
240  
-      if session.key?('flash') && session['flash'].empty?
  236
+      if (!session.respond_to?(:loaded?) || session.loaded?) && # (reset_session uses {}, which doesn't implement #loaded?)
  237
+         session.key?('flash') && session['flash'].empty?
241 238
         session.delete('flash')
242 239
       end
243 240
     end
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.