GitHub OAuth responds with 200 on error

[blakeembrey]

What happened?

The line here assumes the error of the response has been handled, but the GitHub authorization response returns a 200 with the error property set in JSON. I believe the correct behavior would be to check if error has been set before checking for access_token and return early.

Version

1.0.4

Runtime

Cloudflare Workers

Runtime Details

N/A

Code to reproduce

  const response = await oauth2.authorizationCodeGrantRequest(
    as,
    client,
    parameters,
    getRedirectUri(),
    oauth2.generateRandomCodeVerifier()
  );

  const result = await oauth2.processAuthorizationCodeOAuth2Response(
    as,
    client,
    response
  );

Required

• [X] I have searched the issues tracker and discussions for similar topics and couldn't find anything related.
• [X] I agree to follow this project's Code of Conduct

[panva]

The line here assumes the error of the response has been handled, but the GitHub authorization response returns a 200 with the error property set in JSON.

That is unfortunate.

I believe the correct behavior would be to check if error has been set before checking for access_token and return early.

I believe the correct behaviour would be for github to stick to the protocol definition so that client software does not have to work around their quirks. Please make sure to open an issue with GitHub support.

[blakeembrey]

I guess the question is whether you plan to support servers in the real world or just things strictly following the specification. Because I believe other things will pop up like this, and I'm happy to open PRs to fix them. Otherwise I'll probably just be stuck spinning my own library/fork which is what I had prior to discovering you'd done so much work already 🙏

[panva]

I guess the question is whether you plan to support servers in the real world

There are already things in place in this very minimal library to make GitHub's interfaces happy (see if you can spot them ;)). In general it depends on the server, the amount of work, or how far it tips the non-conformance needle.

I will open an issue with GitHub first. They're fast and responsive folk so before maiming my code, let's see if they can fix theirs.

[blakeembrey]

There are already things in place in this very minimal library to make GitHub's interfaces happy

Unfortunately I couldn't spot them, you might have to point it out, sorry. For now I just wrapped everything in a try/catch but would prefer to handle it better. I think I'd have to clone the response myself and read it to check this.

[panva]

Unfortunately I couldn't spot them, you might have to point it out, sorry.

Having to send an accept: application/json header, github otherwise sends back application/x-www-form-urlencoded responses. No other server have I experienced this with.

[blakeembrey]

Ah, I think I’ve encountered that before but can’t remember which provider it was. I’ve always just wrapped my response handling in a try catch for parsing JSON and falling back to form parsing. I vaguely recall it being one server, maybe Instagram, that would return correctly unless an error occurred and it’d switch to using form responses.

[panva]

Github support has deferred me to the official GitHub public feedback discussions repository. I suggest you bring up their issue there.

Coming back to your point

I guess the question is whether you plan to support servers in the real world or just things strictly following the specification.

The problem is not one that makes the library not work with GitHub's early OAuth implementation so I will not add any special handling, I have prototyped the implementation and I'm not a fan of it. The green path works fine, and the fact that errors still get rejected, albeit not using the provided error from the response is fine.