From 3d132f0eed3cc391d6cd6ebd2f29c6cc054d50ae Mon Sep 17 00:00:00 2001 From: ngala Date: Fri, 19 Sep 2025 23:37:55 +0530 Subject: [PATCH 1/3] Prevent cookie sharing between same subgroup We had already introduced cookie path, which was preventing cookie sharing in browser. In this MR, we are checking cookie path with request path to prevent cookie sharing programmatically. Related: https://gitlab.com/gitlab-org/gitlab/-/issues/538323 Changelog: added --- internal/auth/session.go | 37 +++ internal/auth/session_test.go | 282 ++++++++++++++++++ internal/feature/feature.go | 6 + .../acceptance/auth_namespace_in_path_test.go | 112 +++++++ test/acceptance/auth_test.go | 112 ++++++- test/gitlabstub/api_responses.go | 6 + 6 files changed, 554 insertions(+), 1 deletion(-) create mode 100644 internal/auth/session_test.go diff --git a/internal/auth/session.go b/internal/auth/session.go index f921f6b1a..717abfbf1 100644 --- a/internal/auth/session.go +++ b/internal/auth/session.go @@ -16,6 +16,7 @@ import ( const ( sessionHostKey = "_session_host" namespaceInPathKey = "_namespace_in_path" + cookiePathKey = "_cookie_path" ) type hostSession struct { @@ -39,6 +40,7 @@ func (s *hostSession) appendPath(path string) { path = strings.Trim(path, "/") if feature.ProjectPrefixCookiePath.Enabled() && len(path) > 0 { s.Options.Path = strings.TrimSuffix(s.Options.Path, "/") + "/" + path + s.Values[cookiePathKey] = s.Options.Path } } @@ -57,6 +59,11 @@ func (a *Auth) getSessionFromStore(r *http.Request) (*hostSession, error) { } session.Options.MaxAge = int(a.cookieSessionTimeout.Seconds()) + cookiePath, ok := session.Values[cookiePathKey].(string) + if !ok { + cookiePath = "" + } + if session.Values[sessionHostKey] == nil || session.Values[sessionHostKey] != r.Host { logRequest(r).WithFields(log.Fields{ "Session host": session.Values[sessionHostKey], @@ -64,8 +71,16 @@ func (a *Auth) getSessionFromStore(r *http.Request) (*hostSession, error) { "Namespace in path": namespaceInPath, }).Info("Resetting session values") + session.Values = make(map[interface{}]interface{}) + } else if feature.RequestPathCookiePathValidation.Enabled() && !isValidCookiePath(r.URL.Path, cookiePath, namespaceInPath) { + logRequest(r).WithFields(log.Fields{ + "Session cookie path": session.Values[cookiePathKey], + "Request path": r.URL.Path, + }).Info("Resetting session values as cookie path did not match") + session.Values = make(map[interface{}]interface{}) } + if len(namespaceInPath) > 0 { session.Values[namespaceInPathKey] = namespaceInPath } @@ -94,3 +109,25 @@ func (a *Auth) checkSession(w http.ResponseWriter, r *http.Request) (*hostSessio return session, nil } + +func isValidCookiePath(requestPath, cookiePath, namespaceInPath string) bool { + authPath := "/auth" + if namespaceInPath != "" { + authPath = "/" + namespaceInPath + "/auth" + requestPath = "/" + namespaceInPath + requestPath + } + + if cookiePath == "" || cookiePath == authPath || strings.TrimSuffix(cookiePath, "/") == "" { + return true + } + + if !strings.HasPrefix(requestPath, cookiePath) { + return false + } + + if requestPath == cookiePath { + return true + } + + return len(requestPath) > len(cookiePath) && requestPath[len(cookiePath)] == '/' +} diff --git a/internal/auth/session_test.go b/internal/auth/session_test.go new file mode 100644 index 000000000..746be8dc9 --- /dev/null +++ b/internal/auth/session_test.go @@ -0,0 +1,282 @@ +package auth + +import "testing" + +func TestIsValidCookiePath(t *testing.T) { + tests := []struct { + name string + requestPath string + cookiePath string + namespaceInPath string + expected bool + }{ + // Special cookie paths that always match (no namespace) + { + name: "empty cookie path matches any request path", + requestPath: "/any/path", + cookiePath: "", + namespaceInPath: "", + expected: true, + }, + { + name: "auth cookie path matches any request path", + requestPath: "/any/path", + cookiePath: "/auth", + namespaceInPath: "", + expected: true, + }, + + // Exact request path matches (no namespace) + { + name: "exact request path match", + requestPath: "/abc", + cookiePath: "/abc", + namespaceInPath: "", + expected: true, + }, + { + name: "request path with trailing slash matches cookie path", + requestPath: "/abc/", + cookiePath: "/abc", + namespaceInPath: "", + expected: true, + }, + + // Valid request path under cookie path (no namespace) + { + name: "request path under cookie path directory", + requestPath: "/abc/xyz", + cookiePath: "/abc", + namespaceInPath: "", + expected: true, + }, + { + name: "nested request path under cookie path", + requestPath: "/abc/def/xyz", + cookiePath: "/abc", + namespaceInPath: "", + expected: true, + }, + + // Invalid request paths (siblings, not subdirectories) (no namespace) + { + name: "sibling request path not under cookie path", + requestPath: "/abc2/xyz", + cookiePath: "/abc", + namespaceInPath: "", + expected: false, + }, + { + name: "similar named sibling request path", + requestPath: "/abcdef/xyz", + cookiePath: "/abc", + namespaceInPath: "", + expected: false, + }, + { + name: "sibling request path at same level", + requestPath: "/abc2", + cookiePath: "/abc", + namespaceInPath: "", + expected: false, + }, + { + name: "sibling request path with trailing slash", + requestPath: "/abc2/", + cookiePath: "/abc", + namespaceInPath: "", + expected: false, + }, + + // Request paths that don't match cookie path prefix (no namespace) + { + name: "completely different request and cookie paths", + requestPath: "/xyz/abc", + cookiePath: "/abc", + namespaceInPath: "", + expected: false, + }, + { + name: "request path shorter than cookie path", + requestPath: "/ab", + cookiePath: "/abc", + namespaceInPath: "", + expected: false, + }, + + // Root cookie path scenarios (no namespace) + { + name: "root cookie path matches any request path", + requestPath: "/any/path/here", + cookiePath: "/", + namespaceInPath: "", + expected: true, + }, + { + name: "root request path with non-root cookie path", + requestPath: "/", + cookiePath: "/abc", + namespaceInPath: "", + expected: false, + }, + { + name: "both root request and cookie paths", + requestPath: "/", + cookiePath: "/", + namespaceInPath: "", + expected: true, + }, + + // Edge cases (no namespace) + { + name: "empty request path with non-empty cookie path", + requestPath: "", + cookiePath: "/abc", + namespaceInPath: "", + expected: false, + }, + { + name: "both empty request and cookie paths", + requestPath: "", + cookiePath: "", + namespaceInPath: "", + expected: true, + }, + + // Namespace scenarios - special cookie paths + { + name: "empty cookie path matches any request path with namespace", + requestPath: "/any/path", + cookiePath: "", + namespaceInPath: "ns1", + expected: true, + }, + { + name: "namespaced auth cookie path matches any request path", + requestPath: "/any/path", + cookiePath: "/ns1/auth", + namespaceInPath: "ns1", + expected: true, + }, + { + name: "original auth path doesn't match with namespace", + requestPath: "/any/path", + cookiePath: "/auth", + namespaceInPath: "ns1", + expected: false, + }, + + // Namespace scenarios - exact matches + { + name: "exact namespaced request path match", + requestPath: "/abc", + cookiePath: "/ns1/abc", + namespaceInPath: "ns1", + expected: true, + }, + { + name: "namespaced request path with trailing slash matches cookie path", + requestPath: "/abc/", + cookiePath: "/ns1/abc", + namespaceInPath: "ns1", + expected: true, + }, + + // Namespace scenarios - valid paths under cookie path + { + name: "namespaced request path under cookie path directory", + requestPath: "/abc/xyz", + cookiePath: "/ns1/abc", + namespaceInPath: "ns1", + expected: true, + }, + { + name: "nested namespaced request path under cookie path", + requestPath: "/abc/def/xyz", + cookiePath: "/ns1/abc", + namespaceInPath: "ns1", + expected: true, + }, + + // Namespace scenarios - invalid paths + { + name: "sibling namespaced request path not under cookie path", + requestPath: "/abc2/xyz", + cookiePath: "/ns1/abc", + namespaceInPath: "ns1", + expected: false, + }, + { + name: "non-namespaced cookie path doesn't match namespaced request", + requestPath: "/abc", + cookiePath: "/abc", + namespaceInPath: "ns1", + expected: false, + }, + { + name: "wrong namespace in cookie path", + requestPath: "/abc", + cookiePath: "/ns2/abc", + namespaceInPath: "ns1", + expected: false, + }, + + // Namespace scenarios - root paths + { + name: "namespaced root cookie path matches any namespaced request path", + requestPath: "/any/path/here", + cookiePath: "/ns1", + namespaceInPath: "ns1", + expected: true, + }, + { + name: "namespaced root request path with non-root cookie path", + requestPath: "/", + cookiePath: "/ns1/abc", + namespaceInPath: "ns1", + expected: false, + }, + + // Namespace scenarios - edge cases + { + name: "empty request path with namespaced cookie path", + requestPath: "", + cookiePath: "/ns1/abc", + namespaceInPath: "ns1", + expected: false, + }, + { + name: "both empty paths with namespace", + requestPath: "", + cookiePath: "", + namespaceInPath: "ns1", + expected: true, + }, + + // Additional valid namespace scenarios + { + name: "namespace with alphanumeric characters", + requestPath: "/test", + cookiePath: "/ns123/test", + namespaceInPath: "ns123", + expected: true, + }, + { + name: "single character namespace", + requestPath: "/api", + cookiePath: "/a/api", + namespaceInPath: "a", + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isValidCookiePath(tt.requestPath, tt.cookiePath, tt.namespaceInPath) + if result != tt.expected { + t.Errorf("isValidCookiePath(%q, %q, %q) = %v, expected %v", + tt.requestPath, tt.cookiePath, tt.namespaceInPath, result, tt.expected) + } + }) + } +} diff --git a/internal/feature/feature.go b/internal/feature/feature.go index d1b75e41e..d31d2f991 100644 --- a/internal/feature/feature.go +++ b/internal/feature/feature.go @@ -43,3 +43,9 @@ var AuthorizationHeader = Feature{ EnvVariable: "FF_AUTHORIZATION_HEADER", defaultEnabled: true, } + +// RequestPathCookiePathValidation enables session reset when cookie path does not match with request path +var RequestPathCookiePathValidation = Feature{ + EnvVariable: "FF_ENABLE_REQUEST_PATH_COOKIE_PATH_VALIDATION", + defaultEnabled: false, +} diff --git a/test/acceptance/auth_namespace_in_path_test.go b/test/acceptance/auth_namespace_in_path_test.go index fdf8d7414..a346f3ef5 100644 --- a/test/acceptance/auth_namespace_in_path_test.go +++ b/test/acceptance/auth_namespace_in_path_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-pages/internal/feature" "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) @@ -243,6 +244,117 @@ func TestNamespaceInPathEnabled_AccessControlUnderCustomDomain(t *testing.T) { } } +func TestNamespaceInPathEnabled_AccessControlCookieSharing_FFEnabled(t *testing.T) { + testNamespaceInPathEnabledAccessControlCookieSharing(t, "true") +} + +func TestNamespaceInPathEnabled_AccessControlCookieSharing_FFDisabled(t *testing.T) { + testNamespaceInPathEnabledAccessControlCookieSharing(t, "false") +} + +func testNamespaceInPathEnabledAccessControlCookieSharing(t *testing.T, ffEnabled string) { + t.Setenv(feature.RequestPathCookiePathValidation.EnvVariable, ffEnabled) + RunPagesProcess(t, + withListeners([]ListenSpec{httpListener}), + withArguments([]string{ + "-config=" + defaultNamespaceInPathAuthConfig(t), + }), + ) + + tests := map[string]struct { + domain string + namespace string + path1 string + path2 string + }{ + "FF_REQUEST_PATH_COOKIE_PATH_VALIDATION": { + domain: "gitlab-example.com", + namespace: "group.auth", + path1: "/group.auth/private.project/", + path2: "/group.auth/private.project.subpath/", + }, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + rsp, err := GetRedirectPage(t, httpListener, tt.domain, tt.path1) + require.NoError(t, err) + testhelpers.Close(t, rsp.Body) + + domainCookie := rsp.Header.Get("Set-Cookie") + + projectProxyURL, err := url.Parse(rsp.Header.Get("Location")) + require.NoError(t, err) + + state := projectProxyURL.Query().Get("state") + require.Equal(t, "http://"+tt.domain+"/"+tt.namespace, projectProxyURL.Query().Get("domain")) + + // visit projects.gitlab-example.com?state=something + projectsProxyRsp, err := GetRedirectPage(t, httpListener, + projectProxyURL.Host, projectProxyURL.Path+"?"+projectProxyURL.RawQuery) + require.NoError(t, err) + testhelpers.Close(t, projectsProxyRsp.Body) + + projectsCookie := projectsProxyRsp.Header.Get("Set-Cookie") + + // visit gitlab-example.com/projects/auth?state=something&code=1 + authRsp, err := GetRedirectPageWithCookie(t, httpListener, projectProxyURL.Host, "/projects/auth?code=1&state="+ + state, projectsCookie) + + require.NoError(t, err) + testhelpers.Close(t, authRsp.Body) + + backDomainURL, err := projectProxyURL.Parse(authRsp.Header.Get("Location")) + require.NoError(t, err) + + // Will redirect to original URL + require.Equal(t, tt.domain, backDomainURL.Host) + code := backDomainURL.Query().Get("code") + require.NotEqual(t, "1", code) + + // visit gitlab-example.com/auth?code&state will set the cookie and redirect back to original page + selfRedirectRsp, err := GetRedirectPageWithCookie(t, httpListener, tt.domain, "/"+tt.namespace+"/auth?code="+code+"&state="+ + state, domainCookie) + + require.NoError(t, err) + testhelpers.Close(t, selfRedirectRsp.Body) + + // Will redirect to the page + domainCookie = selfRedirectRsp.Header.Get("Set-Cookie") + require.Equal(t, http.StatusFound, selfRedirectRsp.StatusCode) + + selfRedirectURL, err := projectProxyURL.Parse(selfRedirectRsp.Header.Get("Location")) + require.NoError(t, err) + + // Will redirect to original url + require.Equal(t, "http://"+tt.domain+tt.path1, selfRedirectURL.String()) + + // Fetch page + authRsp, err = GetRedirectPageWithCookie(t, httpListener, tt.domain, tt.path1, domainCookie) + require.NoError(t, err) + testhelpers.Close(t, authRsp.Body) + require.Equal(t, http.StatusOK, authRsp.StatusCode) + + // Try to fetch page from another subproject + secondAuthRsp, err := GetRedirectPageWithCookie(t, httpListener, tt.domain, tt.path2, domainCookie) + require.NoError(t, err) + testhelpers.Close(t, secondAuthRsp.Body) + if ffEnabled == "true" { + // it should restart the auth process ignoring already existing cookie + require.Equal(t, http.StatusFound, secondAuthRsp.StatusCode) + secondAuthURL, err := url.Parse(secondAuthRsp.Header.Get("Location")) + require.NoError(t, err) + require.Equal(t, "gitlab-example.com", secondAuthURL.Host) + require.Equal(t, "/projects/auth", secondAuthURL.Path) + require.Equal(t, "http://gitlab-example.com/group.auth", secondAuthURL.Query().Get("domain")) + require.Equal(t, "1", secondAuthURL.Query().Get("root_namespace_id")) + } else { + // it should not restart the auth process and use existing cookie + require.Equal(t, http.StatusOK, secondAuthRsp.StatusCode) + } + }) + } +} + func TestNamespaceInPathEnabled_CustomErrorPageWithAuth(t *testing.T) { RunPagesProcess(t, withListeners([]ListenSpec{httpListener}), diff --git a/test/acceptance/auth_test.go b/test/acceptance/auth_test.go index 6fc913c29..49cd7a460 100644 --- a/test/acceptance/auth_test.go +++ b/test/acceptance/auth_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-pages/internal/feature" "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) @@ -252,7 +253,7 @@ func TestAccessControlUnderCustomDomain(t *testing.T) { // it should restart the auth process ignoring already existing cookie secondAuthRsp, err := GetRedirectPageWithCookie(t, httpListener, "group.auth.gitlab-example.com", "/private.project/", domainCookie) require.NoError(t, err) - testhelpers.Close(t, authRsp.Body) + testhelpers.Close(t, secondAuthRsp.Body) secondAuthURL, err := url.Parse(secondAuthRsp.Header.Get("Location")) require.NoError(t, err) @@ -264,6 +265,115 @@ func TestAccessControlUnderCustomDomain(t *testing.T) { } } +func TestAccessControlCookieSharing_FFEnabled(t *testing.T) { + testAccessControlCookieSharing(t, "true") +} + +func TestAccessControlCookieSharing_FFDisabled(t *testing.T) { + testAccessControlCookieSharing(t, "false") +} + +func testAccessControlCookieSharing(t *testing.T, ffEnabled string) { + t.Setenv(feature.RequestPathCookiePathValidation.EnvVariable, ffEnabled) + RunPagesProcess(t, + withListeners([]ListenSpec{httpListener}), + withArguments([]string{ + "-config=" + defaultAuthConfig(t), + }), + ) + + tests := map[string]struct { + domain string + path1 string + path2 string + }{ + "FF_REQUEST_PATH_COOKIE_PATH_VALIDATION": { + domain: "group.auth.gitlab-example.com", + path1: "/private.project/", + path2: "/private.project.subpath/", + }, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + rsp, err := GetRedirectPage(t, httpListener, tt.domain, tt.path1) + require.NoError(t, err) + testhelpers.Close(t, rsp.Body) + + domainCookie := rsp.Header.Get("Set-Cookie") + + projectProxyURL, err := url.Parse(rsp.Header.Get("Location")) + require.NoError(t, err) + + state := projectProxyURL.Query().Get("state") + require.Equal(t, "http://"+tt.domain, projectProxyURL.Query().Get("domain")) + + // visit projects.gitlab-example.com?state=something + projectsProxyRsp, err := GetRedirectPage(t, httpListener, + projectProxyURL.Host, projectProxyURL.Path+"?"+projectProxyURL.RawQuery) + require.NoError(t, err) + testhelpers.Close(t, projectsProxyRsp.Body) + + projectsCookie := projectsProxyRsp.Header.Get("Set-Cookie") + + // visit projects.gitlab-example.com?state=something&code=1 + authRsp, err := GetRedirectPageWithCookie(t, httpListener, projectProxyURL.Host, "/auth?code=1&state="+ + state, projectsCookie) + + require.NoError(t, err) + testhelpers.Close(t, authRsp.Body) + + backDomainURL, err := projectProxyURL.Parse(authRsp.Header.Get("Location")) + require.NoError(t, err) + + // Will redirect to original URL + require.Equal(t, tt.domain, backDomainURL.Host) + code := backDomainURL.Query().Get("code") + require.NotEqual(t, "1", code) + + // visit group.auth.gitlab-example.com/auth?code&state will set the cookie and redirect back to original page + selfRedirectRsp, err := GetRedirectPageWithCookie(t, httpListener, tt.domain, "/auth?code="+code+"&state="+ + state, domainCookie) + + require.NoError(t, err) + testhelpers.Close(t, selfRedirectRsp.Body) + + // Will redirect to the page + domainCookie = selfRedirectRsp.Header.Get("Set-Cookie") + require.Equal(t, http.StatusFound, selfRedirectRsp.StatusCode) + + selfRedirectURL, err := projectProxyURL.Parse(selfRedirectRsp.Header.Get("Location")) + require.NoError(t, err) + + // Will redirect to original url + require.Equal(t, "http://"+tt.domain+tt.path1, selfRedirectURL.String()) + + // Fetch page + authRsp, err = GetRedirectPageWithCookie(t, httpListener, tt.domain, tt.path1, domainCookie) + require.NoError(t, err) + testhelpers.Close(t, authRsp.Body) + require.Equal(t, http.StatusOK, authRsp.StatusCode) + + // Try to fetch page from another subproject + secondAuthRsp, err := GetRedirectPageWithCookie(t, httpListener, tt.domain, tt.path2, domainCookie) + require.NoError(t, err) + testhelpers.Close(t, secondAuthRsp.Body) + if ffEnabled == "true" { + // it should restart the auth process ignoring already existing cookie + require.Equal(t, http.StatusFound, secondAuthRsp.StatusCode) + secondAuthURL, err := url.Parse(secondAuthRsp.Header.Get("Location")) + require.NoError(t, err) + require.Equal(t, "projects.gitlab-example.com", secondAuthURL.Host) + require.Equal(t, "/auth", secondAuthURL.Path) + require.Equal(t, "http://group.auth.gitlab-example.com", secondAuthURL.Query().Get("domain")) + require.Equal(t, "1", secondAuthURL.Query().Get("root_namespace_id")) + } else { + // it should not restart the auth process and use existing cookie + require.Equal(t, http.StatusOK, secondAuthRsp.StatusCode) + } + }) + } +} + func TestCustomErrorPageWithAuth(t *testing.T) { RunPagesProcess(t, withListeners([]ListenSpec{httpListener}), diff --git a/test/gitlabstub/api_responses.go b/test/gitlabstub/api_responses.go index c63c5b06b..dd885b023 100644 --- a/test/gitlabstub/api_responses.go +++ b/test/gitlabstub/api_responses.go @@ -265,6 +265,12 @@ var apiResponses = APIResponses{ accessControl: true, pathOnDisk: "group.auth/private.project/", }, + "/private.project.subpath": { + projectID: 1007, + accessControl: true, + pathOnDisk: "group.auth/private.project/", + RootNamespaceID: 1, + }, "/private.project.1": { projectID: 2006, accessControl: true, -- GitLab From 3c960c76e45bd1aebc8d5d7466b42b1d45132818 Mon Sep 17 00:00:00 2001 From: ngala Date: Mon, 29 Sep 2025 15:22:12 +0530 Subject: [PATCH 2/3] Address review comment --- internal/auth/session.go | 31 ++++++++++++++----- .../acceptance/auth_namespace_in_path_test.go | 20 ++++++------ test/acceptance/auth_test.go | 20 ++++++------ 3 files changed, 44 insertions(+), 27 deletions(-) diff --git a/internal/auth/session.go b/internal/auth/session.go index 717abfbf1..2b934fe0a 100644 --- a/internal/auth/session.go +++ b/internal/auth/session.go @@ -1,6 +1,7 @@ package auth import ( + "fmt" "net/http" "strings" @@ -59,26 +60,26 @@ func (a *Auth) getSessionFromStore(r *http.Request) (*hostSession, error) { } session.Options.MaxAge = int(a.cookieSessionTimeout.Seconds()) - cookiePath, ok := session.Values[cookiePathKey].(string) - if !ok { - cookiePath = "" - } + cookiePath, err := getCookiePathWithLogging(session) + if err != nil { + logRequest(r).WithError(err).Error("Invalid cookie path") - if session.Values[sessionHostKey] == nil || session.Values[sessionHostKey] != r.Host { + resetSessionValues(session) + } else if session.Values[sessionHostKey] == nil || session.Values[sessionHostKey] != r.Host { logRequest(r).WithFields(log.Fields{ "Session host": session.Values[sessionHostKey], "Request host": r.Host, "Namespace in path": namespaceInPath, }).Info("Resetting session values") - session.Values = make(map[interface{}]interface{}) + resetSessionValues(session) } else if feature.RequestPathCookiePathValidation.Enabled() && !isValidCookiePath(r.URL.Path, cookiePath, namespaceInPath) { logRequest(r).WithFields(log.Fields{ "Session cookie path": session.Values[cookiePathKey], "Request path": r.URL.Path, }).Info("Resetting session values as cookie path did not match") - session.Values = make(map[interface{}]interface{}) + resetSessionValues(session) } if len(namespaceInPath) > 0 { @@ -110,6 +111,22 @@ func (a *Auth) checkSession(w http.ResponseWriter, r *http.Request) (*hostSessio return session, nil } +func resetSessionValues(session *sessions.Session) { + session.Values = make(map[interface{}]interface{}) +} + +func getCookiePathWithLogging(session *sessions.Session) (string, error) { + cookiePath := "" + if cookiePathValue, exists := session.Values[cookiePathKey]; exists { + if path, ok := cookiePathValue.(string); ok { + cookiePath = path + } else { + return "", fmt.Errorf("cookie path exists but is not a string, got type %T with value %v", cookiePathValue, cookiePathValue) + } + } + return cookiePath, nil +} + func isValidCookiePath(requestPath, cookiePath, namespaceInPath string) bool { authPath := "/auth" if namespaceInPath != "" { diff --git a/test/acceptance/auth_namespace_in_path_test.go b/test/acceptance/auth_namespace_in_path_test.go index a346f3ef5..d80487aa3 100644 --- a/test/acceptance/auth_namespace_in_path_test.go +++ b/test/acceptance/auth_namespace_in_path_test.go @@ -282,28 +282,28 @@ func testNamespaceInPathEnabledAccessControlCookieSharing(t *testing.T, ffEnable domainCookie := rsp.Header.Get("Set-Cookie") - projectProxyURL, err := url.Parse(rsp.Header.Get("Location")) + projectRedirectURL, err := url.Parse(rsp.Header.Get("Location")) require.NoError(t, err) - state := projectProxyURL.Query().Get("state") - require.Equal(t, "http://"+tt.domain+"/"+tt.namespace, projectProxyURL.Query().Get("domain")) + state := projectRedirectURL.Query().Get("state") + require.Equal(t, "http://"+tt.domain+"/"+tt.namespace, projectRedirectURL.Query().Get("domain")) // visit projects.gitlab-example.com?state=something - projectsProxyRsp, err := GetRedirectPage(t, httpListener, - projectProxyURL.Host, projectProxyURL.Path+"?"+projectProxyURL.RawQuery) + projectsRedirectRsp, err := GetRedirectPage(t, httpListener, + projectRedirectURL.Host, projectRedirectURL.Path+"?"+projectRedirectURL.RawQuery) require.NoError(t, err) - testhelpers.Close(t, projectsProxyRsp.Body) + testhelpers.Close(t, projectsRedirectRsp.Body) - projectsCookie := projectsProxyRsp.Header.Get("Set-Cookie") + projectsCookie := projectsRedirectRsp.Header.Get("Set-Cookie") // visit gitlab-example.com/projects/auth?state=something&code=1 - authRsp, err := GetRedirectPageWithCookie(t, httpListener, projectProxyURL.Host, "/projects/auth?code=1&state="+ + authRsp, err := GetRedirectPageWithCookie(t, httpListener, projectRedirectURL.Host, "/projects/auth?code=1&state="+ state, projectsCookie) require.NoError(t, err) testhelpers.Close(t, authRsp.Body) - backDomainURL, err := projectProxyURL.Parse(authRsp.Header.Get("Location")) + backDomainURL, err := projectRedirectURL.Parse(authRsp.Header.Get("Location")) require.NoError(t, err) // Will redirect to original URL @@ -322,7 +322,7 @@ func testNamespaceInPathEnabledAccessControlCookieSharing(t *testing.T, ffEnable domainCookie = selfRedirectRsp.Header.Get("Set-Cookie") require.Equal(t, http.StatusFound, selfRedirectRsp.StatusCode) - selfRedirectURL, err := projectProxyURL.Parse(selfRedirectRsp.Header.Get("Location")) + selfRedirectURL, err := projectRedirectURL.Parse(selfRedirectRsp.Header.Get("Location")) require.NoError(t, err) // Will redirect to original url diff --git a/test/acceptance/auth_test.go b/test/acceptance/auth_test.go index 49cd7a460..820df0906 100644 --- a/test/acceptance/auth_test.go +++ b/test/acceptance/auth_test.go @@ -301,28 +301,28 @@ func testAccessControlCookieSharing(t *testing.T, ffEnabled string) { domainCookie := rsp.Header.Get("Set-Cookie") - projectProxyURL, err := url.Parse(rsp.Header.Get("Location")) + projectRedirectURL, err := url.Parse(rsp.Header.Get("Location")) require.NoError(t, err) - state := projectProxyURL.Query().Get("state") - require.Equal(t, "http://"+tt.domain, projectProxyURL.Query().Get("domain")) + state := projectRedirectURL.Query().Get("state") + require.Equal(t, "http://"+tt.domain, projectRedirectURL.Query().Get("domain")) // visit projects.gitlab-example.com?state=something - projectsProxyRsp, err := GetRedirectPage(t, httpListener, - projectProxyURL.Host, projectProxyURL.Path+"?"+projectProxyURL.RawQuery) + projectsRedirectRsp, err := GetRedirectPage(t, httpListener, + projectRedirectURL.Host, projectRedirectURL.Path+"?"+projectRedirectURL.RawQuery) require.NoError(t, err) - testhelpers.Close(t, projectsProxyRsp.Body) + testhelpers.Close(t, projectsRedirectRsp.Body) - projectsCookie := projectsProxyRsp.Header.Get("Set-Cookie") + projectsCookie := projectsRedirectRsp.Header.Get("Set-Cookie") // visit projects.gitlab-example.com?state=something&code=1 - authRsp, err := GetRedirectPageWithCookie(t, httpListener, projectProxyURL.Host, "/auth?code=1&state="+ + authRsp, err := GetRedirectPageWithCookie(t, httpListener, projectRedirectURL.Host, "/auth?code=1&state="+ state, projectsCookie) require.NoError(t, err) testhelpers.Close(t, authRsp.Body) - backDomainURL, err := projectProxyURL.Parse(authRsp.Header.Get("Location")) + backDomainURL, err := projectRedirectURL.Parse(authRsp.Header.Get("Location")) require.NoError(t, err) // Will redirect to original URL @@ -341,7 +341,7 @@ func testAccessControlCookieSharing(t *testing.T, ffEnabled string) { domainCookie = selfRedirectRsp.Header.Get("Set-Cookie") require.Equal(t, http.StatusFound, selfRedirectRsp.StatusCode) - selfRedirectURL, err := projectProxyURL.Parse(selfRedirectRsp.Header.Get("Location")) + selfRedirectURL, err := projectRedirectURL.Parse(selfRedirectRsp.Header.Get("Location")) require.NoError(t, err) // Will redirect to original url -- GitLab From 1af12225a74e1c78c8663b7fe8541196e992e361 Mon Sep 17 00:00:00 2001 From: ngala Date: Fri, 10 Oct 2025 13:12:00 +0530 Subject: [PATCH 3/3] Apply suggestion --- internal/auth/session.go | 80 ++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 39 deletions(-) diff --git a/internal/auth/session.go b/internal/auth/session.go index 2b934fe0a..6e01ab5e7 100644 --- a/internal/auth/session.go +++ b/internal/auth/session.go @@ -48,43 +48,45 @@ func (s *hostSession) appendPath(path string) { func (a *Auth) getSessionFromStore(r *http.Request) (*hostSession, error) { session, err := a.store.Get(r, "gitlab-pages") - if session != nil { - namespaceInPath := a.getNamespaceInPath(r) - - // Cookie just for this domain - session.Options.Path = "/" + namespaceInPath - session.Options.HttpOnly = true - session.Options.Secure = request.IsHTTPS(r) - if !request.IsHTTPS(r) { - session.Options.SameSite = http.SameSiteDefaultMode - } - session.Options.MaxAge = int(a.cookieSessionTimeout.Seconds()) - - cookiePath, err := getCookiePathWithLogging(session) - if err != nil { - logRequest(r).WithError(err).Error("Invalid cookie path") - - resetSessionValues(session) - } else if session.Values[sessionHostKey] == nil || session.Values[sessionHostKey] != r.Host { - logRequest(r).WithFields(log.Fields{ - "Session host": session.Values[sessionHostKey], - "Request host": r.Host, - "Namespace in path": namespaceInPath, - }).Info("Resetting session values") - - resetSessionValues(session) - } else if feature.RequestPathCookiePathValidation.Enabled() && !isValidCookiePath(r.URL.Path, cookiePath, namespaceInPath) { - logRequest(r).WithFields(log.Fields{ - "Session cookie path": session.Values[cookiePathKey], - "Request path": r.URL.Path, - }).Info("Resetting session values as cookie path did not match") - - resetSessionValues(session) - } + if session == nil { + return nil, err + } - if len(namespaceInPath) > 0 { - session.Values[namespaceInPathKey] = namespaceInPath - } + namespaceInPath := a.getNamespaceInPath(r) + + // Cookie just for this domain + session.Options.Path = "/" + namespaceInPath + session.Options.HttpOnly = true + session.Options.Secure = request.IsHTTPS(r) + if !request.IsHTTPS(r) { + session.Options.SameSite = http.SameSiteDefaultMode + } + session.Options.MaxAge = int(a.cookieSessionTimeout.Seconds()) + + cookiePath, err := getCookiePath(session) + if err != nil { + logRequest(r).WithError(err).Error("Invalid cookie path") + + resetSessionValues(session) + } else if session.Values[sessionHostKey] == nil || session.Values[sessionHostKey] != r.Host { + logRequest(r).WithFields(log.Fields{ + "Session host": session.Values[sessionHostKey], + "Request host": r.Host, + "Namespace in path": namespaceInPath, + }).Info("Resetting session values because host in the session doesn't match the current host") + + resetSessionValues(session) + } else if feature.RequestPathCookiePathValidation.Enabled() && !isValidCookiePath(r.URL.Path, cookiePath, namespaceInPath) { + logRequest(r).WithFields(log.Fields{ + "Session cookie path": session.Values[cookiePathKey], + "Request path": r.URL.Path, + }).Info("Resetting session values as cookie path did not match") + + resetSessionValues(session) + } + + if len(namespaceInPath) > 0 { + session.Values[namespaceInPathKey] = namespaceInPath } return &hostSession{session}, err @@ -115,7 +117,7 @@ func resetSessionValues(session *sessions.Session) { session.Values = make(map[interface{}]interface{}) } -func getCookiePathWithLogging(session *sessions.Session) (string, error) { +func getCookiePath(session *sessions.Session) (string, error) { cookiePath := "" if cookiePathValue, exists := session.Values[cookiePathKey]; exists { if path, ok := cookiePathValue.(string); ok { @@ -134,7 +136,7 @@ func isValidCookiePath(requestPath, cookiePath, namespaceInPath string) bool { requestPath = "/" + namespaceInPath + requestPath } - if cookiePath == "" || cookiePath == authPath || strings.TrimSuffix(cookiePath, "/") == "" { + if strings.TrimSuffix(cookiePath, "/") == "" || cookiePath == authPath { return true } @@ -146,5 +148,5 @@ func isValidCookiePath(requestPath, cookiePath, namespaceInPath string) bool { return true } - return len(requestPath) > len(cookiePath) && requestPath[len(cookiePath)] == '/' + return requestPath[len(cookiePath)] == '/' } -- GitLab