Repository navigation
Conversation
|
Additional verification context: the change is limited to preserving the existing 400 GitHub currently shows no completed check jobs or reviews for this commit, so the evidence above refers to the recorded local verification only.
|
0923355 to
30aa219
Compare
|
Rebased onto current
The rebase was clean, and the two changes do not overlap: #1837 edited Force-pushed to the PR branch ( Reproduction note: build/test locally with
|
When the POST body cannot be parsed as a JSON-RPC message, the 400 InvalidRequest response was a fixed one-liner that discarded the JsonException's reason and position. A body truncated by an intermediary (proxy, gateway, or transport) is therefore indistinguishable from any other malformed body without server-side logs (modelcontextprotocol#1842). The catch now includes the parser message and line/byte position in the error message, so the failure is diagnosable from the response alone. The message prefix and the conformant id=null envelope are unchanged.
30aa219 to
911a4dc
Compare
| // with a null id rather than letting the exception bubble up as an opaque 500. The parser's | ||
| // position detail is included so a truncated or corrupted body (e.g. one mangled by an | ||
| // intermediary) is diagnosable from the response alone (#1842). | ||
| var position = ex.BytePositionInLine is null | ||
| ? $"line {ex.LineNumber?.ToString() ?? "unknown"}" | ||
| : $"line {ex.LineNumber?.ToString() ?? "unknown"}, byte position {ex.BytePositionInLine}"; | ||
| await WriteJsonRpcErrorAsync(context, | ||
| "Bad Request: The POST body did not contain a valid JSON-RPC message.", | ||
| $"Bad Request: The POST body did not contain a valid JSON-RPC message: {ex.Message} ({position}).", |
There was a problem hiding this comment.
MVC already exposes JsonException.Message by default, so I'm comfortable doing that for this SDK-controlled parsing path. Can we drop the extra position suffix though? System.Text.Json already includes position information in syntax-error messages.
| // with a null id rather than letting the exception bubble up as an opaque 500. The parser's | |
| // position detail is included so a truncated or corrupted body (e.g. one mangled by an | |
| // intermediary) is diagnosable from the response alone (#1842). | |
| var position = ex.BytePositionInLine is null | |
| ? $"line {ex.LineNumber?.ToString() ?? "unknown"}" | |
| : $"line {ex.LineNumber?.ToString() ?? "unknown"}, byte position {ex.BytePositionInLine}"; | |
| await WriteJsonRpcErrorAsync(context, | |
| "Bad Request: The POST body did not contain a valid JSON-RPC message.", | |
| $"Bad Request: The POST body did not contain a valid JSON-RPC message: {ex.Message} ({position}).", | |
| // with a null id rather than letting the exception bubble up as an opaque 500. | |
| await WriteJsonRpcErrorAsync(context, | |
| $"Bad Request: The POST body did not contain a valid JSON-RPC message: {ex.Message}", |
There was a problem hiding this comment.
Done in 9dc2eb4: the message is now ...valid JSON-RPC message: {ex.Message} with no position suffix.
|
|
||
| var message = doc.RootElement.GetProperty("error").GetProperty("message").GetString(); | ||
| Assert.Contains("did not contain a valid JSON-RPC message", message); | ||
| Assert.Contains("line 0, byte position", message); |
There was a problem hiding this comment.
Rather than looking for "line 0, byte position" in the response, which I've recommended we remove, I think we should do something like the following to verify the response contains the JsonException message:
using var stream = new MemoryStream(Encoding.UTF8.GetBytes(body));
var exception = await Assert.ThrowsAsync<JsonException>(async () =>
{
await JsonSerializer.DeserializeAsync(
stream,
McpJsonUtilities.DefaultOptions.GetTypeInfo<JsonRpcMessage>(),
TestContext.Current.CancellationToken);
});
Assert.Contains(exception.Message, message);There was a problem hiding this comment.
Done in 9dc2eb4: both tests now deserialize the same body with McpJsonUtilities.DefaultOptions.GetTypeInfo<JsonRpcMessage>() and assert the response contains that JsonException.Message (small shared helper). StreamableHttpServerConformanceTests on net10.0: 56 passed.
System.Text.Json already includes the line and byte position in its syntax-error messages, so surface JsonException.Message as-is. The tests now compare against the JsonException produced by deserializing the same body instead of looking for a hard-coded position string.
This branch has not been deployed
3 participants
Summary
Surface JSON parse detail in streamable-HTTP 400 responses: when the POST body is not a valid JSON-RPC message, the JSON-RPC error message now includes
JsonException.Message(which already carries System.Text.Json's line/byte position), so a truncated or corrupted body is diagnosable from the response alone.Verification
dotnet test tests/ModelContextProtocol.AspNetCore.Tests -f net10.0 --filter 'FullyQualifiedName~StreamableHttpServerConformanceTests'(56 passed at 9dc2eb4).PostMalformedJson_Returns400_InvalidRequest_WithNullIdand the newPostTruncatedJson_Returns400_WithParserDetailtheory assert the response contains theJsonException.Messageproduced by deserializing the same body asJsonRpcMessage.Related to #1842