Several improvements about how XML is parsed. #782
Conversation
pitbulk
commented
Nov 30, 2025
pitbulk
commented
- 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
force-pushed
the
v2.x_safe_load_refactor
branch
from
5278ea6 to
0e9e57d
Compare
pitbulk
force-pushed
the
v2.x_safe_load_refactor
branch
from
0e9e57d to
a7c847f
Compare
|
@johnnyshields, @ahacker1-securesaml any chance to check my PR? Thanks |
|
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 |
|
@ahacker1-securesaml, we want to release 2.x soon, so please share your concerns |
|
I shared them here: Main issues: ruby-saml/lib/ruby_saml/response.rb Line 939 in 6798442 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. |
| begin | ||
| noko = RubySaml::XML.safe_load_xml(document.to_s, check_malformed_doc: true) | ||
| rescue StandardError => e | ||
| raise ValidationError.new("XML load failed: #{e.message}") if e.message != 'Empty document' |
There was a problem hiding this comment.
This error raising should probably be part of the safe_load_xml method itself, since you seem to be copying it everywhere.
| check_malformed_doc = check_malformed_doc?(settings) | ||
| unless valid_saml?(document, soft, check_malformed_doc: check_malformed_doc) | ||
| return append_error("Invalid SAML Logout Response. Not match the saml-schema-protocol-2.0.xsd") | ||
| unless valid_saml?(doc_to_analize, soft, check_malformed_doc: check_malformed_doc) |
There was a problem hiding this comment.
typo: should be doc_to_analyze (y instead of i)
| error ||= StandardError.new("There were XML errors when parsing: #{xml.errors}") if check_malformed_doc && !xml.errors.empty? | ||
| if doc.is_a?(Nokogiri::XML::Document) | ||
| StandardError.new('Dangerous XML detected. No Doctype nodes allowed') if doc.internal_subset | ||
| StandardError.new("There were XML errors when parsing: #{doc.errors}") if check_malformed_doc && !doc.errors.empty? |
There was a problem hiding this comment.
These should probably be specialized Error classes, rather than StandardError
