diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md index 65f601b680..5b9a9894c4 100644 --- a/DEVELOPER_GUIDE.md +++ b/DEVELOPER_GUIDE.md @@ -1907,7 +1907,7 @@ The following categories ride the standard JSON-RPC codes: | --- | --- | --- | --- | | `-32700` | `parse_error` | `false` | Frame was not valid JSON. | | `-32700` | `message_too_large` | `false` | Frame is above the per-frame byte cap and is rejected before parsing. The frame reader uses the parse-error code because the frame is unreadable in the same sense as malformed JSON. | -| `-32600` | `invalid_request` | `false` | Frame parsed but is not a JSON object, is missing required JSON-RPC fields, or carries an invalid `id`. | +| `-32600` | `invalid_request` | `false` | Frame parsed but is not a JSON object, is missing required JSON-RPC fields, or carries an invalid `id`. String and numeric request ids are capped before echo/audit serialization; oversized ids return `id:null` plus `data.max_request_id_chars` / `data.max_request_id_bytes`. | | `-32601` | `method_not_found` | `false` | Unknown JSON-RPC method (not one of `initialize` / `tools/list` / `tools/call` / `ping` / supported `notifications/*`). | | `-32601` | `tool_disabled` | `false` | Known MCP tool is disabled by `CDIDX_MCP_TOOLS_ALLOW` / `CDIDX_MCP_TOOLS_DENY` (#1561). The `-32601` wire code is preserved so the pre-#1581 client contract still holds; only the envelope is additive. `data.tool` carries the disabled tool name. | | `-32602` | `tool_unknown` | `false` | `tools/call` received an MCP tool name the server does not implement (typo or version mismatch). `data.tool` carries the unknown name. | @@ -3524,7 +3524,7 @@ JSON-RPC 2.0 は `-32700` と `-32600..-32603` を仕様自身、`-32000..-32099 | --- | --- | --- | --- | | `-32700` | `parse_error` | `false` | フレームが JSON として解析できなかった。 | | `-32700` | `message_too_large` | `false` | フレームがパース前にバイト上限を超えて拒否された。フレームリーダーは「読めない」という意味で parse-error と同じコードを使う。 | -| `-32600` | `invalid_request` | `false` | パースはできたが JSON オブジェクトでない、JSON-RPC 必須フィールド欠落、または不正な `id` を含む。 | +| `-32600` | `invalid_request` | `false` | パースはできたが JSON オブジェクトでない、JSON-RPC 必須フィールド欠落、または不正な `id` を含む。文字列と数値の request id は echo / audit serialization 前に上限を適用し、過大な id は `id:null` と `data.max_request_id_chars` / `data.max_request_id_bytes` 付きで返す。 | | `-32601` | `method_not_found` | `false` | 未知 JSON-RPC メソッド（`initialize` / `tools/list` / `tools/call` / `ping` / サポート対象 `notifications/*` 以外）。 | | `-32601` | `tool_disabled` | `false` | `CDIDX_MCP_TOOLS_ALLOW` / `CDIDX_MCP_TOOLS_DENY`（#1561）で無効化された既知ツール。ワイヤコードは #1581 以前のクライアント契約を保つため `-32601` のまま維持し、envelope のみ additive に追加する。`data.tool` に無効化されたツール名を含める。 | | `-32602` | `tool_unknown` | `false` | `tools/call` がサーバー未実装の MCP ツール名を指定した（typo またはバージョン不整合）。`data.tool` に未知の名前を含める。 | diff --git a/changelog.d/unreleased/3104.security.md b/changelog.d/unreleased/3104.security.md new file mode 100644 index 0000000000..c401cc6e1f --- /dev/null +++ b/changelog.d/unreleased/3104.security.md @@ -0,0 +1,18 @@ +--- +category: security +issues: + - 3104 +affected: + - DEVELOPER_GUIDE.md + - src/CodeIndex/Mcp/McpServer.cs + - tests/CodeIndex.Tests/McpServerTests.cs + - tests/CodeIndex.Tests/McpAuditLogTests.cs +--- + +## English + +- **MCP JSON-RPC request ids are now length-limited before echoing (#3104)** — oversized string or numeric `id` values are rejected with a bounded `id:null` invalid-request response before response metadata, telemetry, or audit records can retain them. + +## 日本語 + +- **MCP JSON-RPC request id を echo 前に長さ制限するようになりました (#3104)** — 過大な文字列または数値の `id` は、レスポンスメタデータ、telemetry、audit record に保持される前に、境界付きの `id:null` invalid-request response として拒否されます。 diff --git a/src/CodeIndex/Mcp/McpServer.cs b/src/CodeIndex/Mcp/McpServer.cs index 7e2580d51a..7e0af6a084 100644 --- a/src/CodeIndex/Mcp/McpServer.cs +++ b/src/CodeIndex/Mcp/McpServer.cs @@ -174,6 +174,8 @@ public partial class McpServer : IDisposable private const string SamplingEnabledEnvironmentVariable = "CDIDX_MCP_SAMPLING"; internal const int MaxJsonDepth = 32; internal const int MaxBatchRequestCount = 100; + internal const int MaxRequestIdCharacterCount = 128; + internal const int MaxRequestIdByteLength = 256; // Stdio buffer for the JSON-RPC loop. Sized to fit typical large MCP payloads (e.g. batch_query) // in a single read so the StreamReader does not grow from its 1 KB default toward MaxLineCharacterCount. // JSON-RPCループのstdioバッファ。大きめのMCPペイロードを1回の読み取りで吸収し、 @@ -968,7 +970,10 @@ private bool TryCompletePendingClientRequest(JsonNode request) || obj["method"] is not null) return false; - var key = id?.ToJsonString(_jsonOptions) ?? "null"; + if (!TrySerializeRequestId(id, out var serializedId, out _)) + return false; + + var key = serializedId ?? "null"; if (!_pendingClientRequests.TryRemove(key, out var pending)) return false; @@ -1223,11 +1228,12 @@ private static void AppendMinimalCorrelationData(StringBuilder builder) // で例外を投げると、認証ゲート前に -32603 が返ってしまい、未認証呼び出し元に dispatch // 内部まで届いた事実が漏れる (#1559)。 var method = TryGetStringMember(obj, "method"); - if (!TryGetRequestId(obj, out var hasId, out var id)) - return CreateErrorResponse(hasId: true, id: null, code: -32600, message: "Invalid request: id must be string, number, or null", + if (!TryGetRequestId(obj, out var hasId, out var id, out var idError)) + return CreateErrorResponse(hasId: true, id: null, code: -32600, message: BuildInvalidRequestIdMessage(idError), category: McpErrorEnvelope.CategoryInvalidRequest, - suggestion: "JSON-RPC 2.0 `id` must be a string, integer, or null. Booleans/objects/arrays are not allowed.", - retrySafe: false); + suggestion: BuildInvalidRequestIdSuggestion(idError), + retrySafe: false, + extraData: BuildInvalidRequestIdData(idError)); using var correlationScope = hasId && CurrentCorrelationContext.Value is null ? BeginRequestCorrelation(id) : null; @@ -2841,16 +2847,7 @@ private static void AddArgKeyMetadata(JsonObject target, IReadOnlyList TryGetRequestId(request, out hasId, out id, out _); + + private static bool TryGetRequestId(JsonObject request, out bool hasId, out JsonNode? id, out RequestIdValidationError error) { + error = RequestIdValidationError.None; hasId = request.TryGetPropertyValue("id", out id); if (!hasId) return true; @@ -3101,19 +3109,122 @@ private static bool TryGetRequestId(JsonObject request, out bool hasId, out Json if (id is null) return true; - if (id is JsonValue) + return TrySerializeRequestId(id, out _, out error); + } + + private static bool TrySerializeRequestId(JsonNode? id, out string? serialized, out RequestIdValidationError error) + { + serialized = null; + error = RequestIdValidationError.None; + if (id is null) + return true; + + if (id is not JsonValue value) { - var serialized = id.ToJsonString(); - if (serialized.Length == 0) - return false; + error = RequestIdValidationError.InvalidType; + return false; + } - var first = serialized[0]; - return first == '"' || first == '-' || char.IsDigit(first) || first == 'n'; + return TrySerializeRequestIdValue(value, out serialized, out error); + } + + private static bool TrySerializeRequestIdValue(JsonValue value, out string? serialized, out RequestIdValidationError error) + { + serialized = null; + error = RequestIdValidationError.None; + JsonValueKind kind; + try + { + kind = value.GetValueKind(); + } + catch + { + error = RequestIdValidationError.InvalidType; + return false; } - return false; + switch (kind) + { + case JsonValueKind.String: + try + { + var requestId = value.GetValue(); + if (!IsRequestIdWithinBounds(requestId)) + { + error = RequestIdValidationError.TooLong; + return false; + } + + serialized = JsonSerializer.Serialize(requestId); + return true; + } + catch + { + error = RequestIdValidationError.InvalidType; + return false; + } + + case JsonValueKind.Number: + try + { + serialized = value.TryGetValue(out var element) && element.ValueKind == JsonValueKind.Number + ? element.GetRawText() + : value.ToJsonString(); + } + catch + { + error = RequestIdValidationError.InvalidType; + return false; + } + + if (serialized.Length == 0 || !(serialized[0] == '-' || char.IsDigit(serialized[0]))) + { + error = RequestIdValidationError.InvalidType; + serialized = null; + return false; + } + + if (!IsRequestIdWithinBounds(serialized)) + { + error = RequestIdValidationError.TooLong; + serialized = null; + return false; + } + + return true; + + case JsonValueKind.Null: + return true; + + default: + error = RequestIdValidationError.InvalidType; + return false; + } } + private static bool IsRequestIdWithinBounds(string value) + => value.Length <= MaxRequestIdCharacterCount + && Encoding.UTF8.GetByteCount(value) <= MaxRequestIdByteLength; + + private static string BuildInvalidRequestIdMessage(RequestIdValidationError error) + => error == RequestIdValidationError.TooLong + ? "Invalid request: id exceeds the request-id length limit" + : "Invalid request: id must be string, number, or null"; + + private static string BuildInvalidRequestIdSuggestion(RequestIdValidationError error) + => error == RequestIdValidationError.TooLong + ? $"JSON-RPC 2.0 `id` must be no more than {MaxRequestIdCharacterCount} characters and {MaxRequestIdByteLength} UTF-8 bytes. Use a compact string or number id." + : "JSON-RPC 2.0 `id` must be a string, integer, or null. Booleans/objects/arrays are not allowed."; + + private static JsonObject? BuildInvalidRequestIdData(RequestIdValidationError error) + => error == RequestIdValidationError.TooLong + ? new JsonObject + { + ["max_request_id_chars"] = MaxRequestIdCharacterCount, + ["max_request_id_bytes"] = MaxRequestIdByteLength, + } + : null; + private static JsonObject CreateSuccessResponse(JsonNode? id, JsonNode result) => CreateSuccessResponse(id is not null, id, result); diff --git a/tests/CodeIndex.Tests/McpAuditLogTests.cs b/tests/CodeIndex.Tests/McpAuditLogTests.cs index b410989cda..3a2664897e 100644 --- a/tests/CodeIndex.Tests/McpAuditLogTests.cs +++ b/tests/CodeIndex.Tests/McpAuditLogTests.cs @@ -124,6 +124,37 @@ public void ToolsCall_MissingToolName_StillEmitsAuditRecord() Assert.Equal("missing_tool_name", record.GetProperty("error").GetString()); } + [Fact] + public void ToolsCall_OversizedRequestId_IsRejectedBeforeAuditRecord_Issue3104() + { + using var sink = new AuditLogSink(_auditPath, AuditLogSink.DefaultMaxBytes, includeValues: false); + using var server = CreateServer(sink); + var oversizedId = new string('r', McpServer.MaxRequestIdCharacterCount + 1); + var request = new JsonObject + { + ["jsonrpc"] = "2.0", + ["id"] = oversizedId, + ["method"] = "tools/call", + ["params"] = new JsonObject + { + ["name"] = "ping", + ["arguments"] = new JsonObject(), + }, + }; + + var response = server.HandleMessage(request)!; + + Assert.Equal(-32600, response["error"]!["code"]!.GetValue()); + Assert.Equal("invalid_request", response["error"]!["data"]!["category"]!.GetValue()); + AssertJsonNullId(response); + if (File.Exists(_auditPath)) + { + var rawLog = File.ReadAllText(_auditPath); + Assert.DoesNotContain(oversizedId, rawLog, StringComparison.Ordinal); + Assert.True(string.IsNullOrWhiteSpace(rawLog), "oversized request ids must be rejected before tool audit emission"); + } + } + [Fact] public void ToolsCall_DisabledTool_EmitsAuditRecordWithToolDisabledError() { @@ -384,4 +415,10 @@ private JsonElement ReadOnlyRecord() Assert.Single(lines); return JsonDocument.Parse(lines[0]).RootElement.Clone(); } + + private static void AssertJsonNullId(JsonNode response) + { + Assert.True(response.AsObject().ContainsKey("id"), "JSON-RPC error responses must include id:null"); + Assert.Null(response["id"]); + } } diff --git a/tests/CodeIndex.Tests/McpServerTests.cs b/tests/CodeIndex.Tests/McpServerTests.cs index e3dcad5db1..2069825a05 100644 --- a/tests/CodeIndex.Tests/McpServerTests.cs +++ b/tests/CodeIndex.Tests/McpServerTests.cs @@ -12252,6 +12252,40 @@ public void ProcessFrame_OversizedUtf8Bytes_ReturnsParseErrorWithNullId() AssertJsonNullId(response); } + [Fact] + public void ProcessFrame_OversizedStringRequestId_ReturnsInvalidRequestWithNullId_Issue3104() + { + var oversizedId = new string('x', McpServer.MaxRequestIdCharacterCount + 1); + var raw = _server.ProcessFrame("{\"jsonrpc\":\"2.0\",\"id\":\"" + oversizedId + "\",\"method\":\"tools/list\"}"); + + Assert.NotNull(raw); + Assert.DoesNotContain(oversizedId, raw, StringComparison.Ordinal); + var response = JsonNode.Parse(raw!)!; + var error = response["error"]!; + Assert.Equal(-32600, error["code"]!.GetValue()); + Assert.Equal("invalid_request", error["data"]!["category"]!.GetValue()); + Assert.Equal(McpServer.MaxRequestIdCharacterCount, error["data"]!["max_request_id_chars"]!.GetValue()); + Assert.Equal(McpServer.MaxRequestIdByteLength, error["data"]!["max_request_id_bytes"]!.GetValue()); + AssertJsonNullId(response); + } + + [Fact] + public void ProcessFrame_OversizedNumericRequestId_ReturnsInvalidRequestWithNullId_Issue3104() + { + var oversizedId = new string('9', McpServer.MaxRequestIdCharacterCount + 1); + var raw = _server.ProcessFrame("{\"jsonrpc\":\"2.0\",\"id\":" + oversizedId + ",\"method\":\"tools/list\"}"); + + Assert.NotNull(raw); + Assert.DoesNotContain(oversizedId, raw, StringComparison.Ordinal); + var response = JsonNode.Parse(raw!)!; + var error = response["error"]!; + Assert.Equal(-32600, error["code"]!.GetValue()); + Assert.Equal("invalid_request", error["data"]!["category"]!.GetValue()); + Assert.Equal(McpServer.MaxRequestIdCharacterCount, error["data"]!["max_request_id_chars"]!.GetValue()); + Assert.Equal(McpServer.MaxRequestIdByteLength, error["data"]!["max_request_id_bytes"]!.GetValue()); + AssertJsonNullId(response); + } + [Fact] public void ProcessFrame_TooDeepJson_ReturnsParseErrorWithNullId() {