This project is archived and is in readonly mode.

#1046 ✓committed
Pelle Braendgaard

Http only cookies in CookieStore

Reported by Pelle Braendgaard | September 15th, 2008 @ 04:55 AM

While the cookie store is tamper proof. Abusive Javascript could still remove the session cookie or cause a TamperedWithCookie exception.

This patch exposes a configuration parameter :session_http_only which defaults to true. This sets the HttpOnly flag on the cookie from the CookieStore.

Comments and changes to this ticket

  • Michael Koziarski

    Michael Koziarski September 15th, 2008 @ 10:24 AM

    • Assigned user set to “Michael Koziarski”
    • Tag changed from actionpack to actionpack, cookiestore, patch, security, sessions

    This patch has no tests, it would be good to add them. The last time we changed the session options we had a few glitches as a result.

    Also, why change the default? It seems that may break some applications

  • Pelle Braendgaard

    Pelle Braendgaard September 15th, 2008 @ 11:16 AM

    I'll work on a group of comprehensive tests. I couldn't find a good starting point for testing it, but I will revisit it. It seems like the individual cookie settings for sessions aren't tested very well anywhere.

    I can't see why it should break applications, as it is only for the session store which wouldn't be tampered with in JS anyway.

  • Michael Koziarski

    Michael Koziarski September 15th, 2008 @ 01:57 PM

    Is there any reason why we shouldn't set the session_id cookie for the other stores to HttpOnly also?

    The breakage I'm worried about is people accessing the cookie from their application code rather than actually trying to do anything with it. f.ex flash movies doing file uploads etc. At the very least we should document it in the release notes.

  • Pelle Braendgaard

    Pelle Braendgaard September 15th, 2008 @ 08:23 PM

    I've updated the patch with support for the rack_process as well as better unit tests. While I was at it I added unit tests for the session_secure flag in the cookie store.

    I am in agreement that session_id cookie should also be HttpOnly. The code that actually creates that cookie is deep in standard ruby library. But I think it should be able to be set in rail's session.rb after it calls ruby's initializer. That said, I think that should be a separate patch and ticket, which I'm happy to take on.

    There shouldn't be any breakage within the application code as HttpOnly only restricts Javascript access to the session cookie and not the rails application itself. The change should definitely be documented in the release notes though.

  • Michael Koziarski

    Michael Koziarski September 16th, 2008 @ 09:48 AM

    • Tag changed from actionpack, cookiestore, patch, security, sessions to actionpack, cookiestore, patch, security, sessions
    • Milestone cleared.

    OK, It'd be great if you could look into the other session cookies.

    I realise HttpOnly won't break the rails code of an app, but they could possibly be dependent on accessing the cookie through javascript. However I think that it's probably enough to clearly document this in the Changelog and release notes.

    Unfortunately, I get errors trying to apply the patch:

    
      % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                     Dload  Upload   Total   Spent    Left  Speed
    100  6446  100  6446    0     0   2471      0  0:00:02  0:00:02 --:--:-- 28342
    Applying: Added support for http_only cookies in cookie_store
    Applying: Added http_only as default to rack_process Added unit tests for secure and http_only cookies to cookie_store
    fatal: patch with only garbage at line 100
    Patch failed at 0002.
    When you have resolved this problem run "git am --resolved".
    If you would prefer to skip this patch, instead run "git am --skip".
    To restore the original branch and stop patching run "git am --abort".
    
  • Pelle Braendgaard

    Pelle Braendgaard September 16th, 2008 @ 05:25 PM

    Hmm. Dunno what went wrong there, anyway I created a new patch that I've actually test applied...

    I will start looking at http_only for regular sessions.

  • Repository

    Repository September 17th, 2008 @ 12:21 PM

    • State changed from “new” to “committed”

    (from [7ecb9689b03335ec28958c506b083390f4212d45]) Added support for http_only cookies in cookie_store Added unit tests for secure and http_only cookies in cookie_store

    Signed-off-by: Michael Koziarski michael@koziarski.com [#1046 state:committed] http://github.com/rails/rails/co...

  • cainlevy

    cainlevy March 20th, 2009 @ 10:47 PM

    You were right Michael -- I was trying to access the cookie from JavaScript to provide a clean solution for Flash uploads. Not sure where this change was noted? The Rails 2.3 release notes mentioned the http_only => httponly name change, but no change to the default value.

    Changing a default is very unexpected in a 2.x release. It would've made more sense in a 3.0. Isn't this the sort of thing that config/initializers/new_rails_defaults.rb is meant for?

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages