Merge pull request #1029 from oauth2-proxy/error-pages

Refactor error page rendering and allow debug messages on error
oauth2-proxy · Feb 13, 2021 · 1e3d854 · 1e3d854
2 parents 9cdcd2b + 9e8c2af
commit 1e3d854
Show file tree

Hide file tree

Showing 10 changed files with 254 additions and 80 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,7 @@
 
 ## Changes since v7.0.1
 
+- [#1029](https://github.com/oauth2-proxy/oauth2-proxy/pull/1029) Refactor error page rendering and allow debug messages on error (@JoelSpeed)
 - [#1028](https://github.com/oauth2-proxy/oauth2-proxy/pull/1028) Refactor templates, update theme and provide styled error pages (@JoelSpeed)
 - [#1039](https://github.com/oauth2-proxy/oauth2-proxy/pull/1039) Ensure errors in tests are logged to the GinkgoWriter (@JoelSpeed)
 

diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md
@@ -115,6 +115,7 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/
 | `--set-xauthrequest` | bool | set X-Auth-Request-User, X-Auth-Request-Groups, X-Auth-Request-Email and X-Auth-Request-Preferred-Username response headers (useful in Nginx auth_request mode). When used with `--pass-access-token`, X-Auth-Request-Access-Token is added to response headers.  | false |
 | `--set-authorization-header` | bool | set Authorization Bearer response header (useful in Nginx auth_request mode) | false |
 | `--set-basic-auth` | bool | set HTTP Basic Auth information in response (useful in Nginx auth_request mode) | false |
+| `--show-debug-on-error` | bool | show detailed error information on error pages (WARNING: this may contain sensitive information - do not use in production) | false |
 | `--signature-key` | string | GAP-Signature request signature key (algorithm:secretkey) | |
 | `--silence-ping-logging` | bool | disable logging of requests to ping endpoint | false |
 | `--skip-auth-preflight` | bool | will skip authentication for OPTIONS requests | false |

diff --git a/oauthproxy.go b/oauthproxy.go
@@ -108,6 +108,7 @@ type OAuthProxy struct {
 	sessionChain alice.Chain
 	headersChain alice.Chain
 	preAuthChain alice.Chain
+	errorPage    *app.ErrorPage
 }
 
 // NewOAuthProxy creates a new instance of OAuthProxy from the options provided
@@ -121,8 +122,16 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr
 	if err != nil {
 		return nil, fmt.Errorf("error loading templates: %v", err)
 	}
-	proxyErrorHandler := upstream.NewProxyErrorHandler(templates.Lookup("error.html"), opts.ProxyPrefix)
-	upstreamProxy, err := upstream.NewProxy(opts.UpstreamServers, opts.GetSignatureData(), proxyErrorHandler)
+
+	errorPage := &app.ErrorPage{
+		Template:    templates.Lookup("error.html"),
+		ProxyPrefix: opts.ProxyPrefix,
+		Footer:      opts.Templates.Footer,
+		Version:     VERSION,
+		Debug:       opts.Templates.Debug,
+	}
+
+	upstreamProxy, err := upstream.NewProxy(opts.UpstreamServers, opts.GetSignatureData(), errorPage.ProxyErrorHandler)
 	if err != nil {
 		return nil, fmt.Errorf("error initialising upstream proxy: %v", err)
 	}
@@ -224,6 +233,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr
 		sessionChain:        sessionChain,
 		headersChain:        headersChain,
 		preAuthChain:        preAuthChain,
+		errorPage:           errorPage,
 	}, nil
 }
 
@@ -507,14 +517,14 @@ func (p *OAuthProxy) RobotsTxt(rw http.ResponseWriter, req *http.Request) {
 	_, err := fmt.Fprintf(rw, "User-agent: *\nDisallow: /")
 	if err != nil {
 		logger.Printf("Error writing robots.txt: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error())
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 		return
 	}
 	rw.WriteHeader(http.StatusOK)
 }
 
 // ErrorPage writes an error response
-func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code int, title string, message 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)
@@ -523,32 +533,7 @@ func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code i
 		redirectURL = "/"
 	}
 
-	rw.WriteHeader(code)
-
-	// We allow unescaped template.HTML since it is user configured options
-	/* #nosec G203 */
-	t := struct {
-		Title       string
-		Message     string
-		ProxyPrefix string
-		StatusCode  int
-		Redirect    string
-		Footer      template.HTML
-		Version     string
-	}{
-		Title:       title,
-		Message:     message,
-		ProxyPrefix: p.ProxyPrefix,
-		StatusCode:  code,
-		Redirect:    redirectURL,
-		Footer:      template.HTML(p.Footer),
-		Version:     VERSION,
-	}
-
-	if err := p.templates.ExecuteTemplate(rw, "error.html", t); err != nil {
-		logger.Printf("Error rendering error.html template: %v", err)
-		http.Error(rw, "Internal Server Error", http.StatusInternalServerError)
-	}
+	p.errorPage.Render(rw, code, redirectURL, appError, messages...)
 }
 
 // IsAllowedRequest is used to check if auth should be skipped for this request
@@ -593,15 +578,15 @@ func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code
 	err := p.ClearSessionCookie(rw, req)
 	if err != nil {
 		logger.Printf("Error clearing session cookie: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error())
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 		return
 	}
 	rw.WriteHeader(code)
 
 	redirectURL, err := p.getAppRedirect(req)
 	if err != nil {
 		logger.Errorf("Error obtaining redirect: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error())
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 		return
 	}
 
@@ -634,7 +619,7 @@ func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code
 	err = p.templates.ExecuteTemplate(rw, "sign_in.html", t)
 	if err != nil {
 		logger.Printf("Error rendering sign_in.html template: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error())
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 	}
 }
 
@@ -662,7 +647,7 @@ func (p *OAuthProxy) SignIn(rw http.ResponseWriter, req *http.Request) {
 	redirect, err := p.getAppRedirect(req)
 	if err != nil {
 		logger.Errorf("Error obtaining redirect: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error())
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 		return
 	}
 
@@ -672,7 +657,7 @@ func (p *OAuthProxy) SignIn(rw http.ResponseWriter, req *http.Request) {
 		err = p.SaveSession(rw, req, session)
 		if err != nil {
 			logger.Printf("Error saving session: %v", err)
-			p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error())
+			p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 			return
 		}
 		http.Redirect(rw, req, redirect, http.StatusFound)
@@ -711,7 +696,7 @@ func (p *OAuthProxy) UserInfo(rw http.ResponseWriter, req *http.Request) {
 	err = json.NewEncoder(rw).Encode(userInfo)
 	if err != nil {
 		logger.Printf("Error encoding user info: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error())
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 	}
 }
 
@@ -720,13 +705,13 @@ func (p *OAuthProxy) SignOut(rw http.ResponseWriter, req *http.Request) {
 	redirect, err := p.getAppRedirect(req)
 	if err != nil {
 		logger.Errorf("Error obtaining redirect: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error())
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 		return
 	}
 	err = p.ClearSessionCookie(rw, req)
 	if err != nil {
 		logger.Errorf("Error clearing session cookie: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error())
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 		return
 	}
 	http.Redirect(rw, req, redirect, http.StatusFound)
@@ -738,14 +723,14 @@ func (p *OAuthProxy) OAuthStart(rw http.ResponseWriter, req *http.Request) {
 	nonce, err := encryption.Nonce()
 	if err != nil {
 		logger.Errorf("Error obtaining nonce: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error())
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 		return
 	}
 	p.SetCSRFCookie(rw, req, nonce)
 	redirect, err := p.getAppRedirect(req)
 	if err != nil {
 		logger.Errorf("Error obtaining redirect: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error())
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 		return
 	}
 	redirectURI := p.getOAuthRedirectURI(req)
@@ -761,48 +746,50 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
 	err := req.ParseForm()
 	if err != nil {
 		logger.Errorf("Error while parsing OAuth2 callback: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error())
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 		return
 	}
 	errorString := req.Form.Get("error")
 	if errorString != "" {
 		logger.Errorf("Error while parsing OAuth2 callback: %s", errorString)
-		p.ErrorPage(rw, req, http.StatusForbidden, "Permission Denied", 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 Server Error", "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 Server Error", "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, "Internal Server Error", "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]
 	redirect := state[1]
 	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, "Permission Denied", 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, "Permission Denied", "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
 	}
 
@@ -820,13 +807,13 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
 		err := p.SaveSession(rw, req, session)
 		if err != nil {
 			logger.Errorf("Error saving session state for %s: %v", remoteAddr, err)
-			p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Server Error", err.Error())
+			p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 			return
 		}
 		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, "Permission Denied", "Invalid Account")
+		p.ErrorPage(rw, req, http.StatusForbidden, "Invalid session: unauthorized")
 	}
 }
 
@@ -908,13 +895,12 @@ func (p *OAuthProxy) Proxy(rw http.ResponseWriter, req *http.Request) {
 		}
 
 	case ErrAccessDenied:
-		p.ErrorPage(rw, req, http.StatusUnauthorized, "Permission Denied", "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", "Internal Error")
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 	}
 }
 

diff --git 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
@@ -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
 }