XSRF protection should throw exception on failure by default #5300

Closed
meder opened this Issue Mar 6, 2012 · 6 comments

Projects

None yet

3 participants

meder commented Mar 6, 2012

Currently XSRF protection resets session and lets the request through when XSRF token verification fails, which by itself is an XSRF vulnerability since it allows anyone to logout users by directing their browser to a page that requires XSRF protection.

One approach proposed by @NZKoz was to have protect_from_forgery accept :with option, e.g.:

protect_from_forgery :with => :exception
protect_from_forgery :with => :null_session

with :exception hopefully being the default.

CC: @NZKoz @igrigorik @ratnikov

Contributor
lest commented Mar 6, 2012

It's possible to override #handle_unverified_request method in controller, e.g.:

class ApplicationController < ActionController::Base
  protect_from_forgery

  protected

  def handle_unverified_request
    # raise an exception or do anything else
  end
end
meder commented Mar 6, 2012

yes, it is, but I am talking about the defaults here.

Member
NZKoz commented Mar 6, 2012

@lest yeah, the intention here is to ensure that we can raise exceptions by default but have a simple comment in the generated app telling people that "if you have an API, change this to :null_session" so we're safer / simpler by default, but still work for APIS.

Contributor
lest commented Mar 7, 2012

@NZKoz I'm going to prepare PR

@NZKoz NZKoz closed this Mar 11, 2012
Member
NZKoz commented Mar 11, 2012

This has been merged now, I'll look to put the proper 'null session' change in prior to 4.0 shipping

meder commented Mar 11, 2012

Awesome! Thanks Michael! Thanks Sergey!

Additionally, please consider one of the features that @ratnikov has implemented in his strict-forgery-protection gem:

https://github.com/ratnikov/strict-forgery-protection

which serves as a great safety net for app devs that forget to enable XSRF protection on controllers. His code basically checks if as a result of the request DB entries have been modified and warns user if that's the case(obviously behaviour could be customized and exception could be thrown in dev mode, etc). I think that this approach will help in 2 cases:

  1. devs not knowing what XSRF is
  2. complex apps with complex controllers hierarchy where by mistake XSRF protection doesn't get enabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment