diff --git a/oauthproxy.go b/oauthproxy.go index ca921efb..e74dcabc 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -128,6 +128,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr ProxyPrefix: opts.ProxyPrefix, Footer: opts.Templates.Footer, Version: VERSION, + Debug: opts.Templates.Debug, } upstreamProxy, err := upstream.NewProxy(opts.UpstreamServers, opts.GetSignatureData(), errorPage.ProxyErrorHandler) @@ -523,7 +524,7 @@ func (p *OAuthProxy) RobotsTxt(rw http.ResponseWriter, req *http.Request) { } // ErrorPage writes an error response -func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code int, appError string) { +func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code int, appError string, messages ...interface{}) { redirectURL, err := p.getAppRedirect(req) if err != nil { logger.Errorf("Error obtaining redirect: %v", err) @@ -532,7 +533,7 @@ func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code i redirectURL = "/" } - p.errorPage.Render(rw, code, redirectURL, appError) + p.errorPage.Render(rw, code, redirectURL, appError, messages...) } // IsAllowedRequest is used to check if auth should be skipped for this request @@ -751,28 +752,30 @@ 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) - p.ErrorPage(rw, req, http.StatusForbidden, errorString) + 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) return } session, err := p.redeemCode(req) if err != nil { logger.Errorf("Error redeeming code during OAuth2 callback: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error") + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } err = p.enrichSessionState(req.Context(), session) if err != nil { logger.Errorf("Error creating session during OAuth2 callback: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error") + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } state := strings.SplitN(req.Form.Get("state"), ":", 2) if len(state) != 2 { logger.Error("Error while parsing OAuth2 state: invalid length") - p.ErrorPage(rw, req, http.StatusInternalServerError, "Invalid State") + p.ErrorPage(rw, req, http.StatusInternalServerError, "State paremeter did not have expected length", "Login Failed: Invalid State after login.") return } nonce := state[0] @@ -780,13 +783,13 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { c, err := req.Cookie(p.CSRFCookieName) if err != nil { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unable to obtain CSRF cookie") - p.ErrorPage(rw, req, http.StatusForbidden, err.Error()) + p.ErrorPage(rw, req, http.StatusForbidden, err.Error(), "Login Failed: Unable to find a valid CSRF token. Please try again.") return } p.ClearCSRFCookie(rw, req) if c.Value != nonce { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: CSRF token mismatch, potential attack") - p.ErrorPage(rw, req, http.StatusForbidden, "CSRF Failed") + p.ErrorPage(rw, req, http.StatusForbidden, "CSRF token mismatch, potential attack", "Login Failed: Unable to find a valid CSRF token. Please try again.") return } @@ -810,7 +813,7 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { http.Redirect(rw, req, redirect, http.StatusFound) } else { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unauthorized") - p.ErrorPage(rw, req, http.StatusForbidden, "Invalid Account") + p.ErrorPage(rw, req, http.StatusForbidden, "Invalid session: unauthorized") } } @@ -892,12 +895,12 @@ func (p *OAuthProxy) Proxy(rw http.ResponseWriter, req *http.Request) { } case ErrAccessDenied: - p.ErrorPage(rw, req, http.StatusUnauthorized, "Unauthorized") + p.ErrorPage(rw, req, http.StatusForbidden, "The session failed authorization checks") default: // unknown error logger.Errorf("Unexpected internal error: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error") + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) } } diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 06b77ca8..06014c9b 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -2815,7 +2815,7 @@ func TestProxyAllowedGroups(t *testing.T) { test.proxy.ServeHTTP(test.rw, test.req) if tt.expectUnauthorized { - assert.Equal(t, http.StatusUnauthorized, test.rw.Code) + assert.Equal(t, http.StatusForbidden, test.rw.Code) } else { assert.Equal(t, http.StatusOK, test.rw.Code) } diff --git a/pkg/apis/options/app.go b/pkg/apis/options/app.go index 1574ac97..76c4f84a 100644 --- a/pkg/apis/options/app.go +++ b/pkg/apis/options/app.go @@ -22,6 +22,12 @@ type Templates struct { // password form if a static passwords file (htpasswd file) has been // configured. DisplayLoginForm bool `flag:"display-htpasswd-form" cfg:"display_htpasswd_form"` + + // Debug renders detailed errors when an error page is shown. + // It is not advised to use this in production as errors may contain sensitive + // information. + // Use only for diagnosing backend errors. + Debug bool `flag:"show-debug-on-error" cfg:"show-debug-on-error"` } func templatesFlagSet() *pflag.FlagSet { @@ -31,6 +37,7 @@ func templatesFlagSet() *pflag.FlagSet { flagSet.String("banner", "", "custom banner string. Use \"-\" to disable default banner.") flagSet.String("footer", "", "custom footer string. Use \"-\" to disable default footer.") flagSet.Bool("display-htpasswd-form", true, "display username / password login form if an htpasswd file is provided") + flagSet.Bool("show-debug-on-error", false, "show detailed error information on error pages (WARNING: this may contain sensitive information - do not use in production)") return flagSet } diff --git a/pkg/app/error_page.go b/pkg/app/error_page.go index d26fed63..56d1c6af 100644 --- a/pkg/app/error_page.go +++ b/pkg/app/error_page.go @@ -1,12 +1,22 @@ package app import ( + "fmt" "html/template" "net/http" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" ) +// errorMessages are default error messages for each of the the different +// http status codes expected to be rendered in the error page. +var errorMessages = map[int]string{ + http.StatusInternalServerError: "Oops! Something went wrong. For more information contact your server administrator.", + http.StatusNotFound: "We could not find the resource you were looking for.", + http.StatusForbidden: "You do not have permission to access this resource.", + http.StatusUnauthorized: "You need to be logged in to access this resource.", +} + // ErrorPage is used to render error pages. type ErrorPage struct { // Template is the error page HTML template. @@ -21,12 +31,16 @@ type ErrorPage struct { // Version is the OAuth2 Proxy version to be used in the default footer. Version string + + // Debug determines whether errors pages should be rendered with detailed + // errors. + Debug bool } // Render writes an error page to the given response writer. // It uses the passed redirectURL to give users the option to go back to where // they originally came from or try signing in again. -func (e *ErrorPage) Render(rw http.ResponseWriter, status int, redirectURL string, appError string) { +func (e *ErrorPage) Render(rw http.ResponseWriter, status int, redirectURL string, appError string, messages ...interface{}) { rw.WriteHeader(status) // We allow unescaped template.HTML since it is user configured options @@ -41,7 +55,7 @@ func (e *ErrorPage) Render(rw http.ResponseWriter, status int, redirectURL strin Version string }{ Title: http.StatusText(status), - Message: appError, + Message: e.getMessage(status, appError, messages...), ProxyPrefix: e.ProxyPrefix, StatusCode: status, Redirect: redirectURL, @@ -60,5 +74,24 @@ func (e *ErrorPage) Render(rw http.ResponseWriter, status int, redirectURL strin // It is expected to always render a bad gateway error. func (e *ErrorPage) ProxyErrorHandler(rw http.ResponseWriter, req *http.Request, proxyErr error) { logger.Errorf("Error proxying to upstream server: %v", proxyErr) - e.Render(rw, http.StatusBadGateway, "", "Error proxying to upstream server") + e.Render(rw, http.StatusBadGateway, "", proxyErr.Error(), "There was a problem connecting to the upstream server.") +} + +// getMessage creates the message for the template parameters. +// If the ErrorPage.Debug is enabled, the application error takes precedence. +// Otherwise, any messages will be used. +// The first message is expected to be a format string. +// If no messages are supplied, a default error message will be used. +func (e *ErrorPage) getMessage(status int, appError string, messages ...interface{}) string { + if e.Debug { + return appError + } + if len(messages) > 0 { + format := fmt.Sprintf("%v", messages[0]) + return fmt.Sprintf(format, messages[1:]...) + } + if msg, ok := errorMessages[status]; ok { + return msg + } + return "Unknown error" } diff --git a/pkg/app/error_page_test.go b/pkg/app/error_page_test.go index f2265431..5c4f78fa 100644 --- a/pkg/app/error_page_test.go +++ b/pkg/app/error_page_test.go @@ -32,7 +32,25 @@ var _ = Describe("Error Page", func() { body, err := ioutil.ReadAll(recorder.Result().Body) Expect(err).ToNot(HaveOccurred()) - Expect(string(body)).To(Equal("Forbidden Access Denied /prefix/ 403 /redirect Custom Footer Text v0.0.0-test")) + Expect(string(body)).To(Equal("Forbidden You do not have permission to access this resource. /prefix/ 403 /redirect Custom Footer Text v0.0.0-test")) + }) + + It("With a different code, uses the stock message for the correct code", func() { + recorder := httptest.NewRecorder() + errorPage.Render(recorder, 500, "/redirect", "Access Denied") + + body, err := ioutil.ReadAll(recorder.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal("Internal Server Error Oops! Something went wrong. For more information contact your server administrator. /prefix/ 500 /redirect Custom Footer Text v0.0.0-test")) + }) + + It("With a message override, uses the message", func() { + recorder := httptest.NewRecorder() + errorPage.Render(recorder, 403, "/redirect", "Access Denied", "An extra message: %s", "with more context.") + + body, err := ioutil.ReadAll(recorder.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal("Forbidden An extra message: with more context. /prefix/ 403 /redirect Custom Footer Text v0.0.0-test")) }) }) @@ -44,7 +62,40 @@ var _ = Describe("Error Page", func() { body, err := ioutil.ReadAll(recorder.Result().Body) Expect(err).ToNot(HaveOccurred()) - Expect(string(body)).To(Equal("Bad Gateway Error proxying to upstream server /prefix/ 502 Custom Footer Text v0.0.0-test")) + Expect(string(body)).To(Equal("Bad Gateway There was a problem connecting to the upstream server. /prefix/ 502 Custom Footer Text v0.0.0-test")) + }) + }) + + Context("With Debug enabled", func() { + BeforeEach(func() { + tmpl, err := template.New("").Parse("{{.Message}}") + Expect(err).ToNot(HaveOccurred()) + + errorPage.Template = tmpl + errorPage.Debug = true + }) + + Context("Render", func() { + It("Writes the detailed error in place of the message", func() { + recorder := httptest.NewRecorder() + errorPage.Render(recorder, 403, "/redirect", "Debug error") + + body, err := ioutil.ReadAll(recorder.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal("Debug error")) + }) + }) + + Context("ProxyErrorHandler", func() { + It("Writes a bad gateway error the response writer", func() { + req := httptest.NewRequest("", "/bad-gateway", nil) + recorder := httptest.NewRecorder() + errorPage.ProxyErrorHandler(recorder, req, errors.New("some upstream error")) + + body, err := ioutil.ReadAll(recorder.Result().Body) + Expect(err).ToNot(HaveOccurred()) + Expect(string(body)).To(Equal("some upstream error")) + }) }) }) })