diff options
author | Naman Jagdish Gala <ngala@gitlab.com> | 2022-12-28 21:20:26 +0300 |
---|---|---|
committer | Naman Jagdish Gala <ngala@gitlab.com> | 2022-12-28 21:20:26 +0300 |
commit | 31f65f5f6dec198d2aeb083d1013cff266437122 (patch) | |
tree | 2966919b9457491c4b262013430e9efc5152bcba /internal | |
parent | 973d93daeaa0e31f0cec2e09db8838cf38c67dc5 (diff) | |
parent | 97b2c4be41e426af4e43d10df23147347252e075 (diff) |
Merge branch 'master' into 'security-arbitrary-protocol-redirection'
# Conflicts:
# internal/auth/auth_test.go
Diffstat (limited to 'internal')
-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 4f085e38..8df98178 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -416,9 +416,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 @@ -435,7 +440,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 c2b25635..9226f847 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 TestTryAuthenticateWithNonHttpDomainAndState(t *testing.T) { auth := createTestAuth(t, "", "") |