From 81863d54f79ecc7f04a866e4bfe691a6b2fd9855 Mon Sep 17 00:00:00 2001 From: tompng Date: Sun, 28 Jun 2026 16:31:05 +0900 Subject: [PATCH 1/8] Fix XPath variable handling and invalid value contamination 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`. Add variable coerce and predicates after variables. --- lib/rexml/functions.rb | 1 + lib/rexml/xpath_parser.rb | 39 +++++++++++++++++++++++++++++---------- test/xpath/test_base.rb | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 10 deletions(-) diff --git a/lib/rexml/functions.rb b/lib/rexml/functions.rb index 1a0f85ec..6546fa1b 100644 --- a/lib/rexml/functions.rb +++ b/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) diff --git a/lib/rexml/xpath_parser.rb b/lib/rexml/xpath_parser.rb index edd11f21..9ee5015d 100644 --- a/lib/rexml/xpath_parser.rb +++ b/lib/rexml/xpath_parser.rb @@ -244,8 +244,10 @@ def expr( path_stack, nodeset, context=nil ) end when :variable var_name = path_stack.shift - return @variables[var_name] + value = coerce_variable(@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 ) @@ -319,15 +321,9 @@ def expr( path_stack, nodeset, context=nil ) 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 +333,29 @@ def expr( path_stack, nodeset, context=nil ) leave(:expr, path_stack, nodeset) if @debug end + def apply_remaining_predicates(path_stack, value) + # If evaluated value is not a nodeset, treat it as an empty nodeset. + # TODO: Decide whether REXML should raise type error or keep this behavior. + value = [] unless value.is_a?(Array) + path_stack.unshift(:node) + step(path_stack) do + [:iterate_nodesets, [XPathParser.sort(value)]] + end + end + + # Coerces a variable value to a type that can be used in XPath expressions. + # TODO: Decide whether REXML should warn, raise, or ignore when a variable value is invalid. + def coerce_variable(value) + case value + when Array + value.grep(REXML::Node).uniq + when Numeric, String, true, false + value + else + "" + 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. diff --git a/test/xpath/test_base.rb b/test/xpath/test_base.rb index 1d9dc67e..f8d6784e 100644 --- a/test/xpath/test_base.rb +++ b/test/xpath/test_base.rb @@ -1573,5 +1573,37 @@ 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("") + 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, d, e = XPath.match(doc, '//*') + assert_equal([''], XPath.match(doc, '$x', nil, {})) + assert_equal([''], XPath.match(doc, '$x', nil, { 'x' => nil })) + assert_equal([''], XPath.match(doc, '$x', nil, { 'x' => Object.new })) + 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] })) + end + + def test_variables_invalid_predicates + doc = Document.new("") + # 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 From 5e91bf8f644486c2fa28a79356617ca7f2f86dc3 Mon Sep 17 00:00:00 2001 From: tompng Date: Tue, 7 Jul 2026 12:50:48 +0900 Subject: [PATCH 2/8] Convert node to a nodeset when passed to XPath variable --- lib/rexml/xpath_parser.rb | 2 ++ test/xpath/test_base.rb | 2 ++ 2 files changed, 4 insertions(+) diff --git a/lib/rexml/xpath_parser.rb b/lib/rexml/xpath_parser.rb index 9ee5015d..e5c410d1 100644 --- a/lib/rexml/xpath_parser.rb +++ b/lib/rexml/xpath_parser.rb @@ -347,6 +347,8 @@ def apply_remaining_predicates(path_stack, value) # TODO: Decide whether REXML should warn, raise, or ignore when a variable value is invalid. def coerce_variable(value) case value + when REXML::Node + [value] when Array value.grep(REXML::Node).uniq when Numeric, String, true, false diff --git a/test/xpath/test_base.rb b/test/xpath/test_base.rb index f8d6784e..70e39c57 100644 --- a/test/xpath/test_base.rb +++ b/test/xpath/test_base.rb @@ -1592,6 +1592,8 @@ def test_variables 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_variables_invalid_predicates From d61638e1c3a019a30c75e5ebaffd2cc7b2186305 Mon Sep 17 00:00:00 2001 From: tompng Date: Tue, 14 Jul 2026 04:00:57 +0900 Subject: [PATCH 3/8] XPath function may return a nodeset (example: ), so paths after functions should be consumed --- lib/rexml/xpath_parser.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/rexml/xpath_parser.rb b/lib/rexml/xpath_parser.rb index e5c410d1..824879c3 100644 --- a/lib/rexml/xpath_parser.rb +++ b/lib/rexml/xpath_parser.rb @@ -317,7 +317,10 @@ 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) From 7cdffee3cc08537f5a6ecfe40ed3b58f60e79b57 Mon Sep 17 00:00:00 2001 From: tompng Date: Tue, 14 Jul 2026 03:45:08 +0900 Subject: [PATCH 4/8] Validate variable existence and types --- lib/rexml/xpath_parser.rb | 19 +++++++++++++------ test/xpath/test_base.rb | 8 ++++++-- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/lib/rexml/xpath_parser.rb b/lib/rexml/xpath_parser.rb index 824879c3..69c8017f 100644 --- a/lib/rexml/xpath_parser.rb +++ b/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 @@ -244,7 +245,8 @@ def expr( path_stack, nodeset, context=nil ) end when :variable var_name = path_stack.shift - value = coerce_variable(@variables[var_name]) + value = @variables[var_name] + raise NameError, "Undefined variable: $#{var_name}" if value.nil? return value if path_stack.empty? nodeset = apply_remaining_predicates(path_stack, value) @@ -346,18 +348,23 @@ def apply_remaining_predicates(path_stack, value) end end - # Coerces a variable value to a type that can be used in XPath expressions. - # TODO: Decide whether REXML should warn, raise, or ignore when a variable value is invalid. + # 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 - value.grep(REXML::Node).uniq + unless value.all?(REXML::Node) + raise TypeError, 'Array variable must contain only REXML::Node objects' + end + value.uniq when Numeric, String, true, false value - else + when nil + # Convert to an empty string for backward compatibility "" + else + raise TypeError, "Unsupported variable type: #{value.class}" end end diff --git a/test/xpath/test_base.rb b/test/xpath/test_base.rb index 70e39c57..06e4680a 100644 --- a/test/xpath/test_base.rb +++ b/test/xpath/test_base.rb @@ -1584,9 +1584,13 @@ def test_unimplemented_id_should_not_contaminate_nil def test_variables doc = Document.new("") a, b, c, d, e = XPath.match(doc, '//*') - assert_equal([''], XPath.match(doc, '$x', nil, {})) + assert_raise(NameError) { 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([''], XPath.match(doc, '$x', nil, { 'x' => Object.new })) 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] })) From d19849a59c1d1f8461acd25d3f1f0da1546329e5 Mon Sep 17 00:00:00 2001 From: tompng Date: Tue, 14 Jul 2026 04:16:07 +0900 Subject: [PATCH 5/8] Add test for attribute variables uniqueness --- test/xpath/test_base.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/xpath/test_base.rb b/test/xpath/test_base.rb index 06e4680a..729dc1d5 100644 --- a/test/xpath/test_base.rb +++ b/test/xpath/test_base.rb @@ -1600,6 +1600,12 @@ def test_variables assert_equal([c], XPath.match(doc, '$x/*', nil, { 'x' => b })) end + def test_attribute_variables + doc = Document.new("") + 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("") # Predicates after variable may be invalid depending on variable type. From cf58ecc1130298b7243755a3a15db4f8fcf52bc2 Mon Sep 17 00:00:00 2001 From: tompng Date: Fri, 17 Jul 2026 23:48:47 +0900 Subject: [PATCH 6/8] Revert NameError on missing xpath variable read, just fallback to "" for now --- lib/rexml/xpath_parser.rb | 3 +-- test/xpath/test_base.rb | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/rexml/xpath_parser.rb b/lib/rexml/xpath_parser.rb index 69c8017f..6f5b017e 100644 --- a/lib/rexml/xpath_parser.rb +++ b/lib/rexml/xpath_parser.rb @@ -245,8 +245,7 @@ def expr( path_stack, nodeset, context=nil ) end when :variable var_name = path_stack.shift - value = @variables[var_name] - raise NameError, "Undefined variable: $#{var_name}" if value.nil? + value = @variables.key?(var_name) ? @variables[var_name] : "" return value if path_stack.empty? nodeset = apply_remaining_predicates(path_stack, value) diff --git a/test/xpath/test_base.rb b/test/xpath/test_base.rb index 729dc1d5..a87cd109 100644 --- a/test/xpath/test_base.rb +++ b/test/xpath/test_base.rb @@ -1584,7 +1584,7 @@ def test_unimplemented_id_should_not_contaminate_nil def test_variables doc = Document.new("") a, b, c, d, e = XPath.match(doc, '//*') - assert_raise(NameError) { XPath.match(doc, '$y', nil, { 'x' => 1 }) } + 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] }) } From b4e2846432db6e59be9c0cf0ad0da65198f0340e Mon Sep 17 00:00:00 2001 From: tompng Date: Fri, 17 Jul 2026 23:59:17 +0900 Subject: [PATCH 7/8] Also coerce variable when it is set from `REXML::XPathParser#[]=`, a method only used from test code --- lib/rexml/xpath_parser.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rexml/xpath_parser.rb b/lib/rexml/xpath_parser.rb index 6f5b017e..d4113534 100644 --- a/lib/rexml/xpath_parser.rb +++ b/lib/rexml/xpath_parser.rb @@ -107,7 +107,7 @@ def predicate path, node end def []=( variable_name, value ) - @variables[ variable_name ] = value + @variables[variable_name] = coerce_variable(value) end From c1c8f177b154a6d4a75150234cbaef8b3f000b6b Mon Sep 17 00:00:00 2001 From: tompng Date: Sat, 18 Jul 2026 13:57:51 +0900 Subject: [PATCH 8/8] Remove TODO comment and filed an issue: https://github.com/ruby/rexml/issues/350 --- lib/rexml/xpath_parser.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/rexml/xpath_parser.rb b/lib/rexml/xpath_parser.rb index d4113534..14e71c87 100644 --- a/lib/rexml/xpath_parser.rb +++ b/lib/rexml/xpath_parser.rb @@ -338,8 +338,6 @@ def expr( path_stack, nodeset, context=nil ) end def apply_remaining_predicates(path_stack, value) - # If evaluated value is not a nodeset, treat it as an empty nodeset. - # TODO: Decide whether REXML should raise type error or keep this behavior. value = [] unless value.is_a?(Array) path_stack.unshift(:node) step(path_stack) do