Skip to content

Conversation

@cmordue
Copy link

@cmordue cmordue commented Dec 13, 2017

Remove namespace-breaking reserialization of signature which used to be in the documented example from xml-crypto but was removed due to this bug

See: node-saml/xml-crypto#105

This is a fix for the example signature validation documentation; the toString() call on the signature node turns out to be harmful, as it removes namespace metadata which would otherwise propagate from the parent document. In situations where the namespace is ds, this works out ok (as ds is provided as a default) but when using other strings for this namespace the example boilerplate fails.

In the example below, the definition of dsig would be undefined in a toString-orphaned signature, and the canonicalization algorithm would resolve the url as an empty string -- changing the underlying canonical text and causing the SignatureValue verification to fail.

PR fixes this in the example, which realistically will get used as boilerplate in many integrations of this library. Having just spent three days chasing down the cause of one of our SAML integration failures, I think its a useful change.

…efined on Signature node.

Remove namespace-breaking reserialization of signature which used to be in the documented example from xml-crypto but was removed due to this bug
See: node-saml/xml-crypto#105
@chrisgarber
Copy link

Can we get this PR merged? This is causing errors for some saml implementations at our org

chrisgarber added a commit to abacuslabs/saml20 that referenced this pull request Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants