Use the request path's case for the project prefix cookie path

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/gitlab-pages:jmrp-pages-cookie-path-request-case into master

What does this MR do?

A private Pages site cannot be reached through a URL that spells the project path in a different letter case from the project prefix: every visit to http://group.example.io/Private.Project/ for the project /private.project/ loops through OAuth without end. This is the loop of gitlab#526293, whose reproduction is the same mismatch the other way round: a project with the slug MyTestProject, whose prefix keeps that letter case, reached through the lowercased URL.

Pages finds the project case-insensitively (gitlab.go#L79-85), so /Private.Project/ serves /private.project/. With FF_ENABLE_PROJECT_PREFIX_COOKIE_PATH, which is enabled by default, the session cookie path is the prefix exactly as the API returns it (auth.go#L501, applied at auth.go#L194). Browsers match cookie paths case-sensitively (RFC 6265, section 5.1.4), so after a successful login the redirect back to /Private.Project/ carries no session cookie and Pages starts OAuth again. No access is granted, the site is simply unreachable through that URL.

This MR takes the prefix in the letter case of the request path when the path starts with it, through a small requestProjectPrefix helper. It is used in two places:

  • When the login starts, for the prefix that becomes the cookie path at the end of the flow.
  • When an expired SSO session is re-authenticated (auth.go#L882). The marker cookie written there must have the same path as the cookie the login set, or the browser keeps the stale cookie. Without this second change, the first one would only move the loop: a mixed-case session whose SSO session expires would be re-authenticated again and again (the mutation checks below show the new acceptance tests catching exactly that).

The cookie stays scoped to the same single project: the helper only changes the letter case of the prefix, and returns the API prefix unchanged when the request path does not start with it or continues it without a / boundary. Requests that already use the prefix's case get exactly the same cookie as before.

Behaviour change: someone who visits both /private.project/ and /Private.Project/ now gets a separate session for each spelling and logs in once for each, instead of being stuck in a loop on the second one. I did not put this behind a feature flag, since only requests that cannot log in today get a different cookie path.

The earlier MRs for gitlab#526293 (gitlab!219165 (closed), gitlab!219867 (closed) and gitlab!232431 (closed), all closed) changed the lookup paths Rails sends. The cookie path is set here, in the daemon, so this MR leaves the lookup paths as they are. It also covers the mixed-case path_prefix of a parallel deployment reported in the issue, where the cookie path was .../mr-1510/ and the request .../MR-1510/: the subgroup case of the unit test is the same shape.

Overlap: !1372 (closed), !1325, and !1370 (closed) and !1371 (closed) (stacked on !1334 (closed)) also edit internal/auth/auth.go, and all but !1325 also internal/auth/auth_test.go. !1371 (closed) adds a block at the top of checkTokenExists, about twenty lines above the line this MR changes there; the others change other functions. git merge-tree of this branch with each of their heads shows no conflict, but whichever merges second may need a rebase.

Related:

  • !1280 (merged) added the re-authentication path whose marker cookie this MR keeps in step with the login cookie.
  • #1098 tracks removing FF_ENABLE_PROJECT_PREFIX_COOKIE_PATH, which would make the cookie path behaviour permanent.

Closes gitlab#526293

The tests
  • TestProjectPrefixInSessionValuesUsesRequestCase (unit, table-driven): the prefix stored at login for the same case, a different case, a different case without a trailing slash, a subgroup, a lowercase request for a mixed-case prefix (the reproduction of the issue), a path that only continues the prefix, a path shorter than the prefix, and the root prefix.
  • TestReauthenticateExpiredSessionUsesRequestCaseForCookiePath (unit): the marker cookie path on re-authentication.
  • TestAccessControlProjectPrefixCookiePathMixedCaseURL (acceptance, real cookie jar): the page is served after a login through a mixed-case URL.
  • TestExpiredSSOSessionProjectPrefixCookiePathMixedCaseURLNoLoop and TestNamespaceInPathEnabled_ExpiredSSOSessionProjectPrefixCookiePathMixedCaseURLNoLoop (acceptance): the existing no-loop tests, moved into helpers without changing their steps, and run once more with a mixed-case URL, with and without namespace-in-path.
  • TestNamespaceInPathEnabled_AccessControlProjectPrefixCookiePathMixedCaseNamespace (acceptance, real cookie jar): with namespace-in-path, after a login at /Group.Auth/private.project/, /Group.Auth/Private.Project/, /group.auth/private.project/, /group.auth/Private.Project/ or /GROUP.AUTH/PRIVATE.PROJECT/ the cookie paths keep the URL's case and the page is served with the session the login stored. The GitLab stub now stores that namespace as Group.Auth and matches hosts case-insensitively, as GitLab's lookup does, so the other tests reach it as group.auth.
New tests on master without the fix

The unit tests and the acceptance tests ran against master at c932f07c with only the test files of this branch, the pages binary rebuilt from that tree:

--- FAIL: TestProjectPrefixInSessionValuesUsesRequestCase/different_case
    expected: "/Project/"
    actual  : "/project/"
--- FAIL: TestProjectPrefixInSessionValuesUsesRequestCase/different_case_without_trailing_slash
    expected: "/PROJECT/"
    actual  : "/project/"
--- FAIL: TestProjectPrefixInSessionValuesUsesRequestCase/different_case_in_a_subgroup
    expected: "/SubGroup/Project/"
    actual  : "/subgroup/project/"
--- FAIL: TestProjectPrefixInSessionValuesUsesRequestCase/lowercase_request_for_a_mixed-case_prefix
    expected: "/mytestproject/"
    actual  : "/MyTestProject/"
--- FAIL: TestReauthenticateExpiredSessionUsesRequestCaseForCookiePath
    expected: "/SubGroup/Project"
    actual  : "/subgroup/project"
--- FAIL: TestAccessControlProjectPrefixCookiePathMixedCaseURL
    expected: 200
    actual  : 302
--- FAIL: TestExpiredSSOSessionProjectPrefixCookiePathMixedCaseURLNoLoop
    expected: "group.auth.gitlab-example.com"
    actual  : "projects.gitlab-example.com"
--- FAIL: TestNamespaceInPathEnabled_ExpiredSSOSessionProjectPrefixCookiePathMixedCaseURLNoLoop
    expected: "/group.auth/Private.Project.3/"
    actual  : "/projects/auth"
--- FAIL: TestNamespaceInPathEnabled_AccessControlProjectPrefixCookiePathMixedCaseNamespace/mixed-case_namespace_and_project
    expected: "/Group.Auth/Private.Project"
    actual  : "/Group.Auth/private.project"
--- FAIL: TestNamespaceInPathEnabled_AccessControlProjectPrefixCookiePathMixedCaseNamespace/lower-case_namespace_and_mixed-case_project
    expected: "/group.auth/Private.Project"
    actual  : "/group.auth/private.project"
--- FAIL: TestNamespaceInPathEnabled_AccessControlProjectPrefixCookiePathMixedCaseNamespace/upper-case_namespace_and_project
    expected: "/GROUP.AUTH/PRIVATE.PROJECT"
    actual  : "/GROUP.AUTH/private.project"

In both no-loop tests the first request after the login is sent back to OAuth instead of reaching the page, which is the loop. The other cases of the table, and the existing no-loop tests in their helper form, pass on master.

With the fix

Unless a line says otherwise, in golang:1.26 (go1.26.10) and golang:1.27 (go1.27.3), the images the CI matrix uses:

  • The new tests and the existing ones next to them pass, with the pages binary rebuilt from this branch.
  • make unit-test: 1022 tests pass.
  • make acceptance: 413 tests pass.
  • make race in golang:1.26, as the race job runs it: 1435 tests pass, no race reported.
  • golangci-lint run ./... in golangci/golangci-lint:v2.12.2, as code_quality runs it: 0 issues.
  • make validate-log-fields in golang:1.26: no deprecated logging field usages.
  • make deps-check and make mocks-check with go1.26.10: both ok.
  • make build FIPS_MODE=1 with GOFIPS140 set to off and to v1.0.0, as fips-build runs it: the binary is built in FIPS mode each time.
  • Mutation checks in golang:1.26, each run against the new tests: using the API prefix again at login, using it again for the re-authentication marker, dropping the / boundary check, dropping the trailing part of the prefix, dropping the length guard, and comparing the lowercased request path with the prefix instead of comparing them case-insensitively each make at least one new test fail.

I have not tried it in a browser against a GitLab instance. The acceptance tests drive the pages binary and the project's GitLab stub through Go's net/http/cookiejar, which matches cookie paths as RFC 6265 says.

TODO

Edited by José M. Requena Plens

Merge request reports