Skip to content
This repository has been archived by the owner on Mar 22, 2021. It is now read-only.

Namespaced model could cause a severe security issue if implemented as the doc suggests #228

Open
serco-chen opened this issue Oct 16, 2018 · 1 comment · May be fixed by #230
Open

Namespaced model could cause a severe security issue if implemented as the doc suggests #228

serco-chen opened this issue Oct 16, 2018 · 1 comment · May be fixed by #230

Comments

@serco-chen
Copy link

This is what doc suggests

If you're using a namespaced model, Knock won't be able to infer it automatically from the method name. Instead you can use authenticate_for directly like this:

class ApplicationController < ActionController::Base
  include Knock::Authenticable
    
  private
  
  def authenticate_v1_user
    authenticate_for V1::User
  end
end

class SecuredController < ApplicationController
  before_action :authenticate_v1_user
end

This gem relies on method_missing to do the actuall authentication work.

However authenticate_v1_user defined in ApplicationController will override it and return a nil when lacking a valid token, what you really need is a head(:unauthorized) response.

I could be wrong since I'm not familiar with the gem. IMO this is a big security issue.

@fabio-padovani89
Copy link

I totally agree with @serco-chen.

I suggest you to update the documentation with something like:

def authenticate_v1_user
  unauthorized_entity('V1::User') unless authenticate_entity('V1::User')
end

sakuraineed added a commit to sakuraineed/knock that referenced this issue Oct 28, 2018
…allback (nsarno#228)

- Add failing test authenticate_for namespaced model
sakuraineed added a commit to sakuraineed/knock that referenced this issue Oct 28, 2018
…allback (nsarno#228)

- Add unauthorized_entity to authenticate_for's default

- Remove unused argument from unauthorized_entity
sakuraineed added a commit to sakuraineed/knock that referenced this issue Oct 28, 2018
…allback (nsarno#228)

- Add unauthorized_entity to authenticate_for's default

- Remove unused argument from unauthorized_entity
@andrerpbts andrerpbts mentioned this issue Dec 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants