Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

Http only cookies in CookieStore

#1046

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.

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

State: committed
Milestone: none
Assigned to: Michael Koziarski Michael Koziarski
Importance: none

Activity

  1. Michael Koziarski
    Michael Koziarski
    • 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

    September 15th, 2008 @ 10:24 AM

  2. Pelle Braendgaard
    Pelle Braendgaard

    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.

    September 15th, 2008 @ 11:16 AM

  3. Michael Koziarski
    Michael Koziarski

    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.

    September 15th, 2008 @ 01:57 PM

  4. Pelle Braendgaard
    Pelle Braendgaard

    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.

    September 15th, 2008 @ 08:23 PM

  5. Michael Koziarski
    Michael Koziarski
    • 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".

    September 16th, 2008 @ 09:48 AM

  6. Pelle Braendgaard
    Pelle Braendgaard

    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.

    September 16th, 2008 @ 05:25 PM

  7. Repository
    Repository
    • 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 Http only cookies in CookieStore state:committed] http://github.com/rails/rails/co...

    September 17th, 2008 @ 12:21 PM

  8. cainlevy
    cainlevy

    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?

    March 20th, 2009 @ 10:47 PM