Rails Ruby on Rails

Comments and changes to this ticket

• 

  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

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

  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.

  • http_only_sessions_ws.diff 6.3 KB
• 

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

• You flagged this item as spam.
  

  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.

  • http_only_sessions.diff 5.9 KB
• 

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

• You flagged this item as spam.
  

  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?