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>
This commit is contained in:
@@ -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) {
|
||||
|
||||
@@ -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")
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
@@ -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)
|
||||
})
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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")
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
})
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user