Tmeister / wp-api-jwt-auth

A simple plugin to add JSON Web Token (JWT) Authentication to WP REST API
GNU General Public License v2.0
558 stars 161 forks source link

Fix Gutenberg compat vel. infinite loop #138

Closed andrzejpiotrowski closed 5 years ago

andrzejpiotrowski commented 5 years ago

This PR:

What happen: Token validation fires on determine_current_user hook, and translations which were there was trying to get user preferences, what invokes function to login user, which fires determine_current_user again creating an infinite loop, which ends up in 'out of memory' errors and mentioned in gutenberg ticket status code 500.

PHP Fatal error: Out of memory (allocated 161480704) (tried to allocate 262144 bytes) in \wp\wp-includes\class-wp-hook.php on line 310

Related issue: https://github.com/Tmeister/wp-api-jwt-auth/issues/126

andrzejpiotrowski commented 5 years ago

Alternatively instead of removing translations from this place we could hook into determining locale, and pre-set it to avoid executing part of code that is trying to determine user, something like adding filter to the top of validate_token() method: add_filter( 'pre_determine_locale', 'get_locale' );

N-Molham commented 5 years ago

I solved this problem at the time by instructing the team to make sure no cookies are sent in the HTTP Request, remove wordpress_logged_in_* cookie from the request if it's there which fallback to the JWT authentication

luisjoserivera commented 5 years ago

I solved this problem at the time by instructing the team to make sure no cookies are sent in the HTTP Request, remove wordpress_logged_in_* cookie from the request if it's there which fallback to the JWT authentication

How can I prevent wp receiving the cookie on rest api requests? Is that something I do in the client side or in the server?

Having to logout doesn't seem like the right answer.

N-Molham commented 5 years ago

@luisjoserivera it's the other way around, make sure to not send any cookies from the client-side that starts with wordpress_logged_in_*

luisjoserivera commented 5 years ago

@N-Molham sorry to bother but how can I do that?

My case: www.domain.com root holds my PWA. www.domain.com/api holds my WP installation.

If I'm not mistaken i guess the browser automatically attach the cookie to the request since the request is coming from the same domain.

N-Molham commented 5 years ago

@luisjoserivera no worries mate, you have to make the requests using something like Postman for testing with cookies removed from the request header. And if you are making the request using js library (ex: jQuery) here is a sample code https://stackoverflow.com/questions/2320110/how-do-i-set-a-cookie-header-with-xmlhttprequest-in-javascript just make sure yo set withCredentials to false

luisjoserivera commented 5 years ago

@N-Molham I'm using Axios (XMLHttpRequest). XMLHttpRequest specification supplies the property option withCredentials: false which lets you make the request without cookies, but sadly that option gets ignored if it's a same-domain request.

In the other hand, the Fetch API has a very similar option credentials: "omit" that work even under the same domain. Sadly I don't feel this feature is reason enough to swap Axios to vanilla Fetch if you take into account Fetch low browser compatibility compared to XMLHttpRequest.

Anyway... after 13 long hours of reverse-engineering WP I came up with 3 workarounds to solve this problem:

Option 1 Via Apache, using RequestHeader edit ... to remove the wordpress_logged_in cookie under specific conditions. I'm attaching a custom header X-Authenticate-With: JWT then in Apache I check if that header is present in the request and if the value is equal to JWT it automatically removes any cookie starting with wordpress_logged_in_.

Option 2 Didn't bother to implement because it was based on automatically login out any active standard authentication or simply expiring the cookie. I didn't liked this one... felt kinda nasty having to unathenticate one method for the other to work.

Option 3 In progress - Via WP, removing the filters that handle the auto auth cookie login. I tested and it works, but I want to limit this action to just REST requests in order not to fall under OPTION 2. Now theres the need to implement a custom way to identify requests made to the REST API (no biggie) because the in-core-solution (REST_REQUEST constant) runs after the auto auth, which I think is the main reason this conflict is reported in almost any JWT related plugin.

I guess reports of this conflict aren't very common because most people using JWT Auth in WP are doing CORS.

PD: There is a non-standard option for XMLHttpRequest that behaves like credentials: "omit" of Fetch, XMLHttpRequest.mozAnon, but just works in Mozilla browsers 👎 I've read around that Chromium had plans to do their own but it seems like it was discarded.