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.
This commit is contained in:
1 parent
c0a3e1d33a
commit
81abfb9f2e
3 files changed
+131
-1
No files matched your search
@@ -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/<tfm>\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.
|
||||
@@ -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
|
||||
|
||||
@@ -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));
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user