configure how unverified request will be handled #5326

Merged
merged 1 commit into from Mar 11, 2012

Projects

None yet

4 participants

@lest

throw InvalidAuthenticityToken exception by default
can be configured using :with option in protect_from_forgery method
or request_forgery_protection_method config option

Original issue #5300

@josevalim
Ruby on Rails member

/cc @NZKoz

I am a bit confused. The default is to raise an exception? Shouldn't the default be to clean up the session? In theory APIs don't even have a session attached to it.

@NZKoz
Ruby on Rails member

Yep, this should also change the generated applications to have something like

# prevent CSRF attacks by raising an exception, if your application has an API, you'll need to use :reset_session
protect_from_forgery :with=>:exception

It's a more secure default with a tiny change for api developers who need it. We could also leave the default for existing apps as reset_session but change newly generated apps to include :with=>:exception

@lest null_session is another change we'd discussed, instead of resetting the session it should just assign the cookie and session objects to values which return null for everything and then send no Set-Cookie headers in the response.

That way it's like the session has been reset, without actually logging the user out for subsequent requests. We can address that in another pull request.

@lest

@NZKoz commit updated

@vijaydev vijaydev and 1 other commented on an outdated diff Mar 8, 2012
...action_controller/metal/request_forgery_protection.rb
@@ -82,7 +90,12 @@ def verify_authenticity_token
# This is the method that defines the application behavior when a request is found to be unverified.
# By default, \Rails resets the session when it finds an unverified request.
@vijaydev
vijaydev Mar 8, 2012

Can we add a line about the options available in the docstring?

@lest
lest Mar 8, 2012

@vijaydev added docs to both protect_from_forgery and handle_unverified_request methods

@NZKoz NZKoz and 1 other commented on an outdated diff Mar 8, 2012
railties/test/application/configuration_test.rb
@@ -257,6 +257,8 @@ def persisted?; true; end
app_file 'app/controllers/posts_controller.rb', <<-RUBY
class PostsController < ApplicationController
+ skip_before_filter :verify_authenticity_token
+
@NZKoz
NZKoz Mar 8, 2012

Still not clear why these are being added? Are we hiding a regression?

@lest
lest Mar 8, 2012

@NZKoz In this test authenticity_token is not sent in patch and put requests. Now exception is raised in this case (previously session reset was happened and test passed), so we can either skip verify or send authenticity_token to make it pass now.

@NZKoz
NZKoz Mar 8, 2012

let's send the authenticity token so we're testing the right thing.

@NZKoz NZKoz commented on an outdated diff Mar 8, 2012
...action_controller/metal/request_forgery_protection.rb
def handle_unverified_request
- reset_session
+ case request_forgery_protection_method
+ when :exception
+ raise ActionController::InvalidAuthenticityToken
+ when :reset_session
+ reset_session
+ end
@NZKoz
NZKoz Mar 8, 2012

This still has the ability to leave an app unprotected, just put an else in the case statement and remove the code from protect_with_forgery

@lest lest configure how unverified request will be handled
can be configured using `:with` option in `protect_from_forgery` method
or `request_forgery_protection_method` config option

possible values:
- :reset_session (default)
- :exception

new applications are generated with:

    protect_from_forgery :with => :exception
2459411
@lest

@NZKoz thanks for comments. commit updated.

@NZKoz NZKoz merged commit 411a826 into rails:master Mar 11, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment