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

Removed whitelist. #60

Merged
merged 2 commits into from Dec 26, 2022

Conversation

sun
Copy link
Collaborator

@sun sun commented Mar 14, 2022

Resolves #56

Problem

Details

  • It is not clear why the whitelist functionality is included in the plugin. Based on the code, it seems to be an attempt to require JWT authentication for all REST API endpoints and not allow anonymous requests or other means of authentication anymore.
  • It seems the whitelist predates wider use of the REST API in the WordPress ecosystem as well as the built-in application passwords in WordPress Core. More endpoints are using an authentication (but not JWT) today.

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 sun self-assigned this Mar 14, 2022
@sun
Copy link
Collaborator Author

sun commented Mar 14, 2022

✅ 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
Copy link
Collaborator Author

sun commented Mar 14, 2022

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
Copy link
Contributor

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

@cedricDevWP
Copy link
Contributor

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

@sun
Copy link
Collaborator Author

sun commented Mar 17, 2022

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.

@sun sun mentioned this pull request Mar 17, 2022
@cedricDevWP
Copy link
Contributor

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
Copy link

mikmikmik commented Mar 18, 2022

@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
Copy link
Collaborator Author

sun commented Mar 19, 2022

@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
Copy link

@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
Copy link

@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
Copy link
Collaborator Author

sun commented Mar 19, 2022

@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
Copy link

mikmikmik commented Mar 20, 2022

@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
Copy link

@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
Copy link

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

@pesseba
Copy link
Collaborator

pesseba commented Nov 17, 2022

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
Copy link
Collaborator

@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.

Copy link
Collaborator

@pesseba pesseba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I say go to remove whitelist...

@pesseba
Copy link
Collaborator

pesseba commented Nov 17, 2022

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
Copy link

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
Copy link

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

@dominic-ks
Copy link
Collaborator

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
Copy link

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

@dominic-ks
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link

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
Copy link

mikmikmik commented Nov 18, 2022

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

@dominic-ks
Copy link
Collaborator

dominic-ks commented Nov 18, 2022

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
Copy link

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
Copy link
Collaborator

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
Copy link

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
Copy link

@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.

Copy link
Contributor

@contactjavas contactjavas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sun ! After reading the explanation from you, @dominic-ks , and @pesseba , I agree as well to remove the whitelist feature.

@contactjavas contactjavas merged commit 8408773 into usefulteam:master Dec 26, 2022
@sun sun deleted the fix/remove-whitelist-sun branch March 15, 2023 17:57
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

Successfully merging this pull request may close these issues.

UI / option page in admin area
6 participants