Skip to content

Accept DOM node when computing/verifying signatures #492

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

elbuo8
Copy link

@elbuo8 elbuo8 commented Apr 7, 2025

In order to handle different versions @xmldom/xmldom from the upstream vs xml-crypto it would be ideal to support injecting the DOM similarly to how other methods in the library already allow it.

This would allow a faster fixing cycle when cases as these happen: #108

Happy to do any additional modifications to improve this so it can be landed.

@elbuo8 elbuo8 closed this Apr 17, 2025
@elbuo8 elbuo8 reopened this Apr 22, 2025
@elbuo8 elbuo8 changed the title Accept DOM when computing signatures Accept DOM node when computing/verifying signatures Apr 22, 2025
@cjbarth
Copy link
Contributor

cjbarth commented Apr 22, 2025

We're in an active update cycle for this library now, so your changes are coming at a good time. Do any of the recent changes affect what you're trying to accomplish?

@elbuo8
Copy link
Author

elbuo8 commented Apr 22, 2025

Do any of the recent changes affect what you're trying to accomplish?

The recent addition/changes of signedReferences works fine. We are tracking the V7 Draft but it still not clear how the interface/methods are going to end up. Ideally (and we'd be more than happy to contribute) they would accept DOMs as we are suggesting here so that the versions of xmldom are completely de-coupled, if possible.

@ahacker1-securesaml
Copy link

ahacker1-securesaml commented Apr 22, 2025

I feel like we should fix the underlying issue in xmldom, which is parsing "\r\n" as "\r\n". It should be normalized to "\n".

Then we can accept the PR.

Furthermore, I would prefer changes to be made on v7.x rather than v6.x (which has poor API design)

@elbuo8
Copy link
Author

elbuo8 commented Apr 22, 2025

While I'd definitely prefer to fix the issue at the downstream dependency, that cycle would take far longer and furthermore, it wouldn't prevent a similar issue in the future given that the upstream can still be using a completely different version of xmldom (which may have other fixes and/or regressions) that do not play well with the version shipped with xml-crypto.

Happy to PR both versions. But I might need some guidance on V7 as I'm seeing checkSignature become _checkSignature but then not clear which is ultimately the exposed/public API that will be the entrypoint.

@ahacker1-securesaml
Copy link

In V7, we move the signature verificaiton part into a new module.
The old SignedXML is then only used for signing XML, and _checkSignature is deprecated.
The new XMLVerifier is then used for verifying XML.

@elbuo8
Copy link
Author

elbuo8 commented Apr 22, 2025

Btw, for the specific bug I'm hunting (and the ticket refers) it's already fixed in xmldom 0.9.

@elbuo8 elbuo8 mentioned this pull request Apr 22, 2025
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.

4 participants