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

fix: update goxmldsig to v1.2.0 #437

Closed
wants to merge 1 commit into from
Closed

Conversation

lucasbbb
Copy link

@lucasbbb lucasbbb commented Jun 8, 2022

This PR updates the github.com/russellhaering/goxmldsig v1.1.1 => v1.2.0.

err := etreeutils.TransformExcC14n(el, canonicalizerPrefixList, false)
if err != nil {
	panic(err)
}

This issue #436 has existed since #428.

Change-Id: Ie5508caf073f74db314329cfc9ecd679954efbb6
@crewjam
Copy link
Owner

crewjam commented Jun 25, 2022

Bumping to version 1.2.0 pulls in this commit which changes how comments are handled.

This causes the TestSPRejectsInjectedComment test to fail with both comments enabled and disabled. That test was added as a regression to a security issue. I don't have my head around this right now, but I don't think we can bump to v1.2.0. I'm going to revert the dependabot-generated version bump for now.

@crewjam crewjam added the close_wait plan to close the issue after a respectable interval of inactivity label Jun 25, 2022
@greenpau
Copy link
Sponsor Contributor

The issue is that the replacement is no longer valid:

y := strings.Replace(string(x), "ross@octolabs.io", "ross@octolabs.io<!-- and a comment -->.example.com", 1)

because the string in saml2:NameID has changed.

<saml2:NameID>ross@<!-- and a comment -->octolabs.io</saml2:NameID>

For the test to pass, the replacement should now be:

y := strings.Replace(string(x), "ross@<!-- and a comment -->octolabs.io", "ross@octolabs.io<!-- and a comment -->.example.com", 1)

@greenpau
Copy link
Sponsor Contributor

Once #478 is merged, this one will be resolved too, because the upgrade is included.

@crewjam
Copy link
Owner

crewjam commented Dec 31, 2022

Thanks!!!

@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
close_wait plan to close the issue after a respectable interval of inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants