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

chore: update github.com/russellhaering/goxmldsig to v1.2.0 #450

Closed
wants to merge 1 commit into from

Conversation

zinic
Copy link

@zinic zinic commented Jul 27, 2022

This change set slightly duplicates earlier submissions but with test fixes and updates: #437 and #446

I ran into this in another project that consumes both github.com/crewjam/saml and github.com/russellhaering/goxmldsig at the top-level. The call signature for github.com/russellhaering/goxmldsig/etreeutils.TransformExcC14n(...) changed slightly and now accepts a boolean parameter that controls stripping out comments from the element's children.

There was a small patch required in schema.go for the receiver func (a *Assertion) Element() *etree.Element to complete the dependency update. Patching up the receiver function to additionally pass false to TransformExcC14n(...) preserves the function's previous behavior.

@zinic
Copy link
Author

zinic commented Jul 27, 2022

So I did some additional digging for the test case that's failing and I had a few notes.

The test case TestSPRejectsInjectedComment no longer passes because it expects github.com/russellhaering/goxmldsig not to strip comments for the following CanonicalizationMethod:
<CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#" />.

For the latest version of github.com/russellhaering/goxmldsig the library now switches on namespace in validate.go.

I'll update the test fixture.

…e TransformExcC14n call to preserve previous behavior
@zinic
Copy link
Author

zinic commented Jul 28, 2022

I've updated the test case with the following note:

// Note: The latest version of github.com/russellhaering/goxmldsig now correctly reads and
// switches on the CanonicalizationMethod presented in the assertion. The test assertion
// currently has the CanonicalizationMethod set to "http://www.w3.org/2001/10/xml-exc-c14n#"
// meaning the above comment injection will now be correctly stripped

Let me know if this is acceptable. Regenerating the test data for this case proved to be difficult, though with some additional effort not impossible.

@greenpau
Copy link
Sponsor Contributor

greenpau commented Aug 20, 2022

@zinic , running into the same issue 😞

BUILD| github.com/crewjam/saml
     | ../../../../pkg/mod/github.com/crewjam/saml@v0.4.8/schema.go:767:41: not enough arguments in call to etreeutils.TransformExcC14n
     |  have (*etree.Element, string)
     |  want (*etree.Element, string, bool)

@ebarendt
Copy link

@crewjam this is one thing preventing us from upgrading from a very old version. Any chance of getting it merged?

@crewjam
Copy link
Owner

crewjam commented Jan 12, 2023

fixed in #478

@crewjam crewjam closed this Jan 12, 2023
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.

None yet

4 participants