Wrong Token and Data Returned

[asadamatic]

Hi there, I'm facing a pretty weird issue with the /token request. That is, no matter what credentials I pass ( even invalid ), I get the data of a particular user and when I use that token for the /users/me request, it returns the data for that particular user.

[dominic-ks]

@asadamatic are you using any REST caching plugins?

[pinoceniccola]

The same thing happened to me and it returned a valid token even with no username and/or password parameters. It was kind of scary at first.

It turns out that you (or your app) are probably carrying on the refresh_token cookie in your request header from a previous response and in the current logic it takes precedence over username and password parameters.

This is basically another side effect of the same issue found in #128.

So, it would be a good idea if for the /token endpoint the refresh_token cookie is ignored. Or, at least, users should be warned of the current logic in the docs. There are many apps and frameworks that silently pass cookies over requests, it's not always easy to catch issues like these.

[asadamatic]

@dominic-ks I'm not using any such plugin. For now, I switched to the JWT Authentication and it seems to be working fine.

@pinoceniccola I checked the Rest API logs and it does contain the refresh_token in the cookie header.

[dominic-ks]

OK, so do we think this is the same issue as #128?

[asadamatic]

@dominic-ks Yes, it seems so

[pinoceniccola]

OK, so do we think this is the same issue as #128?

@dominic-ks definitely. Only difference is if the refresh_token cookie is still valid (this issue) or not (#128).

The problem lies in these lines:

jwt-auth/class-auth.php

Lines 181 to 192 in f99ca5a

	if ( isset( $_COOKIE['refresh_token'] ) ) {
	$device = $request->get_param( 'device' ) ?: '';
	$user_id = $this->validate_refresh_token( $_COOKIE['refresh_token'], $device );
	// If we receive a REST response, then validation failed.
	if ( $user_id instanceof WP_REST_Response ) {
	return $user_id;
	}
	$user = get_user_by( 'id', $user_id );
	} else {
	$user = $this->authenticate_user( $username, $password, $custom_auth );
	}

Where the refresh_token cookie always takes precedence over username / password params. This logic seems very counterintuitive, should be at least explained in the docs.

[dominic-ks]

@sun do you agreed that logic should be switched for this? If so, I'll have a look at that. Only thought is if people have apps that are currently relying on the refresh cookie taking priority over login details, but I can't imagine why they would be....

[sun]

Having username/password take precedence in case a refresh token is also passed is fine. If a client is passing both then we can safely assume that the intention was to log in with user credentials. We can change that. 👍

What I don’t understand is why the client would prompt the user to log in manually with credentials if the client still has a refresh token cookie. Normally I’d expect a client with refresh token to first try that and if it is rejected the refresh token should be deleted before the client reauthenticates. But I guess there can be edge case scenarios in which that is not as clean as in theory. Sorry for not thinking of such possibilities.

[dominic-ks]

Yeah I think the problem is that in this case the browser is retaining the refresh token itself, so perhaps app logic says user is logged out, e.g. deleted any stored tokens from local storage, but then when logging in again the browser is still sending the cookie. Could be wrong, anyway, PR created.