diff --git a/TODO.md b/TODO.md index 0c4ac74..c4d4912 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,16 @@ P2: security: referer blacklist # Completed Steps +- 2026-10-04 the metrics basic auth, CORS preflight, request logging and + metrics recording have tests (closes #79): the basic auth in front of + `/metrics` answers 401 with a challenge without credentials or with a wrong + username or password and lets the configured ones through; a preflight + request gets `*` for any origin when `access_control_allow_origin` is `*` and + no `Access-Control-Allow-Origin` from another origin than the configured + one; a `POST /` carrying the signing key leaves no trace of it in the log + line; the metrics middleware records a request it served, and the router + records nothing while no metrics username is set. Tests only; the basic auth + library already compares the password in constant time. - 2026-10-04 routes, encrypted URLs and config file documented (closes #75): "Routes" in `README.md` lists every route with its method, purpose, what it needs and the status codes it answers with, and says `q` and `fit` are part diff --git a/internal/middleware/middleware_internal_test.go b/internal/middleware/middleware_internal_test.go index 5dac1be..a2b819f 100644 --- a/internal/middleware/middleware_internal_test.go +++ b/internal/middleware/middleware_internal_test.go @@ -1,11 +1,16 @@ package middleware import ( + "bytes" "log/slog" "net/http" "net/http/httptest" + "net/url" + "strings" "testing" + "github.com/prometheus/client_golang/prometheus/promhttp" + "sneak.berlin/go/pixa/internal/config" ) @@ -56,6 +61,202 @@ func TestCORSAnswersWithConfiguredOrigin(t *testing.T) { } } +// TestCORSAnswersPreflightWithConfiguredOrigin checks that the CORS +// middleware answers a preflight request, which the CORS library handles +// apart from other requests, the same way: "*" lets any origin read +// responses and a single origin lets only that origin read them. +func TestCORSAnswersPreflightWithConfiguredOrigin(t *testing.T) { + t.Parallel() + + const appOrigin = "https://app.example.com" + + cases := []struct { + configured string + requestOrigin string + want string + }{ + {"*", "https://any.example.com", "*"}, + {appOrigin, appOrigin, appOrigin}, + {appOrigin, "https://other.example.com", ""}, + } + + testHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + for _, tc := range cases { + mw := &Middleware{ + log: slog.Default(), + config: &config.Config{AccessControlAllowOrigin: tc.configured}, + } + + handler := mw.CORS()(testHandler) + + // An OPTIONS request naming the method it asks about is the + // preflight a browser sends before some cross-origin requests. + req := httptest.NewRequestWithContext( + t.Context(), http.MethodOptions, "/v1/image/example.com/a.jpg/1x1.png", nil) + req.Header.Set("Origin", tc.requestOrigin) + req.Header.Set("Access-Control-Request-Method", http.MethodGet) + + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + got := rec.Header().Get("Access-Control-Allow-Origin") + if got != tc.want { + t.Errorf("configured %q, preflight from %q: "+ + "Access-Control-Allow-Origin = %q, want %q", + tc.configured, tc.requestOrigin, got, tc.want) + } + } +} + +// TestMetricsAuthRequiresConfiguredCredentials checks that the basic auth +// in front of /metrics answers 401 with a challenge to a request without +// credentials or with a wrong username or password, and lets a request with +// the configured username and password through. +func TestMetricsAuthRequiresConfiguredCredentials(t *testing.T) { + t.Parallel() + + const ( + username = "metricsuser" + password = "metricspass" + challenge = `Basic realm="metrics"` + ) + + // An empty username stands for a request sent without credentials. + cases := []struct { + name string + username string + password string + wantReached bool + }{ + {"no credentials", "", "", false}, + {"wrong username", "someone", password, false}, + {"wrong password", username, "wrongpass", false}, + {"configured credentials", username, password, true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + mw := &Middleware{ + log: slog.Default(), + config: &config.Config{ + MetricsUsername: username, + MetricsPassword: password, + }, + } + + reached := false + handler := mw.MetricsAuth()(http.HandlerFunc( + func(http.ResponseWriter, *http.Request) { + reached = true + })) + + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/metrics", nil) + + if tc.username != "" { + req.SetBasicAuth(tc.username, tc.password) + } + + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + if reached != tc.wantReached { + t.Fatalf("request reached /metrics = %v, want %v", + reached, tc.wantReached) + } + + if tc.wantReached { + return + } + + if rec.Code != http.StatusUnauthorized { + t.Errorf("status = %d, want %d", + rec.Code, http.StatusUnauthorized) + } + + if got := rec.Header().Get("WWW-Authenticate"); got != challenge { + t.Errorf("WWW-Authenticate = %q, want %q", got, challenge) + } + }) + } +} + +// TestMetricsRecordsServedRequest checks that the metrics middleware +// records a request it served, so /metrics reports it. It is the only test +// in this package that sets up the metrics middleware, which registers with +// the process-wide Prometheus registry and can do so only once. +func TestMetricsRecordsServedRequest(t *testing.T) { + t.Parallel() + + // The line /metrics shows once one GET /test has been served. + const want = `http_request_duration_seconds_count{` + + `code="200",handler="/test",method="GET",service=""} 1` + + mw := &Middleware{log: slog.Default(), config: &config.Config{}} + + handler := mw.Metrics()(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + handler.ServeHTTP(httptest.NewRecorder(), httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/test", nil)) + + rec := httptest.NewRecorder() + promhttp.Handler().ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/metrics", nil)) + + if !strings.Contains(rec.Body.String(), want) { + t.Errorf("/metrics does not report the GET /test served; "+ + "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. +func TestLoggingLeavesOutSubmittedSigningKey(t *testing.T) { + t.Parallel() + + const signingKey = "test-signing-key-0123456789abcdef" + + var buf bytes.Buffer + + mw := newTestMiddleware(t, &buf) + + // The handler reads the key from the form, as the login handler does. + handler := mw.Logging()(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + if got := r.FormValue("key"); got != signingKey { + t.Errorf("key in form = %q, want %q", got, signingKey) + } + + w.WriteHeader(http.StatusSeeOther) + })) + + form := url.Values{"key": {signingKey}} + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/", + strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + handler.ServeHTTP(httptest.NewRecorder(), req) + + if !strings.Contains(buf.String(), `"method":"POST"`) { + t.Fatalf("no log line for the request; got %q", buf.String()) + } + + if strings.Contains(buf.String(), signingKey) { + t.Errorf("log output contains the signing key; got %q", buf.String()) + } +} + func TestSecurityHeaders(t *testing.T) { t.Parallel() diff --git a/internal/server/metrics_internal_test.go b/internal/server/metrics_internal_test.go new file mode 100644 index 0000000..e1ffea6 --- /dev/null +++ b/internal/server/metrics_internal_test.go @@ -0,0 +1,32 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/prometheus/client_golang/prometheus/promhttp" +) + +// TestNoMetricsRecordedWithoutMetricsUsername checks that with no metrics +// username set the router records nothing about the requests it serves. +// /metrics is not served then, so the process-wide Prometheus registry is +// read directly. TestMaintenanceModeKeepsOtherRoutes records into the same +// registry, but never a GET /robots.txt. +func TestNoMetricsRecordedWithoutMetricsUsername(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + s.ServeHTTP(httptest.NewRecorder(), httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/robots.txt", nil)) + + rec := httptest.NewRecorder() + promhttp.Handler().ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/metrics", nil)) + + if strings.Contains(rec.Body.String(), `handler="/robots.txt"`) { + t.Errorf("with no metrics username GET /robots.txt was recorded:\n%s", + rec.Body.String()) + } +}