fix: JRuby NONET bypass in XML::Schema (v1.19.x) (#3639)

flavorjones · web-flow · commit a856d1e46bda · 2026-06-18T10:27:12.000-04:00
**What problem is this PR intended to solve?** [JRuby] `XML::Schema` now enforces the `NONET` parse option, which Nokogiri enables by default. It was not enforced on JRuby, so a schema parsed with default options could still fetch external resources over the network, potentially enabling SSRF or XXE attacks and bypassing the mitigation for CVE-2020-26247. See [GHSA-8678-w3jw-xfc2](GHSA-8678-w3jw-xfc2) for more information. **Have you included adequate test coverage?** Yes. **Does this change affect the behavior of either the C or the Java implementations?** Java only — this fixes the JRuby (Java) implementation. CRuby is not affected, because libxml2's `xmlNoNetExternalEntityLoader` already blocks all network schemes regardless of scheme or case.
diff --git a/ext/java/nokogiri/XmlSchema.java b/ext/java/nokogiri/XmlSchema.java
@@ -8,6 +8,9 @@
 import java.io.Reader;
 import java.io.StringReader;
 
+import java.net.URI;
+import java.net.URISyntaxException;
+
 import javax.xml.XMLConstants;
 import javax.xml.transform.Source;
 import javax.xml.transform.dom.DOMSource;
@@ -285,24 +288,103 @@ private class SchemaResourceResolver implements LSResourceResolver
                     String systemId,
                     String baseURI)
     {
-      if (noNet && systemId != null && (systemId.startsWith("http://") || systemId.startsWith("ftp://"))) {
-        if (systemId.startsWith(XMLConstants.W3C_XML_SCHEMA_NS_URI)) {
-          return null; // use default resolver
-        }
+      if (noNet && !effectiveResourceIsLocal(systemId, baseURI)) {
         try {
           this.errorHandler.warning(new SAXParseException(String.format("Attempt to load network entity '%s'", systemId), null));
         } catch (SAXException ex) {
         }
-      } else {
-        String adjusted = adjustSystemIdIfNecessary(currentDir, scriptFileName, baseURI, systemId);
-        lsInput.setPublicId(publicId);
-        lsInput.setSystemId(adjusted != null ? adjusted : systemId);
-        lsInput.setBaseURI(baseURI);
+        return new SchemaLSInput(); // an empty input blocks the fetch
       }
+
+      String adjusted = adjustSystemIdIfNecessary(currentDir, scriptFileName, baseURI, systemId);
+      lsInput.setPublicId(publicId);
+      lsInput.setSystemId(adjusted != null ? adjusted : systemId);
+      lsInput.setBaseURI(baseURI);
       return lsInput;
     }
   }
 
+  // We enforce NONET for schema resolution by hand because Xerces-J (the JAXP implementation
+  // backing XML::Schema on JRuby) does not implement the standard JAXP property
+  // XMLConstants.ACCESS_EXTERNAL_SCHEMA — so we cannot simply restrict external access on the
+  // SchemaFactory and must classify each resolved resource in the LSResourceResolver instead.
+  //
+  // Decides whether a schema-import resource may be resolved while NONET is on: true means
+  // local (allowed), false means a network resource (blocked). A relative systemId inherits
+  // its document's base, so it is resolved against baseURI before classification — a relative
+  // import under a remote base is a network fetch even though the systemId alone looks local.
+  private static boolean
+  effectiveResourceIsLocal(String systemId, String baseURI)
+  {
+    // a null systemId means there is nothing external to resolve
+    if (systemId == null) {
+      return true;
+    }
+    try {
+      URI uri = new URI(systemId);
+      if (baseURI != null && !baseURI.isEmpty()) {
+        uri = new URI(baseURI).resolve(uri);
+      }
+      return isLocalResource(uri);
+    } catch (URISyntaxException | IllegalArgumentException e) {
+      // fail closed: an unparseable base or systemId (e.g. a raw UNC path "\\host\share") is
+      // not provably local, and the JVM's file/URL handling may still reach the network
+      return false;
+    }
+  }
+
+  // Test seam for the Ruby suite: local_resource?(systemId, baseURI = nil).
+  @JRubyMethod(meta = true, name = "local_resource?", required = 1, optional = 1, visibility = Visibility.PRIVATE)
+  public static IRubyObject
+  local_resource_eh(ThreadContext context, IRubyObject klazz, IRubyObject[] args)
+  {
+    String systemId = args[0].isNil() ? null : args[0].asJavaString();
+    String baseURI = (args.length > 1 && !args[1].isNil()) ? args[1].asJavaString() : null;
+    return context.runtime.newBoolean(effectiveResourceIsLocal(systemId, baseURI));
+  }
+
+  // Classifies an already-parsed URI. Local is a missing scheme, or the "file" scheme, with
+  // no remote authority and no UNC-shaped path. This is intentionally stricter than libxml2's
+  // xmlNoNetExternalEntityLoader, which folds a remote host (file://host/...) into a local
+  // path rather than rejecting it.
+  //
+  // TODO: a Windows drive-letter path like "C:\path" parses as scheme "c" and would be
+  // blocked; support those if we need it later.
+  private static boolean
+  isLocalResource(URI uri)
+  {
+    // only a missing scheme (a relative or absolute path) or file: can be local; any
+    // other scheme is a network resource
+    String scheme = uri.getScheme();
+    if (scheme != null && !scheme.equalsIgnoreCase("file")) {
+      return false;
+    }
+
+    // an opaque "file:" URI (e.g. file:foo, with no "//") is not a usable local path; reject
+    // it, matching libxml2, which does not resolve that form as a local file either
+    if (uri.isOpaque()) {
+      return false;
+    }
+
+    // a non-empty, non-localhost authority is a remote host — file://host/path, or the
+    // schemeless network-path form //host/path. Stricter than libxml2, which folds such a
+    // host into a (failing) local path.
+    String authority = uri.getRawAuthority();
+    if (authority != null && !authority.isEmpty() && !authority.equalsIgnoreCase("localhost")) {
+      return false;
+    }
+
+    // reject UNC-shaped paths even under an allowed authority: file:////host/share,
+    // file://localhost//host/share, and %2f/%5c-encoded variants. getPath() is decoded, so
+    // the encoded forms are normalized before this check.
+    String path = uri.getPath();
+    if (path != null && (path.startsWith("//") || path.indexOf('\\') >= 0)) {
+      return false;
+    }
+
+    return true;
+  }
+
   private class SchemaLSInput implements LSInput
   {
     protected String fPublicId;
diff --git a/ext/nokogiri/nokogiri.c b/ext/nokogiri/nokogiri.c
@@ -203,9 +203,6 @@ Init_nokogiri(void)
   rb_const_set(mNokogiri, rb_intern("LIBXSLT_COMPILED_VERSION"), NOKOGIRI_STR_NEW2(LIBXSLT_DOTTED_VERSION));
   rb_const_set(mNokogiri, rb_intern("LIBXSLT_LOADED_VERSION"), NOKOGIRI_STR_NEW2(xsltEngineVersion));
 
-  rb_const_set(mNokogiri, rb_intern("LIBXML_ZLIB_ENABLED"),
-               xmlHasFeature(XML_WITH_ZLIB) == 1 ? Qtrue : Qfalse);
-
 #ifdef NOKOGIRI_PACKAGED_LIBRARIES
   rb_const_set(mNokogiri, rb_intern("PACKAGED_LIBRARIES"), Qtrue);
 #  ifdef NOKOGIRI_PRECOMPILED_LIBRARIES
@@ -228,6 +225,12 @@ Init_nokogiri(void)
   rb_const_set(mNokogiri, rb_intern("LIBXML_ICONV_ENABLED"), Qfalse);
 #endif
 
+  rb_const_set(mNokogiri, rb_intern("LIBXML_ZLIB_ENABLED"),
+               xmlHasFeature(XML_WITH_ZLIB) == 1 ? Qtrue : Qfalse);
+
+  rb_const_set(mNokogiri, rb_intern("LIBXML_HTTP_ENABLED"),
+               xmlHasFeature(XML_WITH_HTTP) == 1 ? Qtrue : Qfalse);
+
 #ifdef NOKOGIRI_OTHER_LIBRARY_VERSIONS
   rb_const_set(mNokogiri, rb_intern("OTHER_LIBRARY_VERSIONS"), NOKOGIRI_STR_NEW2(NOKOGIRI_OTHER_LIBRARY_VERSIONS));
 #endif
diff --git a/lib/nokogiri/version/info.rb b/lib/nokogiri/version/info.rb
@@ -53,6 +53,14 @@ def libxml2_has_iconv?
       defined?(Nokogiri::LIBXML_ICONV_ENABLED) && Nokogiri::LIBXML_ICONV_ENABLED
     end
 
+    def libxml2_has_zlib?
+      defined?(Nokogiri::LIBXML_ZLIB_ENABLED) && Nokogiri::LIBXML_ZLIB_ENABLED
+    end
+
+    def libxml2_has_http?
+      defined?(Nokogiri::LIBXML_HTTP_ENABLED) && Nokogiri::LIBXML_HTTP_ENABLED
+    end
+
     def libxslt_has_datetime?
       defined?(Nokogiri::LIBXSLT_DATETIME_ENABLED) && Nokogiri::LIBXSLT_DATETIME_ENABLED
     end
@@ -145,6 +153,8 @@ def to_hash
             end
             libxml["memory_management"] = Nokogiri::LIBXML_MEMORY_MANAGEMENT
             libxml["iconv_enabled"] = libxml2_has_iconv?
+            libxml["zlib_enabled"] = libxml2_has_zlib?
+            libxml["http_enabled"] = libxml2_has_http?
             libxml["compiled"] = compiled_libxml_version.to_s
             libxml["loaded"] = loaded_libxml_version.to_s
           end
diff --git a/test/helper.rb b/test/helper.rb
@@ -22,6 +22,7 @@
 require "nokogiri"
 
 require_relative "helpers/memory_debugger"
+require_relative "helpers/mock_server"
 
 if ENV["TEST_NOKOGIRI_WITH_LIBXML_RUBY"]
   #
@@ -103,6 +104,31 @@ def file_url_for(path)
       path_with_leading_slash = path.start_with?("/") ? path : "/#{path}"
       "file://#{path_with_leading_slash}"
     end
+
+    def assert_network_connection(scheme: "http", path: "/making-a-request", &block)
+      assert tcp_connection_attempted?(scheme:, path:, &block), "should open a network connection"
+    end
+
+    def refute_network_connection(scheme: "http", path: "/making-a-request", &block)
+      refute tcp_connection_attempted?(scheme:, path:, &block), "should not open a network connection"
+    end
+
+    def ignoring_syntax_errors
+      yield
+    rescue Nokogiri::XML::SyntaxError
+    end
+
+    def tcp_connection_attempted?(scheme: "http", path: "/making-a-request")
+      flunk("MockServer is not supported on this platform; the calling test must skip explicitly") unless MockServer.supported?
+
+      listener = MockServer.listener_class.new
+      begin
+        yield("#{scheme}://127.0.0.1:#{listener.port}#{path}")
+      ensure
+        connections = listener.stop
+      end
+      connections.positive?
+    end
   end
 
   class TestBenchmark < Minitest::BenchSpec
diff --git a/test/helpers/mock_server.rb b/test/helpers/mock_server.rb
@@ -0,0 +1,89 @@
+# frozen_string_literal: true
+
+require "socket"
+
+require_relative "memory_debugger"
+
+module Nokogiri
+  module MockServer
+    def self.listener_class
+      Nokogiri.jruby? ? ThreadListener : ProcessListener
+    end
+
+    def self.supported?
+      listener_class.supported?
+    end
+
+    # Accepts and immediately closes TCP connections on an ephemeral port,
+    # counting them. Runs in a forked child process because on CRuby a parse
+    # that accesses the network blocks in a C call that holds the GVL, so an
+    # in-process listener thread would be starved and could never accept the
+    # connection. The child writes one byte per connection to a pipe, and the
+    # parent counts the bytes after reaping the child.
+    class ProcessListener
+      # Process.fork rules out platforms like Windows, and we also avoid
+      # forking under memory debuggers, where children are traced too.
+      def self.supported?
+        Process.respond_to?(:fork) && !MemoryDebugger.active?
+      end
+
+      attr_reader :port
+
+      def initialize
+        server = TCPServer.new("127.0.0.1", 0)
+        @port = server.addr[1]
+        @reader, writer = IO.pipe
+        @pid = Process.fork do
+          @reader.close
+          writer.sync = true
+          loop do
+            client = server.accept
+            writer.write(".")
+            client.close
+          end
+        end
+        writer.close
+        server.close
+      end
+
+      def stop
+        Process.kill(:TERM, @pid)
+        Process.wait(@pid)
+        @reader.read.length
+      ensure
+        @reader.close
+      end
+    end
+
+    # Same API as ProcessListener, but runs in an in-process thread. JRuby has
+    # no GVL, so the listener thread can accept connections while a parse is
+    # blocked, and avoiding process spawn keeps the tests fast on the JVM.
+    class ThreadListener
+      def self.supported?
+        true
+      end
+
+      attr_reader :port
+
+      def initialize
+        @server = TCPServer.new("127.0.0.1", 0)
+        @port = @server.addr[1]
+        @connections = 0
+        @thread = Thread.new do
+          loop do
+            client = @server.accept
+            @connections += 1
+            client.close
+          end
+        rescue IOError, Errno::EBADF
+        end
+      end
+
+      def stop
+        @server.close
+        @thread.join
+        @connections
+      end
+    end
+  end
+end
diff --git a/test/test_version.rb b/test/test_version.rb
@@ -69,6 +69,8 @@ def test_version_info_for_libxml
     assert_equal("#{major}.#{minor}.#{bug}", Nokogiri::VERSION_INFO["libxml"]["loaded"])
 
     assert(version_info["libxml"].key?("iconv_enabled"))
+    assert(version_info["libxml"].key?("zlib_enabled"))
+    assert(version_info["libxml"].key?("http_enabled"))
     assert(version_info["libxslt"].key?("datetime_enabled"))
   end
 
diff --git a/test/xml/test_entity_reference.rb b/test/xml/test_entity_reference.rb
diff --git a/test/xml/test_schema.rb b/test/xml/test_schema.rb
