Fix XPath variable handling and invalid value contamination by tompng · Pull Request #342 · ruby/rexml

tompng · 2026-06-28T08:40:10Z

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) and variable existence check (NameError if undefined variable referenced)

Minor behavior changes for an invalid XPath match:

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)

Copilot

Pull request overview

This PR adjusts REXML’s XPath evaluation to prevent nil/invalid values from “contaminating” results, primarily by making id() return an empty node-set and by coercing variable values + applying remaining predicates/steps after variable/group evaluation.

Changes:

Make unimplemented XPath function id() return [] (empty node-set) instead of nil.
Add variable coercion and ensure remaining predicates/steps are applied after variable and grouped expressions.
Add/extend tests covering id() and variable handling (including invalid predicate application on non-node-set variables).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
test/xpath/test_base.rb	Adds regression tests for `id()` and variable coercion/predicate behavior.
lib/rexml/xpath_parser.rb	Implements variable coercion and applies remaining predicates after variable/group evaluation.
lib/rexml/functions.rb	Changes `id()` to return an empty node-set (`[]`).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

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.

naitoh · 2026-07-12T09:53:58Z

+      # 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.


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.

According to the XPath specification, anything other than a node set results in an error.

https://www.w3.org/TR/1999/REC-xpath-19991116/#node-sets

It is an error if the expression to be filtered does not evaluate to a node-set.

It is an error if the expression does not evaluate to a node-set.

I think it would be better to raise an error, as that would make the processing clearer.
What do you think? @kou @tompng

I think it's better to align with the spec, but it requires much more changes
In my opinion, it must out of scope of this PR, must be done in a separated PR on top of this and on #343
For example, these xpath that requires requires intermediate value to be a nodeset should be changed to raise error.

# Function argument that requires nodeset local-name(1) sum("string") count(1 < 2) # Path after function string(/a)[predicate] string(/a)/path # Path after non-nodeset variable $x[predicate] $x/path # path after literal "string"[predicate] 42/path # path after neg -number(/a)[predicate] -number(/a)/path

naitoh · 2026-07-12T10:15:04Z

+    # 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.


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.

According to the XPath specification, if a variable name is not bound to any value, an error occurs.

https://www.w3.org/TR/1999/REC-xpath-19991116/#section-Basics

A VariableReference evaluates to the value to which the variable name is bound in the set of variable bindings in the context.

I think it would be better to raise an error, as that would make the processing clearer.
What do you think? @kou @tompng

I think referencing unbound variable should raise error. Concern: when to do that, and what kind of error should it raise.

when :variable var_name = path_stack.shift # Handle unbound variables raise unless @variables.key?(var_name) # Handle conversion and validation of variable value # @variables[var_name]==nil case is handled here value = coerce_variable(@variables[var_name]) ...

Even if we reject unbound variables, this TODO comment for bound variables (reject bound invalid-value variable or convert it to a fallback value "") still remains.

Concern: when to do that, and what kind of error should it raise.

I think it would be better to check immediately after parsing and before evaluation begins, rather than raising an exception when the variable statement is evaluated.
I think NameError is the appropriate error type to raise.

Reasons:

If we raise only when the reference is actually evaluated, the error becomes document-dependent. For example, //foo[@bar=$typo] does not raise when no foo reaches the predicate, and or/and short-circuits can also skip the reference. So the typo silently returns [] in exactly the case we most want to detect.

Even if we reject unbound variables, this TODO comment for bound variables (reject bound invalid-value variable or convert it to a fallback value "") still remains.

I think the preferred design is to fall back to "" for compatibility purposes only when the value is nil, and to reject the input in all other cases.
In this case, I think TypeError is the appropriate type of error to raise.

Fix crash with nil XPath variables #13

I think it would be better to check immediately after parsing and before evaluation

It makes sense. Applied in 7cdffee
The added check is worth enabled for normal use, so it is applied even if strict is false (default)

…ions should be consumed

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

tompng · 2026-07-13T19:19:15Z

+        unless value.all?(REXML::Node)
+          raise TypeError, 'Array variable must contain only REXML::Node objects'
+        end
+        value.uniq


Ensured to work correctly with test added in d19849a

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

+    def variables=(vars)
+      vars = vars.transform_values { |v| coerce_variable(v) }
      @functions.variables = vars
      @variables = vars
    end


+      when Array
+        unless value.all?(REXML::Node)
+          raise TypeError, 'Array variable must contain only REXML::Node objects'
+        end
+        value.uniq


Copilot AI review requested due to automatic review settings June 28, 2026 08:40

Copilot started reviewing on behalf of tompng June 28, 2026 08:40 View session

Copilot AI reviewed Jun 28, 2026

View reviewed changes

Comment thread lib/rexml/xpath_parser.rb

Comment thread test/xpath/test_base.rb Outdated

tompng force-pushed the fix_xpath_nil_contamination branch from 3f20d0b to f343f5b Compare June 28, 2026 09:23

Copilot AI review requested due to automatic review settings July 7, 2026 03:51

Copilot started reviewing on behalf of tompng July 7, 2026 03:52 View session

Copilot AI reviewed Jul 7, 2026

View reviewed changes

tompng mentioned this pull request Jul 7, 2026

Fix xpath functions related to names #343

Open

tompng added 2 commits July 12, 2026 12:02

Convert node to a nodeset when passed to XPath variable

5e91bf8

naitoh reviewed Jul 12, 2026

View reviewed changes

XPath function may return a nodeset (example: ), so paths after funct…

d61638e

…ions should be consumed

Copilot AI review requested due to automatic review settings July 13, 2026 19:02

tompng force-pushed the fix_xpath_nil_contamination branch from a31f5ce to 06bafbe Compare July 13, 2026 19:02

Copilot started reviewing on behalf of tompng July 13, 2026 19:02 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

tompng added 2 commits July 14, 2026 04:07

Validate variable existence and types

7cdffee

Add test for attribute variables uniqueness

d19849a

Copilot AI review requested due to automatic review settings July 13, 2026 19:16

tompng force-pushed the fix_xpath_nil_contamination branch from 06bafbe to d19849a Compare July 13, 2026 19:16

Copilot started reviewing on behalf of tompng July 13, 2026 19:16 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

		# 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.

		# 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.

Uh oh!

Conversation

tompng commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

naitoh Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

tompng Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

naitoh Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

tompng Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

naitoh Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

tompng Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

tompng Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tompng commented Jun 28, 2026 •

edited

Loading

tompng Jul 13, 2026 •

edited

Loading