From f9754e61e392203d9dbb61cc2276373b90322e85 Mon Sep 17 00:00:00 2001
From: feistel <6742251-feistel@users.noreply.gitlab.com>
Date: Mon, 25 Apr 2022 18:56:34 +0200
Subject: Add early return and tests for internal/handlers/https

---
 internal/handlers/https.go      |  6 ++++-
 internal/handlers/https_test.go | 58 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 1 deletion(-)
 create mode 100644 internal/handlers/https_test.go

(limited to 'internal')

diff --git a/internal/handlers/https.go b/internal/handlers/https.go
index 84c7e159..5598a8fc 100644
--- a/internal/handlers/https.go
+++ b/internal/handlers/https.go
@@ -7,8 +7,12 @@ import (
 )
 
 func HTTPSRedirectMiddleware(handler http.Handler, redirect bool) http.Handler {
+	if !redirect {
+		return handler
+	}
+
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if redirect && !request.IsHTTPS(r) {
+		if !request.IsHTTPS(r) {
 			redirectToHTTPS(w, r, http.StatusTemporaryRedirect)
 			return
 		}
diff --git a/internal/handlers/https_test.go b/internal/handlers/https_test.go
new file mode 100644
index 00000000..df78ecbd
--- /dev/null
+++ b/internal/handlers/https_test.go
@@ -0,0 +1,58 @@
+package handlers_test
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/handlers"
+)
+
+func TestAcmeMiddleware(t *testing.T) {
+	h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusAccepted)
+	})
+
+	httpsURL := "https://example.com"
+	httpURL := "http://example.com"
+
+	testCases := []struct {
+		name           string
+		redirect       bool
+		path           string
+		expectedStatus int
+	}{
+		{
+			name:           "http redirects to https with redirect enabled",
+			redirect:       true,
+			path:           httpURL,
+			expectedStatus: http.StatusTemporaryRedirect,
+		},
+		{
+			name:           "https handled successfully with redirect enabled",
+			redirect:       true,
+			path:           httpsURL,
+			expectedStatus: http.StatusAccepted,
+		},
+		{
+			name:           "http does not redirect to https with redirect disabled",
+			redirect:       false,
+			path:           httpURL,
+			expectedStatus: http.StatusAccepted,
+		},
+		{
+			name:           "https handled successfully with redirect disabled",
+			redirect:       false,
+			path:           httpsURL,
+			expectedStatus: http.StatusAccepted,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			m := handlers.HTTPSRedirectMiddleware(h, tc.redirect)
+			require.HTTPStatusCode(t, m.ServeHTTP, http.MethodGet, tc.path, nil, tc.expectedStatus)
+		})
+	}
+}
-- 
cgit v1.2.3


From 479af9eb05e15cf682f0c4d3b606721d94476374 Mon Sep 17 00:00:00 2001
From: feistel <6742251-feistel@users.noreply.gitlab.com>
Date: Tue, 26 Apr 2022 20:18:00 +0200
Subject: Rework HTTPS tests and assert location header

---
 internal/handlers/https_test.go | 47 ++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 19 deletions(-)

(limited to 'internal')

diff --git a/internal/handlers/https_test.go b/internal/handlers/https_test.go
index df78ecbd..c5ec812c 100644
--- a/internal/handlers/https_test.go
+++ b/internal/handlers/https_test.go
@@ -2,6 +2,7 @@ package handlers_test
 
 import (
 	"net/http"
+	"net/http/httptest"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -9,7 +10,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/handlers"
 )
 
-func TestAcmeMiddleware(t *testing.T) {
+func TestHTTPSMiddleware(t *testing.T) {
 	h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(http.StatusAccepted)
 	})
@@ -17,42 +18,50 @@ func TestAcmeMiddleware(t *testing.T) {
 	httpsURL := "https://example.com"
 	httpURL := "http://example.com"
 
-	testCases := []struct {
-		name           string
-		redirect       bool
-		path           string
-		expectedStatus int
+	testCases := map[string]struct {
+		redirect         bool
+		path             string
+		expectedStatus   int
+		expectedLocation string
 	}{
-		{
-			name:           "http redirects to https with redirect enabled",
-			redirect:       true,
-			path:           httpURL,
-			expectedStatus: http.StatusTemporaryRedirect,
+		"http redirects to https with redirect enabled": {
+			redirect:         true,
+			path:             httpURL,
+			expectedStatus:   http.StatusTemporaryRedirect,
+			expectedLocation: httpsURL,
 		},
-		{
-			name:           "https handled successfully with redirect enabled",
+		"https handled successfully with redirect enabled": {
 			redirect:       true,
 			path:           httpsURL,
 			expectedStatus: http.StatusAccepted,
 		},
-		{
-			name:           "http does not redirect to https with redirect disabled",
+		"http does not redirect to https with redirect disabled": {
 			redirect:       false,
 			path:           httpURL,
 			expectedStatus: http.StatusAccepted,
 		},
-		{
-			name:           "https handled successfully with redirect disabled",
+		"https handled successfully with redirect disabled": {
 			redirect:       false,
 			path:           httpsURL,
 			expectedStatus: http.StatusAccepted,
 		},
 	}
 
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
 			m := handlers.HTTPSRedirectMiddleware(h, tc.redirect)
 			require.HTTPStatusCode(t, m.ServeHTTP, http.MethodGet, tc.path, nil, tc.expectedStatus)
+
+			// if we expected a redirect make sure the location header is correct
+			if tc.expectedStatus == http.StatusTemporaryRedirect {
+				w := httptest.NewRecorder()
+				req, err := http.NewRequest(http.MethodGet, tc.path, nil)
+				require.NoError(t, err)
+
+				m.ServeHTTP(w, req)
+
+				require.Equal(t, []string{httpsURL}, w.Result().Header["Location"])
+			}
 		})
 	}
 }
-- 
cgit v1.2.3