From 7cc652d57e9b4fb8962318f36fdab737bb0b1be8 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Fri, 24 Jul 2026 19:45:25 +0300 Subject: [PATCH] fix(mcpsrv): gate read tools to owner+agents (spec-jjo) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MCP read tools (spec_search/spec_read/spec_list) enforced no ACL: anyone clearing the Host allowlist could read approved content. graph's /query and the web UI gate reads to owner+agents; /mcp did not. Add the same fail-closed gate to the whole MCP surface — initialize, tools/list and tools/call — mounted inside the resolver middleware so it sees the resolved principal. spec_propose was already fail-closed in service.Propose; this makes the read tools match. Pre-existing since Phase 2, cheap now that /mcp resolves a principal. Closes spec-jjo --- cmd/specsrht/main.go | 8 +++++- mcpsrv/gate_test.go | 68 ++++++++++++++++++++++++++++++++++++++++++++ mcpsrv/mcpsrv.go | 28 ++++++++++++++++++ 3 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 mcpsrv/gate_test.go diff --git a/cmd/specsrht/main.go b/cmd/specsrht/main.go index 0b8849f1687ac3e59d997aa900d2910086fec3bc..2bf5c1a2a853c72bfc738a17354321f932b1cedc 100644 --- a/cmd/specsrht/main.go +++ b/cmd/specsrht/main.go @@ -520,7 +520,13 @@ func newSurfaces(conf ini.File, cfg service.Config, svc *service.Service, versio // call, so /mcp needs the principal middleware the read tools never did. // It sets an anonymous principal when there is no token, which service.Propose // refuses — the ACL stays in service/, this only populates the identity. - mcp = svc.Resolver().Middleware()(mcp) + // + // mcpsrv.Gate sits inside that middleware and closes the read plane: the read + // tools (spec_search/spec_read/spec_list) enforced nothing on their own, so it + // applies the owner+agents ACL to the whole surface — the same one graph's + // /query and the web UI apply. spec_propose stays fail-closed in service/ too; + // the gate just makes the read tools match. + mcp = svc.Resolver().Middleware()(mcpsrv.Gate(mcp)) schema, err := graph.NewSchema(graph.Options{ Reader: svc, diff --git a/mcpsrv/gate_test.go b/mcpsrv/gate_test.go new file mode 100644 index 0000000000000000000000000000000000000000..9e40a7f40a67de78f0bbaaf7213e1cf30dfcbd20 --- /dev/null +++ b/mcpsrv/gate_test.go @@ -0,0 +1,68 @@ +package mcpsrv_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" + "sourcecraft.dev/bigbes/sr-ht-spec/mcpsrv" +) + +// The read plane is fail-closed: only the owner and its agents may reach the MCP +// tools. Before this gate the three read tools (spec_search/spec_read/spec_list) +// served approved content to anyone who cleared the Host allowlist — spec_propose +// was already fail-closed in service.Propose, but the reads checked nothing. The +// gate applies the owner+agents ACL to the whole surface, the same policy graph's +// /query and the web UI apply. +func TestGate(t *testing.T) { + cases := []struct { + name string + principal authn.Principal + wantCode int + wantNext bool + }{ + {"anonymous is refused", authn.Anonymous(), http.StatusUnauthorized, false}, + {"owner may read", authn.Principal{Kind: authn.KindOwner}, http.StatusOK, true}, + {"agent may read", authn.Principal{Kind: authn.KindAgent}, http.StatusOK, true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var reached bool + next := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + }) + + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + req = req.WithContext(authn.WithPrincipal(req.Context(), tc.principal)) + rec := httptest.NewRecorder() + + mcpsrv.Gate(next).ServeHTTP(rec, req) + + require.Equal(t, tc.wantCode, rec.Code) + require.Equal(t, tc.wantNext, reached, + "a refused caller must never reach the MCP server") + }) + } +} + +// A request that never went through the resolver middleware has no principal in +// its context — the zero value, which is anonymous. The gate must fail that +// closed, not open, so a wiring mistake that drops the middleware denies reads +// rather than serving the whole corpus unauthenticated. +func TestGateFailsClosedWithoutMiddleware(t *testing.T) { + var reached bool + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached = true }) + + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + rec := httptest.NewRecorder() + + mcpsrv.Gate(next).ServeHTTP(rec, req) + + require.Equal(t, http.StatusUnauthorized, rec.Code) + require.False(t, reached, "no principal in context must deny, not admit") +} diff --git a/mcpsrv/mcpsrv.go b/mcpsrv/mcpsrv.go index e547f77fb817ba24ef6d83336aa41137202566b4..3c7757e000145cfa3ae6a58dbf7ff18e4fcfc7f1 100644 --- a/mcpsrv/mcpsrv.go +++ b/mcpsrv/mcpsrv.go @@ -58,6 +58,7 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" + "sourcecraft.dev/bigbes/sr-ht-spec/authn" "sourcecraft.dev/bigbes/sr-ht-spec/search" ) @@ -237,6 +238,33 @@ func allowHosts(next http.Handler, origin string) http.Handler { }) } +// Gate refuses a caller with no read authority before any MCP method — not just +// tools/call, but initialize and tools/list too — reaches the server. It is the +// read plane's ACL for this surface: the owner and its agents may read and +// nobody else may, the same policy graph's /query and the web UI apply, spelled +// the same way. Two read surfaces with two policies is how a corpus leaks. +// +// It reads the principal the resolver middleware set, so it must be mounted +// INSIDE that middleware: +// +// mcp = resolver.Middleware()(mcpsrv.Gate(handler)) +// +// Without it, every read tool served approved content to anyone who cleared the +// Host allowlist — spec_propose was already fail-closed in service.Propose, but +// spec_search/spec_read/spec_list checked nothing. The refusal is a 401 with a +// line of plain text and never a login redirect: every caller here is a machine. +func Gate(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + p := authn.PrincipalFromContext(r.Context()) + if !p.IsOwner() && !p.IsAgent() { + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + http.Error(w, "authentication required", http.StatusUnauthorized) + return + } + next.ServeHTTP(w, r) + }) +} + // hostAllowed compares a request's Host against the expected hostname, ignoring // any port and IPv6 brackets. Loopback names stay allowed so `make run-dev` and // a local MCP client keep working.