Change XPath available function check from denylist to allowlist

lib/rexml/functions.rb

@@ -1,4 +1,7 @@
 # frozen_string_literal: false
+
+require 'set'
+
 module REXML
   # If you add a method, keep in mind two things:
   # (1) the first argument will always be a list of nodes from which to
@@ -13,14 +16,35 @@ def initialize
       @node_indexes = nil
     end
 
-    INTERNAL_METHODS = [
-      :context=,
-      :node_indexes=,
-      :target_named_node,
-      :send,
-      :compare_language,
-      :string_value,
-    ].freeze
+    AVAILABLE_FUNCTIONS = %w[
+      boolean
+      ceiling
+      concat
+      contains
+      count
+      false
+      floor
+      id
+      lang
+      last
+      local-name
+      name
+      namespace-uri
+      normalize-space
+      not
+      number
+      position
+      round
+      starts-with
+      string
+      string-length
+      substring
+      substring-after
+      substring-before
+      sum
+      translate
+      true
+    ].to_set.freeze
 
     def context=(value); @context = value; end
 
@@ -402,17 +426,21 @@ def round( number )
       end
     end
 
-    def send(name, *args)
-      name = name.to_sym
-      if self.class.method_defined?(name, false) and
-          !INTERNAL_METHODS.include?(name)
-        super
+    def call(name, args:, fallback: nil)
+      name = name.to_s
+      if AVAILABLE_FUNCTIONS.include?(name)
+        __send__(name.tr('-', '_'), *args)
+      elsif fallback.nil?
+        raise ArgumentError, "Unknown XPath function: #{name}"
       else
-        # TODO: Maybe, this is not XPath spec behavior.
-        # This behavior must be reconsidered.
-        []
+        fallback
       end
     end
+
+    # For compatibility. Use `call` instead.
+    def send(name, *args)
+      call(name.to_s.tr('_', '-'), args: args, fallback: [])
+    end
   end
 
   # Using this singleton instance may cause thread-safety issues.

lib/rexml/xpath_parser.rb

@@ -329,7 +329,7 @@ def expr( path_stack, nodeset, context=nil )
           return -@functions.number(res)
         when :not
         when :function
-          func_name = path_stack.shift.tr('-','_')
+          func_name = path_stack.shift
           arguments = path_stack.shift
 
           if nodeset.size != 1
@@ -351,7 +351,11 @@ def expr( path_stack, nodeset, context=nil )
             expr(arg, nodeset, target_context)
           end
           @functions.context = target_context
-          result = @functions.send(func_name, *args)
+
+          # TODO: Maybe, this is not XPath spec behavior.
+          # This behavior must be reconsidered.
+          result = @functions.call(func_name.tr('_', '-'), args: args, fallback: [])
+
           return result if path_stack.empty?
 
           nodeset = apply_remaining_predicates(path_stack, result)

test/functions/test_base.rb

@@ -19,9 +19,10 @@ def test_available_functions
         name namespace-uri normalize-space not number position round starts-with
         string string-length substring substring-after substring-before sum translate true
       ]
-      methods = REXML::FunctionsClass.instance_methods(false) -
-        REXML::FunctionsClass::INTERNAL_METHODS
-      assert_equal expected_functions, methods.map { |m| m.to_s.tr('_', '-') }.sort
+      assert_equal expected_functions, REXML::FunctionsClass::AVAILABLE_FUNCTIONS.to_a.sort
+      expected_functions.each do |name|
+        assert(REXML::FunctionsClass.method_defined?(name.tr('-', '_')), name)
+      end
     end
 
     def test_functions
@@ -297,13 +298,21 @@ def test_unregistered_method
     end
 
     def test_nonexistent_function
+      assert_raise(ArgumentError) { Functions.call('nonexistent', args: []) }
+      assert_raise(ArgumentError) { Functions.call('string_length', args: ['abc']) }
+      assert_empty(Functions.call('string_length', args: ['abc'], fallback: []))
+      assert_equal(false, Functions.call('nonexistent', args: [], fallback: false))
+      assert_equal(3, Functions.call(:'string-length', args: ['abc']))
+
       doc = Document.new("<root><nonexistent/></root>")
       # TODO: Maybe, this is not XPath spec behavior.
       # This behavior must be reconsidered.
       assert_nil(XPath::first(doc.root, "nonexistent()"))
       assert_empty(XPath::match(doc.root, "nonexistent()"))
       assert_empty(XPath::match(doc, "42()/*"))
-      assert_empty(Functions.send('42'))
+      assert_equal(3, Functions.send('string_length', 'abc'))
+      assert_equal(3, Functions.send(:string_length, 'abc'))
+      assert_empty(Functions.send(:nonexistent))
     end
   end
 end