Skip to content

Conversation

@pitbulk
Copy link
Collaborator

@pitbulk pitbulk commented Nov 30, 2025

  • Rename safe_load_nokogiri into safe_load_xml.
  • Refactor of safe_load_xml method.
  • Improve how handle XML that contains syntax errors, added test.
  • Prevent exceptions on .at_xpath called when the document was not loaded and user invoke elements accesor by checking the @document is not nil.

@pitbulk pitbulk force-pushed the v2.x_safe_load_refactor branch from 5278ea6 to 0e9e57d Compare November 30, 2025 10:42
@pitbulk pitbulk force-pushed the v2.x_safe_load_refactor branch from 0e9e57d to a7c847f Compare November 30, 2025 10:58
@pitbulk
Copy link
Collaborator Author

pitbulk commented Dec 1, 2025

@johnnyshields, @ahacker1-securesaml any chance to check my PR? Thanks

@ahacker1-securesaml
Copy link
Contributor

I think this is good (security-wise). I'm not really concerned about XML parsing, what I am most concerned about is the signature verification components.

Right before releases 2.x, I will raise my concerns about the SignedDocumentValidator class

@pitbulk
Copy link
Collaborator Author

pitbulk commented Dec 3, 2025

@ahacker1-securesaml, we want to release 2.x soon, so please share your concerns

@ahacker1-securesaml
Copy link
Contributor

I shared them here:
https://github.com/SAML-Toolkits/ruby-saml/pull/759/files#r2545845989

Main issues:

subject = RubySaml::XML::SignedDocumentValidator.subject_node(xml)

You should be using canonicalized_subject, not subject.

Everything else from the PR review applies.

The general idea is that you want to load exactly the same bytes as the underlying cryptography library uses. So when you call openssl on some piece of data, you need to make sure you actually use it, and not some other piece of data.

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.

3 participants