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..46f275b 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: @@ -666,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) @@ -699,6 +726,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..f953832 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,127 @@ 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_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( + { + "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) # ---------------------------------------------------------------------------