6ecbc7b · typeable/oauth2-proxy

git.iko.soy/typeable/oauth2-proxy

Allow users to choose detailed error messages on error pages

commit6ecbc7bc4e1aba4a7db5157433d0f318366c880f authorJoel Speed <joel.speed@hotmail.co.uk> dateWed 10 Feb 2021, 19:34 (6 years ago) parenta63ed02

5 files changed+111−17

compare with master

unifiedsplit

oauthproxy.go+14−11 oauthproxy_test.go+1−1 pkg/apis/options/app.go+7 pkg/app/error_page.go+36−3 pkg/app/error_page_test.go+53−2

oauthproxy.go+14−11

@@ -128,6 +128,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr

128128 ProxyPrefix: opts.ProxyPrefix,

129129 Footer: opts.Templates.Footer,

130130 Version: VERSION,

131+ Debug: opts.Templates.Debug,

131132 }

132133

133134 upstreamProxy, err := upstream.NewProxy(opts.UpstreamServers, opts.GetSignatureData(), errorPage.ProxyErrorHandler)

@@ -523,7 +524,7 @@ func (p *OAuthProxy) RobotsTxt(rw http.ResponseWriter, req *http.Request) {

523524}

524525

525526// ErrorPage writes an error response

526−func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code int, appError string) {

527+func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code int, appError string, messages ...interface{}) {

527528 redirectURL, err := p.getAppRedirect(req)

528529 if err != nil {

529530 logger.Errorf("Error obtaining redirect: %v", err)

@@ -532,7 +533,7 @@ func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code i

532533 redirectURL = "/"

533534 }

534535

535− p.errorPage.Render(rw, code, redirectURL, appError)

536+ p.errorPage.Render(rw, code, redirectURL, appError, messages...)

536537}

537538

538539// 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) {

751752 errorString := req.Form.Get("error")

752753 if errorString != "" {

753754 logger.Errorf("Error while parsing OAuth2 callback: %s", errorString)

754− p.ErrorPage(rw, req, http.StatusForbidden, errorString)

755+ message := fmt.Sprintf("Login Failed: The upstream identity provider returned an error: %s", errorString)

756+ // Set the debug message and override the non debug message to be the same for this case

757+ p.ErrorPage(rw, req, http.StatusForbidden, message, message)

755758 return

756759 }

757760

758761 session, err := p.redeemCode(req)

759762 if err != nil {

760763 logger.Errorf("Error redeeming code during OAuth2 callback: %v", err)

761− p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error")

764+ p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())

762765 return

763766 }

764767

765768 err = p.enrichSessionState(req.Context(), session)

766769 if err != nil {

767770 logger.Errorf("Error creating session during OAuth2 callback: %v", err)

768− p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error")

771+ p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())

769772 return

770773 }

771774

772775 state := strings.SplitN(req.Form.Get("state"), ":", 2)

773776 if len(state) != 2 {

774777 logger.Error("Error while parsing OAuth2 state: invalid length")

775− p.ErrorPage(rw, req, http.StatusInternalServerError, "Invalid State")

778+ p.ErrorPage(rw, req, http.StatusInternalServerError, "State paremeter did not have expected length", "Login Failed: Invalid State after login.")

776779 return

777780 }

778781 nonce := state[0]

@@ -780,13 +783,13 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {

780783 c, err := req.Cookie(p.CSRFCookieName)

781784 if err != nil {

782785 logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unable to obtain CSRF cookie")

783− p.ErrorPage(rw, req, http.StatusForbidden, err.Error())

786+ p.ErrorPage(rw, req, http.StatusForbidden, err.Error(), "Login Failed: Unable to find a valid CSRF token. Please try again.")

784787 return

785788 }

786789 p.ClearCSRFCookie(rw, req)

787790 if c.Value != nonce {

788791 logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: CSRF token mismatch, potential attack")

789− p.ErrorPage(rw, req, http.StatusForbidden, "CSRF Failed")

792+ p.ErrorPage(rw, req, http.StatusForbidden, "CSRF token mismatch, potential attack", "Login Failed: Unable to find a valid CSRF token. Please try again.")

790793 return

791794 }

792795

@@ -810,7 +813,7 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {

810813 http.Redirect(rw, req, redirect, http.StatusFound)

811814 } else {

812815 logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unauthorized")

813− p.ErrorPage(rw, req, http.StatusForbidden, "Invalid Account")

816+ p.ErrorPage(rw, req, http.StatusForbidden, "Invalid session: unauthorized")

814817 }

815818}

816819

@@ -892,12 +895,12 @@ func (p *OAuthProxy) Proxy(rw http.ResponseWriter, req *http.Request) {

892895 }

893896

894897 case ErrAccessDenied:

895− p.ErrorPage(rw, req, http.StatusUnauthorized, "Unauthorized")

898+ p.ErrorPage(rw, req, http.StatusForbidden, "The session failed authorization checks")

896899

897900 default:

898901 // unknown error

899902 logger.Errorf("Unexpected internal error: %v", err)

900− p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error")

903+ p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())

901904 }

902905}

903906

oauthproxy_test.go+1−1

@@ -2815,7 +2815,7 @@ func TestProxyAllowedGroups(t *testing.T) {

28152815 test.proxy.ServeHTTP(test.rw, test.req)

28162816

28172817 if tt.expectUnauthorized {

2818− assert.Equal(t, http.StatusUnauthorized, test.rw.Code)

2818+ assert.Equal(t, http.StatusForbidden, test.rw.Code)

28192819 } else {

28202820 assert.Equal(t, http.StatusOK, test.rw.Code)

28212821 }

pkg/apis/options/app.go+7

@@ -22,6 +22,12 @@ type Templates struct {

2222 // password form if a static passwords file (htpasswd file) has been

2323 // configured.

2424 DisplayLoginForm bool `flag:"display-htpasswd-form" cfg:"display_htpasswd_form"`

25+

26+ // Debug renders detailed errors when an error page is shown.

27+ // It is not advised to use this in production as errors may contain sensitive

28+ // information.

29+ // Use only for diagnosing backend errors.

30+ Debug bool `flag:"show-debug-on-error" cfg:"show-debug-on-error"`

2531}

2632

2733func templatesFlagSet() *pflag.FlagSet {

@@ -31,6 +37,7 @@ func templatesFlagSet() *pflag.FlagSet {

3137 flagSet.String("banner", "", "custom banner string. Use \"-\" to disable default banner.")

3238 flagSet.String("footer", "", "custom footer string. Use \"-\" to disable default footer.")

3339 flagSet.Bool("display-htpasswd-form", true, "display username / password login form if an htpasswd file is provided")

40+ 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)")

3441

3542 return flagSet

3643}

pkg/app/error_page.go+36−3

@@ -1,12 +1,22 @@

11package app

33import (

4+ "fmt"

45 "html/template"

56 "net/http"

78 "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger"

89)

910

11+// errorMessages are default error messages for each of the the different

12+// http status codes expected to be rendered in the error page.

13+var errorMessages = map[int]string{

14+ http.StatusInternalServerError: "Oops! Something went wrong. For more information contact your server administrator.",

15+ http.StatusNotFound: "We could not find the resource you were looking for.",

16+ http.StatusForbidden: "You do not have permission to access this resource.",

17+ http.StatusUnauthorized: "You need to be logged in to access this resource.",

18+}

19+

1020// ErrorPage is used to render error pages.

1121type ErrorPage struct {

1222 // Template is the error page HTML template.

@@ -21,12 +31,16 @@ type ErrorPage struct {

2131

2232 // Version is the OAuth2 Proxy version to be used in the default footer.

2333 Version string

34+

35+ // Debug determines whether errors pages should be rendered with detailed

36+ // errors.

37+ Debug bool

2438}

2539

2640// Render writes an error page to the given response writer.

2741// It uses the passed redirectURL to give users the option to go back to where

2842// they originally came from or try signing in again.

29−func (e *ErrorPage) Render(rw http.ResponseWriter, status int, redirectURL string, appError string) {

43+func (e *ErrorPage) Render(rw http.ResponseWriter, status int, redirectURL string, appError string, messages ...interface{}) {

3044 rw.WriteHeader(status)

3145

3246 // 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

4155 Version string

4256 }{

4357 Title: http.StatusText(status),

44− Message: appError,

58+ Message: e.getMessage(status, appError, messages...),

4559 ProxyPrefix: e.ProxyPrefix,

4660 StatusCode: status,

4761 Redirect: redirectURL,

@@ -60,5 +74,24 @@ func (e *ErrorPage) Render(rw http.ResponseWriter, status int, redirectURL strin

6074// It is expected to always render a bad gateway error.

6175func (e *ErrorPage) ProxyErrorHandler(rw http.ResponseWriter, req *http.Request, proxyErr error) {

6276 logger.Errorf("Error proxying to upstream server: %v", proxyErr)

63− e.Render(rw, http.StatusBadGateway, "", "Error proxying to upstream server")

77+ e.Render(rw, http.StatusBadGateway, "", proxyErr.Error(), "There was a problem connecting to the upstream server.")

78+}

79+

80+// getMessage creates the message for the template parameters.

81+// If the ErrorPage.Debug is enabled, the application error takes precedence.

82+// Otherwise, any messages will be used.

83+// The first message is expected to be a format string.

84+// If no messages are supplied, a default error message will be used.

85+func (e *ErrorPage) getMessage(status int, appError string, messages ...interface{}) string {

86+ if e.Debug {

87+ return appError

88+ }

89+ if len(messages) > 0 {

90+ format := fmt.Sprintf("%v", messages[0])

91+ return fmt.Sprintf(format, messages[1:]...)

92+ }

93+ if msg, ok := errorMessages[status]; ok {

94+ return msg

95+ }

96+ return "Unknown error"

6497}

pkg/app/error_page_test.go+53−2

@@ -32,7 +32,25 @@ var _ = Describe("Error Page", func() {

3232

3333 body, err := ioutil.ReadAll(recorder.Result().Body)

3434 Expect(err).ToNot(HaveOccurred())

35− Expect(string(body)).To(Equal("Forbidden Access Denied /prefix/ 403 /redirect Custom Footer Text v0.0.0-test"))

35+ 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"))

36+ })

37+

38+ It("With a different code, uses the stock message for the correct code", func() {

39+ recorder := httptest.NewRecorder()

40+ errorPage.Render(recorder, 500, "/redirect", "Access Denied")

41+

42+ body, err := ioutil.ReadAll(recorder.Result().Body)

43+ Expect(err).ToNot(HaveOccurred())

44+ 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"))

45+ })

46+

47+ It("With a message override, uses the message", func() {

48+ recorder := httptest.NewRecorder()

49+ errorPage.Render(recorder, 403, "/redirect", "Access Denied", "An extra message: %s", "with more context.")

50+

51+ body, err := ioutil.ReadAll(recorder.Result().Body)

52+ Expect(err).ToNot(HaveOccurred())

53+ Expect(string(body)).To(Equal("Forbidden An extra message: with more context. /prefix/ 403 /redirect Custom Footer Text v0.0.0-test"))

3654 })

3755 })

3856

@@ -44,7 +62,40 @@ var _ = Describe("Error Page", func() {

4462

4563 body, err := ioutil.ReadAll(recorder.Result().Body)

4664 Expect(err).ToNot(HaveOccurred())

47− Expect(string(body)).To(Equal("Bad Gateway Error proxying to upstream server /prefix/ 502 Custom Footer Text v0.0.0-test"))

65+ 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"))

66+ })

67+ })

68+

69+ Context("With Debug enabled", func() {

70+ BeforeEach(func() {

71+ tmpl, err := template.New("").Parse("{{.Message}}")

72+ Expect(err).ToNot(HaveOccurred())

73+

74+ errorPage.Template = tmpl

75+ errorPage.Debug = true

76+ })

77+

78+ Context("Render", func() {

79+ It("Writes the detailed error in place of the message", func() {

80+ recorder := httptest.NewRecorder()

81+ errorPage.Render(recorder, 403, "/redirect", "Debug error")

82+

83+ body, err := ioutil.ReadAll(recorder.Result().Body)

84+ Expect(err).ToNot(HaveOccurred())

85+ Expect(string(body)).To(Equal("Debug error"))

86+ })

87+ })

88+

89+ Context("ProxyErrorHandler", func() {

90+ It("Writes a bad gateway error the response writer", func() {

91+ req := httptest.NewRequest("", "/bad-gateway", nil)

92+ recorder := httptest.NewRecorder()

93+ errorPage.ProxyErrorHandler(recorder, req, errors.New("some upstream error"))

94+

95+ body, err := ioutil.ReadAll(recorder.Result().Body)

96+ Expect(err).ToNot(HaveOccurred())

97+ Expect(string(body)).To(Equal("some upstream error"))

98+ })

4899 })

49100 })

50101})