Harden middleware, problemjson, outbound and spa, with route labels and client IP

.claude/skills/verify/SKILL.md

@@ -1,13 +1,13 @@
 ---
 name: verify
-description: Use when the user runs /verify or asks to run make verify. Runs the full verification suite (tests, lint, coverage, benchmarks, license) and fixes every issue found.
+description: Use when the user runs /verify or asks to run make verify. Runs the full verification suite (tests, license, lint, coverage) and fixes every issue found.
 ---
 
 # verify
 
 Run `make verify` and fix all issues until it passes clean.
 
-`make verify` runs in order: `test` → `ui-test` → `license-check` → `lint` → `benchmark` → `coverage`
+`make verify` runs in order: `test` → `license-check` → `lint` → `coverage`
 
 ## How to run
 

AGENTS.md

@@ -5,10 +5,10 @@ This file provides guidance to AI coding agents (e.g. Claude Code) when working
 ## Commands
 
 ```bash
-make test              # run all tests with coverage
+make test              # run all tests with the race detector and coverage
 make lint              # run golangci-lint (must be installed externally)
 make benchmark         # run benchmarks
-make verify            # run tests + lint + benchmarks + coverage check
+make verify            # run tests + license check + lint + coverage check (incl. TestWrapResourceBudget)
 go test ./...          # run all tests
 go test ./middleware/  # run tests for a single package
 go test ./middleware/ -run TestMiddleware  # run a single test
@@ -24,17 +24,19 @@ This is a Go library (`github.com/go-bumbu/http`) providing reusable HTTP compon
 - **middleware/metrics/** — Prometheus-backed `Observer` implementation, configured with a `metrics.Cfg`: `NewObserver` builds the registered request-duration histogram and wraps it as `middleware.Observer` (`NopObserver` is an explicit no-op; buckets are validated ascending at construction). The only package that imports `prometheus/client_golang`.
 - **spa/** — Single Page Application handler serving files from an `fs.FS` (typically embedded), built with `spa.New(spa.Cfg{...})`. Blocks dotfiles (except `.well-known/`) and normalises `PathPrefix`.
 - **outbound/** — Client-side resilience for a service's own outbound calls to third-party APIs: a throttled, retrying `Client` (built with `outbound.New(outbound.Cfg{...})`) with a typed `*Error` carrying a status, a user-facing message, and a `Kind`. The only package that imports `golang.org/x/time/rate`.
-- **problemjson/** — RFC 9457 (`application/problem+json`) error-response `Writer`, built with `problemjson.New(problemjson.Cfg{...})`. `ModeMasked` strips detail in production; `WriteUpstream` bridges an `outbound.*Error` via the structural `UpstreamError` interface (no import) and, in dev mode, adds a `reason` field from an optional `UpstreamReason()` method.
+- **problemjson/** — RFC 9457 (`application/problem+json`) error-response `Writer`, built with `problemjson.New(problemjson.Cfg{...})`. `ModeMasked` strips detail in production, and the optional `Cfg.OnProblem` hook receives the unmasked `Problem` — `Details` (same `reference`) plus validation field errors, upstream reason, and upstream error — exactly once on every write path so the real detail can be logged; `WriteUpstream` bridges an `outbound.*Error` via the structural `UpstreamError` interface (no import) and, in dev mode, adds a `reason` field from an optional `UpstreamReason()` method.
 
 ### Key Design Decisions
 
 - **StatWriter** (`middleware/respwriter.go`) wraps `http.ResponseWriter` to intercept the status code and the error-response body. On an error status it simultaneously buffers the body for logging and forwards (tees) it to the client — this prevents reverse-proxy hangs when the upstream writes an error body. The middleware never replaces the body; success and 1xx responses pass through untouched.
-- **Streaming**: StatWriter implements `Flush`/`FlushError` and `Hijack` directly (not just `Unwrap`), so handlers using either `http.ResponseController` or the older `w.(http.Flusher)` / `w.(http.Hijacker)` type assertions can stream. The first flush calls `releaseInterception`, which commits the status code and switches to passthrough (error bodies are already teed as they are written, so nothing is buffered-but-unsent). `Streaming()` reports that the response has been streamed or hijacked and `Started()` reports that any bytes have been committed (any `Write` latches the status); panic recovery synthesises a 500 only when neither holds, so it never appends `Internal Server Error` over a body the client has already begun receiving — matching net/http. After a hijack, `flushHeader` writes nothing.
-- **Error classification**: `IsStatusError()` (>= 400) vs `IsServerErr()` (>= 500) drives log levels — server errors log at ERROR, client errors at INFO. 1xx informational responses (e.g. 103 Early Hints) pass through without latching the status.
-- **Panic recovery re-panics on `http.ErrAbortHandler`**: it's net/http's sentinel to abort a response so the client detects truncation (`ReverseProxy` uses it when the upstream dies mid-copy); swallowing it would make truncated responses look complete.
-- **Bounded Prometheus label cardinality** (in `middleware/metrics`): the `addr` label uses `r.Pattern` (route pattern, requires Go 1.23 for the `r.Pattern` field) when matched, else the constant `"unmatched"` — never the raw request path. The `method` label is normalised to the standard HTTP methods, else `"other"`. Both cap the label sets so an attacker cannot mint unbounded time-series (net/http accepts any token as a method, and a non-pattern mux leaves `r.Pattern` empty).
+- **Streaming**: StatWriter implements `Flush`/`FlushError` and `Hijack` directly (not just `Unwrap`), so handlers using either `http.ResponseController` or the older `w.(http.Flusher)` / `w.(http.Hijacker)` type assertions can stream. The first flush calls `releaseInterception`, which commits the status code and switches to passthrough (error bodies are already teed as they are written, so nothing is buffered-but-unsent). `Streaming()` reports that the response has been streamed or hijacked and `Started()` reports that any bytes have been committed (any `Write` latches the status); panic recovery synthesises a 500 only when neither holds, so it never appends `Internal Server Error` over a body the client has already begun receiving. When either holds it still logs the panic and records the request, then re-panics with `http.ErrAbortHandler` so net/http aborts the connection and the client detects the truncation — matching net/http without recovery. After a hijack, `flushHeader` writes nothing.
+- **Error classification**: `IsStatusError()` (>= 400) vs `IsServerErr()` (>= 500) drives log levels — server errors log at ERROR, client errors at INFO. 1xx informational responses (e.g. 103 Early Hints) pass through without latching the status; 101 Switching Protocols is the exception and latches like a final status, and a hijack with no status written records 101 for logs/metrics.
+- **Panic recovery re-panics on `http.ErrAbortHandler`**: it's net/http's sentinel to abort a response so the client detects truncation (`ReverseProxy` uses it when the upstream dies mid-copy); swallowing it would make truncated responses look complete. Every path that aborts the connection (a panic without recovery, `http.ErrAbortHandler`, `runtime.Goexit`) goes through `recordAborted`: if no status was committed (`!Started() && !Streaming()`), the request is recorded — logged at ERROR and observed — as 500, never the StatWriter's default 200 (the client got a reset connection); nothing is written. A status already committed (flushed or written 200, 101 after a hijack) is recorded unchanged.
+- **Bounded Prometheus label cardinality** (in `middleware/metrics`): the `addr` label uses the route resolved by `middleware.Route` when matched, else the constant `"unmatched"` — never the raw request path. The `method` label is normalised to the standard HTTP methods, else `"other"`. Both cap the label sets so an attacker cannot mint unbounded time-series (net/http accepts any token as a method, and a non-pattern mux leaves `r.Pattern` empty).
+- **Route resolution** (`middleware/route.go`): only when `Metrics` is set (nothing else reads the route, and it costs a request copy), `Wrap` passes down a request copy carrying a pointer "route slot" in its context and hands that same copy to the `Observer` — `http.ServeMux` writes `r.Pattern` onto the request it receives, so using the caller's original would lose every stdlib route. A deferred restore copies `r.Pattern` back onto the caller's request (path values are unexported and can't be), so wrappers outside `Wrap` still read it. Because the slot is a pointer, `SetRoute` on any further copy (gorilla/chi copy on match) still reaches it; the slot's mutex covers `SetRoute` from another goroutine (`http.TimeoutHandler`). A wrapper between `Wrap` and a `ServeMux` that copies the request (`r.WithContext` in auth/tracing middleware, `TimeoutHandler`, `MaxBytesHandler`, `StripPrefix`) hides the mux's `r.Pattern` from `Wrap`; the recommended setup is `middleware.CaptureRoute` placed directly around the mux. Around an `*http.ServeMux` it looks the pattern up with `ServeMux.Handler` *before* dispatch (same `findHandler` result `ServeHTTP` writes to `r.Pattern`), so a request `TimeoutHandler` times out — observed while the handler goroutine still runs — keeps its route; around any other handler it records `r.Pattern` after it returns (deferred, so also on panic). The context value is a `routeScope{slot, depth}`: each `CaptureRoute` hands its handler a copy one level deeper (and copies that copy's `r.Pattern` back, also on panic), and both reporters are ranked by that depth, not write time (`SetRoute` records `setDepth`, a capture `capturedDepth`; each is overwritten only by a same-or-deeper report). After the handler, the route is resolved by one rule: the innermost reporter wins — `SetRoute` or a `CaptureRoute` whose mux matched — whether it reports before or after dispatch (gorilla's `Use` before, chi's glue after), so a nested mux under `StripPrefix` beats an outer router's template; at equal depth `SetRoute` wins; empty reports (a mux miss, `SetRoute(r, "")`) are ignored and claim no depth, so an inner miss keeps the outer template; with no report it falls back to `r.Pattern`. The inner pattern is relative to the stripped prefix (the template syntaxes differ, so they are deliberately not joined). The result is then mapped through `Cfg.RouteGroups` (whole-segment path prefixes, longest first), and stored for `middleware.Route`; nested middlewares share one slot and each resolves with its own groups. Routers report with `SetRoute`, `ServeMux` through `CaptureRoute`, and the `Observer` interface is unchanged so any backend reads the same route.
 - **Backend-agnostic seams**: the combined middleware depends on the `Observer` (metrics) and `Logger` (logging) interfaces, not concrete types. `*slog.Logger` satisfies `Logger` structurally; `metrics.NewObserver` supplies `Observer`. This keeps `middleware` free of any third-party dependency — Prometheus is compiled only when a consumer imports `middleware/metrics`.
 - **Bounded log buffer**: error-response bodies are captured for logging in an unexported `middleware.limitBuf` (cap `bufMaxBytes` = 2000) that *composes* (not embeds) `bytes.Buffer` and behaves like a capped `io.Discard` — writes past the cap are dropped and flagged via `Truncated()`, so a large error body cannot grow middleware memory without bound. Composition (not embedding) is deliberate: it prevents a promoted `bytes.Buffer` method (`WriteString`, `ReadFrom`, …) from bypassing the cap.
+- **Log hygiene** (`middleware/slog.go`, `middleware/clientip.go`): the `url` field is `r.URL.EscapedPath()` — the query string is never logged (tokens, signatures). The `ip` field comes from `Cfg.ClientIP`, a plain `func(*http.Request) string` like `Cfg.RequestID`; the default `ClientIPDirect` ignores forwarding headers because clients can forge them. `ClientIPFromRealIP`/`ClientIPFromXFF` believe headers only when the direct peer is in a trusted `netip.Prefix` list, and XFF is walked right to left past trusted hops (the leftmost entry is client-controlled). `ClientIPFromXFFHops` picks by position for proxies without stable ranges and does not check the peer.
 - **Constructor convention**: configurable constructors take a single `Cfg` struct — `middleware.New`, `outbound.New`, `problemjson.New`, `spa.New`, and `metrics.NewObserver` — so adjacent same-typed arguments can't be transposed and options can grow without breaking callers. The low-level `middleware.NewWriter(w)` is the deliberate exception (a single wrapper argument, like `bufio.NewWriter`).
 - **spa dotfile blocking & prefix normalisation** (`spa/spa.go`): `http.FileServerFS` serves dotfiles, so `spa.Handler` returns 404 for any path segment beginning with `.` — except the standard `.well-known/` tree — so `/.env` or `/.git/config` can't leak over an `os.DirFS` build directory. A plain `//go:embed dir` already excludes dotfiles from the binary; the serve-time block covers `os.DirFS` and `all:` embeds. `PathPrefix` is normalised in the constructor (`ui`, `/ui`, `/ui/` → `/ui`), so a missing leading slash can't silently route every asset to `index.html`.
 - **Upstream-failure presentation seam**: `outbound` owns the failure taxonomy (`Kind` → HTTP status); `problemjson` owns presentation (status → slug/title). The HTTP status is the deliberate contract between them, so several `Kind`s collapse to one 502 slug by design — the finer `Kind` stays in server logs, and in `problemjson` dev mode it also appears in the `reason` field (read via an optional `UpstreamReason()` method). Neither package imports the other.

Makefile

@@ -6,8 +6,8 @@ default: help
 #==========================================================================================
 ##@ Testing
 #==========================================================================================
-test: ## run fast go tests
-	@go test ./... -cover
+test: ## run go tests with the race detector (needs cgo)
+	@go test -race ./... -cover
 
 lint: ## run go linter
 	# depends on https://github.com/golangci/golangci-lint
@@ -23,21 +23,22 @@ license-check: ## check for invalid licenses
 
 COVERAGE_THRESHOLD ?= 70
 .PHONY: coverage
-coverage: ## check code coverage per package
-	@out=$$(go test -cover -covermode=atomic $$(go list ./middleware/... ./spa/...)) || { echo "$$out"; exit 1; }; \
+coverage: ## check code coverage of every package
+	@out=$$(go test -cover -covermode=atomic $$(go list ./...)) || { echo "$$out"; exit 1; }; \
 	echo "$$out" | awk -v threshold=$(COVERAGE_THRESHOLD) ' \
-		/\[no test files\]/ { printf "⚠️  %-70s no test files\n", $$2; next } \
+		/\[no test files\]/ { printf "❌ %-70s no test files (0%%, below %s%%)\n", $$2, threshold; fail = 1; next } \
 		/coverage:/ { \
+			pkg = ($$1 == "ok") ? $$2 : $$1; \
 			for (i = 1; i <= NF; i++) if ($$i == "coverage:") { cov = $$(i+1); sub(/%/, "", cov); break }; \
-			if (cov + 0 < threshold) { printf "❌ %-70s %s%% (below %s%%)\n", $$2, cov, threshold; fail = 1 } \
-			else { printf "✅ %-70s %s%%\n", $$2, cov } \
+			if (cov + 0 < threshold) { printf "❌ %-70s %s%% (below %s%%)\n", pkg, cov, threshold; fail = 1 } \
+			else { printf "✅ %-70s %s%%\n", pkg, cov } \
 		} \
 		END { exit fail }'
 
 .PHONY: verify
 verify: ## run all checks; runs every check and fails if any fail
 	@fail=0; \
-	for target in test license-check lint benchmark coverage; do \
+	for target in test license-check lint coverage; do \
 		echo "==================== make $$target ===================="; \
 		$(MAKE) --no-print-directory $$target || fail=1; \
 	done; \