From 40aff6da4df74c37a137713bcfaa6d66894f7480 Mon Sep 17 00:00:00 2001 From: PrathmeshPawar <81473333+PrathmeshPawar2110@users.noreply.github.com> Date: Sun, 25 Oct 2026 17:30:25 +0530 Subject: [PATCH] fix: validate Blob/Data setters reject raw binary disguised as base64 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ImageContentBlock.Data, AudioContentBlock.Data, and BlobResourceContents.Blob are ReadOnlyMemory properties documented to hold the base64 text representation of binary data, as UTF-8 bytes — not the raw, unencoded bytes. Assigning raw bytes directly (instead of going through the FromBytes factories) is an easy, understandable mistake: nothing fails at the point of assignment, and the polymorphic JSON writer serializes those bytes as if they already were base64 text. Most raw binary is not valid UTF-8, so the writer corrupts it in the process (replacement characters, escaped control characters), and the corruption only surfaces later, confusingly, as a client-side FormatException when the receiving end tries to base64-decode the mangled text. This isn't a serializer regression — serializing a BlobResourceContents or ImageContentBlock built correctly (via FromBytes, or with Blob/Data set to real base64 UTF-8 bytes) already round-trips correctly. The problem is that misuse of the byte-typed property fails late and unhelpfully instead of failing immediately at the point of misuse. Add EncodingUtilities.ValidateBase64Utf8 and call it from the Blob/Data setters, so assigning non-base64 bytes now throws an ArgumentException immediately, naming the FromBytes factory to use instead. To avoid penalizing the common case, the JSON converters' existing fast path (reusing an unescaped wire value's UTF-8 bytes directly, which are already known-valid base64 text as read from the document) now goes through a new internal FromTrustedBase64Utf8 factory that bypasses this validation, so normal deserialization pays no extra cost. A few existing tests used a non-base64 placeholder string ("base64data", or raw byte literals) as filler Data/Blob content; those are updated to use real base64 text, since that was never the behavior under test. Fixes #1835 Co-Authored-By: Claude Sonnet 5 --- src/Common/EncodingUtilities.cs | 35 ++++++++++ .../Protocol/BlobResourceContents.cs | 26 +++++++- .../Protocol/ContentBlock.cs | 66 +++++++++++++++---- .../Protocol/ResourceContents.cs | 10 +-- .../Protocol/ContentBlockTests.cs | 44 ++++++++++++- .../Protocol/PromptMessageTests.cs | 8 ++- .../Protocol/ReadResourceResultTests.cs | 4 +- .../Protocol/ResourceContentsTests.cs | 17 ++++- 8 files changed, 183 insertions(+), 27 deletions(-) diff --git a/src/Common/EncodingUtilities.cs b/src/Common/EncodingUtilities.cs index 50127882b..a3efb8714 100644 --- a/src/Common/EncodingUtilities.cs +++ b/src/Common/EncodingUtilities.cs @@ -56,4 +56,39 @@ public static ReadOnlyMemory DecodeFromBase64Utf8(ReadOnlyMemory bas throw new FormatException("Invalid base64 data"); } } + + /// + /// Validates that is a well-formed base64 encoding (as UTF-8 bytes), + /// without retaining the decoded result. + /// + /// + /// Several protocol types expose a property that is documented to + /// hold the base64 text representation of binary data (as UTF-8 bytes), with a FromBytes factory + /// provided to construct an instance from the raw, unencoded bytes instead. Assigning raw bytes directly + /// to such a property is a common, easy mistake: the value serializes without error but produces a JSON + /// payload that is not valid base64, which only surfaces as a confusing on + /// the deserializing end. This validates eagerly, at the point of misuse, instead. + /// + /// is not valid base64-encoded UTF-8 data. + public static void ValidateBase64Utf8(ReadOnlyMemory base64Utf8, string paramName, string typeName, string factoryMethodName) + { + int maxLength = Base64.GetMaxDecodedFromUtf8Length(base64Utf8.Length); + byte[] buffer = ArrayPool.Shared.Rent(maxLength); + try + { + OperationStatus status = Base64.DecodeFromUtf8(base64Utf8.Span, buffer, out int bytesConsumed, out _); + if (status != OperationStatus.Done || bytesConsumed != base64Utf8.Length) + { + throw new ArgumentException( + $"The value must be the base64 text representation of the binary data, as UTF-8 bytes, " + + $"not the raw, unencoded bytes. Use {typeName}.{factoryMethodName}(...) to construct an " + + $"instance from raw binary data instead.", + paramName); + } + } + finally + { + ArrayPool.Shared.Return(buffer); + } + } } diff --git a/src/ModelContextProtocol.Core/Protocol/BlobResourceContents.cs b/src/ModelContextProtocol.Core/Protocol/BlobResourceContents.cs index d35325010..e540ca043 100644 --- a/src/ModelContextProtocol.Core/Protocol/BlobResourceContents.cs +++ b/src/ModelContextProtocol.Core/Protocol/BlobResourceContents.cs @@ -56,12 +56,35 @@ private BlobResourceContents(ReadOnlyMemory decodedData, string uri, strin MimeType = mimeType; } + [SetsRequiredMembers] + private BlobResourceContents(ReadOnlyMemory base64Utf8, string uri, string? mimeType, bool alreadyBase64Encoded) + { + _blob = base64Utf8; + Uri = uri; + MimeType = mimeType; + } + + /// + /// Creates a directly from base64-encoded UTF-8 bytes that are + /// already known to be valid, bypassing the validation the public setter performs. + /// + /// + /// Used by for the JSON fast path: when the wire blob value required no + /// unescaping, its raw UTF-8 bytes are exactly the base64 text as read from the document, so + /// re-validating them would be redundant. + /// + internal static BlobResourceContents FromTrustedBase64Utf8(ReadOnlyMemory base64Utf8, string uri, string? mimeType) => + new(base64Utf8, uri, mimeType, alreadyBase64Encoded: true); + /// /// Gets or sets the base64-encoded UTF-8 bytes representing the binary data of the item. /// /// - /// Setting this value will invalidate any cached value of . + /// Setting this value will invalidate any cached value of . The value must be + /// the base64 text representation of the data, as UTF-8 bytes — not the raw, unencoded bytes. To + /// construct an instance from raw binary data, use instead. /// + /// The assigned value is not valid base64-encoded UTF-8 data. [JsonPropertyName("blob")] public required ReadOnlyMemory Blob { @@ -77,6 +100,7 @@ public required ReadOnlyMemory Blob } set { + EncodingUtilities.ValidateBase64Utf8(value, nameof(Blob), nameof(BlobResourceContents), nameof(FromBytes)); _blob = value; _decodedData = null; // Invalidate cache } diff --git a/src/ModelContextProtocol.Core/Protocol/ContentBlock.cs b/src/ModelContextProtocol.Core/Protocol/ContentBlock.cs index d0b1b80ec..bd330c0dd 100644 --- a/src/ModelContextProtocol.Core/Protocol/ContentBlock.cs +++ b/src/ModelContextProtocol.Core/Protocol/ContentBlock.cs @@ -243,20 +243,16 @@ public sealed class Converter : JsonConverter "image" => decodedData is not null ? ImageContentBlock.FromBytes(decodedData.Value, mimeType ?? throw new JsonException("MIME type must be provided for 'image' type.")) : - new ImageContentBlock - { - Data = data ?? throw new JsonException("Image data must be provided for 'image' type."), - MimeType = mimeType ?? throw new JsonException("MIME type must be provided for 'image' type."), - }, + ImageContentBlock.FromTrustedBase64Utf8( + data ?? throw new JsonException("Image data must be provided for 'image' type."), + mimeType ?? throw new JsonException("MIME type must be provided for 'image' type.")), "audio" => decodedData is not null ? AudioContentBlock.FromBytes(decodedData.Value, mimeType ?? throw new JsonException("MIME type must be provided for 'audio' type.")) : - new AudioContentBlock - { - Data = data ?? throw new JsonException("Audio data must be provided for 'audio' type."), - MimeType = mimeType ?? throw new JsonException("MIME type must be provided for 'audio' type."), - }, + AudioContentBlock.FromTrustedBase64Utf8( + data ?? throw new JsonException("Audio data must be provided for 'audio' type."), + mimeType ?? throw new JsonException("MIME type must be provided for 'audio' type.")), "resource" => new EmbeddedResourceBlock { @@ -462,6 +458,25 @@ private ImageContentBlock(ReadOnlyMemory decodedData, string mimeType) MimeType = mimeType; } + [SetsRequiredMembers] + private ImageContentBlock(ReadOnlyMemory base64Utf8, string mimeType, bool alreadyBase64Encoded) + { + _data = base64Utf8; + MimeType = mimeType; + } + + /// + /// Creates an directly from base64-encoded UTF-8 bytes that are + /// already known to be valid, bypassing the validation the public setter performs. + /// + /// + /// Used by for the JSON fast path: when the wire data value + /// required no unescaping, its raw UTF-8 bytes are exactly the base64 text as read from the document, + /// so re-validating them would be redundant. + /// + internal static ImageContentBlock FromTrustedBase64Utf8(ReadOnlyMemory base64Utf8, string mimeType) => + new(base64Utf8, mimeType, alreadyBase64Encoded: true); + /// public override string Type => "image"; @@ -469,8 +484,11 @@ private ImageContentBlock(ReadOnlyMemory decodedData, string mimeType) /// Gets or sets the base64-encoded UTF-8 bytes representing the image data. /// /// - /// Setting this value will invalidate any cached value of . + /// Setting this value will invalidate any cached value of . The value must be + /// the base64 text representation of the data, as UTF-8 bytes — not the raw, unencoded bytes. To + /// construct an instance from raw binary data, use instead. /// + /// The assigned value is not valid base64-encoded UTF-8 data. [JsonPropertyName("data")] public required ReadOnlyMemory Data { @@ -486,6 +504,7 @@ public required ReadOnlyMemory Data } set { + EncodingUtilities.ValidateBase64Utf8(value, nameof(Data), nameof(ImageContentBlock), nameof(FromBytes)); _data = value; _decodedData = null; // Invalidate cache } @@ -571,6 +590,25 @@ private AudioContentBlock(ReadOnlyMemory decodedData, string mimeType) MimeType = mimeType; } + [SetsRequiredMembers] + private AudioContentBlock(ReadOnlyMemory base64Utf8, string mimeType, bool alreadyBase64Encoded) + { + _data = base64Utf8; + MimeType = mimeType; + } + + /// + /// Creates an directly from base64-encoded UTF-8 bytes that are + /// already known to be valid, bypassing the validation the public setter performs. + /// + /// + /// Used by for the JSON fast path: when the wire data value + /// required no unescaping, its raw UTF-8 bytes are exactly the base64 text as read from the document, + /// so re-validating them would be redundant. + /// + internal static AudioContentBlock FromTrustedBase64Utf8(ReadOnlyMemory base64Utf8, string mimeType) => + new(base64Utf8, mimeType, alreadyBase64Encoded: true); + /// public override string Type => "audio"; @@ -578,8 +616,11 @@ private AudioContentBlock(ReadOnlyMemory decodedData, string mimeType) /// Gets or sets the base64-encoded UTF-8 bytes representing the audio data. /// /// - /// Setting this value will invalidate any cached value of . + /// Setting this value will invalidate any cached value of . The value must be + /// the base64 text representation of the data, as UTF-8 bytes — not the raw, unencoded bytes. To + /// construct an instance from raw binary data, use instead. /// + /// The assigned value is not valid base64-encoded UTF-8 data. [JsonPropertyName("data")] public required ReadOnlyMemory Data { @@ -595,6 +636,7 @@ public required ReadOnlyMemory Data } set { + EncodingUtilities.ValidateBase64Utf8(value, nameof(Data), nameof(AudioContentBlock), nameof(FromBytes)); _data = value; _decodedData = null; // Invalidate cache } diff --git a/src/ModelContextProtocol.Core/Protocol/ResourceContents.cs b/src/ModelContextProtocol.Core/Protocol/ResourceContents.cs index 283c47d6b..940ddd52f 100644 --- a/src/ModelContextProtocol.Core/Protocol/ResourceContents.cs +++ b/src/ModelContextProtocol.Core/Protocol/ResourceContents.cs @@ -139,13 +139,9 @@ public sealed class Converter : JsonConverter if (blob is not null) { - return new BlobResourceContents - { - Uri = uri ?? string.Empty, - MimeType = mimeType, - Blob = blob.Value, - Meta = meta, - }; + var blobResource = BlobResourceContents.FromTrustedBase64Utf8(blob.Value, uri ?? string.Empty, mimeType); + blobResource.Meta = meta; + return blobResource; } if (text is not null) diff --git a/tests/ModelContextProtocol.Tests/Protocol/ContentBlockTests.cs b/tests/ModelContextProtocol.Tests/Protocol/ContentBlockTests.cs index 6e943f025..ed00d9c9d 100644 --- a/tests/ModelContextProtocol.Tests/Protocol/ContentBlockTests.cs +++ b/tests/ModelContextProtocol.Tests/Protocol/ContentBlockTests.cs @@ -231,7 +231,11 @@ public void ToolResultContentBlock_SerializationRoundTrip() Content = [ new TextContentBlock { Text = "Result data" }, - new ImageContentBlock { Data = System.Text.Encoding.UTF8.GetBytes("base64data"), MimeType = "image/png" } + new ImageContentBlock + { + Data = System.Text.Encoding.UTF8.GetBytes(Convert.ToBase64String(System.Text.Encoding.UTF8.GetBytes("base64data"))), + MimeType = "image/png", + } ], StructuredContent = JsonElement.Parse("""{"temperature":18,"condition":"cloudy"}"""), IsError = false @@ -246,7 +250,7 @@ public void ToolResultContentBlock_SerializationRoundTrip() var textBlock = Assert.IsType(result.Content[0]); Assert.Equal("Result data", textBlock.Text); var imageBlock = Assert.IsType(result.Content[1]); - Assert.Equal("base64data", System.Text.Encoding.UTF8.GetString(imageBlock.Data.ToArray())); + Assert.Equal("base64data", System.Text.Encoding.UTF8.GetString(imageBlock.DecodedData.ToArray())); Assert.Equal("image/png", imageBlock.MimeType); Assert.NotNull(result.StructuredContent); Assert.Equal(18, result.StructuredContent.Value.GetProperty("temperature").GetInt32()); @@ -292,6 +296,42 @@ public void AudioContentBlock_FromBytes_ThrowsForNullOrWhiteSpaceMimeType(string Assert.ThrowsAny(() => AudioContentBlock.FromBytes((byte[])[1, 2, 3], mimeType!)); } + [Fact] + public void ImageContentBlock_DataSetter_ThrowsForRawUnencodedBytes() + { + // Assigning raw binary directly to Data (instead of going through FromBytes) must fail + // immediately and clearly, rather than silently producing a corrupt JSON payload that only + // fails to parse on the receiving end. Regression test for + // https://github.com/modelcontextprotocol/csharp-sdk/issues/1835. + byte[] rawBytes = new byte[900_000]; + new Random(42).NextBytes(rawBytes); + + var ex = Assert.Throws(() => new ImageContentBlock { Data = rawBytes, MimeType = "image/png" }); + Assert.Equal("Data", ex.ParamName); + Assert.Contains("ImageContentBlock.FromBytes", ex.Message); + } + + [Fact] + public void AudioContentBlock_DataSetter_ThrowsForRawUnencodedBytes() + { + byte[] rawBytes = new byte[900_000]; + new Random(42).NextBytes(rawBytes); + + var ex = Assert.Throws(() => new AudioContentBlock { Data = rawBytes, MimeType = "audio/wav" }); + Assert.Equal("Data", ex.ParamName); + Assert.Contains("AudioContentBlock.FromBytes", ex.Message); + } + + [Fact] + public void ImageContentBlock_DataSetter_ThrowsForOddLengthBase64() + { + // Well-formed base64 alphabet characters are not enough on their own; the length must also + // be consistent with padding rules. + var ex = Assert.Throws(() => + new ImageContentBlock { Data = System.Text.Encoding.UTF8.GetBytes("YWJ"), MimeType = "image/png" }); + Assert.Equal("Data", ex.ParamName); + } + [Fact] public void ImageContentBlock_Deserialization_HandlesEscapedForwardSlashInBase64() { diff --git a/tests/ModelContextProtocol.Tests/Protocol/PromptMessageTests.cs b/tests/ModelContextProtocol.Tests/Protocol/PromptMessageTests.cs index b89f2a424..2246e4f44 100644 --- a/tests/ModelContextProtocol.Tests/Protocol/PromptMessageTests.cs +++ b/tests/ModelContextProtocol.Tests/Protocol/PromptMessageTests.cs @@ -29,7 +29,11 @@ public static void PromptMessage_SerializationRoundTrip_WithImageContent() var original = new PromptMessage { Role = Role.Assistant, - Content = new ImageContentBlock { Data = System.Text.Encoding.UTF8.GetBytes("base64data"), MimeType = "image/png" } + Content = new ImageContentBlock + { + Data = System.Text.Encoding.UTF8.GetBytes(Convert.ToBase64String(System.Text.Encoding.UTF8.GetBytes("base64data"))), + MimeType = "image/png", + } }; string json = JsonSerializer.Serialize(original, McpJsonUtilities.DefaultOptions); @@ -38,7 +42,7 @@ public static void PromptMessage_SerializationRoundTrip_WithImageContent() Assert.NotNull(deserialized); Assert.Equal(Role.Assistant, deserialized.Role); var imageBlock = Assert.IsType(deserialized.Content); - Assert.Equal("base64data", System.Text.Encoding.UTF8.GetString(imageBlock.Data.ToArray())); + Assert.Equal("base64data", System.Text.Encoding.UTF8.GetString(imageBlock.DecodedData.ToArray())); Assert.Equal("image/png", imageBlock.MimeType); } } diff --git a/tests/ModelContextProtocol.Tests/Protocol/ReadResourceResultTests.cs b/tests/ModelContextProtocol.Tests/Protocol/ReadResourceResultTests.cs index fadfba6ca..e6aa721c8 100644 --- a/tests/ModelContextProtocol.Tests/Protocol/ReadResourceResultTests.cs +++ b/tests/ModelContextProtocol.Tests/Protocol/ReadResourceResultTests.cs @@ -23,7 +23,7 @@ public static void ReadResourceResult_SerializationRoundTrip_PreservesAllPropert { Uri = "file:///image.png", MimeType = "image/png", - Blob = System.Text.Encoding.UTF8.GetBytes("base64data") + Blob = System.Text.Encoding.UTF8.GetBytes(Convert.ToBase64String(System.Text.Encoding.UTF8.GetBytes("base64data"))) } ], Meta = new JsonObject { ["key"] = "value" } @@ -43,7 +43,7 @@ public static void ReadResourceResult_SerializationRoundTrip_PreservesAllPropert var blobContent = Assert.IsType(deserialized.Contents[1]); Assert.Equal("file:///image.png", blobContent.Uri); Assert.Equal("image/png", blobContent.MimeType); - Assert.Equal("base64data", System.Text.Encoding.UTF8.GetString(blobContent.Blob.ToArray())); + Assert.Equal("base64data", System.Text.Encoding.UTF8.GetString(blobContent.DecodedData.ToArray())); Assert.NotNull(deserialized.Meta); Assert.Equal("value", (string)deserialized.Meta["key"]!); diff --git a/tests/ModelContextProtocol.Tests/Protocol/ResourceContentsTests.cs b/tests/ModelContextProtocol.Tests/Protocol/ResourceContentsTests.cs index 073f349de..5130605ba 100644 --- a/tests/ModelContextProtocol.Tests/Protocol/ResourceContentsTests.cs +++ b/tests/ModelContextProtocol.Tests/Protocol/ResourceContentsTests.cs @@ -483,7 +483,7 @@ public static void BlobResourceContents_NullMimeType_OmittedFromJson() { Uri = "file:///test.bin", MimeType = null, - Blob = new byte[] { 1, 2, 3 } + Blob = System.Text.Encoding.UTF8.GetBytes(Convert.ToBase64String(new byte[] { 1, 2, 3 })) }; var json = JsonSerializer.Serialize(resource, McpJsonUtilities.DefaultOptions); @@ -509,6 +509,21 @@ public static void BlobResourceContents_Deserialization_HandlesEscapedForwardSla Assert.Equal(originalBytes, blob.DecodedData.ToArray()); } + [Fact] + public static void BlobResourceContents_BlobSetter_ThrowsForRawUnencodedBytes() + { + // Assigning raw binary directly to Blob (instead of going through FromBytes) must fail + // immediately and clearly, rather than silently producing a corrupt JSON payload that only + // fails to parse on the receiving end. Regression test for + // https://github.com/modelcontextprotocol/csharp-sdk/issues/1835. + byte[] rawBytes = new byte[900_000]; + new Random(42).NextBytes(rawBytes); + + var ex = Assert.Throws(() => new BlobResourceContents { Blob = rawBytes, Uri = "demo://files/1/test.pdf" }); + Assert.Equal("Blob", ex.ParamName); + Assert.Contains("BlobResourceContents.FromBytes", ex.Message); + } + [Theory] [MemberData(nameof(ContentBlockTests.Base64TestData), MemberType = typeof(ContentBlockTests))] public static void BlobResourceContents_FromBytes_RoundtripsCorrectly(byte[] originalBytes)