Commit 0d658c5

mo khan <mo@mokhan.ca>
2026-08-18 02:58:08
Compile saml xsds without changing directory main
Schema compilation ran inside a Dir.chdir block, which mutates process global state, so two threads validating a document at the same time could raise "conflicting chdir during another chdir block". Eight threads validating the same metadata reproduced it seven times. Each xsd is now parsed as a document carrying its own path, so libxml2 resolves the relative schemaLocation imports against the xsd's own directory without a working directory change. The compiled schema is cached, since nothing had been reusing it across calls.
1 parent 8123d24
lib/saml/kit/concerns/xsd_validatable.rb
@@ -13,15 +13,42 @@ module Saml
         '../xsd/saml-schema-metadata-2.0.xsd', File.dirname(__FILE__)
       ).freeze
 
+      @schemas = {}
+      @monitor = Mutex.new
+
+      class << self
+        # Validation treats a schema as read only, so one instance is shared
+        # across threads. The hit path reads without the lock, which at worst
+        # compiles the same schema twice on rubies that lack a global lock.
+        #
+        # @!visibility private
+        def schema_for(path)
+          @schemas[path] || @monitor.synchronize do
+            @schemas[path] ||= compile(path)
+          end
+        end
+
+        private
+
+        # The xsd is parsed as a document carrying its own path as a base uri,
+        # so the relative schemaLocation imports in our xsds resolve against
+        # the xsd's own directory. A bare String carries no base uri and would
+        # resolve them against the working directory instead, which is what
+        # Dir.chdir used to paper over at the cost of crashing under threads.
+        def compile(path)
+          Nokogiri::XML::Schema.from_document(
+            Nokogiri::XML::Document.parse(File.read(path), url: path)
+          )
+        end
+      end
+
       # @!visibility private
-      def matches_xsd?(xsd)
+      def matches_xsd?(path)
         return unless to_nokogiri.present?
 
-        Dir.chdir(File.dirname(xsd)) do
-          xsd = Nokogiri::XML::Schema(IO.read(xsd))
-          xsd.validate(to_nokogiri.document).each do |error|
-            errors.add(:base, error.message)
-          end
+        schema = XsdValidatable.schema_for(path)
+        schema.validate(to_nokogiri.document).each do |error|
+          errors.add(:base, error.message)
         end
       end
     end
lib/saml/kit/version.rb
@@ -2,6 +2,6 @@
 
 module Saml
   module Kit
-    VERSION = '1.5.0'
+    VERSION = '1.5.1'
   end
 end
spec/saml/kit/xsd_validatable_spec.rb
@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+RSpec.describe Saml::Kit::XsdValidatable do
+  let(:metadata_xml) do
+    Saml::Kit::IdentityProviderMetadata.build do |x|
+      x.entity_id = 'https://idp.example.com/metadata'
+      x.add_single_sign_on_service(
+        'https://idp.example.com/login', binding: :http_post
+      )
+    end.to_xml
+  end
+
+  let(:request_xml) do
+    Saml::Kit::AuthenticationRequest.build do |x|
+      x.issuer = 'https://sp.example.com/metadata'
+    end.to_xml
+  end
+
+  describe 'concurrent validation' do
+    # Compiling the schema inside Dir.chdir mutated process global state, and
+    # ruby raises when one thread enters a chdir block while another is inside
+    # one, so this raised "conflicting chdir during another chdir block".
+    it 'does not raise when many threads validate at once' do
+      threads = 8.times.map do
+        Thread.new do
+          20.times do
+            Saml::Kit::IdentityProviderMetadata.new(metadata_xml).valid?
+            Saml::Kit::AuthenticationRequest.new(request_xml).valid?
+          end
+        end
+      end
+      expect { threads.each(&:value) }.not_to raise_error
+    end
+
+    it 'reports the same result in every thread' do
+      threads = 8.times.map do
+        Thread.new { Saml::Kit::IdentityProviderMetadata.new(metadata_xml).valid? }
+      end
+      expect(threads.map(&:value).uniq).to eql([true])
+    end
+  end
+
+  describe 'schema resolution' do
+    # Our xsds import each other by relative filename, so this fails if they
+    # ever resolve against the working directory instead of the xsd's own.
+    it 'validates from an unrelated working directory' do
+      Dir.chdir('/') do
+        expect(Saml::Kit::IdentityProviderMetadata.new(metadata_xml)).to be_valid
+      end
+    end
+  end
+
+  describe 'schema compilation' do
+    around do |example|
+      cached = described_class.instance_variable_get(:@schemas)
+      described_class.instance_variable_set(:@schemas, {})
+      example.run
+      described_class.instance_variable_set(:@schemas, cached)
+    end
+
+    it 'compiles a schema once however often it is validated' do
+      allow(Nokogiri::XML::Schema).to receive(:from_document).and_call_original
+      3.times { Saml::Kit::IdentityProviderMetadata.new(metadata_xml).valid? }
+      expect(Nokogiri::XML::Schema).to have_received(:from_document).once
+    end
+  end
+end
CHANGELOG.md
@@ -1,4 +1,4 @@
-Version 1.5.0
+Version 1.5.1
 
 # Changelog
 All notable changes to this project will be documented in this file.
@@ -8,6 +8,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [1.5.1] - 2026-08-18
+### Fixed
+- Compile the SAML XSDs without `Dir.chdir`. Schema compilation ran inside a
+  `Dir.chdir` block, which mutates process global state, so two threads
+  validating a document at the same time could raise `conflicting chdir during
+  another chdir block`. Each schema is now parsed as a document carrying its own
+  path, so its relative imports resolve without changing the working directory.
+- Compile each XSD once instead of on every `valid?` call.
+
 ## [1.5.0] - 2026-08-17
 ### Security
 - Reject a `Response` or `Assertion` that carries no signature. A registered
@@ -142,7 +151,8 @@ exploitable or is a side effect of closing one that was.
 ### Removed
 - Removed optional SessionNotOnOrAfter attribute from AuthnStatement.
 
-[Unreleased]: https://github.com/xlgmokha/saml-kit/compare/v1.5.0...HEAD
+[Unreleased]: https://github.com/xlgmokha/saml-kit/compare/v1.5.1...HEAD
+[1.5.1]: https://github.com/xlgmokha/saml-kit/compare/v1.5.0...v1.5.1
 [1.5.0]: https://github.com/xlgmokha/saml-kit/compare/v1.4.1...v1.5.0
 [1.4.1]: https://github.com/xlgmokha/saml-kit/compare/v1.4.0...v1.4.1
 [1.4.0]: https://github.com/xlgmokha/saml-kit/compare/v1.3.0...v1.4.0
Gemfile.lock
@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    saml-kit (1.5.0)
+    saml-kit (1.5.1)
       activemodel (>= 5.1)
       base64 (~> 0.1)
       cgi (~> 0.1)