Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Rails 3 deleting a cookie AND calling reset_session does not send new session ID #372

Closed
d11wtq opened this Issue · 23 comments

4 participants

@d11wtq

Rails 3.0.7, ruby-1.9.2 (via RVM). Take the following code:

class LoginController < ApplicationController
    # ... SNIP ... 

    def destroy
        cookies.delete(:secureusertokens)
        reset_session
        redirect_to root_url
    end
end

This is simply a logout. It resets the session and deletes any "remember me" cookie that may be set.

Unfortunately, while a new session ID is generated, the Set-Cookie: header lacks this information (presumably because the session logic has a reference to some stale cookie data, and the controller overwrites it).

The session key is just SESSID in these headers.

In the request:

Cookie: hiddenalerts=site_vrfy_124258; __utmz=REMOVED_INFO.utmcsr=(direct)|utmccn=(direct)|utmcmd=(none); elpriv=REMOVED_INFO; SESSID=b39a9a89bb6a39ea91b620fe0da392ed; __utma=REMOVED_INFO; __utmc=REMOVED_INFO; __utmb=REMOVED_INFO

And in the response (no mention of the newly generated session ID):

Set-Cookie: secureusertokens=; path=/; expires=Thu, 01-Jan-1970 00:00:00 GMT

Now if I remove the call to cookies.delete(...) in the controller.

In the request (identical):

Cookie: hiddenalerts=site_vrfy_124258; __utmz=REMOVED_INFO.utmcsr=(direct)|utmccn=(direct)|utmcmd=(none); elpriv=REMOVED_INFO; SESSID=b39a9a89bb6a39ea91b620fe0da392ed; __utma=REMOVED_INFO; __utmc=REMOVED_INFO; __utmb=REMOVED_INFO

And in the response (correct!!):

Set-Cookie: SESSID=50640523cf32b5b0fe8c93eb16aba6dc; path=/; HttpOnly

It seems that I can either have the new session ID sent, or the "remember me" cookie deleted, but not both. I can work around this by manually sending the new cookie, but it seems like a bug to me ;)

I have my own custom SessionStore (MemCache + MySQL), but it does not set these cookies, that happens elsewhere.

@josevalim
Owner

Is this a regression (i.e. did it work in previous rails versions)? Can you provide a failing test case?

@d11wtq

I'm new to Rails, so haven't used a previous version. I'll try to write a test case (haven't quite gotten that far yet, but I assume it's just xUnit style). It should be extremely simple to test manually though:

  1. Put something in the session, take note of the session ID
  2. Set a cookie (anything will do)
  3. In a different controller, call reset_session and also delete that cookie you set
  4. If it's working, you should see a Set-Cookie: for a new session ID to prevent session fixation, and also for the deletion of your other cookie. What you'll actually see, however, is only the deletion of your cookie, not the passing back of the new session ID
  5. Repeat the process, omitting the cookie deletion line. Note that the new session ID is correctly passed back.
@josevalim
Owner

The problem is that we (Rails Core Members) unfortunately don't have the time to reproduce all errors manually. Providing a test case would speed up this issue being fixed considerably. :) Please let me know if you need any help writing the test cases. You can find similar ones inside the directory "actionpack/test/dispatch".

You can also run tests just for actionpack by going into the actionpack directory and running "rake test". If you want to run test for a specific file, you can also go to actionpack directory and type: "ruby -Itest test/dispatch/session_test.rb"

@d11wtq

All good, I'm a TDD developer so I doubt there's a big paradigm shift, I'm just learning my way around Rails at the moment before we start to port a large existing PHP web app to it.

@josevalim
Owner

Any news here? Can we help you somehow?

@d11wtq
@d11wtq

Just a quick update. The bug is even simpler... I'm amazed this hasn't been reported before (either that or I have some weird installation issue). Basically you cannot set more than one cookie in the same request. Try it.

cookies[:first] = 42
cookies[:second] = 99

The only one that actually gets set is cookies[:second].

Will a functional test from a rails install be enough or do I actually have to figure out how to unit test the cookie/controller/whatever component is to blame? There's so much of it, I'd have no idea where to begin.

@josevalim
Owner

The simplest test you are able to produce that reproduces the error is welcome. It doesn't matter if it is functional or unit one.

@d11wtq

Ok, here goes. Testing, I found that the simple cookies[:key] = "value" works ok, but adding :secure breaks it.

It's 2am and I'm about to head to bed, but if I discover anything else tomorrow I'll add a note.

require 'test_helper'

class CookieBugTest < ActionController::TestCase

  COOKIE_KEY_A = "one"
  COOKIE_KEY_B = "two"

  class CookieBugController < ApplicationController
    def set_two_secure_cookies
      cookies[COOKIE_KEY_A] = {
        :value => "anything",
        :secure => true
      }
      cookies[COOKIE_KEY_B] = {
        :value => "another",
        :secure => true
      }
      render :text => "ok"
    end
  end

  def setup
    @controller = CookieBugController.new
    Rails.application.routes.draw do
      match "/anything" => "cookie_bug_test/cookie_bug#set_two_secure_cookies"
    end
  end

  test "should be able to set two secure cookies in the same request" do
    post :set_two_secure_cookies
    assert_response :success

    assert cookies[COOKIE_KEY_A], "first cookie should be set"
    assert cookies[COOKIE_KEY_B], "second cookie should be set"
  end

end
@nakajima

I started taking a look at adding a test case for this, but one already exists (and passes): https://github.com/rails/rails/blob/master/actionpack/test/dispatch/cookies_test.rb#L201-206

@josevalim
Owner

@nakajima Maybe they both need to be secure in order to reproduce the failure?

@d11wtq

Try it with :secure => true. Hmm, facepalm... I have this in my config config.middleware.use Rack::SslEnforcer because we need to enforce SSL (almost, but not entirely) site-wide. It wouldn't surprise me if this a bug with that middleware and not with Rails. I'll have to try uninstalling it tomorrow and see if anything changes.

@stjhimy

@josevalim probably yes, i'm testing the same thing here, only with :secure it break

@nakajima

:secure seems to work. These tests still pass:

diff --git a/actionpack/test/dispatch/cookies_test.rb b/actionpack/test/dispatch/cookies_test.rb
index e42c39f..cdd962d 100644
--- a/actionpack/test/dispatch/cookies_test.rb
+++ b/actionpack/test/dispatch/cookies_test.rb
@@ -28,6 +28,18 @@ class CookiesTest < ActionController::TestCase
       head :ok
     end

+    def set_multiple_secure_cookies
+      cookies["user_name"] = { :value => "david", :expires => Time.utc(2005, 10, 10,5), :secure => true }
+      cookies["login"]     = { :value => "XJ-122", :secure => true }
+      head :ok
+    end
+
+    def set_mixed_secure_cookies
+      cookies["user_name"] = { :value => "david", :expires => Time.utc(2005, 10, 10,5), :secure => true }
+      cookies["login"]     = "XJ-122"
+      head :ok
+    end
+
     def access_frozen_cookies
       cookies["will"] = "work"
       head :ok
@@ -205,6 +217,22 @@ class CookiesTest < ActionController::TestCase
     assert_equal({"login" => "XJ-122", "user_name" => "david"}, @response.cookies)
   end

+  def test_multiple_secure_cookies
+    @request.env["HTTPS"] = "on"
+    get :set_multiple_secure_cookies
+    assert_equal 2, @response.cookies.size
+    assert_cookie_header "user_name=david; path=/; expires=Mon, 10-Oct-2005 05:00:00 GMT; secure\nlogin=XJ-122; path=/; secure"
+    assert_equal({"login" => "XJ-122", "user_name" => "david"}, @response.cookies)
+  end
+
+  def test_mixed_secure_cookies
+    @request.env["HTTPS"] = "on"
+    get :set_mixed_secure_cookies
+    assert_equal 2, @response.cookies.size
+    assert_cookie_header "user_name=david; path=/; expires=Mon, 10-Oct-2005 05:00:00 GMT; secure\nlogin=XJ-122; path=/"
+    assert_equal({"login" => "XJ-122", "user_name" => "david"}, @response.cookies)
+  end
+
   def test_setting_test_cookie
     assert_nothing_raised { get :access_frozen_cookies }
   end
@@ -417,7 +445,7 @@ class CookiesTest < ActionController::TestCase
     assert_cookie_header "user_name=; path=/; expires=Thu, 01-Jan-1970 00:00:00 GMT"
   end

-  
+
   def test_cookies_hash_is_indifferent_access
       get :symbol_key
       assert_equal "david", cookies[:user_name]
@josevalim
Owner

@nakajima, if you want to make those tests a patch, I would happily apply them.

@josevalim
Owner

@d11wtq thanks for all the work debugging this.

@d11wtq

It's odd, playing some more, the cookies hash is completely empty in my test case. @stjhimy, same for you?

This is different to the behaviour I get when not in a functional test.

I'll try disabling this Rack::SSLEnforcer middleware.

Nope. Even with the middleware off, the test still fails. Might be an issue with functional tests themselves not working correctly with secure cookies?

@d11wtq

Ok, I'm seriously not making this up. For the sake of my sanity (and to minimize hair loss), can somebody else please try this outside of the context of a test case and just try it for real? The test case doesn't seem to do the same thing as a manual test. I have no idea why. I'm using 3.0.7.

What I literally just did now:

  1. Open the Inspector console in Chrome and get the document headers panel open
  2. In an empty controller action (though with a template, or some view to prevent it raising an error) just add:

    cookies[:first_cookie] = 42
    cookies[:second_cookie] = 99
  3. Load the page and look at the headers:

    I'm staring at this right now:

    Set-Cookie: second_cookie=99; path=/
    

This is my Gemfile.lock

GEM
  remote: http://rubygems.org/
  specs:
    abstract (1.0.0)
    actionmailer (3.0.7)
      actionpack (= 3.0.7)
      mail (~> 2.2.15)
    actionpack (3.0.7)
      activemodel (= 3.0.7)
      activesupport (= 3.0.7)
      builder (~> 2.1.2)
      erubis (~> 2.6.6)
      i18n (~> 0.5.0)
      rack (~> 1.2.1)
      rack-mount (~> 0.6.14)
      rack-test (~> 0.5.7)
      tzinfo (~> 0.3.23)
    activemodel (3.0.7)
      activesupport (= 3.0.7)
      builder (~> 2.1.2)
      i18n (~> 0.5.0)
    activesupport (3.0.7)
    addressable (2.2.6)
    aws-s3 (0.6.2)
      builder
      mime-types
      xml-simple
    bcrypt-ruby (2.1.4)
    builder (2.1.2)
    capistrano (2.6.0)
      highline
      net-scp (>= 1.0.0)
      net-sftp (>= 2.0.0)
      net-ssh (>= 2.0.14)
      net-ssh-gateway (>= 1.1.0)
    data_objects (0.10.5)
      addressable (~> 2.1)
    database_cleaner (0.6.7)
    dm-active_model (1.1.0)
      activemodel (~> 3.0.4)
      dm-core (~> 1.1.0)
    dm-aggregates (1.1.0)
      dm-core (~> 1.1.0)
    dm-core (1.1.0)
      addressable (~> 2.2.4)
    dm-do-adapter (1.1.0)
      data_objects (~> 0.10.2)
      dm-core (~> 1.1.0)
    dm-migrations (1.1.0)
      dm-core (~> 1.1.0)
    dm-mysql-adapter (1.1.0)
      dm-do-adapter (~> 1.1.0)
      do_mysql (~> 0.10.2)
    dm-rails (1.1.0)
      actionpack (~> 3.0.4)
      dm-active_model (~> 1.1.0)
      dm-core (~> 1.1.0)
      railties (~> 3.0.4)
    dm-timestamps (1.1.0)
      dm-core (~> 1.1.0)
    dm-types (1.1.0)
      bcrypt-ruby (~> 2.1.4)
      dm-core (~> 1.1.0)
      fastercsv (~> 1.5.4)
      json (~> 1.4.6)
      stringex (~> 1.2.0)
      uuidtools (~> 2.1.2)
    dm-validations (1.1.0)
      dm-core (~> 1.1.0)
    do_mysql (0.10.5)
      data_objects (= 0.10.5)
    erubis (2.6.6)
      abstract (>= 1.0.0)
    fastercsv (1.5.4)
    highline (1.6.2)
    i18n (0.5.0)
    json (1.4.6)
    kgio (2.4.0)
    mail (2.2.19)
      activesupport (>= 2.3.6)
      i18n (>= 0.4.0)
      mime-types (~> 1.16)
      treetop (~> 1.4.8)
    memcache-client (1.8.5)
    mime-types (1.16)
    net-scp (1.0.4)
      net-ssh (>= 1.99.1)
    net-sftp (2.0.5)
      net-ssh (>= 2.0.9)
    net-ssh (2.1.4)
    net-ssh-gateway (1.1.0)
      net-ssh (>= 1.99.1)
    polyglot (0.3.1)
    rack (1.2.2)
    rack-mount (0.6.14)
      rack (>= 1.0.0)
    rack-test (0.5.7)
      rack (>= 1.0)
    railties (3.0.7)
      actionpack (= 3.0.7)
      activesupport (= 3.0.7)
      rake (>= 0.8.7)
      thor (~> 0.14.4)
    rake (0.8.7)
    stringex (1.2.1)
    thor (0.14.6)
    treetop (1.4.9)
      polyglot (>= 0.3.1)
    tzinfo (0.3.27)
    unicorn (3.6.2)
      kgio (~> 2.3)
      rack
    uuidtools (2.1.2)
    xml-simple (1.0.15)

PLATFORMS
  ruby

DEPENDENCIES
  actionmailer (~> 3.0.7)
  actionpack (~> 3.0.7)
  activesupport (~> 3.0.7)
  aws-s3
  capistrano
  database_cleaner
  dm-aggregates (~> 1.1)
  dm-migrations (~> 1.1)
  dm-mysql-adapter (~> 1.1)
  dm-rails (~> 1.1)
  dm-timestamps (~> 1.1)
  dm-types (~> 1.1)
  dm-validations (~> 1.1)
  memcache-client
  railties (~> 3.0.7)
  unicorn (~> 3.6)

Let me know if there's anything else I can do to try and narrow this down.

@stjhimy

i'm testing in a new project now, rails 3.0.7 and the two cookies are in the chrome inspector, i will try with ruby 1.9.2 now, brb

@d11wtq

I'll have a go at trying it with 1.8.7 too, thanks for trying this :)

@d11wtq

Creating a blank project it's working for me. Sorry about this. I'll have to figure out what I have installed that's causing it.

@stjhimy
@d11wtq

Seems to be an issue with either squid or perlbal (load balancers/caching proxies). When I by-pass those it's working. I'll close this ticket. Apologies once again. Thanks so much for your help :)

@d11wtq d11wtq closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.