diff --git a/README.md b/README.md index 87d3838..13a6a3b 100644 --- a/README.md +++ b/README.md @@ -685,7 +685,9 @@ See `configs/config.example.yml` for all options with defaults. `metrics.username` and `metrics.password` are set. A request's `handler` label is the pattern of the route it matches, such as `/v1/image/*`, never its path. Requests that match no route share the label `unmatched`: those the router - answers 404 or 405, and CORS preflights + answers 404 or 405, and CORS preflights. A request's `method` label is its + method when that is `GET`, `HEAD`, `POST`, `PUT`, `PATCH`, `DELETE`, + `OPTIONS`, `CONNECT` or `TRACE`, and `other` for any other method - **Logging**: stdlib slog ## Entrypoints diff --git a/TODO.md b/TODO.md index 724a404..3c33482 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,15 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-15 the request metrics record any method that is not a standard one as + `other` (closes #274): their `method` label was each request's method as sent, + and Go's HTTP server accepts any method name, so a client sending made-up + methods added series to the Prometheus registry that were never removed. The + label is now the request's method only for `GET`, `HEAD`, `POST`, `PUT`, + `PATCH`, `DELETE`, `OPTIONS`, `CONNECT` and `TRACE`. The metrics library reads + the method from the request it is handed, so the metrics middleware hands it a + copy of the request carrying the label; the handlers still see the method the + client sent. - 2026-10-15 logging out ends every login session (closes #256): a login session was a cookie with nothing kept on the server, so `/logout` removed it from one browser and a copy of it kept working for its 30 days. Every login is made diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index a35c23e..4053a0a 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -220,7 +220,10 @@ func (s *Middleware) CORS() func(http.Handler) http.Handler { // pattern of the chi route a request matches, such as /v1/image/*, or // "unmatched" for a request that matches none, so the routes fix how many // values the label has. It is never the path, with which any client could -// add series to the registry without limit. +// add series to the registry without limit. Its method label is the +// request's method when that is a standard one, such as GET, and "other" +// for any other: Go's HTTP server accepts any method name, so a label per +// method would let a client add series the same way. func (s *Middleware) Metrics() func(http.Handler) http.Handler { mdlw := ghmm.New(ghmm.Config{ Recorder: metrics.NewRecorder(metrics.Config{ @@ -230,7 +233,19 @@ func (s *Middleware) Metrics() func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - std.Handler(routePattern(r), mdlw, next).ServeHTTP(w, r) + // The metrics library takes the method label from the request + // it is handed, so it is handed a shallow copy of r whose + // method is the label. next is served r itself, through the + // response writer the library wraps to see the status and size. + labelled := r.WithContext(r.Context()) + labelled.Method = methodLabel(r.Method) + + serveNext := http.HandlerFunc( + func(wrapped http.ResponseWriter, _ *http.Request) { + next.ServeHTTP(wrapped, r) + }) + + std.Handler(routePattern(r), mdlw, serveNext).ServeHTTP(w, labelled) }) } } @@ -275,6 +290,20 @@ func routePattern(r *http.Request) string { return pattern } +// methodLabel returns method when it is one of the standard methods net/http +// names: GET, HEAD, POST, PUT, PATCH, DELETE, OPTIONS, CONNECT and TRACE. +// For any other method it returns "other". +func methodLabel(method string) string { + switch method { + case http.MethodGet, http.MethodHead, http.MethodPost, http.MethodPut, + http.MethodPatch, http.MethodDelete, http.MethodOptions, + http.MethodConnect, http.MethodTrace: + return method + default: + return "other" + } +} + // MetricsAuth returns a basic auth middleware for the metrics endpoint. func (s *Middleware) MetricsAuth() func(http.Handler) http.Handler { return basicauth.New( diff --git a/internal/middleware/middleware_internal_test.go b/internal/middleware/middleware_internal_test.go index 0e17a05..21d18b4 100644 --- a/internal/middleware/middleware_internal_test.go +++ b/internal/middleware/middleware_internal_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "slices" "strings" "testing" @@ -314,6 +315,58 @@ func requestCountsByHandler( return counts } +// TestMetricsGivesMadeUpMethodsOneLabel checks that the metrics middleware +// gives requests with methods that are not standard ones a single method +// label, "other", and still serves each request with the method it was sent +// with. With a label per method, any client could add series to the +// registry without limit by making up a new method for each request. +func TestMetricsGivesMadeUpMethodsOneLabel(t *testing.T) { + t.Parallel() + + // The line /metrics shows once both made-up methods have been served. + const want = `http_request_duration_seconds_count{` + + `code="200",handler="unmatched",method="other",service=""} 2` + + registry := prometheus.NewRegistry() + mw := &Middleware{ + log: slog.Default(), + config: &config.Config{}, + metricsRegistry: registry, + } + + var served []string + + handler := mw.Metrics()(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + served = append(served, r.Method) + + w.WriteHeader(http.StatusOK) + })) + + methods := []string{"FROBNICATE", "WIBBLE"} + for _, method := range methods { + handler.ServeHTTP(httptest.NewRecorder(), httptest.NewRequestWithContext( + t.Context(), method, "/", nil)) + } + + t.Logf("methods the handler was served: %v", served) + + if !slices.Equal(served, methods) { + t.Errorf("methods the handler was served = %v, want %v", + served, methods) + } + + rec := httptest.NewRecorder() + promhttp.HandlerFor(registry, promhttp.HandlerOpts{}).ServeHTTP(rec, + httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/metrics", nil)) + + if !strings.Contains(rec.Body.String(), want) { + t.Errorf("/metrics does not count both made-up methods as one; "+ + "want the line %q in:\n%s", want, rec.Body.String()) + } +} + // TestLoggingLeavesOutSubmittedSigningKey checks that a login, a POST / // whose form carries the signing key, leaves no trace of the key in the // request's log line.