Keep _redirects rules that target a path on the site

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/gitlab-pages:jmrp-redirects-splat-leading-slashes into master

What does this MR do?

When a _redirects rule's "to" starts with a splat, such as /blog/* /:splat 301, and the splat itself starts with a slash, the templated target starts with //. regexMultipleSlashes in internal/redirects/matching.go only collapses a run of slashes that follows another character, so the leading // is kept and url.Parse reads the first segment of the splat as a host. The rule names a path on the site, but the redirect goes to another host.

This MR:

  • Collapses a run of slashes at the start of the templated target as well: (^|[^:])//+. The ^ branch leaves $1 empty, and the :// of an absolute target is still left alone.
  • Adds a check to Redirects.Rewrite: when the rewritten URL has a host or a scheme that the rule's "to" does not have, it returns an error instead of the URL. With the first change this cannot happen, so the check only keeps a later change to the matching from sending a rule that targets a path to another host. tryRedirects already treats a rewrite error as no redirect.
Rule Request master This branch
/blog/* /:splat 301 /blog//other.example/x //other.example/x /other.example/x
/blog/* /:splat 301 /blog///other.example/x //other.example/x /other.example/x
/blog/* /:splat/index.html 301 /blog//other.example //other.example/index.html /other.example/index.html
/blog/* /:splat 200 /blog//other.example/x //other.example/x /other.example/x

Related to #1216

Tests
  • TestMatchesRule: one case, /foo/* /:splat with /foo//bar/baz.
  • TestRedirectsRewrite: the four rows of the table above.
  • TestRedirectsRewriteRefusesURLThatLeavesSite: puts the old pattern back for the test, so that templating gives //other.example/x, and checks that Rewrite returns errRewriteLeavesSite and no URL.
  • Test_leavesSite: a path rewritten to a path, to a host and to a scheme, a URL rewritten to a URL, and a "to" that does not parse.

On master (fa2bfa8c) with this branch's test files, the five new cases of TestMatchesRule and TestRedirectsRewrite fail on the target they get. The two tests of the check call code that master does not have, so they were left out of that run:

--- FAIL: TestMatchesRule/splat_at_start_of_target_collapses_leading_slashes
    expected: "/bar/baz"
    actual  : "//bar/baz"
--- FAIL: TestRedirectsRewrite/splat_target_with_leading_slashes_stays_on_site
--- FAIL: TestRedirectsRewrite/splat_target_with_three_leading_slashes_stays_on_site
--- FAIL: TestRedirectsRewrite/splat_target_with_suffix_stays_on_site
--- FAIL: TestRedirectsRewrite/splat_rewrite_with_leading_slashes_stays_on_site

Each of these mutations makes at least one new test fail: the old pattern, Rewrite without the check, leavesSite without its host or its scheme clause, ignoring the host or the scheme of the rule's own "to", and accepting a "to" that does not parse.

The checks I ran

With go1.26.10 and go1.27.3, the versions of the CI matrix:

  • make unit-test: 1040 tests pass.
  • make acceptance: 413 tests pass.

With go1.26.10:

  • go build ./..., go vet ./... and gofmt -l: clean.
  • golangci-lint run ./... with v2.12.2, the version code_quality uses: 0 issues.
  • make validate-log-fields, make deps-check and make mocks-check: ok.
  • make race: no race reported. Every package passes but test/acceptance, where a few timing-dependent tests (rate limits and auth) failed on this loaded host, different ones on each run. make race on master fails the same way here, and TestNamespaceInPathEnabled_IPRateLimits fails now and then on master even when run alone.

TODO

Merge request reports