dualfroz
81abfb9f2e
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.
102 lines
3.0 KiB
Markdown
102 lines
3.0 KiB
Markdown
# 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.
|