~bigbes/sr-ht-spec

7cc652d57e9b4fb8962318f36fdab737bb0b1be8 — Eugene Blikh 24 days ago 096d25a
fix(mcpsrv): gate read tools to owner+agents (spec-jjo)

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
3 files changed, 103 insertions(+), 1 deletions(-)

M cmd/specsrht/main.go
A mcpsrv/gate_test.go
M mcpsrv/mcpsrv.go
M cmd/specsrht/main.go => cmd/specsrht/main.go +7 -1
@@ 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,

A mcpsrv/gate_test.go => mcpsrv/gate_test.go +68 -0
@@ 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")
}

M mcpsrv/mcpsrv.go => mcpsrv/mcpsrv.go +28 -0
@@ 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.