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

User list gettable, users modifiable (!) by non-admin via REST API #18

Open
IBwWG opened this issue Jan 4, 2017 · 6 comments
Open

User list gettable, users modifiable (!) by non-admin via REST API #18

IBwWG opened this issue Jan 4, 2017 · 6 comments

Comments

@IBwWG
Copy link

IBwWG commented Jan 4, 2017

  1. create your admin user normally, so that the admin user already exists and the next one will be a regular user.
  2. curl -X POST -H "Content-Type: application/x-www-form-urlencoded" -H "Cache-Control: no-cache" -d 'name=i have a name&username=nammmmmmmmmmme&password=what the hey&confirmPassword=what the hey&[email protected]' "http://localhost:3030/users"
  3. paste verification link into browser
  4. curl -X POST -H "Content-Type: application/x-www-form-urlencoded" -H "Accept: application/json" -H "Cache-Control: no-cache" -d '[email protected]&password=what the hey' "http://localhost:3030/auth/local"
  5. copy the token from the previous step's response
  6. paste it appropriately in curl -X GET -H "Accept: application/json" -H "Content-Type: application/x-www-form-urlencoded" -H "Authorization: Bearer INSERT-YOUR-TOKEN-HERE" -H "Cache-Control: no-cache" "http://localhost:3030/users"

Result: list of all users, even though it was requested as a non-admin user.

Strange, because there is:

auth.restrictToOwner({ ownerField: idName }),

...in \server\services\user\hooks\index.js. That idName does actually become _id as the config files suggest.

So...why the authorization breach?

@IBwWG
Copy link
Author

IBwWG commented Jan 4, 2017

This also applies to PATCH--as a non-admin you can mess with the admin's (or anyone's) name, roles, etc.

@IBwWG IBwWG changed the title User list gettable by non-admin via REST API User list gettable, users modifiable (!) by non-admin via REST API Jan 4, 2017
@IBwWG
Copy link
Author

IBwWG commented Jan 4, 2017

Interestingly, if I add a hook function to users/hooks:

  const myHook = () => { return (hook) => { if (hook.params.provider) throw new Error("uhoh") } };
 
  return {
    all: [],
    find: [
      auth.verifyToken(),
      auth.populateUser(),
      auth.restrictToAuthenticated(),
    ],
    get: [
      auth.verifyToken(),
      auth.populateUser(),
      auth.restrictToAuthenticated(),
      auth.restrictToOwner({ ownerField: idName }),
	  myHook(),
    ],
...

...no error is thrown on step 4 in the original here. In that code I was banking on the comment at https://github.com/feathersjs/feathers-legacy-authentication-hooks/blob/master/src/restrict-to-owner.js#L21 that implies that if (hook.params.provider) will only run on external calls.

@IBwWG
Copy link
Author

IBwWG commented Jan 4, 2017

This happens on both Win7x64sp1 with Node 7.3.0 and Linux Mint 17.3 with Node 6.9.2.

@IBwWG
Copy link
Author

IBwWG commented Jan 4, 2017

I also tried this, because I noticed that there was a hook function in feathers-service-verify-reset that didn't seem to be getting used here. This would also apply to #9 I guess. Anyway it didn't work, for some reason this hook always fails because .user seems to not be populated. But here was my attempt anyway:

    get: [
      auth.verifyToken(),
      auth.populateUser(),
      auth.restrictToAuthenticated(),
      verifyHooks.restrictToVerified(),
      auth.restrictToOwner({ ownerField: idName }),
    ],

@colinduwe
Copy link
Contributor

colinduwe commented Dec 1, 2017

I reproduced your bug. Were you perhaps trying to fix the wrong service method? Step 6 is actually the find method. So a fix might be

    find: [
      auth.verifyToken(),
      auth.populateUser(),
      auth.restrictToAuthenticated(),
      auth.restrictToRoles({
        roles: ['superAdmin', 'admin'],
      }),
    ],

@colinduwe
Copy link
Contributor

This should be fixed in this commit
0084662

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants