usefulteam / jwt-auth

WordPress JSON Web Token Authentication
https://wordpress.org/plugins/jwt-auth/
124 stars 50 forks source link

Removed whitelist. #60

Closed sun closed 1 year ago

sun commented 2 years ago

Resolves #56

Problem

Details

Proposed solution

  1. Remove the whitelist.
  2. Point users to existing plugins offering the same or similar functionality, in case anyone needs it; e.g.:
sun commented 2 years ago

✅ Tests are still passing

$ URL=http://localhost USERNAME=myuser PASSWORD=mypass composer run test
> ./vendor/bin/phpunit
PHPUnit 9.5.13 by Sebastian Bergmann and contributors.

.............                                                     13 / 13 (100%)

Time: 01:08.257, Memory: 6.00 MB

OK (13 tests, 110 assertions)
sun commented 2 years ago

Just noticed that the WordPress.org support forum for the plugin is full of complaints about this behavior, too: https://wordpress.org/support/plugin/jwt-auth/

cedricDevWP commented 2 years ago

It would be in this case to add a blacklist so that the JWT verification is carried out for certain API requests

cedricDevWP commented 2 years ago

62

I created a new version which allows to accept all requests and to block some if necessary

sun commented 2 years ago

It would be in this case to add a blacklist so that the JWT verification is carried out for certain API requests

@cedricDevWP Could you expand on which certain API requests you mean? The JWT verification is still done without the whitelist, if the client passes a JWT for authentication. But the client can also use a different way to authenticate.

If you want to require users to be authenticated for certain other routes, you should instead implement a filter to modify the REST route definitions to change the permission callback of those routes accordingly.

      'permission_callback' => 'is_user_logged_in',

A whitelist/blacklist duplicates the existing access permission definitions of all routes. It will always be incomplete and incorrect unless site developers are extending the list for all REST routes on the site. But that is a massive effort and completely unnecessary, because the routes already have a permission system.

cedricDevWP commented 2 years ago

I understand the problem, the solution I proposed is a workaround to the problem.

In my case I authorize all api calls (ex: wp-rocket, contact form...) except that I block certain routes for my personal api.

Did you consider implementing a new value for "permission_callback" in your solution to add for example "jwt-auth" to call this extension?

You indicated the possibility of adding the extension https://wordpress.org/plugins/disable-rest-api-and-require-jwt-oauth-authentication/ except that this extension has not been updated since 4 years ... so I do not recommend.

mikmikmik commented 2 years ago

@sun I'm using wordpress as a rest api so users can register inside a vuejs app and get a JWT token. How would you do that if there's no whitelist?

sun commented 2 years ago

@cedricDevWP If I understand you correctly, then you would want to have a permission callback function, so that a route can require authentication via JWT (not allowing any other way of authentication as would be the case with the regular "is_user_logged_in" callback)?

We could consider it. But it sounds insufficient to do actual permission checks. For example, if you have some personal endpoints as you say, then you would normally use a permission callback that requires not only an authenticated user but also a certain user role or user permission/capability.

@mikmikmik Can you elaborate more on how you're using the routes that are not whitelisted? – The jwt-auth plugin does not influence whether the user register route is available. The whitelist only blocks access to routes that are not whitelisted, so after removing the whitelist you can still use the user register route to register users.

Each REST endpoint has its own access permission. The jwt-auth plugin does not modify those permissions.

You can see this in the code that is removed in this PR. It is literally just simply blocking any request to a path that is not in the list, before any normal access checks are happening.

mikmikmik commented 2 years ago

@sun I built a small plugin to create a user, it registers a custom route so I don't have to supercharge the default user register route. This custom route uses wp_create_user and manipulate other custom fields. As it is for visitors it needs to be whitelisted.

Also I use a custom post type on the default wp/v2/ namespace that actually needs to be removed from the whitelist so it can only be accessible to registered users.

mikmikmik commented 2 years ago

@sun for me 'permission_callback' => 'is_user_logged_in' is not usable in my context... I use Wordpress as a Headless CMS and I only use the REST api through json. I chose this because I might still use wordpress front for some static parts like home and for SEO. Either whitelist or blacklist works for me, so I agree with @cedricDevWP I think this feature is important.

sun commented 2 years ago

@mikmikmik You are saying that your custom post type must only be accessible for registered users. Then this case would be covered by setting appropriate permissions/callbacks - ideally when registering the content type already, so that the permissions are set correctly and access is restricted throughout the whole application and not just the REST endpoints.

I did not understand your second comment. I believe that is how everyone is using WordPress with the jwt-auth plugin. Did you mean that you are relying on the whitelist to block access to all endpoints for requests that do not authenticate with a JWT?

In the examples that were provided thus far, I'm missing the information how access is restricted to desired users outside of the REST API? Is access to the content types and resources not protected via user permissions? 🤔 Such a setup would mean that a carefully crafted request could easily access the data and create, edit, and delete the data…?

I'm worried that the whitelist sets a false sense of security, especially with its current default configuration, causing sites to be vulnerable due to insufficient access configuration.

mikmikmik commented 2 years ago

@sun My users won't know about wordpress, they will never see the admin or login, this is to me the purpose of using REST API and JWT. It is a PWA app and I will also build that app with ionic at some point so it will be outside of the browser in full app context (It would be weird to have a wordpress admin in there). So regular access permissions don't apply in this context.

Did you mean that you are relying on the whitelist to block access to all endpoints for requests that do not authenticate with a JWT?

I don't think whitelist works like that, correct me if I'm wrong but I think it behaves as if the plugin does not exist for these endpoints. For exemple if I don't whitelist my custom user creation endpoint it asks for the token, so I whitelist it and it works like before (no auth required)

In the examples that were provided thus far, I'm missing the information how access is restricted to desired users outside of the REST API?

I don't think it is the realm of the plugin, these are Wordpress security rules. I don't understand your confusion, this plugin gives REST API the possibility to use a JWT token, whitelist/blacklist gives the developer the possibility of choosing which endpoints needs it, and leaves 'normal' wordpress behavior or other plugins or filters to take care of it.

I'm worried that the whitelist sets a false sense of security, especially with its current default configuration, causing sites to be vulnerable due to insufficient access configuration.

To me this plugin provides the possibility to add JWT Authentication to some endpoints, it is not a security suite for wordpress, personally I use filters to unset some endpoints like users for example because I don't want them to be exposed. But default wordpress behavior might suit other people, and this plugin shouldn't force the token everywhere, it could be an option though.

mikmikmik commented 2 years ago

@sun Just to clarify a bit more, when a user is "logged in" with the JWT, he doesn't have access to the admin, or pages supposed to be accessible to his role, because he never logged there. To me this is normal behavior of a headless CMS.

mikmikmik commented 2 years ago

@sun You can tell me if I'm using it the wrong way.

pesseba commented 2 years ago

Hi @sun , the whitelist is not working without pretty permalinks... There are another method to call rest api, like this: http://yoursite.com/?rest_route=/wp/v2 So, the whitelist in plugin ignore this kind of call with the route as a parameter

Check the note in wordpress documentation: https://developer.wordpress.org/rest-api/extending-the-rest-api/routes-and-endpoints/

Alert:On sites without pretty permalinks, the route is instead added to the URL as the rest_route parameter. For the above example, the full URL would then be http://example.com/?rest_route=/wp/v2/posts/123

So I agree to remove the whitelist from jwt-auth...

dominic-ks commented 2 years ago

@pesseba @sun I previously was against removing the whitelist, but actually now agree it doesn't fit well in the plugin, though I think that the reason it doesn't is this that @sun said previously:

A whitelist/blacklist duplicates the existing access permission definitions of all routes. It will always be incomplete and incorrect unless site developers are extending the list for all REST routes on the site. But that is a massive effort and completely unnecessary, because the routes already have a permission system.

i.e. if a route should be limited to logged-in users, then the route should be registered this way or be amended to behave this way via one of the available filters. If a route requires a user to be logged in, it shouldn't really matter what method is used to authenticate them.

The point does still stand, though, that I think there should be plenty of notice to existing users who may update automatically from wp.org that this (and the refresh token change) is coming before they go live on the wp.org repo. There may be sites that, for whatever reason, are relying on this plugin to protect ordinary posts, for example, that would suddenly become publicly available with this change.

pesseba commented 2 years ago

The point does still stand, though, that I think there should be plenty of notice to existing users who may update automatically from wp.org that this (and the refresh token change) is coming before they go live on the wp.org repo. There may be sites that, for whatever reason, are relying on this plugin to protect ordinary posts, for example, that would suddenly become publicly available with this change.

Hi @dominic-ks I agree with you... My point is the whitelist is not working at all, because the call without pretty permalinks bypass the whitelist feature anyway... So this is a huge bug. I suggest to put a note/statement in readme about the whitelist remotion and a note inside wordpress admin when users update it. Something like this:

image

mikmikmik commented 2 years ago

If you remove whitelist how do we keep some points without JWT auth? Because by default all custom endpoints are intercepted. I need this feature for user creation, account custom activation, password renewal...

mikmikmik commented 2 years ago

It's either we list the points that needs auth or use whitelist if you force it on all points...

dominic-ks commented 2 years ago

If you remove whitelist how do we keep some points without JWT auth? Because by default all custom endpoints are intercepted. I need this feature for user creation, account custom activation, password renewal...

The point is that this plugin shouldn't decide whether authentication is required for a particular rest route.

For example, say I have a CPT called books with a route like /wp-json/me/v1/books, I could do two things:

  1. If I want users to be able to read published books without being logged in, when creating that route I'd omit the permissions callback, or I'd set it to always return true.

  2. If I want users to need to be logged in to read published books, when creating, I'd add the permissions callback to ensure that a user is logged in.

In either case, there should be no need to whitelist the endpoint, the plugin will check for a JWT in the auth header and will either deem the user to be logged in or not.

In the first case, whether a user is logged in or not, they will be able to access this route, because you don't need to be logged in. This call may also include, for example, private books if the user is logged in and are an admin, for example.

In the second case, if the user is not logged in, the route will return an error because users have to be logged in to read from that route. If a JWT is provided, the user will be logged in and they will pass the permissions callback.

mikmikmik commented 2 years ago

We can use 'permission_callback' => '__return_true' on custom endpoints but for plugins with their own endpoints you force it to them.

dominic-ks commented 2 years ago

The point does still stand, though, that I think there should be plenty of notice to existing users who may update automatically from wp.org that this (and the refresh token change) is coming before they go live on the wp.org repo. There may be sites that, for whatever reason, are relying on this plugin to protect ordinary posts, for example, that would suddenly become publicly available with this change.

Hi @dominic-ks I agree with you... My point is the whitelist is not working at all, because the call without pretty permalinks bypass the whitelist feature anyway... So this is a huge bug. I suggest to put a note/statement in readme about the whitelist remotion and a note inside wordpress admin when users update it. Something like this:

image

OK, cool, we're on the same page. I also think it'd be good to present a warning in the plugins update page before they update to include these features, I've seen plugins like W3TC and WooCommerce do this in warning about major changes. I haven't done this myself before but I can check it out and see how they do it.

dominic-ks commented 2 years ago

We can use 'permission_callback' => '__return_true' on custom endpoints but for plugins with their own endpoints you force it to them.

That's true, but what's the scenario where another plugin is registering a route that does not require authentication, and you feel this plugin should be forcing it to require authentication? Bearing in mind, all it will do is make sure that a user is logged in, it won't be checking their role or capabilities at all.

As an aside, I believe you can use the rest_endpoints filter to make changes to routes that have already been registered, which could be used if you wanted to make changes to the permissions callback for a particular route.

mikmikmik commented 2 years ago

We can use 'permission_callback' => '__return_true' on custom endpoints but for plugins with their own endpoints you force it to them.

That's true, but what's the scenario where another plugin is registering a route that does not require authentication, and you feel this plugin should be forcing it to require authentication? Bearing in mind, all it will do is make sure that a user is logged in, it won't be checking their role or capabilities at all.

As an aside, I believe you can use the rest_endpoints filter to make changes to routes that have already been registered, which could be used if you wanted to make changes to the permissions callback for a particular route.

I use a plugin that handles password reset with a generated code sent by email that doesn't work without the whitelist, although it has a return true in its permission_callback. Actually every public endpoint turns private once you install this plugin, hence the use of a whitelist. Otherwise you need to rely on the permission callback but it doesn't seem to be the case... or I'm doing something wrong maybe.

mikmikmik commented 2 years ago

Or maybe that's what you're implying by removing the whitelist: leave public endpoints public

dominic-ks commented 2 years ago

I use a plugin that handles password reset with a generated code sent by email that doesn't work without the whitelist, although it has a return true in its permission_callback. Actually every public endpoint turns private once you install this plugin, hence the use of a whitelist. Otherwise you need to rely on the permission callback but it doesn't seem to be the case... or I'm doing something wrong maybe.

Right, I see what you're saying. So to confirm, when the whitelist is removed, your other plugin will just work without needing to whitelist. i.e. it will be the equivalent of everything being whitelisted.

What plugin are you using for password resets btw?

Or maybe that's what you're implying by removing the whitelist: leave public endpoints public

Yes that's right.

mikmikmik commented 2 years ago

I use a plugin that handles password reset with a generated code sent by email that doesn't work without the whitelist, although it has a return true in its permission_callback. Actually every public endpoint turns private once you install this plugin, hence the use of a whitelist. Otherwise you need to rely on the permission callback but it doesn't seem to be the case... or I'm doing something wrong maybe.

Right, I see what you're saying. So to confirm, when the whitelist is removed, your other plugin will just work without needing to whitelist. i.e. it will be the equivalent of everything being whitelisted.

What plugin are you using for password resets btw?

bdvs-password-reset

dominic-ks commented 2 years ago

bdvs-password-reset

Cool! I'm the author of that plugin and use it in combination with this one as well, so I'll always be making sure they work together anyway.

mikmikmik commented 2 years ago

bdvs-password-reset

Cool! I'm the author of that plugin and use it in combination with this one as well, so I'll always be making sure they work together anyway.

Thank you for making this plugin, it works great! :)

mikmikmik commented 2 years ago

@sun actually said it already I missed it, my bad, I didn't realize it meant everything whitelisted: The jwt-auth plugin does not influence whether the user register route is available. The whitelist only blocks access to routes that are not whitelisted, so after removing the whitelist you can still use the user register route to register users.