Fix XPath variable handling and invalid value contamination (#342)

Fixes #15

Fix nil and other invalid value contamination. Nil can be injected
through variables and through id function.
Unimplemented function `id()` is fixed to return `[]` instead of `nil`.
Applies predicates/path evaluation if it exists after variable.
Add variable validation (TypeError if invalid).

Variable existence check (NameError if undefined variable exist in xpath
even if it is not referenced) is excluded from this PR, and just
fallbacks to "" for now.

Minor behavior changes for an invalid XPath match:
```ruby
doc = REXML::Document.new("<root/>")
REXML::XPath.match(doc, '($x)[1<2]', {}, {'x'=>42})
#=> [42] → []
REXML::XPath.match(doc, '$x[1<2]', {}, {'x'=>42})
#=> [42] → []
```
It may raise TypeError (because `$x` wasn't evaluated to a nodeset),
though, it shoudn't return `[42]`

## Note
Although nodeset as a variable has been accepted before, the code added
in pull request explicitly permits it.
Nokogiri only accepts string value as a variable, so there's an option
to limit the value types here.
Accepting nodeset may cause a problem if the passed nodes doesn't belong
to a single document root. (or just consider such case as unsupported)

Author: tompng

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

@@ -75,7 +75,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
@@ -103,7 +104,7 @@ def predicate path, node
     end
 
     def []=( variable_name, value )
-      @variables[ variable_name ] = value
+      @variables[variable_name] = coerce_variable(value)
     end
 
 
@@ -267,8 +268,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 )
@@ -338,19 +341,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
@@ -360,6 +360,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

@@ -1563,5 +1563,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