Match the pages domain on a dot boundary in namespace_in_path

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/gitlab-pages:jmrp-namespace-in-path-domain-boundary into master

What does this MR do?

Makes the three host checks of internal/namespaceinpath require a dot before the pages domain, so that with namespace_in_path enabled only subdomains of the pages domain are treated as namespace hosts:

  • isNamespaceInHost, which decides whether the middleware answers a request with a 404 (middleware.go#L109).
  • convertToNamespaceInPath, which the response writer applies to every Location header that is not an auth URL (url.go#L54).
  • isAuthURL in the response writer (responsewriter.go#L93).

Each compared the host with strings.HasSuffix(host, pagesDomain), so with pages domain example.com the custom domain myexample.com was taken for the namespace host of my: every request to it was answered with a 404, and a Location pointing to it was rewritten to https://example.com/my/.... They now compare with "." + pagesDomain, with the port where one is compared, as domainAllowed in internal/auth already does (auth.go#L221).

The middleware and Location changes belong together: with only the middleware fixed and pages domain domain.com, the HTTPS-only redirect of the custom domain test.my-domain.com to itself (http://test.my-domain.com/ to https://test.my-domain.com/) is rewritten to https://domain.com/test.my-/. The isAuthURL change has no visible effect, because for such a host the old check only decided to leave the Location as it was, which is what the response writer now does once convertToNamespaceInPath declines the host. I changed it so the three checks agree, and I am happy to drop it if you prefer.

Behavior change: with namespace_in_path enabled, a custom domain whose name ends with the pages domain string without a dot is served instead of answered with a 404, and a redirect to such a host keeps its Location. It now gets the error log lines every other custom domain already gets: can't convert URL for each request to it, and while writing location header, couldn't convert URL for a Location pointing to it. The pages domain and its subdomains behave as before.

Squash is off on purpose: here a squash commit is the MR title alone, so it would drop the commit's Changelog: fixed trailer and the fix would not reach CHANGELOG.md.

Tests

New cases for myexample.com in TestMiddleware_ServeHTTP, TestMiddleware_IsNamespaceInHost, TestConvertToNamespaceInPath, TestCustomResponseWriter_WriteHeader and TestCustomResponseWriter_IsAuthURL, and for myexample.com:8080 in TestMiddleware_IsNamespaceInHost and TestConvertToNamespaceInPath.

With the three source files of master (8d3ddba6; they are unchanged on today's master), the seven new cases fail and every existing case passes:

--- FAIL: TestMiddleware_ServeHTTP/Custom_domain_ending_with_the_pages_domain_should_not_be_modified
        expected: 200
        actual  : 404
--- FAIL: TestMiddleware_IsNamespaceInHost/Custom_domain_ending_with_the_pages_domain
--- FAIL: TestMiddleware_IsNamespaceInHost/Custom_domain_ending_with_the_pages_domain_with_port
        expected: false
        actual  : true
--- FAIL: TestCustomResponseWriter_WriteHeader/Custom_domain_ending_with_the_pages_domain_is_not_converted
        expected: "https://myexample.com/a-path"
        actual  : "https://example.com/my/a-path"
--- FAIL: TestCustomResponseWriter_IsAuthURL/http://myexample.com/auth
        expected: false
        actual  : true
--- FAIL: TestConvertToNamespaceInPath/Host_ending_with_the_pages_domain_without_a_dot_boundary
--- FAIL: TestConvertToNamespaceInPath/Host_with_port_ending_with_the_pages_domain_without_a_dot_boundary
        Expected error with "can't extract namespace from host" in chain but got nil.

Putting back the old condition in any one of the three files on its own makes the new cases for that check fail again, and only those.

The binary before and after

gitlab-pages started against the GitLab API stub the acceptance tests use (test/gitlabstub/cmd/server) and the repository's shared/pages fixtures, with -pages-domain domain.com -namespace-in-path. The stub's custom domains test2.my-domain.com and test.my-domain.com (HTTPS only) end with domain.com without being subdomains of it, while withacmechallenge.domain.com is a subdomain. curl -H 'Host: <host>' http://127.0.0.1:8090/:

host                          master   master + middleware.go only           this branch
test2.my-domain.com           404      200                                   200
test.my-domain.com            404      301 https://domain.com/test.my-/      301 https://test.my-domain.com/
withacmechallenge.domain.com  404      404                                   404

Without -namespace-in-path, master answers 200 and 301 https://test.my-domain.com/ for the first two.

With -pages-domain ab.com -namespace-in-path, the stub's custom domain redirects.custom-domain.com answers its _redirects rule /goto-domain.html https://GitLab.com/pages.html 302 with Location: https://ab.com/GitL/pages.html on master, and with Location: https://GitLab.com/pages.html on this branch and on master without -namespace-in-path.

Checks I ran

On this branch, in golang:1.26 and golang:1.27, the images the CI matrix uses (go1.26.10 and go1.27.3 today), unless noted:

  • make unit-test and make acceptance: ok
  • make race, in golang:1.26 as the race job runs it: ok
  • golangci-lint run ./... in golangci/golangci-lint:v2.12.2, as code_quality runs it: 0 issues.
  • make deps-check, make mocks-check and make validate-log-fields, in golang:1.26: ok
  • go test -short ./... on this branch merged with today's master (7a6bc3a1), in golang:1.27: ok

Closes #1217 (closed)

TODO

Merge request reports