From 762999c1a72720aad7f899ba99893cb3a163afc7 Mon Sep 17 00:00:00 2001 From: Widthdom Date: Sat, 6 Jun 2026 21:51:29 +0900 Subject: [PATCH 1/2] Fix MCP JsonNode cloning for #3055 --- changelog.d/unreleased/3055.fixed.md | 18 +++++++++ src/CodeIndex/Mcp/McpErrorEnvelope.cs | 2 +- src/CodeIndex/Mcp/McpJsonNode.cs | 9 +++++ src/CodeIndex/Mcp/McpServer.cs | 16 ++++---- tests/CodeIndex.Tests/McpServerTests.cs | 52 +++++++++++++++++++++++++ 5 files changed, 88 insertions(+), 9 deletions(-) create mode 100644 changelog.d/unreleased/3055.fixed.md create mode 100644 src/CodeIndex/Mcp/McpJsonNode.cs diff --git a/changelog.d/unreleased/3055.fixed.md b/changelog.d/unreleased/3055.fixed.md new file mode 100644 index 0000000000..fd9d4c9b9a --- /dev/null +++ b/changelog.d/unreleased/3055.fixed.md @@ -0,0 +1,18 @@ +--- +category: fixed +issues: + - 3055 +affected: + - src/CodeIndex/Mcp/McpJsonNode.cs + - src/CodeIndex/Mcp/McpErrorEnvelope.cs + - src/CodeIndex/Mcp/McpServer.cs + - tests/CodeIndex.Tests/McpServerTests.cs +--- + +## English + +- **MCP JSON node cloning no longer serializes and reparses valid nodes (#3055)** — MCP response ids, session capabilities, progress tokens, and structured error extra data now use `JsonNode.DeepClone()` through a shared helper instead of `ToJsonString()` plus `JsonNode.Parse(...)`. + +## 日本語 + +- **MCP の JSON node clone が有効な node を文字列化して再 parse しなくなりました (#3055)** — MCP response id、session capabilities、progress token、structured error の追加データは、`ToJsonString()` と `JsonNode.Parse(...)` の組み合わせではなく、共有 helper 経由の `JsonNode.DeepClone()` を使うようになりました。 diff --git a/src/CodeIndex/Mcp/McpErrorEnvelope.cs b/src/CodeIndex/Mcp/McpErrorEnvelope.cs index c800a63f79..5366dcd657 100644 --- a/src/CodeIndex/Mcp/McpErrorEnvelope.cs +++ b/src/CodeIndex/Mcp/McpErrorEnvelope.cs @@ -76,7 +76,7 @@ public static JsonObject BuildData(string category, string suggestion, bool retr // canonical キーは extra でも上書きさせない。 if (kvp.Key is "category" or "suggestion" or "retry_safe") continue; - data[kvp.Key] = kvp.Value is null ? null : JsonNode.Parse(kvp.Value.ToJsonString()); + data[kvp.Key] = McpJsonNode.Clone(kvp.Value); } } return data; diff --git a/src/CodeIndex/Mcp/McpJsonNode.cs b/src/CodeIndex/Mcp/McpJsonNode.cs new file mode 100644 index 0000000000..42954af93a --- /dev/null +++ b/src/CodeIndex/Mcp/McpJsonNode.cs @@ -0,0 +1,9 @@ +using System.Text.Json.Nodes; + +namespace CodeIndex.Mcp; + +internal static class McpJsonNode +{ + public static JsonNode? Clone(JsonNode? node) + => node?.DeepClone(); +} diff --git a/src/CodeIndex/Mcp/McpServer.cs b/src/CodeIndex/Mcp/McpServer.cs index de96cb13b3..aa67809cc9 100644 --- a/src/CodeIndex/Mcp/McpServer.cs +++ b/src/CodeIndex/Mcp/McpServer.cs @@ -1153,7 +1153,7 @@ private static void ExtractResponseId(JsonNode request, out bool hasId, out Json if (request is JsonObject obj) { if (TryGetRequestId(obj, out hasId, out var requestId)) - id = requestId is null ? null : JsonNode.Parse(requestId.ToJsonString()); + id = McpJsonNode.Clone(requestId); else id = null; return; @@ -1834,7 +1834,7 @@ private void CaptureClientSession(JsonNode? initializeParams) if (!obj.TryGetPropertyValue("capabilities", out var capabilities)) obj.TryGetPropertyValue("clientCapabilities", out capabilities); if (capabilities is not null) - _clientCapabilities = JsonNode.Parse(capabilities.ToJsonString()); + _clientCapabilities = McpJsonNode.Clone(capabilities); if (TryReadStringValue(obj["rootUri"]) is { Length: > 0 } rootUri) _clientRoots.Add(rootUri); @@ -1850,7 +1850,7 @@ private void CaptureClientSession(JsonNode? initializeParams) } } - internal JsonNode? ClientCapabilitiesForTests => _clientCapabilities is null ? null : JsonNode.Parse(_clientCapabilities.ToJsonString()); + internal JsonNode? ClientCapabilitiesForTests => McpJsonNode.Clone(_clientCapabilities); internal string[] ClientRootsForTests => _clientRoots .Select(root => root?.GetValue()) @@ -2299,7 +2299,7 @@ private static JsonObject CreateUnsupportedProtocolError(JsonNode? id, BoundedMc { ["jsonrpc"] = "2.0", ["error"] = error, - ["id"] = id is null ? JsonNode.Parse("null") : JsonNode.Parse(id.ToJsonString()) + ["id"] = McpJsonNode.Clone(id) }; return response; } @@ -2376,7 +2376,7 @@ internal static JsonObject CreateRateLimitedErrorResponse(JsonNode? id, string t { ["jsonrpc"] = "2.0", ["error"] = error, - ["id"] = id is null ? JsonNode.Parse("null") : JsonNode.Parse(id.ToJsonString()) + ["id"] = McpJsonNode.Clone(id) }; return response; } @@ -2661,7 +2661,7 @@ private void EmitToolInvocationTelemetry(string toolName, JsonNode? args, JsonNo return null; return TryMeasureJsonUtf8BytesWithinLimit(token, _jsonOptions, McpBoundedText.MaxProgressTokenJsonBytes, out _) - ? token.DeepClone() + ? McpJsonNode.Clone(token) : null; } @@ -3424,7 +3424,7 @@ private static JsonObject CreateSuccessResponse(bool hasId, JsonNode? id, JsonNo ["result"] = result }; if (hasId) - response["id"] = id is null ? JsonNode.Parse("null") : JsonNode.Parse(id.ToJsonString()); + response["id"] = McpJsonNode.Clone(id); return response; } @@ -3522,7 +3522,7 @@ private static JsonObject CreateErrorResponse(bool hasId, JsonNode? id, int code } }; if (hasId) - response["id"] = id is null ? JsonNode.Parse("null") : JsonNode.Parse(id.ToJsonString()); + response["id"] = McpJsonNode.Clone(id); return response; } diff --git a/tests/CodeIndex.Tests/McpServerTests.cs b/tests/CodeIndex.Tests/McpServerTests.cs index 226491f0fe..d549378a50 100644 --- a/tests/CodeIndex.Tests/McpServerTests.cs +++ b/tests/CodeIndex.Tests/McpServerTests.cs @@ -866,6 +866,35 @@ public void Initialize_CapturesClientCapabilitiesAndRootsForSessionStatus() Assert.Equal("info", session["log_level"]!.GetValue()); } + [Fact] + public void Initialize_CapturesClientCapabilitiesAsDetachedClone_Issue3055() + { + var capabilities = new JsonObject + { + ["experimental"] = new JsonObject + { + ["progress"] = true, + }, + }; + var request = new JsonObject + { + ["jsonrpc"] = "2.0", + ["id"] = 1, + ["method"] = "initialize", + ["params"] = new JsonObject + { + ["capabilities"] = capabilities, + }, + }; + + _server.HandleMessage(request); + capabilities["experimental"] = new JsonObject { ["progress"] = false }; + var copy = _server.ClientCapabilitiesForTests!; + copy["experimental"]!["progress"] = false; + + Assert.True(_server.ClientCapabilitiesForTests!["experimental"]!["progress"]!.GetValue()); + } + [Fact] public void Initialize_ClientInfo_TruncatesSessionStatusAndCallerIdentity_Issue3120() { @@ -12896,6 +12925,29 @@ private static void AssertEnvelope(JsonNode? data, string expectedCategory, bool Assert.False(string.IsNullOrWhiteSpace(suggestion)); } + [Fact] + public void ErrorEnvelope_ClonesExtraDataWithoutSerializeParseRoundTrip_Issue3055() + { + var extra = new JsonObject + { + ["details"] = new JsonObject + { + ["safe"] = true, + }, + ["category"] = "override", + }; + + var data = McpErrorEnvelope.BuildData( + McpErrorEnvelope.CategoryInvalidArgument, + "Fix the request.", + retrySafe: false, + extra); + extra["details"]!["safe"] = false; + + Assert.Equal(McpErrorEnvelope.CategoryInvalidArgument, data["category"]!.GetValue()); + Assert.True(data["details"]!["safe"]!.GetValue()); + } + [Fact] public void ErrorResponse_InvalidRequest_NotAnObject_CarriesEnvelope() { From e3d648aa46dd4955fa3d8871fe89f58ab27797d4 Mon Sep 17 00:00:00 2001 From: Widthdom Date: Sat, 6 Jun 2026 21:59:49 +0900 Subject: [PATCH 2/2] Cap MCP client response retention for #3098 --- changelog.d/unreleased/3098.security.md | 16 +++ src/CodeIndex/Mcp/McpServer.cs | 34 +++++- tests/CodeIndex.Tests/McpServerTests.cs | 151 ++++++++++++++++++++++++ 3 files changed, 199 insertions(+), 2 deletions(-) create mode 100644 changelog.d/unreleased/3098.security.md diff --git a/changelog.d/unreleased/3098.security.md b/changelog.d/unreleased/3098.security.md new file mode 100644 index 0000000000..48bbc9d97b --- /dev/null +++ b/changelog.d/unreleased/3098.security.md @@ -0,0 +1,16 @@ +--- +category: security +issues: + - 3098 +affected: + - src/CodeIndex/Mcp/McpServer.cs + - tests/CodeIndex.Tests/McpServerTests.cs +--- + +## English + +- **MCP out-of-band client responses are capped before retention (#3098)** — `sampling/createMessage` and other awaited client responses now measure `result` and `error` JSON against the MCP response byte limit before cloning or storing the payload. + +## 日本語 + +- **MCP の out-of-band client response を保持前に上限チェックするようになりました (#3098)** — `sampling/createMessage` など待機対象の client response は、payload を clone / 保持する前に `result` と `error` JSON を MCP response byte limit で測定するようになりました。 diff --git a/src/CodeIndex/Mcp/McpServer.cs b/src/CodeIndex/Mcp/McpServer.cs index aa67809cc9..092710f5aa 100644 --- a/src/CodeIndex/Mcp/McpServer.cs +++ b/src/CodeIndex/Mcp/McpServer.cs @@ -978,12 +978,42 @@ private bool TryCompletePendingClientRequest(JsonNode request) return false; if (obj.TryGetPropertyValue("error", out var error) && error is not null) - pending.TrySetException(new InvalidOperationException(error.ToJsonString(_jsonOptions))); + { + if (!TryCloneClientResponseNodeWithinLimit(error, "error", out var clonedError, out var exception)) + pending.TrySetException(exception); + else + pending.TrySetException(new InvalidOperationException(clonedError!.ToJsonString(_jsonOptions))); + } + else if (obj.TryGetPropertyValue("result", out var result) && result is not null) + { + if (!TryCloneClientResponseNodeWithinLimit(result, "result", out var clonedResult, out var exception)) + pending.TrySetException(exception); + else + pending.TrySetResult(clonedResult); + } else - pending.TrySetResult(obj["result"]?.DeepClone()); + { + pending.TrySetResult(null); + } return true; } + private bool TryCloneClientResponseNodeWithinLimit(JsonNode node, string memberName, out JsonNode? clone, out InvalidOperationException exception) + { + var responseLimit = GetMaxResponseBytes(); + if (TryMeasureJsonUtf8BytesWithinLimit(node, _jsonOptions, responseLimit, out var responseBytes)) + { + clone = McpJsonNode.Clone(node); + exception = null!; + return true; + } + + clone = null; + exception = new InvalidOperationException( + $"MCP client response {memberName} exceeded the retained byte limit ({responseBytes} > {responseLimit})."); + return false; + } + private async Task SendClientRequestAsync(string method, JsonObject? @params, CancellationToken cancellationToken) { if (ClientRequestHandlerForTests is { } handler) diff --git a/tests/CodeIndex.Tests/McpServerTests.cs b/tests/CodeIndex.Tests/McpServerTests.cs index d549378a50..b708e01d24 100644 --- a/tests/CodeIndex.Tests/McpServerTests.cs +++ b/tests/CodeIndex.Tests/McpServerTests.cs @@ -11932,6 +11932,83 @@ public void SuggestImprovement_WhenSamplingResponseJsonIsTooDeep_IgnoresSampledM Assert.Null(stored.SampledTags); } + [Fact] + public async Task RunAsync_SamplingClientResultOverRetainedLimit_IgnoresOversizedPayload_Issue3098() + { + using var env = EnvironmentVariableScope.Capture("CDIDX_MCP_RESPONSE_MAX_BYTES"); + env.Set("CDIDX_MCP_RESPONSE_MAX_BYTES", "4096"); + using var server = new McpServer(_dbPath, "1.0", dbPathExplicit: true); + server.HandleMessage(JsonNode.Parse( + """{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"capabilities":{"sampling":{}}}}""")!); + var uniqueDesc = $"Oversized out-of-band result regression {Guid.NewGuid():N}"; + var request = BuildSuggestImprovementRequest(uniqueDesc); + var transport = new ClientResponseTransport(request.ToJsonString(), id => new JsonObject + { + ["jsonrpc"] = "2.0", + ["id"] = id, + ["result"] = new JsonObject + { + ["content"] = new JsonObject + { + ["type"] = "text", + ["text"] = $$"""{"title":"Should not be retained","tags":["security"],"padding":"{{new string('r', 5000)}}"}""", + }, + }, + }); + + await server.RunAsync(transport, CancellationToken.None); + + var response = JsonNode.Parse(transport.FinalResponse!)!; + var structured = response["result"]!["structuredContent"]!; + Assert.Equal("recorded", structured["status"]!.GetValue()); + Assert.Null(structured["sampled_title"]); + Assert.Contains(transport.WrittenFrames, frame => + frame.Contains("\"method\":\"sampling/createMessage\"", StringComparison.Ordinal)); + var stored = new SuggestionStore(Path.GetDirectoryName(_dbPath)!, Path.GetFileNameWithoutExtension(_dbPath)).LoadAll() + .Single(s => s.Description == uniqueDesc); + Assert.Null(stored.SampledTitle); + Assert.Null(stored.SampledTags); + } + + [Fact] + public async Task RunAsync_SamplingClientErrorOverRetainedLimit_IgnoresOversizedPayload_Issue3098() + { + using var env = EnvironmentVariableScope.Capture("CDIDX_MCP_RESPONSE_MAX_BYTES"); + env.Set("CDIDX_MCP_RESPONSE_MAX_BYTES", "4096"); + using var server = new McpServer(_dbPath, "1.0", dbPathExplicit: true); + server.HandleMessage(JsonNode.Parse( + """{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"capabilities":{"sampling":{}}}}""")!); + var uniqueDesc = $"Oversized out-of-band error regression {Guid.NewGuid():N}"; + var request = BuildSuggestImprovementRequest(uniqueDesc); + var transport = new ClientResponseTransport(request.ToJsonString(), id => new JsonObject + { + ["jsonrpc"] = "2.0", + ["id"] = id, + ["error"] = new JsonObject + { + ["code"] = -32000, + ["message"] = "client rejected sampling", + ["data"] = new JsonObject + { + ["padding"] = new string('e', 5000), + }, + }, + }); + + await server.RunAsync(transport, CancellationToken.None); + + var response = JsonNode.Parse(transport.FinalResponse!)!; + var structured = response["result"]!["structuredContent"]!; + Assert.Equal("recorded", structured["status"]!.GetValue()); + Assert.Null(structured["sampled_title"]); + Assert.DoesNotContain(transport.WrittenFrames, frame => + frame.Contains(new string('e', 5000), StringComparison.Ordinal)); + var stored = new SuggestionStore(Path.GetDirectoryName(_dbPath)!, Path.GetFileNameWithoutExtension(_dbPath)).LoadAll() + .Single(s => s.Description == uniqueDesc); + Assert.Null(stored.SampledTitle); + Assert.Null(stored.SampledTags); + } + [Fact] public void SuggestImprovement_WhenSamplingAvailable_BoundsPromptAndSummarizesInvocationContext() { @@ -13510,6 +13587,63 @@ public Task WriteOutOfBandFrameAsync(string frame, CancellationToken cancellatio public ValueTask DisposeAsync() => ValueTask.CompletedTask; } + private sealed class ClientResponseTransport : IMcpTransport + { + private readonly Queue _frames = new(); + private readonly SemaphoreSlim _availableFrames = new(0, 1); + private readonly Func _responseFactory; + + public ClientResponseTransport(string request, Func responseFactory) + { + _responseFactory = responseFactory; + EnqueueFrame(request); + } + + public string Name => "stdio"; + public string Endpoint => "memory://test"; + public List WrittenFrames { get; } = []; + public string? FinalResponse { get; private set; } + + public async Task ReadFrameAsync(CancellationToken cancellationToken) + { + await _availableFrames.WaitAsync(cancellationToken).ConfigureAwait(false); + lock (_frames) + { + return _frames.Dequeue(); + } + } + + public Task WriteFrameAsync(string? frame, CancellationToken cancellationToken) + { + if (frame is null) + return Task.CompletedTask; + + WrittenFrames.Add(frame); + var node = JsonNode.Parse(frame)!; + if (node["method"] is not null && node["id"] is JsonValue idNode && idNode.TryGetValue(out var id)) + { + EnqueueFrame(_responseFactory(id).ToJsonString()); + } + else if (node["method"] is null) + { + FinalResponse = frame; + EnqueueFrame(null); + } + return Task.CompletedTask; + } + + public ValueTask DisposeAsync() => ValueTask.CompletedTask; + + private void EnqueueFrame(string? frame) + { + lock (_frames) + { + _frames.Enqueue(frame); + } + _availableFrames.Release(); + } + } + // The shutdown helper is the heart of the #1573 fix: cancelling the CTS through Console.CancelKeyPress // (and PosixSignal.SIGTERM on Unix) must trip the loop. This test exercises the cross-platform // Ctrl+C path by raising the .NET CancelKeyPress event directly via reflection — the test cannot @@ -13549,6 +13683,23 @@ public void RegisterShutdownHandlers_AfterDispose_DoesNotInvokeHandler() private static JsonObject BuildRequiredPathArguments(string toolName, string pathValue) => BuildRequiredPathArguments(toolName, JsonValue.Create(pathValue)!); + private static JsonObject BuildSuggestImprovementRequest(string description) + => new() + { + ["jsonrpc"] = "2.0", + ["id"] = 1, + ["method"] = "tools/call", + ["params"] = new JsonObject + { + ["name"] = "suggest_improvement", + ["arguments"] = new JsonObject + { + ["category"] = "other", + ["description"] = description, + }, + }, + }; + private static JsonObject BuildRequiredPathArguments(string toolName, JsonNode pathValue) { var arguments = new JsonObject