Xml enc c14# inclusivenamespace fixes

[tunetheweb]

This is a more generic fix than #172 which had several issues in my opinion:

1. It hardcoded in SAML name spaces so didn't work with other XML types with similar issue
2. It returned true after the first valid digest was encountered so messages with multiple digests were not properly checked.
3. It seemed to check the digest 3 times which is a bit overkill.

This pull request refactors the code to make it more generic meaning it should fix the original issue (#72) but also #94, #145 and also #92. It would be good if all of those who raised this could confirm if this does fix their issues.

I also added a test case for this for a SOAP message from SOAP-UI which likes to use these extra namespaces (my original problem and why I started looking at this!).

There's a bit of refactoring in this and I didn't see why ancestors were only being calculated for non-exclusive canonicalization when this problem shows they are also needed for exclusive. Perhaps I have missed something here? All the test cases still pass (including the new one) with this fix but not sure how comprehensive they are so would be good to get others input on this.

Let me know your thoughts,
Barry

---

Fixes #92
Fixes #94
Fixes #72
Fixes #145

[LoneRifle]

Hey @bazzadp , thanks for the PR. Apologies for not being able to review this in good time due to other commitments.

To answer your question - #163 was originally filed to address ancestors used in canonicalization, but caused issue #167 and was hence backed out. I'm hoping that what you have done has properly fixed this, and will review in good time!

If you wish, I can also give you publishing rights to this package so that there is at least one other person who can do so.

[tunetheweb]

Hi @LoneRifle thanks for getting back.

Will have a more detailed look at #163 but from a (very brief!) look it seems to do a lot of the same thing, but does it in exclusive-canonicalization.js rather than where I've done it in signed-xml.js. To be honest it probably does more belong in with the canonicalization code, than signed-xml code, so maybe I should refactor the code before you accept it? Or if this works, do we go with it for now, and refactor later?

Do we understand why that previous commit failed and does a test case exist to confirm if my code has the same issue?

If you're looking for help to maintain this package then I can possibly give some time, but definitely not approving my own pull request as need a second pair of eyes on it :-)

[LoneRifle]

I've not been able to look further into the problem, and unfortunately a test case that identifies the problem does not exist. Happy for you to refactor as you deem fit. I'll certainly look into the PR as a matter of responsibility, we shouldn't be approving our own PRs =)

[tunetheweb]

@LoneRifle ok the good news is the #163 breaks the test suite for two tests:

1. My new test (valid_signature_with_unused_prefixes.xml)
2. "signer adds existing prefixes" - this was added with Accept existing xml prefixes to avoid adding to signature #171 so didn't exist when Bugfix: a namespace in the inclusive namespace list should be treated… #163 was originally submitted.

My change passes the test suite, so that gives me greater confidence that my test hopefully won't have the same issue. However, without a test case for that it's difficult to be sure though. Have asked for help on #167 to reduce likelihood of issues.

I also looked at refactoring the code to move the actual setting of the top level namespaces to the canonicalization code but not sure it gains us much. Will keep digging though.

[LoneRifle]

Cool. I think there is enough information in #167 to create a test case, which I would try to do this or next weekend, and then we'll see where we go from there.

[tunetheweb]

@LoneRifle I've gone ahead with that refactor after all. It makes the changes a good bit smaller and cleaner. And also means I don't need to change c14nOptions which caused the problems before. Will leave you to review when you get a chance.

[LoneRifle]

lgtm otherwise - one question to answer, one nitpick

[LoneRifle]

Turns out there wasn't quite enough information to construct a test case that displays the behaviour that was reported #167 . Without people being forthcoming with a self-contained test case of our own, I would like to proceed with merging this PR once my review is addressed

[LoneRifle]

Lgtm. Nice work, thanks so much for your contribution!

[LoneRifle]

Let me know if you want npm maintainer rights by replying with your npm login name. I'll publish this when next convenient.

[mrcranky]

To answer your question - #163 was originally filed to address ancestors used in canonicalization, but caused issue #167 and was hence backed out. I'm hoping that what you have done has properly fixed this, and will review in good time!

Just to follow up on this (I work with @gcharnock), #163 was raised because we see differing behaviour between xml-crypto and other libraries in relation to the specific signed SAML assertions we get from a third party. I've checked again with 1.4.0 and it definitely doesn't validate those assertions properly, even with this fix in. 1.0.2 works for us but obviously not for others. Right now we've forked the SAML libraries in use and pinned them to xml-crypto@1.0.2 but that's not ideal.

Unfortunately I can't share one of those assertions here as a test case because the third party in question is sensitive about that, if there is any advice anyone can give me for generating a sample XML doc that shows the problem? I'd like to get these assertions in as a test case to illustrate the issue so that subsequent fixes can try to address both #163 without breaking the other documents.

[tunetheweb]

Well this has since been merged and a test case has been added for my issue. So if you can fix it in the latest branch and it still passes the test cases then I’d be reasonably confident you shouldn’t break my usage and you can submit a pull request.

Of course without a test case you’re able to publicly share its difficult to help suggest a fix. Also even if you fix it and submit a pull request but don’t add a test case for your scenario, then no guarantee it won’t break in future.

If you look at the existing test cases, and coupled with what you know privately about your issue, then you might get some idea of how to create a test case?