Set max-age and secure flag for auth cookies

2 files changed, 49 insertions, 22 deletions

diff --git a/internal/auth/auth.go b/internal/auth/auth.go
index 920c5d12..77bc7d8e 100644
--- a/internal/auth/auth.go
+++ b/internal/auth/auth.go
@@ -35,6 +35,7 @@ const (
 	tokenContentTemplate   = "client_id=%s&client_secret=%s&code=%s&grant_type=authorization_code&redirect_uri=%s"
 	callbackPath           = "/auth"
 	authorizeProxyTemplate = "%s?domain=%s&state=%s"
+	authSessionMaxAge      = 60 * 10 // 10 minutes
 )
 
 var (
@@ -75,10 +76,10 @@ func (a *Auth) getSessionFromStore(r *http.Request) (*sessions.Session, error) {
 
 	if session != nil {
 		// Cookie just for this domain
-		session.Options = &sessions.Options{
-			Path:     "/",
-			HttpOnly: true,
-		}
+		session.Options.Path = "/"
+		session.Options.HttpOnly = true
+		session.Options.Secure = request.IsHTTPS(r)
+		session.Options.MaxAge = authSessionMaxAge
 	}
 
 	return session, err
@@ -582,7 +583,6 @@ func createCookieStore(storeSecret string) sessions.Store {
 // New when authentication supported this will be used to create authentication handler
 func New(pagesDomain string, storeSecret string, clientID string, clientSecret string,
 	redirectURI string, gitLabServer string) *Auth {
-
 	return &Auth{
 		pagesDomain:  pagesDomain,
 		clientID:     clientID,
diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go
index 00cdbd5b..1aa3bfae 100644
--- a/internal/auth/auth_test.go
+++ b/internal/auth/auth_test.go
@@ -29,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{}))
 }
@@ -46,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)
@@ -59,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"
@@ -69,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":
@@ -89,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",
@@ -97,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) {
@@ -139,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"
@@ -176,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"
@@ -252,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"