diff options
author | Alessio Caiazza <376774-nolith@users.noreply.gitlab.com> | 2020-12-22 14:36:46 +0300 |
---|---|---|
committer | Alessio Caiazza <376774-nolith@users.noreply.gitlab.com> | 2020-12-22 14:36:46 +0300 |
commit | 46cb4543286155bc08b9013f3c458299d2e8ddf8 (patch) | |
tree | 25a542f009d9a500c7a461716b233bc8b829b104 | |
parent | 77d2ff97e759eb7ded278286105f61894fde05b0 (diff) | |
parent | 875886462aceff1df4a15da95149734f9666a6d4 (diff) |
Merge branch 'security-1-28-prometheus-lables' into '1-28-stable'
Reject all unknown http methods
See merge request gitlab-org/security/gitlab-pages!9
-rw-r--r-- | acceptance_test.go | 15 | ||||
-rw-r--r-- | app.go | 7 | ||||
-rw-r--r-- | internal/rejectmethods/middleware.go | 30 | ||||
-rw-r--r-- | internal/rejectmethods/middleware_test.go | 43 | ||||
-rw-r--r-- | metrics/metrics.go | 7 |
5 files changed, 102 insertions, 0 deletions
diff --git a/acceptance_test.go b/acceptance_test.go index 1ded9be0..e7b3d2da 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -2156,3 +2156,18 @@ func TestZipServing(t *testing.T) { }) } } + +func TestUnknownHTTPMethod(t *testing.T) { + skipUnlessEnabled(t) + teardown := RunPagesProcess(t, *pagesBinary, listeners, "") + defer teardown() + + req, err := http.NewRequest("UNKNOWN", listeners[0].URL(""), nil) + require.NoError(t, err) + req.Host = "" + + resp, err := DoPagesRequest(t, req) + require.NoError(t, err) + + require.Equal(t, http.StatusMethodNotAllowed, resp.StatusCode) +} @@ -26,6 +26,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/logging" "gitlab.com/gitlab-org/gitlab-pages/internal/netutil" + "gitlab.com/gitlab-org/gitlab-pages/internal/rejectmethods" "gitlab.com/gitlab-org/gitlab-pages/internal/request" "gitlab.com/gitlab-org/gitlab-pages/internal/source" "gitlab.com/gitlab-org/gitlab-pages/metrics" @@ -333,6 +334,12 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { // Custom response headers handler = a.customHeadersMiddleware(handler) + // This MUST be the last handler! + // This handler blocks unknown HTTP methods, + // being the last means it will be evaluated first + // preventing any operation on bogus requests. + handler = rejectmethods.NewMiddleware(handler) + return handler, nil } diff --git a/internal/rejectmethods/middleware.go b/internal/rejectmethods/middleware.go new file mode 100644 index 00000000..e76fc383 --- /dev/null +++ b/internal/rejectmethods/middleware.go @@ -0,0 +1,30 @@ +package rejectmethods + +import ( + "net/http" + + "gitlab.com/gitlab-org/gitlab-pages/metrics" +) + +var acceptedMethods = map[string]bool{ + http.MethodGet: true, + http.MethodHead: true, + http.MethodPost: true, + http.MethodPut: true, + http.MethodPatch: true, + http.MethodConnect: true, + http.MethodOptions: true, + http.MethodTrace: true, +} + +// NewMiddleware returns middleware which rejects all unknown http methods +func NewMiddleware(handler http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if acceptedMethods[r.Method] { + handler.ServeHTTP(w, r) + } else { + metrics.RejectedRequestsCount.Inc() + http.Error(w, http.StatusText(http.StatusMethodNotAllowed), http.StatusMethodNotAllowed) + } + }) +} diff --git a/internal/rejectmethods/middleware_test.go b/internal/rejectmethods/middleware_test.go new file mode 100644 index 00000000..2921975a --- /dev/null +++ b/internal/rejectmethods/middleware_test.go @@ -0,0 +1,43 @@ +package rejectmethods + +import ( + "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) { + io.WriteString(w, "OK\n") + }) + + middleware := NewMiddleware(handler) + + acceptedMethods := []string{"GET", "HEAD", "POST", "PUT", "PATCH", "CONNECT", "OPTIONS", "TRACE"} + for _, method := range acceptedMethods { + t.Run(method, func(t *testing.T) { + tmpRequest, _ := http.NewRequest(method, "/", nil) + recorder := httptest.NewRecorder() + + middleware.ServeHTTP(recorder, tmpRequest) + + result := recorder.Result() + + require.Equal(t, http.StatusOK, result.StatusCode) + }) + } + + t.Run("UNKNOWN", func(t *testing.T) { + tmpRequest, _ := http.NewRequest("UNKNOWN", "/", nil) + recorder := httptest.NewRecorder() + + middleware.ServeHTTP(recorder, tmpRequest) + + result := recorder.Result() + + require.Equal(t, http.StatusMethodNotAllowed, result.StatusCode) + }) +} diff --git a/metrics/metrics.go b/metrics/metrics.go index db7cae9a..045ff26e 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -199,6 +199,13 @@ var ( Help: "The number of files per zip archive total count over time", }, ) + + RejectedRequestsCount = prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "gitlab_pages_unknown_method_rejected_requests", + Help: "The number of requests with unknown HTTP method which were rejected", + }, + ) ) // MustRegister collectors with the Prometheus client |