Fix XPath variable handling and invalid value contamination

lib/rexml/functions.rb

@@ -57,6 +57,7 @@ def count( node_set )
     # Since REXML is non-validating, this method is not implemented as it
     # requires a DTD
     def id( object )
+      []
     end
 
     def local_name(node_set=nil)

lib/rexml/xpath_parser.rb

@@ -72,7 +72,8 @@ def namespaces=( namespaces={} )
       @namespaces = namespaces
     end
 
-    def variables=( vars={} )
+    def variables=(vars)
+      vars = vars.transform_values { |v| coerce_variable(v) }
       @functions.variables = vars
       @variables = vars
     end
@@ -106,7 +107,7 @@ def predicate path, node
     end
 
     def []=( variable_name, value )
-      @variables[ variable_name ] = value
+      @variables[variable_name] = coerce_variable(value)
     end
 
 
@@ -244,8 +245,10 @@ def expr( path_stack, nodeset, context=nil )
           end
         when :variable
           var_name = path_stack.shift
-          return @variables[var_name]
+          value = @variables.key?(var_name) ? @variables[var_name] : ""
+          return value if path_stack.empty?
 
+          nodeset = apply_remaining_predicates(path_stack, value)
         when :eq, :neq, :lt, :lteq, :gt, :gteq
           left = expr( path_stack.shift, nodeset.dup, context )
           right = expr( path_stack.shift, nodeset.dup, context )
@@ -315,19 +318,16 @@ def expr( path_stack, nodeset, context=nil )
             expr(arg, nodeset, target_context)
           end
           @functions.context = target_context
-          return @functions.send(func_name, *args)
+          result = @functions.send(func_name, *args)
+          return result if path_stack.empty?
+
+          nodeset = apply_remaining_predicates(path_stack, result)
         when :group
           sub_expression = path_stack.shift
           result = expr(sub_expression, nodeset, context)
-          if result.is_a?(Array)
-            # If result is a nodeset, apply following predicates
-            path_stack.unshift(:node)
-            nodeset = step(path_stack) do
-              [:iterate_nodesets, [XPathParser.sort(result)]]
-            end
-          else
-            return result
-          end
+          return result if path_stack.empty?
+
+          nodeset = apply_remaining_predicates(path_stack, result)
         else
           raise "[BUG] Unexpected path: <#{op.inspect}>: <#{path_stack.inspect}>"
         end
@@ -337,6 +337,34 @@ def expr( path_stack, nodeset, context=nil )
       leave(:expr, path_stack, nodeset) if @debug
     end
 
+    def apply_remaining_predicates(path_stack, value)
+      value = [] unless value.is_a?(Array)
+      path_stack.unshift(:node)
+      step(path_stack) do
+        [:iterate_nodesets, [XPathParser.sort(value)]]
+      end
+    end
+
+    # Validates and coerces a variable value to a type that can be used in XPath expressions.
+    def coerce_variable(value)
+      case value
+      when REXML::Node
+        [value]
+      when Array
+        unless value.all?(REXML::Node)
+          raise TypeError, 'Array variable must contain only REXML::Node objects'
+        end
+        value.uniq
+      when Numeric, String, true, false
+        value
+      when nil
+        # Convert to an empty string for backward compatibility
+        ""
+      else
+        raise TypeError, "Unsupported variable type: #{value.class}"
+      end
+    end
+
     # Determines if a predicate expression is dependent on the position of nodes.
     # Returns false if the expression is guaranteed to be position-independent.
     # Returns true if the expression might be position-dependent.

test/xpath/test_base.rb

@@ -1573,5 +1573,49 @@ def test_reverse_axis_function_argument_sort
       assert_equal(["e"], XPath.match(doc, "//e[10 + preceding-sibling::* = 11]").map(&:name))
       assert_equal(["e"], XPath.match(doc, "//e[preceding-sibling::* = '1']").map(&:name))
     end
+
+    def test_unimplemented_id_should_not_contaminate_nil
+      doc = Document.new("<root/>")
+      assert_equal([], XPath.match(doc, 'id("foo")'))
+      assert_equal([], XPath.match(doc, 'id("foo")[1]'))
+      assert_equal([], XPath.match(doc, 'id("foo")/bar'))
+    end
+
+    def test_variables
+      doc = Document.new("<a><b><c/></b><d><e/></d></a>")
+      a, b, c, d, e = XPath.match(doc, '//*')
+      assert_equal([''], XPath.match(doc, '$y', nil, { 'x' => 1 }))
+      assert_raise(TypeError) { XPath.match(doc, '$x', nil, { 'x' => Object.new }) }
+      assert_raise(TypeError) { XPath.match(doc, '/a', nil, { 'x' => Object.new }) }
+      assert_raise(TypeError) { XPath.match(doc, '$x', nil, { 'x' => [a, Object.new, b] }) }
+      assert_equal([1], XPath.match(doc, '$x', nil, { 'x' => 1 }))
+      assert_equal([false], XPath.match(doc, '$x', nil, { 'x' => false }))
+      assert_equal([''], XPath.match(doc, '$x', nil, { 'x' => nil }))
+      assert_equal([3], XPath.match(doc, 'count($x)', nil, { 'x' => [b, c, d] }))
+      assert_equal([3], XPath.match(doc, 'count($x)', nil, { 'x' => [a, a, b, b, c, c] }))
+      assert_equal([b, c, d], XPath.match(doc, '$x', nil, { 'x' => [d, c, b] }))
+      assert_equal([a], XPath.match(doc, '//*[name()=$x]', nil, { 'x' => 'a' }))
+      assert_equal([c, e], XPath.match(doc, '$x/*', nil, { 'x' => [b, d] }))
+      assert_equal([c], XPath.match(doc, '$x/*', nil, { 'x' => [b] }))
+      assert_equal([c], XPath.match(doc, '$x/*', nil, { 'x' => b }))
+    end
+
+    def test_attribute_variables
+      doc = Document.new("<a a='1'><b a='1'/></a>")
+      a1, a2 = XPath.match(doc, '//attribute::*')
+      assert_equal([a1, a2], XPath.match(doc, '$x', nil, { 'x' => [a2, a1] }))
+    end
+
+    def test_variables_invalid_predicates
+      doc = Document.new("<root/>")
+      # Predicates after variable may be invalid depending on variable type.
+      # It can raise an exception such as TypeError, or treat the predicate result as an empty node set,
+      # but it should not return the variable value itself.
+      valid_result = [:exception, []]
+      actual = (XPath.match(doc, '$x[1<2]', nil, { 'x' => 42 }) rescue :exception)
+      assert_includes(valid_result, actual)
+      actual = (XPath.match(doc, '($x)[1<2]', nil, { 'x' => 42 }) rescue :exception)
+      assert_includes(valid_result, actual)
+    end
   end
 end