fix: propagate errors during route building (#3383)

* Propagate errors during route building

This fixes cases such as invalid paths being silently discarded after
creation by throwing a visible error in such cases.
Due to the way gorilla/mux's fluent API is designed, it is necessary to
manually call `.GetError()` to check for errors while building routes.

Signed-off-by: Simon Engmann <simon.engmann@sovity.de>

* Add test for route building error propagation

Signed-off-by: Simon Engmann <simon.engmann@sovity.de>

* Add route building error propagation to changelog

Signed-off-by: Simon Engmann <simon.engmann@sovity.de>

---------

Signed-off-by: Simon Engmann <simon.engmann@sovity.de>
Co-authored-by: Simon Engmann <simon.engmann@sovity.de>

CHANGELOG.md

@@ -17,6 +17,7 @@
 - [#3374](https://github.com/oauth2-proxy/oauth2-proxy/pull/3374) fix: handle Unix socket RemoteAddr in IP resolution (@H1net)
 - [#3381](https://github.com/oauth2-proxy/oauth2-proxy/pull/3381) fix: do not log error for backend logout 204 (@artificiosus)
 - [#3327](https://github.com/oauth2-proxy/oauth2-proxy/pull/3327) fix: improve logging when session refresh token is missing (@yosri-brh)
+- [#2767](https://github.com/oauth2-proxy/oauth2-proxy/pull/2767) fix: propagate errors during route building (@sybereal)
 
 
 ## Release Highlights

pkg/upstream/proxy.go

@@ -58,7 +58,9 @@ func NewProxy(upstreams options.UpstreamConfig, sigData *options.SignatureData,
 		}
 	}
 
-	registerTrailingSlashHandler(m.serveMux)
+	if err := registerTrailingSlashHandler(m.serveMux); err != nil {
+		return nil, fmt.Errorf("could not register trailing slash handler: %w", err)
+	}
 	return m, nil
 }
 
@@ -94,8 +96,7 @@ func (m *multiUpstreamProxy) registerHTTPUpstreamProxy(upstream options.Upstream
 // registerHandler ensures the given handler is regiestered with the serveMux.
 func (m *multiUpstreamProxy) registerHandler(upstream options.Upstream, handler http.Handler, writer pagewriter.Writer) error {
 	if upstream.RewriteTarget == "" {
-		m.registerSimpleHandler(upstream.Path, handler)
-		return nil
+		return m.registerSimpleHandler(upstream.Path, handler)
 	}
 
 	return m.registerRewriteHandler(upstream, handler, writer)
@@ -103,12 +104,12 @@ func (m *multiUpstreamProxy) registerHandler(upstream options.Upstream, handler
 
 // registerSimpleHandler maintains the behaviour of the go standard serveMux
 // by ensuring any path with a trailing `/` matches all paths under that prefix.
-func (m *multiUpstreamProxy) registerSimpleHandler(path string, handler http.Handler) {
+func (m *multiUpstreamProxy) registerSimpleHandler(path string, handler http.Handler) error {
 	if strings.HasSuffix(path, "/") {
-		m.serveMux.PathPrefix(path).Handler(handler)
-	} else {
-		m.serveMux.Path(path).Handler(handler)
+		return m.serveMux.PathPrefix(path).Handler(handler).GetError()
 	}
+
+	return m.serveMux.Path(path).Handler(handler).GetError()
 }
 
 // registerRewriteHandler ensures the handler is registered for all paths
@@ -123,19 +124,18 @@ func (m *multiUpstreamProxy) registerRewriteHandler(upstream options.Upstream, h
 
 	rewrite := newRewritePath(rewriteRegExp, upstream.RewriteTarget, writer)
 	h := alice.New(rewrite).Then(handler)
-	m.serveMux.MatcherFunc(func(req *http.Request, _ *mux.RouteMatch) bool {
-		return rewriteRegExp.MatchString(req.URL.Path)
-	}).Handler(h)
 
-	return nil
+	return m.serveMux.MatcherFunc(func(req *http.Request, _ *mux.RouteMatch) bool {
+		return rewriteRegExp.MatchString(req.URL.Path)
+	}).Handler(h).GetError()
 }
 
 // registerTrailingSlashHandler creates a new matcher that will check if the
 // requested path would match if it had a trailing slash appended.
 // If the path matches with a trailing slash, we send back a redirect.
 // This allows us to be consistent with the built in go servemux implementation.
-func registerTrailingSlashHandler(serveMux *mux.Router) {
-	serveMux.MatcherFunc(func(req *http.Request, _ *mux.RouteMatch) bool {
+func registerTrailingSlashHandler(serveMux *mux.Router) error {
+	return serveMux.MatcherFunc(func(req *http.Request, _ *mux.RouteMatch) bool {
 		if strings.HasSuffix(req.URL.Path, "/") {
 			return false
 		}
@@ -149,7 +149,7 @@ func registerTrailingSlashHandler(serveMux *mux.Router) {
 		return serveMux.Match(slashReq, m)
 	}).Handler(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
 		http.Redirect(rw, req, req.URL.String()+"/", http.StatusMovedPermanently)
-	}))
+	})).GetError()
 }
 
 // sortByPathLongest ensures that the upstreams are sorted by longest path.

pkg/upstream/proxy_test.go

@@ -383,6 +383,38 @@ var _ = Describe("Proxy Suite", func() {
 		)
 	})
 
+	Context("multiUpstreamProxy errors", func() {
+		type proxyErrorTableInput struct {
+			upstreams     options.UpstreamConfig
+			expectedError string
+		}
+
+		DescribeTable("NewProxy", func(in *proxyErrorTableInput) {
+			sigData := &options.SignatureData{Hash: crypto.SHA256, Key: "secret"}
+
+			writer := &pagewriter.WriterFuncs{
+				ProxyErrorFunc: func(rw http.ResponseWriter, _ *http.Request, _ error) {
+					rw.WriteHeader(502)
+					rw.Write([]byte("Proxy Error"))
+				},
+			}
+
+			_, err := NewProxy(in.upstreams, sigData, writer)
+			Expect(err).To(MatchError(in.expectedError))
+		},
+			Entry("regex matcher without rewrite target", &proxyErrorTableInput{
+				upstreams: options.UpstreamConfig{
+					Upstreams: []options.Upstream{{
+						ID:   "api",
+						Path: "^/api/$",
+						URI:  "http://example.com",
+					}},
+				},
+				expectedError: `could not register http upstream "api": mux: path must start with a slash, got "^/api/$"`,
+			}),
+		)
+	})
+
 	Context("sortByPathLongest", func() {
 		type sortByPathLongestTableInput struct {
 			input          []options.Upstream