diff --git a/CHANGELOG.md b/CHANGELOG.md index 788e82c2..cb77de0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ ## Changes since v7.15.3 +- [#3459](https://github.com/oauth2-proxy/oauth2-proxy/pull/3459) fix: include the provider's error_description on the OAuth2 callback error page (#3458) (@Molaire) + # V7.15.3 ## Release Highlights diff --git a/oauthproxy.go b/oauthproxy.go index f8dc5471..87ea795b 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -896,10 +896,21 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { } errorString := req.Form.Get("error") if errorString != "" { - logger.Errorf("Error while parsing OAuth2 callback: %s", errorString) + // Many providers (e.g. Okta, Entra ID, Keycloak) return a human-readable + // error_description alongside the error code. Surfacing it lets end users + // distinguish causes of an otherwise opaque code such as access_denied + // (for example "User is not assigned to the application" versus a policy + // denial) without needing access to the proxy logs. + errorDescription := req.Form.Get("error_description") + logger.Errorf("Error while parsing OAuth2 callback: %s %s", errorString, errorDescription) message := fmt.Sprintf("Login Failed: The upstream identity provider returned an error: %s", errorString) - // Set the debug message and override the non debug message to be the same for this case - p.ErrorPage(rw, req, http.StatusForbidden, message, message) + if errorDescription != "" { + message = fmt.Sprintf("%s: %s", message, errorDescription) + } + // Set the debug message and override the non debug message to be the same + // for this case. The message is passed as an argument rather than a format + // string so a provider-supplied description containing '%' is rendered verbatim. + p.ErrorPage(rw, req, http.StatusForbidden, message, "%s", message) return } diff --git a/oauthproxy_test.go b/oauthproxy_test.go index b3271e5b..6939fb31 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -843,6 +843,33 @@ func TestSignInPageSkipProviderDirect(t *testing.T) { } } +func TestOAuthCallbackProviderErrorIncludesDescription(t *testing.T) { + sipTest, err := NewSignInPageTest(false) + if err != nil { + t.Fatal(err) + } + + // Okta (and other OIDC providers) return error_description alongside the + // error code when, for example, the user is not assigned the application. + code, body := sipTest.GetEndpoint( + "/oauth2/callback?error=access_denied&error_description=User+is+not+assigned+to+the+client+application.", + ) + assert.Equal(t, http.StatusForbidden, code) + assert.Contains(t, body, "access_denied") + assert.Contains(t, body, "User is not assigned to the client application.") +} + +func TestOAuthCallbackProviderErrorWithoutDescription(t *testing.T) { + sipTest, err := NewSignInPageTest(false) + if err != nil { + t.Fatal(err) + } + + code, body := sipTest.GetEndpoint("/oauth2/callback?error=access_denied") + assert.Equal(t, http.StatusForbidden, code) + assert.Contains(t, body, "The upstream identity provider returned an error: access_denied") +} + type ProcessCookieTest struct { opts *options.Options proxy *OAuthProxy