Avoid using SqlXml for serialization (#38506)

Author: AndriySvyryd

src/EFCore.SqlServer/Storage/Internal/SqlServerStringTypeMapping.cs

@@ -2,9 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Data;
-using System.Data.SqlTypes;
 using System.Text;
-using System.Xml;
 using Microsoft.Data.SqlClient;
 using Microsoft.EntityFrameworkCore.Storage.Json;
 
@@ -21,15 +19,6 @@ public class SqlServerStringTypeMapping : StringTypeMapping
     private const int UnicodeMax = 4000;
     private const int AnsiMax = 8000;
 
-    // DTD processing is explicitly prohibited and no external resolver is used so that a malicious
-    // payload is rejected rather than processed.
-    private static readonly XmlReaderSettings XmlFragmentSettings = new()
-    {
-        ConformanceLevel = ConformanceLevel.Fragment,
-        DtdProcessing = DtdProcessing.Prohibit,
-        XmlResolver = null
-    };
-
     private static readonly CaseInsensitiveValueComparer CaseInsensitiveValueComparer = new();
 
     private readonly bool _isUtf16;
@@ -169,15 +158,17 @@ protected override void ConfigureParameter(DbParameter parameter)
 
         if (_sqlDbType == SqlDbType.Xml)
         {
-            // SqlClient only sends a parameter as 'xml' (rather than 'nvarchar(max)') when its value is a SqlXml.
-            // Sending it as 'xml' lets SQL Server honor any encoding declared in the XML prolog (e.g. utf-8), which
-            // otherwise fails with "unable to switch the encoding". A fragment-conformant reader is used so that the
-            // content forms that the 'xml' store type accepts - empty strings, text, and multiple top-level nodes -
-            // continue to round-trip.
+            // xml is a max-length type; pin the size to unbounded so SqlClient does not infer it from the
+            // value length (which would otherwise vary per value in logs and the query cache).
+            parameter.Size = -1;
+
+            // SqlClient sends a string parameter as 'nvarchar(max)' even when its SqlDbType is 'xml', which makes
+            // SQL Server reject an XML prolog that declares a non-UTF-16 encoding (e.g. utf-8) with "unable to switch
+            // the encoding". Only the XML declaration conflicts, and it can only appear at the very start, so it is
+            // sliced off and the rest of the value is sent verbatim.
             if (value is string xml)
             {
-                using var reader = XmlReader.Create(new StringReader(xml), XmlFragmentSettings);
-                parameter.Value = new SqlXml(reader);
+                parameter.Value = RemoveXmlProlog(xml);
             }
 
             return;
@@ -220,6 +211,30 @@ protected override void ConfigureParameter(DbParameter parameter)
         }
     }
 
+    private static string RemoveXmlProlog(string xml)
+    {
+        var start = 0;
+        while (start < xml.Length && char.IsWhiteSpace(xml[start]))
+        {
+            start++;
+        }
+
+        // An XML declaration starts with "<?xml" followed by mandatory whitespace. If present, remove everything
+        // up to and including the closing "?>" and return the remainder verbatim.
+        if (start + 5 < xml.Length
+            && string.CompareOrdinal(xml, start, "<?xml", 0, 5) == 0
+            && char.IsWhiteSpace(xml[start + 5]))
+        {
+            var end = xml.IndexOf("?>", start + 6, StringComparison.Ordinal);
+            if (end >= 0)
+            {
+                return xml[(end + 2)..];
+            }
+        }
+
+        return xml;
+    }
+
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
     ///     the same compatibility standards as public APIs. It may be changed or removed without notice in

test/EFCore.SqlServer.FunctionalTests/BuiltInDataTypesSqlServerTest.cs

@@ -3820,20 +3820,31 @@ FROM INFORMATION_SCHEMA.COLUMNS
     // The grinning-face emoji is outside the BMP (a UTF-16 surrogate pair, four UTF-8 bytes) and the euro sign
     // is a single UTF-16 code unit but three UTF-8 bytes; both are represented differently in UTF-16 than in
     // UTF-8 and are lost when an xml value is sent to the server as a non-Unicode string, which makes them good
-    // probes for the SqlXml/SqlDbType.Xml parameter path.
+    // probes for the SqlDbType.Xml parameter path.
     private const string XmlEmoji = "\U0001F600";
     private const string XmlEuro = "\u20AC";
 
     [Theory]
-    [InlineData("<root>" + XmlEmoji + XmlEuro + "</root>", "<root>" + XmlEmoji + XmlEuro + "</root>")]
-    // An explicit non-UTF-16 prolog is accepted because the value is sent as 'xml', not 'nvarchar(max)'.
-    [InlineData("<?xml version=\"1.0\" encoding=\"utf-8\"?><root>" + XmlEmoji + "</root>", "<root>" + XmlEmoji + "</root>")]
-    [InlineData("<?xml version=\"1.0\" encoding=\"utf-16\"?><root>" + XmlEuro + "</root>", "<root>" + XmlEuro + "</root>")]
+    [InlineData(
+        "<root>" + XmlEmoji + XmlEuro + "</root>",
+        "<root>" + XmlEmoji + XmlEuro + "</root>",
+        "<root>" + XmlEmoji + XmlEuro + "</root>")]
+    // Only the XML declaration is removed; a following stylesheet PI and the rest of the value are sent verbatim.
+    [InlineData(
+        "<?xml version=\"1.0\" encoding=\"utf-8\" standalone='yes' ?> <?xml-stylesheet href=\"style.xsl\" type=\"text/xml\"?> <root>" + XmlEmoji + "</root>",
+        " <?xml-stylesheet href=\"style.xsl\" type=\"text/xml\"?> <root>" + XmlEmoji + "</root>",
+        "<?xml-stylesheet href=\"style.xsl\" type=\"text/xml\"?><root>" + XmlEmoji + "</root>")]
+    // The leading whitespace and the declaration are removed when the value is sent.
+    [InlineData(
+        " <?xml version=\"1.1\" encoding=\"utf-16\"?> <root>" + XmlEuro + "</root>",
+        " <root>" + XmlEuro + "</root>",
+        "<root>" + XmlEuro + "</root>")]
     // Content forms that the 'xml' store type accepts beyond a single well-formed document.
-    [InlineData("", "")]
-    [InlineData("text fragment", "text fragment")]
-    [InlineData("<a/><b/>", "<a /><b />")]
-    public async Task Xml_value_round_trips(string value, string expected)
+    [InlineData("", "", "")]
+    [InlineData("text fragment", "text fragment", "text fragment")]
+    // The content is sent verbatim, but the server expands self-closing tags when the xml column is read back.
+    [InlineData("<a/><b/>", "<a/><b/>", "<a /><b />")]
+    public async Task Xml_value_round_trips(string value, string expected, string roundTripped)
     {
         await using var context = CreateContext();
 
@@ -3845,7 +3856,7 @@ public async Task Xml_value_round_trips(string value, string expected)
         context.ChangeTracker.Clear();
 
         // xml columns cannot be compared directly in a WHERE clause, so the row is fetched by its key. Coalescing
-        // the column with the original value sends that value as an 'xml' parameter, exercising the SqlXml
+        // the column with the original value sends that value as an 'xml' parameter, exercising the prolog-removal
         // parameter path in a query in addition to the insert above.
         var query = context.Set<XmlTestDocument>()
             .Where(d => d.Id == id)
@@ -3860,14 +3871,15 @@ SELECT COALESCE([x].[Content], @value)
 FROM [XmlTestDocument] AS [x]
 WHERE [x].[Id] = @id
 """,
-            query.ToQueryString());
+            query.ToQueryString(),
+            ignoreLineEndingDifferences: true);
 
-        var roundTripped = await query.SingleAsync();
-        Assert.Equal(expected, roundTripped);
+        var actual = await query.SingleAsync();
+        Assert.Equal(roundTripped, actual);
 
         AssertSql(
             $"""
-@p0='{expected}' (DbType = Xml)
+@p0='{expected}' (Size = -1) (DbType = Xml)
 
 SET IMPLICIT_TRANSACTIONS OFF;
 SET NOCOUNT ON;
@@ -3877,7 +3889,7 @@ OUTPUT INSERTED.[Id]
 """,
             //
             $"""
-@value='{expected}' (DbType = Xml)
+@value='{expected}' (Size = -1) (DbType = Xml)
 @id='{id}'
 
 SELECT TOP(2) COALESCE([x].[Content], @value)
@@ -3886,40 +3898,6 @@ FROM [XmlTestDocument] AS [x]
 """);
     }
 
-    [Fact]
-    public async Task Xml_value_with_dtd_payload_is_rejected()
-    {
-        await using var context = CreateContext();
-
-        // A "billion laughs" entity-expansion payload: the reader must reject the DTD rather than expand it.
-        const string maliciousXml =
-            "<?xml version=\"1.0\"?>"
-            + "<!DOCTYPE lolz [<!ENTITY lol \"lol\">"
-            + "<!ENTITY lol2 \"&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;\">"
-            + "<!ENTITY lol3 \"&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;\">]>"
-            + "<lolz>&lol3;</lolz>";
-
-        context.Add(new XmlTestDocument { Content = maliciousXml });
-
-        var exception = await Assert.ThrowsAnyAsync<Exception>(() => context.SaveChangesAsync());
-        Assert.True(
-            HasXmlException(exception),
-            $"Expected an {nameof(XmlException)} in the exception chain but found: {exception}");
-
-        static bool HasXmlException(Exception exception)
-        {
-            for (var current = exception; current is not null; current = current.InnerException)
-            {
-                if (current is XmlException)
-                {
-                    return true;
-                }
-            }
-
-            return false;
-        }
-    }
-
     private class XmlTestDocument
     {
         public int Id { get; set; }

test/EFCore.SqlServer.Tests/Storage/SqlServerTypeMappingTest.cs

@@ -7,6 +7,7 @@
 using Microsoft.EntityFrameworkCore.Design.Internal;
 using Microsoft.EntityFrameworkCore.SqlServer.Infrastructure.Internal;
 using Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal;
+using Microsoft.EntityFrameworkCore.Storage.Internal;
 
 // ReSharper disable InconsistentNaming
 namespace Microsoft.EntityFrameworkCore.Storage;
@@ -379,12 +380,18 @@ public virtual void Char_Utf8()
     }
 
     [Theory]
-    [InlineData("<r>a</r>")]
-    [InlineData("<?xml version=\"1.0\" encoding=\"utf-8\"?><r>a</r>")]
-    [InlineData("")]
-    [InlineData("text fragment")]
-    [InlineData("<a/><b/>")]
-    public virtual void Xml_parameter_is_sent_as_SqlXml(string value)
+    [InlineData("<r>a</r>", "<r>a</r>")]
+    // The XML declaration is removed so the value can be sent as a string without an encoding conflict.
+    [InlineData("<?xml version=\"1.0\" encoding=\"utf-8\"?><r>a</r>", "<r>a</r>")]
+    // The declaration's closing '>' is found even when a space precedes it, and any leading whitespace is dropped too.
+    [InlineData(" <?xml version=\"1.0\" encoding=\"utf-8\" ?> <r>a</r>", " <r>a</r>")]
+    // Only the declaration is removed; a following stylesheet PI and the rest are kept verbatim.
+    [InlineData("<?xml version=\"1.1\" encoding=\"utf-8\"?><?xml-stylesheet href=\"s.xsl\"?><r>a</r>", "<?xml-stylesheet href=\"s.xsl\"?><r>a</r>")]
+    [InlineData("", "")]
+    [InlineData("text fragment", "text fragment")]
+    // Content after the prolog is sent verbatim, so the original formatting is preserved.
+    [InlineData("<a/><b/>", "<a/><b/>")]
+    public virtual void Xml_parameter_is_sent_as_string_with_prolog_removed(string value, string expected)
     {
         var mapping = GetMapping("xml");
         Assert.Equal("xml", mapping.StoreType);
@@ -393,8 +400,12 @@ public virtual void Xml_parameter_is_sent_as_SqlXml(string value)
         var parameter = (SqlParameter)mapping.CreateParameter(command, "foo", value);
 
         Assert.Equal(SqlDbType.Xml, parameter.SqlDbType);
-        var sqlXml = Assert.IsType<System.Data.SqlTypes.SqlXml>(parameter.Value);
-        Assert.False(sqlXml.IsNull);
+
+        // The value stays a string so it can still be rendered by the diagnostics logger.
+        Assert.Equal(expected, parameter.Value);
+        Assert.Equal(
+            $"foo='{expected}' (Nullable = false) (Size = -1) (DbType = Xml)",
+            parameter.FormatParameter(logParameterValues: true));
     }
 
     [Fact]