Fix for query strings being stripped

This ensures the query string is persisted when redirecting
to add a trailing slash.

2 files changed, 80 insertions, 11 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
+}