Several improvements about how XML is parsed.

lib/ruby_saml/idp_metadata_parser.rb

@@ -161,7 +161,12 @@ def parse_to_array(idp_metadata, options = {})
     end
 
     def parse_to_idp_metadata_array(idp_metadata, options = {})
-      @document = Nokogiri::XML(idp_metadata) # TODO: RubySaml::XML.safe_load_nokogiri
+      begin
+        @document = RubySaml::XML.safe_load_xml(idp_metadata, check_malformed_doc: true)
+      rescue StandardError => e
+        raise ArgumentError.new("XML load failed: #{e.message}") if e.message != 'Empty document'
+      end
+
       @options = options
 
       idpsso_descriptors = self.class.get_idps(@document, options[:entity_id])

lib/ruby_saml/logoutresponse.rb

@@ -33,6 +33,8 @@ def initialize(response, settings = nil, options = {})
       raise ArgumentError.new("Logoutresponse cannot be nil") if response.nil?
       @settings = settings
 
+      raise ValidationError.new("Invalid settings type: expected RubySaml::Settings, got #{@settings.class.name}") if [email protected]_a?(Settings) && [email protected]?
+
       if settings.nil? || settings.soft.nil?
         @soft = true
       else
@@ -41,7 +43,14 @@ def initialize(response, settings = nil, options = {})
 
       @options = options
       @response = RubySaml::XML::Decoder.decode_message(response, @settings&.message_max_bytesize)
-      @document = RubySaml::XML.safe_load_nokogiri(@response)
+      begin
+        @document = RubySaml::XML.safe_load_xml(@response, check_malformed_doc: @soft)
+      rescue StandardError => e
+        @errors << "XML load failed: #{e.message}" if e.message != "Empty document"
+        return if @soft
+        raise ValidationError.new("XML load failed: #{e.message}") if e.message != "Empty document"
+      end
+
       super()
     end
 
@@ -136,9 +145,13 @@ def validate_success_status
     # @raise [ValidationError] if soft == false and validation fails
     #
     def validate_structure
+      structure_error_msg = "Invalid SAML Logout Response. Not match the saml-schema-protocol-2.0.xsd"
+
+      doc_to_analize = @document.nil? ? @response : @document
+
       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)
+        return append_error(structure_error_msg)
       end
 
       true

lib/ruby_saml/response.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require "ruby_saml/settings"
 require "ruby_saml/xml"
 require "ruby_saml/attributes"
 require "time"
@@ -52,18 +53,27 @@ def initialize(response, options = {})
 
       @options = options
       @soft = true
+      message_max_bytesize = nil
       unless options[:settings].nil?
         @settings = options[:settings]
-        unless @settings.soft.nil?
-          @soft = @settings.soft
-        end
+
+        raise ValidationError.new("Invalid settings type: expected RubySaml::Settings, got #{@settings.class.name}") if [email protected]_a?(Settings) && [email protected]?
+
+        @soft = @settings.respond_to?(:soft) && [email protected]?  ? @settings.soft : true
+        message_max_bytesize = @settings.message_max_bytesize if @settings.respond_to?(:message_max_bytesize)
       end
 
-      @response = RubySaml::XML::Decoder.decode_message(response, @settings&.message_max_bytesize)
-      @document = RubySaml::XML.safe_load_nokogiri(@response)
+      @response = RubySaml::XML::Decoder.decode_message(response, message_max_bytesize)
+      begin
+        @document = RubySaml::XML.safe_load_xml(@response, check_malformed_doc: @soft)
+      rescue StandardError => e
+        @errors << "XML load failed: #{e.message}" if e.message != 'Empty document'
+        return if @soft
+        raise ValidationError.new("XML load failed: #{e.message}") if e.message != 'Empty document'
+      end
 
-      if assertion_encrypted?
-        @decrypted_document = generate_decrypted_document
+      if [email protected]? && assertion_encrypted?
+          @decrypted_document = generate_decrypted_document
       end
 
       super()
@@ -131,6 +141,8 @@ def sessionindex
     # @raise [ValidationError] if there are 2+ Attribute with the same Name
     #
     def attributes
+      return nil if @document.nil?
+
       @attr_statements ||= begin
         attributes = Attributes.new
 
@@ -367,6 +379,9 @@ def assertion_id
     #
     def validate(collect_errors = false)
       reset_errors!
+
+      return append_error("Blank response") if @document.nil?
+
       return false unless validate_response_state
 
       validations = %i[
@@ -417,8 +432,10 @@ def validate_success_status
     def validate_structure
       structure_error_msg = "Invalid SAML Response. Not match the saml-schema-protocol-2.0.xsd"
 
+      doc_to_analize = @document.nil? ? @response : @document
+
       check_malformed_doc = check_malformed_doc_enabled?
-      unless valid_saml?(document, soft, check_malformed_doc: check_malformed_doc)
+      unless valid_saml?(doc_to_analize, soft, check_malformed_doc: check_malformed_doc)
         return append_error(structure_error_msg)
       end
 
@@ -900,6 +917,8 @@ def validate_signature
     end
 
     def name_id_node
+      return nil if @document.nil?
+
       @name_id_node ||=
         begin
           encrypted_node = xpath_first_from_signed_assertion('/a:Subject/a:EncryptedID')

lib/ruby_saml/saml_message.rb

@@ -29,6 +29,8 @@ def id(document)
     end
 
     def root_attribute(document, attribute)
+      return nil if document.nil?
+
       document.at_xpath(
         "/p:AuthnRequest | /p:Response | /p:LogoutResponse | /p:LogoutRequest",
         { "p" => RubySaml::XML::NS_PROTOCOL }
@@ -43,10 +45,10 @@ def root_attribute(document, attribute)
     # @raise [ValidationError] if soft == false and validation fails
     def valid_saml?(document, soft = true, check_malformed_doc: true)
       begin
-        xml = RubySaml::XML.safe_load_nokogiri(document, check_malformed_doc: check_malformed_doc)
+        xml = RubySaml::XML.safe_load_xml(document, check_malformed_doc: check_malformed_doc)
       rescue StandardError => error
         return false if soft
-        raise ValidationError.new("XML load failed: #{error.message}")
+        raise ValidationError.new("XML load failed: #{error.message}") if error.message != "Empty document"
       end
 
       SamlMessage.schema.validate(xml).each do |schema_error|

lib/ruby_saml/slo_logoutrequest.rb

@@ -36,11 +36,22 @@ def initialize(request, options = {})
       @soft = true
       unless options[:settings].nil?
         @settings = options[:settings]
-        @soft = @settings.soft unless @settings.soft.nil?
+
+        raise ValidationError.new("Invalid settings type: expected RubySaml::Settings, got #{@settings.class.name}") if [email protected]_a?(Settings) && [email protected]?
+
+        @soft = @settings.respond_to?(:soft) && [email protected]? ? @settings.soft : true
+        message_max_bytesize = @settings.message_max_bytesize if @settings.respond_to?(:message_max_bytesize)
+      end
+
+      @request = RubySaml::XML::Decoder.decode_message(request, message_max_bytesize)
+      begin
+        @document = RubySaml::XML.safe_load_xml(@request, check_malformed_doc: @soft)
+      rescue StandardError => e
+        @errors << "XML load failed: #{e.message}" if e.message != 'Empty document'
+        return if @soft
+        raise ValidationError.new("XML load failed: #{e.message}") if e.message != 'Empty document'
       end
 
-      @request = RubySaml::XML::Decoder.decode_message(request, @settings&.message_max_bytesize)
-      @document = RubySaml::XML.safe_load_nokogiri(@request)
       super()
     end
 
@@ -157,6 +168,8 @@ def validate(collect_errors = false)
     # @return [Boolean] True if the Logout Request contains an ID, otherwise returns False
     #
     def validate_id
+      return append_error("Missing ID attribute on Logout Request") if document.nil?
+
       return true if id
       append_error("Missing ID attribute on Logout Request")
     end
@@ -166,6 +179,8 @@ def validate_id
     # @return [Boolean] True if the Logout Request is 2.0, otherwise returns False
     #
     def validate_version
+      return append_error("Unsupported SAML version") if document.nil?
+
       return true if version(document) == "2.0"
       append_error("Unsupported SAML version")
     end
@@ -191,8 +206,10 @@ def validate_not_on_or_after
     # @raise [ValidationError] if soft == false and validation fails
     #
     def validate_structure
+      doc_to_analize = @document.nil? ? @request : @document
+
       check_malformed_doc = check_malformed_doc?(settings)
-      unless valid_saml?(document, soft, check_malformed_doc: check_malformed_doc)
+      unless valid_saml?(doc_to_analize, soft, check_malformed_doc: check_malformed_doc)
         return append_error("Invalid SAML Logout Request. Not match the saml-schema-protocol-2.0.xsd")
       end
 

lib/ruby_saml/xml.rb

@@ -49,49 +49,34 @@ module XML
     NOKOGIRI_OPTIONS = Nokogiri::XML::ParseOptions::STRICT |
                        Nokogiri::XML::ParseOptions::NONET
 
-    # TODO: safe_load_message (rename safe_load_nokogiri --> safe_load_xml)
-    # def safe_load_message(message, check_malformed_doc: true)
-    #   message = Decoder.decode(message)
-    #   begin
-    #     safe_load_nokogiri(message, check_malformed_doc: check_malformed_doc)
-    #   rescue RubySaml::Errors::XMLLoadError
-    #     Nokogiri::XML::Document.new
-    #   end
-    # end
-
     # Safely load the SAML Message XML.
     # @param document [String | Nokogiri::XML::Document] The message to be loaded
     # @param check_malformed_doc [Boolean] check_malformed_doc Enable or Disable the check for malformed XML
     # @return [Nokogiri::XML::Document] The nokogiri document
-    # @raise [ValidationError] If there was a problem loading the SAML Message XML
-    def safe_load_nokogiri(document, check_malformed_doc: true)
+    # @raise [StandardError] If there was a problem loading the SAML Message XML
+    def safe_load_xml(document, check_malformed_doc: true)
       doc_str = document.to_s
-      error = nil
-      error = StandardError.new('Dangerous XML detected. No Doctype nodes allowed') if doc_str.include?('<!DOCTYPE')
-
-      xml = nil
-      unless error
-        begin
-          xml = Nokogiri::XML(doc_str) do |config|
-            config.options = NOKOGIRI_OPTIONS
-          end
-        rescue StandardError => e
-          error ||= e
-          # raise StandardError.new(e.message)
+      raise StandardError.new('Dangerous XML detected. No Doctype nodes allowed') if doc_str.include?('<!DOCTYPE')
+
+      begin
+        doc = Nokogiri::XML(doc_str) do |config|
+          config.options = NOKOGIRI_OPTIONS
         end
+      rescue StandardError => e
+        raise StandardError.new(e.message)
+      rescue SyntaxError => e
+        raise StandardError.new(e.message) if check_malformed_doc && e.message != 'Empty document'
       end
 
-      # TODO: This is messy, its shims how the old REXML parser works
-      if xml
-        error ||= StandardError.new('Dangerous XML detected. No Doctype nodes allowed') if xml.internal_subset
-        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?
       end
-      return Nokogiri::XML::Document.new if error || !xml
 
-      xml
+      doc
     end
 
-    def copy_nokogiri(noko)
+    def copy_xml(noko)
       Nokogiri::XML(noko.to_xml(save_with: Nokogiri::XML::Node::SaveOptions::AS_XML)) do |config|
         config.options = NOKOGIRI_OPTIONS
       end

lib/ruby_saml/xml/decryptor.rb

@@ -11,8 +11,13 @@ module Decryptor
       # @param decryption_keys [Array] Array of private keys for decryption
       # @return [Nokogiri::XML::Document] The SAML document with assertions decrypted
       def decrypt_document(document, decryption_keys)
-        # Copy the document
-        document = RubySaml::XML.safe_load_nokogiri(document.to_s)
+        # Copy the document to avoid modifying the original one
+        begin
+          document = 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'
+        end
+
         validate_decryption_keys!(decryption_keys)
 
         response_node = document.at_xpath(

lib/ruby_saml/xml/document_signer.rb

@@ -27,7 +27,11 @@ module DocumentSigner
       #   <Object />
       # </Signature>
       def sign_document(document, private_key, certificate, signature_method = RubySaml::XML::RSA_SHA256, digest_method = RubySaml::XML::SHA256)
-        noko = RubySaml::XML.safe_load_nokogiri(document.to_s)
+        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'
+        end
 
         sign_document!(noko, private_key, certificate, signature_method, digest_method)
       end

lib/ruby_saml/xml/signed_document_info.rb

@@ -13,12 +13,17 @@ class SignedDocumentInfo
       # @param noko [Nokogiri::XML] The XML document to validate
       # @param check_malformed_doc [Boolean] Whether to check for malformed documents
       def initialize(noko, check_malformed_doc: true)
-        noko = if noko.is_a?(Nokogiri::XML::Document)
-                 RubySaml::XML.copy_nokogiri(noko)
-               else
-                 RubySaml::XML.safe_load_nokogiri(noko, check_malformed_doc: check_malformed_doc)
-               end
-        @noko = noko
+        @noko = if noko.is_a?(Nokogiri::XML::Document)
+                  RubySaml::XML.copy_xml(noko)
+                else
+                  begin
+                    RubySaml::XML.safe_load_xml(noko, check_malformed_doc: check_malformed_doc)
+                  rescue StandardError => e
+                    raise ValidationError.new("XML load failed: #{e.message}") if e.message != 'Empty document'
+
+                    nil
+                  end
+                end
         @check_malformed_doc = check_malformed_doc
       end
 

test/response_test.rb

@@ -65,6 +65,15 @@ def generate_audience_error(expected, actual)
       assert_raises(ArgumentError) { RubySaml::Response.new(nil) }
     end
 
+    it 'raise an exception when the settings provided are not a RubySaml::Settings object' do
+      settings = "invalid settings"
+      error_msg = "Invalid settings type: expected RubySaml::Settings, got String"
+      options = { :settings => settings }
+      assert_raises(RubySaml::ValidationError, error_msg) do
+        RubySaml::Response.new(response_document_valid_signed, options)
+      end
+    end
+
     it 'not filter available options only' do
       options = { skip_destination: true, foo: :bar }
       response = RubySaml::Response.new(response_document_valid_signed, options)
@@ -85,6 +94,36 @@ def generate_audience_error(expected, actual)
       assert_includes ampersands_response.errors, 'SAML Response must contain 1 assertion'
     end
 
+    it 'Raise ValidationError if XML contains SyntaxError trying to initialize and soft = false' do
+      settings.soft = false
+      error_msg = if jruby?
+                    'XML load failed: The element type "ds:X509Certificate" must be terminated by the matching end-tag "</ds:X509Certificate>".'
+                  else
+                    'XML load failed: 53:875: FATAL: Opening and ending tag mismatch: X509Certificate line 53 and SignatureValue'
+                  end
+      assert_raises(RubySaml::ValidationError, error_msg) do
+        OneLogin::RubySaml::Response.new(fixture(:response_wrong_syntax), :settings => settings)
+      end
+    end
+
+    it "Do not raise validation error when XML contains SyntaxError and soft = true, but validation fails" do
+      settings.soft = true
+      settings.idp_cert_fingerprint = ruby_saml_cert_fingerprint
+      response = OneLogin::RubySaml::Response.new(fixture(:response_wrong_syntax), :settings => settings)
+      error_msg = if jruby?
+                    'XML load failed: The element type "ds:X509Certificate" must be terminated by the matching end-tag "</ds:X509Certificate>".'
+                  else
+                    'XML load failed: 53:875: FATAL: Opening and ending tag mismatch: X509Certificate line 53 and SignatureValue'
+                  end
+      assert_includes response.errors, error_msg
+
+      refute response.is_valid?
+
+      assert_includes response.errors, 'Blank response'
+      assert_nil response.nameid
+      assert_nil response.attributes
+    end
+
     describe 'Prevent node text with comment attack (VU#475445)' do
       before do
         @response = RubySaml::Response.new(read_response('response_node_text_attack.xml.base64'))
@@ -140,6 +179,7 @@ def generate_audience_error(expected, actual)
       it 'raise when evil attack vector is present, soft = false ' do
         @response.soft = false
         error_msg = 'XML load failed: Dangerous XML detected. No Doctype nodes allowed'
+
         assert_raises(RubySaml::ValidationError, error_msg) do
           @response.send(:validate_structure)
         end