AuthCodeGrant doesn't reply with invalid_grant on bad "code" request paramter, but with invalid_request

[paulmhh]

https://www.rfc-editor.org/rfc/rfc6749#section-5.2
says:
invalid_grant
The provided authorization grant (e.g., authorization
code[...]) is
invalid, expired, revoked, does not match the redirection
URI used in the authorization request, or was issued to
another client.

but

oauth2-server/src/Grant/AuthCodeGrant.php

Line 126 in 2ed9e5f

} catch (LogicException $e) {

throws a "invalid_request" instead. That is not RFC conform

[paulmhh]

fix:
#1431

[Sephster]

Thanks for this PR. I understand why you raised it but I think the code should remain as is. At present, we throw a LogicException if we have an issue decrypting the code. This could be because the code provided is invalid, but it could also be for a whole host of other reasons such as a missing encryption key on the server, an integrity check violation, type issues etc.

The section of code you highlighted just captures that decryption failed. If decryption is successful, we then validate the auth code in the validateAuthorizationCode function which will throw invalidGrant if the auth code has been revoked for example.

If implementations are correct, we shouldn't ever really come across this error anyway. I think I'm therefore inclined to leave the code as is as we cannot say for definite that a bad auth code caused this error.

[paulmhh]

Quay for example sends a "badcode" code to establish server availability and expects "invalid_grant" to proceed using that OAuth2 / OIDC server.

https://github.com/quay/quay/blob/3c8ed17b171b9923f35e9b9b8ff30dd00baa1f95/config-tool/pkg/lib/shared/validators.go#L949

a code like "badcode" is not a hex encoded binary data and therefore fails in
\Defuse\Crypto\Encoding::hexToBin
which is then catched and thrown as
\Defuse\Crypto\Exception\WrongKeyOrModifiedCiphertextException
in
\Defuse\Crypto\Crypto::decryptInternal

which makes it hard to distinguish between client data error and server ("wrong key") issue.
\League\OAuth2\Server\CryptTrait::decrypt
whould have to examine the exception message, not very nice.

It is a violation of the RFC though and as you claim to be "a standards compliant implementation", maybe keep the issue open?

[Sephster]

I've had a stab at fixing this on reflection. There probably is something we can do to improve the situation. It isn't perfect as there is a case where the authcode might be valid but the key/password might have changed but we can't distinguish between these cases. In this event, I've opted to respond with invalid_grant anyway as it is more likely that the code will be incorrect.

[Sephster]

would appreciate any feedback you have prior to merging. Thanks!

[paulmhh]

it solves my problem, it takes care of server side key issues, would be happy to see this merged. thank you