fix: enforce a per-string length cap on tool arguments (#562)

[Yatsuiii]

Closes #562.

docs/spec/proxy-security.md's Fuzzing Definition of Done specs MAX_STRING_LENGTH = 1 * 1024 * 1024 # 1MB per string field. It is not implemented anywhere in src/ or scripts/. The depth and key-count caps that landed in #556 and #561 bound how deep and how wide a payload is, not how large any one piece of it is, so a single oversized string sitting inside an otherwise shallow, low-key-count arguments object passes both of them unbounded, up to whatever the whole-body byte cap happens to be.

This extends the existing _arg_shape_violation walk in both files with a UTF-8 byte length check.

Why the cap is not the spec's literal 1MB

This is the part I would look at first if I were reviewing it.

The spec's MAX_STRING_LENGTH is 1MB. MAX_REQUEST_BYTES is also 1MB in both files today. A 1MB string plus the JSON structure around it already exceeds the whole-body cap, so a check at the literal spec value could never fire before DOS-001's size rejection already had. Implemented at the spec's number, this would have been dead code that reads like a control.

So the cap is half the whole-body limit instead, and it is derived from a named constant rather than a restated literal:

_DEFAULT_MAX_REQUEST_BYTES = 1_000_000
...
_MAX_ARG_STRING_LENGTH = _DEFAULT_MAX_REQUEST_BYTES // 2

Hoisting the constructor default into _DEFAULT_MAX_REQUEST_BYTES is what makes that derivation possible without stating 1_000_000 twice. Raising the default now carries the string cap along with it instead of silently leaving it behind at a stale absolute number.

Verified reachable rather than assumed: an over-cap string is 500,101 bytes on the wire against a 1,000,000 byte body cap, and an over-cap key is 500,099. Both land inside the window where the string check is the thing that rejects them.

This is scoped against the default max_request_bytes. A deployment that configures something smaller just has the whole-body cap bind first, which is a safe direction to fail in rather than a gap.

Object keys, not only values

_MAX_ARG_KEYS bounds how many keys an object has, not how large each one is. A single huge key would otherwise pass every check in this function, so keys are measured against the same cap. _object_shape_violation was extracted to hold that without pushing _arg_shape_violation past the complexity limit.

Byte length, not character count

Measured as UTF-8 bytes. A codepoint count understates the real memory and processing cost of multi-byte text, and it is the byte length that the whole-body cap is already denominated in, so the two caps now speak the same unit. There is a test for a string that is under the cap by characters and over it by bytes.

What I am deliberately not doing here

MAX_REQUEST_BYTES, 10MB in the spec versus 1MB in both implementations. Flagged on #562 rather than resolved. It is not obvious whether 1MB was a deliberate tightening or spec drift, and picking a number is a maintainer call, not one to make inside a change scoped to a different constant. This PR works around the mismatch rather than resolving it.

MAX_PARSE_TIME_MS = 100. Also unenforced anywhere, also left open on #562. A real wall-clock bound on json.loads needs either a signal-based timeout, which does not work on the Windows CI legs this repo runs, or an executor or subprocess. That is a design decision rather than a missing check.

params.name is not shape-checked. It is a string field, so the spec's MAX_STRING_LENGTH arguably covers it, but it sits outside the arguments walk this PR extends and is bounded by the whole-body cap today. Naming it rather than widening the scope of a change that already touches two files.

server.py still does not require arguments to be an object, while the mock does. That gap was raised on #561 and left alone deliberately. One behaviour note for the record: a bare oversized string passed as arguments now gets rejected by the string cap on the gateway side, where before it fell through the shape walk untouched. Stricter, in the safe direction, but it is a change on a path that was previously discussed.

On the duplication

Worth stating plainly, since it was noted on #561 that the caps are kept in sync by comment reference rather than by shared code. This change makes that worse, not better: it is now three constants and three functions duplicated across scripts/mock_upstream.py and src/cmcp_runtime/mcp/server.py.

The reason is that scripts/mock_upstream.py imports stdlib only. Giving it a shared module to import from cmcp_runtime would cost it the standalone property that lets it run as demo scaffolding from docker-compose.yml and docs/quickstart.md without the package installed. I did not think that trade was mine to make unilaterally, so I kept the duplication and the comment-reference sync. If the preference is a shared module and the mock taking a dependency on the package, that is a small follow-up and I am happy to do it.

The two copies are byte-identical, which is at least checkable.

Verification

cap parity        : server 500,000 == mock 500,000
reachable         : over-cap string = 500,101 bytes on the wire vs the 1,000,000 body cap
                    over-cap key    = 500,099 bytes
helpers identical : diff across both files is empty

over-cap string value            -> string value over the length cap of 500000 bytes
over-cap object key              -> object key over the length cap of 500000 bytes
over-cap string in list          -> string value over the length cap of 500000 bytes
over-cap bare string arguments   -> string value over the length cap of 500000 bytes
at-cap string                    -> None          (boundary is >, not >=)
multibyte over cap by bytes only -> string value over the length cap of 500000 bytes

• tests/unit/test_mock_upstream_gate.py and tests/unit/test_mcp_server_auth.py: 90 passed, 9 of them new. Both suites drive the real handler over a socket or the real ASGI app rather than reimplementing the rules.
• Full tests/unit: 1338 passed. The 6 test_startup.py failures I see locally are FileNotFoundError: 'tpm2_pcrread', reproduce identically on a clean origin/main with this diff stashed, and are a missing tpm2-tools binary on my machine rather than anything from this change.
• ruff check on all four touched files is clean apart from one pre-existing T201 print at scripts/mock_upstream.py:241, confirmed pre-existing on main the same way. Left alone as unrelated.

Based on origin/main at a2893da, after #556 and #561 landed, so the diff here is only this change.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[imran-siddique]

You flagged the deviation from the spec as the thing to look at first, so I looked at it first, and you are right.

At pinned 0d3cb8eb: docs/spec/proxy-security.md:41 specs MAX_STRING_LENGTH = 1 * 1024 * 1024, and the whole-body cap is 1_000_000 in scripts/mock_upstream.py and at server.py:258. A 1 MiB string is already larger than the entire permitted body before you add the JSON scaffolding around it, so a per-string check at the spec's literal value could not fire. Ever.

"Dead code that reads like a control" is exactly the right framing and it is worse than no control. A missing check is visible as missing. A present check that cannot trigger passes review, passes an audit read of the source, and gets counted as coverage in a Definition of Done. Implementing the spec's number would have satisfied #562 on paper and left the gap open.

Deriving it as _DEFAULT_MAX_REQUEST_BYTES // 2 rather than writing 500_000 is what stops this recurring. If someone raises the body cap later, the string cap moves with it and the derivation stays true; a restated literal would silently re-create the same dead-code condition at the new ratio without anyone noticing. Hoisting the constructor default into a named constant to make that possible is a small change carrying the whole argument.

The gap itself is real and is the natural third one: #556 and #561 bounded how deep and how wide a payload is, not how large any single piece of it is, so one oversized string in a shallow object with few keys walked through both.

Keeping the gateway and the mock in step again is right, for the same reason as last time: a mock that accepts what the gateway rejects is not a conformance reference.

That leaves the spec itself stating a number that cannot be enforced as written. Not your problem in this PR, and worth fixing separately so the next person implementing from that document does not reach the same conclusion the hard way.

Third one of these today, and each time you have named the thing you were unsure about rather than hoping nobody looked. That is why they are quick to review.

Approving and merging.