During refresh token grant if token is invalid or expired return status code should be 400

love-bhardwaj commented 1 year ago

Preflight checklist

[X] I could not find a solution in the existing issues, docs, nor discussions.
[X] I agree to follow this project's Code of Conduct.
[X] I have read and am following this repository's Contribution Guidelines.
[ ] This issue affects my Ory Network project.
[ ] I have joined the Ory Community Slack.
[ ] I am signed up to the Ory Security Patch Newsletter.

Describe the bug

Issue when you implement revocation with your own TokenRevocationStorage.

The grant type refresh_token used to refresh and access the token should returns errors as specified in RFC.

The authorization server responds with an HTTP 400 (Bad Request) status code (unless specified otherwise) and includes the following parameters with the response:
     invalid_grant:
           The provided authorization grant (e.g., authorization
           code, resource owner credentials) or refresh token is
           invalid, expired, revoked, does not match the redirection
           URI used in the authorization request, or was issued to
           another client.

The refresh token handler should return a 400 status code error for an invalid, expired, or revoked refresh token, or an ErrInvalidGrant error for Fostie. However, it currently fails to account for this scenario. As a result, if the TokenRevocationStorage method GetRefreshTokenSession implementation returns fosite.ErrInvalidGrant, it will be converted into a 500 error.

The workaround, for now, is to return fosite.ErrNotFound from the GetRefreshTokenSession implementation which is then converted to fosite.ErrInvalidGrant in the handler.

Reproducing the bug

This setup for this would require a lot more than a simple step. As such I am keeping it abstract Implement the TokenRevocationStorage interface and from GetRefreshTokenSession return fosite.ErrInvalidGrant.

Relevant log output

No response

Relevant configuration

No response

Version

v0.42.2

On which operating system are you observing this issue?

Linux

In which environment are you deploying?

Kubernetes with Helm

Additional Context

No response

james-d-elliott commented 1 year ago

Users implementing their own storage interface implementation should realistically carefully model their implementation similar to the memory example as this is often used in the testing.

You will notice that upon inspection that the GetRefreshTokenSession of this specific struct returns only two specific errors. If the token does not exist it returns fosite.ErrNotFound, and if it returns fosite.ErrInactiveToken if the token is inactive. As such I don't believe this is a workaround but is the intended method to communicate this issue from that interface but it's just not documented (the library is mostly intended for use with hydra however I think there is reasonable argument that this should be documented to help avoid bugs over on hydra itself).

Other errors are usually indicative of SQL errors and the like. I can see an argument for enhancing this interaction (globally) where if the error is a *fosite.RFC6749Error that is appropriate for the request type that it is just returned however this could also lead to serious bugs with other implementations. This kind of behavior is also a lot harder to communicate clearly.

love-bhardwaj commented 1 year ago

Please note that returning fosite.ErrInactiveToken from the store means that the response status code is 401 fosite/errors.go which is not the correct response status as per RFC as it should be 400.

In case where refresh token is invalid, expired, or revoked it should return an invalid_grant error with a 400 status code.

dtam-cybozu commented 1 year ago

Bumping this issue, I've also observed that Ory returns a 401 and token_inactive (a string which I can't find in the spec at all) when a refresh token is reused, rather than 400 and invalid_grant as one would expect by reading the spec.

Happy to write a test and a more complete/fleshed out issue for this if you agree that this is a bug.

ory / fosite