From f6bb22c68b2dfb5b5162335b6ffda813b79e8426 Mon Sep 17 00:00:00 2001
From: Jaime Martinez <jmartinez@gitlab.com>
Date: Mon, 3 Feb 2020 15:58:19 +0000
Subject: use gorilla/handlers.ProxyHeaders to get the X-Forwarded-* headers
 and set them in the appropriate http.Request fields

---
 internal/request/request.go         | 25 ++++++++++++++--
 internal/request/request_test.go    | 57 +++++++++++++++++++++++++++++++++++++
 internal/testhelpers/testhelpers.go | 16 +++++++++++
 3 files changed, 96 insertions(+), 2 deletions(-)

(limited to 'internal')

diff --git a/internal/request/request.go b/internal/request/request.go
index 06a45071..ba56f45b 100644
--- a/internal/request/request.go
+++ b/internal/request/request.go
@@ -5,6 +5,8 @@ import (
 	"net"
 	"net/http"
 
+	log "github.com/sirupsen/logrus"
+
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 )
 
@@ -14,6 +16,11 @@ const (
 	ctxHTTPSKey  ctxKey = "https"
 	ctxHostKey   ctxKey = "host"
 	ctxDomainKey ctxKey = "domain"
+
+	// SchemeHTTP name for the HTTP scheme
+	SchemeHTTP = "http"
+	// SchemeHTTPS name for the HTTPS scheme
+	SchemeHTTPS = "https"
 )
 
 // WithHTTPSFlag saves https flag in request's context
@@ -23,9 +30,23 @@ func WithHTTPSFlag(r *http.Request, https bool) *http.Request {
 	return r.WithContext(ctx)
 }
 
-// IsHTTPS restores https flag from request's context
+// IsHTTPS checks whether the request originated from HTTP or HTTPS.
+// It reads the ctxHTTPSKey from the context and returns its value
+// It also checks that r.URL.Scheme matches the value in ctxHTTPSKey for HTTPS requests
+// TODO: remove the ctxHTTPSKey from the context https://gitlab.com/gitlab-org/gitlab-pages/issues/219
 func IsHTTPS(r *http.Request) bool {
-	return r.Context().Value(ctxHTTPSKey).(bool)
+	https := r.Context().Value(ctxHTTPSKey).(bool)
+
+	if https != (r.URL.Scheme == SchemeHTTPS) {
+		log.WithFields(log.Fields{
+			"ctxHTTPSKey": https,
+			"scheme":      r.URL.Scheme,
+		}).Warn("request: r.URL.Scheme does not match value in ctxHTTPSKey")
+	}
+
+	// Returning the value of ctxHTTPSKey for now, can just switch to r.URL.Scheme == SchemeHTTPS later
+	// and later can remove IsHTTPS method altogether
+	return https
 }
 
 // WithHostAndDomain saves host name and domain in the request's context
diff --git a/internal/request/request_test.go b/internal/request/request_test.go
index 642209f0..3c0f970c 100644
--- a/internal/request/request_test.go
+++ b/internal/request/request_test.go
@@ -5,9 +5,11 @@ import (
 	"net/http/httptest"
 	"testing"
 
+	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers"
 )
 
 func TestWithHTTPSFlag(t *testing.T) {
@@ -15,10 +17,65 @@ func TestWithHTTPSFlag(t *testing.T) {
 	require.NoError(t, err)
 
 	httpsRequest := WithHTTPSFlag(r, true)
+	httpsRequest.URL.Scheme = SchemeHTTPS
 	require.True(t, IsHTTPS(httpsRequest))
 
 	httpRequest := WithHTTPSFlag(r, false)
+	httpsRequest.URL.Scheme = SchemeHTTP
 	require.False(t, IsHTTPS(httpRequest))
+
+}
+
+func TestIsHTTPS(t *testing.T) {
+	hook := test.NewGlobal()
+
+	tests := []struct {
+		name           string
+		flag           bool
+		scheme         string
+		wantLogEntries string
+	}{
+		{
+			name:   "https",
+			flag:   true,
+			scheme: "https",
+		},
+		{
+			name:   "http",
+			flag:   false,
+			scheme: "http",
+		},
+		{
+			name:           "scheme true but flag is false",
+			flag:           false,
+			scheme:         "https",
+			wantLogEntries: "request: r.URL.Scheme does not match value in ctxHTTPSKey",
+		},
+		{
+			name:           "scheme false but flag is true",
+			flag:           true,
+			scheme:         "http",
+			wantLogEntries: "request: r.URL.Scheme does not match value in ctxHTTPSKey",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			hook.Reset()
+
+			r, err := http.NewRequest("GET", "/", nil)
+			require.NoError(t, err)
+			r.URL.Scheme = tt.scheme
+
+			httpsRequest := WithHTTPSFlag(r, tt.flag)
+
+			got := IsHTTPS(httpsRequest)
+			require.Equal(t, tt.flag, got)
+
+			testhelpers.AssertLogContains(t, tt.wantLogEntries, hook.AllEntries())
+		})
+	}
+
 }
 
 func TestPanics(t *testing.T) {
diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go
index c0b87d93..d703769b 100644
--- a/internal/testhelpers/testhelpers.go
+++ b/internal/testhelpers/testhelpers.go
@@ -7,6 +7,7 @@ import (
 	"net/url"
 	"testing"
 
+	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 )
 
@@ -40,4 +41,19 @@ func AssertRedirectTo(t *testing.T, handler http.HandlerFunc, method string,
 	handler(recorder, req)
 
 	require.Equal(t, expectedURL, recorder.Header().Get("Location"))
+
+}
+
+// AssertLogContains checks that wantLogEntry is contained in at least one of the log entries
+func AssertLogContains(t *testing.T, wantLogEntry string, entries []*logrus.Entry) {
+	t.Helper()
+
+	if wantLogEntry != "" {
+		messages := make([]string, len(entries))
+		for k, entry := range entries {
+			messages[k] = entry.Message
+		}
+
+		require.Contains(t, messages, wantLogEntry)
+	}
 }
-- 
cgit v1.2.3