tests/NLog.UnitTests/LayoutRenderers/Wrappers/XmlEncodeTests.cs (1)

55-112: Good coverage for invalid XML characters, but consider adding a surrogate pair test.

The test validates that all non-surrogate, non-forbidden characters survive encoding. However, it explicitly skips surrogates (line 74–77), which means valid surrogate pairs (e.g., emoji like "\ud83d\ude00") are not tested. Given the surrogate handling issue in RemoveInvalidXmlIfNeeded, adding a test with valid surrogate pairs would be valuable to prevent regressions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/NLog.UnitTests/LayoutRenderers/Wrappers/XmlEncodeTests.cs` around lines
55 - 112, Add a new test (or extend XmlEncodeBadStringTest1) that includes valid
surrogate pairs to ensure RemoveInvalidXmlIfNeeded preserves them: create a
string containing known valid surrogate pair(s) (e.g., "\ud83d\ude00" or other
emoji), push it into the ScopeContext property "foo" and render with
SimpleLayout using "${xml-encode:${scopeproperty:foo}}" and
"${xml-encode:${scopeproperty:foo}:CDataEncode=true}", then assert the output is
non-empty and that the surrogate pair characters remain valid XML characters
(use System.Xml.XmlConvert.IsXmlChar on each resulting char or verify the
original codepoint survives). Reference the existing XmlEncodeBadStringTest1,
ScopeContext.PushProperty, SimpleLayout rendering, and the
RemoveInvalidXmlIfNeeded behavior when adding the assertions.

src/NLog/Internal/XmlHelper.cs (1)

176-194: String-based EscapeXmlWhenNeeded — minor dead code observation.

The else branch at line 187 (EscapeXmlString(text, xmlEncodeNewlines, startIndex, sb)) appears unreachable: we only enter the if (startIndex >= 0) block when text contains at least one XML escape char (<>&'"\r\n), all of which are valid XML chars. Therefore RemoveInvalidXmlIfNeeded will always append at least that char, ensuring orgLength != sb.Length.

Not a functional issue — just a note for code clarity.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NLog/Internal/XmlHelper.cs` around lines 176 - 194, The code in
EscapeXmlWhenNeeded has an unreachable else branch after
RemoveInvalidXmlIfNeeded: since startIndex >= 0 guarantees at least one
XML-escape character is present, RemoveInvalidXmlIfNeeded will always increase
sb.Length, so the branch that calls EscapeXmlString(text, xmlEncodeNewlines,
startIndex, sb) when orgLength == sb.Length is dead; simplify by removing that
else branch and always calling EscapeXmlWhenNeeded(sb, orgLength,
xmlEncodeNewlines) when startIndex >= 0 (or adjust the logic to only call
RemoveInvalidXmlIfNeeded and then process the sb), cleaning up the unused path
and related comments while keeping references to EscapeXmlWhenNeeded,
RemoveInvalidXmlIfNeeded and EscapeXmlString intact.

src/NLog/Internal/XmlHelper.cs (1)

176-194: The else branch (line 187) is unreachable when startIndex >= 0.

When startIndex >= 0, at least one XML escape char exists in text. Since all escape chars (<, >, &, ', ", \r, \n) are valid XML characters, RemoveInvalidXmlIfNeeded will always preserve at least one char in the builder, so orgLength != sb.Length is always true. The else branch calling EscapeXmlString directly (bypassing invalid-char removal) can never execute in this code path.

This doesn't cause a runtime issue, but it's misleading — a future reader might assume both branches are reachable and that the else path intentionally skips sanitization.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NLog/Internal/XmlHelper.cs` around lines 176 - 194, When startIndex >= 0
the branch that calls EscapeXmlString(...) is unreachable because
RemoveInvalidXmlIfNeeded(...) will always change sb.Length for inputs containing
escape chars; remove that misleading branch and simplify
EscapeXmlWhenNeeded(string,text,...) so that after creating sb and recording
orgLength you call RemoveInvalidXmlIfNeeded(text, sb) and then always invoke
EscapeXmlWhenNeeded(sb, orgLength, xmlEncodeNewlines) (dropping the
EscapeXmlString call), then return sb.ToString() or string.Empty as before;
update references to methods EscapeXmlWhenNeeded, RemoveInvalidXmlIfNeeded,
EscapeXmlString, sb, and orgLength accordingly.

src/NLog/Internal/XmlHelper.cs (2)

138-163: Recursive restart from startPos re-scans already-verified chars; pass i instead.

In the !XmlConvertIsXmlChar branch (Line 149), the recursive call passes startPos rather than i. Chars from startPos..i-1 were already confirmed to be valid XML chars that require no escaping, so re-scanning them is pure waste. Since XML special chars are single-char tokens (unlike the ]]> sliding window in EscapeCDataWhenNeeded, which legitimately needs an orgLength restart), it's safe to pass i here.

♻️ Proposed change

-            EscapeXmlWhenNeeded(builder, startPos, xmlEncodeNewlines, removedInvalidXmlChars: true);
+            EscapeXmlWhenNeeded(builder, i, xmlEncodeNewlines, removedInvalidXmlChars: true);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NLog/Internal/XmlHelper.cs` around lines 138 - 163, In
EscapeXmlWhenNeeded, the recursive restart in the branch that handles non-XML
chars incorrectly passes startPos causing previously-verified chars to be
re-scanned; change the recursive call EscapeXmlWhenNeeded(builder, startPos,
xmlEncodeNewlines, removedInvalidXmlChars: true) to pass the current index i
instead (EscapeXmlWhenNeeded(builder, i, xmlEncodeNewlines,
removedInvalidXmlChars: true)) so scanning resumes at the first unprocessed
character; locate this inside the !XmlConvertIsXmlChar branch where
RemoveInvalidXmlChars(text, builder) is called and update the parameter to i to
avoid redundant work.

---

165-188: Unnecessary O(n) append+truncate in the common (no-invalid-chars) path; fragile ReferenceEquals contract.

Two related points:

1. Double-scan cost: When startIndex >= 0 and the text contains no invalid XML chars (by far the common case for well-formed log messages), RemoveInvalidXmlChars(text, sb) eagerly appends the full text to sb (Line 172), only for sb.Length = orgLength to immediately undo it (Line 175) before EscapeXmlString does its own O(n) scan. A cheap HasInvalidXmlChars sentinel would save the redundant allocation/copy.

2. Fragile sentinel: Line 173 relies on RemoveInvalidXmlChars returning the exact same reference (ReferenceEquals) to signal "nothing was changed". This invariant isn't documented. If RemoveInvalidXmlChars is ever changed to return a copy even for clean input, the logic silently falls into the wrong branch. A brief inline comment would make this safe-to-maintain.

♻️ Minimal improvement: document the ReferenceEquals contract

+            // RemoveInvalidXmlChars returns the original reference when nothing was changed;
+            // ReferenceEquals is the sentinel for "no invalid chars found".
             var validText = RemoveInvalidXmlChars(text, sb);
             if (ReferenceEquals(validText, text))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NLog/Internal/XmlHelper.cs` around lines 165 - 188, EscapeXmlWhenNeeded
currently does an unnecessary append+truncate and relies on ReferenceEquals from
RemoveInvalidXmlChars to detect "no change"; implement an explicit check to
avoid the double O(n) work and the fragile contract: add or use a
HasInvalidXmlChars(text) sentinel (or change RemoveInvalidXmlChars to return a
bool/tuple) and in EscapeXmlWhenNeeded first check that sentinel when startIndex
>= 0 — if no invalid chars, call EscapeXmlString(text, xmlEncodeNewlines,
startIndex, sb) directly (no append/trim); if there are invalid chars, call
RemoveInvalidXmlChars(text, sb) and then EscapeXmlWhenNeeded(sb, orgLength,
xmlEncodeNewlines, removedInvalidXmlChars: true) as before; remove the
ReferenceEquals check and/or add a short comment documenting the intended
contract if you keep it.

tests/NLog.UnitTests/LayoutRenderers/Wrappers/XmlEncodeTests.cs (1)

105-115: XmlEncodeBadStringTest1 CDATA path should also validate CDATA structure.

The assertions only check that each output char is a valid XML char — which passes even when the CDATA wrapper is completely absent (because <, !, [, C, D, A, T, > are all valid XML chars). This means the critical bug in EscapeCData (missing <![CDATA[...]]> wrapper when input contains invalid XML chars) is not caught here.

✅ Suggested additional assertions

             var goodString = l.Render(LogEventInfo.CreateNullEvent());
             Assert.NotEmpty(goodString);
+            Assert.StartsWith("<![CDATA[", goodString);
+            Assert.EndsWith("]]>", goodString);
             foreach (char c in goodString)
             {
                 Assert.True(System.Xml.XmlConvert.IsXmlChar(c), $"Invalid char {Convert.ToInt32(c)} was not removed");
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/NLog.UnitTests/LayoutRenderers/Wrappers/XmlEncodeTests.cs` around lines
105 - 115, The test XmlEncodeBadStringTest1 currently only validates that output
chars are legal XML chars; update it to also assert that when CDataEncode=true
the rendered string is wrapped in a proper CDATA section and the inner content
is the escaped value: verify the rendered string begins with "<![CDATA[" and
ends with "]]>" (using the SimpleLayout l =
"${xml-encode:${scopeproperty:foo}:CDataEncode=true}" and the rendered
goodString), and additionally assert the inner substring (between the wrappers)
does not contain the forbidden sequence "]]>" (or that any original invalid
chars were properly escaped/removed by EscapeCData). Ensure these assertions run
inside the existing ScopeContext.PushProperty block that sets "foo" to badString
so the test fails if the CDATA wrapper is missing or malformed.

src/NLog/Internal/XmlHelper.cs (1)

138-163: Surrogate pairs take the expensive fallback path in the StringBuilder overload.

When removedInvalidXmlChars is false (which is the case for direct callers like XmlEncodeLayoutRendererWrapper and XmlAttribute), a valid high surrogate at line 144 fails XmlConvertIsXmlChar, entering the expensive extract→truncate→RemoveInvalidXmlChars→recurse path. RemoveInvalidXmlChars handles surrogate pairs correctly so correctness is preserved, but you could add a surrogate-pair short-circuit here (as done in RemoveInvalidXmlChars at line 99) to avoid the allocation for texts containing emoji/supplementary characters.

Optional: add surrogate-pair fast path

                 var chr = builder[i];
                 if (!XmlConvertIsXmlChar(chr) && !removedInvalidXmlChars)
                 {
+                    if (i + 1 < builderLength && XmlConvertIsXmlSurrogatePair(builder[i + 1], chr))
+                    {
+                        ++i;    // valid surrogate pair — skip low surrogate
+                        continue;
+                    }
                     var text = builder.ToString(i, builderLength - i);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NLog/Internal/XmlHelper.cs` around lines 138 - 163, EscapeXmlWhenNeeded
currently treats a valid high surrogate as invalid (calling the expensive
extract→RemoveInvalidXmlChars→recurse path) because XmlConvertIsXmlChar returns
false for lone surrogates; update EscapeXmlWhenNeeded to short‑circuit when
encountering a high surrogate followed by a low surrogate by detecting
char.IsHighSurrogate(chr) && i+1 < builderLength &&
char.IsLowSurrogate(builder[i+1]) and simply advance i past the pair (continue
loop) so surrogate pairs are handled cheaply; keep the existing behavior for
other invalid chars and still call RemoveInvalidXmlChars/ EscapeXmlString when
necessary, referencing the EscapeXmlWhenNeeded, XmlConvertIsXmlChar,
RemoveInvalidXmlChars and RequiresXmlEscape symbols.