Fixes acme redirection issues when using a wildcard redirect

Changelog: fixed

11 files changed, 83 insertions, 2 deletions

diff --git a/internal/acme/acme.go b/internal/acme/acme.go
index c83fcc08..ae808cdd 100644
--- a/internal/acme/acme.go
+++ b/internal/acme/acme.go
@@ -15,7 +15,7 @@ type FallbackStrategy func(http.ResponseWriter, *http.Request) bool
 
 // ServeAcmeChallenges identifies if request is acme-challenge and redirects to GitLab in that case
 func ServeAcmeChallenges(w http.ResponseWriter, r *http.Request, fallback FallbackStrategy, gitlabURL *url.URL) bool {
-	if !isAcmeChallenge(r.URL.Path) {
+	if !IsAcmeChallenge(r.URL.Path) {
 		return false
 	}
 
@@ -27,7 +27,7 @@ func ServeAcmeChallenges(w http.ResponseWriter, r *http.Request, fallback Fallba
 	return true
 }
 
-func isAcmeChallenge(path string) bool {
+func IsAcmeChallenge(path string) bool {
 	return strings.HasPrefix(filepath.Clean(path), "/.well-known/acme-challenge/")
 }
 
diff --git a/internal/redirects/redirects.go b/internal/redirects/redirects.go
index 4add6537..4903518a 100644
--- a/internal/redirects/redirects.go
+++ b/internal/redirects/redirects.go
@@ -13,6 +13,7 @@ import (
 	netlifyRedirects "github.com/tj/go-redirects"
 	"gitlab.com/gitlab-org/labkit/log"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/acme"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs"
 )
@@ -106,6 +107,10 @@ 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(originalURL *url.URL) (*url.URL, int, error) {
+	if acme.IsAcmeChallenge(originalURL.Path) {
+		return nil, 0, ErrNoRedirect
+	}
+
 	rule, newPath := r.match(originalURL.Path)
 	if rule == nil {
 		return nil, 0, ErrNoRedirect
diff --git a/internal/redirects/redirects_test.go b/internal/redirects/redirects_test.go
index d40b065b..4ec34222 100644
--- a/internal/redirects/redirects_test.go
+++ b/internal/redirects/redirects_test.go
@@ -107,6 +107,14 @@ func TestRedirectsRewrite(t *testing.T) {
 			expectedURL:    "/the/cake/is/a/lie",
 			expectedStatus: http.StatusOK,
 		},
+		{
+			name:           "does_not_redirect_acme_challenges",
+			url:            "/.well-known/acme-challenge/token",
+			rule:           "/* /to/path 200",
+			expectedURL:    "",
+			expectedStatus: 0,
+			expectedErr:    ErrNoRedirect,
+		},
 	}
 
 	for _, tt := range tests {
diff --git a/shared/pages/group.acme/with.redirects/config.json b/shared/pages/group.acme/with.redirects/config.json
new file mode 100644
index 00000000..cc0d8ce6
--- /dev/null
+++ b/shared/pages/group.acme/with.redirects/config.json
@@ -0,0 +1,6 @@
+{ "domains": [
+  {
+    "domain": "acmewithredirects.domain.com"
+  }
+]
+}
diff --git a/shared/pages/group.acme/with.redirects/public.zip b/shared/pages/group.acme/with.redirects/public.zip
new file mode 100644
index 00000000..d6142bec
--- /dev/null
+++ b/shared/pages/group.acme/with.redirects/public.zip
diff --git a/shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/existingtoken b/shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/existingtoken
new file mode 100644
index 00000000..84455e1d
--- /dev/null
+++ b/shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/existingtoken
@@ -0,0 +1 @@
+this is token
diff --git a/shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/foldertoken/index.html b/shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/foldertoken/index.html
new file mode 100644
index 00000000..3212dd99
--- /dev/null
+++ b/shared/pages/group.acme/with.redirects/public/.well-known/acme-challenge/foldertoken/index.html
@@ -0,0 +1 @@
+foldertoken content
diff --git a/shared/pages/group.acme/with.redirects/public/_redirects b/shared/pages/group.acme/with.redirects/public/_redirects
new file mode 100644
index 00000000..f8243379
--- /dev/null
+++ b/shared/pages/group.acme/with.redirects/public/_redirects
@@ -0,0 +1 @@
+/* /index.html 200
\ No newline at end of file
diff --git a/shared/pages/group.acme/with.redirects/public/index.html b/shared/pages/group.acme/with.redirects/public/index.html
new file mode 100644
index 00000000..2f47ed25
--- /dev/null
+++ b/shared/pages/group.acme/with.redirects/public/index.html
@@ -0,0 +1 @@
+acme-challenge-project
diff --git a/test/acceptance/acme_test.go b/test/acceptance/acme_test.go
index 40d98a21..89c57c96 100644
--- a/test/acceptance/acme_test.go
+++ b/test/acceptance/acme_test.go
@@ -7,6 +7,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers"
 )
 
 const (
@@ -97,3 +99,55 @@ func TestAcmeChallengesWhenItIsConfigured(t *testing.T) {
 		})
 	}
 }
+
+func TestAcmeChallengesNoRedirection(t *testing.T) {
+	RunPagesProcess(t,
+		withListeners([]ListenSpec{httpListener}),
+		withExtraArgument("gitlab-server", "https://gitlab-acme.com"),
+	)
+
+	tests := map[string]struct {
+		path             string
+		expectedStatus   int
+		expectedContent  string
+		expectedLocation string
+	}{
+		"wildcard redirect should not redirect acme challenge": {
+			path:            existingAcmeTokenPath,
+			expectedStatus:  http.StatusOK,
+			expectedContent: "this is token\n",
+		},
+		"non-acme paths should be redirected": {
+			path: "/example",
+			// rule inside _redirects is a 200 rewrite of /index.html
+			expectedStatus:  http.StatusOK,
+			expectedContent: "acme-challenge-project\n",
+		},
+		"When domain folder doesn't contain requested acme challenge it redirects to GitLab": {
+			path:             notExistingAcmeTokenPath,
+			expectedStatus:   http.StatusTemporaryRedirect,
+			expectedContent:  "",
+			expectedLocation: "https://gitlab-acme.com/-/acme-challenge?domain=acmewithredirects.domain.com&token=notexistingtoken",
+		},
+	}
+
+	for tn, tt := range tests {
+		t.Run(tn, func(t *testing.T) {
+			rsp, err := GetRedirectPage(t, httpListener, "acmewithredirects.domain.com", tt.path)
+			require.NoError(t, err)
+			testhelpers.Close(t, rsp.Body)
+
+			require.Equal(t, tt.expectedStatus, rsp.StatusCode)
+			body, err := io.ReadAll(rsp.Body)
+			require.NoError(t, rsp.Body.Close())
+			require.NoError(t, err)
+
+			require.Contains(t, string(body), tt.expectedContent)
+
+			redirectURL, err := url.Parse(rsp.Header.Get("Location"))
+			require.NoError(t, err)
+
+			require.Equal(t, redirectURL.String(), tt.expectedLocation)
+		})
+	}
+}
diff --git a/test/gitlabstub/api_responses.go b/test/gitlabstub/api_responses.go
index 3ebf4e1f..93bc8995 100644
--- a/test/gitlabstub/api_responses.go
+++ b/test/gitlabstub/api_responses.go
@@ -110,6 +110,10 @@ var domainResponses = map[string]responseFn{
 		accessControl: true,
 		pathOnDisk:    "group.auth/private.project",
 	}),
+	"acmewithredirects.domain.com": customDomain(projectConfig{
+		projectID:  1008,
+		pathOnDisk: "group.acme/with.redirects",
+	}),
 	// NOTE: before adding more domains here, generate the zip archive by running (per project)
 	// make zip PROJECT_SUBDIR=group/serving
 	// make zip PROJECT_SUBDIR=group/project2