fix: ignore unparsed entity logic for predefined xml entities

[SethFalco]

There was a bug in the implementation for resolving entities. Predefined XML entities like <, &, and " should not be interpreted, as this breaks parsing of the document.

For example, see the following XML document (an SVG) and render in the browser:

<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" [
<!ENTITY entity_reference "<text x='4' y='64'>entity reference</text>">
<!ENTITY escaped_entity_reference "&lt;text&gt;escaped entity reference&lt;/text&gt;">
]>
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 300 128">
  <text x="4" y="32">
    &lt;text&gt;escaped literal&lt;/text&gt;
  </text>
  &entity_reference;
  <text x="4" y="96">
    &escaped_entity_reference;
  </text>
</svg>

We should resolve custom entities like entity_reference and escaped_entity_reference. However, not &amp, <, or ", otherwise they get interpreted by the parser instead.

Currently, when parsing this XML document in sax v1.3.0 we get this error:

Error: Invalid character in tag name
Line: 6
Column: 14
Char: &
    at error (/home/seth/Documents/repos/maintainer/svg/svgo/node_modules/sax/lib/sax.js:652:10)
    at strictFail (/home/seth/Documents/repos/maintainer/svg/svgo/node_modules/sax/lib/sax.js:678:7)
    at SAXParser.write (/home/seth/Documents/repos/maintainer/svg/svgo/node_modules/sax/lib/sax.js:1284:17)

Related

• Closes Parse result of entity replacement #199
• Closes Parse result of custom entity replacement #200 (supersedes)
• Resolves bug introduced by Entity declarations with XML elements or other entities #260

---

Just an aside, if you'd like some help with maintaining sax, I'd love to lend a hand. We've been using this in SVGO, and since joining as a maintainer I've been meaning to move away from our (unmaintained) fork back to upstream. No pressure, and I'll see if I can open other pull requests in time.

If you'd accept it, I think it would be very valuable to have a task in CI that attempts to parse the XML Conformance Test Suites just to ensure no errors are thrown. That also would've caught this bug. ^-^'

https://www.w3.org/XML/Test/

Update

Since opening this pull request, I threw together a script for regression testing.

- name: Regression
  run: |
    wget https://www.w3.org/XML/Test/xmlts20130923.tar.gz
    tar -tf xmlts20130923.tar.gz | grep -E '^xmlconf/xmltest/valid/sa/[0-9]+\.xml$' > filter.txt
    tar -xf xmlts20130923.tar.gz -T filter.txt
    FAILURES=0
    for FILE in $(grep -REL xmlconf/ -e '<!ENTITY\s+(\S+)\s+"')
    do
      if [ $(file -bi $FILE | sed -e "s/.* charset=//") == 'utf-16le' ]; then echo "$(iconv -f utf-16le -t utf-8 $FILE)" > $FILE; fi
      node -e "require('./lib/sax').parser(true).write(require('fs').readFileSync('$FILE', { encoding: 'utf8' })).close();" \
        || echo "$FILE failed" && ((FAILURES+=1))
      node -e "require('./lib/sax').parser(true, { unparsedEntities: true }).write(require('fs').readFileSync('$FILE', { encoding: 'utf8' })).close();" \
        || echo "$FILE failed with { unparsedEntities: true }" && ((FAILURES=+1))
    done
    if [ FAILURES != 0 ]; then exit 1; fi

• Fetches the conformance test suite from W3C.
• Extracts only valid standalone XML documents. This task is only to ensure the parser doesn't throw exceptions on valid documents.
• Skips documents that declare custom entities, since the library doesn't handle that on its own.
• Re-encode utf-16le files as utf-8 so it's easy to read from Node.js.

This actually helped me identify a problem with my initial solution. I didn't account for the fact that " isn't the only way to insert quotes. One can do " instead, so now I'm comparing against Object.values(XML_ENTITIES) rather than checking if the entity string exists as a property of XML_ENTITIES.

[HeikoTheissen]

Looks good to me. I hadn't thought of this case when I suggested #260.

[SethFalco]

@isaacs Sorry to ping, but I was wondering if you had time to review my three pull requests to this repo? I'd be happy to sponsor $100 via GitHub Sponsors if we can get all three reviewed and hopefully merged/released?

• fix: ignore unparsed entity logic for predefined xml entities #266 (this one)
• fix: don't throw on comment in dtd #267
• feat: add explicit option for unquoted attribute values #268

These would all help close issues in SVGO, which depends on sax-js.