diff options
19 files changed, 736 insertions, 88 deletions
diff --git a/internal/redirects/helpers.go b/internal/redirects/helpers.go index 4223faae..52ae7991 100644 --- a/internal/redirects/helpers.go +++ b/internal/redirects/helpers.go @@ -1,6 +1,7 @@ package redirects import ( + "os" "strings" ) @@ -9,3 +10,9 @@ import ( func normalizePath(path string) string { return strings.TrimSuffix(path, "/") + "/" } + +// placeholdersEnabled returns whether or not placeholders and splats +// are enabled for use in the _redirects file. +func placeholdersEnabled() bool { + return os.Getenv(FFEnablePlaceholders) == "true" +} diff --git a/internal/redirects/helpers_test.go b/internal/redirects/helpers_test.go index b07763ef..988a9a8c 100644 --- a/internal/redirects/helpers_test.go +++ b/internal/redirects/helpers_test.go @@ -7,30 +7,27 @@ import ( ) func Test_normalizePath(t *testing.T) { - tests := []struct { + tests := map[string]struct { name string path string expected string }{ - { - name: "add_trailing_slash", + "add_trailing_slash": { path: "foo", expected: "foo/", }, - { - name: "leave_existing_trailing_slash", + "leave_existing_trailing_slash": { path: "foo/", expected: "foo/", }, - { - name: "leave_existing_double_trailing_slash", + "leave_existing_double_trailing_slash": { path: "foo//", expected: "foo//", }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range tests { + t.Run(name, func(t *testing.T) { got := normalizePath(tt.path) require.Equal(t, tt.expected, got) }) diff --git a/internal/redirects/matching.go b/internal/redirects/matching.go new file mode 100644 index 00000000..913cb359 --- /dev/null +++ b/internal/redirects/matching.go @@ -0,0 +1,118 @@ +package redirects + +import ( + "fmt" + "regexp" + "strings" + + netlifyRedirects "github.com/tj/go-redirects" + "gitlab.com/gitlab-org/labkit/log" +) + +var ( + regexMultipleSlashes = regexp.MustCompile(`//+`) + regexPlaceholderOrSplats = regexp.MustCompile(`(?i)\*|:[a-z]+`) +) + +// matchesRule returns `true` if the rule's "from" pattern matches the requested URL. +// +// For example, given a "from" URL like this: +// +// /a/*/url/with/:placeholders +// +// this function would match URLs like this: +// +// /a/nice/url/with/text +// /a/super/extra/nice/url/with/matches +// +// If the first return value is `true`, the second return value is the path that this +// rule should redirect/rewrite to. This path is effectively the rule's "to" path that +// has been templated with all the placeholders (if any) from the originally requested URL. +// +// TODO: Likely these should include host comparison once we have domain-level redirects +// https://gitlab.com/gitlab-org/gitlab-pages/-/issues/601 +func matchesRule(rule *netlifyRedirects.Rule, path string) (bool, string) { + // If the requested URL exactly matches this rule's "from" path, + // exit early and return the rule's "to" path to avoid building + // and compiling the regex below. + // However, only do this if there's nothing to template in the "to" path, + // to avoid redirect/rewriting to a url with a literal `:placeholder` in it. + if normalizePath(rule.From) == normalizePath(path) && !regexPlaceholderOrSplats.MatchString(rule.To) { + return true, rule.To + } + + // Any logic beyond this point handles placeholders and splats. + // If the FF_ENABLE_PLACEHOLDERS feature flag isn't enabled, exit now. + if !placeholdersEnabled() { + return false, "" + } + + var regexSegments []string + for _, segment := range strings.Split(rule.From, "/") { + if segment == "" { + continue + } else if regexSplat.MatchString(segment) { + regexSegments = append(regexSegments, `/*(?P<splat>.*)/*`) + } else if regexPlaceholder.MatchString(segment) { + segmentName := strings.Replace(segment, ":", "", 1) + regexSegments = append(regexSegments, fmt.Sprintf(`/+(?P<%s>[^/]+)`, segmentName)) + } else { + regexSegments = append(regexSegments, "/+"+regexp.QuoteMeta(segment)) + } + } + + fromRegexString := `(?i)^` + strings.Join(regexSegments, "") + `/*$` + fromRegex, err := regexp.Compile(fromRegexString) + if err != nil { + log.WithFields(log.Fields{ + "fromRegexString": fromRegexString, + "rule.From": rule.From, + "rule.To": rule.To, + "rule.Status": rule.Status, + "path": path, + }).WithError(err).Warnf("matchesRule generated an invalid regex: %q", fromRegexString) + + return false, "" + } + + template := regexPlaceholderReplacement.ReplaceAllString(rule.To, `${$placeholder}`) + submatchIndex := fromRegex.FindStringSubmatchIndex(path) + + if submatchIndex == nil { + return false, "" + } + + templatedToPath := []byte{} + templatedToPath = fromRegex.ExpandString(templatedToPath, template, path, submatchIndex) + + // Some replacements result in subsequent slashes. For example, a rule with a "to" + // like `foo/:splat/bar` will result in a path like `foo//bar` if the splat + // character matches nothing. To avoid this, replace all instances + // of multiple subsequent forward slashes with a single forward slash. + templatedToPath = regexMultipleSlashes.ReplaceAll(templatedToPath, []byte("/")) + + return true, string(templatedToPath) +} + +// `match` returns: +// 1. The first valid redirect or rewrite rule that matches the requested URL +// 2. The URL to redirect/rewrite to +// +// If no rule matches, this function returns `nil` and an empty string +func (r *Redirects) match(path string) (*netlifyRedirects.Rule, string) { + for i := range r.rules { + // assign rule to a new var to prevent the following gosec error + // G601: Implicit memory aliasing in for loop + rule := r.rules[i] + + if validateRule(rule) != nil { + continue + } + + if isMatch, path := matchesRule(&rule, path); isMatch { + return &rule, path + } + } + + return nil, "" +} diff --git a/internal/redirects/matching_test.go b/internal/redirects/matching_test.go new file mode 100644 index 00000000..6ebb12fb --- /dev/null +++ b/internal/redirects/matching_test.go @@ -0,0 +1,346 @@ +package redirects + +import ( + "testing" + + "github.com/stretchr/testify/require" + netlifyRedirects "github.com/tj/go-redirects" +) + +type testCaseData struct { + rule string + path string + expectMatch bool + expectedPath string +} + +type matchingTestSuite = map[string]testCaseData + +func mergeTestSuites(suites ...matchingTestSuite) matchingTestSuite { + var merged = make(matchingTestSuite) + for _, suite := range suites { + for name, test := range suite { + merged[name] = test + } + } + + return merged +} + +var testsWithoutPlaceholders = map[string]testCaseData{ + "exact_match": { + rule: "/foo/ /bar/", + path: "/foo/", + expectMatch: true, + expectedPath: "/bar/", + }, + "single_trailing_slash": { + rule: "/foo/ /bar/", + path: "/foo", + expectMatch: true, + expectedPath: "/bar/", + }, + "ignore_missing_slash": { + rule: "/foo /bar/", + path: "/foo/", + expectMatch: true, + expectedPath: "/bar/", + }, + "no_match": { + rule: "/foo /bar/", + path: "/foo/bar", + expectMatch: false, + expectedPath: "", + }, +} + +func Test_matchesRule(t *testing.T) { + enablePlaceholders(t) + + tests := mergeTestSuites(testsWithoutPlaceholders, map[string]testCaseData{ + // Note: the following 3 cases behave differently when + // placeholders are disabled. See the similar test cases below. + "multiple_trailing_slashes": { + rule: "/foo/ /bar/", + path: "/foo//", + expectMatch: true, + expectedPath: "/bar/", + }, + "multiple_leading_slashes": { + rule: "/foo/ /bar/", + path: "//foo", + expectMatch: true, + expectedPath: "/bar/", + }, + "multiple_slashes_in_middle": { + rule: "/foo/bar /baz/", + path: "/foo//bar", + expectMatch: true, + expectedPath: "/baz/", + }, + + "splat_match": { + rule: "/foo/*/bar /foo/:splat/qux", + path: "/foo/baz/bar", + expectMatch: true, + expectedPath: "/foo/baz/qux", + }, + "splat_match_multiple_segments": { + rule: "/foo/*/bar /foo/:splat/qux", + path: "/foo/hello/world/bar", + expectMatch: true, + expectedPath: "/foo/hello/world/qux", + }, + "splat_match_ignore_trailing_slash": { + rule: "/foo/*/bar /foo/:splat/qux", + path: "/foo/baz/bar/", + expectMatch: true, + expectedPath: "/foo/baz/qux", + }, + "splat_match_end": { + rule: "/foo/* /qux/:splat", + path: "/foo/baz/bar", + expectMatch: true, + expectedPath: "/qux/baz/bar", + }, + "splat_match_end_with_slash": { + rule: "/foo/* /qux/:splat", + path: "/foo/baz/bar/", + expectMatch: true, + expectedPath: "/qux/baz/bar/", + }, + "splat_match_beginning": { + rule: "/*/baz/bar /qux/:splat", + path: "/foo/baz/bar", + expectMatch: true, + expectedPath: "/qux/foo", + }, + "splat_match_empty_suffix": { + rule: "/foo/* /qux/:splat", + path: "/foo/", + expectMatch: true, + expectedPath: "/qux/", + }, + "splat_consumes_trailing_slash": { + rule: "/foo/* /qux/:splat", + path: "/foo", + expectMatch: true, + expectedPath: "/qux/", + }, + "splat_match_empty_prefix": { + rule: "/*/foo /qux/:splat", + path: "/foo", + expectMatch: true, + expectedPath: "/qux/", + }, + "splat_mid_segment": { + rule: "/foo*bar /qux/:splat", + path: "/foobazbar", + expectMatch: false, + expectedPath: "", + }, + "splat_mid_segment_no_content": { + rule: "/foo*bar /qux/:splat", + path: "/foobar", + expectMatch: false, + expectedPath: "", + }, + "lone_splat": { + rule: "/* /qux/:splat", + path: "/foo/bar", + expectMatch: true, + expectedPath: "/qux/foo/bar", + }, + "multiple_splats": { + rule: "/foo/*/bar/*/baz /qux/:splat", + path: "/foo/hello/bar/world/baz", + expectMatch: true, + expectedPath: "/qux/hello", + }, + "duplicate_splat_replacements": { + rule: "/foo/* /qux/:splat/:splat", + path: "/foo/hello", + expectMatch: true, + expectedPath: "/qux/hello/hello", + }, + "splat_missing_path_segment_behavior": { + rule: "/foo/*/bar /foo/:splat/qux", + path: "/foo/bar", + expectMatch: true, + expectedPath: "/foo/qux", + }, + "missing_splat_placeholder": { + rule: "/foo/ /qux/:splat", + path: "/foo/", + expectMatch: true, + expectedPath: "/qux/", + }, + "placeholder_match": { + rule: "/foo/:year/:month/:day/bar /qux/:year-:month-:day", + path: "/foo/2021/08/16/bar", + expectMatch: true, + expectedPath: "/qux/2021-08-16", + }, + "placeholder_match_end": { + rule: "/foo/:placeholder /qux/:placeholder", + path: "/foo/bar", + expectMatch: true, + expectedPath: "/qux/bar", + }, + "placeholder_match_beginning": { + rule: "/:placeholder/foo /qux/:placeholder", + path: "/baz/foo", + expectMatch: true, + expectedPath: "/qux/baz", + }, + "placeholder_no_multiple_segments": { + rule: "/foo/:placeholder/bar /foo/:placeholder/qux", + path: "/foo/hello/world/bar", + expectMatch: false, + expectedPath: "", + }, + "placeholder_at_beginning_no_content": { + rule: "/:placeholder/foo /qux/:placeholder", + path: "/foo", + expectMatch: false, + expectedPath: "", + }, + "placeholder_at_end_no_content": { + rule: "/foo/:placeholder /qux/:placeholder", + path: "/foo/", + expectMatch: false, + expectedPath: "", + }, + "placeholder_mid_segment_in_from": { + rule: "/foo:placeholder /qux/:placeholder", + path: "/foorbar", + expectMatch: false, + expectedPath: "", + }, + "placeholder_mid_segment_in_to": { + rule: "/foo/:placeholder /qux/bar:placeholder", + path: "/foo/baz", + expectMatch: true, + expectedPath: "/qux/barbaz", + }, + "placeholder_missing_replacement_with_substring": { + rule: "/:foo /:foobar", + path: "/baz", + expectMatch: true, + expectedPath: "/", + }, + "placeholder_mid_segment_no_content": { + rule: "/foo:placeholder /qux/:splat", + path: "/foo", + expectMatch: false, + expectedPath: "", + }, + "placeholder_name_substring": { + rule: "/foo/:foo/:foobar /qux/:foo/:foobar", + path: "/foo/baz/quux", + expectMatch: true, + expectedPath: "/qux/baz/quux", + }, + "lone_placeholder": { + rule: "/:placeholder /qux/:placeholder", + path: "/foo", + expectMatch: true, + expectedPath: "/qux/foo", + }, + "duplicate_placeholders": { + rule: "/foo/:placeholder/bar/:placeholder/baz /qux/:placeholder", + path: "/foo/hello/bar/world/baz", + expectMatch: true, + expectedPath: "/qux/hello", + }, + "duplicate_placeholder_replacements": { + rule: "/foo/:placeholder /qux/:placeholder/:placeholder", + path: "/foo/hello", + expectMatch: true, + expectedPath: "/qux/hello/hello", + }, + "splat_and_placeholder_named_splat": { + rule: "/foo/*/bar/:splat /qux/:splat", + path: "/foo/hello/bar/world", + expectMatch: true, + expectedPath: "/qux/hello", + }, + "placeholder_named_splat_and_splat": { + rule: "/foo/:splat/bar/* /qux/:splat", + path: "/foo/hello/bar/world", + expectMatch: true, + expectedPath: "/qux/hello", + }, + + // Note: These differ slightly from Netlify's matching behavior. + // GitLab replaces _all_ placeholders in the "to" path, even if + // the placeholder doesn't have corresponding match in the "from". + // Netlify only replaces placeholders that appear in the "from". + "missing_placeholder_exact_match": { + rule: "/foo/ /qux/:placeholder", + path: "/foo/", + expectMatch: true, + + // Netlify would instead redirect to "/qux/:placeholder" + expectedPath: "/qux/", + }, + "missing_placeholder_nonexact_match": { + rule: "/foo/:placeholderA /qux/:placeholderB", + path: "/foo/bar", + expectMatch: true, + + // Netlify would instead redirect to "/qux/:placeholderB" + expectedPath: "/qux/", + }, + }) + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + rules, err := netlifyRedirects.ParseString(tt.rule) + require.NoError(t, err) + + isMatch, path := matchesRule(&rules[0], tt.path) + require.Equal(t, tt.expectMatch, isMatch) + require.Equal(t, tt.expectedPath, path) + }) + } +} + +// Tests matching behavior when the `FF_ENABLE_PLACEHOLDERS` +// feature flag is not enabled. These tests can be removed when the +// `FF_ENABLE_PLACEHOLDERS` flag is removed. +func Test_matchesRule_NoPlaceholders(t *testing.T) { + tests := mergeTestSuites(testsWithoutPlaceholders, map[string]testCaseData{ + // Note: the following 3 case behaves differently when + // placeholders are enabled. See the similar test cases above. + "multiple_trailing_slashes": { + rule: "/foo/ /bar/", + path: "/foo//", + expectMatch: false, + expectedPath: "", + }, + "multiple_leading_slashes": { + rule: "/foo/ /bar/", + path: "//foo", + expectMatch: false, + expectedPath: "", + }, + "multiple_slashes_in_middle": { + rule: "/foo/bar /baz/", + path: "/foo//bar", + expectMatch: false, + expectedPath: "", + }, + }) + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + rules, err := netlifyRedirects.ParseString(tt.rule) + require.NoError(t, err) + + isMatch, path := matchesRule(&rules[0], tt.path) + require.Equal(t, tt.expectMatch, isMatch) + require.Equal(t, tt.expectedPath, path) + }) + } +} diff --git a/internal/redirects/redirects.go b/internal/redirects/redirects.go index 8c30f747..79fa8dd6 100644 --- a/internal/redirects/redirects.go +++ b/internal/redirects/redirects.go @@ -19,13 +19,19 @@ import ( const ( // ConfigFile is the default name of the file containing the redirect rules. - // It follows Netlify's syntax but we don't support the special options yet like splats, placeholders, query parameters + // It follows Netlify's syntax but we don't support all of the special options yet // - https://docs.netlify.com/routing/redirects/ // - https://docs.netlify.com/routing/redirects/redirect-options/ ConfigFile = "_redirects" // Check https://gitlab.com/gitlab-org/gitlab-pages/-/issues/472 before increasing this value maxConfigSize = 64 * 1024 + + // maxPathSegments is used to limit the number of path segments allowed in rules URLs + maxPathSegments = 25 + + // FFEnablePlaceholders used to check whether placeholder matching is enabled or not + FFEnablePlaceholders = "FF_ENABLE_PLACEHOLDERS" ) var ( @@ -45,6 +51,7 @@ var ( errNoParams = errors.New("params not supported") errUnsupportedStatus = errors.New("status not supported") errNoForce = errors.New("force! not supported") + errTooManyPathSegments = errors.New("url path cannot contain more than 25 forward slashes") regexpPlaceholder = regexp.MustCompile(`(?i)/:[a-z]+`) ) @@ -75,15 +82,16 @@ func (r *Redirects) Status() string { // Rewrite takes in a URL and uses the parsed Netlify rules to rewrite // the URL to the new location if it matches any rule -func (r *Redirects) Rewrite(url *url.URL) (*url.URL, int, error) { - rule := r.match(url) +func (r *Redirects) Rewrite(originalURL *url.URL) (*url.URL, int, error) { + rule, newPath := r.match(originalURL.Path) if rule == nil { return nil, 0, ErrNoRedirect } - newURL, err := url.Parse(rule.To) + newURL, err := url.Parse(newPath) + log.WithFields(log.Fields{ - "url": url, + "url": originalURL, "newURL": newURL, "err": err, "rule.From": rule.From, diff --git a/internal/redirects/redirects_benchmark_test.go b/internal/redirects/redirects_benchmark_test.go index 1a05a00c..35e5cc25 100644 --- a/internal/redirects/redirects_benchmark_test.go +++ b/internal/redirects/redirects_benchmark_test.go @@ -4,6 +4,7 @@ import ( "context" "io/ioutil" "net/url" + "os" "path" "strings" "testing" @@ -14,8 +15,15 @@ import ( ) func generateRedirectsFile(dirPath string, count int) error { - content := strings.Repeat("/goto.html /target.html 301\n", count) - content = content + "/entrance.html /exit.html 301\n" + content := "/start.html /redirect.html 301\n" + if placeholdersEnabled() { + content += strings.Repeat("/foo/*/bar /foo/:splat/qux 200\n", count/2) + content += strings.Repeat("/foo/:placeholder /qux/:placeholder 200\n", count/2) + } else { + content += strings.Repeat("/goto.html /target.html 301\n", count) + } + + content += "/entrance.html /exit.html 301\n" return ioutil.WriteFile(path.Join(dirPath, ConfigFile), []byte(content), 0600) } @@ -41,7 +49,17 @@ func benchmarkRedirectsRewrite(b *testing.B, redirectsCount int) { } } -func BenchmarkRedirectsRewrite(b *testing.B) { +func BenchmarkRedirectsRewrite_withoutPlaceholders(b *testing.B) { + b.Run("10 redirects", func(b *testing.B) { benchmarkRedirectsRewrite(b, 10) }) + b.Run("100 redirects", func(b *testing.B) { benchmarkRedirectsRewrite(b, 100) }) + b.Run("1000 redirects", func(b *testing.B) { benchmarkRedirectsRewrite(b, 1000) }) +} + +func BenchmarkRedirectsRewrite_PlaceholdersEnabled(b *testing.B) { + orig := os.Getenv(FFEnablePlaceholders) + os.Setenv(FFEnablePlaceholders, "true") + b.Cleanup(func() { os.Setenv(FFEnablePlaceholders, orig) }) + b.Run("10 redirects", func(b *testing.B) { benchmarkRedirectsRewrite(b, 10) }) b.Run("100 redirects", func(b *testing.B) { benchmarkRedirectsRewrite(b, 100) }) b.Run("1000 redirects", func(b *testing.B) { benchmarkRedirectsRewrite(b, 1000) }) diff --git a/internal/redirects/redirects_test.go b/internal/redirects/redirects_test.go index 3e8f5236..3287091c 100644 --- a/internal/redirects/redirects_test.go +++ b/internal/redirects/redirects_test.go @@ -4,6 +4,7 @@ import ( "context" "io/ioutil" "net/url" + "os" "path" "strings" "testing" @@ -14,7 +15,18 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) +// enablePlaceholders enables the FF_ENABLE_PLACEHOLDERS in tests +func enablePlaceholders(t *testing.T) { + t.Helper() + + orig := os.Getenv(FFEnablePlaceholders) + os.Setenv(FFEnablePlaceholders, "true") + t.Cleanup(func() { os.Setenv(FFEnablePlaceholders, orig) }) +} + func TestRedirectsRewrite(t *testing.T) { + enablePlaceholders(t) + tests := []struct { name string url string @@ -87,6 +99,30 @@ func TestRedirectsRewrite(t *testing.T) { expectedStatus: 301, expectedErr: "", }, + { + name: "matches_splat_rule", + url: "/the-cake/is-delicious", + rule: "/the-cake/* /is-a-lie 200", + expectedURL: "/is-a-lie", + expectedStatus: 200, + expectedErr: "", + }, + { + name: "replaces_splat_placeholdes", + url: "/from/weighted/companion/cube/path", + rule: "/from/*/path /to/:splat/path 200", + expectedURL: "/to/weighted/companion/cube/path", + expectedStatus: 200, + expectedErr: "", + }, + { + name: "matches_placeholder_rule", + url: "/the/cake/is/delicious", + rule: "/the/:placeholder/is/delicious /the/:placeholder/is/a/lie 200", + expectedURL: "/the/cake/is/a/lie", + expectedStatus: 200, + expectedErr: "", + }, } for _, tt := range tests { diff --git a/internal/redirects/validations.go b/internal/redirects/validations.go index 64ae4b95..1af5c5c5 100644 --- a/internal/redirects/validations.go +++ b/internal/redirects/validations.go @@ -3,11 +3,18 @@ package redirects import ( "net/http" "net/url" + "regexp" "strings" netlifyRedirects "github.com/tj/go-redirects" ) +var ( + regexPlaceholder = regexp.MustCompile(`(?i)^:[a-z]+$`) + regexSplat = regexp.MustCompile(`^\*$`) + regexPlaceholderReplacement = regexp.MustCompile(`(?i):(?P<placeholder>[a-z]+)`) +) + // validateURL runs validations against a rule URL. // Returns `nil` if the URL is valid. func validateURL(urlText string) error { @@ -29,21 +36,30 @@ func validateURL(urlText string) error { return errNoStartingForwardSlashInURLPath } - // No support for splats, https://docs.netlify.com/routing/redirects/redirect-options/#splats - if strings.Contains(url.Path, "*") { - return errNoSplats - } + if placeholdersEnabled() { + // Limit the number of path segments a rule can contain. + // This prevents the matching logic from generating regular + // expressions that are too large/complex. + if strings.Count(url.Path, "/") > maxPathSegments { + return errTooManyPathSegments + } + } else { + // No support for splats, https://docs.netlify.com/routing/redirects/redirect-options/#splats + if strings.Contains(url.Path, "*") { + return errNoSplats + } - // No support for placeholders, https://docs.netlify.com/routing/redirects/redirect-options/#placeholders - if regexpPlaceholder.MatchString(url.Path) { - return errNoPlaceholders + // No support for placeholders, https://docs.netlify.com/routing/redirects/redirect-options/#placeholders + if regexpPlaceholder.MatchString(url.Path) { + return errNoPlaceholders + } } return nil } // validateRule runs all validation rules on the provided rule. -// Returns `nil` if the rule is valid. +// Returns `nil` if the rule is valid func validateRule(r netlifyRedirects.Rule) error { if err := validateURL(r.From); err != nil { return err @@ -60,7 +76,7 @@ func validateRule(r netlifyRedirects.Rule) error { // We strictly validate return status codes switch r.Status { - case http.StatusMovedPermanently, http.StatusFound: + case http.StatusOK, http.StatusMovedPermanently, http.StatusFound: // noop default: return errUnsupportedStatus @@ -73,15 +89,3 @@ func validateRule(r netlifyRedirects.Rule) error { return nil } - -// match returns the first redirect or rewrite rule that matches the requested URL -func (r *Redirects) match(url *url.URL) *netlifyRedirects.Rule { - for _, rule := range r.rules { - // TODO: Likely this should include host comparison once we have domain-level redirects - if normalizePath(rule.From) == normalizePath(url.Path) && validateRule(rule) == nil { - return &rule - } - } - - return nil -} diff --git a/internal/redirects/validations_test.go b/internal/redirects/validations_test.go index 011ae9e0..9d891ece 100644 --- a/internal/redirects/validations_test.go +++ b/internal/redirects/validations_test.go @@ -1,6 +1,7 @@ package redirects import ( + "strings" "testing" "github.com/stretchr/testify/require" @@ -8,109 +9,139 @@ import ( ) func TestRedirectsValidateUrl(t *testing.T) { - tests := []struct { - name string + enablePlaceholders(t) + + tests := map[string]struct { url string expectedErr string }{ - { - name: "Valid url", + "valid_url": { url: "/goto.html", expectedErr: "", }, - { - name: "No domain-level redirects", + "no_domain_level_redirects": { url: "https://GitLab.com", expectedErr: errNoDomainLevelRedirects.Error(), }, - { - name: "No Schema-less URL domain-level redirects", + "no_schemaless_url_domain_level_redirects": { url: "//GitLab.com/pages.html", expectedErr: errNoDomainLevelRedirects.Error(), }, - { - name: "No bare domain-level redirects", + "no_bare_domain_level_redirects": { url: "GitLab.com", expectedErr: errNoStartingForwardSlashInURLPath.Error(), }, - { - name: "No parent traversing relative URL", + "no_parent_traversing_relative_url": { url: "../target.html", expectedErr: errNoStartingForwardSlashInURLPath.Error(), }, - { - name: "No splats", + "too_many_slashes": { + url: strings.Repeat("/a", 26), + expectedErr: errTooManyPathSegments.Error(), + }, + "placeholders": { + url: "/news/:year/:month/:date/:slug", + expectedErr: "", + }, + "splats": { + url: "/blog/*", + expectedErr: "", + }, + "splat_placeholders": { + url: "/new/path/:splat", + expectedErr: "", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + err := validateURL(tt.url) + if tt.expectedErr != "" { + require.EqualError(t, err, tt.expectedErr) + return + } + + require.NoError(t, err) + }) + } +} + +// Tests validation rules that only apply when the `FF_ENABLE_PLACEHOLDERS` +// feature flag is not enabled. These tests can be removed when the +// `FF_ENABLE_PLACEHOLDERS` flag is removed. +func TestRedirectsValidateUrlNoPlaceholders(t *testing.T) { + tests := map[string]struct { + url string + expectedErr string + }{ + "no_splats": { url: "/blog/*", expectedErr: errNoSplats.Error(), }, - { - name: "No Placeholders", + "no_placeholders": { url: "/news/:year/:month/:date/:slug", expectedErr: errNoPlaceholders.Error(), }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range tests { + t.Run(name, func(t *testing.T) { err := validateURL(tt.url) if tt.expectedErr != "" { require.EqualError(t, err, tt.expectedErr) - } else { - require.NoError(t, err) + return } + + require.NoError(t, err) }) } } func TestRedirectsValidateRule(t *testing.T) { - tests := []struct { - name string + enablePlaceholders(t) + + tests := map[string]struct { rule string expectedErr string }{ - { - name: "valid rule", + "valid_rule": { rule: "/goto.html /target.html 301", expectedErr: "", }, - { - name: "invalid From URL", + "invalid_from_url": { rule: "invalid.com /teapot.html 302", expectedErr: errNoStartingForwardSlashInURLPath.Error(), }, - { - name: "invalid To URL", + "invalid_to_url": { rule: "/goto.html invalid.com", expectedErr: errNoStartingForwardSlashInURLPath.Error(), }, - { - name: "No parameters", + "no_parameters": { rule: "/ /something 302 foo=bar", expectedErr: errNoParams.Error(), }, - { - name: "Invalid status", + "invalid_status": { rule: "/goto.html /target.html 418", expectedErr: errUnsupportedStatus.Error(), }, - { - name: "Force not supported", + "force_not_supported": { rule: "/goto.html /target.html 302!", expectedErr: errNoForce.Error(), }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range tests { + t.Run(name, func(t *testing.T) { rules, err := netlifyRedirects.ParseString(tt.rule) require.NoError(t, err) err = validateRule(rules[0]) if tt.expectedErr != "" { require.EqualError(t, err, tt.expectedErr) - } else { - require.NoError(t, err) + return } + + require.NoError(t, err) }) } } diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go index 12223bad..7b92dfad 100644 --- a/internal/serving/disk/reader.go +++ b/internal/serving/disk/reader.go @@ -57,6 +57,11 @@ func (reader *Reader) tryRedirects(h serving.Handler) bool { return false } + if status == http.StatusOK { + h.SubPath = strings.TrimPrefix(rewrittenURL.Path, h.LookupPath.Prefix) + return reader.tryFile(h) + } + http.Redirect(h.Writer, h.Request, rewrittenURL.Path, status) return true } diff --git a/shared/pages/group.redirects/project-redirects/public.zip b/shared/pages/group.redirects/project-redirects/public.zip Binary files differindex c6421223..88c2cbb0 100644 --- a/shared/pages/group.redirects/project-redirects/public.zip +++ b/shared/pages/group.redirects/project-redirects/public.zip diff --git a/shared/pages/group.redirects/project-redirects/public/_redirects b/shared/pages/group.redirects/project-redirects/public/_redirects index 04c44ee4..0adee383 100644 --- a/shared/pages/group.redirects/project-redirects/public/_redirects +++ b/shared/pages/group.redirects/project-redirects/public/_redirects @@ -1,11 +1,14 @@ -/project-redirects/redirect-portal.html /project-redirects/magic-land.html 302 -/project-redirects/cake-portal.html /project-redirects/still-alive.html 302 -/project-redirects/jobs/* /project-redirects/careers/:splat -/project-redirects/wardrobe.html /project-redirects/narnia.html 302 +/project-redirects/redirect-portal.html /project-redirects/magic-land.html 302 +/project-redirects/cake-portal.html /project-redirects/still-alive.html 302 +/project-redirects/jobs/* /project-redirects/careers/:splat +/project-redirects/arrakis/* /project-redirects/rakis/:splat 200 +/project-redirects/wardrobe.html /project-redirects/narnia.html 302 /project-redirects/news/:year/:month/:date/:slug /project-redirects/blog/:year/:month/:date/:slug -/project-redirects/pit.html /project-redirects/spikes.html 302 -/project-redirects/goto-domain.html https://GitLab.com/pages.html 302 -/project-redirects/goto-bare-domain.html GitLab.com/pages.html 302 -/project-redirects/goto-schemaless.html //GitLab.com/pages.html 302 -/project-redirects/cake-portal/ /project-redirects/still-alive/ 302 -/project-redirects/file-override.html /project-redirects/should-not-be-here.html 302 +/project-redirects/pit.html /project-redirects/spikes.html 302 +/project-redirects/goto-domain.html https://GitLab.com/pages.html 302 +/project-redirects/goto-bare-domain.html GitLab.com/pages.html 302 +/project-redirects/goto-schemaless.html //GitLab.com/pages.html 302 +/project-redirects/cake-portal/ /project-redirects/still-alive/ 302 +/project-redirects/file-override.html /project-redirects/should-not-be-here.html 302 +/project-redirects/spa/* /project-redirects/spa/index.html 200 +/project-redirects/blog/:year/:month/:day /project-redirects/blog-post-:year-:month-:day.html 200 diff --git a/shared/pages/group.redirects/project-redirects/public/blog-post-2021-08-12.html b/shared/pages/group.redirects/project-redirects/public/blog-post-2021-08-12.html new file mode 100644 index 00000000..58cea2d0 --- /dev/null +++ b/shared/pages/group.redirects/project-redirects/public/blog-post-2021-08-12.html @@ -0,0 +1 @@ +Rewrites are pretty neat! diff --git a/shared/pages/group.redirects/project-redirects/public/rakis/visitors-guide.html b/shared/pages/group.redirects/project-redirects/public/rakis/visitors-guide.html new file mode 100644 index 00000000..dca3a06b --- /dev/null +++ b/shared/pages/group.redirects/project-redirects/public/rakis/visitors-guide.html @@ -0,0 +1 @@ +Welcome to Dune! diff --git a/shared/pages/group.redirects/project-redirects/public/spa/existing-file.html b/shared/pages/group.redirects/project-redirects/public/spa/existing-file.html new file mode 100644 index 00000000..25e581b4 --- /dev/null +++ b/shared/pages/group.redirects/project-redirects/public/spa/existing-file.html @@ -0,0 +1 @@ +This is an existing file diff --git a/shared/pages/group.redirects/project-redirects/public/spa/index.html b/shared/pages/group.redirects/project-redirects/public/spa/index.html new file mode 100644 index 00000000..2c48053e --- /dev/null +++ b/shared/pages/group.redirects/project-redirects/public/spa/index.html @@ -0,0 +1 @@ +This is an SPA diff --git a/shared/pages/group/project/public/_redirects b/shared/pages/group/project/public/_redirects new file mode 100644 index 00000000..a8c19716 --- /dev/null +++ b/shared/pages/group/project/public/_redirects @@ -0,0 +1 @@ +/project/old/path/* /project/index.html 200 diff --git a/test/acceptance/redirects_test.go b/test/acceptance/redirects_test.go index f2748789..dd1b6891 100644 --- a/test/acceptance/redirects_test.go +++ b/test/acceptance/redirects_test.go @@ -7,12 +7,14 @@ import ( "testing" "github.com/stretchr/testify/require" + + redirects "gitlab.com/gitlab-org/gitlab-pages/internal/redirects" ) func TestDisabledRedirects(t *testing.T) { RunPagesProcess(t, withListeners([]ListenSpec{httpListener}), - withEnv([]string{"FF_ENABLE_REDIRECTS=false"}), + withEnv([]string{"FF_ENABLE_REDIRECTS=false", redirects.FFEnablePlaceholders + "=true"}), ) // Test that redirects status page is forbidden @@ -33,6 +35,7 @@ func TestDisabledRedirects(t *testing.T) { func TestRedirectStatusPage(t *testing.T) { RunPagesProcess(t, withListeners([]ListenSpec{httpListener}), + withEnv([]string{redirects.FFEnablePlaceholders + "=true"}), ) rsp, err := GetPageFromListener(t, httpListener, "group.redirects.gitlab-example.com", "/project-redirects/_redirects") @@ -42,13 +45,14 @@ func TestRedirectStatusPage(t *testing.T) { require.NoError(t, err) defer rsp.Body.Close() - require.Contains(t, string(body), "11 rules") + require.Contains(t, string(body), "14 rules") require.Equal(t, http.StatusOK, rsp.StatusCode) } func TestRedirect(t *testing.T) { RunPagesProcess(t, withListeners([]ListenSpec{httpListener}), + withEnv([]string{redirects.FFEnablePlaceholders + "=true"}), ) // Test that serving a file still works with redirects enabled @@ -99,6 +103,13 @@ func TestRedirect(t *testing.T) { expectedStatus: http.StatusFound, expectedLocation: "/magic-land.html", }, + // Permanent redirect for splat (*) with replacement (:splat) + { + host: "group.redirects.gitlab-example.com", + path: "/project-redirects/jobs/assistant-to-the-regional-manager.html", + expectedStatus: http.StatusMovedPermanently, + expectedLocation: "/project-redirects/careers/assistant-to-the-regional-manager.html", + }, } for _, tt := range tests { diff --git a/test/acceptance/rewrites_test.go b/test/acceptance/rewrites_test.go new file mode 100644 index 00000000..a446a8cd --- /dev/null +++ b/test/acceptance/rewrites_test.go @@ -0,0 +1,59 @@ +package acceptance_test + +import ( + "io/ioutil" + "net/http" + "testing" + + "github.com/stretchr/testify/require" + + redirects "gitlab.com/gitlab-org/gitlab-pages/internal/redirects" +) + +func TestRewrites(t *testing.T) { + RunPagesProcess(t, + withListeners([]ListenSpec{httpListener}), + withEnv([]string{redirects.FFEnablePlaceholders + "=true"}), + ) + + tests := map[string]struct { + host string + path string + expectedBody string + }{ + "rewrite_for_splat_with_replacement": { + host: "group.redirects.gitlab-example.com", + path: "/project-redirects/arrakis/visitors-guide.html", + expectedBody: "Welcome to Dune!", + }, + "splat_with_no_replacement": { + host: "group.redirects.gitlab-example.com", + path: "/project-redirects/spa/client/side/route", + expectedBody: "This is an SPA", + }, + "existing_content_takes_precedence_over_rewrite_rules": { + host: "group.redirects.gitlab-example.com", + path: "/project-redirects/spa/existing-file.html", + expectedBody: "This is an existing file", + }, + "rewrite_using_placeholders": { + host: "group.redirects.gitlab-example.com", + path: "/project-redirects/blog/2021/08/12", + expectedBody: "Rewrites are pretty neat!", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + rsp, err := GetPageFromListener(t, httpListener, tt.host, tt.path) + require.NoError(t, err) + defer rsp.Body.Close() + + body, err := ioutil.ReadAll(rsp.Body) + require.NoError(t, err) + + require.Contains(t, string(body), tt.expectedBody) + require.Equal(t, http.StatusOK, rsp.StatusCode) + }) + } +} |