Race condition when issuing access token from authorization code

[pokej6]

Issue

The OAuth2 specification has the following:

If an authorization code is used more than once, the authorization server must deny the subsequent requests.

This repository addresses this by revoking the authorization code after it has been used to generate an access token. However the way that is being done allows multiple concurrent requests to result in generated access tokens before the authorization code is revoked.

See https://github.com/thephpleague/oauth2-server/blob/master/src/Grant/AuthCodeGrant.php

public function respondToAccessTokenRequest(
        ServerRequestInterface $request,
        ResponseTypeInterface $responseType,
        DateInterval $accessTokenTTL
    ) {
        // OAuth client lookup, authorization code validation/verification, scope finalization, code challenge verification
...
        // Issue and persist new access token
        $accessToken = $this->issueAccessToken($accessTokenTTL, $client, $authCodePayload->user_id, $scopes);
        $this->getEmitter()->emit(new RequestAccessTokenEvent(RequestEvent::ACCESS_TOKEN_ISSUED, $request, $accessToken));
        $responseType->setAccessToken($accessToken);

        // Issue and persist new refresh token if given
        $refreshToken = $this->issueRefreshToken($accessToken);
...
        // Revoke used auth code
        $this->authCodeRepository->revokeAuthCode($authCodePayload->auth_code_id);

        return $responseType;
    }

An easy way to reproduce this with live debugging is to set a breakpoint prior to the revocation of the authorization code. Then make 2 or more requests with the authorization code to get an access token. Once both hit your breakpoint, allow the requests to finish. You will have 2 access codes/refresh codes/etc.

Suggestions

There are a couple options I can think of to remediate this problem.

1. Add column data to the authorization code table which identifies if a code has been used already.
2. Revoke the code as soon as validation/verification is finished but before generating tokens.

[Sephster]

This is unfortunately not an easy fix. The problem here is that the access token generation can fail. If we revoke the auth code before the access token is generated, then it fails, the client is left in a dead end state.

For the two solutions you've suggested, I think number 1 is already covered by the revokeAuthCode() function which should either remove the authcode from storage or flag it as already having been used (the implementation details are left up to the author).

For the second one, I don't think we can do this because of the aforementioned issue of the access token generation failure.

In the real world, I'm wondering how likely it is that this situation would actually occur where a client would deliberately want to act in this way? They'd have to rely on a very small window (probably miliseconds) to hit this issue

[pokej6]

In the real world, I'm wondering how likely it is that this situation would actually occur where a client would deliberately want to act in this way?

As is the case with most race conditions, it's rarely something the client wants to experience. I expect this could come up in some fringe workflows or more likely in an environment with a bad actor trying to do something nefarious.

The problem here is that the access token generation can fail. If we revoke the auth code before the access token is generated

Is this more likely to happen than the race condition? It seems like this would also be a rare occurrence, though I do agree that leaving the client in a dead end state isn't cool considering they haven't done anything wrong. Perhaps instead of a binary used/unused flag, there could be a column representing the "state" of an authorization code. This could allow the code in question to lock the code while it's being run. If the flow completes successfully, the code can be revoked. If the flow fails to generate an access token, the code can be unlocked. If multiple requests come in for the same code, only one will be able to lock it and the other one will fail the lookup (something like SELECT ... WHERE code.status != 'locked' AND...).

[Sephster]

Ok I will flag this as something to fix. I think you're right, we would likely have to have a temp hold on using the auth code until either the request fails or completes, at which point we release the lock and either revoke the token or respond saying there was an issue generating the access token.

This would need to be done in a major release. Thanks for your input on this