fix(decode): prevent 11-bit consumed overflow for overlong numeric entities#2271
fix(decode): prevent 11-bit consumed overflow for overlong numeric entities#2271fb55 wants to merge 1 commit into
Conversation
…tities parseNumericEntity packs its result as (consumed << 21) | codepoint, giving consumed 11 bits. Numeric entities of 2048+ characters wrapped the shift mod 2^32, corrupting the advance past the entity: decodeHTML could swallow ~2K characters of input or re-emit entity bodies as text, and the strict/XML flavors silently failed to decode instead of emitting U+FFFD. Saturate the packed consumed at 0x7ff and have callers recompute the true length from the input via numericEntityLength() when they see the saturated value. The hot path keeps the packed fast path (one predictable compare); the recompute only runs for pathological 2047+ character entities. Also clamp the streaming EntityDecoder's numeric accumulators to 0x110000 (1 past the Unicode max) so both parsers clamp identically — previously the streaming path relied on float saturation and could pass Infinity to the errors callback — and cross-reference the sync and streaming numeric parsers so fixes are mirrored. Expected decode results for the boundary matrix (2044-4096 digit bodies, decimal/hex, with/without semicolon, all four decode functions) are verified against entities@7.0.1.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes an overflow bug in the non-streaming numeric-entity decoding path caused by packing the consumed-length into an 11-bit field, which could wrap for extremely long numeric entities and corrupt how far the decoder advances in the input.
Changes:
- Saturate the packed
consumedfield inparseNumericEntityand add a cold-pathnumericEntityLength()helper to recompute true entity length when saturation occurs. - Update both sync decode call sites (
decodeWithTrie,decodeXML) to detect saturation and recompute length before advancing. - Clamp streaming
EntityDecodernumeric accumulators to0x110000(1 past Unicode max) to avoid passingInfinityinto validation callbacks, and add targeted regression/boundary tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/decode.ts | Prevents packed-length wrap by saturating consumed and recomputing true length on the cold path; clamps streaming numeric accumulation. |
| src/decode.spec.ts | Adds a boundary matrix and regressions to ensure correct advancement/behavior for overlong numeric entities across sync + streaming implementations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The bug
parseNumericEntityinsrc/decode.tspacks its result as(consumed << 21) | codepoint, givingconsumed11 bits. Numeric entities with bodies of ~2046+ digits overflow the field and wrap the shift mod 2^32, corrupting the advance past the entity. Unreleased — introduced by #2199, no published version is affected. (First divergence from spec-correct behavior: 2046 decimal / 2045 hex digits.)Repros against
entities@7.0.1as ground truth:decodeHTMLStrict/decodeXMLsilently fail to decode such entities instead of emittingU+FFFD. The nearby comment claimed the packing could never change emitted characters — the leading-zeros case shows it can.The streaming
EntityDecoderpath was not affected (itsconsumedis a plain field).The fix
parseNumericEntitysaturates the packedconsumedatNUMERIC_CONSUMED_MAX(0x7ff) instead of letting it wrap.decodeWithTrie,decodeXML) recompute the true length from the input via a newnumericEntityLength()helper when they see the saturated value — a cold path that only runs for pathological 2047+ character entities. The hot path keeps the packed-integer trick and gains just one predictable compare.EntityDecodernumeric accumulators (stateNumericDecimal/stateNumericHex) now clamp to0x110000(1 past the Unicode max) exactly like the sync parser. Previously they relied on float saturation — harmless for output, but could passInfinityto thevalidateNumericCharacterReferenceerrors callback.Tests
Boundary matrix: bodies of 2044/2045/2046/2047/2048/4096 digits × decimal/hex × with/without semicolon, asserted for all four decode functions (
decodeHTML,decodeHTMLStrict,decodeHTMLAttribute,decodeXML), running through the existing sync + streaming (all-at-once and 1-char chunks)describe.eachharness so the streamingEntityDecoderis asserted to agree with the sync results. Expected strings verified againstentities@7.0.1. Plus regression tests for both repros above and for the streaming error-callback clamp.Benchmark
Numeric-entity-heavy corpus
"é𝒳text".repeat(50_000), fresh process per run, 100 iterations after warmup, min of 7 interleaved base/fix pairs:decodeHTMLdecodeXMLWithin run-to-run noise.
Note: #2248 rewrites
parseNumericEntityand receives an equivalent fix separately; conflicts between the two are expected.