fix: css pseudo-class error raised by XPathVisitor (#3223)

**What problem is this PR intended to solve?**

After some thought, I decided I didn't like the solution to #3193
introduced in #3197, in which the exception was raised by the CSS
parser, because it's an XPath translation problem and not a CSS problem.

This PR reverts that change, and instead raises the exception from the
XPathVisitor class when it tries to translate the pseudo-class selector
or function into an XPath function call.


**Have you included adequate test coverage?**

Yes.


**Does this change affect the behavior of either the C or the Java
implementations?**

N/A

CHANGELOG.md

@@ -16,7 +16,7 @@ Nokogiri follows [Semantic Versioning](https://semver.org/), please see the [REA
 ### Fixed
 
 * `Node#clone`, `NodeSet#clone`, and `*::Document#clone` all properly copy the metaclass of the original as expected. Previously, `#clone` had been aliased to `#dup` for these classes (since v1.3.0 in 2009). [#316, #3117] @flavorjones
-* CSS queries for pseudo-selectors that cannot be transpiled into XPath queries now raise a more descriptive `Nokogiri::CSS::SyntaxError` when they are parsed. Previously, an invalid XPath query was created and a hard-to-understand XPath error was being raised by the query engine. [#3197] @flavorjones
+* CSS queries for pseudo-selectors that cannot be translated into XPath expressions now raise a more descriptive `Nokogiri::CSS::SyntaxError` when they are parsed. Previously, an invalid XPath expression was evaluated and a hard-to-understand XPath error was raised by the query engine. [#3193] @flavorjones
 * [CRuby] libgumbo (the HTML5 parser) treats reaching max-depth as EOF. This addresses a class of issues when the parser is interrupted in this way. [#3121] @stevecheckoway
 * [CRuby] Update node GC lifecycle to avoid a potential memory leak with fragments in libxml 2.13.0 caused by changes in `xmlAddChild`. [#3156] @flavorjones
 

lib/nokogiri/css/parser.y

@@ -1,6 +1,6 @@
 class Nokogiri::CSS::Parser
 
-token FUNCTION INCLUDES DASHMATCH LBRACE HASH PLUS MINUS GREATER S STRING IDENT
+token FUNCTION INCLUDES DASHMATCH LBRACE HASH PLUS GREATER S STRING IDENT
 token COMMA NUMBER PREFIXMATCH SUFFIXMATCH SUBSTRINGMATCH TILDE NOT_EQUAL
 token SLASH DOUBLESLASH NOT EQUAL RPAREN LSQUARE RSQUARE HAS
 
@@ -143,17 +143,13 @@ rule
  raise Racc::ParseError, "parse error on IDENT '#{val[1]}'"
  end
  }
- | IDENT PLUS NUMBER { # n+3
+ | IDENT PLUS NUMBER { # n+3, -n+3
  if val[0] == 'n'
  val.unshift("1")
  result = Node.new(:NTH, val)
- else
- raise Racc::ParseError, "parse error on IDENT '#{val[0]}'"
- end
- }
- | MINUS IDENT PLUS NUMBER { # -n+3
- if val[1] == 'n'
- val[0] = '-1'
+ elsif val[0] == '-n'
+ val[0] = 'n'
+ val.unshift("-1")
  result = Node.new(:NTH, val)
  else
  raise Racc::ParseError, "parse error on IDENT '#{val[1]}'"

lib/nokogiri/css/tokenizer.rb

@@ -63,10 +63,10 @@ def _next_token
  when (text = @ss.scan(/has\([\s]*/))
  action { [:HAS, text] }
 
- when (text = @ss.scan(/([_A-Za-z]|[^\0-\177]|(\\[0-9A-Fa-f]{1,6}(\r\n|[\s])?|\\[^\n\r\f0-9A-Fa-f]))([_A-Za-z0-9-]|[^\0-\177]|(\\[0-9A-Fa-f]{1,6}(\r\n|[\s])?|\\[^\n\r\f0-9A-Fa-f]))*\([\s]*/))
+ when (text = @ss.scan(/-?([_A-Za-z]|[^\0-\177]|(\\[0-9A-Fa-f]{1,6}(\r\n|[\s])?|\\[^\n\r\f0-9A-Fa-f]))([_A-Za-z0-9-]|[^\0-\177]|(\\[0-9A-Fa-f]{1,6}(\r\n|[\s])?|\\[^\n\r\f0-9A-Fa-f]))*\([\s]*/))
  action { [:FUNCTION, text] }
 
- when (text = @ss.scan(/([_A-Za-z]|[^\0-\177]|(\\[0-9A-Fa-f]{1,6}(\r\n|[\s])?|\\[^\n\r\f0-9A-Fa-f]))([_A-Za-z0-9-]|[^\0-\177]|(\\[0-9A-Fa-f]{1,6}(\r\n|[\s])?|\\[^\n\r\f0-9A-Fa-f]))*/))
+ when (text = @ss.scan(/-?([_A-Za-z]|[^\0-\177]|(\\[0-9A-Fa-f]{1,6}(\r\n|[\s])?|\\[^\n\r\f0-9A-Fa-f]))([_A-Za-z0-9-]|[^\0-\177]|(\\[0-9A-Fa-f]{1,6}(\r\n|[\s])?|\\[^\n\r\f0-9A-Fa-f]))*/))
  action { [:IDENT, text] }
 
  when (text = @ss.scan(/\#([_A-Za-z0-9-]|[^\0-\177]|(\\[0-9A-Fa-f]{1,6}(\r\n|[\s])?|\\[^\n\r\f0-9A-Fa-f]))+/))
@@ -120,9 +120,6 @@ def _next_token
  when (text = @ss.scan(/-?([0-9]+|[0-9]*\.[0-9]+)/))
  action { [:NUMBER, text] }
 
- when (text = @ss.scan(/[\s]*\-[\s]*/))
- action { [:MINUS, text] }
-
  when (text = @ss.scan(/[\s]*\/\/[\s]*/))
  action { [:DOUBLESLASH, text] }
 

lib/nokogiri/css/tokenizer.rex

@@ -14,6 +14,7 @@ macro
  nmchar ([_A-Za-z0-9-]|{nonascii}|{escape})
  nmstart ([_A-Za-z]|{nonascii}|{escape})
  name {nmstart}{nmchar}*
+ ident -?{name}
  charref {nmchar}+
  string1 "([^\n\r\f"]|{nl}|{nonascii}|{escape})*(?<!\\)(?:\\{2})*"
  string2 '([^\n\r\f']|{nl}|{nonascii}|{escape})*(?<!\\)(?:\\{2})*'
@@ -24,8 +25,8 @@ rule
 # [:state] pattern [actions]
 
  has\({w} { [:HAS, text] }
- {name}\({w}  { [:FUNCTION, text] }
- {name}  { [:IDENT, text] }
+ {ident}\({w} { [:FUNCTION, text] }
+ {ident} { [:IDENT, text] }
  \#{charref} { [:HASH, text] }
  {w}~={w} { [:INCLUDES, text] }
  {w}\|={w} { [:DASHMATCH, text] }
@@ -43,7 +44,6 @@ rule
  {w}~{w} { [:TILDE, text] }
  \:not\({w} { [:NOT, text] }
  {num} { [:NUMBER, text] }
- {w}\-{w} { [:MINUS, text] }
  {w}\/\/{w} { [:DOUBLESLASH, text] }
  {w}\/{w} { [:SLASH, text] }
 

lib/nokogiri/css/xpath_visitor.rb

@@ -128,6 +128,8 @@ def visit_function(node)
  is_direct = node.value[1].value[0].nil? # e.g. "has(> a)", "has(~ a)", "has(+ a)"
  ".#{"//" unless is_direct}#{node.value[1].accept(self)}"
  else
+ validate_xpath_function_name(node.value.first)
+
  # xpath function call, let's marshal those arguments
  args = ["."]
  args += node.value[1..-1].map do |n|
@@ -207,6 +209,7 @@ def visit_pseudo_class(node)
  when "parent" then "node()"
  when "root" then "not(parent::*)"
  else
+ validate_xpath_function_name(node.value.first)
  "nokogiri:#{node.value.first}(.)"
  end
  end
@@ -270,6 +273,12 @@ def accept(node)
 
  private
 
+ def validate_xpath_function_name(name)
+ if name.start_with?("-")
+ raise Nokogiri::CSS::SyntaxError, "Invalid XPath function name '#{name}'"
+ end
+ end
+
  def html5_element_name_needs_namespace_handling(node)
  # if this is the wildcard selector "*", use it as normal
  node.value.first != "*" &&

test/css/test_tokenizer.rb

@@ -262,8 +262,7 @@ def test_scan_nth
  [:IDENT, "x"],
  [":", ":"],
  [:FUNCTION, "nth-child("],
- [:MINUS, "-"],
- [:IDENT, "n"],
+ [:IDENT, "-n"],
  [:PLUS, "+"],
  [:NUMBER, "3"],
  [:RPAREN, ")"],

test/css/test_xpath_visitor.rb

@@ -372,7 +372,8 @@ def assert_xpath(expecteds, asts)
 
  it "raises an exception for pseudo-classes that are not XPath Names" do
  # see https://github.com/sparklemotion/nokogiri/issues/3193
- assert_raises(Nokogiri::CSS::SyntaxError) { parser.parse("div:-moz-drag-over") }
+ assert_raises(Nokogiri::CSS::SyntaxError) { Nokogiri::CSS.xpath_for("div:-moz-drag-over") }
+ assert_raises(Nokogiri::CSS::SyntaxError) { Nokogiri::CSS.xpath_for("div:-moz-drag-over()") }
  end
  end