From 4a1abafd8f09f22d9880842b7a74afc931cc2ca3 Mon Sep 17 00:00:00 2001 From: Mathias Date: Mon, 5 Oct 2026 09:41:30 +0200 Subject: [PATCH 1/2] 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"`) +} -- 2.54.0 From 4c7fcb6bdc68cca7eadd7f117e8cd54dc898bb7c Mon Sep 17 00:00:00 2001 From: Mathias Date: Mon, 5 Oct 2026 16:34:16 +0200 Subject: [PATCH 2/2] test(ingestion): /mcp's JWT validator and resource_metadata; narrow comments PR #183 review (comment 15474): - TestBuildMuxAnswers503ToAJWTOnMCPWhileTheIssuerIsUnresolved: building /mcp's middleware with a nil validator turns it red. - TestBuildMuxMCP401CarriesResourceMetadataInWWWAuthenticate: building it with resourceMetadataURL "" turns it red. - main.go, routes.go and the testMux comment now say exactly what is untested: main's restAuth fields and public map passed to buildMux, and ListenAndServe (hyperguild#182). Stale public-route list in the TestBuildMuxDoesNotWrapPublicRoutes comment corrected. Co-Authored-By: Claude Opus 5.5 --- ingestion/cmd/server/jwt_validator_test.go | 28 ++++++++++++++++++++++ ingestion/cmd/server/main.go | 6 +++-- ingestion/cmd/server/routes.go | 5 ++-- ingestion/cmd/server/routes_test.go | 17 +++++++------ 4 files changed, 45 insertions(+), 11 deletions(-) diff --git a/ingestion/cmd/server/jwt_validator_test.go b/ingestion/cmd/server/jwt_validator_test.go index 448673d..9793a00 100644 --- a/ingestion/cmd/server/jwt_validator_test.go +++ b/ingestion/cmd/server/jwt_validator_test.go @@ -81,3 +81,31 @@ func TestBuildMuxAnswers503ToAJWTWhileTheIssuerIsUnresolved(t *testing.T) { code, _ := do(t, mux, "GET", "/pending", "Bearer aaa.bbb.ccc") assert.Equal(t, http.StatusServiceUnavailable, code) } + +// PR #183 review: /mcp shares REST's bearer, but only the static token was +// asserted on it. With the issuer unresolved, a JWT on /mcp answers 503 too, +// so /mcp's middleware was built with the JWT validator. +func TestBuildMuxAnswers503ToAJWTOnMCPWhileTheIssuerIsUnresolved(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), + okHandler("mcp"), metrics.New(), nil) + require.NoError(t, err) + code, _ := do(t, mux, "POST", "/mcp", "Bearer aaa.bbb.ccc") + assert.Equal(t, http.StatusServiceUnavailable, code) +} + +// PR #183 review: a 401 on /mcp names the protected-resource metadata in +// WWW-Authenticate (what the claude.ai OAuth connector discovers from), so +// /mcp's middleware was built with resourceMetadataURL. +func TestBuildMuxMCP401CarriesResourceMetadataInWWWAuthenticate(t *testing.T) { + const rm = "https://brain.example/.well-known/oauth-protected-resource" + mux, err := buildMux(restAuth{token: routesTok, realm: "brain", resourceMetadataURL: rm}, testHandler(t), + okHandler("mcp"), metrics.New(), nil) + require.NoError(t, err) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/mcp", nil)) + require.Equal(t, http.StatusUnauthorized, rec.Code) + assert.Contains(t, rec.Header().Get("WWW-Authenticate"), `resource_metadata="`+rm+`"`) +} diff --git a/ingestion/cmd/server/main.go b/ingestion/cmd/server/main.go index 65c48eb..db721e4 100644 --- a/ingestion/cmd/server/main.go +++ b/ingestion/cmd/server/main.go @@ -587,8 +587,10 @@ func main() { // // 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. + // The REST API's credential is the same as /mcp's. Untested here: the + // restAuth fields and the public map this call passes in (e.g. a nil + // validator would drop JWT auth on /mcp and REST and the suite stays + // green), and ListenAndServe serving the result unchanged (hyperguild#182). handler, err := buildMux(restAuth{token: mcpToken, validator: jwtValidator, realm: "brain", resourceMetadataURL: resourceMetadataURL}, h, mcpSrv, metrics.New(), public) if err != nil { diff --git a/ingestion/cmd/server/routes.go b/ingestion/cmd/server/routes.go index cd76b6f..671d0f4 100644 --- a/ingestion/cmd/server/routes.go +++ b/ingestion/cmd/server/routes.go @@ -41,8 +41,9 @@ type restAuth struct { // 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. +// result. What stays untested is main's call site: the restAuth fields and +// the public map main passes in, and ListenAndServe serving this handler +// unchanged (hyperguild#182). The tests build their own arguments. func buildMux( auth restAuth, h *api.Handler, mcpHandler http.Handler, reg *metrics.Registry, public map[string]http.Handler, ) (http.Handler, error) { diff --git a/ingestion/cmd/server/routes_test.go b/ingestion/cmd/server/routes_test.go index ae9674e..be538fb 100644 --- a/ingestion/cmd/server/routes_test.go +++ b/ingestion/cmd/server/routes_test.go @@ -22,11 +22,13 @@ func okHandler(body string) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte(body)) }) } -// testMux builds the router exactly as main does: buildMux with the full -// 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. +// testMux builds the router through buildMux, the function main calls, with +// every allowlisted public pattern (stub handlers), a stub MCP server and a +// static token only: no JWT validator and no resourceMetadataURL (the tests +// in jwt_validator_test.go pass those). It does not use main's own restAuth +// or public map (hyperguild#182). 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() mux, _ := testMuxAndRegistry(t) @@ -87,8 +89,9 @@ func TestBuildMuxAuthenticatesAnUnregisteredPath(t *testing.T) { } // buildMux registers public handlers as given, without adding auth: each -// public route owns its own (bearer on /mcp, HMAC on the webhook, client -// secret on /oauth/token) or is meant to be open (/metrics, OAuth metadata). +// public route owns its own (bearer/JWT on /capture, HMAC on the webhook, +// client secret on /oauth/token) or is meant to be open (OAuth metadata). +// /mcp and /metrics are not public routes: buildMux builds them itself. func TestBuildMuxDoesNotWrapPublicRoutes(t *testing.T) { r := testMux(t) for p := range publicRoutes { -- 2.54.0