Skip to content

Fix JUnit reporter crash on invalid XML characters in test names and exceptions #4262

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thomhurst merged 3 commits into from
Jan 8, 2026
+95 −21
Merged

Fix JUnit reporter crash on invalid XML characters in test names and exceptions #4262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c919bb5
Initial plan
Copilot Jan 8, 2026
442dfa4
Fix JUnit reporter crash with invalid XML characters - add sanitizati…
Copilot Jan 8, 2026
b69e524
Add test for failing test with invalid XML characters in exception me…
Copilot Jan 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 54 additions & 21 deletions TUnit.Engine/Xml/JUnitXmlWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,39 @@ namespace TUnit.Engine.Xml;

internal static class JUnitXmlWriter
{
/// <summary>
/// Sanitizes a string to be XML-safe by removing invalid XML characters.
/// According to XML 1.0 spec, valid characters are:
/// #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x10000-#x10FFFF]
/// </summary>
private static string SanitizeForXml(string? value)
{
if (string.IsNullOrEmpty(value))
{
return string.Empty;
}

// At this point, value is guaranteed to not be null
var builder = new StringBuilder(value!.Length);
foreach (var ch in value!)
Comment on lines +26 to +27
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The redundant null-forgiving operators (!) on lines 26 and 27 are unnecessary since the check on line 20 already confirms the value is not null or empty. The value cannot be null at this point in the execution.

Suggested change
var builder = new StringBuilder(value!.Length);
foreach (var ch in value!)
var builder = new StringBuilder(value.Length);
foreach (var ch in value)

Copilot uses AI. Check for mistakes.
{
// Check if character is valid according to XML 1.0 spec
if (ch == 0x9 || ch == 0xA || ch == 0xD ||
(ch >= 0x20 && ch <= 0xD7FF) ||
(ch >= 0xE000 && ch <= 0xFFFD))
{
builder.Append(ch);
}
else
{
// Replace invalid character with its hex representation
builder.Append($"[0x{((int)ch):X}]");
}
Comment on lines +27 to +40
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SanitizeForXml method does not correctly handle Unicode surrogate pairs. When iterating character-by-character, characters in the range U+10000 to U+10FFFF (which are valid per XML 1.0 spec) are represented as two char values (high and low surrogates). The current implementation will incorrectly process each surrogate individually, potentially replacing valid characters with hex notation. Consider using StringInfo or Rune enumeration to properly handle multi-char Unicode sequences.

Suggested change
foreach (var ch in value!)
{
// Check if character is valid according to XML 1.0 spec
if (ch == 0x9 || ch == 0xA || ch == 0xD ||
(ch >= 0x20 && ch <= 0xD7FF) ||
(ch >= 0xE000 && ch <= 0xFFFD))
{
builder.Append(ch);
}
else
{
// Replace invalid character with its hex representation
builder.Append($"[0x{((int)ch):X}]");
}
var length = value!.Length;
for (var i = 0; i < length; i++)
{
var ch = value[i];
int codePoint;
var charsConsumed = 1;
// Handle surrogate pairs to correctly process characters in the range U+10000 to U+10FFFF
if (char.IsHighSurrogate(ch) && i + 1 < length && char.IsLowSurrogate(value[i + 1]))
{
codePoint = char.ConvertToUtf32(ch, value[i + 1]);
charsConsumed = 2;
}
else
{
codePoint = ch;
}
// Check if character is valid according to XML 1.0 spec
var isValid =
codePoint == 0x9 ||
codePoint == 0xA ||
codePoint == 0xD ||
(codePoint >= 0x20 && codePoint <= 0xD7FF) ||
(codePoint >= 0xE000 && codePoint <= 0xFFFD) ||
(codePoint >= 0x10000 && codePoint <= 0x10FFFF);
if (isValid)
{
if (charsConsumed == 1)
{
builder.Append((char)codePoint);
}
else
{
// Append the original surrogate pair
builder.Append(ch);
builder.Append(value[i + 1]);
}
}
else
{
// Replace invalid character (or unpaired surrogate) with its hex representation
builder.Append($"[0x{codePoint:X}]");
}
if (charsConsumed == 2)
{
i++;
}

Copilot uses AI. Check for mistakes.
}

return builder.ToString();
}
Comment on lines +18 to +44
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SanitizeForXml method lacks dedicated unit tests. While integration tests exist in TUnit.TestProject, there should be unit tests in TUnit.Engine.Tests that directly test the XML sanitization logic with various edge cases (null, empty, valid control characters, invalid characters, surrogate pairs, boundary conditions). This would ensure the sanitization function works correctly in isolation.

Copilot uses AI. Check for mistakes.

public static string GenerateXml(
IEnumerable<TestNodeUpdateMessage> testUpdates,
string? filter)
Expand Down Expand Up @@ -49,7 +82,7 @@ public static string GenerateXml(

// <testsuites>
xmlWriter.WriteStartElement("testsuites");
xmlWriter.WriteAttributeString("name", assemblyName);
xmlWriter.WriteAttributeString("name", SanitizeForXml(assemblyName));
xmlWriter.WriteAttributeString("tests", summary.Total.ToString(CultureInfo.InvariantCulture));
xmlWriter.WriteAttributeString("failures", summary.Failures.ToString(CultureInfo.InvariantCulture));
xmlWriter.WriteAttributeString("errors", summary.Errors.ToString(CultureInfo.InvariantCulture));
Expand All @@ -75,14 +108,14 @@ private static void WriteTestSuite(
string? filter)
{
writer.WriteStartElement("testsuite");
writer.WriteAttributeString("name", assemblyName);
writer.WriteAttributeString("name", SanitizeForXml(assemblyName));
writer.WriteAttributeString("tests", summary.Total.ToString(CultureInfo.InvariantCulture));
writer.WriteAttributeString("failures", summary.Failures.ToString(CultureInfo.InvariantCulture));
writer.WriteAttributeString("errors", summary.Errors.ToString(CultureInfo.InvariantCulture));
writer.WriteAttributeString("skipped", summary.Skipped.ToString(CultureInfo.InvariantCulture));
writer.WriteAttributeString("time", summary.TotalTime.TotalSeconds.ToString("F3", CultureInfo.InvariantCulture));
writer.WriteAttributeString("timestamp", summary.Timestamp.ToString("o", CultureInfo.InvariantCulture));
writer.WriteAttributeString("hostname", Environment.MachineName);
writer.WriteAttributeString("hostname", SanitizeForXml(Environment.MachineName));

// Write properties
WriteProperties(writer, targetFramework, filter);
Expand All @@ -102,24 +135,24 @@ private static void WriteProperties(XmlWriter writer, string targetFramework, st

writer.WriteStartElement("property");
writer.WriteAttributeString("name", "framework");
writer.WriteAttributeString("value", targetFramework);
writer.WriteAttributeString("value", SanitizeForXml(targetFramework));
writer.WriteEndElement();

writer.WriteStartElement("property");
writer.WriteAttributeString("name", "platform");
writer.WriteAttributeString("value", RuntimeInformation.OSDescription);
writer.WriteAttributeString("value", SanitizeForXml(RuntimeInformation.OSDescription));
writer.WriteEndElement();

writer.WriteStartElement("property");
writer.WriteAttributeString("name", "runtime");
writer.WriteAttributeString("value", RuntimeInformation.FrameworkDescription);
writer.WriteAttributeString("value", SanitizeForXml(RuntimeInformation.FrameworkDescription));
writer.WriteEndElement();

if (!string.IsNullOrEmpty(filter))
{
writer.WriteStartElement("property");
writer.WriteAttributeString("name", "filter");
writer.WriteAttributeString("value", filter);
writer.WriteAttributeString("value", SanitizeForXml(filter));
writer.WriteEndElement();
}

Expand Down Expand Up @@ -152,8 +185,8 @@ private static void WriteTestCase(XmlWriter writer, TestNodeUpdateMessage test)

// Write testcase element
writer.WriteStartElement("testcase");
writer.WriteAttributeString("name", testName);
writer.WriteAttributeString("classname", className);
writer.WriteAttributeString("name", SanitizeForXml(testName));
writer.WriteAttributeString("classname", SanitizeForXml(className));
writer.WriteAttributeString("time", duration.TotalSeconds.ToString("F3", CultureInfo.InvariantCulture));

// Write state-specific child elements
Expand Down Expand Up @@ -198,16 +231,16 @@ private static void WriteFailure(XmlWriter writer, FailedTestNodeStateProperty f
var exception = failed.Exception;
if (exception != null)
{
writer.WriteAttributeString("message", exception.Message);
writer.WriteAttributeString("message", SanitizeForXml(exception.Message));
writer.WriteAttributeString("type", exception.GetType().FullName ?? "AssertionException");
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception type names from exception.GetType().FullName are written to XML without sanitization. While standard .NET type names won't contain invalid XML characters, custom exception types could theoretically have non-ASCII or problematic characters in their namespace or type names. For consistency and defensive programming, these should also be sanitized.

Copilot uses AI. Check for mistakes.
writer.WriteString(exception.ToString());
writer.WriteString(SanitizeForXml(exception.ToString()));
}
else
{
var message = failed.Explanation ?? "Test failed";
writer.WriteAttributeString("message", message);
writer.WriteAttributeString("message", SanitizeForXml(message));
writer.WriteAttributeString("type", "TestFailedException");
writer.WriteString(message);
writer.WriteString(SanitizeForXml(message));
}

writer.WriteEndElement(); // failure
Expand All @@ -220,16 +253,16 @@ private static void WriteError(XmlWriter writer, ErrorTestNodeStateProperty erro
var exception = error.Exception;
if (exception != null)
{
writer.WriteAttributeString("message", exception.Message);
writer.WriteAttributeString("message", SanitizeForXml(exception.Message));
writer.WriteAttributeString("type", exception.GetType().FullName ?? "Exception");
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception type names from exception.GetType().FullName are written to XML without sanitization. While standard .NET type names won't contain invalid XML characters, custom exception types could theoretically have non-ASCII or problematic characters in their namespace or type names. For consistency and defensive programming, these should also be sanitized.

Copilot uses AI. Check for mistakes.
writer.WriteString(exception.ToString());
writer.WriteString(SanitizeForXml(exception.ToString()));
}
else
{
var message = error.Explanation ?? "Test error";
writer.WriteAttributeString("message", message);
writer.WriteAttributeString("message", SanitizeForXml(message));
writer.WriteAttributeString("type", "TestErrorException");
writer.WriteString(message);
writer.WriteString(SanitizeForXml(message));
}

writer.WriteEndElement(); // error
Expand All @@ -239,9 +272,9 @@ private static void WriteTimeoutError(XmlWriter writer, TimeoutTestNodeStateProp
{
writer.WriteStartElement("error");
var message = timeout.Explanation ?? "Test timed out";
writer.WriteAttributeString("message", message);
writer.WriteAttributeString("message", SanitizeForXml(message));
writer.WriteAttributeString("type", "TimeoutException");
writer.WriteString(message);
writer.WriteString(SanitizeForXml(message));
writer.WriteEndElement(); // error
}

Expand All @@ -267,8 +300,8 @@ private static void WriteSkipped(XmlWriter writer, SkippedTestNodeStateProperty
{
writer.WriteStartElement("skipped");
var message = skipped.Explanation ?? "Test skipped";
writer.WriteAttributeString("message", message);
writer.WriteString(message);
writer.WriteAttributeString("message", SanitizeForXml(message));
writer.WriteString(SanitizeForXml(message));
writer.WriteEndElement(); // skipped
}

Expand Down
41 changes: 41 additions & 0 deletions TUnit.TestProject/JUnitReporterInvalidXmlCharacterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
using TUnit.Core;

namespace TUnit.TestProject;

public class JUnitReporterInvalidXmlCharacterTests
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test class is missing Category attributes. According to TUnit's testing approach, tests in TUnit.TestProject should be properly categorized (e.g., "Pass" or "Fail") to enable filtered test execution. The TestFailingWithInvalidCharacterInException test intentionally fails and should have a [Category("Fail")] attribute. The other tests that pass should have [Category("Pass")] attributes. This enables proper filtering when running the test suite.

Copilot uses AI. Check for mistakes.
{
[Test]
[Arguments("Some valid string")]
public async Task TestWithValidString(string parameter)
{
await Assert.That(parameter).IsNotNull();
}

[Test]
[Arguments("A string with an invalid \x04 character")]
public async Task TestWithInvalidXmlCharacter(string parameter)
{
await Assert.That(parameter).IsNotNull();
}

[Test]
[Arguments("Multiple\x01\x02\x03\x04invalid characters")]
public async Task TestWithMultipleInvalidXmlCharacters(string parameter)
{
await Assert.That(parameter).IsNotNull();
}

[Test]
[Arguments("Test\twith\nvalid\rcontrol characters")]
public async Task TestWithValidControlCharacters(string parameter)
{
await Assert.That(parameter).IsNotNull();
}

[Test]
public async Task TestFailingWithInvalidCharacterInException()
{
// This test intentionally fails with an exception message containing invalid XML characters
throw new InvalidOperationException("Error with invalid \x04 character in exception message");
}
}
Loading