Merge branch 'nfriend-limit-redirects-rule-count' into 'master'

feat: Add _redirects max rule count validation

See merge request gitlab-org/gitlab-pages!555

6 files changed, 79 insertions, 33 deletions

diff --git a/internal/redirects/matching.go b/internal/redirects/matching.go
index 913cb359..b5bceb71 100644
--- a/internal/redirects/matching.go
+++ b/internal/redirects/matching.go
@@ -101,6 +101,11 @@ func matchesRule(rule *netlifyRedirects.Rule, path string) (bool, string) {
 // 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 {
+		if i >= maxRuleCount {
+			// do not process any more rules
+			return nil, ""
+		}
+
 		// assign rule to a new var to prevent the following gosec error
 		// G601: Implicit memory aliasing in for loop
 		rule := r.rules[i]
diff --git a/internal/redirects/redirects.go b/internal/redirects/redirects.go
index 79fa8dd6..24ce8692 100644
--- a/internal/redirects/redirects.go
+++ b/internal/redirects/redirects.go
@@ -30,6 +30,9 @@ const (
 	// maxPathSegments is used to limit the number of path segments allowed in rules URLs
 	maxPathSegments = 25
 
+	// maxRuleCount is used to limit the total number of rules allowed in _redirects
+	maxRuleCount = 1000
+
 	// FFEnablePlaceholders used to check whether placeholder matching is enabled or not
 	FFEnablePlaceholders = "FF_ENABLE_PLACEHOLDERS"
 )
@@ -51,7 +54,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")
+	errTooManyPathSegments             = fmt.Errorf("url path cannot contain more than %d forward slashes", maxPathSegments)
 	regexpPlaceholder                  = regexp.MustCompile(`(?i)/:[a-z]+`)
 )
 
@@ -70,6 +73,20 @@ func (r *Redirects) Status() string {
 	messages = append(messages, fmt.Sprintf("%d rules", len(r.rules)))
 
 	for i, rule := range r.rules {
+		if i >= maxRuleCount {
+			messages = append([]string{
+				fmt.Sprintf(
+					"The _redirects file contains (%d) rules, more than the maximum of %d rules. Only the first %d rules will be processed.",
+					len(r.rules),
+					maxRuleCount,
+					maxRuleCount,
+				)},
+				messages...,
+			)
+
+			break
+		}
+
 		if err := validateRule(rule); err != nil {
 			messages = append(messages, fmt.Sprintf("rule %d: error: %s", i+1, err.Error()))
 		} else {
diff --git a/internal/redirects/redirects_benchmark_test.go b/internal/redirects/redirects_benchmark_test.go
index 38adcb68..d1c16a1c 100644
--- a/internal/redirects/redirects_benchmark_test.go
+++ b/internal/redirects/redirects_benchmark_test.go
@@ -2,8 +2,8 @@ package redirects
 
 import (
 	"context"
-	"io/ioutil"
 	"net/url"
+	"os"
 	"path"
 	"strings"
 	"testing"
@@ -24,14 +24,13 @@ func generateRedirectsFile(dirPath string, count int) error {
 
 	content += "/entrance.html /exit.html 301\n"
 
-	return ioutil.WriteFile(path.Join(dirPath, ConfigFile), []byte(content), 0600)
+	return os.WriteFile(path.Join(dirPath, ConfigFile), []byte(content), 0600)
 }
 
 func benchmarkRedirectsRewrite(b *testing.B, redirectsCount int) {
 	ctx := context.Background()
 
-	root, tmpDir, cleanup := testhelpers.TmpDir(nil, "ParseRedirects_benchmarks")
-	defer cleanup()
+	root, tmpDir := testhelpers.TmpDir(b, "ParseRedirects_benchmarks")
 
 	err := generateRedirectsFile(tmpDir, redirectsCount)
 	require.NoError(b, err)
@@ -51,7 +50,7 @@ func benchmarkRedirectsRewrite(b *testing.B, redirectsCount int) {
 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) })
+	b.Run("1000 redirects", func(b *testing.B) { benchmarkRedirectsRewrite(b, 998) })
 }
 
 func BenchmarkRedirectsRewrite_PlaceholdersEnabled(b *testing.B) {
@@ -59,14 +58,13 @@ func BenchmarkRedirectsRewrite_PlaceholdersEnabled(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) })
+	b.Run("1000 redirects", func(b *testing.B) { benchmarkRedirectsRewrite(b, 998) })
 }
 
 func benchmarkRedirectsParseRedirects(b *testing.B, redirectsCount int) {
 	ctx := context.Background()
 
-	root, tmpDir, cleanup := testhelpers.TmpDir(nil, "ParseRedirects_benchmarks")
-	defer cleanup()
+	root, tmpDir := testhelpers.TmpDir(b, "ParseRedirects_benchmarks")
 
 	err := generateRedirectsFile(tmpDir, redirectsCount)
 	require.NoError(b, err)
@@ -80,5 +78,5 @@ func benchmarkRedirectsParseRedirects(b *testing.B, redirectsCount int) {
 func BenchmarkRedirectsParseRedirects(b *testing.B) {
 	b.Run("10 redirects", func(b *testing.B) { benchmarkRedirectsParseRedirects(b, 10) })
 	b.Run("100 redirects", func(b *testing.B) { benchmarkRedirectsParseRedirects(b, 100) })
-	b.Run("1000 redirects", func(b *testing.B) { benchmarkRedirectsParseRedirects(b, 1000) })
+	b.Run("1000 redirects", func(b *testing.B) { benchmarkRedirectsParseRedirects(b, 998) })
 }
diff --git a/internal/redirects/redirects_test.go b/internal/redirects/redirects_test.go
index 6ae141f5..118b99a5 100644
--- a/internal/redirects/redirects_test.go
+++ b/internal/redirects/redirects_test.go
@@ -2,7 +2,6 @@ package redirects
 
 import (
 	"context"
-	"io/ioutil"
 	"net/url"
 	"os"
 	"path"
@@ -160,8 +159,7 @@ func TestRedirectsRewrite(t *testing.T) {
 func TestRedirectsParseRedirects(t *testing.T) {
 	ctx := context.Background()
 
-	root, tmpDir, cleanup := testhelpers.TmpDir(t, "ParseRedirects_tests")
-	defer cleanup()
+	root, tmpDir := testhelpers.TmpDir(t, "ParseRedirects_tests")
 
 	tests := []struct {
 		name          string
@@ -206,7 +204,7 @@ func TestRedirectsParseRedirects(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			if tt.redirectsFile != "" {
-				err := ioutil.WriteFile(path.Join(tmpDir, ConfigFile), []byte(tt.redirectsFile), 0600)
+				err := os.WriteFile(path.Join(tmpDir, ConfigFile), []byte(tt.redirectsFile), 0600)
 				require.NoError(t, err)
 			}
 
@@ -222,3 +220,36 @@ func TestRedirectsParseRedirects(t *testing.T) {
 		})
 	}
 }
+
+func TestMaxRuleCount(t *testing.T) {
+	root, tmpDir := testhelpers.TmpDir(t, "TooManyRules_tests")
+
+	err := os.WriteFile(path.Join(tmpDir, ConfigFile), []byte(strings.Repeat("/goto.html /target.html 301\n", maxRuleCount-1)+
+		"/1000.html /target1000 301\n"+
+		"/1001.html /target1001 301\n",
+	), 0600)
+	require.NoError(t, err)
+
+	redirects := ParseRedirects(context.Background(), root)
+
+	testFn := func(path, expectedToURL string, expectedStatus int, expectedErr string) func(t *testing.T) {
+		return func(t *testing.T) {
+			originalURL, err := url.Parse(path)
+			require.NoError(t, err)
+
+			toURL, status, err := redirects.Rewrite(originalURL)
+			if expectedErr != "" {
+				require.EqualError(t, err, expectedErr)
+				return
+			}
+
+			require.NoError(t, err)
+
+			require.Equal(t, expectedToURL, toURL.String())
+			require.Equal(t, expectedStatus, status)
+		}
+	}
+
+	t.Run("maxRuleCount matches", testFn("/1000.html", "/target1000", 301, ""))
+	t.Run("maxRuleCount+1 does not match", testFn("/1001.html", "", 0, ErrNoRedirect.Error()))
+}
diff --git a/internal/serving/disk/symlink/path_test.go b/internal/serving/disk/symlink/path_test.go
index 66b4fceb..c9da51c3 100644
--- a/internal/serving/disk/symlink/path_test.go
+++ b/internal/serving/disk/symlink/path_test.go
@@ -85,8 +85,7 @@ func testEvalSymlinks(t *testing.T, wd, path, want string) {
 }
 
 func TestEvalSymlinks(t *testing.T) {
-	_, tmpDir, cleanup := testhelpers.TmpDir(t, "symlink_tests")
-	defer cleanup()
+	_, tmpDir := testhelpers.TmpDir(t, "symlink_tests")
 
 	// Create the symlink farm using relative paths.
 	for _, d := range EvalSymlinksTestDirs {
@@ -124,8 +123,7 @@ func TestEvalSymlinks(t *testing.T) {
 }
 
 func TestEvalSymlinksIsNotExist(t *testing.T) {
-	root, _, cleanup := testhelpers.TmpDir(t, "symlink_tests")
-	defer cleanup()
+	root, _ := testhelpers.TmpDir(t, "symlink_tests")
 
 	_, err := symlink.EvalSymlinks(context.Background(), root, "notexist")
 	if !os.IsNotExist(err) {
@@ -145,8 +143,7 @@ func TestEvalSymlinksIsNotExist(t *testing.T) {
 }
 
 func TestIssue13582(t *testing.T) {
-	root, tmpDir, cleanup := testhelpers.TmpDir(t, "symlink_tests")
-	defer cleanup()
+	root, tmpDir := testhelpers.TmpDir(t, "symlink_tests")
 
 	dir := filepath.Join(tmpDir, "dir")
 	err := os.Mkdir(dir, 0755)
diff --git a/internal/testhelpers/tmpdir.go b/internal/testhelpers/tmpdir.go
index 81f0781b..5765d308 100644
--- a/internal/testhelpers/tmpdir.go
+++ b/internal/testhelpers/tmpdir.go
@@ -15,24 +15,22 @@ import (
 
 var fs = vfs.Instrumented(&local.VFS{})
 
-func TmpDir(t *testing.T, pattern string) (vfs.Root, string, func()) {
+func TmpDir(tb testing.TB, pattern string) (vfs.Root, string) {
+	tb.Helper()
+
 	tmpDir, err := ioutil.TempDir("", pattern)
-	if t != nil {
-		require.NoError(t, err)
-	}
+	require.NoError(tb, err)
 
 	// On some systems `/tmp` can be a symlink
 	tmpDir, err = filepath.EvalSymlinks(tmpDir)
-	if t != nil {
-		require.NoError(t, err)
-	}
+	require.NoError(tb, err)
 
 	root, err := fs.Root(context.Background(), tmpDir)
-	if t != nil {
-		require.NoError(t, err)
-	}
+	require.NoError(tb, err)
+
+	tb.Cleanup(func() {
+		require.NoError(tb, os.RemoveAll(tmpDir))
+	})
 
-	return root, tmpDir, func() {
-		os.RemoveAll(tmpDir)
-	}
+	return root, tmpDir
 }