Keep literal dollar signs in _redirects rule targets

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/gitlab-pages:jmrp-redirects-literal-dollar into master

What does this MR do?

Escapes every $ in a _redirects rule's "to" path as $$ before matchesRuleWithPlaceholderOrSplats in internal/redirects/matching.go turns it into an ExpandString template, so a literal $ in a target is kept instead of being read as a reference to a group of the "from" regular expression.

The template replaces each :name with ${name}, and regexp.(*Regexp).ExpandString then also reads any $1, $5, $id or ${id} the site owner wrote, replacing it with a captured value or with nothing when there is no such group. Every rule with a splat or a placeholder goes through it, and placeholders are enabled by default. So does a rule with neither when the request matches it only through a repeated slash or a different case: only an exact match of a rule with nothing to substitute returns the target as written. The template has worked this way since !458 (merged) added splats and placeholders.

Rule Request master This branch
/docs/* /archive/$1/:splat 301 /docs/intro /archive/intro/intro /archive/$1/intro
/foo/:id /qux/$id-:id 301 /foo/7 /qux/7-7 /qux/$id-7
/foo/* /qux$5/:splat 302 /foo/bar /qux/bar /qux$5/bar
/foo/:id /qux/$:id 301 /foo/7 /qux/${id} /qux/$7
/foo/ /qux/$1/ 301 /foo// /qux/ /qux/$1/

Behavior change: a target that used $1 or ${splat} on purpose to get a captured value now gets that text literally. The redirects documentation describes only :splat and :placeholder as substitutions, and both work as before.

Tests
  • TestMatchesRule gets five cases with a literal $ in the target: $1 and $2 with a splat, ${id} next to a placeholder of that name, $$, a $ right before a placeholder, and a rule without placeholders matched through a repeated slash.
  • TestRedirectsRewrite gets one case, /docs/* /archive/$1/:splat 301, which goes through Redirects.Rewrite.

Without the fix, on this branch's base (8d3ddba6, where the three files are as on master today), all six cases fail on the path they get. With the fix all six pass. The run without the fix, trimmed to the new cases:

--- FAIL: TestMatchesRule (0.00s)
    --- FAIL: TestMatchesRule/literal_double_dollar_with_placeholder (0.00s)
    --- FAIL: TestMatchesRule/literal_dollar_before_placeholder (0.00s)
    --- FAIL: TestMatchesRule/literal_dollar_braces_with_placeholder (0.00s)
    --- FAIL: TestMatchesRule/literal_dollar_multiple_trailing_slashes (0.00s)
    --- FAIL: TestMatchesRule/literal_dollar_numbers_with_splat (0.00s)
=== RUN   TestRedirectsRewrite/splat_rule_keeps_literal_dollar_in_target
        	            	expected: "/archive/$1/intro"
        	            	actual  : "/archive/intro/intro"
--- FAIL: TestRedirectsRewrite (0.00s)
    --- FAIL: TestRedirectsRewrite/splat_rule_keeps_literal_dollar_in_target (0.00s)

I also checked that the cases catch a wrong fix: without the escape the same six fail, escaping after the placeholders are substituted fails 70 subtests in the package, and escaping only the first $ fails the case with two of them.

The checks I ran

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:

  • make unit-test: 994 tests pass.
  • go test -race -count=1 ./internal/redirects/...: ok.
  • make acceptance: passes on this branch merged with current master (7a6bc3a1), and on this branch itself in golang:1.26.
  • make race on this branch merged with current master, in golang:1.26 as the race job runs it: 1415 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: no deprecated logging field usages.

Closes #1218 (closed)

TODO

Merge request reports