From 4a1abafd8f09f22d9880842b7a74afc931cc2ca3 Mon Sep 17 00:00:00 2001 From: Mathias Date: Mon, 5 Oct 2026 09:41:30 +0200 Subject: [PATCH] fix(ingestion): build /mcp's bearer and the metrics wrap inside buildMux /mcp's bearer was built in main.go and the buildMux tests used stubs, so dropping it would have kept CI green. buildMux now takes the MCP handler and wraps it with the same BearerMiddleware and credential as the REST API. "/mcp" and "GET /metrics" left the public allowlist, so passing either as a public route is refused. buildMux also registers GET /metrics and returns the metrics-wrapped handler, so main passes its result to ListenAndServe unchanged. Flip-tested: registering /mcp unwrapped turns TestBuildMuxRefusesMCPWithoutAValidToken red, and dropping the metrics wrap turns the four metrics tests red. Comments now claim only what is tested. The ListenAndServe line and the symlink tripwire wiring in main (#154 comment) are still untested and tracked in #182. Closes #154 Co-Authored-By: Claude Opus 5.5 --- ingestion/cmd/server/jwt_validator_test.go | 5 +- ingestion/cmd/server/main.go | 29 ++++----- ingestion/cmd/server/routes.go | 49 +++++++++----- .../cmd/server/routes_observability_test.go | 14 ++-- ingestion/cmd/server/routes_test.go | 65 ++++++++++++++++--- 5 files changed, 109 insertions(+), 53 deletions(-) diff --git a/ingestion/cmd/server/jwt_validator_test.go b/ingestion/cmd/server/jwt_validator_test.go index 5b04ede..448673d 100644 --- a/ingestion/cmd/server/jwt_validator_test.go +++ b/ingestion/cmd/server/jwt_validator_test.go @@ -12,6 +12,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/mathiasbq/hyperguild/ingestion/internal/metrics" ) // fakeIssuer answers OIDC discovery with 502 for the first fail requests, as @@ -73,7 +75,8 @@ func TestBuildMuxAnswers503ToAJWTWhileTheIssuerIsUnresolved(t *testing.T) { srv, _ := fakeIssuer(t, 1000) v, err := buildJWTValidator(context.Background(), slog.New(slog.DiscardHandler), srv.URL, "brain") require.NoError(t, err) - mux, err := buildMux(restAuth{token: routesTok, validator: v, realm: "brain"}, testHandler(t), nil) + mux, err := buildMux(restAuth{token: routesTok, validator: v, realm: "brain"}, testHandler(t), + okHandler("mcp"), metrics.New(), nil) require.NoError(t, err) code, _ := do(t, mux, "GET", "/pending", "Bearer aaa.bbb.ccc") assert.Equal(t, http.StatusServiceUnavailable, code) diff --git a/ingestion/cmd/server/main.go b/ingestion/cmd/server/main.go index ca58b1d..65c48eb 100644 --- a/ingestion/cmd/server/main.go +++ b/ingestion/cmd/server/main.go @@ -516,8 +516,6 @@ func main() { } } - public["/mcp"] = chassisauth.BearerMiddleware(mcpToken, jwtValidator, "brain", resourceMetadataURL, mcpSrv) - // POST /capture (#53/#54): the uniform capture REST door. Needs a ticket // tracker to file action items, so it only mounts when Gitea is // configured. It reuses the MCP server's graph-wired brain store (one @@ -541,7 +539,7 @@ func main() { public["POST /capture"] = captureH // Same use-case behind the MCP `capture` tool (#55 relay) so MCP-native // harnesses (claude.ai, Crush, Pi, LLM Council) reach capture through - // the existing /mcp OAuth connector. mcpSrv is already wrapped above; + // the existing /mcp OAuth connector. buildMux wraps this same mcpSrv; // WithCapture mutates the same instance, so the tool appears live. mcpSrv.WithCapture(captureSvc, jwtValidator, mcpToken, "local-cli", resolver) // hyperguild#97: a governance-file rewrite gets the same trail a @@ -583,19 +581,16 @@ func main() { os.Exit(1) } - // /metrics — unauthenticated Prometheus endpoint. kube-prometheus-stack - // scrapes it via the ServiceMonitor in k3s/apps/supervisor/. The metrics - // middleware below wraps every other registered handler so it observes - // real request latency. /metrics itself is excluded from its own - // observation by registering it on the outer mux (post-wrap). - reg := metrics.New() - public["GET /metrics"] = http.HandlerFunc(reg.Handler()) - - // The whole router, built in one place so the test of buildMux is a test - // of production wiring (PR #152 review). The REST API's credential is the - // same as /mcp's. - mux, err := buildMux(restAuth{token: mcpToken, validator: jwtValidator, realm: "brain", - resourceMetadataURL: resourceMetadataURL}, h, public) + // /metrics — unauthenticated Prometheus endpoint, scraped via the + // ServiceMonitor in k3s/apps/supervisor/. buildMux registers it and wraps + // the whole router in the registry's middleware. + // + // The whole router, /mcp's bearer included, is built in buildMux so its + // tests are tests of production wiring (PR #152 review, hyperguild#154). + // The REST API's credential is the same as /mcp's. ListenAndServe below + // must serve its result unchanged: that one line is the untested part. + handler, err := buildMux(restAuth{token: mcpToken, validator: jwtValidator, realm: "brain", + resourceMetadataURL: resourceMetadataURL}, h, mcpSrv, metrics.New(), public) if err != nil { logger.Error("build router", "err", err) os.Exit(1) @@ -616,7 +611,7 @@ func main() { "watch_interval", watchIntervalLog, "mcp_enabled", true, ) - if err := http.ListenAndServe(addr, reg.Middleware(mux)); err != nil { + if err := http.ListenAndServe(addr, handler); err != nil { logger.Error("server stopped", "err", err) os.Exit(1) } diff --git a/ingestion/cmd/server/routes.go b/ingestion/cmd/server/routes.go index f321a36..cd76b6f 100644 --- a/ingestion/cmd/server/routes.go +++ b/ingestion/cmd/server/routes.go @@ -8,22 +8,22 @@ import ( chassisauth "git.d-ma.be/mathias/mcp-chassis/auth" "github.com/mathiasbq/hyperguild/ingestion/internal/api" + "github.com/mathiasbq/hyperguild/ingestion/internal/metrics" ) -// publicRoutes is the complete set of patterns allowed on the outer mux, -// outside the REST API's auth (hyperguild#149). Each authenticates itself or is -// meant to be open: /mcp (bearer/JWT), /capture (bearer/JWT), the webhook -// (HMAC), /oauth/token (client secret), OAuth metadata and /metrics (open). -// A REST pattern added here would beat the authenticated catch-all by -// specificity and ship open, which is why buildMux refuses anything else. +// publicRoutes is the complete set of patterns a caller may register on the +// outer mux, outside the REST API's auth (hyperguild#149). Each authenticates +// itself or is meant to be open: /capture (bearer/JWT), the webhook (HMAC), +// /oauth/token (client secret) and OAuth metadata (open). /mcp and /metrics +// are not here: buildMux builds them itself (hyperguild#154). A REST pattern +// added here would beat the authenticated catch-all by specificity and ship +// open, which is why buildMux refuses anything else. var publicRoutes = map[string]bool{ - "/mcp": true, "POST /capture": true, "POST /webhooks/brain-sync": true, "GET /.well-known/oauth-protected-resource": true, "GET /.well-known/oauth-authorization-server": true, "POST /oauth/token": true, - "GET /metrics": true, } // restAuth is the credential the REST API checks: the same as /mcp's. @@ -34,16 +34,24 @@ type restAuth struct { resourceMetadataURL string } -// buildMux is the server's whole router. The REST API is mounted at "/" -// behind bearer auth that buildMux constructs itself, so a caller cannot pass -// a no-op; public handlers are registered as given, and only for patterns on -// the publicRoutes allowlist. It returns an http.Handler, not the -// *ServeMux, so nothing can register a route on the result afterwards and -// bypass the allowlist. -func buildMux(auth restAuth, h *api.Handler, public map[string]http.Handler) (http.Handler, error) { +// buildMux builds the handler main passes to ListenAndServe unchanged. The +// REST API is mounted at "/" and /mcp is registered, both behind bearer auth +// that buildMux constructs itself from auth (one credential), so a caller +// cannot pass a no-op. GET /metrics serves reg, and the whole router is +// wrapped in reg's middleware. Other public handlers are registered as given, +// and only for patterns on the publicRoutes allowlist. It returns an +// http.Handler, not the *ServeMux, so nothing can register a route on the +// result. What stays untested is main's own line: a handler main built around +// this one before ListenAndServe would not be seen by these tests. +func buildMux( + auth restAuth, h *api.Handler, mcpHandler http.Handler, reg *metrics.Registry, public map[string]http.Handler, +) (http.Handler, error) { if auth.token == "" { return nil, errors.New("buildMux: REST auth needs a token") } + if mcpHandler == nil || reg == nil { + return nil, errors.New("buildMux: needs the MCP handler and the metrics registry") + } mux := http.NewServeMux() for pattern, handler := range public { if !publicRoutes[pattern] { @@ -51,10 +59,15 @@ func buildMux(auth restAuth, h *api.Handler, public map[string]http.Handler) (ht } mux.Handle(pattern, handler) } - mountREST(mux, h, func(next http.Handler) http.Handler { + bearer := func(next http.Handler) http.Handler { return chassisauth.BearerMiddleware(auth.token, auth.validator, auth.realm, auth.resourceMetadataURL, next) - }) - return mux, nil + } + mux.Handle("/mcp", bearer(mcpHandler)) + // /metrics is registered on the mux and so observed by its own + // middleware, like any route. + mux.Handle("GET /metrics", reg.Handler()) + mountREST(mux, h, bearer) + return reg.Middleware(mux), nil } // mountREST registers the REST API on its own mux and mounts it at "/" behind diff --git a/ingestion/cmd/server/routes_observability_test.go b/ingestion/cmd/server/routes_observability_test.go index 1dd6643..93bd3fb 100644 --- a/ingestion/cmd/server/routes_observability_test.go +++ b/ingestion/cmd/server/routes_observability_test.go @@ -9,8 +9,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - - "github.com/mathiasbq/hyperguild/ingestion/internal/metrics" ) // wrongTokSentinel is a presented credential distinctive enough that any @@ -53,8 +51,8 @@ func TestBuildMuxAcceptedLogLineOmitsTheBareStaticToken(t *testing.T) { // An authenticated REST request is labelled with the REST mux's pattern: // the inner mux sets it on the same *http.Request the outer middleware reads. func TestMetricsLabelAnAuthenticatedRESTRequestWithItsRoutePattern(t *testing.T) { - reg := metrics.New() - code, _ := do(t, reg.Middleware(testMux(t)), "GET", "/pending", "Bearer "+routesTok) + mux, reg := testMuxAndRegistry(t) + code, _ := do(t, mux, "GET", "/pending", "Bearer "+routesTok) require.Equal(t, http.StatusOK, code) rec := httptest.NewRecorder() @@ -67,8 +65,8 @@ func TestMetricsLabelAnAuthenticatedRESTRequestWithItsRoutePattern(t *testing.T) // so it was labelled path="/" and a 4xx on /query could not be told from one // on /write. It is now labelled with the REST pattern it was addressed to. func TestMetricsLabelARejectedRESTRequestWithItsRoutePattern(t *testing.T) { - reg := metrics.New() - code, _ := do(t, reg.Middleware(testMux(t)), "POST", "/query", "") + mux, reg := testMuxAndRegistry(t) + code, _ := do(t, mux, "POST", "/query", "") require.Equal(t, http.StatusUnauthorized, code) rec := httptest.NewRecorder() @@ -79,8 +77,8 @@ func TestMetricsLabelARejectedRESTRequestWithItsRoutePattern(t *testing.T) { // A path no REST route matches keeps the catch-all label, so an attacker // probing random URLs cannot grow the series set. func TestMetricsLabelARejectedUnknownPathWithTheCatchAll(t *testing.T) { - reg := metrics.New() - code, _ := do(t, reg.Middleware(testMux(t)), "POST", "/no-such-route-zq", "") + mux, reg := testMuxAndRegistry(t) + code, _ := do(t, mux, "POST", "/no-such-route-zq", "") require.Equal(t, http.StatusUnauthorized, code) rec := httptest.NewRecorder() diff --git a/ingestion/cmd/server/routes_test.go b/ingestion/cmd/server/routes_test.go index 4455cb5..ae9674e 100644 --- a/ingestion/cmd/server/routes_test.go +++ b/ingestion/cmd/server/routes_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "github.com/mathiasbq/hyperguild/ingestion/internal/api" + "github.com/mathiasbq/hyperguild/ingestion/internal/metrics" "github.com/mathiasbq/hyperguild/ingestion/internal/pipeline" ) @@ -22,19 +23,26 @@ func okHandler(body string) http.Handler { } // testMux builds the router exactly as main does: buildMux with the full -// public set main registers (stub handlers) and real bearer config. -// hyperguild#149: the REST API is authenticated as a whole; the PR #152 -// review showed a test of a COPY of the router let main.go regress unseen. +// public set main registers (stub handlers), a stub MCP server, and real +// bearer config. hyperguild#149: the REST API is authenticated as a whole; +// the PR #152 review showed a test of a COPY of the router let main.go +// regress unseen. func testMux(t *testing.T) http.Handler { t.Helper() - h := testHandler(t) + mux, _ := testMuxAndRegistry(t) + return mux +} + +func testMuxAndRegistry(t *testing.T) (http.Handler, *metrics.Registry) { + t.Helper() public := map[string]http.Handler{} for p := range publicRoutes { public[p] = okHandler("public " + p) } - mux, err := buildMux(restAuth{token: routesTok, realm: "brain"}, h, public) + reg := metrics.New() + mux, err := buildMux(restAuth{token: routesTok, realm: "brain"}, testHandler(t), okHandler("mcp"), reg, public) require.NoError(t, err) - return mux + return mux, reg } func testHandler(t *testing.T) *api.Handler { @@ -98,14 +106,53 @@ func TestBuildMuxDoesNotWrapPublicRoutes(t *testing.T) { // over the authenticated catch-all (more specific pattern), so it is refused. func TestBuildMuxRefusesAPublicPatternOffTheAllowlist(t *testing.T) { h := api.NewHandler(t.TempDir(), slog.New(slog.NewTextHandler(os.Stderr, nil)), pipeline.Config{}) - for _, p := range []string{"POST /query", "GET /pending", "/", "POST /metrics"} { - _, err := buildMux(restAuth{token: routesTok, realm: "brain"}, h, map[string]http.Handler{p: okHandler("x")}) + // "/mcp" and "GET /metrics" are built by buildMux itself (hyperguild#154): + // passing either as public would register them unauthenticated or twice. + for _, p := range []string{"POST /query", "GET /pending", "/", "POST /metrics", "/mcp", "GET /metrics"} { + _, err := buildMux(restAuth{token: routesTok, realm: "brain"}, h, okHandler("mcp"), metrics.New(), + map[string]http.Handler{p: okHandler("x")}) assert.Error(t, err, "public pattern %q must be refused", p) } } func TestBuildMuxRefusesAnEmptyToken(t *testing.T) { h := api.NewHandler(t.TempDir(), slog.New(slog.NewTextHandler(os.Stderr, nil)), pipeline.Config{}) - _, err := buildMux(restAuth{token: "", realm: "brain"}, h, nil) + _, err := buildMux(restAuth{token: "", realm: "brain"}, h, okHandler("mcp"), metrics.New(), nil) assert.Error(t, err) } + +// hyperguild#154: /mcp's bearer was built in main.go, untested; the buildMux +// tests used stubs. buildMux now wraps the MCP handler itself, with the same +// credential as the REST API. +func TestBuildMuxRefusesMCPWithoutAValidToken(t *testing.T) { + r := testMux(t) + for _, authz := range []string{"", "Bearer wrong"} { + code, body := do(t, r, "POST", "/mcp", authz) + assert.Equal(t, http.StatusUnauthorized, code, "authz %q", authz) + assert.NotEqual(t, "mcp", body) + } + code, body := do(t, r, "POST", "/mcp", "Bearer "+routesTok) + assert.Equal(t, http.StatusOK, code) + assert.Equal(t, "mcp", body) +} + +func TestBuildMuxRefusesANilMCPHandlerOrRegistry(t *testing.T) { + auth := restAuth{token: routesTok, realm: "brain"} + _, err := buildMux(auth, testHandler(t), nil, metrics.New(), nil) + assert.Error(t, err) + _, err = buildMux(auth, testHandler(t), okHandler("mcp"), nil, nil) + assert.Error(t, err) +} + +// hyperguild#154: main wrapped buildMux's result in the metrics middleware +// before ListenAndServe, untested. buildMux now returns the handler main +// serves as-is, metrics included: a request through it is observed and +// exposed on its own GET /metrics. +func TestBuildMuxObservesRequestsAndServesMetrics(t *testing.T) { + r := testMux(t) + code, _ := do(t, r, "GET", "/pending", "Bearer "+routesTok) + require.Equal(t, http.StatusOK, code) + code, body := do(t, r, "GET", "/metrics", "") + require.Equal(t, http.StatusOK, code) + assert.Contains(t, body, `path="GET /pending"`) +}