From 764e75d9b9265371a7bd3eb4b9798f20d9f279ed Mon Sep 17 00:00:00 2001 From: SteveTheEngineer Date: Fri, 24 Jun 2022 05:05:23 +0300 Subject: [PATCH] Catch the error before the response is processed by goth. (#20000) (#20102) The code introduced by #18185 gets the error from response after it was processed by goth. That is incorrect, as goth (and golang.org/x/oauth) doesn't really care about the error, and it sends a token request with an empty authorization code to the server anyway, which always results in a `oauth2: cannot fetch token: 400 Bad Request` error from goth. It means that unless the "state" parameter is omitted from the error response (which is required to be present, according to [RFC 6749, Section 4.1.2.1](https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1)) or the page is reloaded (makes the session invalid), a 500 Internal Server Error page will be displayed. This fixes it by handling the error before the request is passed to goth. --- routers/web/auth/oauth.go | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index d868b05a4..56f8294b1 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -37,6 +37,7 @@ import ( "gitea.com/go-chi/binding" "github.com/golang-jwt/jwt/v4" "github.com/markbates/goth" + "github.com/markbates/goth/gothic" ) const ( @@ -1098,24 +1099,31 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model func oAuth2UserLoginCallback(authSource *auth.Source, request *http.Request, response http.ResponseWriter) (*user_model.User, goth.User, error) { oauth2Source := authSource.Cfg.(*oauth2.Source) + // Make sure that the response is not an error response. + errorName := request.FormValue("error") + + if len(errorName) > 0 { + errorDescription := request.FormValue("error_description") + + // Delete the goth session + err := gothic.Logout(response, request) + if err != nil { + return nil, goth.User{}, err + } + + return nil, goth.User{}, errCallback{ + Code: errorName, + Description: errorDescription, + } + } + + // Proceed to authenticate through goth. gothUser, err := oauth2Source.Callback(request, response) if err != nil { if err.Error() == "securecookie: the value is too long" || strings.Contains(err.Error(), "Data too long") { log.Error("OAuth2 Provider %s returned too long a token. Current max: %d. Either increase the [OAuth2] MAX_TOKEN_LENGTH or reduce the information returned from the OAuth2 provider", authSource.Name, setting.OAuth2.MaxTokenLength) err = fmt.Errorf("OAuth2 Provider %s returned too long a token. Current max: %d. Either increase the [OAuth2] MAX_TOKEN_LENGTH or reduce the information returned from the OAuth2 provider", authSource.Name, setting.OAuth2.MaxTokenLength) } - // goth does not provide the original error message - // https://github.com/markbates/goth/issues/348 - if strings.Contains(err.Error(), "server response missing access_token") || strings.Contains(err.Error(), "could not find a matching session for this request") { - errorCode := request.FormValue("error") - errorDescription := request.FormValue("error_description") - if errorCode != "" || errorDescription != "" { - return nil, goth.User{}, errCallback{ - Code: errorCode, - Description: errorDescription, - } - } - } return nil, goth.User{}, err }