Merge branch 'rmay-fix-query-string-redirects' into 'master'

Fix for query strings being stripped

Closes #191

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

4 files changed, 155 insertions, 50 deletions

diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go
index 5b0c56f0..12223bad 100644
--- a/internal/serving/disk/reader.go
+++ b/internal/serving/disk/reader.go
@@ -77,23 +77,13 @@ func (reader *Reader) tryFile(h serving.Handler) bool {
 	fullPath, err := reader.resolvePath(ctx, root, h.SubPath)
 
 	request := h.Request
-	host := request.Host
 	urlPath := request.URL.Path
 
 	if locationError, _ := err.(*locationDirectoryError); locationError != nil {
 		if endsWithSlash(urlPath) {
 			fullPath, err = reader.resolvePath(ctx, root, h.SubPath, "index.html")
 		} else {
-			// TODO why are we doing that? In tests it redirects to HTTPS. This seems wrong,
-			// issue about this: https://gitlab.com/gitlab-org/gitlab-pages/issues/273
-
-			// Concat Host with URL.Path
-			redirectPath := "//" + host + "/"
-			redirectPath += strings.TrimPrefix(urlPath, "/")
-
-			// Ensure that there's always "/" at end
-			redirectPath = strings.TrimSuffix(redirectPath, "/") + "/"
-			http.Redirect(h.Writer, h.Request, redirectPath, 302)
+			http.Redirect(h.Writer, h.Request, redirectPath(h.Request), 302)
 			return true
 		}
 	}
@@ -124,6 +114,17 @@ func (reader *Reader) tryFile(h serving.Handler) bool {
 	return reader.serveFile(ctx, h.Writer, h.Request, root, fullPath, h.LookupPath.HasAccessControl)
 }
 
+func redirectPath(request *http.Request) string {
+	url := *request.URL
+
+	// This ensures that path starts with `//<host>/`
+	url.Scheme = ""
+	url.Host = request.Host
+	url.Path = strings.TrimPrefix(url.Path, "/") + "/"
+
+	return strings.TrimSuffix(url.String(), "?")
+}
+
 func (reader *Reader) tryNotFound(h serving.Handler) bool {
 	ctx := h.Request.Context()
 
diff --git a/internal/serving/disk/reader_test.go b/internal/serving/disk/reader_test.go
new file mode 100644
index 00000000..53ea3d9a
--- /dev/null
+++ b/internal/serving/disk/reader_test.go
@@ -0,0 +1,68 @@
+package disk
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func Test_redirectPath(t *testing.T) {
+	tests := map[string]struct {
+		request      *http.Request
+		expectedPath string
+	}{
+		"simple_url_no_path": {
+			request:      newRequest(t, "https://domain.gitlab.io"),
+			expectedPath: "//domain.gitlab.io/",
+		},
+		"path_only": {
+			request:      newRequest(t, "https://domain.gitlab.io/index.html"),
+			expectedPath: "//domain.gitlab.io/index.html/",
+		},
+		"query_only": {
+			request:      newRequest(t, "https://domain.gitlab.io?query=test"),
+			expectedPath: "//domain.gitlab.io/?query=test",
+		},
+		"empty_query": {
+			request:      newRequest(t, "https://domain.gitlab.io?"),
+			expectedPath: "//domain.gitlab.io/",
+		},
+		"fragment_only": {
+			request:      newRequest(t, "https://domain.gitlab.io#fragment"),
+			expectedPath: "//domain.gitlab.io/#fragment",
+		},
+		"path_and_query": {
+			request:      newRequest(t, "https://domain.gitlab.io/index.html?query=test"),
+			expectedPath: "//domain.gitlab.io/index.html/?query=test",
+		},
+		"path_and_fragment": {
+			request:      newRequest(t, "https://domain.gitlab.io/index.html#fragment"),
+			expectedPath: "//domain.gitlab.io/index.html/#fragment",
+		},
+		"query_and_fragment": {
+			request:      newRequest(t, "https://domain.gitlab.io?query=test#fragment"),
+			expectedPath: "//domain.gitlab.io/?query=test#fragment",
+		},
+		"path_query_and_fragment": {
+			request:      newRequest(t, "https://domain.gitlab.io/index.html?query=test#fragment"),
+			expectedPath: "//domain.gitlab.io/index.html/?query=test#fragment",
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			got := redirectPath(test.request)
+			require.Equal(t, test.expectedPath, got)
+		})
+	}
+}
+
+func newRequest(t *testing.T, url string) *http.Request {
+	t.Helper()
+
+	r, err := http.NewRequest("GET", url, nil)
+	require.NoError(t, err)
+
+	return r
+}
diff --git a/test/acceptance/auth_test.go b/test/acceptance/auth_test.go
index e4c621cf..b2233591 100644
--- a/test/acceptance/auth_test.go
+++ b/test/acceptance/auth_test.go
@@ -189,60 +189,77 @@ func TestAccessControlUnderCustomDomain(t *testing.T) {
 	teardown := RunPagesProcessWithAuthServer(t, *pagesBinary, listeners, "", testServer.URL)
 	defer teardown()
 
-	rsp, err := GetRedirectPage(t, httpListener, "private.domain.com", "/")
-	require.NoError(t, err)
-	defer rsp.Body.Close()
+	tests := map[string]struct {
+		domain string
+		path   string
+	}{
+		"private_domain": {
+			domain: "private.domain.com",
+			path:   "",
+		},
+		"private_domain_with_query": {
+			domain: "private.domain.com",
+			path:   "?q=test",
+		},
+	}
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			rsp, err := GetRedirectPage(t, httpListener, tt.domain, tt.path)
+			require.NoError(t, err)
+			defer rsp.Body.Close()
 
-	cookie := rsp.Header.Get("Set-Cookie")
+			cookie := rsp.Header.Get("Set-Cookie")
 
-	url, err := url.Parse(rsp.Header.Get("Location"))
-	require.NoError(t, err)
+			url, err := url.Parse(rsp.Header.Get("Location"))
+			require.NoError(t, err)
 
-	state := url.Query().Get("state")
-	require.Equal(t, url.Query().Get("domain"), "http://private.domain.com")
+			state := url.Query().Get("state")
+			require.Equal(t, "http://"+tt.domain, url.Query().Get("domain"))
 
-	pagesrsp, err := GetRedirectPage(t, httpListener, url.Host, url.Path+"?"+url.RawQuery)
-	require.NoError(t, err)
-	defer pagesrsp.Body.Close()
+			pagesrsp, err := GetRedirectPage(t, httpListener, url.Host, url.Path+"?"+url.RawQuery)
+			require.NoError(t, err)
+			defer pagesrsp.Body.Close()
 
-	pagescookie := pagesrsp.Header.Get("Set-Cookie")
+			pagescookie := pagesrsp.Header.Get("Set-Cookie")
 
-	// Go to auth page with correct state will cause fetching the token
-	authrsp, err := GetRedirectPageWithCookie(t, httpListener, "projects.gitlab-example.com", "/auth?code=1&state="+
-		state, pagescookie)
+			// Go to auth page with correct state will cause fetching the token
+			authrsp, err := GetRedirectPageWithCookie(t, httpListener, tt.domain, "/auth?code=1&state="+
+				state, pagescookie)
 
-	require.NoError(t, err)
-	defer authrsp.Body.Close()
+			require.NoError(t, err)
+			defer authrsp.Body.Close()
 
-	url, err = url.Parse(authrsp.Header.Get("Location"))
-	require.NoError(t, err)
+			url, err = url.Parse(authrsp.Header.Get("Location"))
+			require.NoError(t, err)
 
-	// Will redirect to custom domain
-	require.Equal(t, "private.domain.com", url.Host)
-	require.Equal(t, "1", url.Query().Get("code"))
-	require.Equal(t, state, url.Query().Get("state"))
+			// Will redirect to custom domain
+			require.Equal(t, tt.domain, url.Host)
+			require.Equal(t, "1", url.Query().Get("code"))
+			require.Equal(t, state, url.Query().Get("state"))
 
-	// Run auth callback in custom domain
-	authrsp, err = GetRedirectPageWithCookie(t, httpListener, "private.domain.com", "/auth?code=1&state="+
-		state, cookie)
+			// Run auth callback in custom domain
+			authrsp, err = GetRedirectPageWithCookie(t, httpListener, tt.domain, "/auth?code=1&state="+
+				state, cookie)
 
-	require.NoError(t, err)
-	defer authrsp.Body.Close()
+			require.NoError(t, err)
+			defer authrsp.Body.Close()
 
-	// Will redirect to the page
-	cookie = authrsp.Header.Get("Set-Cookie")
-	require.Equal(t, http.StatusFound, authrsp.StatusCode)
+			// Will redirect to the page
+			cookie = authrsp.Header.Get("Set-Cookie")
+			require.Equal(t, http.StatusFound, authrsp.StatusCode)
 
-	url, err = url.Parse(authrsp.Header.Get("Location"))
-	require.NoError(t, err)
+			url, err = url.Parse(authrsp.Header.Get("Location"))
+			require.NoError(t, err)
 
-	// Will redirect to custom domain
-	require.Equal(t, "http://private.domain.com/", url.String())
+			// Will redirect to custom domain
+			require.Equal(t, "http://"+tt.domain+"/"+tt.path, url.String())
 
-	// Fetch page in custom domain
-	authrsp, err = GetRedirectPageWithCookie(t, httpListener, "private.domain.com", "/", cookie)
-	require.NoError(t, err)
-	require.Equal(t, http.StatusOK, authrsp.StatusCode)
+			// Fetch page in custom domain
+			authrsp, err = GetRedirectPageWithCookie(t, httpListener, tt.domain, tt.path, cookie)
+			require.NoError(t, err)
+			require.Equal(t, http.StatusOK, authrsp.StatusCode)
+		})
+	}
 }
 
 func TestCustomErrorPageWithAuth(t *testing.T) {
diff --git a/test/acceptance/serving_test.go b/test/acceptance/serving_test.go
index da2843a0..66b5fa47 100644
--- a/test/acceptance/serving_test.go
+++ b/test/acceptance/serving_test.go
@@ -552,3 +552,22 @@ func doCrossOriginRequest(t *testing.T, spec ListenSpec, method, reqMethod, url
 	rsp.Body.Close()
 	return rsp
 }
+
+func TestQueryStringPersistedInSlashRewrite(t *testing.T) {
+	skipUnlessEnabled(t)
+	teardown := RunPagesProcess(t, *pagesBinary, listeners, "")
+	defer teardown()
+
+	rsp, err := GetRedirectPage(t, httpsListener, "group.gitlab-example.com", "project?q=test")
+	require.NoError(t, err)
+	defer rsp.Body.Close()
+
+	require.Equal(t, http.StatusFound, rsp.StatusCode)
+	require.Equal(t, 1, len(rsp.Header["Location"]))
+	require.Equal(t, "//group.gitlab-example.com/project/?q=test", rsp.Header.Get("Location"))
+
+	rsp, err = GetPageFromListener(t, httpsListener, "group.gitlab-example.com", "project/?q=test")
+	require.NoError(t, err)
+	defer rsp.Body.Close()
+	require.Equal(t, http.StatusOK, rsp.StatusCode)
+}