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

oauth2 swallows token response error if HTTP code is 200 #441

Closed
beyang opened this issue Oct 11, 2020 · 0 comments
Closed

oauth2 swallows token response error if HTTP code is 200 #441

beyang opened this issue Oct 11, 2020 · 0 comments

Comments

@beyang
Copy link

beyang commented Oct 11, 2020

What version of Go are you using (go version)? go version go1.15.2 linux/amd64
What operating system and processor architecture are you using? Ubuntu, x86

The OAuth2 spec states the authorization server should respond with a HTTP 400 if there is an error in the access token response. The details of the error are conveyed in the error, error_description, and error_uri fields in the response.

GitHub OAuth does not follow this spec exactly and can return an error response (examples) with HTTP code 200. In this case, the error from GitHub is swallowed and instead we return a generic error. This makes it difficult to debug exactly what went wrong when GitHub returns an access token response error.

Reproduction steps:

  • Configure GitHub as OAuth2 authorization provider for a web service that uses this package.
  • Change the GitHub client secret so that it no longer matches.
  • Attempt login via GitHub.
  • Note that the error returned by this package is the generic oauth2: server response missing access_token, rather than the actual error returned by GitHub: incorrect_client_credentials.

Proposed fix: Parse the error field in the access token response. If it is non-empty, then treat the response as an error and return an error containing the error, error_description, and error_uri fields in the access token response. An implementation of this fix is here: sourcegraph@6057702.

bobheadxi added a commit to sourcegraph/sourcegraph-public-snapshot that referenced this issue May 9, 2023
The issue this fork was meant to address
(golang/oauth2#441) has been fixed upstream:

- [proposed fix that was part of
fork](golang/oauth2#442)
- [upstream
fix](golang/oauth2@cfe200d)
(golang/go#58125)

Notably, both implementations change the error returned to render more
verbose details on `.Error()` if the response is a status 200 _but_ an
error (see above issues)

A diff against upstream indicates that there are no other changes:
sourcegraph/oauth2#3

## Test plan

Test suites pass
VolhaBakanouskaya pushed a commit to VolhaBakanouskaya/sourcegraph-public that referenced this issue Jun 30, 2023
The issue this fork was meant to address
(golang/oauth2#441) has been fixed upstream:

- [proposed fix that was part of
fork](golang/oauth2#442)
- [upstream
fix](golang/oauth2@cfe200d)
(golang/go#58125)

Notably, both implementations change the error returned to render more
verbose details on `.Error()` if the response is a status 200 _but_ an
error (see above issues)

A diff against upstream indicates that there are no other changes:
sourcegraph/oauth2#3

## Test plan

Test suites pass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant