diff --git a/CHANGELOG.md b/CHANGELOG.md index d31564e0..d105951f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,29 @@ any change to one appears here. ## [Unreleased] -Nothing yet. +### Fixed + +- **The gateway compared mirrored header values by re-parsing them, and Python's parser is + lenient.** `Mcp-Param-{Name}` validation ran the header through `int()`, so a body carrying + `amount: 2000` was certified as agreeing with headers spelling it `2_000`, `+2000`, `02000`, + ` 2000`, `2000 `, and the Arabic-indic and fullwidth digit forms; booleans were matched + case-insensitively, so `TRUE`, `True` and `tRuE` all agreed with `true`. None of those is + what a JSON serializer writes, and each is read differently — or refused — by another + parser: `parseInt("2_000")` is 2 in JavaScript and `strconv.Atoi` errors in Go. + + That is the exact hazard SPEC-v0.2 §6.4 exists to prevent. The gateway's job there is to + certify that a routing intermediary and CTRLRun are looking at the same value, and it was + certifying agreement that held only under Python's rules. CTRLRun's own decisions were never + affected — the action is built from the body, and the headers are only checked — so this + costs an intermediary's correctness rather than an approval binding. + + A header value must now be the body value's canonical rendering, compared as text, with no + parser in the comparison — and only a string, an integer or a boolean has one. The revision + permits `x-mcp-header` on those three types alone and omits the header for a `null`, so a + header naming an argument of any other type is refused rather than compared against a + rendering CTRLRun invented for it. This also declines the revision's SHOULD that servers compare + integers numerically (`42.0` equals `42`): v0.1 §2.3 refuses a float in the body outright, so + the leniency has no legitimate case here. SPEC-v0.2 §6.4 states both rules and the reasoning. ## [0.2.0] — unreleased diff --git a/docs/SPEC-v0.2.md b/docs/SPEC-v0.2.md index 06892b75..5d0dca0f 100644 --- a/docs/SPEC-v0.2.md +++ b/docs/SPEC-v0.2.md @@ -513,8 +513,30 @@ for. So: - The gateway MUST parse the body of every request it forwards and MUST validate `MCP-Protocol-Version`, `Mcp-Method`, `Mcp-Name` and every `Mcp-Param-{Name}` against it, - decoding the `=?base64?…?=` sentinel before comparing, and comparing integers numerically. - A mismatch, or a missing required header, is HTTP 400 with `-32020`. + decoding the `=?base64?…?=` sentinel before comparing. A mismatch, or a missing required + header, is HTTP 400 with `-32020`. +- **A header value MUST be the body value's canonical rendering**, compared as text, and only + three types have one. The revision's Value Encoding is exact: *"`string`: Use the value + as-is"*, *"`integer`: Convert to decimal string representation (e.g., `42`, `-7`)"*, + *"`boolean`: Convert to lowercase `"true"` or `"false"`"* — and `x-mcp-header` *"**MUST** + only be applied to parameters with primitive types (integer, string, boolean)"*, with a + `null` parameter omitting its header entirely. A `Mcp-Param-{Name}` naming an argument of any + other type — a list, a mapping, a `null` — therefore cannot agree with it, and is refused + rather than compared against a rendering CTRLRun invented for it. + + This is stricter than the revision's *"servers SHOULD compare the header value and the body + value numerically rather than as strings (e.g., `42.0` and `42` are considered equal)"*, and + the gateway **declines that SHOULD**, for two reasons. A float cannot appear in the body at + all — v0.1 §2.3 refuses it and §6.6 turns it into `-41008` — so the leniency has no + legitimate case here; taking it would mean accepting a spelling of a number the body could + never hold. And re-parsing is what creates the hazard this section is about: `int("2_000")` + is 2000 in Python, `parseInt("2_000")` is 2 in JavaScript, and `strconv.Atoi("2_000")` is an + error in Go, so a gateway that certified `Mcp-Param-Amount: 2_000` as agreeing with a body + carrying `2000` would be telling a load balancer that a header they read differently is + safe to route on. `+2000`, `02000`, leading or trailing whitespace, and the Arabic-indic and + fullwidth spellings of the digits are the same defect wearing other hats; so is matching + `true` case-insensitively. The comparison has no parser in it, so it has no parser to + disagree with. - The gateway MUST decide what to intercept from the **body's** `method` and `params.name`, never from the headers. The headers are checked; the body is believed. - The gateway MUST NOT rely on the upstream to perform this validation. A `-32020` arriving diff --git a/src/ctrlrun/gateway/mcp.py b/src/ctrlrun/gateway/mcp.py index 2306e654..e218f7d3 100644 --- a/src/ctrlrun/gateway/mcp.py +++ b/src/ctrlrun/gateway/mcp.py @@ -273,21 +273,35 @@ def encode_header_value(value: str) -> str: def _agrees(declared: str, value: object) -> bool: - """Whether a header value agrees with the body's, comparing integers numerically (§6.4). - - `2000` in a header is text and the body's is an `int`; comparing them as strings would - refuse every well-formed request that annotated a numeric parameter. + """Whether a header value is the body value's canonical rendering (SPEC-v0.2 §6.4). + + A string's header carries its bare text; everything else must be exactly what JSON writes + for it — `2000`, `true`, `null`. One rendering per value, produced by the serializer the + body already went through, so there is nothing to re-parse and nothing to be lenient about. + + This used to call `int(declared)`, which reads `2_000`, `+2000`, `02000`, ` 2000`, and the + Arabic-indic and fullwidth spellings of 2000 as agreeing with a body carrying `2000`. None + of them is what a JSON serializer writes, and each is read differently — or not at all — by + another parser: `parseInt("2_000")` is 2 in JavaScript and `strconv.Atoi` refuses it in Go. + §6.4 exists because "different components in the network rely on different sources of + truth", so certifying agreement that only holds under Python's rules is the hazard rather + than a convenience. Booleans were compared case-insensitively, with the same consequence. + + It also declines the revision's SHOULD that a server compare integers numerically, under + which `2000.0` equals `2000`. v0.1 §2.3 refuses a float in the body outright (§6.6), so the + leniency has no legitimate case here; taking it would mean accepting a spelling of a number + that the body could never hold. + + Anything that is not a string, an integer or a boolean has **no** header encoding at all: + the revision permits `x-mcp-header` only on those three, and says a `null` parameter omits + its header entirely. So a header naming an argument of any other type cannot agree with it, + and the answer is `False` rather than a comparison against a rendering CTRLRun made up. """ - if isinstance(value, bool): - return declared.lower() == str(value).lower() - if isinstance(value, int): - try: - return int(declared) == value - except ValueError: - return False if isinstance(value, str): return declared == value - return declared == json.dumps(value, separators=(",", ":")) + if isinstance(value, int): # bool included, and JSON writes it as `true` / `false` + return declared == json.dumps(value) + return False def find_unrepresentable(arguments: object, pointer: str = "") -> str | None: diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 09329d28..0d80da8c 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -241,22 +241,122 @@ def test_an_Mcp_Param_agreeing_with_the_body_is_accepted(): assert not isinstance(parse_request(_body(), headers), Refusal) -def test_an_Mcp_Param_integer_is_compared_numerically(): - """§6.4 — `2000` in a header is a string; the body's is an int. Comparing them as text - would refuse every well-formed request that annotated a numeric parameter.""" +def test_an_Mcp_Param_integer_agrees_with_its_canonical_spelling(): + """§6.4 — `2000` in a header is text and the body's is an `int`, so the comparison is + against what JSON writes for it. This is also the control for the parametrization below: + without it, an implementation that refused every integer header would pass those.""" headers = _headers() headers["Mcp-Param-amount"] = "2000" assert not isinstance(parse_request(_body(), headers), Refusal) -def test_an_Mcp_Param_integer_that_differs_numerically_is_refused(): +def test_an_Mcp_Param_integer_that_differs_is_refused(): headers = _headers() headers["Mcp-Param-amount"] = "2001" assert isinstance(parse_request(_body(), headers), Refusal) +#: Spellings that Python's `int()` accepts and that no JSON serializer produces. Each is a +#: header whose text differs from the body's value, so an intermediary that parses it with a +#: different set of rules reads a different number — the split-brain §6.4 exists to prevent. +#: JavaScript's `parseInt("4_2")` is 4, Go's `strconv.Atoi("4_2")` is an error, and Python's +#: `int("4_2")` is 42. +_LENIENT_INTEGER_SPELLINGS = [ + "2_000", # PEP 515 underscore; JS parseInt reads 2 + "+2000", # leading plus + " 2000", # leading whitespace + "2000 ", # trailing whitespace + "2000\n", # trailing newline + "02000", # leading zero + "\u0662\u0660\u0660\u0660", # arabic-indic digits for 2000 + "\uff12\uff10\uff10\uff10", # fullwidth digits for 2000 +] + + +@pytest.mark.parametrize("declared", _LENIENT_INTEGER_SPELLINGS) +def test_an_Mcp_Param_integer_must_be_the_body_value_as_text(declared): + """§6.4 — the header must be the body value's canonical rendering, not merely something + Python's `int()` maps onto it. Each of these parses to 2000 under `int()` and to something + else, or to nothing, under another parser.""" + headers = _headers() + headers["Mcp-Param-amount"] = declared + + refusal = parse_request(_body(), headers) + + assert isinstance(refusal, Refusal), f"{declared!r} was accepted" + assert (refusal.http_status, refusal.code) == (400, -32020) + + +@pytest.mark.parametrize("declared", ["TRUE", "True", "tRuE", "1", "yes"]) +def test_an_Mcp_Param_boolean_must_be_its_json_spelling(declared): + """§6.4 — `true` and `false`, as JSON writes them. A case-insensitive match accepts four + spellings of one value, and an intermediary comparing bytes sees four different headers.""" + document = { + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": {"name": "create_refund", "arguments": {"partial": True}}, + } + headers = _headers() + headers["Mcp-Param-partial"] = declared + + refusal = parse_request(_body(document), headers) + + assert isinstance(refusal, Refusal), f"{declared!r} was accepted" + assert (refusal.http_status, refusal.code) == (400, -32020) + + +def test_the_canonical_boolean_spelling_is_still_accepted(): + document = { + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": {"name": "create_refund", "arguments": {"partial": True}}, + } + headers = _headers() + headers["Mcp-Param-partial"] = "true" + + assert not isinstance(parse_request(_body(document), headers), Refusal) + + +@pytest.mark.parametrize("value", [None, [1, 2], {"a": 1}], ids=["null", "list", "object"]) +def test_an_Mcp_Param_naming_a_non_primitive_argument_is_refused(value): + """§6.4 — the revision defines an encoding for exactly three types: string, integer and + boolean. It permits `x-mcp-header` on nothing else, and a `null` parameter omits the header + entirely. So no header value can agree with any of these, and comparing against a rendering + CTRLRun invented would certify an agreement under nobody's rules but its own. + + The header carries the compact rendering the old code compared against, so each case fails + on the rule rather than on a stray space. A float is not here: §6.6 refuses one in the body + outright with `-41008`, so it never reaches this comparison. + """ + document = { + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": {"name": "create_refund", "arguments": {"extra": value}}, + } + headers = _headers() + headers["Mcp-Param-extra"] = json.dumps(value, separators=(",", ":")) + + refusal = parse_request(_body(document), headers) + + assert isinstance(refusal, Refusal), f"{value!r} was accepted" + assert (refusal.http_status, refusal.code) == (400, -32020) + + +def test_a_float_spelling_of_an_integer_is_refused(): + """§6.4 declines the revision's numeric-leniency SHOULD (`42.0` equals `42`). v0.1 §2.3 + refuses a float in the body outright, so the leniency has no legitimate case here, and + honouring it would mean accepting a header spelling other parsers read differently.""" + headers = _headers() + headers["Mcp-Param-amount"] = "2000.0" + + assert isinstance(parse_request(_body(), headers), Refusal) + + def test_an_Mcp_Param_naming_an_argument_the_body_does_not_have_is_refused(): headers = _headers() headers["Mcp-Param-nothing"] = "x"