Gracefully handle invalid authenticity token

[ebababi]

In a vanilla Rails 4.1.9 installation with Devise 3.4.1 when protect_from_forgery with: :null_session is defined in ApplicationController in case of CSRF mismatch the following exception occurs:

NoMethodError (undefined method `to_key' for nil:NilClass): 
  vendor/bundle/ruby/2.1.0/gems/devise-3.4.1/app/controllers/devise/registrations_controller.rb:50:in `update' 
  vendor/bundle/ruby/2.1.0/gems/actionpack-4.1.9/lib/action_controller/metal/implicit_render.rb:4:in `send_action' 
  vendor/bundle/ruby/2.1.0/gems/actionpack-4.1.9/lib/abstract_controller/base.rb:189:in `process_action' 
  vendor/bundle/ruby/2.1.0/gems/actionpack-4.1.9/lib/action_controller/metal/rendering.rb:10:in `process_action' 
  vendor/bundle/ruby/2.1.0/gems/actionpack-4.1.9/lib/abstract_controller/callbacks.rb:20:in `block in process_action' 
  vendor/bundle/ruby/2.1.0/gems/activesupport-4.1.9/lib/active_support/callbacks.rb:113:in `call' 
  vendor/bundle/ruby/2.1.0/gems/activesupport-4.1.9/lib/active_support/callbacks.rb:113:in `call' 
  vendor/bundle/ruby/2.1.0/gems/activesupport-4.1.9/lib/active_support/callbacks.rb:149:in `block in halting_and_conditional' 
  vendor/bundle/ruby/2.1.0/gems/activesupport-4.1.9/lib/active_support/callbacks.rb:149:in `call' 
  vendor/bundle/ruby/2.1.0/gems/activesupport-4.1.9/lib/active_support/callbacks.rb:149:in `block in halting_and_conditional' 
  vendor/bundle/ruby/2.1.0/gems/activesupport-4.1.9/lib/active_support/callbacks.rb:149:in `call' 
  vendor/bundle/ruby/2.1.0/gems/activesupport-4.1.9/lib/active_support/callbacks.rb:149:in `block in halting_and_conditional' 
...

To replicate just head over to /users/edit form as a signed-in user and before submitting the form alter its authenticity_token value.

Probably this happens due to the execution order of the action filters, i.e. :authenticate_scope! is performed before :verify_authenticity_token.

My current solution was to override handle_unverified_request as follows:

class ApplicationController < ActionController::Base

  # Overwrite unverified request handler to force a refresh / redirect.
  def handle_unverified_request
    super # call the default behaviour, including Devise override
    throw :warden, redirect: request.referer || request.url
  end
end

What is your opinion? Am I missing something?

[josevalim]

The current override may not make much sense. The purpose of null session is to continue serving requests, if you are going to redirect, you may as well not use null session? Other than that, it looks fine.

That's exactly the problem with null session, there will be code that assumes a session exists, and it will definitely break. On Devise and on your application.

[ebababi]

@josevalim thank you for your response and sorry for coming back to this, but let's document a best practice or a recommendation on this. The question would be: What's the recommendation for handling invalid authenticity tokens?

My understanding is that there could be two flows:

1. Refuse the request as invalid, i.e. respond a 400 or a 498 (non-standard, but common) status.

   To implement this someone could leave protect_from_forgery with: :exception and then write a rescue block handling the error:

   rescue_from ActionController::InvalidAuthenticityToken do
     render text: 'Token expired/invalid', status: 498
   end

2. Empty the session. This should force an authentication, as if the request came in with an empty session.

   So, when protect_from_forgery with: :null_session is applied, someone could write this to handle the case:

   def handle_unverified_request
     super # call the default behaviour, including Devise override
     authenticate_user!
   end

there will be code that assumes a session exists, and it will definitely break

I believe that code may assume a session exists, if measures are taken in order to ensure that a session actually exists. Since Devise already overrides handle_unverified_request to do its stuff, it already takes a part in the flow of the handling of invalid authenticity tokens. Don't you think it's easy to write something like (2) above in Devise handle_unverified_request override?

In that case we are not limiting user choices since:

1. protect_from_forgery with: :exception: will raise an exception, code from Devise will not run, user should handle the exception as she wish.
2. protect_from_forgery with: :null_session: if we write something like (2) above in Devise handle_unverified_request override, Devise will re-authenticate user and the fact that "a session exists" is ensured.
3. protect_from_forgery with: :reset_session: the same as :null_session.

In that way, we could DRY this up and offer a general solution on this. I would be happy to write a PR for this, but I would face the following problems:

1. How without a session would I know which scope to authenticate?
2. Since this is an override on ApplicationController, how do I know if the inheriting controller actually needs authenticate_scope! and for which actions?

In my mind it would be easier if we could control the execution order in the before_action chain and somehow enforce the authenticate_scope! to execute after the protect_from_forgery callback.

What's your thoughts?