Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WriteRevocationResponse returns no error when there is an internal error #644

Open
3 of 6 tasks
mitar opened this issue Jan 18, 2022 · 4 comments
Open
3 of 6 tasks
Labels
bug Something is not working.

Comments

@mitar
Copy link
Contributor

mitar commented Jan 18, 2022

Preflight checklist

Describe the bug

WriteRevocationResponse returns 200 HTTP status code on any error except ErrInvalidRequest and ErrInvalidClient. This is problematic because I just had some bad code (internal error) but the error response was 200. So nothing was being revoked in my implementation, but nobody really detected this for some time.

Reproducing the bug

Pass some regular error to WriteRevocationResponse.

Relevant log output

No response

Relevant configuration

No response

Version

0.42.0

On which operating system are you observing this issue?

No response

In which environment are you deploying?

No response

Additional Context

No response

@mitar mitar added the bug Something is not working. label Jan 18, 2022
@mitar
Copy link
Contributor Author

mitar commented Jan 18, 2022

So the error was ErrTemporarilyUnavailable returned from storeErrorsToRevocationError.

@aeneasr
Copy link
Member

aeneasr commented Apr 5, 2022

What would be the solution? The spec here requires us to send 200 except for 400 & 401

@mitar
Copy link
Contributor Author

mitar commented Apr 5, 2022

The spec is slightly confusing:

Here it references Section 5.2 of [RFC6749] but also later on says "If the server responds with HTTP status code 503" so at least 503 is allowed as well?

Other implementations seems to define 500 as return code as well: https://connect2id.com/products/server/docs/api/token-revocation

But yea, RFC6749 does not list server error (and 500 code) as possible response. But I think it would really be strange not to return 500 when there is some internal error?

@aeneasr
Copy link
Member

aeneasr commented Apr 16, 2022

I see - in the light of usuability issues I think it would be acceptable to return 500 which implies that the error needs to be retried - for example when the database is down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working.
Projects
None yet
Development

No branches or pull requests

2 participants