diff options
author | Vladimir Shushlin <vshushlin@gitlab.com> | 2022-12-28 15:52:14 +0300 |
---|---|---|
committer | Vladimir Shushlin <vshushlin@gitlab.com> | 2022-12-28 15:52:14 +0300 |
commit | 97b2c4be41e426af4e43d10df23147347252e075 (patch) | |
tree | 70fbd88362a104817bb8f5f8b26f3e51f9ac77d9 | |
parent | f62ebec61d2bbdd1810b30ebc906737cc0dc4481 (diff) | |
parent | 1abe98392d9457c05387cb40293919a0af0f181f (diff) |
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>
-rw-r--r-- | internal/auth/auth.go | 13 | ||||
-rw-r--r-- | internal/auth/auth_test.go | 21 |
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() |