Infinite-loop error

[natlibfi-arlehiko]

When passing an object to DOMParser's parseFromString you'll get infinetely looping error:

const {DOMParser} = require('xmldom');
new DOMParser().parseFromString({});

Obviously you shouldn't pass an object there, but still this should be fixed.

[brodybits]

Adding help wanted & needs-investigation labels. Maybe related to these:

• issue Parser locks up on long input lines #97
• PR Replace regex with an equivalent using String.indexOf() #43
• issue replace lib/sax.js? #55 (replace lib/sax.js, ideally with a better known & tested SAX parser)

[jesse-y]

Hi @brodybits

I've taken a look at this to see where some potential issues with input sanitation might lie.

The XMLReader.parse() method within lib/dom-parser.js runs its logic in a perpetual while loop - while (true) { ... }. The exit condition for this loop is when the parser is unable to find an opening tag <. This is determined by using native string methods (source.indexOf('<'), so when a different data type is passed through (A POJO in this case) the while loop runs into TypeError: source.indexOf is not a function errors and hits the catch block before it can reach the exiting logic.

The catch block doesn't explicitly rethrow the error - it instead defers to an errorHandler that it's provided. When DOMParser is not provided an errorHandler option, it defaults to the handler from DOMHandler that it constructed internally in DOMParser.parseFromString().

DOMHandler has three methods, warning, error, and fatalError - only fatalError actually throws - the others just log to the console. XMLReader.parse() will only call error when it encounters a problem. Interestingly, the comments in parseElementStartPart() mention "fatalError" - so it seems like there was an intent for the catch block to throw fatal errors.

DOMParser.parseFromString() only does rudimentary checks - it will at most confirm that the source provided is truthly, but won't guarantee that it's also a string, which XMLReader.parse() implicitly depends on.

---

It's possible to get the parser to bail out early - passing an errorHandler implementation should do the trick.

const { DOMParser } = require('xmldom');

try {
  const parser = new DOMParser({
    errorHandler: {
      error: errorMessage => {
        throw new Error(errorMessage);
      }
    }
  });
  parser.parseFromString({});
} catch (error) {
  console.error(`Caught something!`, error);
}

---

In terms of fixing this, perhaps:

• Update DOMParser.parseFromString() to explicitly check that its source parameter is a string
• Update XMLReader.parse() to cast its source parameter to a string before attempting to process it
• Update DOMHandler.error() to throw an error, or change XMLReader.parse() to call fatalError() in the errorHandler it's provided.

[karfau]

Wow, great research and findings!
I would even argue that ensuring the input argument is a string could be treated as a non breaking change, since it's the only intended behavior.

Looking closer at the whole loop and error handling might also help us to fix other bugs.
Thx a lot.

[jesse-y]

Cheers - I'd be happy to have a go at fixing this, would you guys be alright with me raising a PR?

[brodybits]

A PR would be much appreciated, thanks.