search

cmd-johnson / deno-oauth2-client

Minimalistic OAuth 2.0 client for Deno.
MIT License
45 stars 9 forks source link

Fail with better error message when the access token cannot be retrieved #25

Closed zifeo closed 1 year ago

zifeo commented 1 year ago

I had some of our tokens misconfigured and GitHub was answering 200 from https://github.com/login/oauth/access_token with the following body:

{                                                                                                                                             
  error: "incorrect_client_credentials",                                                                                                      
  error_description: "The client_id and/or client_secret passed are incorrect.",                                                              
  error_uri: "https://docs.github.com/apps/managing-oauth-apps/troubleshooting-oauth-app-access-token-request-erro"... 32 more characters     
}

Instead, I received Invalid token response: missing access_token and it took me a while to debug as I had to inject the library to find the root issue. Indeed GitHub implementation is far from ideal, yet I believe this corner case should become more visible when it happens (e.g. showing the body without sensitive fields or checking whether there is an error field).

cmd-johnson commented 1 year ago

Returning a 200 status code when there actually was an error is definitely weird behavior on GitHub's side here. I just tried it myself with an invalid code and I also get a 200 for that…

In these cases the module throws TokenResponseErrors, which have a .response property that you could use to access the full response returned by the token endpoint.

Although I just saw that the module isn't doing a response.clone() before getting the response's body, so you wouldn't be able to access the actual response body on your end right now :thinking: However this should easily be fixed by simply changing this line to body = await response.clone().json();

Do you think that would be enough in this case? It feels a bit weird to check for an error field when the server actually responded with 200, although I could check for an error field in any case and handle the response as an error if the field is present.

zifeo commented 1 year ago

@cmd-johnson I have reported the unexpected behaviour to GitHub (#2187732). Let's see what they answer.

Your suggested fix sounds good to me 👍.

zifeo commented 1 year ago

@cmd-johnson GitHub acknowledged the bug, and it has been added in their backlog. However they do not expect a quick change as their bug-fixing is breaking.