Repository: oauth2-proxy/oauth2-proxy
Pull request #668 (2020-07-07 08:01:22)

Add req.host to targetURL when redirecting to https
The req.URL.Host might not be present when redirecting to https if the
given req.URL is something like "/". In such scenario, the req.Host is
still present and valid.

## Description

This commit adds the original req.Host to the targetURL before returning
the 308 status, to avoid having a `Location: https:///` in the response.

## Motivation and Context

When running oauth2-proxy on heroku as a buildpack, some cookies where blocked since requests could be served using http. When `--force-https` was enabled, the return value in the `Location` header was: `Location: https:///`. Which meant that the missing part in that URL was the host.

## How Has This Been Tested?

I added a unit test

I also deployed several times the binary to Heroku to see what the actual values for `req.URL.String()`, `req.URL.Host` and finally `req.URL.String` were:

```
2020-07-07T07:37:13.445508+00:00 app[web.1]: [2020/07/07 07:37:13] [redirect_to_https.go:41] DEBUG: req.URL.String: /
2020-07-07T07:37:13.445530+00:00 app[web.1]: [2020/07/07 07:37:13] [redirect_to_https.go:54] DEBUG: req.URL.Host:
2020-07-07T07:37:13.445558+00:00 app[web.1]: [2020/07/07 07:37:13] [redirect_to_https.go:55] DEBUG: req.Host: staging.mydomain.com
```
The problem being `req.URL.String` just a path, meant that when copying this to the targetURL the value of the host did not exist ref: https://github.com/zucaritask/oauth2-proxy/blob/8288ef24e5b63ff44d12d8f9bb032c97b48bc056/pkg/middleware/redirect_to_https.go#L37

## Checklist:

- [x] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [X] I have created a feature (non-master) branch for my PR.


JoelSpeed: Ok, if you've tested this then and it works as you say it does, then I'm happy to merge it.

We only get the host set in the request in the tests because we are using `httptest`, it checks if the host is empty and sets it to `example.com`, so it might be worth a note on that around that test case? I had to look it up

Please add a changelog entry as well

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index bca36842..8ba2989e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,7 @@
 
 ## Changes since v6.0.0
 
+- [#668](https://github.com/oauth2-proxy/oauth2-proxy/pull/668) Use req.Host in --force-https when req.URL.Host is empty (@zucaritask)
 - [#660](https://github.com/oauth2-proxy/oauth2-proxy/pull/660) Use builder pattern to simplify requests to external endpoints (@JoelSpeed)
 - [#591](https://github.com/oauth2-proxy/oauth2-proxy/pull/591) Introduce upstream package with new reverse proxy implementation (@JoelSpeed)
 - [#576](https://github.com/oauth2-proxy/oauth2-proxy/pull/576) Separate Cookie validation out of main options validation (@JoelSpeed)
diff --git a/pkg/middleware/redirect_to_https.go b/pkg/middleware/redirect_to_https.go
index 141d076a..27384561 100644
--- a/pkg/middleware/redirect_to_https.go
+++ b/pkg/middleware/redirect_to_https.go
@@ -38,6 +38,11 @@ func redirectToHTTPS(httpsPort string, next http.Handler) http.Handler {
 		// Set the scheme to HTTPS
 		targetURL.Scheme = httpsScheme
 
+		// Set the req.Host when the targetURL still does not have one
+		if targetURL.Host == "" {
+			targetURL.Host = req.Host
+		}
+
 		// Overwrite the port if the original request was to a non-standard port
 		if targetURL.Port() != "" {
 			// If Port was not empty, this should be fine to ignore the error


Test patch:
diff --git a/pkg/middleware/redirect_to_https_test.go b/pkg/middleware/redirect_to_https_test.go
index c4b631b5..238c3b81 100644
--- a/pkg/middleware/redirect_to_https_test.go
+++ b/pkg/middleware/redirect_to_https_test.go
@@ -154,5 +154,15 @@ var _ = Describe("RedirectToHTTPS suite", func() {
 			expectedStatus: 200,
 			expectedBody:   "test",
 		}),
+		// By using newRequest from httptest we get example.com as a Host
+		// when the target is just a path.
+		// For details: https://golang.org/pkg/net/http/httptest/#NewRequest
+		Entry("without TLS with a path as request", &requestTableInput{
+			requestString:    "/",
+			useTLS:           false,
+			expectedStatus:   308,
+			expectedBody:     permanentRedirectBody("https://example.com/"),
+			expectedLocation: "https://example.com/",
+		}),
 	)
 })