fix: reject requests with very long URIs

Some parts of the application may be vulnerable to very long URIs being passed.

E.g. Auth will try to save URI to session cookie, and it will fails, which will result in 500 error

Changelog: fixed

8 files changed, 183 insertions, 3 deletions

diff --git a/app.go b/app.go
index cddf8d49..3f286033 100644
--- a/app.go
+++ b/app.go
@@ -39,6 +39,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/zip"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/urilimiter"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
@@ -292,10 +293,10 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) {
 	handler = handlePanicMiddleware(handler)
 	handler = correlation.InjectCorrelationID(handler, correlationOpts...)
 
-	// This MUST be the last handler!
-	// This handler blocks unknown HTTP methods,
-	// being the last means it will be evaluated first
+	// These middlewares MUST be added in the end.
+	// Being last means they will be evaluated first
 	// preventing any operation on bogus requests.
+	handler = urilimiter.NewMiddleware(handler, a.config.General.MaxURILength)
 	handler = rejectmethods.NewMiddleware(handler)
 
 	return handler, nil
diff --git a/internal/config/config.go b/internal/config/config.go
index 3e03f7d6..c6f91db0 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -47,6 +47,7 @@ type Config struct {
 type General struct {
 	Domain          string
 	MaxConns        int
+	MaxURILength    int
 	MetricsAddress  string
 	RedirectHTTP    bool
 	RootCertificate []byte
@@ -181,6 +182,7 @@ func loadConfig() (*Config, error) {
 		General: General{
 			Domain:                     strings.ToLower(*pagesDomain),
 			MaxConns:                   *maxConns,
+			MaxURILength:               *maxURILength,
 			MetricsAddress:             *metricsAddress,
 			RedirectHTTP:               *redirectHTTP,
 			RootDir:                    *pagesRoot,
@@ -307,6 +309,8 @@ func LogConfig(config *Config) {
 		"enable-disk":                   config.GitLab.EnableDisk,
 		"auth-redirect-uri":             config.Authentication.RedirectURI,
 		"auth-scope":                    config.Authentication.Scope,
+		"max-conns":                     config.General.MaxConns,
+		"max-uri-length":                config.General.MaxURILength,
 		"zip-cache-expiration":          config.Zip.ExpirationInterval,
 		"zip-cache-cleanup":             config.Zip.CleanupInterval,
 		"zip-cache-refresh":             config.Zip.RefreshInterval,
diff --git a/internal/config/flags.go b/internal/config/flags.go
index c61447c7..6c9bd4a6 100644
--- a/internal/config/flags.go
+++ b/internal/config/flags.go
@@ -51,6 +51,7 @@ var (
 	redirectURI        = flag.String("auth-redirect-uri", "", "GitLab application redirect URI")
 	authScope          = flag.String("auth-scope", "api", "Scope to be used for authentication (must match GitLab Pages OAuth application settings)")
 	maxConns           = flag.Int("max-conns", 0, "Limit on the number of concurrent connections to the HTTP, HTTPS or proxy listeners, 0 for no limit")
+	maxURILength       = flag.Int("max-uri-length", 1024, "Limit the length of URI, 0 for unlimited.")
 	insecureCiphers    = flag.Bool("insecure-ciphers", false, "Use default list of cipher suites, may contain insecure ones like 3DES and RC4")
 	tlsMinVersion      = flag.String("tls-min-version", "tls1.2", tls.FlagUsage("min"))
 	tlsMaxVersion      = flag.String("tls-max-version", "", tls.FlagUsage("max"))
diff --git a/internal/httperrors/httperrors.go b/internal/httperrors/httperrors.go
index 8e61d590..a96e4d85 100644
--- a/internal/httperrors/httperrors.go
+++ b/internal/httperrors/httperrors.go
@@ -34,6 +34,14 @@ var (
      <p>Make sure the address is correct and that the page hasn't moved.</p>
      <p>Please contact your GitLab administrator if you think this is a mistake.</p>`,
 	}
+	content414 = content{
+		status:       http.StatusRequestURITooLong,
+		title:        "Request URI Too Long (414)",
+		statusString: "414",
+		header:       "Request URI Too Long.",
+		subHeader: `<p>The URI provided was too long for the server to process.</p>
+			<p>Try to make the request URI shorter.</p>`,
+	}
 
 	content429 = content{
 		http.StatusTooManyRequests,
@@ -184,6 +192,11 @@ func Serve404(w http.ResponseWriter) {
 	serveErrorPage(w, content404)
 }
 
+// Serve414 returns a 414 error response / HTML page to the http.ResponseWriter
+func Serve414(w http.ResponseWriter) {
+	serveErrorPage(w, content414)
+}
+
 // Serve429 returns a 429 error response / HTML page to the http.ResponseWriter
 func Serve429(w http.ResponseWriter) {
 	serveErrorPage(w, content429)
diff --git a/internal/httperrors/httperrors_test.go b/internal/httperrors/httperrors_test.go
index b003ce6f..671890f8 100644
--- a/internal/httperrors/httperrors_test.go
+++ b/internal/httperrors/httperrors_test.go
@@ -92,6 +92,18 @@ func TestServe404(t *testing.T) {
 	require.Contains(t, w.Content(), content404.subHeader)
 }
 
+func TestServe414(t *testing.T) {
+	w := newTestResponseWriter(httptest.NewRecorder())
+	Serve414(w)
+	require.Equal(t, w.Header().Get("Content-Type"), "text/html; charset=utf-8")
+	require.Equal(t, w.Header().Get("X-Content-Type-Options"), "nosniff")
+	require.Equal(t, w.Status(), content414.status)
+	require.Contains(t, w.Content(), content414.title)
+	require.Contains(t, w.Content(), content414.statusString)
+	require.Contains(t, w.Content(), content414.header)
+	require.Contains(t, w.Content(), content414.subHeader)
+}
+
 func TestServe500(t *testing.T) {
 	w := newTestResponseWriter(httptest.NewRecorder())
 	Serve500(w)
diff --git a/internal/urilimiter/urilimiter.go b/internal/urilimiter/urilimiter.go
new file mode 100644
index 00000000..65d664a5
--- /dev/null
+++ b/internal/urilimiter/urilimiter.go
@@ -0,0 +1,23 @@
+package urilimiter
+
+import (
+	"net/http"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
+)
+
+func NewMiddleware(handler http.Handler, limit int) http.Handler {
+	if limit == 0 {
+		return handler
+	}
+
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if len(r.RequestURI) > limit {
+			httperrors.Serve414(w)
+
+			return
+		}
+
+		handler.ServeHTTP(w, r)
+	})
+}
diff --git a/internal/urilimiter/urilimiter_test.go b/internal/urilimiter/urilimiter_test.go
new file mode 100644
index 00000000..0ac89ea0
--- /dev/null
+++ b/internal/urilimiter/urilimiter_test.go
@@ -0,0 +1,70 @@
+package urilimiter
+
+import (
+	"fmt"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestNewMiddleware(t *testing.T) {
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(w, "hello")
+	})
+
+	tests := map[string]struct {
+		limit          int
+		url            string
+		expectedStatus int
+	}{
+		"with_disabled_middleware": {
+			limit:          0,
+			url:            "/index.html",
+			expectedStatus: http.StatusOK,
+		},
+		"with_limit_set_to_request_length": {
+			limit:          17,
+			url:            "/index.html?q=a#b",
+			expectedStatus: http.StatusOK,
+		},
+		"with_uri_length_exceeding_the_limit": {
+			limit:          17,
+			url:            "/index1.html?q=a#b",
+			expectedStatus: http.StatusRequestURITooLong,
+		},
+		"with_uri_length_exceeding_the_limit_with_query": {
+			limit:          17,
+			url:            "/index.html?q=aa#b",
+			expectedStatus: http.StatusRequestURITooLong,
+		},
+		"with_uri_length_exceeding_the_limit_with_fragment": {
+			limit:          17,
+			url:            "/index.html?q=a#bb",
+			expectedStatus: http.StatusRequestURITooLong,
+		},
+	}
+	for tn, tt := range tests {
+		t.Run(tn, func(t *testing.T) {
+			middleware := NewMiddleware(handler, tt.limit)
+
+			ww := httptest.NewRecorder()
+			rr := httptest.NewRequest(http.MethodGet, tt.url, nil)
+
+			middleware.ServeHTTP(ww, rr)
+
+			res := ww.Result()
+			defer res.Body.Close()
+
+			require.Equal(t, tt.expectedStatus, res.StatusCode)
+			if tt.expectedStatus == http.StatusOK {
+				b, err := io.ReadAll(res.Body)
+				require.NoError(t, err)
+
+				require.Equal(t, "hello", string(b))
+			}
+		})
+	}
+}
diff --git a/test/acceptance/urilimiter_test.go b/test/acceptance/urilimiter_test.go
new file mode 100644
index 00000000..5e97921f
--- /dev/null
+++ b/test/acceptance/urilimiter_test.go
@@ -0,0 +1,56 @@
+package acceptance_test
+
+import (
+	"io"
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestURILimits proves fix for https://gitlab.com/gitlab-org/gitlab-pages/-/issues/659
+func TestURILimits(t *testing.T) {
+	tests := map[string]struct {
+		limit          string
+		path           string
+		expectedStatus int
+	}{
+		"with_disabled_limit": {
+			limit:          "0",
+			path:           "project/",
+			expectedStatus: http.StatusOK,
+		},
+		"with_limit_set_to_request_length": {
+			limit:          "19",
+			path:           "/project/index.html",
+			expectedStatus: http.StatusOK,
+		},
+		"with_uri_length_exceeding_the_limit": {
+			limit:          "19",
+			path:           "/project/index1.html",
+			expectedStatus: http.StatusRequestURITooLong,
+		},
+	}
+
+	for tn, tt := range tests {
+		t.Run(tn, func(t *testing.T) {
+			RunPagesProcess(t, withListeners([]ListenSpec{httpsListener}), withExtraArgument("max-uri-length", tt.limit))
+
+			rsp, err := GetPageFromListener(t, httpsListener, "group.gitlab-example.com", tt.path)
+			require.NoError(t, err)
+			defer func() {
+				require.NoError(t, rsp.Body.Close())
+			}()
+
+			require.Equal(t, tt.expectedStatus, rsp.StatusCode)
+
+			b, err := io.ReadAll(rsp.Body)
+			require.NoError(t, err)
+			if tt.expectedStatus == http.StatusOK {
+				require.Equal(t, "project-subdir\n", string(b))
+			} else {
+				require.Contains(t, string(b), "Request URI Too Long")
+			}
+		})
+	}
+}