From 0bda14331d1150ef56c032c3324912cb66f0aecc Mon Sep 17 00:00:00 2001 From: Yatsuiii Date: Tue, 25 Aug 2026 10:47:23 +0530 Subject: [PATCH 1/2] fix: enforce a per-string length cap on tool arguments (#562) docs/spec/proxy-security.md's Fuzzing Definition of Done specs MAX_STRING_LENGTH at 1MB per string field. It was not implemented anywhere in src/ or scripts/. A single oversized string sits inside an otherwise shallow, low-key-count payload and passes the depth and key-count caps from #556/#561 unbounded, up to the whole-body byte cap. Extends the existing _arg_shape_violation walk in both files with a UTF-8 byte length check, covering string values and object keys. Keys are checked because the key-count cap bounds how many there are, not how large each one is, so one huge key would otherwise pass everything. The cap is not the spec's literal 1MB. That equals MAX_REQUEST_BYTES in both files today, so a 1MB string plus any surrounding JSON already exceeds the whole-body cap and the check could never fire before DOS-001's size rejection already had. It would have been dead code. Set to half the whole-body cap instead, derived from a named constant rather than a restated literal so raising the default carries this along with it. Verified reachable: an over-cap string is 500,101 bytes on the wire against a 1,000,000 byte body cap. The spec's 10MB MAX_REQUEST_BYTES versus the 1MB implemented in both files is left alone. Picking a number there is a maintainer call, not one to make inside this change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KDXJ4ghkW6v56W8St2w5kg --- scripts/mock_upstream.py | 56 ++++++++++-- src/cmcp_runtime/mcp/server.py | 70 ++++++++++++--- tests/unit/test_mcp_server_auth.py | 93 ++++++++++++++++++- tests/unit/test_mock_upstream_gate.py | 123 ++++++++++++++++++++++++-- 4 files changed, 316 insertions(+), 26 deletions(-) diff --git a/scripts/mock_upstream.py b/scripts/mock_upstream.py index ec8b667..8496ee4 100644 --- a/scripts/mock_upstream.py +++ b/scripts/mock_upstream.py @@ -43,6 +43,26 @@ _MAX_ARG_DEPTH = 20 _MAX_ARG_KEYS = 256 +# #562: docs/spec/proxy-security.md's Fuzzing Definition of Done specs +# MAX_STRING_LENGTH at 1MB per string field, separate from the depth/key +# caps above. A single oversized string can sit inside an otherwise +# shallow, low-key-count payload and slip past both of those unbounded, +# up to whatever the whole-body byte cap happens to be. +# +# Not set to the spec's literal 1MB: that would equal MAX_REQUEST_BYTES +# itself, and a 1MB string plus any JSON structure around it already +# exceeds the whole-body cap, so the check could never fire before +# DOS-001's size rejection already had. Set to half of MAX_REQUEST_BYTES +# instead, so a single string cannot consume the whole request budget and +# the cap is actually reachable. Worth revisiting to the spec's literal +# value if MAX_REQUEST_BYTES itself is ever raised toward the spec's +# stated 10MB (see #562). +# +# Checked as UTF-8 byte length, not character count, since a codepoint +# count understates the actual memory and processing cost of multi-byte +# text. +_MAX_ARG_STRING_LENGTH = MAX_REQUEST_BYTES // 2 + logger = logging.getLogger("mock_upstream") @@ -56,23 +76,41 @@ def _valid_rpc_id(value: Any) -> bool: return value is None or (isinstance(value, (str, int, float)) and not isinstance(value, bool)) +def _over_string_cap(text: str) -> bool: + return len(text.encode("utf-8")) > _MAX_ARG_STRING_LENGTH + + +def _object_shape_violation(value: dict[str, Any], depth: int) -> str | None: + if len(value) > _MAX_ARG_KEYS: + return f"object has more than {_MAX_ARG_KEYS} keys" + for key, child in value.items(): + # Keys carry the same cost as values and are not covered by the key + # *count* cap above, so a single huge key would otherwise slip + # through every check here. + if isinstance(key, str) and _over_string_cap(key): + return f"object key over the length cap of {_MAX_ARG_STRING_LENGTH} bytes" + violation = _arg_shape_violation(child, depth=depth + 1) + if violation is not None: + return violation + return None + + def _arg_shape_violation(value: Any, *, depth: int = 0) -> str | None: - """Return a message describing the first depth/key-count violation found - under `value`, or None if it fits within `_MAX_ARG_DEPTH` / `_MAX_ARG_KEYS`.""" + """Return a message describing the first depth/key-count/string-length + violation found under `value`, or None if it fits within `_MAX_ARG_DEPTH` / + `_MAX_ARG_KEYS` / `_MAX_ARG_STRING_LENGTH`.""" if depth > _MAX_ARG_DEPTH: return f"arguments nested past the depth cap of {_MAX_ARG_DEPTH}" if isinstance(value, dict): - if len(value) > _MAX_ARG_KEYS: - return f"object has more than {_MAX_ARG_KEYS} keys" - for child in value.values(): - violation = _arg_shape_violation(child, depth=depth + 1) - if violation is not None: - return violation - elif isinstance(value, list): + return _object_shape_violation(value, depth) + if isinstance(value, list): for child in value: violation = _arg_shape_violation(child, depth=depth + 1) if violation is not None: return violation + return None + if isinstance(value, str) and _over_string_cap(value): + return f"string value over the length cap of {_MAX_ARG_STRING_LENGTH} bytes" return None diff --git a/src/cmcp_runtime/mcp/server.py b/src/cmcp_runtime/mcp/server.py index bb11bed..d8380ce 100644 --- a/src/cmcp_runtime/mcp/server.py +++ b/src/cmcp_runtime/mcp/server.py @@ -41,6 +41,12 @@ # Endpoints exempt from bearer-token auth (Kubernetes liveness / readiness probes) _AUTH_EXEMPT_PATHS = {"/health", "/readyz"} +# DOS-001: default ceiling on a single request body. Overridable per +# deployment via MCPServer(max_request_bytes=...). Named here rather than +# left inline on the constructor so the argument-shape caps below can be +# derived from it instead of restating the number. +_DEFAULT_MAX_REQUEST_BYTES = 1_000_000 + # #518: DOS-001's byte cap bounds total size, not shape. A payload well under # the limit can still push toward Python's recursion limit through deep # nesting, or cost real time to iterate through a flat object with thousands @@ -51,6 +57,29 @@ _MAX_ARG_DEPTH = 20 _MAX_ARG_KEYS = 256 +# #562: docs/spec/proxy-security.md's Fuzzing Definition of Done specs +# MAX_STRING_LENGTH at 1MB per string field, separate from the depth/key +# caps above. A single oversized string can sit inside an otherwise +# shallow, low-key-count payload and slip past both of those unbounded, +# up to whatever the whole-body byte cap happens to be. +# +# Not set to the spec's literal 1MB: that equals the whole-body default +# below, and a 1MB string plus any JSON structure around it already +# exceeds that cap, so the check could never fire before DOS-001's size +# rejection already had. Derived from the default instead, so a single +# string cannot consume the whole request budget, the cap is actually +# reachable, and raising the default carries this along with it rather +# than silently leaving it behind. This is scoped against the *default* +# max_request_bytes; a deployment that configures a smaller value simply +# has the whole-body cap bind first, which is a safe direction to fail +# in, not a gap. Worth revisiting to the spec's literal value if the +# default is ever raised toward the spec's stated 10MB (see #562). +# +# Checked as UTF-8 byte length, not character count, since a codepoint +# count understates the actual memory and processing cost of multi-byte +# text. +_MAX_ARG_STRING_LENGTH = _DEFAULT_MAX_REQUEST_BYTES // 2 + def _reject_nan_and_infinity(text: str) -> float: raise ValueError(f"non-standard JSON value not allowed: {text}") @@ -62,23 +91,41 @@ def _valid_rpc_id(value: Any) -> bool: return value is None or (isinstance(value, (str, int, float)) and not isinstance(value, bool)) +def _over_string_cap(text: str) -> bool: + return len(text.encode("utf-8")) > _MAX_ARG_STRING_LENGTH + + +def _object_shape_violation(value: dict[str, Any], depth: int) -> str | None: + if len(value) > _MAX_ARG_KEYS: + return f"object has more than {_MAX_ARG_KEYS} keys" + for key, child in value.items(): + # Keys carry the same cost as values and are not covered by the key + # *count* cap above, so a single huge key would otherwise slip + # through every check here. + if isinstance(key, str) and _over_string_cap(key): + return f"object key over the length cap of {_MAX_ARG_STRING_LENGTH} bytes" + violation = _arg_shape_violation(child, depth=depth + 1) + if violation is not None: + return violation + return None + + def _arg_shape_violation(value: Any, *, depth: int = 0) -> str | None: - """Return a message describing the first depth/key-count violation found - under `value`, or None if it fits within `_MAX_ARG_DEPTH` / `_MAX_ARG_KEYS`.""" + """Return a message describing the first depth/key-count/string-length + violation found under `value`, or None if it fits within `_MAX_ARG_DEPTH` / + `_MAX_ARG_KEYS` / `_MAX_ARG_STRING_LENGTH`.""" if depth > _MAX_ARG_DEPTH: return f"arguments nested past the depth cap of {_MAX_ARG_DEPTH}" if isinstance(value, dict): - if len(value) > _MAX_ARG_KEYS: - return f"object has more than {_MAX_ARG_KEYS} keys" - for child in value.values(): - violation = _arg_shape_violation(child, depth=depth + 1) - if violation is not None: - return violation - elif isinstance(value, list): + return _object_shape_violation(value, depth) + if isinstance(value, list): for child in value: violation = _arg_shape_violation(child, depth=depth + 1) if violation is not None: return violation + return None + if isinstance(value, str) and _over_string_cap(value): + return f"string value over the length cap of {_MAX_ARG_STRING_LENGTH} bytes" return None @@ -255,7 +302,7 @@ def __init__( audit_chain: AuditChain | None = None, bearer_token: str | None = None, session: SessionState | None = None, - max_request_bytes: int = 1_000_000, + max_request_bytes: int = _DEFAULT_MAX_REQUEST_BYTES, ) -> None: self._proxy = proxy self._session_manager = session_manager @@ -506,7 +553,8 @@ async def _handle_tool_call(self, rpc_id: Any, params: dict[str, Any]) -> Respon tool_name: str = params.get("name", "").lower() arguments: dict[str, Any] = params.get("arguments", {}) - # #518: depth/key-count cap, matching scripts/mock_upstream.py. + # #518/#562: depth, key-count and string-length caps, matching + # scripts/mock_upstream.py. violation = _arg_shape_violation(arguments) if violation is not None: return JSONResponse( diff --git a/tests/unit/test_mcp_server_auth.py b/tests/unit/test_mcp_server_auth.py index a2166d6..a99fda2 100644 --- a/tests/unit/test_mcp_server_auth.py +++ b/tests/unit/test_mcp_server_auth.py @@ -2,13 +2,19 @@ from __future__ import annotations +import json from typing import Any from unittest.mock import AsyncMock, MagicMock, patch import pytest from starlette.testclient import TestClient -from cmcp_runtime.mcp.server import _MAX_ARG_DEPTH, _MAX_ARG_KEYS, MCPServer +from cmcp_runtime.mcp.server import ( + _MAX_ARG_DEPTH, + _MAX_ARG_KEYS, + _MAX_ARG_STRING_LENGTH, + MCPServer, +) def _make_server(bearer_token: str | None = None) -> MCPServer: @@ -699,6 +705,91 @@ def test_arguments_exceeding_key_cap_returns_invalid_params(): assert resp.json()["error"]["code"] == -32602 +# ── #562: argument string-length cap, matching scripts/mock_upstream.py ───── + +def test_string_within_length_cap_is_allowed(): + server = _make_server() + client = TestClient(server.app, raise_server_exceptions=False) + resp = client.post( + "/mcp", + json={ + "jsonrpc": "2.0", + "method": "tools/call", + "params": {"name": "t", "arguments": {"text": "a" * 1000}}, + "id": 1, + }, + ) + assert resp.status_code == 200 + + +def test_string_exceeding_length_cap_returns_invalid_params(): + server = _make_server() + client = TestClient(server.app, raise_server_exceptions=False) + resp = client.post( + "/mcp", + json={ + "jsonrpc": "2.0", + "method": "tools/call", + "params": {"name": "t", "arguments": {"text": "a" * (_MAX_ARG_STRING_LENGTH + 1)}}, + "id": 1, + }, + ) + assert resp.status_code == 400 + assert resp.json()["error"]["code"] == -32602 + + +def test_oversized_object_key_returns_invalid_params(): + """A huge key is as expensive as a huge value and is not bounded by the + key *count* cap, so it must be rejected on its own.""" + server = _make_server() + client = TestClient(server.app, raise_server_exceptions=False) + key = "a" * (_MAX_ARG_STRING_LENGTH + 1) + resp = client.post( + "/mcp", + json={ + "jsonrpc": "2.0", + "method": "tools/call", + "params": {"name": "t", "arguments": {key: 1}}, + "id": 1, + }, + ) + assert resp.status_code == 400 + assert resp.json()["error"]["code"] == -32602 + + +def test_multibyte_string_length_measured_in_bytes_not_characters(): + """A 4-byte-per-char string well under the byte cap in character count + but over it in UTF-8 bytes must still be rejected.""" + # U+1F600 (😀) is 4 bytes in UTF-8, 1 codepoint in Python's len(). Choose + # a character count that clears the string cap in bytes while staying + # well under the server's default max_request_bytes once JSON-encoded. + char_count = (_MAX_ARG_STRING_LENGTH // 4) + 1 + oversized_by_bytes = "\U0001F600" * char_count + assert len(oversized_by_bytes.encode("utf-8")) > _MAX_ARG_STRING_LENGTH + server = _make_server() + client = TestClient(server.app, raise_server_exceptions=False) + # Built and encoded by hand with ensure_ascii=False, rather than passed + # via the json= kwarg: both json.dumps' default and httpx's json= + # encoder escape each emoji to a 12-byte \uXXXX\uXXXX surrogate pair, + # inflating the wire size far past the string's actual UTF-8 length and + # tripping max_request_bytes instead of the check this test targets. + body = json.dumps( + { + "jsonrpc": "2.0", + "method": "tools/call", + "params": {"name": "t", "arguments": {"text": oversized_by_bytes}}, + "id": 1, + }, + ensure_ascii=False, + ).encode("utf-8") + assert len(body) < 1_000_000 + resp = client.post( + "/mcp", content=body, headers={"Content-Type": "application/json"} + ) + assert resp.status_code == 400 + assert resp.json()["error"]["code"] == -32602 + + # ── #518: non-standard JSON values (NaN, Infinity, -Infinity) ─────────────── @pytest.mark.parametrize("literal", ["NaN", "Infinity", "-Infinity"]) diff --git a/tests/unit/test_mock_upstream_gate.py b/tests/unit/test_mock_upstream_gate.py index 98b037e..db9c31b 100644 --- a/tests/unit/test_mock_upstream_gate.py +++ b/tests/unit/test_mock_upstream_gate.py @@ -20,7 +20,11 @@ import pytest sys.path.insert(0, str(Path(__file__).resolve().parents[2] / "scripts")) -from mock_upstream import MockMCPHandler # noqa: E402 +from mock_upstream import ( # noqa: E402 + _MAX_ARG_STRING_LENGTH, + MAX_REQUEST_BYTES, + MockMCPHandler, +) @pytest.fixture(scope="module") @@ -227,17 +231,23 @@ def test_far_oversized_request_beyond_the_drain_ceiling_still_gets_a_clean_respo def test_request_at_the_limit_is_accepted(upstream): - # Build a request whose total serialized size sits just under the cap. - filler_len = 1_000_000 - 200 + # Build a request whose total serialized size sits just under the whole + # body cap. Split across two string fields, each under the separate + # per-string length cap (#562), so this test stays targeted at the + # DOS-001 whole-body boundary rather than also tripping the string cap. + field_len = _MAX_ARG_STRING_LENGTH - 100 body = json.dumps( { "jsonrpc": "2.0", "id": 10, "method": "tools/call", - "params": {"name": "echo", "arguments": {"message": "x" * filler_len}}, + "params": { + "name": "echo", + "arguments": {"a": "x" * field_len, "b": "x" * field_len}, + }, } ).encode() - assert len(body) < 1_000_000 + assert len(body) < MAX_REQUEST_BYTES status, resp = _post(upstream, body) assert status == 200 assert resp["id"] == 10 @@ -417,6 +427,109 @@ def test_arguments_within_key_count_cap_are_accepted(upstream): assert status == 200 +# --------------------------------------------------------------------------- +# Argument string-length cap (#562, docs/spec/proxy-security.md MAX_STRING_LENGTH) +# --------------------------------------------------------------------------- + + +def test_string_within_length_cap_is_accepted(upstream): + req = json.dumps( + { + "jsonrpc": "2.0", + "id": 34, + "method": "tools/call", + "params": {"name": "echo", "arguments": {"text": "a" * 1000}}, + } + ).encode() + status, body = _post(upstream, req) + assert status == 200 + + +def test_string_past_length_cap_returns_invalid_params(upstream): + # Past the string cap but comfortably under MAX_REQUEST_BYTES, so this + # exercises the string-length check itself, not the whole-body size gate. + assert _MAX_ARG_STRING_LENGTH + 1 < MAX_REQUEST_BYTES + req = json.dumps( + { + "jsonrpc": "2.0", + "id": 35, + "method": "tools/call", + "params": {"name": "echo", "arguments": {"text": "a" * (_MAX_ARG_STRING_LENGTH + 1)}}, + } + ).encode() + status, body = _post(upstream, req) + assert status == 400 + assert body["error"]["code"] == -32602 + assert body["id"] == 35 + + +def test_oversized_object_key_returns_invalid_params(upstream): + """A huge key is as expensive as a huge value and is not bounded by the + key *count* cap, so it must be rejected on its own.""" + key = "a" * (_MAX_ARG_STRING_LENGTH + 1) + req = json.dumps( + { + "jsonrpc": "2.0", + "id": 38, + "method": "tools/call", + "params": {"name": "echo", "arguments": {key: 1}}, + } + ).encode() + # Reachable: over the string cap but still under the whole-body cap, so + # this exercises the key check rather than DOS-001's size rejection. + assert len(req) < MAX_REQUEST_BYTES + status, body = _post(upstream, req) + assert status == 400 + assert body["error"]["code"] == -32602 + assert body["id"] == 38 + + +def test_oversized_string_nested_inside_arguments_is_caught(upstream): + """The cap applies at any depth, not only to top-level string values.""" + req = json.dumps( + { + "jsonrpc": "2.0", + "id": 36, + "method": "tools/call", + "params": { + "name": "echo", + "arguments": {"child": {"text": "a" * (_MAX_ARG_STRING_LENGTH + 1)}}, + }, + } + ).encode() + status, body = _post(upstream, req) + assert status == 400 + assert body["error"]["code"] == -32602 + + +def test_multibyte_string_length_measured_in_bytes_not_characters(upstream): + """A 4-byte-per-char string well under the byte cap in character count + but over it in UTF-8 bytes must still be rejected.""" + # U+1F600 (😀) is 4 bytes in UTF-8, 1 codepoint in Python's len(). Choose + # a character count that clears the string cap in bytes while staying + # well under MAX_REQUEST_BYTES once JSON-encoded. + char_count = (_MAX_ARG_STRING_LENGTH // 4) + 1 + oversized_by_bytes = "\U0001F600" * char_count + assert len(oversized_by_bytes.encode("utf-8")) > _MAX_ARG_STRING_LENGTH + # ensure_ascii=False: the default True would escape each emoji to a + # 12-byte \uXXXX\uXXXX surrogate pair, inflating the wire size far past + # the string's actual UTF-8 length and tripping MAX_REQUEST_BYTES + # instead of the check this test targets. + req = json.dumps( + { + "jsonrpc": "2.0", + "id": 37, + "method": "tools/call", + "params": {"name": "echo", "arguments": {"text": oversized_by_bytes}}, + }, + ensure_ascii=False, + ).encode() + assert len(req) < MAX_REQUEST_BYTES + status, body = _post(upstream, req) + assert status == 400 + assert body["error"]["code"] == -32602 + + # --------------------------------------------------------------------------- # Non-standard JSON values (NaN, Infinity, -Infinity) (#518) # --------------------------------------------------------------------------- From 2c969289972fcc913bce49e794df0c546b4b704f Mon Sep 17 00:00:00 2001 From: Yatsuiii Date: Tue, 25 Aug 2026 10:59:54 +0530 Subject: [PATCH 2/2] test: cover the clean-list fall-through Codecov flagged The list branch of _arg_shape_violation returns None explicitly once the loop finds nothing, so that every branch terminates on its own rather than depending on an earlier branch's return for the string check below it to be reachable. That return had no test. The only list coverage was the rejection path, which would still pass if the walk wrongly rejected every list it saw. Adds the accepting case to both files so the two stay in step. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KDXJ4ghkW6v56W8St2w5kg --- tests/unit/test_mcp_server_auth.py | 21 +++++++++++++++++++++ tests/unit/test_mock_upstream_gate.py | 18 ++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/tests/unit/test_mcp_server_auth.py b/tests/unit/test_mcp_server_auth.py index a99fda2..46f275b 100644 --- a/tests/unit/test_mcp_server_auth.py +++ b/tests/unit/test_mcp_server_auth.py @@ -672,6 +672,27 @@ def test_depth_cap_violation_inside_a_list_is_caught(): assert resp.json()["error"]["code"] == -32602 +def test_well_formed_list_in_arguments_is_accepted(): + """The list walk must fall through cleanly when nothing inside it violates a + cap. Without this the only list coverage is the rejection path, which would + still pass if the walk rejected every list it saw.""" + server = _make_server() + client = TestClient(server.app, raise_server_exceptions=False) + resp = client.post( + "/mcp", + json={ + "jsonrpc": "2.0", + "method": "tools/call", + "params": { + "name": "t", + "arguments": {"items": [1, "ok", {"nested": ["fine"]}, None]}, + }, + "id": 1, + }, + ) + assert resp.status_code != 400 + + def test_arguments_within_key_cap_is_allowed(): server = _make_server() client = TestClient(server.app, raise_server_exceptions=False) diff --git a/tests/unit/test_mock_upstream_gate.py b/tests/unit/test_mock_upstream_gate.py index db9c31b..f953832 100644 --- a/tests/unit/test_mock_upstream_gate.py +++ b/tests/unit/test_mock_upstream_gate.py @@ -484,6 +484,24 @@ def test_oversized_object_key_returns_invalid_params(upstream): assert body["id"] == 38 +def test_well_formed_list_in_arguments_is_accepted(upstream): + """The list walk must fall through cleanly when nothing inside it violates a + cap, mirroring the same case in tests/unit/test_mcp_server_auth.py.""" + req = json.dumps( + { + "jsonrpc": "2.0", + "id": 40, + "method": "tools/call", + "params": { + "name": "echo", + "arguments": {"items": [1, "ok", {"nested": ["fine"]}, None]}, + }, + } + ).encode() + status, _ = _post(upstream, req) + assert status == 200 + + def test_oversized_string_nested_inside_arguments_is_caught(upstream): """The cap applies at any depth, not only to top-level string values.""" req = json.dumps(