diff options
-rw-r--r-- | app.go | 7 | ||||
-rw-r--r-- | internal/config/config.go | 4 | ||||
-rw-r--r-- | internal/config/flags.go | 1 | ||||
-rw-r--r-- | internal/httperrors/httperrors.go | 13 | ||||
-rw-r--r-- | internal/httperrors/httperrors_test.go | 12 | ||||
-rw-r--r-- | internal/urilimiter/urilimiter.go | 23 | ||||
-rw-r--r-- | internal/urilimiter/urilimiter_test.go | 70 | ||||
-rw-r--r-- | test/acceptance/urilimiter_test.go | 56 |
8 files changed, 183 insertions, 3 deletions
@@ -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") + } + }) + } +} |