2026-10-04 14:39:28 +02:00
4 changed files with 308 additions and 0 deletions
@@ -29,6 +29,21 @@ P2: security: referer blacklist

 # Completed Steps

+- 2026-10-04 the metrics basic auth, CORS preflight, request logging and
+  metrics recording have tests (closes #79): `MetricsAuth` on its own 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 request log
+  line, and the login handler's own log lines leave out the submitted key; the
+  metrics middleware on its own records a request it served, and the router
+  records nothing while no metrics username is set. Not tested: that the router
+  puts the basic auth in front of `/metrics` and records requests when a
+  metrics username is set. Only one test per package can set up `/metrics`, and
+  in `internal/server` that is `TestMaintenanceModeKeepsOtherRoutes`, which
+  needs the owner's approval to change; #180 holds it. Tests only; the basic
+  auth library already compares the password in constant time.
 - 2026-10-04 the image route's signature check and error answers are tested
  (closes #76): new tests in `internal/handlers`, with no network, check the
  status and JSON error body for a missing, wrong, unpadded, upper-case or
@@ -0,0 +1,59 @@
+package handlers
+
+import (
+	"bytes"
+	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"testing"
+
+	"sneak.berlin/go/pixa/internal/config"
+	"sneak.berlin/go/pixa/internal/session"
+)
+
+// TestLoginLogLeavesOutSubmittedKey verifies that the log lines for a
+// failed and for a successful login do not contain the submitted key.
+func TestLoginLogLeavesOutSubmittedKey(t *testing.T) {
+	t.Parallel()
+
+	const wrongKey = "wrong-signing-key-fedcba9876543210"
+
+	var buf bytes.Buffer
+
+	sessMgr, err := session.NewManager(testSigningKey)
+	if err != nil {
+		t.Fatalf("session.NewManager() error = %v", err)
+	}
+
+	h := &Handlers{
+		log:     slog.New(slog.NewJSONHandler(&buf, nil)),
+		config:  &config.Config{SigningKey: testSigningKey},
+		sessMgr: sessMgr,
+	}
+
+	submittedKeys := []string{wrongKey, testSigningKey}
+
+	for _, key := range submittedKeys {
+		form := url.Values{loginKeyField: {key}}
+		req := httptest.NewRequestWithContext(
+			t.Context(), http.MethodPost, "/",
+			strings.NewReader(form.Encode()))
+		req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+
+		h.handleLoginPost(httptest.NewRecorder(), req)
+	}
+
+	for _, msg := range []string{"failed login attempt", "successful login"} {
+		if !strings.Contains(buf.String(), msg) {
+			t.Fatalf("log missing %q; got %q", msg, buf.String())
+		}
+	}
+
+	for _, key := range submittedKeys {
+		if strings.Contains(buf.String(), key) {
+			t.Errorf("log contains submitted key %q; got %q", key, buf.String())
+		}
+	}
+}
@@ -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,203 @@ 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 MetricsAuth on
+// its own 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. That the router puts it in front of
+// /metrics is not tested.
+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()

@@ -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())
+	}
+}