diff --git a/internal/auth/session.go b/internal/auth/session.go index f921f6b1a18737ac4c06a316a83c36307473a059..6e01ab5e725fa152d43f8598e5bff2266ebd1e13 100644 --- a/internal/auth/session.go +++ b/internal/auth/session.go @@ -1,6 +1,7 @@ package auth import ( + "fmt" "net/http" "strings" @@ -16,6 +17,7 @@ import ( const ( sessionHostKey = "_session_host" namespaceInPathKey = "_namespace_in_path" + cookiePathKey = "_cookie_path" ) type hostSession struct { @@ -39,36 +41,52 @@ 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 } } func (a *Auth) getSessionFromStore(r *http.Request) (*hostSession, error) { session, err := a.store.Get(r, "gitlab-pages") - if session != nil { - namespaceInPath := a.getNamespaceInPath(r) + if session == nil { + return nil, err + } - // 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()) + namespaceInPath := a.getNamespaceInPath(r) - 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") + // 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) + } - session.Values = make(map[interface{}]interface{}) - } - if len(namespaceInPath) > 0 { - session.Values[namespaceInPathKey] = namespaceInPath - } + if len(namespaceInPath) > 0 { + session.Values[namespaceInPathKey] = namespaceInPath } return &hostSession{session}, err @@ -94,3 +112,41 @@ 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 getCookiePath(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 != "" { + authPath = "/" + namespaceInPath + "/auth" + requestPath = "/" + namespaceInPath + requestPath + } + + if strings.TrimSuffix(cookiePath, "/") == "" || cookiePath == authPath { + return true + } + + if !strings.HasPrefix(requestPath, cookiePath) { + return false + } + + if requestPath == cookiePath { + return true + } + + return requestPath[len(cookiePath)] == '/' +} diff --git a/internal/auth/session_test.go b/internal/auth/session_test.go new file mode 100644 index 0000000000000000000000000000000000000000..746be8dc9ad6b5bc6d47e222054fa4ae87d2b929 --- /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 d1b75e41e7ef284f5a41fbb0df31bad320b9255e..d31d2f991828ca515f3d47bf95a22ce135fd3692 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 fdf8d74145df7516b341ff78dc9aee8db8231883..d80487aa37cc6cfaceb803ec0a599fa3e248f73e 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") + + projectRedirectURL, err := url.Parse(rsp.Header.Get("Location")) + require.NoError(t, err) + + state := projectRedirectURL.Query().Get("state") + require.Equal(t, "http://"+tt.domain+"/"+tt.namespace, projectRedirectURL.Query().Get("domain")) + + // visit projects.gitlab-example.com?state=something + projectsRedirectRsp, err := GetRedirectPage(t, httpListener, + projectRedirectURL.Host, projectRedirectURL.Path+"?"+projectRedirectURL.RawQuery) + require.NoError(t, err) + testhelpers.Close(t, projectsRedirectRsp.Body) + + projectsCookie := projectsRedirectRsp.Header.Get("Set-Cookie") + + // visit gitlab-example.com/projects/auth?state=something&code=1 + 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 := projectRedirectURL.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 := projectRedirectURL.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 6fc913c299f2dd0402ac4b3382a988901a53f1b3..820df09061e08f5c52ef6505dc8aed0f3a1fec8b 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") + + projectRedirectURL, err := url.Parse(rsp.Header.Get("Location")) + require.NoError(t, err) + + state := projectRedirectURL.Query().Get("state") + require.Equal(t, "http://"+tt.domain, projectRedirectURL.Query().Get("domain")) + + // visit projects.gitlab-example.com?state=something + projectsRedirectRsp, err := GetRedirectPage(t, httpListener, + projectRedirectURL.Host, projectRedirectURL.Path+"?"+projectRedirectURL.RawQuery) + require.NoError(t, err) + testhelpers.Close(t, projectsRedirectRsp.Body) + + projectsCookie := projectsRedirectRsp.Header.Get("Set-Cookie") + + // visit projects.gitlab-example.com?state=something&code=1 + authRsp, err := GetRedirectPageWithCookie(t, httpListener, projectRedirectURL.Host, "/auth?code=1&state="+ + state, projectsCookie) + + require.NoError(t, err) + testhelpers.Close(t, authRsp.Body) + + backDomainURL, err := projectRedirectURL.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 := projectRedirectURL.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 c63c5b06b24c540c6ff8fcb4c2e8db090074b087..dd885b023b819387a63f9471899bd91229dba8ae 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,