Throw if multiple XML roots detected

[cjbarth]

Description

Based on the discussion at #191, it seems there is a better way to protect against XML files with multiple roots. This PR is to implement that.

[codecov]

Codecov Report

Merging #195 (dff48f0) into master (b850552) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
+ Coverage   80.75%   80.85%   +0.09%     
==========================================
  Files          11       11              
  Lines         816      820       +4     
  Branches      247      247              
==========================================
+ Hits          659      663       +4     
  Misses         71       71              
  Partials       86       86              

Impacted Files	Coverage Δ
src/saml.ts	78.26% <100.00%> (-0.16%)	⬇️
src/xml.ts	80.00% <100.00%> (+1.42%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[srd90]

In addition to hardening validatePostResponseAsync there are also at least these places which process input which might be altered before sent to snode-saml / passport-saml components (and processing involves @xmldom/xmldom meannig that multiroot XML documents are allowed if there aren't any additional checks implemented):

• validateRedirectAsync
  

  node-saml/src/saml.ts

  Line 853 in d539543

  const dom = parseDomFromString(inflated.toString());

• validatePostRequestAsync
  

  node-saml/src/saml.ts

  Line 1233 in d539543

  const dom = parseDomFromString(xml);

[cjbarth]

I've made some changes to take into considerations @srd90's latest comments about how other parts of the code could use the same XML validation.

[markstos]

I agree that throwing if we detect multiple SAML roots is a stronger approach than our first attempt.

I saw that parseDomFromString switched to an async method that returned a promise. I reviewed the docs to see if this was a public method and thus a breaking change, but it appears that it was not a public method and this not a breaking change.

[frumioj]

I think this is the best place for you to make this specific change, and it looks correct to me at least reading the code. Thanks for doing this.

The only thing which could be stronger would be to not even parse as far as the end of the AuthnResponse, and return an authentication error when the status is read (earlier than the end of the response) but I'm guessing this would be harder because the parse strategy would have to change (SAX interface vs DOM).

I will try to look at whether that is even possible or a good idea.

The other thing I'll note is that I submitted a similar comment to the security list for the xmldom package, and they may change their behaviour to error/throw when multiple root nodes are discovered. I don't think that will change this approach, but it might make it unnecessary.

[frumioj]

The other thing I'll note is that I submitted a similar comment to the security list for the xmldom package, and they may change their behaviour to error/throw when multiple root nodes are discovered. I don't think that will change this approach, but it might make it unnecessary.

GHSA-crh6-fp67-6883