From bdc1e51a1a8e623456707ac7e227005ea4d2fb0b Mon Sep 17 00:00:00 2001 From: Mathias Date: Mon, 5 Oct 2026 17:48:44 +0000 Subject: [PATCH] fix(ingestion): build /mcp's bearer and the metrics wrap inside buildMux (#154) (#183) Co-authored-by: Mathias --- ingestion/cmd/server/jwt_validator_test.go | 33 ++++++++- ingestion/cmd/server/main.go | 31 ++++---- ingestion/cmd/server/routes.go | 50 ++++++++----- .../cmd/server/routes_observability_test.go | 14 ++-- ingestion/cmd/server/routes_test.go | 74 ++++++++++++++++--- 5 files changed, 146 insertions(+), 56 deletions(-) diff --git a/ingestion/cmd/server/jwt_validator_test.go b/ingestion/cmd/server/jwt_validator_test.go index 5b04ede..9793a00 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,8 +75,37 @@ 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) } + +// 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 ca58b1d..db721e4 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,18 @@ 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. 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 { logger.Error("build router", "err", err) os.Exit(1) @@ -616,7 +613,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..671d0f4 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,25 @@ 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 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) { 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 +60,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..be538fb 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" ) @@ -21,20 +22,29 @@ 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) 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() - 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 { @@ -79,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 { @@ -98,14 +109,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"`) +}