From 87ff3b1653589fa69a4c934c464b68718db74892 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Wed, 21 Aug 2019 16:00:52 +0000 Subject: Fix https downgrade for pages behind proxy We can't rely on r.TLS when pages are served behind proxy So we save https flag to a context for later usage Right now I'm trying to keep changes to a minimum since I'm planning to backport this to older versions That's why https flag is not refactored throughout the codebase The alternative way would be to use gorilla's proxy headers I'm planning to refactor to that version later --- internal/auth/auth_test.go | 3 +++ 1 file changed, 3 insertions(+) (limited to 'internal/auth/auth_test.go') diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 2fbbb938..00cdbd5b 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 { @@ -214,6 +215,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" @@ -289,6 +291,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" -- cgit v1.2.3 From 5d829169df76e7ec382c74d41307c523a32b06bc Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Mon, 5 Aug 2019 13:05:07 +0300 Subject: Set max-age and secure flag for auth cookies --- internal/auth/auth_test.go | 61 +++++++++++++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 17 deletions(-) (limited to 'internal/auth/auth_test.go') 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" -- cgit v1.2.3