add opt-in XML character sanitization for package save by SimonCropp · Pull Request #2064 · dotnet/Open-XML-SDK

SimonCropp · 2026-04-15T06:59:10Z

Introduces IXmlWriterFactoryFeature and the UseXmlTextSanitization() extension on OpenXmlPackage. When enabled, invalid XML 1.0 characters (control chars, orphaned surrogates, non-characters) are silently removed from text content, attribute values, CDATA, comments, and PI bodies before the underlying XmlWriter sees them. Element/attribute names and WriteRaw payloads are untouched.

Previously, saving a document with text like "Hello\u0002World" — common when exporting data straight from legacy databases — threw deep inside System.Xml.XmlUtf8RawTextWriter. The feature is pay-for-play: packages that don't opt in pay nothing.

Plumbing centralizes XmlWriter creation behind the feature so that OpenXmlPartWriter, OpenXmlPartRootElement.Save, and every XmlDOMTextWriter call site (including InnerXml/OuterXml paths) route through it, inherited transparently from package → part → element.

Covers Word, Excel, and PowerPoint round-trip tests plus unit tests for the sanitizer, the decorator, and the feature registration.

fixes #1532

SimonCropp · 2026-04-15T07:01:06Z

IMO this should be the default behaviour.

github-actions · 2026-04-15T07:17:59Z

Test Results

66 files - 3 66 suites - 3 1h 39m 24s ⏱️ - 1m 39s
2 105 tests + 37 2 102 ✅ + 37 3 💤 ±0 0 ❌ ±0
38 540 runs +467 38 498 ✅ +467 42 💤 ±0 0 ❌ ±0

Results for commit 25a58a1. ± Comparison against base commit 0cb2f6b.

twsouthwick

Thanks for the implementation!

twsouthwick · 2026-04-27T23:34:30Z

+    }
+
+    public XmlWriter Create(Stream stream, XmlWriterSettings? settings)
+        => new SanitizingXmlWriter(DefaultXmlWriterFactoryFeature.Instance.Create(stream, settings));


this shouldn't just use the default - it should pass in the existing and wrap it

twsouthwick · 2026-04-27T23:34:48Z

+
+internal static class XmlCharacterSanitizer
+{
+    public static bool IsValidXmlChar(char c)


is there a reason to not use XmlConvert.IsXmlChar here?

twsouthwick · 2026-04-28T00:24:48Z

+
+        if (package.Features.Get<IXmlWriterFactoryFeature>() is not SanitizingXmlWriterFactoryFeature)
+        {
+            package.Features.Set<IXmlWriterFactoryFeature>(SanitizingXmlWriterFactoryFeature.Instance);


wrap the existing one here

twsouthwick · 2026-04-28T00:25:54Z

+            _xmlWriter = ResolveFactory(openXmlPart).Create(partStream, xmlWriterSettings);
        }

+        private static IXmlWriterFactoryFeature ResolveFactory(OpenXmlPart openXmlPart)


I'd prefer to just use GetRequiredFeature - there's a default one already so we don't need it here as well

twsouthwick · 2026-04-28T00:27:22Z

-            _writer = Create(stream);
+        }
+
+        public XmlDOMTextWriter(Stream stream, IFeatureCollection? features)


pass in a IXmlWriterFactoryFeature here instead of a full collection

Copilot

Pull request overview

Adds an opt-in feature to sanitize invalid XML 1.0 characters during package/part serialization by centralizing XmlWriter creation behind a new IXmlWriterFactoryFeature, with a built-in sanitizing implementation that decorates the writer.

Changes:

Introduces IXmlWriterFactoryFeature plus default and sanitizing factory implementations, and OpenXmlPackage.UseXmlTextSanitization() to enable it.
Routes key SDK serialization paths (OpenXmlPartWriter, OpenXmlPartRootElement.Save, XmlDOMTextWriter call sites) through the factory.
Adds unit and round-trip tests for sanitizer behavior across Word/Excel/PowerPoint, and updates public API + docs.

Show a summary per file

File	Description
test/DocumentFormat.OpenXml.Framework.Tests/Features/XmlTextSanitizationTests.cs	Adds unit tests for sanitizer + sanitizing writer behavior.
test/DocumentFormat.OpenXml.Framework.Tests/Features/XmlTextSanitizationTests.Word.cs	Word round-trip tests validating opt-in sanitization behavior.
test/DocumentFormat.OpenXml.Framework.Tests/Features/XmlTextSanitizationTests.Excel.cs	Excel round-trip tests validating opt-in sanitization behavior.
test/DocumentFormat.OpenXml.Framework.Tests/Features/XmlTextSanitizationTests.Powerpoint.cs	PowerPoint round-trip tests validating opt-in sanitization behavior.
src/DocumentFormat.OpenXml.Linq/Feature/OpenXmlPartRootXElementFeature.cs	Uses `XmlDOMTextWriter` overload that can consume part features (factory).
src/DocumentFormat.OpenXml.Framework/XmlDOMTextWriter.cs	Adds feature-aware constructors and resolves writer factory from `IFeatureCollection`.
src/DocumentFormat.OpenXml.Framework/PublicAPI/PublicAPI.Unshipped.txt	Declares new public surface area (`IXmlWriterFactoryFeature`, extension).
src/DocumentFormat.OpenXml.Framework/OpenXmlPartWriter.cs	Switches part-based writer creation to use the resolved writer factory.
src/DocumentFormat.OpenXml.Framework/OpenXmlPartRootElement.cs	Routes `Save(Stream)` writer creation through `XmlDOMTextWriter` + features.
src/DocumentFormat.OpenXml.Framework/OpenXmlLeafTextElement.cs	Routes `InnerXml` writing through feature-aware `XmlDOMTextWriter`.
src/DocumentFormat.OpenXml.Framework/OpenXmlElement.cs	Routes `InnerXml`/`OuterXml` writing through feature-aware `XmlDOMTextWriter`.
src/DocumentFormat.OpenXml.Framework/OpenXmlCompositeElement.cs	Routes `InnerXml` wrapper writing through feature-aware `XmlDOMTextWriter`.
src/DocumentFormat.OpenXml.Framework/Features/XmlTextSanitizationExtensions.cs	Adds `UseXmlTextSanitization()` opt-in extension for packages.
src/DocumentFormat.OpenXml.Framework/Features/XmlCharacterSanitizer.cs	Implements core XML 1.0 character filtering logic.
src/DocumentFormat.OpenXml.Framework/Features/SanitizingXmlWriterFactoryFeature.cs	Factory feature that wraps writers with `SanitizingXmlWriter`.
src/DocumentFormat.OpenXml.Framework/Features/SanitizingXmlWriter.cs	`XmlWriter` decorator that sanitizes text-ish writes while leaving `WriteRaw` untouched.
src/DocumentFormat.OpenXml.Framework/Features/IXmlWriterFactoryFeature.cs	New public feature interface for centralized writer creation.
src/DocumentFormat.OpenXml.Framework/Features/DefaultXmlWriterFactoryFeature.cs	Default factory mapping to `System.Xml.XmlWriter.Create`.
src/DocumentFormat.OpenXml.Framework/Features/DefaultFeatures.cs	Registers `IXmlWriterFactoryFeature` in the default feature set.
docs/Features.md	Documents the new feature and the opt-in sanitization behavior.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

src/DocumentFormat.OpenXml.Framework/Features/SanitizingXmlWriter.cs:204

WriteCharsAsync has the same parameter-validation gap as the sync path: it only checks buffer != null before iterating/sanitizing. Validate index/count (and index + count overflow) up front so invalid arguments produce consistent XmlWriter-style exceptions instead of IndexOutOfRangeException.

    public override Task WriteCharsAsync(char[] buffer, int index, int count)
    {
        if (buffer is null)
        {
            throw new ArgumentNullException(nameof(buffer));
        }

Files reviewed: 20/20 changed files
Comments generated: 1

Copilot · 2026-04-28T00:32:32Z

+        {
+            throw new ArgumentNullException(nameof(buffer));
+        }
+


WriteChars validates only buffer != null. If index/count are negative or out of range, the sanitizer loop will throw IndexOutOfRangeException (or worse on overflow) rather than the ArgumentOutOfRangeException/ArgumentException behavior callers expect from XmlWriter. Add explicit bounds validation (including index + count overflow) before calling TrySanitizeSpan/iterating, and mirror the inner writer’s exception types.

This issue also appears on line 199 of the same file.

Suggested change

if (index < 0)

{

throw new ArgumentOutOfRangeException(nameof(index));

}

if (count < 0)

{

throw new ArgumentOutOfRangeException(nameof(count));

}

if (index > buffer.Length - count)

{

throw new ArgumentException("The sum of index and count is larger than the buffer length.");

}

SimonCropp added 2 commits April 15, 2026 16:48

XmlTextSanitization

889f74a

.

25a58a1

SimonCropp changed the title ~~add opt-in XML character sanitization for package save (#1532)~~ add opt-in XML character sanitization for package save Apr 15, 2026

twsouthwick requested changes Apr 28, 2026

View reviewed changes

twsouthwick requested a review from Copilot April 28, 2026 00:28

Copilot started reviewing on behalf of twsouthwick April 28, 2026 00:29 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add opt-in XML character sanitization for package save#2064

add opt-in XML character sanitization for package save#2064
SimonCropp wants to merge 2 commits intodotnet:mainfrom
SimonCropp:XmlTextSanitization

SimonCropp commented Apr 15, 2026 •

edited

Loading

Uh oh!

SimonCropp commented Apr 15, 2026

Uh oh!

github-actions Bot commented Apr 15, 2026

Uh oh!

twsouthwick left a comment

Uh oh!

twsouthwick Apr 27, 2026

Uh oh!

twsouthwick Apr 27, 2026

Uh oh!

twsouthwick Apr 28, 2026

Uh oh!

twsouthwick Apr 28, 2026

Uh oh!

twsouthwick Apr 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

+        if (index < 0)
+        {
+            throw new ArgumentOutOfRangeException(nameof(index));
+        }
+        if (count < 0)
+        {
+            throw new ArgumentOutOfRangeException(nameof(count));
+        }
+        if (index > buffer.Length - count)
+        {
+            throw new ArgumentException("The sum of index and count is larger than the buffer length.");
+        }

Conversation

SimonCropp commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonCropp commented Apr 15, 2026

Uh oh!

github-actions Bot commented Apr 15, 2026

Test Results

Uh oh!

twsouthwick left a comment

Choose a reason for hiding this comment

Uh oh!

twsouthwick Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

twsouthwick Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

twsouthwick Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

twsouthwick Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

twsouthwick Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SimonCropp commented Apr 15, 2026 •

edited

Loading