From 81abfb9f2e9d4f645b48eba43730980cce47f706 Mon Sep 17 00:00:00 2001 From: dualfroz Date: Fri, 4 Sep 2026 09:42:55 +0200 Subject: [PATCH] Fix HtmlEntity.DeEntitize duplicating '#' on invalid numeric entities When a numeric character reference (&#...;) fails to parse, the catch block restored the text as "&#" + e where e already begins with '#', producing a doubled '#' (for example "&#foo;" became "&##foo;"). Restore the original text as "&" + e + ";" to match the named-entity fallback and keep DeEntitize idempotent for unparseable numeric entities. --- PR.md | 101 ++++++++++++++++++ src/HtmlAgilityPack.Shared/HtmlEntity.cs | 5 +- .../HtmlEntityDeEntitizeTests.cs | 26 +++++ 3 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 PR.md create mode 100644 src/Tests/HtmlAgilityPack.Tests.NetStandard2_0/HtmlEntityDeEntitizeTests.cs diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..bebdb64 --- /dev/null +++ b/PR.md @@ -0,0 +1,101 @@ +# Fix HtmlEntity.DeEntitize duplicating '#' on invalid numeric entities + +## Problem + +`HtmlEntity.DeEntitize` corrupts text that contains an invalid numeric character +reference. Instead of leaving an unparseable `&#...;` sequence untouched, it inserts a +spurious second `#`: + +```csharp +HtmlEntity.DeEntitize("&#foo;"); // returns "&##foo;" (expected "&#foo;") +HtmlEntity.DeEntitize("&#xGG;"); // returns "&##xGG;" (expected "&#xGG;") +HtmlEntity.DeEntitize("&#;"); // returns "&##;" (expected "&#;") +``` + +Valid references (`A` -> `A`) and named entities (`&` -> `&`) are handled +correctly; only the invalid numeric fallback is wrong. This makes `DeEntitize` +non-idempotent and can silently damage document text. + +## Root cause + +`src/HtmlAgilityPack.Shared/HtmlEntity.cs`, in `DeEntitize`. + +When a numeric entity is detected (`entity[0] == '#'`), the code copies the whole +entity body, including the leading `#`, into `e`: + +```csharp +string e = entity.ToString(); // e == "#foo" +... +int code = Convert.ToInt32(codeStr, fromBase); +sb.Append(Convert.ToChar(code)); +``` + +If parsing or conversion throws (non-numeric digits, invalid hex, out-of-range code +point, etc.), the catch block tries to restore the original text but prepends `"&#"` +to a value that already starts with `#`: + +```csharp +catch +{ + sb.Append("&#" + e + ";"); // "&#" + "#foo" + ";" == "&##foo;" +} +``` + +The named-entity fallback a few lines below already restores the text correctly with +`sb.Append("&" + entity + ";")`; the numeric branch should do the same because `e` +already contains the `#`. + +## Fix + +```csharp +catch +{ + // 'e' already starts with '#', so restore the original text as + // "&" + e + ";" (e.g. "&#foo;"). Prepending "&#" duplicates the '#'. + sb.Append("&" + e + ";"); +} +``` + +## Test + +`src/Tests/HtmlAgilityPack.Tests.NetStandard2_0/HtmlEntityDeEntitizeTests.cs`: +`DeEntitize_HandlesInvalidNumericEntitiesWithoutDuplicatingHash` covers invalid numeric +entities (`&#foo;`, `&#xGG;`, `&#;`, and one embedded in surrounding text), plus valid +numeric (`A`, `A`) and named (`&`, `¬real;`) cases as regression guards. + +## Verification + +Target framework built and tested: `net6.0`. + +New test, before the fix: + +``` +Failed! - Failed: 4, Passed: 4, Total: 8 +Assert.Equal() Failure +Expected: &#foo; +Actual: &##foo; +``` + +New test, after the fix: + +``` +Passed! - Failed: 0, Passed: 8, Skipped: 0, Total: 8 +``` + +Full test project after the fix: + +``` +Failed! - Failed: 1, Passed: 48, Skipped: 0, Total: 49 +``` + +The single failure is pre-existing and unrelated: `PreserveOriginalQuoteTest` loads a +fixture using a hard-coded Windows path separator (`bin/Debug/\files\...`) and +fails with `DirectoryNotFoundException` on non-Windows file systems, independent of this +change. + +## Flags for the account owner + +The test project (`HtmlAgilityPack.Tests.NetStandard2_0`) targets `netcoreapp3.1` +upstream. It was retargeted to `net6.0` locally only for running the gate (that local +change is not part of this branch); the fix and the added test are framework +independent. diff --git a/src/HtmlAgilityPack.Shared/HtmlEntity.cs b/src/HtmlAgilityPack.Shared/HtmlEntity.cs index 52cea1b..a928c2b 100644 --- a/src/HtmlAgilityPack.Shared/HtmlEntity.cs +++ b/src/HtmlAgilityPack.Shared/HtmlEntity.cs @@ -663,7 +663,10 @@ namespace HtmlAgilityPack } catch { - sb.Append("&#" + e + ";"); + // 'e' already starts with '#', so restore the + // original text as "&" + e + ";" (e.g. "&#foo;"). + // Prepending "&#" here would duplicate the '#'. + sb.Append("&" + e + ";"); } } else diff --git a/src/Tests/HtmlAgilityPack.Tests.NetStandard2_0/HtmlEntityDeEntitizeTests.cs b/src/Tests/HtmlAgilityPack.Tests.NetStandard2_0/HtmlEntityDeEntitizeTests.cs new file mode 100644 index 0000000..3ef6525 --- /dev/null +++ b/src/Tests/HtmlAgilityPack.Tests.NetStandard2_0/HtmlEntityDeEntitizeTests.cs @@ -0,0 +1,26 @@ +using Xunit; + +namespace HtmlAgilityPack.Tests.NetStandard2_0 +{ + public class HtmlEntityDeEntitizeTests + { + [Theory] + // Invalid numeric entities that still reach the terminating ';' must be left + // untouched. They used to be corrupted with a duplicated '#', e.g. "&#foo;" + // became "&##foo;". + [InlineData("&#foo;", "&#foo;")] + [InlineData("&#xGG;", "&#xGG;")] + [InlineData("&#;", "&#;")] + [InlineData("prefix &#foo; suffix", "prefix &#foo; suffix")] + // Valid numeric entities keep decoding correctly. + [InlineData("A", "A")] + [InlineData("A", "A")] + // Named entities are unaffected. + [InlineData("&", "&")] + [InlineData("¬real;", "¬real;")] + public void DeEntitize_HandlesInvalidNumericEntitiesWithoutDuplicatingHash(string input, string expected) + { + Assert.Equal(expected, HtmlEntity.DeEntitize(input)); + } + } +}