Codecov Report

Merging #221 (c829eb3) into master (939879d) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
- Coverage   80.70%   80.68%   -0.03%     
==========================================
  Files          11       11              
  Lines         819      818       -1     
  Branches      251      251              
==========================================
- Hits          661      660       -1     
  Misses         68       68              
  Partials       90       90              

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

So, the presumption here is that the downstream signature validation will cover this case and we shouldn't be normalizing the signature at all. Correct?

We should be normalizing the signature (and only the signature) we provide through loadSignature, all paths that lead to this call appears to provide a node that comes out of parseDomFromString, so it is already normalized. This way we can keep fullXml the way it is. I guess another way to do it if you wanted to keep a string would be to normalize both \r and XML entities, but since you already have a normalized node, that seems like unnecessary work.

Now that we have a test to make sure things continue working, we'll know if something else changes.

Hey @cjbarth, any chance we can get these changes published? I am working on moving some projects from passport-saml to @node-saml/node-saml and this is currently the only problem we have found so far. I was able to locally verify these changes and they work for our integrations ;)

Released @jonathansamines .

awesome, thank you @cjbarth

@cjbarth Great to see that you seem to have solved this problem! I noticed version 4.0.2 seems to be missing from npm, though: https://www.npmjs.com/package/@node-saml/passport-saml?activeTab=versions. Maybe you just haven't had time to publish it or something, but just letting you know 👍

He doesn't need to, the semver range is perfectly appropriate.