Repository navigation
fix(parse): remove quadratic parsing paths and prototype-key corruption - #22
Conversation
- Scan attributes linearly instead of with a regex that backtracked from every offset in a long run of name characters - Flush only newly seen text at each '<' instead of re-trimming and re-decoding the whole accumulated text, which also stops entities being decoded twice - Store element and attribute names such as __proto__, constructor and toString as own data properties, and detect duplicates with Object.hasOwn
paustint
left a comment
paustint
left a comment
There was a problem hiding this comment.
🟢 Ready to approve
No issues found at confidence >= 8. The three fixes in src/parse.ts match what the PR and changelog describe, and I found no regression in the paths I traced.
Reviewed: 8554e88 — 3 files, +151/-29. Focus: the linear attribute scanner against the regex it replaces, the pending-text flush against the old flush-at-every-< behavior, and the setOwn / Object.hasOwn handling of prototype-member keys.
Checked and clean: attribute scanner accepts the same pairs as the old regex (a failed name fails identically from every suffix, so resuming after the name is equivalent; unterminated quotes, valueless names, unquoted values and a==b="c" all behave the same); text trim/decode semantics are unchanged except for the documented double-decode fix; leaf, mixed-content (#text), CDATA and trimValues: false paths; setOwn covers every assignment of a document-controlled key (child names, attribute names, #text); no existing review threads to dedupe; changelog heading classifies as a patch bump and the repo's formatting and commit conventions are followed. This was a static review: nothing was installed, built or run.
🤖 Final-gate review via Claude Code /pr-gatekeeper
Summary
Fixes three issues in
src/parse.tsfound by a security scan of the repo. All three are reachable by anything that passes untrusted XML toparse()./([^\s=]+)\s*=\s*(?:"…"|'…')/gretried from every offset inside a long run of name characters with no=. A tag of about 100 KB could block the event loop. Attributes are now read with one linear scan that accepts the samename = "value"/name = 'value'pairs.<, the whole accumulated element text was re-trimmed and re-entity-decoded, so long text interrupted by many comments, PIs, CDATA sections or child tags cost O(text × markers). Only newly seen text is processed now.__proto__,constructorandtoStringswapped the result's prototype, dropped the value, or wrapped the inherited member in an array (key in parentis true for inherited names). Keys are now stored withObject.defineProperty, and duplicates are detected withObject.hasOwn. Result objects keep the normal prototype, so consumers callingresult.hasOwnProperty(...)are unaffected.Behaviour change to review
Text that contained an entity-looking sequence after decoding was decoded twice if a
<followed it. For example,<a>&lt;<!---->x</a>gave<x, but<a>&lt;x</a>gave<x. Both now give<x. This is noted under Fixed in the changelog.A second, very narrow difference: when a numeric entity decodes to whitespace (such as
 ) between two comments, that space is now kept.Testing
=, single and double quotes, valueless names, unquoted values).main(the long-tag test takes about 27 s there) and pass with this change.npm run format:check,npm run lint,npm run typecheckandnpm test(127 tests) pass.[Unreleased]classifies as a patch bump.