Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XXXOrRestrict undermines provider (security) logic #395

Closed
eikaramba opened this Issue Jan 8, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@eikaramba
Copy link
Contributor

eikaramba commented Jan 8, 2017

Steps to reproduce

  1. Use following before hooks in myservice:
find: [
    auth.verifyOrRestrict(),
    auth.populateOrRestrict(),
    custom.myhook(),
  ],
exports.myhook= function () {
  return function (hook) {
    console.log("provider",hook.params.provider);
    if (hook.params.user || !hook.params.provider) {
       //restrict even more unless internal server call
    }
}}
  1. Use browser to navigate to http://localhost:myport/myservice

Expected behavior

Myservice should be called only once with the provider="rest"

Actual behavior

Call will be made, but myhook will be called twice. First with provider="rest" (correct) and second with provider=undefined (wrong). The latter will undermine any security concept one is doing by utilizing the provider param.

The final output is the one from the second call.

Logs will show:

provider rest
provider undefined

System configuration

Module versions:
"feathers-authentication": "^0.7.12",
"feathers": "^2.0.2",
"feathers-rest": "^1.6.0",
"feathers-sequelize": "^1.4.0"

NodeJS version: 7.4.0

Operating System: win10

Browser Version: ALL

@eikaramba

This comment has been minimized.

Copy link
Contributor Author

eikaramba commented Jan 8, 2017

I know that with the new authentication and permission plugins this issue might be gone, however currently this was a big issue for me. Especially as i first thought that browser calls and internal calls are the same as both was showing up with provider=undefined. just after some debugging i learned that i can safely assume if provider=undefined, that this must be an internal call and thus i do not restrict anything. However this is not true if one is using one of the xxxOrRestrict hooks.

Also the behaviour is kind of intended as the code states https://github.com/feathersjs/feathers-authentication/blob/875bbe436c939bb5355e2bf7c83e779a31b96e27/src/hooks/verify-or-restrict.js#L32, this is done to prevent some infinite loops when calling the service again on itself (what the hook is actually doing). nevertheless, still scary if someone is relying on the provider param to restrict services

@eikaramba eikaramba changed the title verifyOrRestrict undermines provider (security) logic XXXOrRestrict undermines provider (security) logic Jan 8, 2017

@eikaramba

This comment has been minimized.

Copy link
Contributor Author

eikaramba commented Jan 9, 2017

for everyone that has the same use case as me:
Having a Service which is:

  1. accessible without any form of authentication --> public entries
  2. accessible with authentication but restricted if user is not special --> public & special entries
  3. accessible via internal call without any restrictions --> everything

I copied the code from verify and populate hooks and just customized my hook(removed feathers verify and populate hooks):

exports.myhook= function (options) {
  return function (hook) {
    if(!hook.params.token){
      if(hook.params.provider) {
        hook.params.query = hook.params.query || {};
        //only called when no authentication and not internal call --> e.g. case 1)
      }
   //code for case 3
    }else{
      var token = hook.params.token;
      var authOptions = hook.app.get('auth') || {};
      var options = Object.assign({}, authOptions, authOptions.token);
      var secret = options.secret;
      if (options.algorithm) {
        options.algorithms = [options.algorithm];
        delete options.algorithm;
      }
      return new Promise(function (resolve, reject) {
          jsonwebtoken.verify(token, secret, options, (error, payload) => {
            if (error) {
              return reject();
            }
            hook.params.payload = payload;
            return hook.app.service('users').get(hook.params.payload['id']).then(user => {
              hook.params.user = user;
              ////code for case 2
              return resolve(hook);
            }).catch(function () {
              return reject();
            })
          })
        })
    }
  };
};

@ekryski ekryski added the Bug label Jan 20, 2017

@eikaramba

This comment has been minimized.

Copy link
Contributor Author

eikaramba commented May 1, 2017

I just converted everything to AUK Release with latest authentication version. Unfortunately the approach from above does not work anymore (there is no hook.params.token anymore). How can i do different things depending whether the user is authenticated (user object must be attached to params) or not?

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Jun 11, 2018

The orRestrict hooks have now been deprecated. feathers-authentication-hooks now only contains queryWithCurrentUser and restrictToOwner.

Role based permissions can be achieved through feathers-permissions, conditional restrictions should be implemented in your own conditional hooks (useful: feathers-hooks-common#iff).

@daffl daffl closed this Jun 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.