Merge changes from datagovsg fork #161
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Update Asana/xml-crypto with upstream changes
Patch for non exclusive c14n
- removed an old (~5y ago) hack for windows store XML canonicalization (and removed the test) - fixed an issue with over-eager prefix inclusion in non-exclusive c14n algorithm (+ test) - replaced a test for empty-URI that is impossible to update (because the private key is missing)
…he output of transforms returns a node-set rather than an octet stream, which is whenever there are no transforms or the last transform is enveloped signature. Add back test for when canonicalization method is not specified in transforms
…tion Fix default canonicalization Summary: According to the Digest Method Spec [1], the DigestMethod takes a octet stream, ad if the output of the URI dereference is a node set instead, it needs to be converted as described in the Reference Processing Spec [2] to an octet stream. The Reference Processing Spec says to use Canonical XML. Enveloped Signature returns a node-set, as does dereferencing a same-document reference [3], [4]. Canonicalization algorithms are the only other transforms we support, and they all return an octet stream, so the only times we need to add canonicalization are if we haven't done any transforms or if the last one is enveloped signature. https://www.w3.org/TR/xmldsig-core1/#sec-DigestMethod https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel https://www.w3.org/TR/xmldsig-core1/#sec-EnvelopedSignature https://www.w3.org/TR/xmldsig-core1/#sec-Same-Document Tests: Added back the test from when this was implemented slightly differently for Windows Mobile, checked against a SamlResponse without a canonicalization method in the transforms Reviewer: @srir
212200b changed the xpath to match all signature nodes descended from the specified local node. This is incorrect; the relevant spec says that only immediate signature elements should be removed This commit corrects the behaviour simply by changing the xpath to only match immediate children Fixes node-saml#135 , where further discussion can be found
|
Hi, @LoneRifle added you as collaborator. |
|
I intend to make one more edit to Thanks for having me look into this! |
|
what's your npm username? |
|
it's |
|
done! can you bump a major version before publishing? given the amount of changes seems safer. also there is a github notification on a vulnerable dependency in the current source (hapijs / hoek). |
|
Sure, will do! I'll look into the vuln, bumping the major release, and possibly #160 |
|
thanks, you do awesome work! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fork currently embodies fixes that addresses the following issues:
Fixes #113
Fixes #153
Fixes #158
Fixes #135
These are currently held in the datagovsg fork of xml-crypto, itself a fork of the Asana fork of the original.
I would also like to take the opportunity to request access to the repo so that I can merge changes, and access to the npm package so that I can cut releases for this library.