Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch for non exclusive c14n #157

Merged
merged 9 commits into from
Sep 10, 2018

Conversation

Hinaser
Copy link
Contributor

@Hinaser Hinaser commented Jun 8, 2018

Summary

This is a patch proposal for issues regarding to non-exclusive c14n handling bug.
This PR contains README update and corresponding unit test specs.

Issues relating:
#113
#153

Note

It was so painful to write code to be compatible with Node.js 0.10.
Give me a praise... 😫

Optional Suggestion

As you know, there is a quick-patch in lib/signed-xml.js as below.

  //***workaround for validating windows mobile store signatures - it uses c14n but does not state it in the transforms
  if (!hasImplicitTransforms && transforms.length==1 && transforms[0]=="http://www.w3.org/2000/09/xmldsig#enveloped-signature")
    transforms.push("http://www.w3.org/2001/10/xml-exc-c14n#")

I found that there are a lot of signed XML in the world with implicit transforms.
Not limited to "validating windows mobile store signatures", sometimes developers need to guess
and apply those hidden implicit transform.

So I added option to specify such implicit transform like this:

new SignedXml(null, {implicitTransforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"]});

This new option is described in README in my PR.

Then this is a suggestion.

Can we eliminate the workaround code for windows mobile store signature?

I know there will be an breaking impact on current xml-crypto users, but it is healthier to specify such an implicit transform into an option.

Currently, my PR preserves that workaround code. So if you are worried to have impacts on existing users, you can safely ignore my suggestion.

Best regards.

@LoneRifle
Copy link
Collaborator

LoneRifle commented Sep 6, 2018

Duplicate of #161 , which incorporates this PR.

@LoneRifle LoneRifle merged commit 2450505 into node-saml:master Sep 10, 2018
@cjbarth cjbarth added the bug label Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants