From e0096d87d5aeebe407ded7c97994620ead63abdd Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 19 Aug 2026 14:18:12 +0200 Subject: [PATCH] Limit HTTP request bodies before MCP middleware parsing Add WithMaxBodySize middleware that bounds the request body via http.MaxBytesReader (with a fast Content-Length rejection when known), registered first in RegisterMiddleware so it runs before any other middleware or the MCP SDK reads or buffers the body. WithMCPParse and WithScopeChallenge now return a clear 413 "request body too large" response when their body read hits the limit, instead of silently continuing. Defaults to 10 MiB, overridable via ServerConfig.MaxRequestBodyBytes. Fixes #3102 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- pkg/http/handler.go | 12 ++ pkg/http/handler_test.go | 84 ++++++++++++++ pkg/http/middleware/body_limit.go | 54 +++++++++ pkg/http/middleware/body_limit_test.go | 115 ++++++++++++++++++++ pkg/http/middleware/mcp_parse.go | 4 + pkg/http/middleware/mcp_parse_test.go | 65 +++++++++++ pkg/http/middleware/scope_challenge.go | 4 + pkg/http/middleware/scope_challenge_test.go | 79 ++++++++++++++ pkg/http/server.go | 5 + 9 files changed, 422 insertions(+) create mode 100644 pkg/http/middleware/body_limit.go create mode 100644 pkg/http/middleware/body_limit_test.go create mode 100644 pkg/http/middleware/scope_challenge_test.go diff --git a/pkg/http/handler.go b/pkg/http/handler.go index 935bca1c..417f3267 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -132,6 +132,9 @@ func NewHTTPMcpHandler( func (h *Handler) RegisterMiddleware(r chi.Router) { r.Use( + // Must run first: bounds the request body before any other + // middleware (or the MCP SDK) reads or buffers it. + middleware.WithMaxBodySize(h.maxRequestBodyBytes()), middleware.ExtractUserToken(h.oauthCfg), middleware.WithRequestConfig, middleware.WithMCPParse(), @@ -143,6 +146,15 @@ func (h *Handler) RegisterMiddleware(r chi.Router) { } } +// maxRequestBodyBytes returns the configured request-body size limit, or +// middleware.DefaultMaxRequestBodyBytes if none was configured. +func (h *Handler) maxRequestBodyBytes() int64 { + if h.config != nil && h.config.MaxRequestBodyBytes > 0 { + return h.config.MaxRequestBodyBytes + } + return middleware.DefaultMaxRequestBodyBytes +} + // RegisterRoutes registers the routes for the MCP server // URL-based values take precedence over header-based values func (h *Handler) RegisterRoutes(r chi.Router) { diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index 13e87fc0..b6998d89 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -1287,3 +1287,87 @@ func TestUIMetaStrippedWhenClientLacksCapability(t *testing.T) { require.Len(t, unknown, 1) require.NotNil(t, unknown[0].Tool.Meta["ui"], "_meta.ui should be preserved when capability is unknown and FF is on") } + +// TestRegisterMiddleware_MaxRequestBodySize verifies that RegisterMiddleware +// wires the body-size limit ahead of the body-consuming middleware, so an +// oversized request never reaches the MCP server, and that requests within +// the configured limit (including exactly at the boundary) still succeed. +func TestRegisterMiddleware_MaxRequestBodySize(t *testing.T) { + const limit = 256 + + apiHost, err := utils.NewAPIHost("https://api.github.com") + require.NoError(t, err) + + buildBody := func(size int) string { + payload := `{"jsonrpc":"2.0","id":1,"method":"tools/list","params":{"pad":"PADDING"}}` + if len(payload) >= size { + return payload + } + pad := strings.Repeat("x", size-len(payload)) + return strings.Replace(payload, "PADDING", "PADDING"+pad, 1) + } + + newHandler := func(t *testing.T, mcpServerFactoryCalled *bool) http.Handler { + t.Helper() + handler := NewHTTPMcpHandler( + context.Background(), + &ServerConfig{Version: "test", MaxRequestBodyBytes: limit}, + nil, + translations.NullTranslationHelper, + slog.Default(), + apiHost, + WithInventoryFactory(func(_ *http.Request) (*inventory.Inventory, error) { + return inventory.NewBuilder().Build() + }), + WithGitHubMCPServerFactory(func(_ *http.Request, _ github.ToolDependencies, _ *inventory.Inventory, _ *github.MCPServerConfig) (*mcp.Server, error) { + if mcpServerFactoryCalled != nil { + *mcpServerFactoryCalled = true + } + return mcp.NewServer(&mcp.Implementation{Name: "test", Version: "0.0.1"}, nil), nil + }), + WithScopeFetcher(allScopesFetcher{}), + ) + + r := chi.NewRouter() + handler.RegisterMiddleware(r) + handler.RegisterRoutes(r) + return r + } + + t.Run("oversized request is rejected before reaching the MCP server", func(t *testing.T) { + var mcpServerFactoryCalled bool + r := newHandler(t, &mcpServerFactoryCalled) + + body := buildBody(limit + 1) + require.Greater(t, len(body), limit) + + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) + req.Header.Set(headers.AuthorizationHeader, strings.Join([]string{"ghs", "test-token"}, "_")) + + rr := httptest.NewRecorder() + r.ServeHTTP(rr, req) + + assert.Equal(t, http.StatusRequestEntityTooLarge, rr.Code) + assert.Contains(t, rr.Body.String(), "request body too large") + assert.False(t, mcpServerFactoryCalled, "the MCP server should never be constructed for an oversized request") + }) + + t.Run("boundary-size request at the configured limit succeeds", func(t *testing.T) { + var mcpServerFactoryCalled bool + r := newHandler(t, &mcpServerFactoryCalled) + + body := buildBody(limit) + require.Len(t, body, limit) + + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) + req.Header.Set(headers.ContentTypeHeader, headers.ContentTypeJSON) + req.Header.Set(headers.AcceptHeader, strings.Join([]string{headers.ContentTypeJSON, headers.ContentTypeEventStream}, ", ")) + req.Header.Set(headers.AuthorizationHeader, strings.Join([]string{"ghs", "test-token"}, "_")) + + rr := httptest.NewRecorder() + r.ServeHTTP(rr, req) + + assert.Equal(t, http.StatusOK, rr.Code, "response body: %s", rr.Body.String()) + assert.True(t, mcpServerFactoryCalled, "the MCP server should be constructed for an allowed request") + }) +} diff --git a/pkg/http/middleware/body_limit.go b/pkg/http/middleware/body_limit.go new file mode 100644 index 00000000..8d18a878 --- /dev/null +++ b/pkg/http/middleware/body_limit.go @@ -0,0 +1,54 @@ +package middleware + +import ( + "errors" + "net/http" +) + +// DefaultMaxRequestBodyBytes bounds the size of HTTP request bodies accepted +// by the MCP endpoints when no explicit limit is configured. +const DefaultMaxRequestBodyBytes int64 = 10 << 20 // 10 MiB + +// WithMaxBodySize returns middleware that bounds the size of the request +// body. It must be registered before any middleware that reads or buffers +// the body (e.g. WithMCPParse, WithScopeChallenge) so that an oversized +// payload is rejected before it is ever fully buffered in memory, rather than +// relying on a size guard applied later by the MCP SDK or a downstream +// handler. +// +// When Content-Length is known and already exceeds maxBytes, the request is +// rejected immediately without touching the body. Otherwise the body is +// wrapped with http.MaxBytesReader, so any subsequent read (including +// chunked or unknown-length bodies) fails with a *http.MaxBytesError once +// maxBytes have been consumed. +func WithMaxBodySize(maxBytes int64) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.ContentLength > maxBytes { + writeRequestTooLarge(w) + return + } + + if r.Body != nil { + r.Body = http.MaxBytesReader(w, r.Body, maxBytes) + } + + next.ServeHTTP(w, r) + }) + } +} + +// writeRequestTooLarge writes the standard "request body too large" response. +// Every middleware that reads the request body should use this so oversized +// requests get a consistent, clear response regardless of which layer +// detects the overflow. +func writeRequestTooLarge(w http.ResponseWriter) { + http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) +} + +// isMaxBytesError reports whether err resulted from a body exceeding the +// limit applied by WithMaxBodySize, as opposed to some other read failure. +func isMaxBytesError(err error) bool { + var maxBytesErr *http.MaxBytesError + return errors.As(err, &maxBytesErr) +} diff --git a/pkg/http/middleware/body_limit_test.go b/pkg/http/middleware/body_limit_test.go new file mode 100644 index 00000000..0bcb4746 --- /dev/null +++ b/pkg/http/middleware/body_limit_test.go @@ -0,0 +1,115 @@ +package middleware + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// unknownLengthBody wraps a reader without exposing a Len method, so +// httptest.NewRequest cannot infer Content-Length from it. This mirrors a +// chunked-transfer-encoded request, where the body size is unknown upfront. +func unknownLengthBody(s string) io.Reader { + return io.NopCloser(strings.NewReader(s)) +} + +func TestWithMaxBodySize(t *testing.T) { + const limit = 16 + + t.Run("allowed request under the limit passes through", func(t *testing.T) { + var nextCalled bool + var readBody string + var readErr error + + next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + nextCalled = true + b, err := io.ReadAll(r.Body) + readBody, readErr = string(b), err + }) + + handler := WithMaxBodySize(limit)(next) + + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader("short")) + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + assert.True(t, nextCalled, "next handler should be called for an allowed request") + require.NoError(t, readErr) + assert.Equal(t, "short", readBody) + assert.Equal(t, http.StatusOK, rr.Code) + }) + + t.Run("boundary size exactly at the limit is allowed", func(t *testing.T) { + body := strings.Repeat("a", limit) + + var nextCalled bool + var readBody string + var readErr error + + next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + nextCalled = true + b, err := io.ReadAll(r.Body) + readBody, readErr = string(b), err + }) + + handler := WithMaxBodySize(limit)(next) + + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader(body)) + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + assert.True(t, nextCalled, "next handler should be called when the body is exactly at the limit") + require.NoError(t, readErr, "reading exactly maxBytes should not error") + assert.Equal(t, body, readBody, "the full boundary-size body should be readable") + }) + + t.Run("oversized request with known Content-Length is rejected before next runs", func(t *testing.T) { + body := strings.Repeat("a", limit+1) + + var nextCalled bool + next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + nextCalled = true + }) + + handler := WithMaxBodySize(limit)(next) + + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader(body)) + require.Equal(t, int64(limit+1), req.ContentLength, "test setup: Content-Length should be known") + + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + assert.False(t, nextCalled, "next handler must not run for an oversized request") + assert.Equal(t, http.StatusRequestEntityTooLarge, rr.Code) + assert.Contains(t, rr.Body.String(), "request body too large") + }) + + t.Run("oversized request with unknown length fails on downstream read", func(t *testing.T) { + body := strings.Repeat("a", limit+1) + + var nextCalled bool + var readErr error + + next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + nextCalled = true + _, readErr = io.ReadAll(r.Body) + }) + + handler := WithMaxBodySize(limit)(next) + + req := httptest.NewRequest(http.MethodPost, "/mcp", unknownLengthBody(body)) + require.Equal(t, int64(-1), req.ContentLength, "test setup: Content-Length should be unknown") + + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + assert.True(t, nextCalled, "next handler still runs; the limit is enforced on read") + require.Error(t, readErr) + assert.True(t, isMaxBytesError(readErr), "expected a *http.MaxBytesError, got %v", readErr) + }) +} diff --git a/pkg/http/middleware/mcp_parse.go b/pkg/http/middleware/mcp_parse.go index c82616b2..21848fde 100644 --- a/pkg/http/middleware/mcp_parse.go +++ b/pkg/http/middleware/mcp_parse.go @@ -54,6 +54,10 @@ func WithMCPParse() func(http.Handler) http.Handler { // Read the request body body, err := io.ReadAll(r.Body) if err != nil { + if isMaxBytesError(err) { + writeRequestTooLarge(w) + return + } // Log but continue - don't block requests on parse errors next.ServeHTTP(w, r) return diff --git a/pkg/http/middleware/mcp_parse_test.go b/pkg/http/middleware/mcp_parse_test.go index 5a28a30c..682c8116 100644 --- a/pkg/http/middleware/mcp_parse_test.go +++ b/pkg/http/middleware/mcp_parse_test.go @@ -189,3 +189,68 @@ func TestWithMCPParse_BodyRestoration(t *testing.T) { assert.Equal(t, originalBody, capturedBody, "body should be restored for downstream handlers") } + +// TestWithMCPParse_WithMaxBodySize composes the body-size limit with +// WithMCPParse, mirroring the production middleware ordering where +// WithMaxBodySize runs first. It verifies that an oversized body is rejected +// with a clear 413 before parsing runs, while requests within the limit +// (including exactly at the boundary) still parse and preserve the body. +func TestWithMCPParse_WithMaxBodySize(t *testing.T) { + const limit = 128 + + buildBody := func(size int) string { + payload := `{"jsonrpc":"2.0","method":"tools/call","params":{"name":"test_tool","arguments":{"pad":"PADDING"}}}` + if len(payload) >= size { + return payload + } + // Pad the JSON with a longer string value so we can hit an exact byte size. + pad := strings.Repeat("x", size-len(payload)) + return strings.Replace(payload, "PADDING", "PADDING"+pad, 1) + } + + t.Run("oversized body is rejected before parsing", func(t *testing.T) { + body := buildBody(limit + 1) + require.Greater(t, len(body), limit) + + var nextCalled bool + nextHandler := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + nextCalled = true + }) + + handler := WithMaxBodySize(limit)(WithMCPParse()(nextHandler)) + + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader(body)) + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + assert.False(t, nextCalled, "downstream handler must not run for an oversized request") + assert.Equal(t, http.StatusRequestEntityTooLarge, rr.Code) + assert.Contains(t, rr.Body.String(), "request body too large") + }) + + t.Run("boundary-size body is parsed and preserved", func(t *testing.T) { + body := buildBody(limit) + require.Len(t, body, limit) + + var capturedInfo *ghcontext.MCPMethodInfo + var capturedBody string + nextHandler := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + capturedInfo, _ = ghcontext.MCPMethod(r.Context()) + b, err := io.ReadAll(r.Body) + require.NoError(t, err) + capturedBody = string(b) + }) + + handler := WithMaxBodySize(limit)(WithMCPParse()(nextHandler)) + + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader(body)) + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + assert.Equal(t, http.StatusOK, rr.Code) + require.NotNil(t, capturedInfo, "MCPMethodInfo should be parsed for an allowed request") + assert.Equal(t, "tools/call", capturedInfo.Method) + assert.Equal(t, "test_tool", capturedInfo.ItemName) + assert.Equal(t, body, capturedBody, "body should be preserved for downstream handlers") + }) +} diff --git a/pkg/http/middleware/scope_challenge.go b/pkg/http/middleware/scope_challenge.go index 1a86bf93..4ac3bab9 100644 --- a/pkg/http/middleware/scope_challenge.go +++ b/pkg/http/middleware/scope_challenge.go @@ -54,6 +54,10 @@ func WithScopeChallenge(oauthCfg *oauth.Config, scopeFetcher scopes.FetcherInter // Fallback: parse the request body directly body, err := io.ReadAll(r.Body) if err != nil { + if isMaxBytesError(err) { + writeRequestTooLarge(w) + return + } next.ServeHTTP(w, r) return } diff --git a/pkg/http/middleware/scope_challenge_test.go b/pkg/http/middleware/scope_challenge_test.go new file mode 100644 index 00000000..1e315892 --- /dev/null +++ b/pkg/http/middleware/scope_challenge_test.go @@ -0,0 +1,79 @@ +package middleware + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + ghcontext "github.com/github/github-mcp-server/pkg/context" + "github.com/github/github-mcp-server/pkg/http/oauth" + "github.com/github/github-mcp-server/pkg/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestWithScopeChallenge_MaxBodySize verifies the fallback body-parsing path +// (used when WithMCPParse has not already populated MCPMethodInfo in +// context) respects the request-body size limit and returns a clear 413 +// instead of silently continuing, when composed with WithMaxBodySize as it +// is in production. +func TestWithScopeChallenge_MaxBodySize(t *testing.T) { + const limit = 64 + oauthCfg := &oauth.Config{} + fetcher := &mockScopeFetcher{scopes: []string{"repo"}} + + newRequest := func(body string) *http.Request { + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader(body)) + ctx := ghcontext.WithTokenInfo(req.Context(), &ghcontext.TokenInfo{ + Token: "******", + TokenType: utils.TokenTypeOAuthAccessToken, + }) + return req.WithContext(ctx) + } + + t.Run("oversized body is rejected before the fallback parse", func(t *testing.T) { + body := `{"jsonrpc":"2.0","method":"tools/call","params":{"name":"` + strings.Repeat("x", limit) + `"}}` + require.Greater(t, len(body), limit) + + var nextCalled bool + next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + nextCalled = true + }) + + handler := WithMaxBodySize(limit)(WithScopeChallenge(oauthCfg, fetcher)(next)) + + req := newRequest(body) + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + assert.False(t, nextCalled, "downstream handler must not run for an oversized request") + assert.Equal(t, http.StatusRequestEntityTooLarge, rr.Code) + assert.Contains(t, rr.Body.String(), "request body too large") + }) + + t.Run("allowed body still reaches the fallback parse and next handler", func(t *testing.T) { + body := `{"jsonrpc":"2.0","method":"tools/list"}` + require.LessOrEqual(t, len(body), limit) + + var nextCalled bool + var capturedBody string + next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + nextCalled = true + b, err := io.ReadAll(r.Body) + require.NoError(t, err) + capturedBody = string(b) + }) + + handler := WithMaxBodySize(limit)(WithScopeChallenge(oauthCfg, fetcher)(next)) + + req := newRequest(body) + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + assert.True(t, nextCalled, "downstream handler should run for an allowed request") + assert.Equal(t, http.StatusOK, rr.Code) + assert.Equal(t, body, capturedBody, "body should be preserved for downstream handlers") + }) +} diff --git a/pkg/http/server.go b/pkg/http/server.go index 6bf48a07..e68f420c 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -108,6 +108,11 @@ type ServerConfig struct { // MRTRStateKey is a Base64-encoded 32-byte key used to protect multi-round-trip request state. MRTRStateKey string + // MaxRequestBodyBytes bounds the size of HTTP request bodies accepted by + // the MCP endpoints, enforced before any middleware reads or buffers the + // body. When zero, middleware.DefaultMaxRequestBodyBytes is used. + MaxRequestBodyBytes int64 + disableDeleteRepository bool }