### Summary When a `_redirects` rule matches a request, Pages builds the target by replacing each `:name` in the rule's "to" path with `${name}` and expanding the result with `regexp.(*Regexp).ExpandString` against the match of the "from" path ([`internal/redirects/matching.go#L103-111`](https://gitlab.com/gitlab-org/gitlab-pages/-/blob/7a6bc3a11d2852b5f6885753ef1f0eaa99b09f82/internal/redirects/matching.go#L103-111)). A `$` that the site owner wrote in the "to" path is not escaped first, and `ExpandString` reads `$name` and `${name}` as references to groups of the regular expression ([`Regexp.Expand`](https://pkg.go.dev/regexp#Regexp.Expand)). So `$1`, `$5`, `$id` or `${id}` in a target is replaced with a captured value, or with nothing when the expression has no such group, and the request is redirected or rewritten to a path the rule does not contain. The rule passes validation, so nothing points the site owner at it. Every rule with a splat or a placeholder goes through this template, since placeholders are enabled by default (`FF_ENABLE_PLACEHOLDERS`, [`internal/feature/feature.go#L25-28`](https://gitlab.com/gitlab-org/gitlab-pages/-/blob/7a6bc3a11d2852b5f6885753ef1f0eaa99b09f82/internal/feature/feature.go#L25-28)). A rule with neither returns its "to" path as written only when the request matches the "from" path exactly ([`matching.go#L52-54`](https://gitlab.com/gitlab-org/gitlab-pages/-/blob/7a6bc3a11d2852b5f6885753ef1f0eaa99b09f82/internal/redirects/matching.go#L52-54)); a request that matches it through a repeated slash or a different case takes the same template. ### Steps to reproduce On `master` (7a6bc3a11d2852b5f6885753ef1f0eaa99b09f82), save this test as `internal/redirects/dollar_test.go` and run `go test -run TestLiteralDollarInTarget -v ./internal/redirects/`: ```go package redirects import ( "context" "net/url" "testing" "github.com/stretchr/testify/require" netlifyRedirects "github.com/tj/go-redirects" ) func TestLiteralDollarInTarget(t *testing.T) { for _, c := range []struct{ rule, path string }{ {"/docs/* /archive/$1/:splat 301", "/docs/intro"}, {"/foo/:id /qux/$id-:id 301", "/foo/7"}, {"/foo/* /qux$5/:splat 302", "/foo/bar"}, {"/foo/:id /qux/$:id 301", "/foo/7"}, {"/foo/ /qux/$1/ 301", "/foo//"}, } { rules, err := netlifyRedirects.ParseString(c.rule) require.NoError(t, err) require.NoError(t, validateRule(rules[0])) u, err := url.Parse(c.path) require.NoError(t, err) to, status, err := (&Redirects{rules: rules}).Rewrite(context.Background(), u, "") require.NoError(t, err) t.Logf("%-32s %-12s -> %d %s", c.rule, c.path, status, to.Path) } } ``` It prints: ```plaintext === RUN TestLiteralDollarInTarget dollar_test.go:27: /docs/* /archive/$1/:splat 301 /docs/intro -> 301 /archive/intro/intro dollar_test.go:27: /foo/:id /qux/$id-:id 301 /foo/7 -> 301 /qux/7-7 dollar_test.go:27: /foo/* /qux$5/:splat 302 /foo/bar -> 302 /qux/bar dollar_test.go:27: /foo/:id /qux/$:id 301 /foo/7 -> 301 /qux/${id} dollar_test.go:27: /foo/ /qux/$1/ 301 /foo// -> 301 /qux/ --- PASS: TestLiteralDollarInTarget (0.00s) ``` I reproduced it at this level, through `Redirects.Rewrite`; I did not publish a site. ### Current behavior | Rule | Request | Target | | --- | --- | --- | | `/docs/* /archive/$1/:splat 301` | `/docs/intro` | `/archive/intro/intro` | | `/foo/:id /qux/$id-:id 301` | `/foo/7` | `/qux/7-7` | | `/foo/* /qux$5/:splat 302` | `/foo/bar` | `/qux/bar` | | `/foo/:id /qux/$:id 301` | `/foo/7` | `/qux/${id}` | | `/foo/ /qux/$1/ 301` | `/foo//` | `/qux/` | In the fourth rule the `$` before the placeholder pairs with the `$` of `${id}`, so `ExpandString` writes a `$` and the placeholder is not substituted at all. ### Expected behavior The target keeps every `$` the rule contains, as the last rule already does when the request is exactly `/foo/`: | Rule | Request | Target | | --- | --- | --- | | `/docs/* /archive/$1/:splat 301` | `/docs/intro` | `/archive/$1/intro` | | `/foo/:id /qux/$id-:id 301` | `/foo/7` | `/qux/$id-7` | | `/foo/* /qux$5/:splat 302` | `/foo/bar` | `/qux$5/bar` | | `/foo/:id /qux/$:id 301` | `/foo/7` | `/qux/$7` | | `/foo/ /qux/$1/ 301` | `/foo//` | `/qux/$1/` | The [redirects documentation](https://docs.gitlab.com/user/project/pages/redirects/) describes `:splat` and `:placeholder` as the only substitutions in a target. ### Possible fix Escape every `$` in the "to" path as `$$` before `regexPlaceholderReplacement` replaces `:name` with `${name}` ([`internal/redirects/validations.go#L20`](https://gitlab.com/gitlab-org/gitlab-pages/-/blob/7a6bc3a11d2852b5f6885753ef1f0eaa99b09f82/internal/redirects/validations.go#L20)). `ExpandString` writes `$$` back as a single `$`, and the `${name}` references added for the placeholders keep working. I have a merge request ready for it.