Merge branch 'kassio/fix-authentication-race-condition' into 'master'

Reduce race condition on pages authentication

See merge request https://gitlab.com/gitlab-org/gitlab-pages/-/merge_requests/843

Merged-by: Vladimir Shushlin <vshushlin@gitlab.com>
Approved-by: Naman Jagdish Gala <ngala@gitlab.com>
Approved-by: Vladimir Shushlin <vshushlin@gitlab.com>
Reviewed-by: Naman Jagdish Gala <ngala@gitlab.com>
Co-authored-by: Kassio Borges <kassioborgesm@gmail.com>

2 files changed, 30 insertions, 4 deletions

diff --git a/internal/auth/auth.go b/internal/auth/auth.go
index b4dc73aa..05e6c23c 100644
--- a/internal/auth/auth.go
+++ b/internal/auth/auth.go
@@ -405,9 +405,14 @@ func (a *Auth) checkTokenExists(session *hostSession, w http.ResponseWriter, r *
 	if session.Values["access_token"] == nil {
 		logRequest(r).Debug("No access token exists, redirecting user to OAuth2 login")
 
-		// Generate state hash and store requested address
-		state := base64.URLEncoding.EncodeToString(securecookie.GenerateRandomKey(16))
-		session.Values["state"] = state
+		// When the user tries to authenticate and reload the page concurrently,
+		// gitlab pages might receive a authentication request with the state already set.
+		// In these cases, we should re-use the state instead of creating a new one.
+		if session.Values["state"] == nil {
+			//Generate state hash and store requested address
+			session.Values["state"] = base64.URLEncoding.EncodeToString(securecookie.GenerateRandomKey(16))
+		}
+
 		session.Values["uri"] = getRequestAddress(r)
 
 		// Clear possible proxying
@@ -424,7 +429,7 @@ func (a *Auth) checkTokenExists(session *hostSession, w http.ResponseWriter, r *
 
 		// Because the pages domain might be in public suffix list, we have to
 		// redirect to pages domain to trigger authorization flow
-		http.Redirect(w, r, a.getProxyAddress(r, state), http.StatusFound)
+		http.Redirect(w, r, a.getProxyAddress(r, session.Values["state"].(string)), http.StatusFound)
 
 		return true
 	}
diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go
index 47a43d4c..e16c8f0b 100644
--- a/internal/auth/auth_test.go
+++ b/internal/auth/auth_test.go
@@ -173,6 +173,27 @@ func TestTryAuthenticateWithDomainAndState(t *testing.T) {
 	require.Equal(t, "/public-gitlab.example.com/oauth/authorize?client_id=id&redirect_uri=http://pages.gitlab-example.com/auth&response_type=code&state=state&scope=scope", redirect.String())
 }
 
+func TestCheckAuthenticationWhenStateIsAlreadySet(t *testing.T) {
+	auth := createTestAuth(t, "", "")
+
+	result := httptest.NewRecorder()
+
+	r, err := http.NewRequest("Get", "https://example.com/", nil)
+	require.NoError(t, err)
+
+	// pre-set an state
+	setSessionValues(t, r, auth, map[interface{}]interface{}{
+		"state": "given_state",
+	})
+
+	contentServed := auth.CheckAuthentication(result, r, &domainMock{projectID: 1000})
+	require.True(t, contentServed)
+
+	// check if the state was re-used instead of re-created
+	session, _ := auth.getSessionFromStore(r)
+	require.Equal(t, "given_state", session.Values["state"], "did not reuse the pre-set state")
+}
+
 func testTryAuthenticateWithCodeAndState(t *testing.T, https bool) {
 	t.Helper()