diff --git a/internal/feature/feature.go b/internal/feature/feature.go index 5efc79ec9578f8ad71b2e649b3d237ebabbdd9f5..4a14152ae4d84065e0281454f40376f831af6e4f 100644 --- a/internal/feature/feature.go +++ b/internal/feature/feature.go @@ -37,9 +37,3 @@ var ProjectPrefixCookiePath = Feature{ EnvVariable: "FF_ENABLE_PROJECT_PREFIX_COOKIE_PATH", defaultEnabled: true, } - -// DomainRedirects enables support for domain level redirects -var DomainRedirects = Feature{ - EnvVariable: "FF_ENABLE_DOMAIN_REDIRECT", - defaultEnabled: true, -} diff --git a/internal/redirects/matching.go b/internal/redirects/matching.go index ff5f0cc239d26e4a0bfa058b1d6eae16dc19cb25..6e291292038f1c5447e32031281f043965981bdf 100644 --- a/internal/redirects/matching.go +++ b/internal/redirects/matching.go @@ -42,10 +42,6 @@ func matchesRule(rule *netlifyRedirects.Rule, originalURL *url.URL) (bool, strin path := originalURL.Path - if !feature.DomainRedirects.Enabled() && utils.IsDomainURL(rule.To) { - return false, "" - } - // 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. diff --git a/internal/redirects/matching_test.go b/internal/redirects/matching_test.go index 3796daff71bfb8a795189f85d719db1e13cd57b6..6661100a08f7a16272e4ebb492474d7552329832 100644 --- a/internal/redirects/matching_test.go +++ b/internal/redirects/matching_test.go @@ -57,7 +57,7 @@ var testsWithoutPlaceholders = map[string]testCaseData{ }, } -func testMatchesRule(t *testing.T) { +func TestMatchesRule(t *testing.T) { t.Setenv(feature.RedirectsPlaceholders.EnvVariable, "true") tests := mergeTestSuites(testsWithoutPlaceholders, map[string]testCaseData{ @@ -320,16 +320,6 @@ func testMatchesRule(t *testing.T) { } } -func Test_matchesRule(t *testing.T) { - t.Setenv(feature.DomainRedirects.EnvVariable, "false") - testMatchesRule(t) -} - -func Test_matchesNonDomainRule_DomainRedirects_enabled(t *testing.T) { - t.Setenv(feature.DomainRedirects.EnvVariable, "true") - testMatchesRule(t) -} - // 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. @@ -379,7 +369,6 @@ func Test_matchesRule_NoPlaceholders(t *testing.T) { // feature flag is enabled. func Test_matchesDomainRule(t *testing.T) { t.Setenv(feature.RedirectsPlaceholders.EnvVariable, "true") - t.Setenv(feature.DomainRedirects.EnvVariable, "true") tests := map[string]testCaseData{ "exact_path_match": { @@ -658,34 +647,3 @@ func Test_matchesDomainRule(t *testing.T) { }) } } - -// Tests matching behavior when the `FF_ENABLE_DOMAIN_REDIRECT` -// feature flag is not enabled. These tests can be removed when the -// `FF_ENABLE_DOMAIN_REDIRECT` flag is removed. -func Test_matchesDomainRule_DomainRedirect_Disabled(t *testing.T) { - t.Setenv(feature.RedirectsPlaceholders.EnvVariable, "true") - t.Setenv(feature.DomainRedirects.EnvVariable, "false") - - tests := map[string]testCaseData{ - "exact_match": { - rule: "http://pages.example.io/foo/ http://test.example.io/bar/", - path: "http://pages.example.io/foo/", - 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) - - parsedURL, err := url.Parse(tt.path) - require.NoError(t, err) - - isMatch, path := matchesRule(&rules[0], parsedURL) - 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 2c01f396a2bc5d97c3bac0ada28266f1b2955c5c..1862c626ebce3606585db8b1dee13b8284343779 100644 --- a/internal/redirects/redirects.go +++ b/internal/redirects/redirects.go @@ -44,24 +44,22 @@ var ( // ErrNoRedirect is the error thrown when a no redirect rule matches while trying to Rewrite URL. // This means that no redirect applies to the URL and you can fallback to serving actual content instead. - ErrNoRedirect = errors.New("no redirect found") - errConfigNotFound = errors.New("_redirects file not found") - errNeedRegularFile = errors.New("_redirects needs to be a regular file (not a directory)") - errFileTooLarge = errors.New("_redirects file too large") - errFailedToOpenConfig = errors.New("unable to open _redirects file") - errFailedToParseConfig = errors.New("failed to parse _redirects file") - errFailedToParseURL = errors.New("unable to parse URL") - errNoDomainLevelRedirects = errors.New("no domain-level redirects to outside sites") - errNoDomainLevelRewrite = errors.New("no domain-level rewrite to outside sites") - errNoStartingForwardSlashInURLPath = errors.New("url path must start with forward slash /") - errNoValidStartingInURLPath = errors.New("url path must start with forward slash / or http:// or https://") - errNoSplats = errors.New("splats are not enabled. See https://docs.gitlab.com/ee/user/project/pages/redirects.html#feature-flag-for-rewrites") - errMoreThanOneSplats = errors.New("rule cannot contain more than 1 asterisk (*) in its from path") - errNoPlaceholders = errors.New("placeholders are not enabled. See https://docs.gitlab.com/ee/user/project/pages/redirects.html#feature-flag-for-rewrites") - errNoParams = errors.New("params not supported") - errUnsupportedStatus = errors.New("status not supported") - errNoForce = errors.New("force! not supported") - regexpPlaceholder = regexp.MustCompile(`(?i)/:[a-z]+`) + ErrNoRedirect = errors.New("no redirect found") + errConfigNotFound = errors.New("_redirects file not found") + errNeedRegularFile = errors.New("_redirects needs to be a regular file (not a directory)") + errFileTooLarge = errors.New("_redirects file too large") + errFailedToOpenConfig = errors.New("unable to open _redirects file") + errFailedToParseConfig = errors.New("failed to parse _redirects file") + errFailedToParseURL = errors.New("unable to parse URL") + errNoDomainLevelRewrite = errors.New("no domain-level rewrite to outside sites") + errNoValidStartingInURLPath = errors.New("url path must start with forward slash / or http:// or https://") + errNoSplats = errors.New("splats are not enabled. See https://docs.gitlab.com/ee/user/project/pages/redirects.html#feature-flag-for-rewrites") + errMoreThanOneSplats = errors.New("rule cannot contain more than 1 asterisk (*) in its from path") + errNoPlaceholders = errors.New("placeholders are not enabled. See https://docs.gitlab.com/ee/user/project/pages/redirects.html#feature-flag-for-rewrites") + errNoParams = errors.New("params not supported") + errUnsupportedStatus = errors.New("status not supported") + errNoForce = errors.New("force! not supported") + regexpPlaceholder = regexp.MustCompile(`(?i)/:[a-z]+`) ) func SetConfig(redirectsConfig config.Redirects) { diff --git a/internal/redirects/validations.go b/internal/redirects/validations.go index 7a608f3c0dc3d640866870f994a67a7705e03c0a..410ced279d12ce83a6d3e7a7f80ad62757267106 100644 --- a/internal/redirects/validations.go +++ b/internal/redirects/validations.go @@ -58,49 +58,29 @@ func validateFromURL(urlText string) error { // validateURL runs validations against a rule URL. // Returns `nil` if the URL is valid. -// nolint: gocyclo func validateToURL(urlText string, status int) error { toURL, err := url.Parse(urlText) if err != nil { return errFailedToParseURL } - allowedPrefix := []string{"/"} - if feature.DomainRedirects.Enabled() { - // No support for domain level redirects starting with // or special characters: - // - `//google.com` - // - `/\google.com` - if (toURL.Host == "") != (toURL.Scheme == "") || strings.HasPrefix(toURL.Path, "/\\") { - return errNoValidStartingInURLPath - } - - // No support for domain level rewrite - if utils.IsDomainURL(urlText) { - if status == http.StatusOK { - return errNoDomainLevelRewrite - } - allowedPrefix = append(allowedPrefix, "http://", "https://") - } + // No support for domain level redirects starting with // or special characters: + // - `//google.com` + // - `/\google.com` + if (toURL.Host == "") != (toURL.Scheme == "") || strings.HasPrefix(toURL.Path, "/\\") { + return errNoValidStartingInURLPath + } - // No parent traversing relative URL's with `./` or `../` - // No ambiguous URLs like bare domains `GitLab.com` - if !startsWithAnyPrefix(urlText, allowedPrefix...) { - return errNoValidStartingInURLPath - } - } else { - // No support for domain-level redirects to outside sites: - // - `https://google.com` - // - `//google.com` - // - `/\google.com` - if toURL.Host != "" || toURL.Scheme != "" || strings.HasPrefix(toURL.Path, "/\\") { - return errNoDomainLevelRedirects - } + // No support for domain level rewrite + if utils.IsDomainURL(urlText) && status == http.StatusOK { + return errNoDomainLevelRewrite + } - // No parent traversing relative URL's with `./` or `../` - // No ambiguous URLs like bare domains `GitLab.com` - if !startsWithAnyPrefix(urlText, allowedPrefix...) { - return errNoStartingForwardSlashInURLPath - } + allowedPrefix := []string{"/", "http://", "https://"} + // No parent traversing relative URL's with `./` or `../` + // No ambiguous URLs like bare domains `GitLab.com` + if !startsWithAnyPrefix(urlText, allowedPrefix...) { + return errNoValidStartingInURLPath } return validateSplatAndPlaceholders(toURL.Path) diff --git a/internal/redirects/validations_test.go b/internal/redirects/validations_test.go index a295fb4387ee50c77ac255181d74504a027f81b2..3e2062e445a10f3d373eb42ca80734efb66864f5 100644 --- a/internal/redirects/validations_test.go +++ b/internal/redirects/validations_test.go @@ -12,126 +12,6 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/feature" ) -func TestRedirectsValidateFromUrl(t *testing.T) { - t.Setenv(feature.RedirectsPlaceholders.EnvVariable, "true") - - tests := map[string]struct { - url string - expectedErr error - }{ - "valid_url": { - url: "/goto.html", - }, - "no_domain_level_redirects": { - url: "https://GitLab.com", - }, - "no_special_characters_escape_domain_level_redirects": { - url: "/\\GitLab.com", - expectedErr: errNoValidStartingInURLPath, - }, - "no_schemaless_url_domain_level_redirects": { - url: "//GitLab.com/pages.html", - expectedErr: errNoValidStartingInURLPath, - }, - "no_bare_domain_level_redirects": { - url: "GitLab.com", - expectedErr: errNoValidStartingInURLPath, - }, - "no_parent_traversing_relative_url": { - url: "../target.html", - expectedErr: errNoValidStartingInURLPath, - }, - "too_many_slashes": { - url: strings.Repeat("/a", 26), - expectedErr: fmt.Errorf("url path cannot contain more than %d forward slashes", defaultMaxPathSegments), - }, - "placeholders": { - url: "/news/:year/:month/:date/:slug", - }, - "splats": { - url: "/blog/*", - }, - "no_multiple_splats_redirects": { - url: "/foo/*/bar/*/baz", - expectedErr: errMoreThanOneSplats, - }, - "splat_placeholders": { - url: "/new/path/:splat", - }, - } - - for name, tt := range tests { - t.Run(name, func(t *testing.T) { - err := validateFromURL(tt.url) - if tt.expectedErr != nil { - require.EqualError(t, err, tt.expectedErr.Error()) - return - } - - require.NoError(t, err) - }) - } -} - -func TestRedirectsValidateToUrl(t *testing.T) { - t.Setenv(feature.RedirectsPlaceholders.EnvVariable, "true") - t.Setenv(feature.DomainRedirects.EnvVariable, "false") - - tests := map[string]struct { - url string - expectedErr error - }{ - "valid_url": { - url: "/goto.html", - }, - "no_domain_level_redirects": { - url: "https://GitLab.com", - expectedErr: errNoDomainLevelRedirects, - }, - "no_special_characters_escape_domain_level_redirects": { - url: "/\\GitLab.com", - expectedErr: errNoDomainLevelRedirects, - }, - "no_schemaless_url_domain_level_redirects": { - url: "//GitLab.com/pages.html", - expectedErr: errNoDomainLevelRedirects, - }, - "no_bare_domain_level_redirects": { - url: "GitLab.com", - expectedErr: errNoStartingForwardSlashInURLPath, - }, - "no_parent_traversing_relative_url": { - url: "../target.html", - expectedErr: errNoStartingForwardSlashInURLPath, - }, - "too_many_slashes": { - url: strings.Repeat("/a", 26), - expectedErr: fmt.Errorf("url path cannot contain more than %d forward slashes", defaultMaxPathSegments), - }, - "placeholders": { - url: "/news/:year/:month/:date/:slug", - }, - "splats": { - url: "/blog/*", - }, - "splat_placeholders": { - url: "/new/path/:splat", - }, - } - - for name, tt := range tests { - t.Run(name, func(t *testing.T) { - err := validateToURL(tt.url, http.StatusMovedPermanently) - if tt.expectedErr != nil { - require.EqualError(t, err, tt.expectedErr.Error()) - 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. @@ -163,7 +43,6 @@ func TestRedirectsValidateUrlNoPlaceholders(t *testing.T) { func TestRedirectsValidateRule(t *testing.T) { t.Setenv(feature.RedirectsPlaceholders.EnvVariable, "true") - t.Setenv(feature.DomainRedirects.EnvVariable, "false") tests := map[string]struct { rule string @@ -175,13 +54,16 @@ func TestRedirectsValidateRule(t *testing.T) { "valid_from_host_url": { rule: "http://valid.com/ /teapot.html 302", }, + "valid_to_url": { + rule: "/goto.html https://example.com/target.html 301", + }, "invalid_from_url": { rule: "invalid.com /teapot.html 302", expectedErr: errNoValidStartingInURLPath, }, "invalid_to_url": { rule: "/goto.html invalid.com", - expectedErr: errNoStartingForwardSlashInURLPath, + expectedErr: errNoValidStartingInURLPath, }, "no_parameters": { rule: "/ /something 302 foo=bar", @@ -208,9 +90,8 @@ func TestRedirectsValidateRule(t *testing.T) { } } -func TestRedirectsValidateFromUrl_DomainRedirect_Enabled(t *testing.T) { +func TestRedirectsValidateFromUrl(t *testing.T) { t.Setenv(feature.RedirectsPlaceholders.EnvVariable, "true") - t.Setenv(feature.DomainRedirects.EnvVariable, "true") tests := map[string]struct { url string @@ -278,102 +159,8 @@ func TestRedirectsValidateFromUrl_DomainRedirect_Enabled(t *testing.T) { } } -func TestRedirectsValidateToUrl_DomainRedirect_Enabled(t *testing.T) { - t.Setenv(feature.RedirectsPlaceholders.EnvVariable, "true") - t.Setenv(feature.DomainRedirects.EnvVariable, "true") - - tests := map[string]struct { - url string - status int - expectedErr error - }{ - "valid_url": { - url: "/goto.html", - status: http.StatusOK, - }, - "domain_level_redirects": { - url: "https://GitLab.com", - status: http.StatusMovedPermanently, - }, - "domain_level_rewrite": { - url: "https://GitLab.com", - status: http.StatusOK, - expectedErr: errNoDomainLevelRewrite, - }, - "no_special_characters_escape_domain_level_redirects": { - url: "/\\GitLab.com", - status: http.StatusMovedPermanently, - expectedErr: errNoValidStartingInURLPath, - }, - "no_schemaless_url_domain_level_redirects": { - url: "//GitLab.com/pages.html", - status: http.StatusMovedPermanently, - expectedErr: errNoValidStartingInURLPath, - }, - "no_bare_domain_level_redirects": { - url: "GitLab.com", - status: http.StatusMovedPermanently, - expectedErr: errNoValidStartingInURLPath, - }, - "no_parent_traversing_relative_url": { - url: "../target.html", - status: http.StatusMovedPermanently, - expectedErr: errNoValidStartingInURLPath, - }, - "too_many_slashes": { - url: strings.Repeat("/a", 26), - status: http.StatusMovedPermanently, - expectedErr: fmt.Errorf("url path cannot contain more than %d forward slashes", defaultMaxPathSegments), - }, - "placeholders": { - url: "/news/:year/:month/:date/:slug", - status: http.StatusMovedPermanently, - }, - "domain_redirect_placeholders": { - url: "https://GitLab.com/news/:year/:month/:date/:slug", - status: http.StatusMovedPermanently, - }, - "splats": { - url: "/blog/*", - status: http.StatusMovedPermanently, - }, - "lone_splats": { - url: "https://GitLab.com/*", - status: http.StatusMovedPermanently, - }, - "domain_redirect_splats": { - url: "https://GitLab.com/blog/*", - status: http.StatusMovedPermanently, - }, - "splat_placeholders": { - url: "/new/path/:splat", - status: http.StatusMovedPermanently, - }, - "domain_redirect_splat_placeholders": { - url: "https://GitLab.com/new/path/:splat", - status: http.StatusMovedPermanently, - }, - } - - for name, tt := range tests { - t.Run(name, func(t *testing.T) { - err := validateToURL(tt.url, tt.status) - if tt.expectedErr != nil { - require.EqualError(t, err, tt.expectedErr.Error()) - return - } - - require.NoError(t, err) - }) - } -} - -// Tests validation rules that only apply when the `FF_ENABLE_DOMAIN_REDIRECT` -// feature flag is not enabled. These tests can be removed when the -// `FF_ENABLE_DOMAIN_REDIRECT` flag is removed. -func TestRedirectsValidateToUrl_DomainRedirect_Disabled(t *testing.T) { +func TestRedirectsValidateToUrl(t *testing.T) { t.Setenv(feature.RedirectsPlaceholders.EnvVariable, "true") - t.Setenv(feature.DomainRedirects.EnvVariable, "true") tests := map[string]struct { url string diff --git a/test/acceptance/redirects_test.go b/test/acceptance/redirects_test.go index a4243dc9d46e6f03644c0096b1a74c39b6e2c07d..426964ce54feb7d3580c2f96373ab29ee0c6c594 100644 --- a/test/acceptance/redirects_test.go +++ b/test/acceptance/redirects_test.go @@ -14,7 +14,6 @@ import ( func runRedirectStatusTests(t *testing.T, listener ListenSpec, host, path string, extraArgs ...processOption) { t.Setenv(feature.RedirectsPlaceholders.EnvVariable, "true") - t.Setenv(feature.DomainRedirects.EnvVariable, "true") RunPagesProcess(t, append(extraArgs, withListeners([]ListenSpec{httpListener}))..., @@ -70,135 +69,7 @@ func testRedirect(t *testing.T, tests []redirect, extraArgs ...processOption) { } } -func TestRedirect_DomainRedirects_Disabled(t *testing.T) { - t.Setenv(feature.DomainRedirects.EnvVariable, "false") - tests := []redirect{ - // Serving a file works - { - host: "group.redirects.gitlab-example.com", - path: "/project-redirects/index.html", - expectedStatus: http.StatusOK, - expectedLocation: "", - }, - // Project domain - { - host: "group.redirects.gitlab-example.com", - path: "/project-redirects/redirect-portal.html", - expectedStatus: http.StatusFound, - expectedLocation: "http://group.redirects.gitlab-example.com/project-redirects/magic-land.html", - }, - // Make sure invalid rule does not redirect - { - host: "group.redirects.gitlab-example.com", - path: "/project-redirects/goto-no-schema-domain.html", - expectedStatus: http.StatusNotFound, - expectedLocation: "", - }, - // Actual file on disk should override any redirects that match - { - host: "group.redirects.gitlab-example.com", - path: "/project-redirects/file-override.html", - expectedStatus: http.StatusOK, - expectedLocation: "", - }, - // Group-level domain - { - host: "group.redirects.gitlab-example.com", - path: "/redirect-portal.html", - expectedStatus: http.StatusFound, - expectedLocation: "http://group.redirects.gitlab-example.com/magic-land.html", - }, - // Custom domain - { - host: "redirects.custom-domain.com", - path: "/redirect-portal.html", - expectedStatus: http.StatusFound, - expectedLocation: "http://redirects.custom-domain.com/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: "http://group.redirects.gitlab-example.com/project-redirects/careers/assistant-to-the-regional-manager.html", - }, - // Domain redirect - { - host: "group.redirects.gitlab-example.com", - path: "/project-redirects/goto-domain.html", - expectedStatus: http.StatusNotFound, - expectedLocation: "", - }, - } - testRedirect(t, tests) -} - -func TestNamespaceInPathEnabled_Redirect_DomainRedirects_Disabled(t *testing.T) { - t.Setenv(feature.DomainRedirects.EnvVariable, "false") - tests := []redirect{ - // Serving a file works - { - host: "gitlab-example.com", - path: "/group.redirects/project-redirects/index.html", - expectedStatus: http.StatusOK, - expectedLocation: "", - }, - // Project domain - { - host: "gitlab-example.com", - path: "/group.redirects/project-redirects/redirect-portal.html", - expectedStatus: http.StatusFound, - expectedLocation: "http://gitlab-example.com/group.redirects/project-redirects/magic-land.html", - }, - // Make sure invalid rule does not redirect - { - host: "gitlab-example.com", - path: "/group.redirects/project-redirects/goto-no-schema-domain.html", - expectedStatus: http.StatusNotFound, - expectedLocation: "", - }, - // Actual file on disk should override any redirects that match - { - host: "gitlab-example.com", - path: "/group.redirects/project-redirects/file-override.html", - expectedStatus: http.StatusOK, - expectedLocation: "", - }, - // Group-level domain - { - host: "gitlab-example.com", - path: "/group.redirects/redirect-portal.html", - expectedStatus: http.StatusFound, - expectedLocation: "http://gitlab-example.com/group.redirects/magic-land.html", - }, - // Custom domain - { - host: "redirects.custom-domain.com", - path: "/redirect-portal.html", - expectedStatus: http.StatusFound, - expectedLocation: "http://redirects.custom-domain.com/magic-land.html", - }, - // Permanent redirect for splat (*) with replacement (:splat) - { - host: "gitlab-example.com", - path: "/group.redirects/project-redirects/jobs/assistant-to-the-regional-manager.html", - expectedStatus: http.StatusMovedPermanently, - expectedLocation: "http://gitlab-example.com/group.redirects/project-redirects/careers/assistant-to-the-regional-manager.html", - }, - // Domain redirect - { - host: "gitlab-example.com", - path: "/group.redirects/project-redirects/goto-domain.html", - expectedStatus: http.StatusNotFound, - expectedLocation: "", - }, - } - testRedirect(t, tests, withExtraArgument("namespace-in-path", "true")) -} - -func TestRedirect_DomainRedirects_Enabled(t *testing.T) { - t.Setenv(feature.DomainRedirects.EnvVariable, "true") - +func TestRedirect(t *testing.T) { tests := []redirect{ // Serving a file works { @@ -260,9 +131,7 @@ func TestRedirect_DomainRedirects_Enabled(t *testing.T) { testRedirect(t, tests) } -func TestNamespaceInPathEnabled_Redirect_DomainRedirects_Enabled(t *testing.T) { - t.Setenv(feature.DomainRedirects.EnvVariable, "true") - +func TestNamespaceInPathEnabled_Redirect(t *testing.T) { tests := []redirect{ // Serving a file works {