Skip to content

Commit 688a956

Browse files
Copilotstephentoub
Copilot
and
stephentoub
committed
Serialize Exception.Data upfront to prevent stream corruption
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
1 parent adcffc6 commit 688a956

3 files changed

Lines changed: 144 additions & 108 deletions

File tree

‎src/ModelContextProtocol.Core/McpSessionHandler.cs‎

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -207,17 +207,7 @@ ex is OperationCanceledException &&
207207
Context = new JsonRpcMessageContext { RelatedTransport = request.Context?.RelatedTransport },
208208
};
209209

210-
try
211-
{
212-
await SendMessageAsync(errorMessage, cancellationToken).ConfigureAwait(false);
213-
}
214-
catch (Exception sendException) when (sendException is JsonException or NotSupportedException && detail.Data is not null)
215-
{
216-
// If serialization fails (e.g., non-serializable data in Exception.Data),
217-
// retry without the data to ensure the client receives an error response.
218-
detail.Data = null;
219-
await SendMessageAsync(errorMessage, cancellationToken).ConfigureAwait(false);
220-
}
210+
await SendMessageAsync(errorMessage, cancellationToken).ConfigureAwait(false);
221211
}
222212
else if (ex is not OperationCanceledException)
223213
{
@@ -782,28 +772,50 @@ private static TimeSpan GetElapsed(long startingTimestamp) =>
782772

783773
/// <summary>
784774
/// Converts the <see cref="Exception.Data"/> dictionary to a serializable <see cref="Dictionary{TKey, TValue}"/>.
785-
/// Returns null if the data dictionary is empty or contains no string keys.
775+
/// Returns null if the data dictionary is empty or contains no string keys with serializable values.
786776
/// </summary>
787777
/// <remarks>
778+
/// <para>
788779
/// Only entries with string keys are included in the result. Entries with non-string keys are ignored.
780+
/// </para>
781+
/// <para>
782+
/// Each value is serialized to a <see cref="JsonElement"/> to ensure it can be safely included in the
783+
/// JSON-RPC error response. Values that cannot be serialized are silently skipped.
784+
/// </para>
789785
/// </remarks>
790-
private static Dictionary<string, object?>? ConvertExceptionData(System.Collections.IDictionary data)
786+
private static Dictionary<string, JsonElement>? ConvertExceptionData(System.Collections.IDictionary data)
791787
{
792788
if (data.Count == 0)
793789
{
794790
return null;
795791
}
796792

797-
var result = new Dictionary<string, object?>(data.Count);
793+
var typeInfo = McpJsonUtilities.DefaultOptions.GetTypeInfo<object?>();
794+
795+
Dictionary<string, JsonElement>? result = null;
798796
foreach (System.Collections.DictionaryEntry entry in data)
799797
{
800798
if (entry.Key is string key)
801799
{
802-
result[key] = entry.Value;
800+
try
801+
{
802+
// Serialize each value upfront to catch any serialization issues
803+
// before attempting to send the message. If the value is already a
804+
// JsonElement, use it directly.
805+
var element = entry.Value is JsonElement je
806+
? je
807+
: JsonSerializer.SerializeToElement(entry.Value, typeInfo);
808+
result ??= new(data.Count);
809+
result[key] = element;
810+
}
811+
catch (Exception ex) when (ex is JsonException or NotSupportedException)
812+
{
813+
// Skip non-serializable values silently
814+
}
803815
}
804816
}
805817

806-
return result.Count > 0 ? result : null;
818+
return result?.Count > 0 ? result : null;
807819
}
808820

809821
[LoggerMessage(Level = LogLevel.Information, Message = "{EndpointName} message processing canceled.")]

‎tests/ModelContextProtocol.Tests/McpProtocolExceptionDataTests.cs‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
using Microsoft.Extensions.DependencyInjection;
2+
using ModelContextProtocol.Client;
3+
using ModelContextProtocol.Protocol;
4+
using ModelContextProtocol.Server;
5+
using ModelContextProtocol.Tests.Utils;
6+
7+
namespace ModelContextProtocol.Tests;
8+
9+
/// <summary>
10+
/// Tests for McpProtocolException.Data propagation to JSON-RPC error responses.
11+
/// </summary>
12+
public class McpProtocolExceptionDataTests : ClientServerTestBase
13+
{
14+
public McpProtocolExceptionDataTests(ITestOutputHelper testOutputHelper)
15+
: base(testOutputHelper)
16+
{
17+
}
18+
19+
protected override void ConfigureServices(ServiceCollection services, IMcpServerBuilder mcpServerBuilder)
20+
{
21+
mcpServerBuilder.WithCallToolHandler((request, cancellationToken) =>
22+
{
23+
var toolName = request.Params?.Name;
24+
25+
switch (toolName)
26+
{
27+
case "throw_with_serializable_data":
28+
throw new McpProtocolException("Resource not found", (McpErrorCode)(-32002))
29+
{
30+
Data =
31+
{
32+
{ "uri", "file:///path/to/resource" },
33+
{ "code", 404 }
34+
}
35+
};
36+
37+
case "throw_with_nonserializable_data":
38+
throw new McpProtocolException("Resource not found", (McpErrorCode)(-32002))
39+
{
40+
Data =
41+
{
42+
// Circular reference - cannot be serialized
43+
{ "nonSerializable", new NonSerializableObject() },
44+
// This one should still be included
45+
{ "uri", "file:///path/to/resource" }
46+
}
47+
};
48+
49+
case "throw_with_only_nonserializable_data":
50+
throw new McpProtocolException("Resource not found", (McpErrorCode)(-32002))
51+
{
52+
Data =
53+
{
54+
// Only non-serializable data - should result in null data
55+
{ "nonSerializable", new NonSerializableObject() }
56+
}
57+
};
58+
59+
default:
60+
throw new McpProtocolException($"Unknown tool: '{toolName}'", McpErrorCode.InvalidParams);
61+
}
62+
});
63+
}
64+
65+
[Fact]
66+
public async Task Exception_With_Serializable_Data_Propagates_To_Client()
67+
{
68+
await using McpClient client = await CreateMcpClientForServer();
69+
70+
var exception = await Assert.ThrowsAsync<McpProtocolException>(async () =>
71+
await client.CallToolAsync("throw_with_serializable_data", cancellationToken: TestContext.Current.CancellationToken));
72+
73+
Assert.Equal("Request failed (remote): Resource not found", exception.Message);
74+
Assert.Equal((McpErrorCode)(-32002), exception.ErrorCode);
75+
}
76+
77+
[Fact]
78+
public async Task Exception_With_NonSerializable_Data_Still_Propagates_Error_To_Client()
79+
{
80+
await using McpClient client = await CreateMcpClientForServer();
81+
82+
// The tool throws McpProtocolException with non-serializable data in Exception.Data.
83+
// The server should still send a proper error response to the client, with non-serializable
84+
// values filtered out.
85+
var exception = await Assert.ThrowsAsync<McpProtocolException>(async () =>
86+
await client.CallToolAsync("throw_with_nonserializable_data", cancellationToken: TestContext.Current.CancellationToken));
87+
88+
Assert.Equal("Request failed (remote): Resource not found", exception.Message);
89+
Assert.Equal((McpErrorCode)(-32002), exception.ErrorCode);
90+
}
91+
92+
[Fact]
93+
public async Task Exception_With_Only_NonSerializable_Data_Still_Propagates_Error_To_Client()
94+
{
95+
await using McpClient client = await CreateMcpClientForServer();
96+
97+
// When all data is non-serializable, the error should still be sent (with null data)
98+
var exception = await Assert.ThrowsAsync<McpProtocolException>(async () =>
99+
await client.CallToolAsync("throw_with_only_nonserializable_data", cancellationToken: TestContext.Current.CancellationToken));
100+
101+
Assert.Equal("Request failed (remote): Resource not found", exception.Message);
102+
Assert.Equal((McpErrorCode)(-32002), exception.ErrorCode);
103+
}
104+
105+
/// <summary>
106+
/// A class that cannot be serialized by System.Text.Json due to circular reference.
107+
/// </summary>
108+
private sealed class NonSerializableObject
109+
{
110+
public NonSerializableObject() => Self = this;
111+
public NonSerializableObject Self { get; set; }
112+
}
113+
}

‎tests/ModelContextProtocol.Tests/Server/McpServerTests.cs‎

Lines changed: 3 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
using System.Runtime.InteropServices;
77
using System.Text.Json;
88
using System.Text.Json.Nodes;
9-
using System.Threading.Channels;
109

1110
namespace ModelContextProtocol.Tests.Server;
1211

@@ -721,103 +720,15 @@ await transport.SendMessageAsync(
721720
Assert.Equal(ErrorMessage, error.Error.Message);
722721
Assert.NotNull(error.Error.Data);
723722

724-
// Verify the data contains the uri
725-
var dataDict = Assert.IsType<Dictionary<string, object?>>(error.Error.Data);
723+
// Verify the data contains the uri (values are now JsonElements after serialization)
724+
var dataDict = Assert.IsType<Dictionary<string, JsonElement>>(error.Error.Data);
726725
Assert.True(dataDict.ContainsKey("uri"));
727-
Assert.Equal(ResourceUri, dataDict["uri"]);
726+
Assert.Equal(ResourceUri, dataDict["uri"].GetString());
728727

729728
await transport.DisposeAsync();
730729
await runTask;
731730
}
732731

733-
[Fact]
734-
public async Task Can_Handle_Call_Tool_Requests_With_McpProtocolException_And_NonSerializableData()
735-
{
736-
const string ErrorMessage = "Resource not found";
737-
const McpErrorCode ErrorCode = (McpErrorCode)(-32002);
738-
739-
await using var transport = new SerializingTestServerTransport();
740-
var options = CreateOptions(new ServerCapabilities { Tools = new() });
741-
options.Handlers.CallToolHandler = async (request, ct) =>
742-
{
743-
throw new McpProtocolException(ErrorMessage, ErrorCode)
744-
{
745-
Data =
746-
{
747-
// Add a non-serializable object (an object with circular reference)
748-
{ "nonSerializable", new NonSerializableObject() }
749-
}
750-
};
751-
};
752-
options.Handlers.ListToolsHandler = (request, ct) => throw new NotImplementedException();
753-
754-
await using var server = McpServer.Create(transport, options, LoggerFactory);
755-
756-
var runTask = server.RunAsync(TestContext.Current.CancellationToken);
757-
758-
var receivedMessage = new TaskCompletionSource<JsonRpcError>();
759-
760-
transport.OnMessageSent = (message) =>
761-
{
762-
if (message is JsonRpcError error && error.Id.ToString() == "55")
763-
receivedMessage.SetResult(error);
764-
};
765-
766-
await transport.SendMessageAsync(
767-
new JsonRpcRequest
768-
{
769-
Method = RequestMethods.ToolsCall,
770-
Id = new RequestId(55)
771-
},
772-
TestContext.Current.CancellationToken
773-
);
774-
775-
// Client should still receive an error response, even though the data couldn't be serialized
776-
var error = await receivedMessage.Task.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken);
777-
Assert.NotNull(error);
778-
Assert.NotNull(error.Error);
779-
Assert.Equal((int)ErrorCode, error.Error.Code);
780-
Assert.Equal(ErrorMessage, error.Error.Message);
781-
// Data should be null since it couldn't be serialized
782-
Assert.Null(error.Error.Data);
783-
784-
await transport.DisposeAsync();
785-
await runTask;
786-
}
787-
788-
/// <summary>
789-
/// A class that cannot be serialized by System.Text.Json due to circular reference.
790-
/// </summary>
791-
private sealed class NonSerializableObject
792-
{
793-
public NonSerializableObject() => Self = this;
794-
public NonSerializableObject Self { get; set; }
795-
}
796-
797-
/// <summary>
798-
/// A test transport that simulates JSON serialization failure for non-serializable data.
799-
/// </summary>
800-
private sealed class SerializingTestServerTransport : ITransport
801-
{
802-
private readonly TestServerTransport _inner = new();
803-
804-
public bool IsConnected => _inner.IsConnected;
805-
public ChannelReader<JsonRpcMessage> MessageReader => _inner.MessageReader;
806-
public string? SessionId => _inner.SessionId;
807-
public Action<JsonRpcMessage>? OnMessageSent { get => _inner.OnMessageSent; set => _inner.OnMessageSent = value; }
808-
809-
public Task SendMessageAsync(JsonRpcMessage message, CancellationToken cancellationToken = default)
810-
{
811-
// Serialize the message to verify it can be serialized (this will throw JsonException if not)
812-
// We serialize synchronously before any async operations to ensure the exception propagates correctly
813-
_ = JsonSerializer.Serialize(message, McpJsonUtilities.DefaultOptions);
814-
815-
return _inner.SendMessageAsync(message, cancellationToken);
816-
}
817-
818-
public ValueTask DisposeAsync() => _inner.DisposeAsync();
819-
}
820-
821732
private async Task Can_Handle_Requests(ServerCapabilities? serverCapabilities, string method, Action<McpServerOptions>? configureOptions, Action<McpServer, JsonNode?> assertResult)
822733
{
823734
await using var transport = new TestServerTransport();

0 commit comments

Comments
 (0)