Merge pull request #1115 from oauth2-proxy/remove-force-query

Fix upstream proxy appending `?` to requests

CHANGELOG.md

@@ -16,6 +16,7 @@
 
 ## Changes since v7.0.1
 
+- [#1115](https://github.com/oauth2-proxy/oauth2-proxy/pull/1115) Fix upstream proxy appending ? to requests (@JoelSpeed)
 - [#1117](https://github.com/oauth2-proxy/oauth2-proxy/pull/1117)  Deprecate GCP HealthCheck option (@JoelSpeed)
 - [#1104](https://github.com/oauth2-proxy/oauth2-proxy/pull/1104) Allow custom robots text pages (@JoelSpeed)
 - [#1045](https://github.com/oauth2-proxy/oauth2-proxy/pull/1045) Ensure redirect URI always has a scheme (@JoelSpeed)

pkg/upstream/http.go

@@ -116,11 +116,11 @@ func newReverseProxy(target *url.URL, upstream options.Upstream, errorHandler Pr
 		}
 	}
 
-	// Set the request director based on the PassHostHeader option
+	// Ensure we always pass the original request path
+	setProxyDirector(proxy)
+
 	if upstream.PassHostHeader != nil && !*upstream.PassHostHeader {
 		setProxyUpstreamHostHeader(proxy, target)
-	} else {
-		setProxyDirector(proxy)
 	}
 
 	// Set the error handler so that upstream connection failures render the
@@ -137,10 +137,7 @@ func setProxyUpstreamHostHeader(proxy *httputil.ReverseProxy, target *url.URL) {
 	director := proxy.Director
 	proxy.Director = func(req *http.Request) {
 		director(req)
-		// use RequestURI so that we aren't unescaping encoded slashes in the request path
 		req.Host = target.Host
-		req.URL.Opaque = req.RequestURI
-		req.URL.RawQuery = ""
 	}
 }
 
@@ -153,6 +150,7 @@ func setProxyDirector(proxy *httputil.ReverseProxy) {
 		// use RequestURI so that we aren't unescaping encoded slashes in the request path
 		req.URL.Opaque = req.RequestURI
 		req.URL.RawQuery = ""
+		req.URL.ForceQuery = false
 	}
 }
 

pkg/upstream/http_test.go

@@ -30,16 +30,17 @@ var _ = Describe("HTTP Upstream Suite", func() {
 	falsum := false
 
 	type httpUpstreamTableInput struct {
-		id               string
-		serverAddr       *string
-		target           string
-		method           string
-		body             []byte
-		signatureData    *options.SignatureData
-		existingHeaders  map[string]string
-		expectedResponse testHTTPResponse
-		expectedUpstream string
-		errorHandler     ProxyErrorHandler
+		id                     string
+		serverAddr             *string
+		target                 string
+		method                 string
+		body                   []byte
+		passUpstreamHostHeader bool
+		signatureData          *options.SignatureData
+		existingHeaders        map[string]string
+		expectedResponse       testHTTPResponse
+		expectedUpstream       string
+		errorHandler           ProxyErrorHandler
 	}
 
 	DescribeTable("HTTP Upstream ServeHTTP",
@@ -52,6 +53,9 @@ var _ = Describe("HTTP Upstream Suite", func() {
 			for key, value := range in.existingHeaders {
 				req.Header.Add(key, value)
 			}
+			if host := req.Header.Get("Host"); host != "" {
+				req.Host = host
+			}
 
 			req = middlewareapi.AddRequestScope(req, &middlewareapi.RequestScope{})
 			rw := httptest.NewRecorder()
@@ -60,7 +64,7 @@ var _ = Describe("HTTP Upstream Suite", func() {
 
 			upstream := options.Upstream{
 				ID:                    in.id,
-				PassHostHeader:        &truth,
+				PassHostHeader:        &in.passUpstreamHostHeader,
 				ProxyWebSockets:       &falsum,
 				InsecureSkipTLSVerify: false,
 				FlushInterval:         &flush,
@@ -140,6 +144,29 @@ var _ = Describe("HTTP Upstream Suite", func() {
 			},
 			expectedUpstream: "encodedSlashes",
 		}),
+		Entry("request a path with an empty query string", &httpUpstreamTableInput{
+			id:           "default",
+			serverAddr:   &serverAddr,
+			target:       "http://example.localhost/foo?",
+			method:       "GET",
+			body:         []byte{},
+			errorHandler: nil,
+			expectedResponse: testHTTPResponse{
+				code: 200,
+				header: map[string][]string{
+					contentType: {applicationJSON},
+				},
+				request: testHTTPRequest{
+					Method:     "GET",
+					URL:        "http://example.localhost/foo?",
+					Header:     map[string][]string{},
+					Body:       []byte{},
+					Host:       "example.localhost",
+					RequestURI: "http://example.localhost/foo?",
+				},
+			},
+			expectedUpstream: "default",
+		}),
 		Entry("when the request has a body", &httpUpstreamTableInput{
 			id:           "requestWithBody",
 			serverAddr:   &serverAddr,
@@ -257,6 +284,33 @@ var _ = Describe("HTTP Upstream Suite", func() {
 			},
 			expectedUpstream: "existingHeaders",
 		}),
+		Entry("when passing the existing host header", &httpUpstreamTableInput{
+			id:                     "passExistingHostHeader",
+			serverAddr:             &serverAddr,
+			target:                 "/existingHostHeader",
+			method:                 "GET",
+			body:                   []byte{},
+			errorHandler:           nil,
+			passUpstreamHostHeader: true,
+			existingHeaders: map[string]string{
+				"Host": "existing-host",
+			},
+			expectedResponse: testHTTPResponse{
+				code: 200,
+				header: map[string][]string{
+					contentType: {applicationJSON},
+				},
+				request: testHTTPRequest{
+					Method:     "GET",
+					URL:        "/existingHostHeader",
+					Header:     map[string][]string{},
+					Body:       []byte{},
+					Host:       "existing-host",
+					RequestURI: "/existingHostHeader",
+				},
+			},
+			expectedUpstream: "passExistingHostHeader",
+		}),
 	)
 
 	It("ServeHTTP, when not passing a host header", func() {