mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-26 14:30:13 +00:00
fix(server): cap the MCP request body at 7 MiB, answering 413 (BUG-3534) (#1988)
* fix(server): cap the MCP request body at 7 MiB, answering 413 (BUG-3534) The MCP transport (mcp-go's StreamableHTTPServer) reads a POST body whole into memory and has no cap of its own, so any caller, before auth, could make the server buffer any amount. Both MCP mounts now refuse a body over max(3 x the 2 MiB JSON cap, 6 x the artifact import cap) + 1 MiB with 413 and a JSON-RPC error, before auth and before anything reads the body. A declared length is checked without a read; a chunked body is read up to the limit and replayed. The bound is derived: every tools/call's payload lands in a downstream request already capped at 2 MiB of JSON (up to 3x once a client \u-escapes it) or in an artifact (up to 6x as control characters inside a JSON string), so no call that could otherwise succeed is refused. It follows PAD_IMPORT_ARTIFACT_MAX_BYTES. A sweep of every request-body reader (the body-reader census plus every reader outside internal/server) found mcp-go's the only uncapped one. * fix(server): cap the MCP body after auth, so an unauthenticated body is never read (BUG-3534, codex r1) * test(server): pin that a 413 is audited; narrow the unread claims to the limiter (BUG-3534, codex r2)
This commit is contained in:
xarmian
@@ -34,6 +34,8 @@ Both are also returned by `pad://_meta/version` and `pad_meta.action: version`.
|
||||
|
||||
**The wire contract is pinned as golden bytes** (TASK-2306): `cmd/pad/mcp_wire_golden_test.go` compares the `initialize` and `tools/list` results, on BOTH transports, against `cmd/pad/testdata/mcp_wire/*.json`. It uses the production bindings: the real `pad mcp serve` subcommand, and `registerRemoteMCP` behind `NewRemoteTransport`. Any catalog, annotation, schema or `instructions.md` edit moves those bytes. Regenerate with `PAD_UPDATE_MCP_GOLDEN=1 go test ./cmd/pad/ -run TestMCPWireGolden`, and review the testdata diff as the contract diff it is: that diff is where you decide whether the change owes a `ToolSurfaceVersion` bump. A refactor or SDK bump that is meant to be behavior-neutral must leave the files untouched.
|
||||
|
||||
**Request body limit (remote).** Both MCP mounts (`/mcp`, `/mcp/chatgpt`) refuse a POST body over `max(3 x 2 MiB, 6 x the artifact import cap) + 1 MiB` (7 MiB by default; it follows `PAD_IMPORT_ARTIFACT_MAX_BYTES`) with HTTP 413 and a JSON-RPC error (`id` null, code -32600, `data.code` `too_large`, `data.limit_bytes`), before the transport reads it (BUG-3534). The transport reads a body whole and has no cap of its own. The check runs after auth: an unauthenticated request is refused 401 without its body being read, as before. Before the check, the audit log reads at most 64 KiB of any authenticated body (it records the tool name). Past that, a declared `Content-Length` over the limit is refused without being read further, and a chunked body is read up to one byte past the limit, then refused or replayed. A 413 is audited as `error` / `client_error_413`. The bound is derived, not picked: every tools/call's payload lands in a downstream request already capped at 2 MiB of JSON (up to 3x larger once a client `\u`-escapes it) or in an artifact (up to 6x as control characters inside a JSON string), so no call that could otherwise succeed is refused. Stdio is unaffected.
|
||||
|
||||
**Where result caps live.** Two layers, deliberately different numbers. The MCP catalog action injects the agent-facing default and ceiling (list / backlinks / history: default 50, max 300) because a token budget is only knowable there. The HTTP endpoint's own clamp is a server-resource ceiling on what any caller may ASK for (`maxItemListQueryLimit` = 1000; `maxItemVersionsQueryLimit` = 500, lower because resolving a version can cost a patch application per row), and an ABSENT limit is left unbounded rather than defaulted — a server that truncates a request nobody bounded is a silent-truncation trap for direct API consumers. The CLI carries its own default for the same reason the catalog does.
|
||||
|
||||
**Dispatchers.** Two ship in `internal/mcp/`:
|
||||
|
||||
@@ -268,22 +268,23 @@ func TestEveryRequestBodyReaderIsAccountedFor(t *testing.T) {
|
||||
t.Parallel()
|
||||
// WHY each file reads a body. Every site below must name a file here.
|
||||
bodyReaderFileWhy := map[string]string{
|
||||
"middleware_request_text.go": "the chokepoint itself: readBodyForDecode reads the body under the caller's cap so bodyDecodesNUL can scan it",
|
||||
"handlers_import_bundle.go": "tar.gz bundle import — streams the body through gzip, and its pad-export.json is checked with bodyDecodesNUL before ImportWorkspace",
|
||||
"handlers_attachments.go": "multipart upload — the body is binary blob content, not text, and must NOT be scanned for text validity",
|
||||
"app_api_attachments.go": "app upload (TASK-3401 U6c) — a raw binary body with a required Content-Length, handed to appstore.StageUpload, which bounds it with a LimitReader at the declared size plus one; blob content, never JSON",
|
||||
"artifact_import.go": "raw artifact TEXT (not JSON) — checked with bindableText, the same predicate ValidatePath and ValidateQuery apply",
|
||||
"handlers_cloud.go": "bodyHasCloudSecret PEEKS at the body and restores the first 64 KiB of it — a larger body loses its tail, a bound that file documents and accepts; the real decode still happens through decodeJSON downstream",
|
||||
"middleware_mcp_audit.go": "audit capture — parses the body ITSELF and binds the decoded method / params.name to mcp_audit_log.tool_name, so it is a second READER, not a pass-through. That the MCP dispatcher decodes the body again is true and says nothing about what this middleware persists — the earlier rationale here made exactly that mistake and certified it safe (codex round 20). parseMCPRequestBody now runs both caller-derived returns through sanitiseStoredText",
|
||||
"middleware_mcp_scope.go": "the /mcp insufficient_scope pre-check (TASK-2308) PEEKS at up to 1 MiB of a POST body, decodes only method, params.name and the exact arguments.action, and restores every byte for mcp-go. The decoded values are compared against the catalog's action table and NOTHING is stored or echoed, so the NUL rule and the size cap have nothing to protect here; a body past the bound is passed on unread",
|
||||
"handlers_tokens.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — so the body is read by the chokepoint, which applies the cap and the NUL rule. The earlier reason here said it never reads the body, which was simply false (codex round 29): a wrong reason in this list is the same defect as a missing entry, since both let a reader pass as reviewed",
|
||||
"app_tokens.go": "the install-client token rules (TASK-3394) read the same FORM-encoded OAuth body handleOAuthToken/handleOAuthIntrospect read, behind ValidateFormBody (BUG-2811), to classify the client exactly as fosite will",
|
||||
"app_delegated_consent.go": "the delegated app sign-in (TASK-3399) is the OAuth authorize/decide doors' form: appSignInAvailable parses it for client_id and decideAppConsent reads app_access, both on routes wrapped in ValidateFormBody (BUG-2811) like the rest of handlers_oauth.go; a form-encoded body, never JSON",
|
||||
"handlers_oauth.go": "the OAuth handlers read FORM-encoded bodies (r.Form/FormValue). Every POST route they serve is wrapped in ValidateFormBody (BUG-2811), which applies bindableText to the body before these reads. The transport rules cover the query half of r.Form; ValidateFormBody covers the body half",
|
||||
"middleware_form_body.go": "the form-body chokepoint itself: ValidateFormBody reads a form-encoded body up to net/http's own ParseForm cap, checks it with validQueryText, and hands the same bytes (or the same read error) back to the handler (BUG-2811)",
|
||||
"handlers_watches.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — the closing-round-4 fix for the chunked-body drop; the one reader expression is the nil check itself, and the body bytes flow through the chokepoint",
|
||||
"handlers_item_lease.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — same shape as handlers_watches.go; the one reader expression is the nil check, and the body (optional holder/ttl_seconds) flows through the chokepoint, so the caller-text holder gets BUG-2803's NUL refusal before it can reach items.lease_holder",
|
||||
"import_read_deadline.go": "a PASS-THROUGH, not a reader: it replaces r.Body with a wrapper whose Read arms the connection deadline and delegates, byte for byte, to the original body (BUG-3184). Nothing in the file inspects the bytes; the import route's two decoders (decodeJSONWithLimit, and the bundle path's bodyDecodesNUL on pad-export.json) read them afterwards, unchanged",
|
||||
"middleware_request_text.go": "the chokepoint itself: readBodyForDecode reads the body under the caller's cap so bodyDecodesNUL can scan it",
|
||||
"handlers_import_bundle.go": "tar.gz bundle import — streams the body through gzip, and its pad-export.json is checked with bodyDecodesNUL before ImportWorkspace",
|
||||
"handlers_attachments.go": "multipart upload — the body is binary blob content, not text, and must NOT be scanned for text validity",
|
||||
"app_api_attachments.go": "app upload (TASK-3401 U6c) — a raw binary body with a required Content-Length, handed to appstore.StageUpload, which bounds it with a LimitReader at the declared size plus one; blob content, never JSON",
|
||||
"artifact_import.go": "raw artifact TEXT (not JSON) — checked with bindableText, the same predicate ValidatePath and ValidateQuery apply",
|
||||
"handlers_cloud.go": "bodyHasCloudSecret PEEKS at the body and restores the first 64 KiB of it — a larger body loses its tail, a bound that file documents and accepts; the real decode still happens through decodeJSON downstream",
|
||||
"middleware_mcp_audit.go": "audit capture — parses the body ITSELF and binds the decoded method / params.name to mcp_audit_log.tool_name, so it is a second READER, not a pass-through. That the MCP dispatcher decodes the body again is true and says nothing about what this middleware persists — the earlier rationale here made exactly that mistake and certified it safe (codex round 20). parseMCPRequestBody now runs both caller-derived returns through sanitiseStoredText",
|
||||
"middleware_mcp_body_limit.go": "the MCP mounts' body cap (BUG-3534). It decodes nothing and stores nothing: a declared length is compared against the limit without a read, and a chunked body is read up to limit+1 bytes only to measure it, then replayed byte for byte to the transport (or refused 413). It exists because the transport (mcp-go) reads the whole body itself and has no cap, which this census cannot see because that read is in a dependency",
|
||||
"middleware_mcp_scope.go": "the /mcp insufficient_scope pre-check (TASK-2308) PEEKS at up to 1 MiB of a POST body, decodes only method, params.name and the exact arguments.action, and restores every byte for mcp-go. The decoded values are compared against the catalog's action table and NOTHING is stored or echoed, so the NUL rule and the size cap have nothing to protect here; a body past the bound is passed on unread",
|
||||
"handlers_tokens.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — so the body is read by the chokepoint, which applies the cap and the NUL rule. The earlier reason here said it never reads the body, which was simply false (codex round 29): a wrong reason in this list is the same defect as a missing entry, since both let a reader pass as reviewed",
|
||||
"app_tokens.go": "the install-client token rules (TASK-3394) read the same FORM-encoded OAuth body handleOAuthToken/handleOAuthIntrospect read, behind ValidateFormBody (BUG-2811), to classify the client exactly as fosite will",
|
||||
"app_delegated_consent.go": "the delegated app sign-in (TASK-3399) is the OAuth authorize/decide doors' form: appSignInAvailable parses it for client_id and decideAppConsent reads app_access, both on routes wrapped in ValidateFormBody (BUG-2811) like the rest of handlers_oauth.go; a form-encoded body, never JSON",
|
||||
"handlers_oauth.go": "the OAuth handlers read FORM-encoded bodies (r.Form/FormValue). Every POST route they serve is wrapped in ValidateFormBody (BUG-2811), which applies bindableText to the body before these reads. The transport rules cover the query half of r.Form; ValidateFormBody covers the body half",
|
||||
"middleware_form_body.go": "the form-body chokepoint itself: ValidateFormBody reads a form-encoded body up to net/http's own ParseForm cap, checks it with validQueryText, and hands the same bytes (or the same read error) back to the handler (BUG-2811)",
|
||||
"handlers_watches.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — the closing-round-4 fix for the chunked-body drop; the one reader expression is the nil check itself, and the body bytes flow through the chokepoint",
|
||||
"handlers_item_lease.go": "guards on r.Body != nil && r.ContentLength != 0, then decodes THROUGH decodeJSON — same shape as handlers_watches.go; the one reader expression is the nil check, and the body (optional holder/ttl_seconds) flows through the chokepoint, so the caller-text holder gets BUG-2803's NUL refusal before it can reach items.lease_holder",
|
||||
"import_read_deadline.go": "a PASS-THROUGH, not a reader: it replaces r.Body with a wrapper whose Read arms the connection deadline and delegates, byte for byte, to the original body (BUG-3184). Nothing in the file inspects the bytes; the import route's two decoders (decodeJSONWithLimit, and the bundle path's bodyDecodesNUL on pad-export.json) read them afterwards, unchanged",
|
||||
}
|
||||
|
||||
// Every call site, counted. MEASURED from the census, not written from
|
||||
@@ -328,6 +329,7 @@ func TestEveryRequestBodyReaderIsAccountedFor(t *testing.T) {
|
||||
"middleware_mcp_audit.go::Server.MCPAuditLog::Body": 5,
|
||||
"middleware_mcp_audit.go::Server.emitMCPAuditDenied::Body": 2,
|
||||
"middleware_mcp_scope.go::Server.MCPInsufficientScope::Body": 5,
|
||||
"middleware_mcp_body_limit.go::Server.limitMCPBody::Body": 7,
|
||||
"middleware_request_text.go::readBodyForDecode::Body": 4,
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,201 @@
|
||||
package server
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"encoding/json"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// BUG-3534: the MCP mounts refuse an authenticated POST body over
|
||||
// mcpBodyLimit with 413 before the transport reads it, on both the
|
||||
// declared-length and the chunked path; an unauthenticated body is never
|
||||
// read. Driven through the router so the middleware's place in each
|
||||
// mount's chain is what is tested.
|
||||
|
||||
func postMCPSized(srv *Server, path, token string, n int64, chunked bool) *httptest.ResponseRecorder {
|
||||
rr, _ := postMCPSizedCounting(srv, path, token, n, chunked)
|
||||
return rr
|
||||
}
|
||||
|
||||
// postMCPSizedCounting also reports how many body bytes anything read.
|
||||
func postMCPSizedCounting(srv *Server, path, token string, n int64, chunked bool) (*httptest.ResponseRecorder, int64) {
|
||||
body := &mcpBodyCountingReader{r: io.LimitReader(repeatReader('x'), n)}
|
||||
req := httptest.NewRequest("POST", path, body)
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
if token != "" {
|
||||
req.Header.Set("Authorization", "Bearer "+token)
|
||||
}
|
||||
if chunked {
|
||||
req.ContentLength = -1
|
||||
} else {
|
||||
req.ContentLength = n
|
||||
}
|
||||
req.RemoteAddr = "192.0.2.1:1234"
|
||||
rr := httptest.NewRecorder()
|
||||
srv.ServeHTTP(rr, req)
|
||||
return rr, body.n
|
||||
}
|
||||
|
||||
type mcpBodyCountingReader struct {
|
||||
r io.Reader
|
||||
n int64
|
||||
}
|
||||
|
||||
func (c *mcpBodyCountingReader) Read(p []byte) (int, error) {
|
||||
k, err := c.r.Read(p)
|
||||
c.n += int64(k)
|
||||
return k, err
|
||||
}
|
||||
|
||||
// repeatReader is an endless stream of one byte, so a large body costs no
|
||||
// memory to build.
|
||||
type repeatReader byte
|
||||
|
||||
func (b repeatReader) Read(p []byte) (int, error) {
|
||||
for i := range p {
|
||||
p[i] = byte(b)
|
||||
}
|
||||
return len(p), nil
|
||||
}
|
||||
|
||||
func isBodyTooLarge(t *testing.T, rr *httptest.ResponseRecorder, limit int64) bool {
|
||||
t.Helper()
|
||||
if rr.Code != http.StatusRequestEntityTooLarge {
|
||||
return false
|
||||
}
|
||||
var env struct {
|
||||
JSONRPC string `json:"jsonrpc"`
|
||||
ID *json.RawMessage `json:"id"`
|
||||
Error struct {
|
||||
Code int `json:"code"`
|
||||
Data struct {
|
||||
Code string `json:"code"`
|
||||
LimitBytes int64 `json:"limit_bytes"`
|
||||
} `json:"data"`
|
||||
} `json:"error"`
|
||||
}
|
||||
if err := json.Unmarshal(rr.Body.Bytes(), &env); err != nil {
|
||||
t.Errorf("413 body is not JSON: %q", rr.Body.String())
|
||||
return false
|
||||
}
|
||||
if env.JSONRPC != "2.0" || env.Error.Code != -32600 || env.Error.Data.Code != "too_large" || env.Error.Data.LimitBytes != limit {
|
||||
t.Errorf("413 body = %s", rr.Body.String())
|
||||
}
|
||||
return true
|
||||
}
|
||||
|
||||
func TestBUG3534_DefaultLimitIsSevenMiB(t *testing.T) {
|
||||
srv := testServer(t)
|
||||
if got, want := srv.mcpBodyLimit(), int64(7<<20); got != want {
|
||||
t.Fatalf("mcpBodyLimit = %d, want %d", got, want)
|
||||
}
|
||||
}
|
||||
|
||||
// At the limit the request reaches the mount's transport; one byte over,
|
||||
// it is refused 413 before the transport. Both mounts, both length paths,
|
||||
// each with a token for its own mount.
|
||||
func TestBUG3534_LimitOnBothMountsAndBothLengthPaths(t *testing.T) {
|
||||
srv := chatGPTMountServer(t, true)
|
||||
sess := newOAuthSession(t, srv)
|
||||
mcpTok, _ := mintWithResource(t, srv, sess, testCanonicalAudience)
|
||||
gptTok, _ := mintWithResource(t, srv, sess, testChatGPTResource)
|
||||
limit := srv.mcpBodyLimit()
|
||||
for _, m := range []struct{ path, token, stub string }{
|
||||
{"/mcp", mcpTok, "mcp"},
|
||||
{"/mcp/chatgpt", gptTok, "chatgpt"},
|
||||
} {
|
||||
for _, chunked := range []bool{false, true} {
|
||||
name := m.path + map[bool]string{false: " content-length", true: " chunked"}[chunked]
|
||||
if rr := postMCPSized(srv, m.path, m.token, limit, chunked); rr.Code != http.StatusOK || rr.Body.String() != m.stub {
|
||||
t.Errorf("%s at the limit: %d %q, want the transport's 200", name, rr.Code, rr.Body.String())
|
||||
}
|
||||
if rr := postMCPSized(srv, m.path, m.token, limit+1, chunked); !isBodyTooLarge(t, rr, limit) {
|
||||
t.Errorf("%s one byte over: %d %q, want 413", name, rr.Code, rr.Body.String())
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Nothing reads an unauthenticated body (codex review): an oversized one,
|
||||
// with or without a declared length, is refused 401 by auth, which the
|
||||
// pre-auth rate limit meters, with zero body bytes read.
|
||||
func TestBUG3534_UnauthenticatedBodyIsNeverRead(t *testing.T) {
|
||||
srv := chatGPTMountServer(t, true)
|
||||
limit := srv.mcpBodyLimit()
|
||||
for _, path := range []string{"/mcp", "/mcp/chatgpt"} {
|
||||
for _, chunked := range []bool{false, true} {
|
||||
rr, read := postMCPSizedCounting(srv, path, "", limit+1, chunked)
|
||||
if rr.Code != http.StatusUnauthorized || read != 0 {
|
||||
t.Errorf("%s chunked=%v without a token: %d after reading %d body bytes, want 401 and 0", path, chunked, rr.Code, read)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Within the limit, the transport receives the body byte for byte, on the
|
||||
// chunked path too, which is the one this middleware reads and replays.
|
||||
func TestBUG3534_BodyWithinTheLimitArrivesIntact(t *testing.T) {
|
||||
srv, rt := scopeTestServer(t)
|
||||
tok, code := mintWithResourceTier(t, srv, newOAuthSession(t, srv), testCanonicalAudience, "pad:read pad:write", "write")
|
||||
if code != http.StatusOK {
|
||||
t.Fatalf("mint: %d", code)
|
||||
}
|
||||
body := `{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"pad_item","arguments":{"action":"create","content":"` +
|
||||
strings.Repeat("y", 3<<20) + `"}}}`
|
||||
for _, chunked := range []bool{false, true} {
|
||||
req := httptest.NewRequest("POST", "/mcp", bytes.NewReader([]byte(body)))
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
req.Header.Set("Authorization", "Bearer "+tok)
|
||||
if chunked {
|
||||
req.ContentLength = -1
|
||||
}
|
||||
req.RemoteAddr = "192.0.2.1:1234"
|
||||
rr := httptest.NewRecorder()
|
||||
srv.ServeHTTP(rr, req)
|
||||
if rr.Code != http.StatusOK || rr.Body.String() != "mcp" {
|
||||
t.Fatalf("chunked=%v: %d %q, want 200 from the transport", chunked, rr.Code, rr.Body.String())
|
||||
}
|
||||
if got := rt.take(); len(got) != 1 || got[0] != body {
|
||||
t.Errorf("chunked=%v: the transport did not receive the body intact", chunked)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// The limit follows the operator's artifact cap, so raising that cap never
|
||||
// makes remote import refuse a legal artifact at this door first.
|
||||
func TestBUG3534_LimitFollowsTheArtifactCap(t *testing.T) {
|
||||
srv := chatGPTMountServer(t, true)
|
||||
srv.SetImportArtifactMaxBytes(4 << 20)
|
||||
tok, _ := mintWithResource(t, srv, newOAuthSession(t, srv), testCanonicalAudience)
|
||||
limit := srv.mcpBodyLimit()
|
||||
if want := int64(6*(4<<20) + 1<<20); limit != want {
|
||||
t.Fatalf("mcpBodyLimit with a 4 MiB artifact cap = %d, want %d", limit, want)
|
||||
}
|
||||
if rr := postMCPSized(srv, "/mcp", tok, 10<<20, false); rr.Code != http.StatusOK {
|
||||
t.Errorf("10 MiB under a 25 MiB limit: %d, want the transport's 200", rr.Code)
|
||||
}
|
||||
if rr := postMCPSized(srv, "/mcp", tok, limit+1, false); !isBodyTooLarge(t, rr, limit) {
|
||||
t.Errorf("one byte over the raised limit: %d", rr.Code)
|
||||
}
|
||||
}
|
||||
|
||||
// The 413 is audited, as the response of an authenticated call
|
||||
// (codex round 2): one row, classified error / client_error_413 by
|
||||
// classifyMCPResult like any other 4xx that is not an auth or rate gate.
|
||||
func TestBUG3534_TooLargeIsAudited(t *testing.T) {
|
||||
srv, user, bearer := auditedMCPServer(t)
|
||||
limit := srv.mcpBodyLimit()
|
||||
if rr := postMCPSized(srv, "/mcp", bearer, limit+1, false); !isBodyTooLarge(t, rr, limit) {
|
||||
t.Fatalf("over the limit: %d", rr.Code)
|
||||
}
|
||||
rows := waitForAuditRows(t, srv, user.ID, 1)
|
||||
if len(rows) != 1 || rows[0].ResultStatus != models.MCPAuditResultError || rows[0].ErrorKind == nil || *rows[0].ErrorKind != "client_error_413" {
|
||||
t.Fatalf("audit rows = %+v, want one error / client_error_413", rows)
|
||||
}
|
||||
}
|
||||
@@ -75,8 +75,10 @@ func (s *Server) registerChatGPTMCPRoutes(r chi.Router) {
|
||||
}
|
||||
// The resource is fixed when the route is built: the mount's audience
|
||||
// is configuration, never a request's.
|
||||
// limitMCPBody: the same transport, the same uncapped read (BUG-3534),
|
||||
// placed as on /mcp: after auth and the audit log.
|
||||
r.With(s.requireChatGPTAvailable, s.requireConfiguredHost, WithMCPResource(s.chatGPTResource),
|
||||
s.refusePATs, s.MCPBearerAuth, s.MCPAuditLog).Mount("/mcp/chatgpt", transport)
|
||||
s.refusePATs, s.MCPBearerAuth, s.MCPAuditLog, s.limitMCPBody).Mount("/mcp/chatgpt", transport)
|
||||
// Path-aware RFC 9728 document for this resource (the /mcp ones are
|
||||
// unchanged).
|
||||
r.With(s.requireChatGPTAvailable, s.requireConfiguredHost).
|
||||
|
||||
@@ -177,7 +177,12 @@ func (s *Server) registerMCPRoutes(r chi.Router) {
|
||||
//
|
||||
// MCPInsufficientScope (TASK-2308) sits after MCPAuditLog so its 403
|
||||
// is audited as denied.
|
||||
r.With(s.requireMCPAvailable, s.requireConfiguredHost, s.MCPBearerAuth, s.MCPAuditLog, s.MCPInsufficientScope).Mount("/mcp", transport)
|
||||
//
|
||||
// limitMCPBody (BUG-3534) caps the body the transport would read whole.
|
||||
// It sits AFTER auth, so an unauthenticated body is never read (it is
|
||||
// refused 401 first, as before), and after MCPAuditLog, so a 413 is
|
||||
// audited.
|
||||
r.With(s.requireMCPAvailable, s.requireConfiguredHost, s.MCPBearerAuth, s.MCPAuditLog, s.limitMCPBody, s.MCPInsufficientScope).Mount("/mcp", transport)
|
||||
|
||||
// Discovery endpoints — unauthenticated, oauthAvailable-gated. RFC 9728
|
||||
// (protected-resource) and RFC 8414 (auth-server) metadata.
|
||||
|
||||
@@ -0,0 +1,109 @@
|
||||
package server
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
)
|
||||
|
||||
// mcpBodyEnvelopeSlack is the room mcpBodyLimit leaves for the JSON-RPC
|
||||
// envelope and the tool's other arguments around the payload it sizes.
|
||||
const mcpBodyEnvelopeSlack int64 = 1 << 20
|
||||
|
||||
// mcpBodyLimit is the largest POST body the MCP mounts accept (BUG-3534).
|
||||
// The MCP transport (mcp-go's StreamableHTTPServer) reads the whole body
|
||||
// into memory and has no cap of its own, so without this an authenticated
|
||||
// or unauthenticated caller could make the server buffer any amount.
|
||||
//
|
||||
// The limit is derived from what a legitimate tools/call carries. Its
|
||||
// payload lands in requests the server already bounds, so the bound here
|
||||
// covers the largest of those after JSON escaping:
|
||||
//
|
||||
// - a JSON body, capped at defaultJSONBodyLimit downstream. A client may
|
||||
// \u-escape what Go writes raw, and the worst growth is 3x (a 2-byte
|
||||
// UTF-8 character is a 6-byte \uXXXX; a 4-byte one a 12-byte
|
||||
// surrogate pair);
|
||||
// - an artifact, capped at the artifact import limit and carried inside
|
||||
// a JSON string. bindableText refuses only NUL and invalid UTF-8, so a
|
||||
// control character's 6-byte \u00XX makes it up to 6x.
|
||||
//
|
||||
// Plus mcpBodyEnvelopeSlack. With the defaults that is 7 MiB, about 46
|
||||
// times the largest item body on the dogfood instance when this was
|
||||
// written (158,072 bytes over 10,508 items; BUG-3534's trail). It follows
|
||||
// PAD_IMPORT_ARTIFACT_MAX_BYTES, so raising that cap never makes remote
|
||||
// import refuse a legal artifact here first.
|
||||
func (s *Server) mcpBodyLimit() int64 {
|
||||
artifactCap := s.importArtifactMaxBytes
|
||||
if artifactCap <= 0 {
|
||||
artifactCap = defaultImportArtifactMaxBytes
|
||||
}
|
||||
return max(3*int64(defaultJSONBodyLimit), 6*artifactCap) + mcpBodyEnvelopeSlack
|
||||
}
|
||||
|
||||
// limitMCPBody refuses an MCP POST body larger than mcpBodyLimit with 413
|
||||
// and a JSON-RPC error, before the transport reads it (BUG-3534). Mounted
|
||||
// on both MCP mounts AFTER auth and the audit log. After auth because
|
||||
// nothing reads an unauthenticated body: it is refused 401 first, and
|
||||
// that 401 is what the pre-auth rate limit meters. Reading a chunked body
|
||||
// here, before auth, would let an anonymous caller make the server take
|
||||
// in up to the limit per request ahead of both (codex review). After the
|
||||
// audit log so a 413 is recorded.
|
||||
//
|
||||
// - A declared Content-Length over the limit is refused without this
|
||||
// middleware reading it. MCPAuditLog, which runs first, has already
|
||||
// peeked at most mcpAuditBodyMaxBytes (64 KiB) of an authenticated body
|
||||
// and replays it.
|
||||
// - A declared Content-Length within it passes untouched: net/http ends
|
||||
// the body at the declared length, so nothing can read more.
|
||||
// - No declared length (chunked): up to limit+1 bytes are read here. Over
|
||||
// the limit is refused; within it, the bytes are replayed.
|
||||
func (s *Server) limitMCPBody(next http.Handler) http.Handler {
|
||||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.Method != http.MethodPost || r.Body == nil || r.Body == http.NoBody {
|
||||
next.ServeHTTP(w, r)
|
||||
return
|
||||
}
|
||||
limit := s.mcpBodyLimit()
|
||||
if r.ContentLength > limit {
|
||||
writeMCPBodyTooLarge(w, limit)
|
||||
return
|
||||
}
|
||||
if r.ContentLength >= 0 {
|
||||
next.ServeHTTP(w, r)
|
||||
return
|
||||
}
|
||||
buf, err := io.ReadAll(io.LimitReader(r.Body, limit+1))
|
||||
if err != nil {
|
||||
// The transport answers a body it cannot read with its own parse
|
||||
// error, so hand it the same failure rather than a new one.
|
||||
r.Body = replayBody{io.MultiReader(bytes.NewReader(buf), errReader{err}), r.Body}
|
||||
next.ServeHTTP(w, r)
|
||||
return
|
||||
}
|
||||
if int64(len(buf)) > limit {
|
||||
writeMCPBodyTooLarge(w, limit)
|
||||
return
|
||||
}
|
||||
r.Body = replayBody{bytes.NewReader(buf), r.Body}
|
||||
next.ServeHTTP(w, r)
|
||||
})
|
||||
}
|
||||
|
||||
// writeMCPBodyTooLarge writes limitMCPBody's 413: a JSON-RPC error with a
|
||||
// null id, since the request was never parsed.
|
||||
func writeMCPBodyTooLarge(w http.ResponseWriter, limit int64) {
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.Header().Set("Connection", "close")
|
||||
w.WriteHeader(http.StatusRequestEntityTooLarge)
|
||||
_ = json.NewEncoder(w).Encode(map[string]any{
|
||||
"jsonrpc": "2.0",
|
||||
"id": nil,
|
||||
"error": map[string]any{
|
||||
"code": -32600,
|
||||
"message": fmt.Sprintf("request body too large: the limit is %d bytes", limit),
|
||||
"data": map[string]any{"code": "too_large", "limit_bytes": limit},
|
||||
},
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user