diff --git a/pkg/http/handler.go b/pkg/http/handler.go index 935bca1c0e..e4a9d198ec 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -132,6 +132,8 @@ func NewHTTPMcpHandler( func (h *Handler) RegisterMiddleware(r chi.Router) { r.Use( + // Must run first: bounds the body before anything downstream reads it. + middleware.WithMaxBodySize(h.maxRequestBodyBytes()), middleware.ExtractUserToken(h.oauthCfg), middleware.WithRequestConfig, middleware.WithMCPParse(), @@ -143,6 +145,15 @@ func (h *Handler) RegisterMiddleware(r chi.Router) { } } +// maxRequestBodyBytes returns the effective request-body size limit, applied +// both by the early middleware and by the MCP SDK handler. +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) { @@ -239,6 +250,9 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return ghServer }, &mcp.StreamableHTTPOptions{ Stateless: true, + // Required, not just belt-and-braces: the effective limit exceeds the + // SDK's own default, which would otherwise cap it. + MaxRequestBodyBytes: h.maxRequestBodyBytes(), }) mcpHandler.ServeHTTP(w, r) diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index 13e87fc034..f051084785 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -3,6 +3,7 @@ package http import ( "context" "encoding/json" + "fmt" "log/slog" "net/http" "net/http/httptest" @@ -14,6 +15,7 @@ import ( ghcontext "github.com/github/github-mcp-server/pkg/context" "github.com/github/github-mcp-server/pkg/github" "github.com/github/github-mcp-server/pkg/http/headers" + "github.com/github/github-mcp-server/pkg/http/middleware" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/scopes" "github.com/github/github-mcp-server/pkg/translations" @@ -1287,3 +1289,136 @@ 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") } + +// TestMaxRequestBodyBytes checks the effective limit and, critically, that it +// sits above the MCP SDK default — which is why it must also be passed to +// StreamableHTTPOptions rather than left to the SDK. +func TestMaxRequestBodyBytes(t *testing.T) { + t.Run("default leaves headroom above the MCP SDK limit", func(t *testing.T) { + h := &Handler{config: &ServerConfig{}} + + assert.Equal(t, int64(5<<20), h.maxRequestBodyBytes()) + assert.Greater(t, h.maxRequestBodyBytes(), int64(mcp.DefaultMaxRequestBodyBytes), + "the default intentionally exceeds the SDK limit, so the SDK must be told about it") + }) + + t.Run("configured value overrides the default", func(t *testing.T) { + h := &Handler{config: &ServerConfig{MaxRequestBodyBytes: 1234}} + assert.Equal(t, int64(1234), h.maxRequestBodyBytes()) + }) +} + +// TestMaxRequestBodySizeEnforcement exercises both layers the limit is applied +// at: the early middleware, and the MCP SDK handler the request is delegated to. +func TestMaxRequestBodySizeEnforcement(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, maxBytes int64, mcpServerFactoryCalled *bool) *Handler { + t.Helper() + return NewHTTPMcpHandler( + context.Background(), + &ServerConfig{Version: "test", MaxRequestBodyBytes: maxBytes}, + 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{}), + ) + } + + newRouter := func(h *Handler) http.Handler { + r := chi.NewRouter() + h.RegisterMiddleware(r) + h.RegisterRoutes(r) + return r + } + + newRequest := func(body string) *http.Request { + 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"}, "_")) + return req + } + + t.Run("middleware rejects an oversized request before the MCP server is built", func(t *testing.T) { + var mcpServerFactoryCalled bool + r := newRouter(newHandler(t, limit, &mcpServerFactoryCalled)) + + body := buildBody(limit + 1) + require.Greater(t, len(body), limit) + + rr := httptest.NewRecorder() + r.ServeHTTP(rr, newRequest(body)) + + 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("request at the configured limit succeeds", func(t *testing.T) { + var mcpServerFactoryCalled bool + r := newRouter(newHandler(t, limit, &mcpServerFactoryCalled)) + + body := buildBody(limit) + require.Len(t, body, limit) + + rr := httptest.NewRecorder() + r.ServeHTTP(rr, newRequest(body)) + + 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") + }) + + t.Run("SDK handler enforces the configured limit when the middleware is bypassed", func(t *testing.T) { + h := newHandler(t, limit, nil) + + body := buildBody(limit + 1) + require.Greater(t, len(body), limit) + + rr := httptest.NewRecorder() + h.ServeHTTP(rr, newRequest(body)) + + assert.Equal(t, http.StatusRequestEntityTooLarge, rr.Code) + assert.Contains(t, rr.Body.String(), fmt.Sprintf("request body exceeds %d bytes", limit), + "the SDK should report the configured limit, not its own default") + }) + + // The default headroom only exists if it reaches the SDK as well; leaving + // the SDK on its own default would silently reject this request. + t.Run("unconfigured handler accepts a request above the MCP SDK limit", func(t *testing.T) { + var mcpServerFactoryCalled bool + r := newRouter(newHandler(t, 0, &mcpServerFactoryCalled)) + + body := buildBody(mcp.DefaultMaxRequestBodyBytes + 1024) + require.Greater(t, int64(len(body)), int64(mcp.DefaultMaxRequestBodyBytes)) + require.Less(t, int64(len(body)), middleware.DefaultMaxRequestBodyBytes) + + rr := httptest.NewRecorder() + r.ServeHTTP(rr, newRequest(body)) + + 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 0000000000..59fc8c0bc7 --- /dev/null +++ b/pkg/http/middleware/body_limit.go @@ -0,0 +1,46 @@ +package middleware + +import ( + "errors" + "net/http" +) + +// DefaultMaxRequestBodyBytes bounds the total HTTP request, not just the tool +// payload within it. It sits modestly above the MCP SDK's own default to leave +// room for JSON-RPC and tool-call envelope overhead; because it is the larger +// of the two, callers must also pass it to the SDK or the SDK would cap it. +const DefaultMaxRequestBodyBytes int64 = 5 << 20 // 5 MiB + +// WithMaxBodySize bounds the size of the request body. It must be registered +// before any middleware that reads or buffers the body (WithMCPParse, +// WithScopeChallenge), so an oversized payload is rejected before it is +// buffered in memory rather than by a later guard in the MCP SDK. +// +// A body of unknown length (chunked, HTTP/2) cannot be rejected upfront, so +// the limit is instead enforced on read and surfaces as a *http.MaxBytesError +// to whichever middleware reads the body first. +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) + }) + } +} + +func writeRequestTooLarge(w http.ResponseWriter) { + http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) +} + +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 0000000000..c82e4e2724 --- /dev/null +++ b/pkg/http/middleware/body_limit_test.go @@ -0,0 +1,114 @@ +package middleware + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// unknownLengthBody hides Len from httptest.NewRequest so ContentLength is -1, +// as it is for a chunked or HTTP/2 request. +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 c82616b270..21848fdeb4 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 5a28a30c3b..0965c6db61 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 mirrors the production middleware ordering, +// where WithMaxBodySize runs ahead of WithMCPParse. +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 := 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)) + + // An unknown length skips WithMaxBodySize's Content-Length fast path, + // so the overflow surfaces from WithMCPParse's own read. + 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.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 1a86bf93ce..4ac3bab906 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 0000000000..9ce67bc7cd --- /dev/null +++ b/pkg/http/middleware/scope_challenge_test.go @@ -0,0 +1,84 @@ +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 covers the fallback body-parsing path, +// used when WithMCPParse has not already populated MCPMethodInfo in context. +func TestWithScopeChallenge_MaxBodySize(t *testing.T) { + const limit = 64 + oauthCfg := &oauth.Config{} + fetcher := &mockScopeFetcher{scopes: []string{"repo"}} + + newRequestWithBody := func(body io.Reader) *http.Request { + req := httptest.NewRequest(http.MethodPost, "/mcp", body) + ctx := ghcontext.WithTokenInfo(req.Context(), &ghcontext.TokenInfo{ + Token: "******", + TokenType: utils.TokenTypeOAuthAccessToken, + }) + return req.WithContext(ctx) + } + + newRequest := func(body string) *http.Request { + return newRequestWithBody(strings.NewReader(body)) + } + + 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)) + + // An unknown length skips WithMaxBodySize's Content-Length fast path, + // so the overflow surfaces from the fallback read. + req := newRequestWithBody(unknownLengthBody(body)) + require.Equal(t, int64(-1), req.ContentLength, "test setup: Content-Length should be unknown") + + 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 6bf48a07a7..0cac24af30 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -108,6 +108,10 @@ 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. When zero, middleware.DefaultMaxRequestBodyBytes is used. + MaxRequestBodyBytes int64 + disableDeleteRepository bool }