Merge branch '1-7-auth-cookie-fixes' into '1-7-stable'v1.7.21-7-stable

Set max-age and secure flag for auth cookies

See merge request gitlab/gitlab-pages!16

1 files changed, 47 insertions, 17 deletions

diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go
index 2fbbb938..1aa3bfae 100644
--- a/internal/auth/auth_test.go
+++ b/internal/auth/auth_test.go
@@ -13,6 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
 )
 
 func createAuth(t *testing.T) *Auth {
@@ -28,13 +29,32 @@ func defaultCookieStore() sessions.Store {
 	return createCookieStore("something-very-secret")
 }
 
+// Gorilla's sessions use request context to save session
+// Which makes session sharable between test code and actually manipulating session
+// Which leads to negative side effects: we can't test encryption, and cookie params
+// like max-age and secure are not being properly set
+// To avoid that we use fake request, and set only session cookie without copying context
+func setSessionValues(r *http.Request, values map[interface{}]interface{}) {
+	tmpRequest, _ := http.NewRequest("GET", "/", nil)
+	result := httptest.NewRecorder()
+	store := defaultCookieStore()
+
+	session, _ := store.Get(tmpRequest, "gitlab-pages")
+	session.Values = values
+	session.Save(tmpRequest, result)
+
+	for _, cookie := range result.Result().Cookies() {
+		r.AddCookie(cookie)
+	}
+}
+
 func TestTryAuthenticate(t *testing.T) {
 	auth := createAuth(t)
 
 	result := httptest.NewRecorder()
 	reqURL, err := url.Parse("/something/else")
 	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true)
 
 	assert.Equal(t, false, auth.TryAuthenticate(result, r, make(domain.Map), &sync.RWMutex{}))
 }
@@ -45,7 +65,7 @@ func TestTryAuthenticateWithError(t *testing.T) {
 	result := httptest.NewRecorder()
 	reqURL, err := url.Parse("/auth?error=access_denied")
 	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true)
 
 	assert.Equal(t, true, auth.TryAuthenticate(result, r, make(domain.Map), &sync.RWMutex{}))
 	assert.Equal(t, 401, result.Code)
@@ -58,7 +78,7 @@ func TestTryAuthenticateWithCodeButInvalidState(t *testing.T) {
 	result := httptest.NewRecorder()
 	reqURL, err := url.Parse("/auth?code=1&state=invalid")
 	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true)
 
 	session, _ := store.Get(r, "gitlab-pages")
 	session.Values["state"] = "state"
@@ -68,7 +88,7 @@ func TestTryAuthenticateWithCodeButInvalidState(t *testing.T) {
 	assert.Equal(t, 401, result.Code)
 }
 
-func TestTryAuthenticateWithCodeAndState(t *testing.T) {
+func testTryAuthenticateWithCodeAndState(t *testing.T, https bool) {
 	apiServer := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		switch r.URL.Path {
 		case "/oauth/token":
@@ -88,7 +108,6 @@ func TestTryAuthenticateWithCodeAndState(t *testing.T) {
 	apiServer.Start()
 	defer apiServer.Close()
 
-	store := defaultCookieStore()
 	auth := New("pages.gitlab-example.com",
 		"something-very-secret",
 		"id",
@@ -96,19 +115,28 @@ func TestTryAuthenticateWithCodeAndState(t *testing.T) {
 		"http://pages.gitlab-example.com/auth",
 		apiServer.URL)
 
-	result := httptest.NewRecorder()
-	reqURL, err := url.Parse("/auth?code=1&state=state")
-	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r, _ := http.NewRequest("GET", "/auth?code=1&state=state", nil)
+	r = request.WithHTTPSFlag(r, https)
 
-	session, _ := store.Get(r, "gitlab-pages")
-	session.Values["uri"] = "http://pages.gitlab-example.com/project/"
-	session.Values["state"] = "state"
-	session.Save(r, result)
+	setSessionValues(r, map[interface{}]interface{}{
+		"uri":   "https://pages.gitlab-example.com/project/",
+		"state": "state",
+	})
 
+	result := httptest.NewRecorder()
 	assert.Equal(t, true, auth.TryAuthenticate(result, r, make(domain.Map), &sync.RWMutex{}))
 	assert.Equal(t, 302, result.Code)
-	assert.Equal(t, "http://pages.gitlab-example.com/project/", result.Header().Get("Location"))
+	assert.Equal(t, "https://pages.gitlab-example.com/project/", result.Header().Get("Location"))
+	assert.Equal(t, 600, result.Result().Cookies()[0].MaxAge)
+	assert.Equal(t, https, result.Result().Cookies()[0].Secure)
+}
+
+func TestTryAuthenticateWithCodeAndStateOverHTTP(t *testing.T) {
+	testTryAuthenticateWithCodeAndState(t, false)
+}
+
+func TestTryAuthenticateWithCodeAndStateOverHTTPS(t *testing.T) {
+	testTryAuthenticateWithCodeAndState(t, true)
 }
 
 func TestCheckAuthenticationWhenAccess(t *testing.T) {
@@ -138,7 +166,7 @@ func TestCheckAuthenticationWhenAccess(t *testing.T) {
 	result := httptest.NewRecorder()
 	reqURL, err := url.Parse("/auth?code=1&state=state")
 	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true)
 
 	session, _ := store.Get(r, "gitlab-pages")
 	session.Values["access_token"] = "abc"
@@ -175,7 +203,7 @@ func TestCheckAuthenticationWhenNoAccess(t *testing.T) {
 	result := httptest.NewRecorder()
 	reqURL, err := url.Parse("/auth?code=1&state=state")
 	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true)
 
 	session, _ := store.Get(r, "gitlab-pages")
 	session.Values["access_token"] = "abc"
@@ -214,6 +242,7 @@ func TestCheckAuthenticationWhenInvalidToken(t *testing.T) {
 	reqURL, err := url.Parse("/auth?code=1&state=state")
 	require.NoError(t, err)
 	r := &http.Request{URL: reqURL}
+	r = request.WithHTTPSFlag(r, false)
 
 	session, _ := store.Get(r, "gitlab-pages")
 	session.Values["access_token"] = "abc"
@@ -250,7 +279,7 @@ func TestCheckAuthenticationWithoutProject(t *testing.T) {
 	result := httptest.NewRecorder()
 	reqURL, err := url.Parse("/auth?code=1&state=state")
 	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true)
 
 	session, _ := store.Get(r, "gitlab-pages")
 	session.Values["access_token"] = "abc"
@@ -289,6 +318,7 @@ func TestCheckAuthenticationWithoutProjectWhenInvalidToken(t *testing.T) {
 	reqURL, err := url.Parse("/auth?code=1&state=state")
 	require.NoError(t, err)
 	r := &http.Request{URL: reqURL}
+	r = request.WithHTTPSFlag(r, false)
 
 	session, _ := store.Get(r, "gitlab-pages")
 	session.Values["access_token"] = "abc"