Merge branch 'nfriend-restructure-validations-and-helpers' into 'master'

Break redirect code into helpers and validations

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

6 files changed, 252 insertions, 186 deletions

diff --git a/internal/redirects/helpers.go b/internal/redirects/helpers.go
new file mode 100644
index 00000000..4223faae
--- /dev/null
+++ b/internal/redirects/helpers.go
@@ -0,0 +1,11 @@
+package redirects
+
+import (
+	"strings"
+)
+
+// normalizePath ensures that the provided path ends with at least one trailing slash.
+// Multiple trailing slashs are not trimmed.
+func normalizePath(path string) string {
+	return strings.TrimSuffix(path, "/") + "/"
+}
diff --git a/internal/redirects/helpers_test.go b/internal/redirects/helpers_test.go
new file mode 100644
index 00000000..b07763ef
--- /dev/null
+++ b/internal/redirects/helpers_test.go
@@ -0,0 +1,38 @@
+package redirects
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func Test_normalizePath(t *testing.T) {
+	tests := []struct {
+		name     string
+		path     string
+		expected string
+	}{
+		{
+			name:     "add_trailing_slash",
+			path:     "foo",
+			expected: "foo/",
+		},
+		{
+			name:     "leave_existing_trailing_slash",
+			path:     "foo/",
+			expected: "foo/",
+		},
+		{
+			name:     "leave_existing_double_trailing_slash",
+			path:     "foo//",
+			expected: "foo//",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := normalizePath(tt.path)
+			require.Equal(t, tt.expected, got)
+		})
+	}
+}
diff --git a/internal/redirects/redirects.go b/internal/redirects/redirects.go
index 48dc2c05..8c30f747 100644
--- a/internal/redirects/redirects.go
+++ b/internal/redirects/redirects.go
@@ -6,7 +6,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"net/http"
 	"net/url"
 	"regexp"
 	"strings"
@@ -74,83 +73,6 @@ func (r *Redirects) Status() string {
 	return strings.Join(messages, "\n")
 }
 
-func validateURL(urlText string) error {
-	url, err := url.Parse(urlText)
-	if err != nil {
-		return errFailedToParseURL
-	}
-
-	// No support for domain-level redirects to outside sites:
-	// - `https://google.com`
-	// - `//google.com`
-	if url.Host != "" || url.Scheme != "" {
-		return errNoDomainLevelRedirects
-	}
-
-	// No parent traversing relative URL's with `./` or `../`
-	// No ambiguous URLs like bare domains `GitLab.com`
-	if !strings.HasPrefix(url.Path, "/") {
-		return errNoStartingForwardSlashInURLPath
-	}
-
-	// 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
-	}
-
-	return nil
-}
-
-func validateRule(r netlifyRedirects.Rule) error {
-	if err := validateURL(r.From); err != nil {
-		return err
-	}
-
-	if err := validateURL(r.To); err != nil {
-		return err
-	}
-
-	// No support for query parameters, https://docs.netlify.com/routing/redirects/redirect-options/#query-parameters
-	if r.Params != nil {
-		return errNoParams
-	}
-
-	// We strictly validate return status codes
-	switch r.Status {
-	case http.StatusMovedPermanently, http.StatusFound:
-		// noop
-	default:
-		return errUnsupportedStatus
-	}
-
-	// No support for rules that use ! force
-	if r.Force {
-		return errNoForce
-	}
-
-	return nil
-}
-
-func normalizePath(path string) string {
-	return strings.TrimSuffix(path, "/") + "/"
-}
-
-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
-}
-
 // 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) {
diff --git a/internal/redirects/redirects_test.go b/internal/redirects/redirects_test.go
index f538fc4b..3e8f5236 100644
--- a/internal/redirects/redirects_test.go
+++ b/internal/redirects/redirects_test.go
@@ -14,114 +14,6 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers"
 )
 
-func TestRedirectsValidateUrl(t *testing.T) {
-	tests := []struct {
-		name        string
-		url         string
-		expectedErr string
-	}{
-		{
-			name:        "Valid url",
-			url:         "/goto.html",
-			expectedErr: "",
-		},
-		{
-			name:        "No domain-level redirects",
-			url:         "https://GitLab.com",
-			expectedErr: errNoDomainLevelRedirects.Error(),
-		},
-		{
-			name:        "No Schema-less URL domain-level redirects",
-			url:         "//GitLab.com/pages.html",
-			expectedErr: errNoDomainLevelRedirects.Error(),
-		},
-		{
-			name:        "No bare domain-level redirects",
-			url:         "GitLab.com",
-			expectedErr: errNoStartingForwardSlashInURLPath.Error(),
-		},
-		{
-			name:        "No parent traversing relative URL",
-			url:         "../target.html",
-			expectedErr: errNoStartingForwardSlashInURLPath.Error(),
-		},
-		{
-			name:        "No splats",
-			url:         "/blog/*",
-			expectedErr: errNoSplats.Error(),
-		},
-		{
-			name:        "No Placeholders",
-			url:         "/news/:year/:month/:date/:slug",
-			expectedErr: errNoPlaceholders.Error(),
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			err := validateURL(tt.url)
-			if tt.expectedErr != "" {
-				require.EqualError(t, err, tt.expectedErr)
-			} else {
-				require.NoError(t, err)
-			}
-		})
-	}
-}
-
-func TestRedirectsValidateRule(t *testing.T) {
-	tests := []struct {
-		name        string
-		rule        string
-		expectedErr string
-	}{
-		{
-			name:        "valid rule",
-			rule:        "/goto.html /target.html 301",
-			expectedErr: "",
-		},
-		{
-			name:        "invalid From URL",
-			rule:        "invalid.com /teapot.html 302",
-			expectedErr: errNoStartingForwardSlashInURLPath.Error(),
-		},
-		{
-			name:        "invalid To URL",
-			rule:        "/goto.html invalid.com",
-			expectedErr: errNoStartingForwardSlashInURLPath.Error(),
-		},
-		{
-			name: "No parameters",
-			rule: "/	/something	302	foo=bar",
-			expectedErr: errNoParams.Error(),
-		},
-		{
-			name:        "Invalid status",
-			rule:        "/goto.html /target.html 418",
-			expectedErr: errUnsupportedStatus.Error(),
-		},
-		{
-			name:        "Force not supported",
-			rule:        "/goto.html /target.html 302!",
-			expectedErr: errNoForce.Error(),
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.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)
-			}
-		})
-	}
-}
-
 func TestRedirectsRewrite(t *testing.T) {
 	tests := []struct {
 		name           string
diff --git a/internal/redirects/validations.go b/internal/redirects/validations.go
new file mode 100644
index 00000000..64ae4b95
--- /dev/null
+++ b/internal/redirects/validations.go
@@ -0,0 +1,87 @@
+package redirects
+
+import (
+	"net/http"
+	"net/url"
+	"strings"
+
+	netlifyRedirects "github.com/tj/go-redirects"
+)
+
+// validateURL runs validations against a rule URL.
+// Returns `nil` if the URL is valid.
+func validateURL(urlText string) error {
+	url, err := url.Parse(urlText)
+	if err != nil {
+		return errFailedToParseURL
+	}
+
+	// No support for domain-level redirects to outside sites:
+	// - `https://google.com`
+	// - `//google.com`
+	if url.Host != "" || url.Scheme != "" {
+		return errNoDomainLevelRedirects
+	}
+
+	// No parent traversing relative URL's with `./` or `../`
+	// No ambiguous URLs like bare domains `GitLab.com`
+	if !strings.HasPrefix(url.Path, "/") {
+		return errNoStartingForwardSlashInURLPath
+	}
+
+	// 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
+	}
+
+	return nil
+}
+
+// validateRule runs all validation rules on the provided rule.
+// Returns `nil` if the rule is valid.
+func validateRule(r netlifyRedirects.Rule) error {
+	if err := validateURL(r.From); err != nil {
+		return err
+	}
+
+	if err := validateURL(r.To); err != nil {
+		return err
+	}
+
+	// No support for query parameters, https://docs.netlify.com/routing/redirects/redirect-options/#query-parameters
+	if r.Params != nil {
+		return errNoParams
+	}
+
+	// We strictly validate return status codes
+	switch r.Status {
+	case http.StatusMovedPermanently, http.StatusFound:
+		// noop
+	default:
+		return errUnsupportedStatus
+	}
+
+	// No support for rules that use ! force
+	if r.Force {
+		return errNoForce
+	}
+
+	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
new file mode 100644
index 00000000..011ae9e0
--- /dev/null
+++ b/internal/redirects/validations_test.go
@@ -0,0 +1,116 @@
+package redirects
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	netlifyRedirects "github.com/tj/go-redirects"
+)
+
+func TestRedirectsValidateUrl(t *testing.T) {
+	tests := []struct {
+		name        string
+		url         string
+		expectedErr string
+	}{
+		{
+			name:        "Valid url",
+			url:         "/goto.html",
+			expectedErr: "",
+		},
+		{
+			name:        "No domain-level redirects",
+			url:         "https://GitLab.com",
+			expectedErr: errNoDomainLevelRedirects.Error(),
+		},
+		{
+			name:        "No Schema-less URL domain-level redirects",
+			url:         "//GitLab.com/pages.html",
+			expectedErr: errNoDomainLevelRedirects.Error(),
+		},
+		{
+			name:        "No bare domain-level redirects",
+			url:         "GitLab.com",
+			expectedErr: errNoStartingForwardSlashInURLPath.Error(),
+		},
+		{
+			name:        "No parent traversing relative URL",
+			url:         "../target.html",
+			expectedErr: errNoStartingForwardSlashInURLPath.Error(),
+		},
+		{
+			name:        "No splats",
+			url:         "/blog/*",
+			expectedErr: errNoSplats.Error(),
+		},
+		{
+			name:        "No Placeholders",
+			url:         "/news/:year/:month/:date/:slug",
+			expectedErr: errNoPlaceholders.Error(),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateURL(tt.url)
+			if tt.expectedErr != "" {
+				require.EqualError(t, err, tt.expectedErr)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestRedirectsValidateRule(t *testing.T) {
+	tests := []struct {
+		name        string
+		rule        string
+		expectedErr string
+	}{
+		{
+			name:        "valid rule",
+			rule:        "/goto.html /target.html 301",
+			expectedErr: "",
+		},
+		{
+			name:        "invalid From URL",
+			rule:        "invalid.com /teapot.html 302",
+			expectedErr: errNoStartingForwardSlashInURLPath.Error(),
+		},
+		{
+			name:        "invalid To URL",
+			rule:        "/goto.html invalid.com",
+			expectedErr: errNoStartingForwardSlashInURLPath.Error(),
+		},
+		{
+			name: "No parameters",
+			rule: "/	/something	302	foo=bar",
+			expectedErr: errNoParams.Error(),
+		},
+		{
+			name:        "Invalid status",
+			rule:        "/goto.html /target.html 418",
+			expectedErr: errUnsupportedStatus.Error(),
+		},
+		{
+			name:        "Force not supported",
+			rule:        "/goto.html /target.html 302!",
+			expectedErr: errNoForce.Error(),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.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)
+			}
+		})
+	}
+}