From 824788ab8269bd6c58de5848bc4545450fac7aaf Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sun, 16 Aug 2026 11:23:31 +0300 Subject: [PATCH] mcpsrv: mark /mcp uncacheable, fail closed on origin, split tool errors from faults MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three gaps between this surface and the cov/dolt pattern the siblings settled on. The write tools are untouched, the per-tool grant scheme is untouched, and the transport stays stateful. The endpoint set no Cache-Control and no Vary at all. Every answer here depends entirely on the credential the request carried and says nothing about it in its URL, and some of them are the whole approved corpus, so a shared cache was free to keep one and replay it to the next caller. cache.go is a local copy of ecore/mcphttp.PrivateCache, byte for byte on the header values so the retrofit is a delete and an import once that commit is published. It commits the headers on Write and Flush as well as WriteHeader: the SDK answers a POST with an event stream that never calls WriteHeader, so a wrapper hooking only that one sets nothing on the response an agent actually gets, while passing every other header test. Measured — with only WriteHeader hooked the streamed answer leaves with the SDK's own `no-cache, no-transform` and no Vary. Vary names both planes, where dolt.sr.ht names Authorization alone. dolt is right for dolt: its /mcp is bearer-only, so naming Cookie would promise a cache a dependency the surface never reads. It is wrong here. authn.Resolver.Resolve prefers a bearer token when one is present but falls through to login.UsernameFromRequest when none is, and an owner cookie resolves to KindOwner — which is exactly what Gate admits. On this service the cookie is the difference between the whole corpus and a 401. This is the one string a future mcphttp retrofit has to reconcile between the two services. An origin with no host in it was a warning and then an unguarded endpoint. The Host allowlist is the only thing protecting /mcp once the SDK's own rebinding guard is disabled, so that path turned one unparseable config value into a silently open endpoint indistinguishable in every functional test from a correctly guarded one. It is a construction error now. The daemon cannot reach it either way: service.Config.Validate already refuses to start unless the origin parses and carries a host. Errors from below travelled to the agent as tool results carrying their own text, so a dead git object store and a missing document were the same kind of answer. A tool result means "the call was understood and the thing you asked for is not there", so an agent reading one for a store outage concludes the document does not exist and rewrites a specification around a document that is perfectly real — and the store's own words reached it. errors.go splits the two: service.ErrNotFound is a tool result whose sentence is built from the call's own arguments, and everything else is a jsonrpc protocol error saying "internal server error" with the detail logged. The old tests asserted the behaviour being removed — that the agent was shown the words "on fire" — and are replaced by ones that pin the split in both directions. --- mcpsrv/cache.go | 133 +++++++++++++++++++++++++++ mcpsrv/cache_internal_test.go | 168 ++++++++++++++++++++++++++++++++++ mcpsrv/cache_test.go | 152 ++++++++++++++++++++++++++++++ mcpsrv/errors.go | 90 ++++++++++++++++++ mcpsrv/hostguard_test.go | 57 +++++++----- mcpsrv/list.go | 8 +- mcpsrv/mcpsrv.go | 58 ++++++++---- mcpsrv/mcpsrv_test.go | 79 ++++++++++++---- mcpsrv/read.go | 14 +-- mcpsrv/search.go | 6 +- 10 files changed, 700 insertions(+), 65 deletions(-) create mode 100644 mcpsrv/cache.go create mode 100644 mcpsrv/cache_internal_test.go create mode 100644 mcpsrv/cache_test.go create mode 100644 mcpsrv/errors.go diff --git a/mcpsrv/cache.go b/mcpsrv/cache.go new file mode 100644 index 0000000000000000000000000000000000000000..13f7aebef339367c5f983e1c348a2d473f37dd79 --- /dev/null +++ b/mcpsrv/cache.go @@ -0,0 +1,133 @@ +package mcpsrv + +import "net/http" + +// What this endpoint tells a cache, and why it is said here rather than by the +// router's middleware. +// +// # The shared helper this is a copy of +// +// sr-ht-ecore/mcphttp.PrivateCache is this function, one package up, written for +// the services on the instance that mount an MCP endpoint. It is not imported +// because it is not resolvable: the ecore commit that adds mcphttp is not +// published, and this module pins an ecore from before it. So the code is here, +// deliberately identical in behaviour and in header values, and the swap when +// mcphttp is reachable is a delete and an import with no observable change to +// any response — which is the reason the constants below are byte for byte what +// mcphttp emits rather than a spelling of this package's own. +const ( + // cacheControl is what every response of this endpoint carries. + // + // private and no-store are the instance's pair for an answer that depends + // entirely on the credential the request carried and says nothing about it in + // its URL — which is every answer here. no-cache would not do: it still + // permits a shared cache to *store* the body and merely revalidate, which is + // the one thing no-store forbids. + // + // no-transform is the SDK's own, kept. Its streamable transport writes + // `no-cache, no-transform` on every response it produces; no-transform + // protects the SSE framing from an intermediary that would recompress or + // rechunk it, and there is no reason to drop it. Only no-cache is replaced. + cacheControl = "private, no-store, no-transform" + + // cacheVary names what an answer here depends on, and it names both planes + // because on this service both planes genuinely reach /mcp. + // + // That is worth stating, because the sibling that mounts the same surface + // decided the other way: dolt.sr.ht varies on Authorization alone, arguing + // that its /mcp is bearer-only and that naming Cookie would promise a cache + // a dependency the surface never reads. The argument is right and does not + // apply here. authn.Resolver.Resolve prefers a bearer token when one is + // present, but falls through to login.UsernameFromRequest when none is — + // and an owner cookie resolves to KindOwner, which is exactly what Gate's + // CanRead admits. So on this service the cookie is not an unread header: it + // is the difference between the whole corpus and a 401, which is the + // strongest reason a response can have to vary on something. + cacheVary = "Cookie, Authorization" +) + +// privateCache marks every response this endpoint writes as one no cache may +// keep, and states what it depends on. +// +// It cannot be a middleware that sets the headers before the handler runs, which +// is how the rest of this service does it. The SDK's streamable transport sets +// Cache-Control itself, with Set, from inside the handler — so a value written +// on the way in is overwritten on the way out, and the response leaves with +// `no-cache, no-transform` and no Vary at all. The headers are therefore written +// at the last moment they still can be: when the status line is committed and +// every Set the handler was going to make has been made. +// +// It wraps everything Handler owns, the Host allowlist's 403 included. Gate's +// 401 is written outside this chain — it has to be, since Gate runs inside the +// resolver middleware and Handler runs inside Gate — so Gate sets the same two +// headers itself. +func privateCache(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + next.ServeHTTP(&cacheWriter{ResponseWriter: w}, r) + }) +} + +// cacheWriter is the http.ResponseWriter privateCache hands down: it sets the +// two headers when the response is committed, by whichever of the three routes +// the handler takes. +type cacheWriter struct { + http.ResponseWriter + committed bool +} + +func (w *cacheWriter) WriteHeader(status int) { + w.commit() + w.ResponseWriter.WriteHeader(status) +} + +// Write commits first: net/http commits the response on the first Write and +// drops every header set after that, silently. A handler that never calls +// WriteHeader is the ordinary case and not an exotic one. +func (w *cacheWriter) Write(b []byte) (int, error) { + w.commit() + return w.ResponseWriter.Write(b) +} + +// Flush is the third commit and the one an event stream takes. It has to be a +// method on this type: http.ResponseController prefers a Flush on the writer it +// was handed over one reached through Unwrap, so without this the flush would +// commit the response at the writer below and the two headers would never be +// written — on exactly the answers that stay open longest, and with every other +// test in this package still green (TestHeadersLandOnAFlush). +func (w *cacheWriter) Flush() { + w.commit() + // http.Flusher.Flush reports nothing, and the controller's error can only be + // "this writer does not support flushing" — which, if it happens, is a writer + // that could not have streamed through any wrapper. + _ = http.NewResponseController(w.ResponseWriter).Flush() +} + +func (w *cacheWriter) commit() { + if w.committed { + return + } + w.committed = true + w.Header().Set("Cache-Control", cacheControl) + w.Header().Set("Vary", cacheVary) +} + +// Unwrap is what http.ResponseController follows to reach the real writer for +// everything this type does not implement itself: the deadlines, Hijack, +// EnableFullDuplex. Without it a controller handed this writer answers +// ErrNotSupported to all of them, because cacheWriter embeds the +// http.ResponseWriter *interface* and so promotes nothing of the writer below. +// +// It is worth being exact about what it does *not* do, because the sentence it +// is usually given — "without Unwrap the wrapper hides the flusher and SSE +// streaming breaks" — is not true of this type and is easy to keep repeating. +// It is true of a wrapper whose only method is WriteHeader; here Flush above is +// a method on cacheWriter, so a controller finds that one and never needs to +// unwrap to flush. Deleting Unwrap leaves every flush, every header and every +// MCP session in this package's tests working, which is measured rather than +// asserted: TestUnwrapReachesTheWriterBelow pins the deadline call, which is the +// thing that actually stops working, and it is the test that goes red. +// +// The SDK asks for none of those today — its streamable transport calls Flush +// and nothing else — so this method is here for the wrapper to be a wrapper +// rather than to keep a feature alive. +func (w *cacheWriter) Unwrap() http.ResponseWriter { return w.ResponseWriter } diff --git a/mcpsrv/cache_internal_test.go b/mcpsrv/cache_internal_test.go new file mode 100644 index 0000000000000000000000000000000000000000..963828622facd051c54343f837f270920ca86214 --- /dev/null +++ b/mcpsrv/cache_internal_test.go @@ -0,0 +1,168 @@ +package mcpsrv + +import ( + "bufio" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The wrapper privateCache hands down, tested where it can be reached. The +// endpoint-level statement — that every answer of /mcp carries these two headers +// — is in cache_test.go; what is here is the three ways a handler can commit a +// response, because a wrapper that catches only one of them passes every header +// test while setting no header at all on the path the SDK actually takes. + +// TestHeadersLandOnAnExplicitWriteHeader is the ordinary path. +func TestHeadersLandOnAnExplicitWriteHeader(t *testing.T) { + h := privateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusAccepted) + })) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/mcp", nil)) + + assert.Equal(t, http.StatusAccepted, rec.Code) + assert.Equal(t, cacheControl, rec.Header().Get("Cache-Control")) + assert.Equal(t, cacheVary, rec.Header().Get("Vary")) +} + +// TestHeadersLandOnAnImplicitCommit covers the handler that never calls +// WriteHeader at all. net/http commits on the first Write and drops every header +// set after that point, silently. +func TestHeadersLandOnAnImplicitCommit(t *testing.T) { + h := privateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"jsonrpc":"2.0"}`)) + })) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/mcp", nil)) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, cacheControl, rec.Header().Get("Cache-Control")) + assert.Equal(t, cacheVary, rec.Header().Get("Vary")) + assert.Equal(t, `{"jsonrpc":"2.0"}`, rec.Body.String()) +} + +// TestHeadersLandOnAFlush is the third commit, and the one a stream takes: an +// SSE handler writes nothing before its first flush, so a wrapper that hooks +// Write and WriteHeader alone leaves the response uncached-marked exactly on the +// answers that stay open longest. +func TestHeadersLandOnAFlush(t *testing.T) { + h := privateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + require.NoError(t, http.NewResponseController(w).Flush()) + })) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/mcp", nil)) + + assert.True(t, rec.Flushed) + assert.Equal(t, cacheControl, rec.Header().Get("Cache-Control")) + assert.Equal(t, cacheVary, rec.Header().Get("Vary")) +} + +// TestTheSDKsOwnDirectivesAreOverridden is why this is a wrapper and not a +// middleware that sets the headers on the way in. The streamable transport sets +// Cache-Control itself, with Set, from inside the handler, so anything written +// before it runs loses. The handler here does exactly what the SDK does. +func TestTheSDKsOwnDirectivesAreOverridden(t *testing.T) { + h := privateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Cache-Control", "no-cache, no-transform") + w.WriteHeader(http.StatusOK) + })) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/mcp", nil)) + + assert.Equal(t, cacheControl, rec.Header().Get("Cache-Control"), + "no-cache lets a cache store the body and revalidate, which is the thing no-store forbids") + assert.Equal(t, cacheVary, rec.Header().Get("Vary")) +} + +// TestOnlyTheFirstCommitWrites checks the committed flag: a handler that writes +// after committing must not have its headers re-Set on every Write. +func TestOnlyTheFirstCommitWrites(t *testing.T) { + h := privateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("a")) + _, _ = w.Write([]byte("b")) + })) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/mcp", nil)) + + assert.Equal(t, []string{cacheControl}, rec.Header().Values("Cache-Control")) + assert.Equal(t, []string{cacheVary}, rec.Header().Values("Vary")) + assert.Equal(t, "ab", rec.Body.String()) +} + +// TestUnwrapReachesTheWriterBelow is the test the Unwrap method exists for, and +// it deliberately does not measure flushing. +// +// The sentence usually attached to Unwrap on a writer like this — that without +// it the flusher is hidden and SSE breaks — is not true of this type: Flush is a +// method on cacheWriter, so a controller finds that one first and never unwraps. +// (Measured, not assumed: with Unwrap deleted every other test in this file, and +// every MCP session the rest of this package opens, stays green.) What stops +// working is everything cacheWriter does not implement itself, and the deadlines +// are the reachable half of that — so this is written against a deadline, which +// is the call that actually goes ErrNotSupported. +// +// It runs over a real server because httptest.ResponseRecorder supports no +// deadline at all, and a recorder therefore cannot tell the two cases apart. +func TestUnwrapReachesTheWriterBelow(t *testing.T) { + var deadlineErr error + srv := httptest.NewServer(privateCache(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + deadlineErr = http.NewResponseController(w).SetWriteDeadline(time.Now().Add(time.Minute)) + }))) + t.Cleanup(srv.Close) + + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL, nil) + require.NoError(t, err) + resp, err := (&http.Client{Timeout: 5 * time.Second}).Do(req) + require.NoError(t, err) + t.Cleanup(func() { _ = resp.Body.Close() }) + + require.NoError(t, deadlineErr, "a controller must reach the writer this one wraps") +} + +// TestAStreamReachesTheClientBeforeTheHandlerReturns is the same property +// measured rather than asserted: over a real connection, with a real client, +// bytes flushed inside the handler have to arrive while the handler is still +// running. A recorder cannot tell a flush that worked from one that was buffered +// until the end — and a broken flush here is what would turn every MCP session +// on this endpoint into a hang rather than an error. +func TestAStreamReachesTheClientBeforeTheHandlerReturns(t *testing.T) { + release := make(chan struct{}) + srv := httptest.NewServer(privateCache(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + _, _ = w.Write([]byte("data: first\n\n")) + if err := http.NewResponseController(w).Flush(); err != nil { + t.Errorf("flush: %v", err) + } + <-release + }))) + t.Cleanup(func() { + close(release) + srv.Close() + }) + + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL, nil) + require.NoError(t, err) + resp, err := (&http.Client{Timeout: 5 * time.Second}).Do(req) + require.NoError(t, err) + t.Cleanup(func() { _ = resp.Body.Close() }) + + assert.Equal(t, cacheControl, resp.Header.Get("Cache-Control")) + assert.Equal(t, cacheVary, resp.Header.Get("Vary")) + + line, err := bufio.NewReader(resp.Body).ReadString('\n') + require.NoError(t, err, "the first event must arrive while the handler is still blocked") + assert.Equal(t, "data: first\n", line) +} diff --git a/mcpsrv/cache_test.go b/mcpsrv/cache_test.go new file mode 100644 index 0000000000000000000000000000000000000000..c6e4e5255d78305f8df922390f86f30ffb155794 --- /dev/null +++ b/mcpsrv/cache_test.go @@ -0,0 +1,152 @@ +package mcpsrv_test + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" + "sourcecraft.dev/bigbes/sr-ht-spec/mcpsrv" +) + +// What a cache is told about /mcp, stated at the endpoint rather than at the +// wrapper (cache_internal_test.go has the wrapper). +// +// Every answer here depends entirely on the credential the request carried and +// says nothing about it in its URL, and some of those answers are the whole +// approved corpus. A shared cache that stored one and replayed it to the next +// caller would be handing one principal's read to another. + +const ( + wantCacheControl = "private, no-store, no-transform" + wantVary = "Cookie, Authorization" +) + +// mount builds the chain cmd/specsrht builds, minus the resolver middleware: +// Gate outside, Handler inside, with a principal planted directly so the test +// does not need a tokens.sr.ht plane. +func mount(t *testing.T, p authn.Principal) *httptest.Server { + t.Helper() + r, s := newFixture() + h, err := mcpsrv.Handler(mcpsrv.Backend{Docs: r, Index: s}, "test", "https://spec.srht.bigb.es") + require.NoError(t, err) + + withPrincipal := func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + next.ServeHTTP(w, req.WithContext(authn.WithPrincipal(req.Context(), p))) + }) + } + + router := chi.NewRouter() + router.Handle("/mcp", withPrincipal(mcpsrv.Gate(h))) + srv := httptest.NewServer(router) + t.Cleanup(srv.Close) + return srv +} + +func post(t *testing.T, srv *httptest.Server, host, accept, body string) *http.Response { + t.Helper() + req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, srv.URL+"/mcp", strings.NewReader(body)) + require.NoError(t, err) + req.Host = host + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", accept) + + resp, err := srv.Client().Do(req) + require.NoError(t, err) + t.Cleanup(func() { _ = resp.Body.Close() }) + return resp +} + +const initialize = `{"jsonrpc":"2.0","id":1,"method":"initialize","params":{` + + `"protocolVersion":"2025-06-18","capabilities":{},` + + `"clientInfo":{"name":"test-client","version":"test"}}}` + +// The answer an agent actually gets, on the transport it actually uses: the SDK +// replies to a POST with an event stream, which is the response that never calls +// WriteHeader and commits through Flush instead. This is the case a wrapper +// hooking WriteHeader alone would miss while passing every other test here. +func TestTheHeadersLandOnAStreamedAnswer(t *testing.T) { + srv := mount(t, authn.Principal{Kind: authn.KindOwner}) + + resp := post(t, srv, "spec.srht.bigb.es", "application/json, text/event-stream", initialize) + + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Equal(t, "text/event-stream", strings.Split(resp.Header.Get("Content-Type"), ";")[0], + "this test is worth nothing unless the answer really was a stream") + + assert.Equal(t, wantCacheControl, resp.Header.Get("Cache-Control")) + assert.Equal(t, wantVary, resp.Header.Get("Vary")) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Contains(t, string(body), mcpsrv.ServerName, "and it really was this server answering") +} + +// The SDK writes `no-cache, no-transform` on its own responses. no-cache still +// permits a shared cache to store the body and merely revalidate, which is the +// one thing no-store forbids — so the wrapper has to win, and it can only do +// that by writing at commit time rather than on the way in. +func TestTheSDKsCacheControlDoesNotSurvive(t *testing.T) { + srv := mount(t, authn.Principal{Kind: authn.KindOwner}) + + resp := post(t, srv, "spec.srht.bigb.es", "application/json, text/event-stream", initialize) + + require.Equal(t, []string{wantCacheControl}, resp.Header.Values("Cache-Control"), + "exactly one directive set, and it is ours") + assert.NotContains(t, resp.Header.Get("Cache-Control"), "no-cache") +} + +// A refusal is an answer too. The Host allowlist writes this one before the SDK +// is reached at all, and it is inside the wrapper for that reason. +func TestTheHeadersLandOnAHostRefusal(t *testing.T) { + srv := mount(t, authn.Principal{Kind: authn.KindOwner}) + + resp := post(t, srv, "evil.example.com", "application/json, text/event-stream", initialize) + + require.Equal(t, http.StatusForbidden, resp.StatusCode) + assert.Equal(t, wantCacheControl, resp.Header.Get("Cache-Control")) + assert.Equal(t, wantVary, resp.Header.Get("Vary")) +} + +// Gate's 401 is written outside the wrapper — Gate runs inside the resolver +// middleware and Handler runs inside Gate — so Gate sets the two headers itself. +// A cache free to keep this 401 would refuse a credential this service never +// saw. +func TestTheHeadersLandOnAGateRefusal(t *testing.T) { + srv := mount(t, authn.Anonymous()) + + resp := post(t, srv, "spec.srht.bigb.es", "application/json, text/event-stream", initialize) + + require.Equal(t, http.StatusUnauthorized, resp.StatusCode) + assert.Equal(t, wantCacheControl, resp.Header.Get("Cache-Control")) + assert.Equal(t, wantVary, resp.Header.Get("Vary")) +} + +// Vary names the cookie because on this service the cookie genuinely decides the +// answer, which is the one thing that separates this endpoint from dolt.sr.ht's. +// +// dolt varies on Authorization alone and is right to: its /mcp is bearer-only, +// so naming Cookie there would promise a cache a dependency the surface never +// reads. Here authn.Resolver.Resolve falls through to the unified-login cookie +// whenever no bearer token is present, and an owner cookie resolves to KindOwner +// — which is exactly what Gate admits. The two requests below differ in nothing +// but the principal the cookie plane would have produced, and they get different +// statuses; that difference is what Vary: Cookie is for. +func TestTheCookiePlaneReallyDoesDecideTheAnswer(t *testing.T) { + asOwner := post(t, mount(t, authn.Principal{Kind: authn.KindOwner}), + "spec.srht.bigb.es", "application/json, text/event-stream", initialize) + asAnonymous := post(t, mount(t, authn.Anonymous()), + "spec.srht.bigb.es", "application/json, text/event-stream", initialize) + + require.Equal(t, http.StatusOK, asOwner.StatusCode) + require.Equal(t, http.StatusUnauthorized, asAnonymous.StatusCode) + require.NotEqual(t, asOwner.StatusCode, asAnonymous.StatusCode, + "if these ever agree, Vary: Cookie has stopped being a statement about this endpoint") +} diff --git a/mcpsrv/errors.go b/mcpsrv/errors.go new file mode 100644 index 0000000000000000000000000000000000000000..f6e365c68a3cce7980ae9472a21ad3edc2d5ba6f --- /dev/null +++ b/mcpsrv/errors.go @@ -0,0 +1,90 @@ +package mcpsrv + +import ( + "errors" + "log/slog" + + "github.com/modelcontextprotocol/go-sdk/jsonrpc" + "go.bigb.es/auxilia/scribe" + + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/service" +) + +// The one place this surface decides what a failure *is*, which on MCP is a +// question with two answers rather than a status code. +// +// - A tool result error (CallToolResult.IsError) is an answer to the agent: +// the call was understood, executed, and the thing asked for is not there. +// The SDK produces one out of any ordinary error a handler returns, and the +// agent reads it as text and decides what to ask next. +// - A protocol error (a *jsonrpc.Error returned by a handler, which the SDK +// passes through as the JSON-RPC error of the response) says the call did +// not produce an answer at all. The client's CallTool returns an error +// rather than a result, which is exactly right for a git object store that +// is down: an agent must not read "the store could not answer" as "that +// document does not exist" and go rewrite a specification around a document +// that is perfectly real. +// +// Before this file the two were one. Every error from below travelled to the +// agent as a tool result carrying its own text, so `git object store is on +// fire` and `no document "SPEC-0007"` were the same kind of answer, told apart +// only by prose the agent would have had to parse. The tests pinned it: a +// backend failure asserted that the agent was shown the words "on fire". +// +// The whole table is missingOrDenied plus a default, and the default is the +// protocol arm on purpose: an unmapped error is a bug in a layer below, and +// rendering it as a tool result would report that bug to the agent as a fact +// about the corpus. + +// internalMessage is the message of every protocol error this surface returns. +// The detail is logged, never sent: the errors below name spaces, revisions, +// paths and git internals, and an agent holding a working token is not the +// audience for any of it. +const internalMessage = "internal server error" + +// missingOrDenied is the answer to a read that resolved to nothing. +// +// missing is the sentence the agent sees, and every caller builds it from the +// arguments of the call being answered ("no space ~alice/rfcs"). That is +// deliberate: the sentence is written from what the caller passed, so it +// discloses nothing the caller did not already know, and it does not carry the +// wrapped text of the error it is replacing — service/ wraps its misses with +// what it looked up, and echoing that is how a surface eventually publishes the +// difference between "no such space" and "not yours". +// +// where is the operator's half — the tool name — and appears only in the log +// line of the protocol arm. +func missingOrDenied(err error, where, missing string) error { + if errors.Is(err, service.ErrNotFound) { + // A tool result error: the SDK packs an ordinary error into + // CallToolResult with IsError set. + return errors.New(missing) + } + return internalError(err, where) +} + +// internalError logs the cause and returns the protocol error the client sees. +// +// The error goes through scribe.Err, which expands a culpa chain into err.msg, +// err.code and err.hint instead of flattening it with %v. +func internalError(err error, where string) error { + slog.Error("a tool call failed", "tool", where, scribe.Err(err)) + return &jsonrpc.Error{Code: jsonrpc.CodeInternalError, Message: internalMessage} +} + +// noSpace and noRevision are the two "missing" sentences the read tools pass to +// missingOrDenied, built from the call's own arguments and nothing else. +// +// There is no masked-versus-absent distinction to preserve here, unlike the +// sibling services: Gate has already established that the caller is the owner +// or one of its agents, and this is a single-user instance whose spaces all +// belong to that owner. A space this caller cannot see does not exist. +func noSpace(ref core.SpaceRef) string { return "no space " + ref.String() } + +func noRevision(ref core.SpaceRef, rev string) string { + if rev == service.ApprovedRev { + return "space " + ref.String() + " has no approved revision to read" + } + return "no revision " + rev + " in " + ref.String() +} diff --git a/mcpsrv/hostguard_test.go b/mcpsrv/hostguard_test.go index f1ab9733422c723e6a737d9bb3ebf2317b41db6c..813671226e2b61ba4435299b2cdfa234ea442132 100644 --- a/mcpsrv/hostguard_test.go +++ b/mcpsrv/hostguard_test.go @@ -6,6 +6,8 @@ import ( "strings" "testing" + "github.com/stretchr/testify/require" + "sourcecraft.dev/bigbes/sr-ht-spec/mcpsrv" ) @@ -57,27 +59,40 @@ func TestHostGuard(t *testing.T) { } } -// An unusable origin must leave a loud trail rather than silently unguarding the -// endpoint. It still serves (refusing to start over a config typo is worse), but -// Handler logs a warning; this pins that it does not instead start refusing -// every request, which would look like the guard "working". -func TestHostGuardWithoutOriginStillServes(t *testing.T) { - r, s := newFixture() - h, err := mcpsrv.Handler(mcpsrv.Backend{Docs: r, Index: s}, "test", "") - if err != nil { - t.Fatalf("Handler: %v", err) - } - - req := httptest.NewRequest(http.MethodPost, "/mcp", - strings.NewReader(`{"jsonrpc":"2.0","id":1,"method":"initialize"}`)) - req.Host = "anything.example.com" - req.Header.Set("Content-Type", "application/json") - req.Header.Set("Accept", "application/json, text/event-stream") - - w := httptest.NewRecorder() - h.ServeHTTP(w, req) +// An origin with no host in it is a construction error, not a warning. +// +// This used to be the opposite: Handler logged "Host validation on /mcp is +// DISABLED" and returned the bare handler, on the reasoning that refusing to +// start over a config typo is worse than running unguarded. It is not. The +// guard is the only thing protecting /mcp once the SDK's own is disabled, so +// that path turned one unparseable value into a silently open endpoint that +// passes every functional test — the class of failure nobody discovers. The +// daemon cannot reach it in any case: service.Config.Validate already refuses +// to start unless the origin parses and carries a host. +func TestHandlerRefusesAnOriginItCannotGuardWith(t *testing.T) { + for _, origin := range []string{ + "", + " ", + "not a url at all", + "https://", // parses, but carries no host + "/just/path", // relative, no host + } { + t.Run(origin, func(t *testing.T) { + r, s := newFixture() + h, err := mcpsrv.Handler(mcpsrv.Backend{Docs: r, Index: s}, "test", origin) - if w.Code == http.StatusForbidden { - t.Fatalf("unconfigured origin should not 403 every request; got %d", w.Code) + require.Error(t, err, "an origin with no host must not yield a handler") + require.Nil(t, h, "an unguarded handler must never escape the constructor") + require.Contains(t, err.Error(), "no host to guard /mcp with") + }) } } + +// A usable origin still builds, which is what keeps the test above from passing +// for the wrong reason. +func TestHandlerAcceptsAUsableOrigin(t *testing.T) { + r, s := newFixture() + h, err := mcpsrv.Handler(mcpsrv.Backend{Docs: r, Index: s}, "test", "https://spec.srht.bigb.es") + require.NoError(t, err) + require.NotNil(t, h) +} diff --git a/mcpsrv/list.go b/mcpsrv/list.go index b028380984b1142d2d90e21164d36721c76b6473..48802f917a176233a90cdaf653cef1779c7f7b2f 100644 --- a/mcpsrv/list.go +++ b/mcpsrv/list.go @@ -73,11 +73,11 @@ func listHandler(ctx context.Context, b Backend, in listInput) (listOutput, erro } sp, err := b.Docs.OpenSpace(ctx, ref) if err != nil { - return listOutput{}, err + return listOutput{}, missingOrDenied(err, "spec_list", noSpace(ref)) } arc, _, resolved, err := archiveAt(ctx, b, sp, rev) if err != nil { - return listOutput{}, err + return listOutput{}, missingOrDenied(err, "spec_list", noRevision(ref, rev)) } out := listOutput{Space: ref.String(), Rev: resolved, Documents: make([]documentEntry, 0, len(arc.All()))} @@ -100,7 +100,9 @@ func listHandler(ctx context.Context, b Backend, in listInput) (listOutput, erro func listSpaces(ctx context.Context, b Backend) (listOutput, error) { spaces, err := b.Docs.ListSpaces(ctx) if err != nil { - return listOutput{}, err + // No space was named, so there is no "that one does not exist" to + // answer: whatever went wrong here is this service's. + return listOutput{}, internalError(err, "spec_list") } out := listOutput{Spaces: make([]spaceEntry, 0, len(spaces))} for _, sp := range spaces { diff --git a/mcpsrv/mcpsrv.go b/mcpsrv/mcpsrv.go index 84a008d482d02eba36341f0931bd1f76cb78fb35..3eb235b76e991c6f0e7c655e2ee8e21c6ca7cb8a 100644 --- a/mcpsrv/mcpsrv.go +++ b/mcpsrv/mcpsrv.go @@ -56,7 +56,7 @@ package mcpsrv import ( "context" "errors" - "log/slog" + "fmt" "net" "net/http" "strings" @@ -231,37 +231,53 @@ func New(b Backend, version string) (*mcp.Server, error) { // rebinding attack carries the attacker's name in Host and fails that; nginx // forwards our real hostname and passes. Disabling the SDK guard without this // replacement would be a genuine regression, not a formality. +// +// origin is therefore required, and an origin with no host in it is refused +// here rather than warned about and then served unguarded. This used to warn: +// it logged that Host validation was disabled and returned the bare handler, +// on the reasoning that refusing to start over a config typo is worse than +// running without the guard. That trade is the wrong way round. The guard is +// the *only* thing protecting /mcp once the SDK's own is disabled, so the warn +// path turned one unparseable config value into a silently open endpoint — +// discoverable, in principle, from a log line nobody reads, and indistinguishable +// in every functional test from a correctly guarded one. A daemon cannot reach +// this call with such an origin anyway: service.Config.Validate already refuses +// to start unless [spec.sr.ht] origin parses and carries a host. A caller that +// got here without one is not an operator to be warned, it is a bug. func Handler(b Backend, version, origin string) (http.Handler, error) { srv, err := New(b, version) if err != nil { return nil, err } + // instconf.OriginHost and not a local parse: this is the reading of an + // origin, and it is the same one authn's mailbox derivation and service's + // config validation make. An origin nobody can extract a host from answers + // "" here — never a guess such as "localhost", which would silently make + // every malformed origin agree with a local client on the one code path + // where that decides an allowlist. + want := instconf.OriginHost(origin) + if want == "" { + return nil, fmt.Errorf("mcpsrv: origin %q has no host to guard /mcp with", origin) + } h := mcp.NewStreamableHTTPHandler( func(*http.Request) *mcp.Server { return srv }, &mcp.StreamableHTTPOptions{DisableLocalhostProtection: true}, ) - return allowHosts(h, origin), nil + // The cache directives wrap the Host allowlist rather than the other way + // round, so that the 403 carries them too: a refusal by hostname is as + // unstorable as an answer, and it is written before the SDK is reached at all. + return privateCache(allowHosts(h, want)), nil } // allowHosts is this endpoint's DNS-rebinding protection, in the form the // deployment actually needs: Host must be the service's own origin hostname, or // a loopback name for local development. // -// An empty or unparseable origin leaves the endpoint unguarded, so it says so -// loudly. A misconfigured origin must not quietly become the difference between -// protected and open — that is the class of failure nobody discovers. -func allowHosts(next http.Handler, origin string) http.Handler { - // instconf.OriginHost and not a local parse: this is the reading of an - // origin, and it is the same one authn's mailbox derivation and service's - // config validation make. An origin nobody can extract a host from answers - // "" here — never a guess such as "localhost", which would silently make - // every malformed origin agree with a local client — and "" is what turns - // the guard off, loudly, below. - want := instconf.OriginHost(origin) - if want == "" { - slog.Warn("mcpsrv: no usable origin configured; Host validation on /mcp is DISABLED") - return next - } +// want is the hostname already extracted from the origin by Handler, which is +// also where an origin that yields none is refused. It is a resolved host and +// never an origin, so there is no path through this function that leaves the +// guard off. +func allowHosts(next http.Handler, want string) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if !hostAllowed(r.Host, want) { http.Error(w, "Forbidden: unexpected Host header", http.StatusForbidden) @@ -293,11 +309,19 @@ func allowHosts(next http.Handler, origin string) http.Handler { // tokens.sr.ht token minted for spec:propose alone at `initialize`, before it // ever named a tool. The grant is therefore checked per tool, by requireRead and // by service.Propose, each of which knows what is being attempted. +// +// The refusal carries the cache directives itself, which privateCache would +// otherwise have written for it. It has to: Gate runs inside the resolver +// middleware and Handler runs inside Gate, so a 401 written here never reaches +// the wrapper Handler installs. A shared cache free to keep this 401 and replay +// it to the next caller would refuse a credential this service never saw. func Gate(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if !authn.PrincipalFromContext(r.Context()).CanRead() { w.Header().Set("Content-Type", "text/plain; charset=utf-8") w.Header().Set("WWW-Authenticate", authn.Challenge()) + w.Header().Set("Cache-Control", cacheControl) + w.Header().Set("Vary", cacheVary) http.Error(w, "authentication required", http.StatusUnauthorized) return } diff --git a/mcpsrv/mcpsrv_test.go b/mcpsrv/mcpsrv_test.go index c8d92c24b1a866390371c59bec0dafb48dc8150f..42f0601b98c10fb9025a5b68e7faabb27feadf52 100644 --- a/mcpsrv/mcpsrv_test.go +++ b/mcpsrv/mcpsrv_test.go @@ -345,17 +345,25 @@ func TestReadMissing(t *testing.T) { require.Contains(t, errorText(res), `no document "SPEC-9999"`) require.Contains(t, errorText(res), "~bigbes/rfcs") + // The sentence is built from the argument the caller passed, not from the + // error service/ wrapped its miss with. It used to be the latter, so what + // reached the agent was the sentinel's own text — "service: not found: + // space ~bigbes/nope" — which names a layer the agent cannot see and is one + // refactor away from saying something different about a private space than + // about an absent one. res = call(t, session, "spec_read", map[string]any{ "space": "~bigbes/nope", "document": "SPEC-0007", }) require.True(t, res.IsError) - require.Contains(t, errorText(res), "not found") + require.Contains(t, errorText(res), "no space ~bigbes/nope") + require.NotContains(t, errorText(res), "service:") res = call(t, session, "spec_read", map[string]any{ "space": "~bigbes/rfcs", "document": "SPEC-0007", "rev": blob(99), }) require.True(t, res.IsError) - require.Contains(t, errorText(res), "not found") + require.Contains(t, errorText(res), "no revision "+blob(99)) + require.NotContains(t, errorText(res), "service:") res = call(t, session, "spec_read", map[string]any{"space": "", "document": "SPEC-0007"}) require.True(t, res.IsError) @@ -366,18 +374,62 @@ func TestReadMissing(t *testing.T) { require.Contains(t, errorText(res), "document must not be empty") } -// A failure below service/ surfaces as a tool error rather than taking the -// session down. -func TestReadBackendFailure(t *testing.T) { +// A store that could not answer is a protocol error, not a tool result — the +// distinction errors.go exists for. +// +// This test used to assert the opposite: that the agent was handed a tool +// result carrying the words "on fire". That is the failure mode bench shipped +// and cov corrected. A tool result means "the call was understood and the thing +// you asked for is not there", so an agent reading one for a dead object store +// concludes the document does not exist and rewrites its plan around a document +// that is perfectly real. It also published the store's own words to the agent. +func TestAStoreThatCouldNotAnswerIsAProtocolError(t *testing.T) { r, s := newFixture() r.listErr = errors.New("git object store is on fire") session := connect(t, r, s) - res := call(t, session, "spec_read", map[string]any{ - "space": "~bigbes/rfcs", "document": "SPEC-0007", + res, err := session.CallTool(context.Background(), &mcp.CallToolParams{ + Name: "spec_read", + Arguments: map[string]any{"space": "~bigbes/rfcs", "document": "SPEC-0007"}, + }) + require.Error(t, err, "a store outage must fail the call, not answer it") + require.Nil(t, res, "a protocol error carries no result for an agent to read as an answer") + + // And it says nothing about what failed. The detail is logged, never sent. + require.Contains(t, err.Error(), "internal server error") + require.NotContains(t, err.Error(), "on fire") + require.NotContains(t, err.Error(), "object store") +} + +// The same for the search index, which is the other store this surface reads. +func TestASearchIndexOutageIsAProtocolError(t *testing.T) { + r, s := newFixture() + s.err = errors.New("index is closed") + session := connect(t, r, s) + + res, err := session.CallTool(context.Background(), &mcp.CallToolParams{ + Name: "spec_search", + Arguments: map[string]any{"query": "storage"}, }) + require.Error(t, err) + require.Nil(t, res) + require.Contains(t, err.Error(), "internal server error") + require.NotContains(t, err.Error(), "index is closed") +} + +// A miss stays a tool result, which is the other half of the split: the agent +// is meant to read this one and ask something else. +func TestAMissStaysAToolResult(t *testing.T) { + r, s := newFixture() + session := connect(t, r, s) + + res, err := session.CallTool(context.Background(), &mcp.CallToolParams{ + Name: "spec_read", + Arguments: map[string]any{"space": "~bigbes/nope", "document": "SPEC-0007"}, + }) + require.NoError(t, err, "a miss is an answer, not a failed call") require.True(t, res.IsError) - require.Contains(t, errorText(res), "on fire") + require.Contains(t, errorText(res), "no space ~bigbes/nope") } // --- search ----------------------------------------------------------------- @@ -510,15 +562,8 @@ func TestSearchHitShape(t *testing.T) { require.Equal(t, "notes/dev-log.md", doc.Path) } -func TestSearchBackendFailure(t *testing.T) { - r, s := newFixture() - s.err = errors.New("index is closed") - session := connect(t, r, s) - - res := call(t, session, "spec_search", map[string]any{"query": "q"}) - require.True(t, res.IsError) - require.Contains(t, errorText(res), "index is closed") -} +// The index-outage case lives with the other half of the split, in +// TestASearchIndexOutageIsAProtocolError. // --- list ------------------------------------------------------------------- diff --git a/mcpsrv/read.go b/mcpsrv/read.go index 5b0c4d42ce6635cc1014731b30339094e0d2cc75..9ee708db1507c569bbef5ce4d3a82df57d8cf433 100644 --- a/mcpsrv/read.go +++ b/mcpsrv/read.go @@ -64,11 +64,11 @@ func readHandler(ctx context.Context, b Backend, in readInput) (readOutput, erro } sp, err := b.Docs.OpenSpace(ctx, ref) if err != nil { - return readOutput{}, err + return readOutput{}, missingOrDenied(err, "spec_read", noSpace(ref)) } arc, bodies, resolved, err := archiveAt(ctx, b, sp, rev) if err != nil { - return readOutput{}, err + return readOutput{}, missingOrDenied(err, "spec_read", noRevision(ref, rev)) } page, err := resolvePage(arc, in.Document) if err != nil { @@ -77,10 +77,12 @@ func readHandler(ctx context.Context, b Backend, in readInput) (readOutput, erro body, ok := bodies[page.Path] if !ok { // The archive is built from these very bodies, so a page without one - // is a broken invariant rather than a missing document. Returning an - // empty markdown field would be indistinguishable from an empty - // document. - return readOutput{}, fmt.Errorf("document %s at %s in %s has no body", page.Path, resolved, ref) + // is a broken invariant rather than a missing document — this service's + // fault and not an answer about the corpus, so it goes to the agent as a + // protocol error and the detail goes to the log. Returning an empty + // markdown field would be indistinguishable from an empty document. + return readOutput{}, internalError( + fmt.Errorf("document %s at %s in %s has no body", page.Path, resolved, ref), "spec_read") } return readOutput{ Space: ref.String(), diff --git a/mcpsrv/search.go b/mcpsrv/search.go index bb08f0d079873ca1ebecd2c23994a82758d06958..e6522a5976e10bff7ddb12c9db73d736bb4c1247 100644 --- a/mcpsrv/search.go +++ b/mcpsrv/search.go @@ -79,7 +79,11 @@ func searchHandler(ctx context.Context, b Backend, in searchInput) (searchOutput Offset: in.Offset, }) if err != nil { - return searchOutput{}, err + // A query that matched nothing is an empty result set and not an error, + // so every error the index can return here is the index failing. An + // agent told "no hits" by a closed index would conclude the corpus does + // not cover what it asked about. + return searchOutput{}, internalError(err, "spec_search") } out := searchOutput{Hits: make([]searchHit, 0, len(res.Hits)), Total: res.Total}