Align x-mcp-header implementation with SEP-2243 spec clarifications#1619
Align x-mcp-header implementation with SEP-2243 spec clarifications#1619tarekgh wants to merge 6 commits into
Conversation
- Enforce RFC 9110 tchar validation for header names using SearchValues on .NET 8+ and bitmap lookup on netstandard2.0 - Remove 'number' from allowed x-mcp-header types (only string, integer, boolean are permitted); reject float/double/decimal in [McpHeader] - Add base64 sentinel collision check in McpHeaderEncoder.RequiresBase64Encoding so literal values resembling =?base64?...?= are encoded to avoid confusion - Fix DecodeValue to use case-sensitive prefix matching per spec requirement that sentinel markers must appear exactly as shown (lowercase) - Support nested property traversal for x-mcp-header validation and extraction on both client and server sides - Share tchar validation logic between McpHeaderAttribute and McpHeaderExtractor via internal FindFirstNonTchar method - Add recursive ValidateCustomParamHeadersFromProperties in StreamableHttpHandler for server-side nested property header/body comparison - Add 28 new tests covering tchar validation, sentinel collision encoding, number type rejection, nested property handling, and case-sensitive decoding
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK’s SEP-2243 x-mcp-header handling to match the latest spec clarifications, tightening header-name validation, restricting allowed header types, hardening Base64 sentinel parsing, and extending validation/extraction to nested schema properties.
Changes:
- Enforce RFC 9110
tcharvalidation forx-mcp-header/[McpHeader]names and share that logic across client/server code. - Disallow
"number"/ floating-point header types forx-mcp-header(client-side schema rejection; server-side attribute/type checks). - Make Base64 sentinel decoding case-sensitive and add “sentinel collision” encoding safeguards; add nested-schema traversal for
x-mcp-header.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ModelContextProtocol.Tests/Server/McpHeaderAttributeTests.cs | Expands test coverage for RFC 9110 tchar header-name validation. |
| tests/ModelContextProtocol.Tests/Client/McpHeaderExtractorValidationTests.cs | Adds integration tests for number rejection, nested properties, and tchar enforcement. |
| tests/ModelContextProtocol.Tests/Client/McpHeaderEncoderTests.cs | Adds sentinel-collision tests and asserts case-sensitive sentinel decoding. |
| src/ModelContextProtocol.Core/Server/McpHeaderAttribute.cs | Switches header-name validation to RFC 9110 tchar via shared helper. |
| src/ModelContextProtocol.Core/Server/AIFunctionMcpServerTool.cs | Tightens [McpHeader] supported types to integer/string/boolean (removes float/double/decimal). |
| src/ModelContextProtocol.Core/Protocol/McpHeaderEncoder.cs | Enforces case-sensitive sentinel parsing and prevents sentinel-collision ambiguity. |
| src/ModelContextProtocol.Core/Client/McpHeaderExtractor.cs | Adds nested property traversal and stricter schema validation for x-mcp-header. |
| src/ModelContextProtocol.Core/Client/McpClient.Methods.cs | Updates inline comments around x-mcp-header validation behavior. |
| src/ModelContextProtocol.AspNetCore/StreamableHttpHandler.cs | Adds recursive server-side validation for nested x-mcp-header properties. |
- Reject x-mcp-header parameters whose JSON Schema type is a disallowed or malformed value, including non-string/union types. A union array is accepted only when it contains at least one of string/integer/boolean (with optional "null"); an absent type is still treated as unknown and allowed to avoid excluding valid enum/const/$ref schemas. - Reword the rejection message: "number" is a JSON primitive but is not a permitted header type, so report it as an unsupported type rather than non-primitive. - Clarify the ListToolsAsync comment to note that the client validates x-mcp-header annotations on all transports, while only Streamable HTTP is required to by SEP-2243. - Drop the unused parameter from EncodeValue_NonSentinelPattern_NotBase64Encoded. - Add tests for nullable union, disallowed union, null-only union, and missing-type schemas.
halter73
left a comment
halter73
left a comment
There was a problem hiding this comment.
I had AI Compare the state of this PR to the changes in modelcontextprotocol/modelcontextprotocol#2772, and it came up with this feedback. I looked over the four comments, and they make sense to me, but let me know if there's anything off.
| /// <summary> | ||
| /// Recursively validates x-mcp-header annotated properties at any nesting depth. | ||
| /// </summary> | ||
| private static bool ValidateCustomParamHeadersFromProperties( |
There was a problem hiding this comment.
Two spec-compliance gaps in the server-side validation flow that this new helper is the entry point for:
1. The safe-integer-range MUST isn't enforced. The merged SEP-2243 clarification adds (emphasis mine):
Integer values MUST be within the safe range for integers represented using IEEE754 double-precision floating point numbers (−2⁵³+1 to 2⁵³−1).
Today, the existing ValuesMatch happily exact-matches "9007199254740993" on both sides (long.TryParse succeeds for both), even though no double-based JSON parser can round-trip that value — exactly the precision hazard the WG was avoiding.
2. ValuesMatch falls back to Math.Abs(a - b) < 1e-9 for the "integer" path. This is the floating-point tolerance approach that the Transports WG explicitly rejected during review of modelcontextprotocol/modelcontextprotocol#2772. The consensus was: integers only, exact comparison. Additionally, the schemaType == "number" branch in that same method is now dead code that silently tolerates spec-illegal tool definitions.
Suggested fixes in ValuesMatch:
- Drop the
"number"branch entirely (returnfalseifschemaType == "number"— a compliant tool definition can never reach that path, and we shouldn't silently tolerate it from non-compliant servers). - For
"integer": if either side parses to along, require both sides parse to the samelongand be within±(2⁵³−1). For decimal-formatted forms like"42.0", normalize first (e.g., parse asdecimal, verify zero fractional part, verify range, then compare as integers). No tolerance window.
A small helper would keep the rule centralized:
private const long MaxSafeInteger = 9007199254740991L; // 2^53 − 1
private static bool IsSafeInteger(long v) => v >= -MaxSafeInteger && v <= MaxSafeInteger;Happy to take this as a follow-up if you'd prefer to keep this PR focused.
| // Avoid sentinel collision: if the value matches the base64 wrapper pattern, | ||
| // it must be encoded to prevent ambiguity during decoding. | ||
| if (value.StartsWith(Base64Prefix, StringComparison.Ordinal) && | ||
| value.EndsWith(Base64Suffix, StringComparison.Ordinal)) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Nit: with number now spec-disallowed for x-mcp-header, the public EncodeValue(double) overload and the float/double/decimal cases in ConvertToString are dead-or-misleading API surface. Worth either marking [Obsolete] or dropping (this package is still preview, so I'd lean toward dropping). At minimum, the class-level <remarks> and the XML doc on EncodeValue(object?) still say "string, numeric types, and boolean" — those should be updated to "string, integer, and boolean" to match the spec.
While here: the class summary's encoding-rules <list> doesn't mention sentinel-collision encoding even though RequiresBase64Encoding now does it. Worth adding a bullet so the doc reflects the new behavior.
|
|
||
| // Look for the corresponding argument value | ||
| if (!arguments.Value.TryGetProperty(property.Name, out var argValue)) | ||
| if (!arguments.TryGetProperty(property.Name, out var argValue)) |
There was a problem hiding this comment.
A few lines below this, the emitter calls McpHeaderEncoder.ConvertToHeaderValue(argValue), which for numbers uses JsonElement.GetRawText() (and the JsonNode overload uses ToJsonString()). That means a body value of 42.0 for an integer-typed parameter becomes the header literal "42.0".
The SEP-2243 clarification PR is explicit about the emission rule:
integer: Convert to decimal string representation (e.g.,42,-7)
So an intermediary keying on the canonical "42" won't match the "42.0" we emit — even when the body itself happens to round-trip, the header is no longer in canonical form. This also interacts badly with the SHOULD on the server side ("compare numerically rather than as strings"), because intermediaries don't necessarily implement that lenience.
Recommendation: when the schema type at this property is "integer", normalize on emission — parse the numeric value, verify it's a whole number within ±(2⁵³−1), and emit the canonical decimal string (long.ToString(InvariantCulture)). If the value isn't representable as a safe integer, skip the header / surface an error rather than emit a spec-violating value (the server will then reject for missing required header, which is the right failure mode).
| type == typeof(long) || | ||
| type == typeof(ulong) || | ||
| type == typeof(float) || | ||
| type == typeof(double) || | ||
| type == typeof(decimal); | ||
| type == typeof(ulong); |
There was a problem hiding this comment.
ulong should probably be dropped from [McpHeader]-allowed-types for the same reason float/double/decimal were — its value range (up to ~1.8 × 10¹⁹) far exceeds the spec's safe integer range of ±(2⁵³−1) ≈ ±9 × 10¹⁵, so a ulong parameter can hold values that violate SEP-2243 by construction.
| type == typeof(long) || | |
| type == typeof(ulong) || | |
| type == typeof(float) || | |
| type == typeof(double) || | |
| type == typeof(decimal); | |
| type == typeof(ulong); | |
| type == typeof(long); |
Note that long/uint can also exceed the safe range at runtime (long.MaxValue is well above 2⁵³−1). Those need a per-value bounds check at emission time (client) and at validation time (server) rather than a type-level rejection. See the related comment on StreamableHttpHandler.cs.
There was a problem hiding this comment.
Done in the latest push. Dropped ulong as suggested, and added the per-value safe-integer bounds checks you mentioned: the client (McpHeaderExtractor) now canonicalizes integers and throws for non-whole or out-of-range values before sending, and the server (StreamableHttpHandler.ValuesMatch) enforces the ±(2^53-1) range and rejects out-of-range values (including identical header/body pairs and exponent forms too large for decimal).
One small correction on the note: uint is actually within the safe range (uint.MaxValue is ~4.29e9, well below 2^53-1 ~= 9.007e15), so it does not need special handling. Only long and ulong can exceed the safe range, and both are now covered by the per-value checks (long via the runtime bounds check, ulong via the type-level drop here).
Align x-mcp-header integer handling with the merged spec clarifications: - Server (StreamableHttpHandler.ValuesMatch): compare integer header and body values numerically with exact precision, enforce the JavaScript safe integer range (-2^53+1 to 2^53-1), and reject out-of-range values with a distinct error. Detect integer union types (e.g. ["integer", "null"]) and accept decimal and exponent forms (42.0, 4.2e1). Removed the dead "number" branch and the tolerance-based double comparison. - Client (McpHeaderExtractor): emit integer parameters in canonical decimal form (42.0 becomes "42") and reject non-whole or out-of-range values before sending. - Server tool guard (IsPrimitiveHeaderType): drop ulong, whose domain exceeds the signed safe range. - McpHeaderEncoder: remove the unused double overload and float/double/decimal conversions now that the number type is disallowed, and update docs. Added tests covering canonical emission, safe-range and exponent rejection, union integer matching, and ulong rejection at tool creation.
Replace the hand-rolled LooksLikeNumericLiteral scanner with a double.TryParse gate. When a header/body value is a well-formed numeric literal too large for decimal, double can still parse it, which lets us flag it as out of the safe integer range. double is only used as a yes/no numeric gate here; its loss of integer precision past 2^53 does not matter because every in-range value is handled by the decimal parse. This code only ships in ModelContextProtocol.AspNetCore (net8/9/10), so the modern overflow-to-infinity parsing behavior is guaranteed.
Use long.TryParse for safe-integer validation on both the client (McpHeaderExtractor) and server (StreamableHttpHandler) instead of decimal. decimal silently rounds high-precision values, so non-integers such as 42.0000000000000000000000000001 and 1e-100000 were accepted as whole integers. long.TryParse inspects the actual digits (rejecting non-integers without rounding) and fails fast on overflow (so a literal such as 1e1000000 cannot allocate a large number). On the server, classify each value as SafeInteger, OutOfRange, NonInteger, or NotNumeric so a numeric-but-non-integer value for an integer-typed parameter is rejected rather than slipping through the ordinal comparison when the header and body strings are identical. Decode base64-wrapped header values with a strict UTF-8 decoder so malformed byte sequences are rejected instead of being replaced with U+FFFD. Remove the unused IsValidTcharString helper, and fix a numeric equivalence test that passed the body value as a double (which stringified 42.0 back to 42) so the decimal and exponent body forms are actually exercised. Add tests for non-integer rejection, high-precision fraction rejection, canonical exponent emission, and invalid-UTF-8 base64 decoding.
…iance # Conflicts: # src/ModelContextProtocol.AspNetCore/StreamableHttpHandler.cs
Summary
Implements the spec clarifications from modelcontextprotocol/modelcontextprotocol#2772 to ensure the SDK fully complies with SEP-2243.
Changes
RFC 9110 tchar validation for header names
Remove "number" from allowed x-mcp-header types
Base64 sentinel collision check
Nested property support
Tests
28 new tests added:
All existing tests pass across net8.0, net9.0, net10.0, and net472.