From 3aa1fcfd9a55d3a42f0e8e3f5ca52725f9a2b2a6 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sun, 16 Aug 2026 01:53:10 +0300 Subject: [PATCH] mcphttp: the MCP endpoint plumbing both services already share hostAllowed is byte-identical in cov and bench, and neither should own it. PrivateCache is cov's alone today, which is why bench's /mcp currently sets no Cache-Control and no Vary at all. StreamableOptions carries the pair both pass and the reason for it. HostGuard takes cov's fail-closed stance: an origin with no host to guard with is a construction error, not a warning. bench serves unguarded in that case and has a test pinning it; reconciling that is bench's to do. Deliberately not a shared MCP framework. Once the go-sdk and the per-service tools are subtracted, this is the whole overlap. --- go.mod | 9 ++- go.sum | 14 ++++ mcphttp/cache.go | 93 ++++++++++++++++++++++++ mcphttp/cache_test.go | 146 +++++++++++++++++++++++++++++++++++++ mcphttp/hostguard.go | 95 ++++++++++++++++++++++++ mcphttp/hostguard_test.go | 148 ++++++++++++++++++++++++++++++++++++++ mcphttp/mcphttp.go | 99 +++++++++++++++++++++++++ mcphttp/options_test.go | 42 +++++++++++ 8 files changed, 645 insertions(+), 1 deletion(-) create mode 100644 mcphttp/cache.go create mode 100644 mcphttp/cache_test.go create mode 100644 mcphttp/hostguard.go create mode 100644 mcphttp/hostguard_test.go create mode 100644 mcphttp/mcphttp.go create mode 100644 mcphttp/options_test.go diff --git a/go.mod b/go.mod index c603c4a33a3e8ba8e1d4402b3dfb676112d2a39f..fdd6bef9d1b9f69745e8a943ea42805bd502cd9d 100644 --- a/go.mod +++ b/go.mod @@ -1,9 +1,10 @@ module sourcecraft.dev/bigbes/sr-ht-ecore -go 1.24 +go 1.25.0 require ( github.com/go-chi/chi/v5 v5.3.1 + github.com/modelcontextprotocol/go-sdk v1.6.1 github.com/stretchr/testify v1.10.0 github.com/vaughan0/go-ini v0.0.0-20130923145212-a98ad7ee00ec sourcecraft.dev/bigbes/sr-ht-core v0.0.0-20260718185800-dd418a200152 @@ -15,10 +16,16 @@ require ( github.com/Masterminds/squirrel v1.5.4 // indirect github.com/davecgh/go-spew v1.1.1 // indirect github.com/fernet/fernet-go v0.0.0-20211208181803-9f70042a33ee // indirect + github.com/google/jsonschema-go v0.4.3 // indirect github.com/lann/builder v0.0.0-20180802200727-47ae307949d0 // indirect github.com/lann/ps v0.0.0-20150810152359-62de8c46ede0 // indirect github.com/lib/pq v1.10.9 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect + github.com/segmentio/asm v1.1.3 // indirect + github.com/segmentio/encoding v0.5.4 // indirect github.com/vektah/gqlparser/v2 v2.5.8 // indirect + github.com/yosida95/uritemplate/v3 v3.0.2 // indirect + golang.org/x/oauth2 v0.35.0 // indirect + golang.org/x/sys v0.41.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index 918d27dcc2b09827c3f1190737ccb3d37e879ae2..18940080c1b4af46ecc74e96f41667b1b819fde7 100644 --- a/go.sum +++ b/go.sum @@ -46,6 +46,8 @@ github.com/go-chi/chi/v5 v5.3.1/go.mod h1:R+tYY2hNuVUUjxoPtqUdgBqevM9s9njzkTLutV github.com/go-chi/cors v1.2.2/go.mod h1:sSbTewc+6wYHBBCW7ytsFSn836hqM7JxpglAy2Vzc58= github.com/go-redis/redis/v8 v8.11.5/go.mod h1:gREzHqY1hg6oD9ngVRbLStwAWKhA0FEgq8Jd4h5lpwo= github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps= +github.com/google/jsonschema-go v0.4.3 h1:/DBOLZTfDow7pe2GmaJNhltueGTtDKICi8V8p+DQPd0= +github.com/google/jsonschema-go v0.4.3/go.mod h1:r5quNTdLOYEz95Ru18zA0ydNbBuYoo9tgaYcxEYhJVE= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/gorilla/websocket v1.5.0/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE= github.com/hashicorp/golang-lru/v2 v2.0.7/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM= @@ -69,6 +71,8 @@ github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovk github.com/mattn/go-isatty v0.0.19/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= github.com/matttproud/golang_protobuf_extensions v1.0.4/go.mod h1:BSXmuO+STAnVfrANrmjBb36TMTDstsz7MSK+HVaYKv4= github.com/mitchellh/mapstructure v1.5.0/go.mod h1:bFUtVrKA4DC2yAKiSyO/QUcy7e+RRV2QTWOzhPopBRo= +github.com/modelcontextprotocol/go-sdk v1.6.1 h1:0zOSupjKUxPKSocPT1Wtago+mUHU2/uZ4xSOY0FGReU= +github.com/modelcontextprotocol/go-sdk v1.6.1/go.mod h1:kzm3kzFL1/+AziGOE0nUs3gvPoNxMCvkxokMkuFapXQ= github.com/oklog/ulid/v2 v2.1.1/go.mod h1:rcEKHmBBKfef9DhnvX7y1HZBYxjXb0cP5ExxNsTT1QQ= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= @@ -78,6 +82,10 @@ github.com/prometheus/common v0.44.0/go.mod h1:ofAIvZbQ1e/nugmZGz4/qCb9Ap1VoSTIO github.com/prometheus/procfs v0.12.0/go.mod h1:pcuDEFsWDnvcgNzo4EEweacyhjeA9Zk3cnaOZAZEfOo= github.com/rogpeppe/go-internal v1.13.1/go.mod h1:uMEvuHeurkdAXX61udpOXGD/AzZDWNMNyH2VO9fmH0o= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= +github.com/segmentio/asm v1.1.3 h1:WM03sfUOENvvKexOLp+pCqgb/WDjsi7EK8gIsICtzhc= +github.com/segmentio/asm v1.1.3/go.mod h1:Ld3L4ZXGNcSLRg4JBsZ3//1+f/TjYl0Mzen/DQy1EJg= +github.com/segmentio/encoding v0.5.4 h1:OW1VRern8Nw6ITAtwSZ7Idrl3MXCFwXHPgqESYfvNt0= +github.com/segmentio/encoding v0.5.4/go.mod h1:HS1ZKa3kSN32ZHVZ7ZLPLXWvOVIiZtyJnO1gPH1sKt0= github.com/sergi/go-diff v1.3.1 h1:xkr+Oxo4BOQKmkn/B9eMK0g5Kg/983T9DqqPHwYqD+8= github.com/sergi/go-diff v1.3.1/go.mod h1:aMJSSKb2lpPvRNec0+w3fl7LP9IOFzdc9Pa4NFbPK1I= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= @@ -94,10 +102,16 @@ github.com/vaughan0/go-ini v0.0.0-20130923145212-a98ad7ee00ec/go.mod h1:owBmyHYM github.com/vektah/gqlparser/v2 v2.5.8 h1:pm6WOnGdzFOCfcQo9L3+xzW51mKrlwTEg4Wr7AH1JW4= github.com/vektah/gqlparser/v2 v2.5.8/go.mod h1:z8xXUff237NntSuH8mLFijZ+1tjV1swDbpDqjJmk6ME= github.com/xrash/smetrics v0.0.0-20201216005158-039620a65673/go.mod h1:N3UwUGtsrSj3ccvlPHLoLsHnpR27oXr4ZE984MbSER8= +github.com/yosida95/uritemplate/v3 v3.0.2 h1:Ed3Oyj9yrmi9087+NczuL5BwkIc4wvTb5zIM+UJPGz4= +github.com/yosida95/uritemplate/v3 v3.0.2/go.mod h1:ILOh0sOhIJR3+L/8afwt/kE++YT040gmv5BQTMR2HP4= golang.org/x/crypto v0.33.0/go.mod h1:bVdXmD7IV/4GdElGPozy6U7lWdRXA4qyRVGJV57uQ5M= golang.org/x/mod v0.10.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/net v0.33.0/go.mod h1:HXLR5J+9DxmrqMwG9qjGCxZ+zKXxBru04zlTvWlWuN4= +golang.org/x/oauth2 v0.35.0 h1:Mv2mzuHuZuY2+bkyWXIHMfhNdJAdwW3FuWeCPYN5GVQ= +golang.org/x/oauth2 v0.35.0/go.mod h1:lzm5WQJQwKZ3nwavOZ3IS5Aulzxi68dUSgRHujetwEA= golang.org/x/sys v0.30.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/sys v0.41.0 h1:Ivj+2Cp/ylzLiEU89QhWblYnOE9zerudt9Ftecq2C6k= +golang.org/x/sys v0.41.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= golang.org/x/text v0.22.0/go.mod h1:YRoo4H8PVmsu+E3Ou7cqLVH8oXWIHVoX0jqUWALQhfY= golang.org/x/tools v0.9.3/go.mod h1:owI94Op576fPu3cIGQeHs3joujW/2Oc6MtlxbF5dfNc= google.golang.org/protobuf v1.33.0/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos= diff --git a/mcphttp/cache.go b/mcphttp/cache.go new file mode 100644 index 0000000000000000000000000000000000000000..8d38dc43d1c323602becfc77297aaabc1a3dc10e --- /dev/null +++ b/mcphttp/cache.go @@ -0,0 +1,93 @@ +package mcphttp + +import "net/http" + +const ( + // cacheControl is what every response of an MCP endpoint carries. + // + // private and no-store are middleware.SetPrivateCache's pair, unchanged and + // for its reason: an answer here depends entirely on the credential the + // request carried and says nothing about it in the URL, and it may be a + // PRIVATE repository's data. no-cache would not do — it still permits a cache + // to *store* the body and merely revalidate, which is the thing no-store + // forbids. + // + // no-transform is the SDK's own, kept. The 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" + + // vary names what an answer here actually depends on: the unified-login + // cookie and the bearer token. Same two names the rest of the instance varies + // on, because it is the same statement. + vary = "Cookie, Authorization" +) + +// PrivateCache marks every response an MCP endpoint writes as one no cache may +// keep, and states what it depends on. +// +// It is not middleware.PrivateCache and cannot be. That one sets the headers +// before the handler runs, which is right for a router whose handlers do not +// touch Cache-Control; 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. +// +// The result is not a replacement of what the SDK asked for but a narrowing of +// it — see cacheControl. +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, whether that is an explicit +// WriteHeader or the implicit one of the first Write. +type cacheWriter struct { + http.ResponseWriter + committed bool +} + +func (w *cacheWriter) WriteHeader(status int) { + w.commit() + w.ResponseWriter.WriteHeader(status) +} + +func (w *cacheWriter) Write(b []byte) (int, error) { + w.commit() + return w.ResponseWriter.Write(b) +} + +// Flush commits before flushing so a handler that streams without ever calling +// WriteHeader still leaves with the headers. http.NewResponseController prefers a +// Flush on the writer it is handed over one reached through Unwrap, so this +// method is what it finds — without it the flush would commit the response at the +// writer below and the two headers would never be written. +func (w *cacheWriter) Flush() { + w.commit() + //nolint:errcheck // http.Flusher.Flush reports nothing; the controller's error + // is only about the writer not supporting flush, which Unwrap guarantees it does. + _ = 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", vary) +} + +// Unwrap is load-bearing, not boilerplate: it is what keeps the streamable +// transport working through this wrapper. http.NewResponseController follows +// Unwrap to reach the real writer's Flush, and an SSE stream that could not be +// flushed would be a response no client sees until the handler returns — which, +// for a stream, is a response nobody sees at all. Deleting this method breaks +// streaming while leaving every status code and header test green, so +// TestUnwrapReachesTheUnderlyingFlusher exists to fail instead. +func (w *cacheWriter) Unwrap() http.ResponseWriter { return w.ResponseWriter } diff --git a/mcphttp/cache_test.go b/mcphttp/cache_test.go new file mode 100644 index 0000000000000000000000000000000000000000..a7536ac704f876e6f66f1e25e60ada0eb95bca62 --- /dev/null +++ b/mcphttp/cache_test.go @@ -0,0 +1,146 @@ +package mcphttp_test + +import ( + "bufio" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-ecore/mcphttp" +) + +const ( + wantCacheControl = "private, no-store, no-transform" + wantVary = "Cookie, Authorization" +) + +// TestHeadersLandOnAnExplicitWriteHeader is the ordinary path: a handler that +// commits with WriteHeader. +func TestHeadersLandOnAnExplicitWriteHeader(t *testing.T) { + h := mcphttp.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, wantCacheControl, rec.Header().Get("Cache-Control")) + assert.Equal(t, wantVary, rec.Header().Get("Vary")) +} + +// TestHeadersLandOnAnImplicitCommit covers the handler that never calls +// WriteHeader at all. net/http commits on the first Write, and headers set after +// that point are dropped silently — so the wrapper has to catch Write too. +func TestHeadersLandOnAnImplicitCommit(t *testing.T) { + h := mcphttp.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, wantCacheControl, rec.Header().Get("Cache-Control")) + assert.Equal(t, wantVary, rec.Header().Get("Vary")) + assert.Equal(t, `{"jsonrpc":"2.0"}`, rec.Body.String()) +} + +// TestTheSDKsOwnDirectivesAreOverridden is the reason this wrapper exists rather +// than middleware.PrivateCache. The streamable transport sets Cache-Control with +// Set from inside the handler, so anything written on the way in loses. What +// arrives here is a handler doing exactly what the SDK does. +func TestTheSDKsOwnDirectivesAreOverridden(t *testing.T) { + h := mcphttp.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, wantCacheControl, rec.Header().Get("Cache-Control"), + "no-cache permits a cache to store the body and revalidate, which is what no-store forbids") + assert.Equal(t, wantVary, rec.Header().Get("Vary")) +} + +// TestOnlyTheFirstCommitWrites checks the committed flag: a handler that writes +// its own header after committing is doing something net/http would ignore +// anyway, and the wrapper must not undo a status that is already on the wire by +// re-running its Set on every Write. +func TestOnlyTheFirstCommitWrites(t *testing.T) { + h := mcphttp.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{wantCacheControl}, rec.Header().Values("Cache-Control")) + assert.Equal(t, []string{wantVary}, rec.Header().Values("Vary")) + assert.Equal(t, "ab", rec.Body.String()) +} + +// TestUnwrapReachesTheUnderlyingFlusher is the test the Unwrap method exists +// for. +// +// cacheWriter embeds the http.ResponseWriter *interface*, so it promotes no +// Flush of its own; http.NewResponseController can only reach the real writer's +// through Unwrap. Delete the method and this fails with ErrNotSupported while +// every status-code and header test above stays green — which is precisely why +// it is written as a test and not as a comment. +func TestUnwrapReachesTheUnderlyingFlusher(t *testing.T) { + var flushErr error + h := mcphttp.PrivateCache(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("event: message\n")) + flushErr = http.NewResponseController(w).Flush() + })) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/mcp", nil)) + + require.NoError(t, flushErr, "an SSE stream that cannot be flushed is a response no client sees") + assert.True(t, rec.Flushed) + assert.Equal(t, wantCacheControl, rec.Header().Get("Cache-Control")) +} + +// TestAStreamReachesTheClientBeforeTheHandlerReturns is the same property +// measured rather than asserted: over a real connection, with a real client, +// bytes written and 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. +func TestAStreamReachesTheClientBeforeTheHandlerReturns(t *testing.T) { + release := make(chan struct{}) + srv := httptest.NewServer(mcphttp.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.NewRequest(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, wantCacheControl, resp.Header.Get("Cache-Control")) + assert.Equal(t, wantVary, 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/mcphttp/hostguard.go b/mcphttp/hostguard.go new file mode 100644 index 0000000000000000000000000000000000000000..e668e450db66f465c8ee1a732927cc56abad6ef9 --- /dev/null +++ b/mcphttp/hostguard.go @@ -0,0 +1,95 @@ +package mcphttp + +import ( + "errors" + "fmt" + "net" + "net/http" + "strings" + + "sourcecraft.dev/bigbes/sr-ht-ecore/instconf" +) + +// ErrNoOriginHost is returned by [HostGuard] when the origin it was handed names +// no host to build an allowlist from — it is empty, it does not parse, or it +// parses to no hostname. Callers match it with errors.Is; the wrapped error +// quotes the origin. +var ErrNoOriginHost = errors.New("mcphttp: origin names no host") + +// HostGuard is an MCP endpoint's DNS-rebinding protection in the form this +// deployment needs: Host must name the instance's own origin, or be a loopback +// name for local development. +// +// It is a wrapper rather than a check inside the handler so that the refusal +// happens before the SDK sees a byte of the body. It replaces the SDK's own +// guard, which [StreamableOptions] turns off — see there for why that trade is +// the safe direction. +// +// The expected host is [instconf.OriginHost]'s, which is the instance's one +// reading of "what host does this origin name": the name without the port, and +// "" for anything that does not parse. Never a guessed "localhost", which is +// what one earlier copy answered and which would have made every malformed +// origin agree with a local client on the one code path that decides an +// allowlist. +// +// # Fail-closed, and the stance that lost +// +// An origin with no host is a construction error here, not a warning. The two +// donors disagreed about this and the disagreement is worth recording rather +// than quietly resolving. +// +// cov.sr.ht refuses to build the surface at all: an origin is a required +// configuration key, config validation already requires it to parse and to carry +// a host, so a daemon that reached this call has one and a caller that did not +// is a bug rather than an operator to be warned. bench.sr.ht logs "Host +// validation on the MCP endpoint is DISABLED" and serves the endpoint +// unguarded, reasoning that refusing every request is a worse answer to a +// misconfiguration than checking no request. +// +// This package takes cov's side. The failure bench's stance produces is silent +// in exactly the deployment where it matters: the warning is one line at startup +// among many, and what follows it is a service that works — it answers every +// request, including the rebinding attack's. A service that refuses to start +// says the same thing in the one register nobody can scroll past. And the +// premise that made bench's choice cheap is the premise that makes it +// unnecessary: if config validation already requires an origin, the open path is +// unreachable by any real daemon, so keeping it buys nothing and costs a +// security property. bench has a test pinning its behaviour +// (TestNoOriginLeavesTheEndpointOpen) and retrofitting that is bench's to do, +// not this package's to force. +func HostGuard(next http.Handler, origin string) (http.Handler, error) { + want := instconf.OriginHost(origin) + if want == "" { + return nil, fmt.Errorf("%w: %q has no host to guard the MCP endpoint with", ErrNoOriginHost, origin) + } + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !hostAllowed(r.Host, want) { + http.Error(w, "Forbidden: unexpected Host header", http.StatusForbidden) + return + } + next.ServeHTTP(w, r) + }), nil +} + +// hostAllowed compares a request's Host against the expected hostname, ignoring +// any port and IPv6 brackets. +// +// Loopback names stay allowed on purpose: a developer running the daemon by hand +// and a local MCP client pointed at it address it as localhost, and those names +// cannot be a rebinding attack's — an attacker's page has to carry a name it +// controls. +func hostAllowed(reqHost, want string) bool { + h := reqHost + if stripped, _, err := net.SplitHostPort(h); err == nil { + h = stripped + } + h = strings.TrimSuffix(strings.TrimPrefix(h, "["), "]") + switch { + case strings.EqualFold(h, want): + return true + case h == "localhost", h == "127.0.0.1", h == "::1": + return true + default: + return false + } +} diff --git a/mcphttp/hostguard_test.go b/mcphttp/hostguard_test.go new file mode 100644 index 0000000000000000000000000000000000000000..9de6d7cb9e8c22c49f40eab53af2494ae8a2d7b6 --- /dev/null +++ b/mcphttp/hostguard_test.go @@ -0,0 +1,148 @@ +package mcphttp_test + +import ( + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-ecore/mcphttp" +) + +const testOrigin = "https://mcp.example.org" + +// okHandler is what the guard protects: anything that reaches it answers 200, so +// a test's status code says whether the guard let the request through. +func okHandler(reached *bool) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if reached != nil { + *reached = true + } + w.WriteHeader(http.StatusOK) + }) +} + +func guard(t *testing.T, origin string, reached *bool) http.Handler { + t.Helper() + h, err := mcphttp.HostGuard(okHandler(reached), origin) + require.NoError(t, err) + return h +} + +// TestProxiedHostIsAccepted pins the deployment shape, which is invisible +// otherwise: the daemon listens on loopback and nginx forwards the instance's +// public Host. Without this the endpoint could be guarded into refusing every +// production request while passing every local check. +func TestProxiedHostIsAccepted(t *testing.T) { + for _, host := range []string{"mcp.example.org", "mcp.example.org:443", "MCP.EXAMPLE.ORG"} { + t.Run(host, func(t *testing.T) { + reached := false + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + req.Host = host + rec := httptest.NewRecorder() + + guard(t, testOrigin, &reached).ServeHTTP(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.True(t, reached, "the guard must pass the instance's own host through") + }) + } +} + +// TestUnexpectedHostIsRefused is the guard doing its job: a rebinding attack +// carries a name the attacker controls, and the suffix case is here because a +// check written with HasSuffix instead of an equality would let it in. +func TestUnexpectedHostIsRefused(t *testing.T) { + for _, host := range []string{ + "evil.example", + "mcp.example.org.evil.example", + "evilmcp.example.org", + "10.0.0.5", + "10.0.0.5:5093", + "", + } { + t.Run(host, func(t *testing.T) { + reached := false + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + req.Host = host + rec := httptest.NewRecorder() + + guard(t, testOrigin, &reached).ServeHTTP(rec, req) + + assert.Equal(t, http.StatusForbidden, rec.Code) + assert.False(t, reached, "a refused request must not reach the handler behind the guard") + }) + } +} + +// TestLoopbackHostsStayAllowed is the concession the guard makes on purpose: a +// developer running the daemon by hand, and a local MCP client pointed at it, +// address it by a loopback name, and no attacker's page can carry one. +func TestLoopbackHostsStayAllowed(t *testing.T) { + for _, host := range []string{"localhost", "localhost:5093", "127.0.0.1", "127.0.0.1:5093", "[::1]", "[::1]:5093"} { + t.Run(host, func(t *testing.T) { + reached := false + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + req.Host = host + rec := httptest.NewRecorder() + + guard(t, testOrigin, &reached).ServeHTTP(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.True(t, reached) + }) + } +} + +// TestAnOriginWithNoHostIsAConstructionError is the fail-closed stance, pinned. +// The alternative a donor took — warn and serve unguarded — is a service that +// works, which is exactly why nobody discovers it; this must be an error a +// daemon cannot start through. +func TestAnOriginWithNoHostIsAConstructionError(t *testing.T) { + for name, origin := range map[string]string{ + "empty": "", + "blank": " ", + "schemeless": "mcp.example.org", + "path only": "/mcp", + "unparseable": "://mcp.example.org", + "scheme alone": "https://", + } { + t.Run(name, func(t *testing.T) { + h, err := mcphttp.HostGuard(okHandler(nil), origin) + + require.Error(t, err) + assert.Nil(t, h, "a refused guard must not hand back a handler somebody could mount anyway") + assert.ErrorIs(t, err, mcphttp.ErrNoOriginHost) + assert.Contains(t, err.Error(), origin, "the error must quote the origin an operator has to fix") + }) + } +} + +// TestErrNoOriginHostIsMatchable states the contract callers rely on: they +// classify with errors.Is, not by reading the message. +func TestErrNoOriginHostIsMatchable(t *testing.T) { + _, err := mcphttp.HostGuard(okHandler(nil), "") + require.Error(t, err) + assert.True(t, errors.Is(err, mcphttp.ErrNoOriginHost)) +} + +// TestARefusalByHostnameIsUncacheable pins the composition the package +// documents: PrivateCache wraps HostGuard, so a 403 written before the SDK is +// reached at all still carries the directives. A cached 403 would be its own +// bug, and the ordering that prevents it is easy to reverse by accident. +func TestARefusalByHostnameIsUncacheable(t *testing.T) { + h := mcphttp.PrivateCache(guard(t, testOrigin, nil)) + + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + req.Host = "evil.example" + rec := httptest.NewRecorder() + + h.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusForbidden, rec.Code) + assert.Equal(t, "private, no-store, no-transform", rec.Header().Get("Cache-Control")) + assert.Equal(t, "Cookie, Authorization", rec.Header().Get("Vary")) +} diff --git a/mcphttp/mcphttp.go b/mcphttp/mcphttp.go new file mode 100644 index 0000000000000000000000000000000000000000..f850020f30a818bb21a07de1ff608ad962816911 --- /dev/null +++ b/mcphttp/mcphttp.go @@ -0,0 +1,99 @@ +// Package mcphttp is the HTTP-level plumbing an MCP endpoint on this instance +// needs around the Go SDK's streamable transport: the Host allowlist that +// replaces the SDK's own DNS-rebinding guard, the cache directives that keep a +// private repository's data out of every cache, and the two transport options +// both services set. +// +// It is deliberately three functions and not a framework. cov.sr.ht and +// bench.sr.ht each mount an MCP surface, and an audit of the two found the +// genuinely shared part to be about forty lines — everything else differs +// because it was meant to. Their tool sets are their own, their service +// interfaces are their own, and above all their authentication gates are their +// own: cov refuses a caller with no read grant outright, bench serves an +// anonymous caller everything PUBLIC. Unifying those would not be deduplication, +// it would be a policy change smuggled in as one. So the gates stay in the +// services and only what is identical in both, or identical once one of the two +// stances is chosen, lives here. +// +// The three: +// +// - [HostGuard], the Host-header allowlist. The predicate was already +// byte-for-byte identical in both donors. +// - [PrivateCache], the response wrapper that writes the cache directives at +// the moment the response is committed, because the SDK writes its own on +// the way out and a middleware that set them on the way in loses. +// - [StreamableOptions], the transport options: stateless sessions, SDK +// rebinding guard off because [HostGuard] replaces it. +// +// A service composes them around its own gate, outermost first: +// +// h := mcp.NewStreamableHTTPHandler( +// func(*http.Request) *mcp.Server { return srv }, +// mcphttp.StreamableOptions(), +// ) +// guarded, err := mcphttp.HostGuard(myAuthGate(h), origin) +// if err != nil { +// return err +// } +// r.Handle("/mcp", mcphttp.PrivateCache(guarded)) +// +// The cache wrapper goes outside the Host guard on purpose: a 403 by hostname is +// as unstorable as an answer, and it is written before the SDK is reached at all. +package mcphttp + +import ( + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// StreamableOptions is the streamable transport's configuration for an MCP +// endpoint deployed the way this instance deploys them. Both donors set exactly +// these two fields and nothing else, and each of them is load-bearing. +// +// # Stateless, because a stateful session is itself a credential +// +// In the SDK's stateful mode a session spans requests, and a tool handler runs +// under the context of the request that *initialised* the session — not the one +// that carried the call. Every surface here reads the caller off that context. +// So the session id becomes the credential: whoever presents it is answered as +// whoever opened the session, having presented nothing themselves. A session +// opened anonymously keeps answering as anonymous even when a later request +// carries a token, and a token revoked mid-session keeps working until the +// client reconnects. A session id is not a secret in the way a token is — it +// travels in a plain header, lands in proxy logs, crash reports and a client's +// state file — and none of the machinery that mints, scopes, expires and revokes +// tokens applies to it. +// +// Stateless mode gives each POST its own throwaway session and its own context, +// so the identity a tool sees is the identity of the request that carried it, by +// construction rather than by care. What it costs is the server-to-client half +// of the protocol: no standalone SSE stream, therefore no server-initiated +// requests, and a bare GET is answered 405. Both services' tools are reads that +// answer in one response and none of them samples, elicits or reports progress, +// so there is nothing to give up. +// +// # DisableLocalhostProtection, because the SDK's guard cannot see this proxy +// +// Turning off a security default is usually a mistake, so: the SDK's guard +// refuses any request that arrives on a loopback address while carrying a +// non-loopback Host. That is precisely this deployment — every daemon binds +// 127.0.0.1 behind nginx, which forwards with the instance's public Host — so +// every genuine request would be a 403, and only in production, since a local +// client sends a loopback Host and passes. +// +// The guard does have something real to catch: a browser on the daemon's own +// host can reach the loopback port directly with an attacker's Host. It simply +// cannot tell that request from nginx's, because both arrive from loopback with +// a non-loopback Host, and the SDK exposes no allowlist to separate them. So it +// is disabled and *replaced* by [HostGuard], which requires Host to name this +// instance where the SDK asked only whether it was loopback. Disabling it +// without that replacement would be a regression rather than a formality, which +// is why the two are documented as one decision. +// +// A fresh value is returned on every call: the SDK takes a pointer, and a shared +// one would let any caller reconfigure every other endpoint. +func StreamableOptions() *mcp.StreamableHTTPOptions { + return &mcp.StreamableHTTPOptions{ + Stateless: true, + DisableLocalhostProtection: true, + } +} diff --git a/mcphttp/options_test.go b/mcphttp/options_test.go new file mode 100644 index 0000000000000000000000000000000000000000..ec43c3de1422caf10983ef26c19c160d119c8fad --- /dev/null +++ b/mcphttp/options_test.go @@ -0,0 +1,42 @@ +package mcphttp_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-ecore/mcphttp" +) + +// TestStatelessSessions pins the option that decides where a tool handler's +// identity comes from. With Stateless off, the SDK hands every call the context +// of the request that initialised the session, so the session id becomes a +// credential nobody minted, scoped or can revoke. That is a security change that +// would show up as no compile error and no failing tool test. +func TestStatelessSessions(t *testing.T) { + opts := mcphttp.StreamableOptions() + require.NotNil(t, opts) + + assert.True(t, opts.Stateless, "a stateful session authenticates by session id") +} + +// TestTheSDKGuardIsDisabled pins the other half of the deployment decision. It is +// only defensible together with HostGuard, so read it next to +// TestUnexpectedHostIsRefused: this option removes a check and that test is the +// replacement. +func TestTheSDKGuardIsDisabled(t *testing.T) { + assert.True(t, mcphttp.StreamableOptions().DisableLocalhostProtection, + "the SDK guard refuses every request nginx forwards, and only in production") +} + +// TestEachCallGetsItsOwnValue keeps one endpoint from reconfiguring another: the +// SDK takes a pointer and would happily hold a shared one. +func TestEachCallGetsItsOwnValue(t *testing.T) { + a, b := mcphttp.StreamableOptions(), mcphttp.StreamableOptions() + + require.NotSame(t, a, b) + + a.Stateless = false + assert.True(t, b.Stateless, "one caller's mutation must not reach another's options") +}