rest: route-label the direct mux.Handle registrations (#166)

rest/metrics.go

@@ -34,7 +34,7 @@ var (
 		prometheus.CounterOpts{
 			Name: "api_http_requests_total",
 			Help: "API requests by mux route pattern, method, HTTP status, and validated key id. " +
-				"route is empty when the request never reached routing (auth 401s, datastore 503); " +
+				"route is empty when the request never reached routing (the auth 401/503 tiers, or a pre-routing panic); " +
 				"\"/\" is the catch-all (unknown path / wrong method / clean-path 307). " +
 				"key_id is empty when no key validated.",
 		},
@@ -45,7 +45,7 @@ var (
 		prometheus.HistogramOpts{
 			Name: "api_http_request_duration_seconds",
 			Help: "API request latency by mux route pattern and method. " +
-				"route is empty when the request never reached routing (auth 401s, datastore 503); " +
+				"route is empty when the request never reached routing (the auth 401/503 tiers, or a pre-routing panic); " +
 				"\"/\" is the catch-all (unknown path / wrong method / clean-path 307). " +
 				"Deliberately NO key_id label (cardinality discipline).",
 			Buckets: prometheus.DefBuckets,
@@ -102,7 +102,11 @@ func MetricsHandler() http.Handler {
 // validation, and the route slot is filled from r.Pattern inside the mux,
 // where the matched pattern is actually set.
 type metricLabels struct {
-	route string // mux pattern, e.g. "GET /api/v1/milpacs/ranks"; "" if never routed
+	// route is the matched mux pattern, e.g. "GET /api/v1/milpacs/ranks".
+	// "" means exactly one thing: the request never reached routing (the
+	// auth 401/503 tiers, or a pre-routing panic) — EVERY registration fills
+	// the slot, including the direct mux.Handle ones outside handle() (#166).
+	route string
 	keyID string // decimal key id, e.g. "101"; "" if no key validated
 }
 

rest/metrics_test.go

@@ -359,7 +359,7 @@ func TestMetrics_WrongMethod405MetersUnderCatchAll(t *testing.T) {
 // The clean-path 307 (ruled, #128 round 3) meters like the fallback's
 // 404s/405s: the bounded catch-all "/" route label — never the raw unclean
 // path (attacker-controlled cardinality) and never "" (the label documented
-// as "auth rejected before routing": this request authenticated, so its key
+// as "never reached routing": this request authenticated, so its key
 // id attributes the redirect). Before the ruling fix the redirect bypassed
 // routeLabel and metered under that empty label.
 func TestMetrics_CleanPath307MetersUnderCatchAllWithKeyId(t *testing.T) {
@@ -380,6 +380,99 @@ func TestMetrics_CleanPath307MetersUnderCatchAllWithKeyId(t *testing.T) {
 	assert.Equal(t, before+1, after, "clean-path 307s must meter under the catch-all pattern with the key id")
 }
 
+// The scoped-resource registration handle() cannot serve — the
+// {ticket_id}/{sub} dispatcher (ticketSubResource) — carries the same
+// routeLabel wrap as every handle()-registered route: a messages 200 meters
+// under its registration pattern. Before #166 this route's traffic metered
+// under route="", conflating a real route with "rejected before routing ever
+// happened".
+func TestMetrics_TicketMessagesMetersUnderSubResourcePattern(t *testing.T) {
+	h := newStack(t)
+	labels := map[string]string{
+		"route":  "GET /api/v1/tickets/{ticket_id}/{sub}",
+		"method": "GET",
+		"status": "200",
+		"key_id": "102",
+	}
+
+	before := counterValue(t, scrapeMetrics(t), "api_http_requests_total", labels)
+
+	rr := do(h, http.MethodGet, "/api/v1/tickets/42/messages", "cav7_ticketskey")
+	require.Equal(t, http.StatusOK, rr.Code)
+
+	after := counterValue(t, scrapeMetrics(t), "api_http_requests_total", labels)
+	assert.Equal(t, before+1, after, "messages 200s must meter under the sub-resource registration pattern, never route=\"\"")
+}
+
+// Wrap order on the sub-resource registration: routeLabel sits OUTSIDE the
+// scope gate (same order handle() applies), so a wrong-scope 403 on the
+// messages route still meters under the route it was denied on, with the
+// denied key's id — per-key error attribution, like every handle()-registered
+// route.
+func TestMetrics_TicketMessagesScopeDenialMetersUnderSubResourcePattern(t *testing.T) {
+	h := newStack(t)
+	labels := map[string]string{
+		"route":  "GET /api/v1/tickets/{ticket_id}/{sub}",
+		"method": "GET",
+		"status": "403",
+		"key_id": "101",
+	}
+
+	before := counterValue(t, scrapeMetrics(t), "api_http_requests_total", labels)
+
+	rr := do(h, http.MethodGet, "/api/v1/tickets/42/messages", "cav7_readkey") // read ≠ read:tickets
+	require.Equal(t, http.StatusForbidden, rr.Code)
+
+	after := counterValue(t, scrapeMetrics(t), "api_http_requests_total", labels)
+	assert.Equal(t, before+1, after, "messages scope 403s must meter under the denied route with the key id")
+}
+
+// The dispatcher's own 404 (a {sub} that is not "messages") ROUTED — the mux
+// matched the {ticket_id}/{sub} registration — so it meters under that bounded
+// pattern, not the catch-all "/" (which never matched) and never route=""
+// (which means the request never reached routing at all).
+func TestMetrics_TicketUnknownSub404MetersUnderSubResourcePattern(t *testing.T) {
+	h := newStack(t)
+	labels := map[string]string{
+		"route":  "GET /api/v1/tickets/{ticket_id}/{sub}",
+		"method": "GET",
+		"status": "404",
+		"key_id": "102",
+	}
+
+	before := counterValue(t, scrapeMetrics(t), "api_http_requests_total", labels)
+
+	rr := do(h, http.MethodGet, "/api/v1/tickets/42/attachments", "cav7_ticketskey")
+	require.Equal(t, http.StatusNotFound, rr.Code)
+
+	after := counterValue(t, scrapeMetrics(t), "api_http_requests_total", labels)
+	assert.Equal(t, before+1, after, "unknown-sub 404s must meter under the sub-resource registration pattern")
+}
+
+// The other direct mux.Handle registration #166 wrapped — the scope-independent
+// /tickets/ref/messages parity shim — is route-labeled too: its frozen 400
+// meters under its literal pattern (a bounded label), never route="". With
+// both #166 registrations wrapped (the catch-all was already labeled),
+// route="" means exactly one thing across the whole table: the request never
+// reached routing.
+func TestMetrics_TicketsRefMessagesFrozen400MetersUnderItsPattern(t *testing.T) {
+	h := newStack(t)
+	labels := map[string]string{
+		"route":  "GET /api/v1/tickets/ref/messages",
+		"method": "GET",
+		"status": "400",
+		"key_id": "101",
+	}
+
+	before := counterValue(t, scrapeMetrics(t), "api_http_requests_total", labels)
+
+	rr := do(h, http.MethodGet, "/api/v1/tickets/ref/messages", "cav7_readkey")
+	require.Equal(t, http.StatusBadRequest, rr.Code)
+
+	after := counterValue(t, scrapeMetrics(t), "api_http_requests_total", labels)
+	assert.Equal(t, before+1, after, "the ref/messages frozen 400 must meter under its literal pattern")
+}
+
 // counterFamilyTotal sums every child of the named counter family — the
 // family-wide request count, label-set independent.
 func counterFamilyTotal(families map[string]*dto.MetricFamily, name string) float64 {

rest/redirect.go

@@ -15,7 +15,7 @@ import (
 // served those forms as 200s; the ruling ACCEPTS the redirect but not its
 // incidental warts: net/http's RedirectHandler carries a default HTML body —
 // off-contract for an all-JSON API — and bypasses routeLabel, metering under
-// the empty label rest.go documents as "auth rejected before routing".
+// the empty label metrics.go reserves for "never reached routing".
 //
 // cleanPathRedirect answers the mux's would-be clean-path redirect IN FRONT of
 // the mux instead: same detection (the mux's own cleanPath semantics, copied
@@ -62,8 +62,8 @@ func cleanPathRedirect(next http.Handler) http.Handler {
 func writeCleanPathRedirect(w http.ResponseWriter, r *http.Request, cleaned string) {
 	// Meter like the fallback meters its 404s/405s: the bounded catch-all
 	// "/" route label — never the raw unclean path (attacker-controlled
-	// cardinality) and never "" (that label means auth rejected the request
-	// before routing; this request authenticated and reached routing's
+	// cardinality) and never "" (that label means the request never reached
+	// routing; this request authenticated and reached routing's
 	// doorstep). The key-id slot is already filled — auth runs outside this
 	// layer.
 	if labels := metricLabelsFromContext(r.Context()); labels != nil {

rest/rest.go

@@ -161,13 +161,17 @@ func routes(ds datastores.Datastore, rc datastores.TicketReferenceCache) *http.S
 	// lookup. Deliberately NOT scope-gated: the old 400 fired in the gateway
 	// before the RPC, so RequireScope never ran. No cacheControl wrap either:
 	// the shim answers nothing but the frozen 400, and only 200s carry the
-	// freshness signal.
-	mux.Handle("GET /api/v1/tickets/ref/messages", refMessagesParity())
-	mux.Handle(ticketSubPattern, ticketSubResource(ds))
+	// freshness signal. routeLabel DOES wrap it: the frozen 400 routed, so it
+	// meters under this literal pattern, never route="" (#166).
+	mux.Handle("GET /api/v1/tickets/ref/messages", routeLabel(refMessagesParity()))
+	// routeLabel OUTSIDE the dispatcher, mirroring handle()'s wrap order:
+	// everything the {ticket_id}/{sub} registration answers — the messages
+	// 200s, the scope 403s, the unknown-sub 404s — meters under its pattern.
+	mux.Handle(ticketSubPattern, routeLabel(ticketSubResource(ds)))
 
 	// The catch-all is route-labeled like every registered pattern: 404s and
 	// 405s meter under its "/" pattern — bounded, and distinct from "" (a
-	// request auth rejected before routing ever happened).
+	// request that never reached routing).
 	mux.Handle("/", routeLabel(fallback(mux)))
 
 	return mux
@@ -200,8 +204,9 @@ const ticketSubPattern = "GET /api/v1/tickets/{ticket_id}/{sub}"
 func ticketSubResource(ds datastores.Datastore) http.Handler {
 	// requireScope and cacheControl applied HERE because handle() cannot
 	// register this route — the two wraps stay explicit at the registration
-	// site. handle()'s third wrap, routeLabel, is absent (known pre-existing
-	// gap: messages meters under route="").
+	// site. handle()'s third wrap, routeLabel, wraps this dispatcher at its
+	// mux.Handle call in routes() — OUTSIDE the scope gate, the same order
+	// handle() applies, so even a 403 meters under this route (#166).
 	messages := requireScope("read:tickets", cacheControl(maxAgeTickets, listTicketMessages(ds)))
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if !knownTicketSub(r.PathValue("sub")) {