Delegating to devise's built-in authenticate_user! introduces security risk in API context

[joefiorini]

I'm building a JSON API with an Ember front-end. Therefore, I want to be able to turn of CSRF validation for JSON requests, as references a number of times in previous issues. If I do that, requiring authentication tokens is the only way to secure JSON requests.

However, based on what I can see in https://github.com/gonzalo-bulnes/simple_token_authentication/blob/master/lib/simple_token_authentication/acts_as_token_authentication_handler.rb#L23-L26, this gem delegates to Devise's default authentication mechanism after doing token authentication.

Devise's default authentication makes every request fallback to cookie authentication. That means any existing users will still be using cookies until they sign out & sign back in. Also, when I disable CSRF validation on JSON requests, I'll still be vulnerable to CSRF attacks. The reason I want token authentication is so I can force clients to send identifying information with every request. Falling back to devise's authentication seems to defeat that purpose.

I can think of a few possible ways to solve this:

1. Remove the fallback entirely, but that might not work well for existing users
2. Only fallback for non-JSON requests by default (maybe with an opt-in for the previous behavior)
3. Disable fallback for all requests with opt-in to fallback on all requests

I'm thinking I might implement option 2 and send a PR. @gonzalo-bulnes any thoughts on this?

[adg29]

Bump @gonzalo-bulnes

[gonzalo-bulnes]

Hi @joefiorini and @adg29,

First of all, @joefiorini, your comment makes a lot sense to me. But I not sure at all about the solution to implement to this issue. Keeping your numbering, here are some thoughts:

1. Removing the authenticate_entity! fallback entirely

Simple Token Authentication was first designed as a replacement for the token_authenticatable Devise strategy. The fallback in that context is quite natural and I think it still makes sense to many users (Off-topic: @bryanaka that may partially respond to your question).
Since it's not the only use case (I'm not even sure it is the most used) I totally agree the other use cases must be supported (and safe to use), but not by removing that one arbitrary. If the bug affected every scenario it would be a different story, but it doesn't. In fact, reading at your own comment, I believe that we agree on this point.

2. Only fallback for non-JSON requests by default (with opt-in)

That might be, but I don't like much the "non-JSON" part of it. Simple Token Authentication should not, IMO, care about the content types you work with. What about XML API in such a case? But there is something to keep from this idea.

3. An global fallback option (opt-in or opt-out)

Could be, but I find no reason a priori to make that option global. (I may, and some people do, want to serve both an API and a web app from the same application.)

New 4. A fallback option, to be used from acts_as_token_authentication_handler_for

That would flexible enough to keep configuration at a controller level. I tend to prefer an opt-out option to keep the gem behaviour consistent, but I also think that the opt-in or opt-out question is not the main point here.

The usage could be something like:

# app/controllers/api/posts_controller.rb

class API::PostsController < ApplicationController
  acts_as_token_authentication_handler_for User, fallback_to_devise: false
  # ...
end

Eventually, not disabling the fallback for JSON, XML requests (extensible list) could print a warning somewhere which would to an API-specific documentation section. (The idea would be to provide additional support based on content type without depending on it.)

@ both: What do you think?

[gonzalo-bulnes]

Hello!

We are thinking about making the Devise fallback (authenticate_resource!) optional, I think the last parts of these two comments, which discuss the possible implementation, may interest you. Please feel free to participate of the discussion. : )

@sebvst @lfglopes @nikue

[onemanstartup]

Fallback option is definitely the best way.

[lfglopes]

Thanks for the call 👍

On one hand I think the first approach is correct as well since IMO is the expected behaviour for anyone that has been using devise, but since implementing it right now can be quite dangerous I think the fallback alternative (4th) is the best option at this stage.

The 3rd option I guess could create a problem similar to the one I'm facing now (#53) so it doesn't really seem like the way to go.

[jbender]

+1 for opting-in to the fallback

[ngmaloney]

👍 for making fallback_to_devise optional

[donbobka]

before_filter :authenticate_entity_from_token! should be optional too
Example:
I want make a controller with only specific actions should check authorization

class HomeController < ApplicationController
  acts_as_token_authentication_handler_for User, before_filter: false

  before_filter :authenticate_entity_from_token!, only: :destroy

  def index
    # ... Accessible for unauthenticated users ...
  end

  def destroy
    # ... Only for authenticated users...
  end
end

My implementation in PR #61