From 5c30a60e305f4a763c2be7bffea5127c1c6e0396 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 14 Apr 2020 08:47:41 -0700
Subject: [PATCH] Tighten Ferret query parsing of empty tokens and empty
 functions

Summary:
Ref T13509. Certain query tokens like `title:=""` are currently accepted by the parser but discarded, and have no impact on the query. This isn't desirable.

Instead, require that tokens making an assertion about field content must be nonempty.

Test Plan: Added unit tests, made them pass.

Maniphest Tasks: T13509

Differential Revision: https://secure.phabricator.com/D21106
---
 .../compiler/PhutilSearchQueryCompiler.php    | 73 +++++++++++++------
 .../PhutilSearchQueryCompilerTestCase.php     | 31 +++++---
 2 files changed, 73 insertions(+), 31 deletions(-)

diff --git a/src/applications/search/compiler/PhutilSearchQueryCompiler.php b/src/applications/search/compiler/PhutilSearchQueryCompiler.php
index eb5ee860c9..0426180197 100644
--- a/src/applications/search/compiler/PhutilSearchQueryCompiler.php
+++ b/src/applications/search/compiler/PhutilSearchQueryCompiler.php
@@ -243,34 +243,26 @@ final class PhutilSearchQueryCompiler
           'Query contains unmatched double quotes.'));
     }
 
-    if ($mode == 'operator') {
-      throw new PhutilSearchQueryCompilerSyntaxException(
-        pht(
-          'Query contains operator ("%s") with no search term.',
-          implode('', $current_operator)));
+    // If the input query has trailing space, like "a b ", we may exit the
+    // parser without a final token.
+    if ($current_function !== null || $current_operator || $current_token) {
+      $token = array(
+        'operator' => $current_operator,
+        'quoted' => false,
+        'value' => $current_token,
+      );
+
+      if ($enable_functions) {
+        $token['function'] = $current_function;
+      }
+
+      $tokens[] = $token;
     }
 
-    $token = array(
-      'operator' => $current_operator,
-      'quoted' => false,
-      'value' => $current_token,
-    );
-
-    if ($enable_functions) {
-      $token['function'] = $current_function;
-    }
-
-    $tokens[] = $token;
-
     $results = array();
     foreach ($tokens as $token) {
       $value = implode('', $token['value']);
       $operator_string = implode('', $token['operator']);
-
-      if (!strlen($value)) {
-        continue;
-      }
-
       $is_quoted = $token['quoted'];
 
       switch ($operator_string) {
@@ -304,6 +296,24 @@ final class PhutilSearchQueryCompiler
               $operator_string));
       }
 
+      if (!strlen($value)) {
+        $require_value = $is_quoted;
+
+        switch ($operator) {
+          default:
+            $require_value = true;
+            break;
+        }
+
+        if ($require_value) {
+          throw new PhutilSearchQueryCompilerSyntaxException(
+            pht(
+              'Query contains a token ("%s") with no search term. Query '.
+              'tokens specify text to search for.',
+              $this->getDisplayToken($token)));
+        }
+      }
+
       $result = array(
         'operator' => $operator,
         'quoted' => $is_quoted,
@@ -371,4 +381,23 @@ final class PhutilSearchQueryCompiler
     return $open_quote.$value.$close_quote;
   }
 
+  private function getDisplayToken(array $token) {
+    if (isset($token['function'])) {
+      $function = $token['function'].':';
+    } else {
+      $function = '';
+    }
+
+    $operator_string = implode('', $token['operator']);
+
+    $value = implode('', $token['value']);
+
+    $is_quoted = $token['quoted'];
+    if ($is_quoted) {
+      $value = $this->quoteToken($value);
+    }
+
+    return sprintf('%s%s%s', $function, $operator_string, $value);
+  }
+
 }
diff --git a/src/applications/search/compiler/__tests__/PhutilSearchQueryCompilerTestCase.php b/src/applications/search/compiler/__tests__/PhutilSearchQueryCompilerTestCase.php
index 647386f52c..0369cc9cd3 100644
--- a/src/applications/search/compiler/__tests__/PhutilSearchQueryCompilerTestCase.php
+++ b/src/applications/search/compiler/__tests__/PhutilSearchQueryCompilerTestCase.php
@@ -36,6 +36,13 @@ final class PhutilSearchQueryCompilerTestCase
       '"cat' => false,
       'A"' => false,
       'A"B"' => '+"A" +"B"',
+
+      // Trailing whitespace should be discarded.
+      'a b ' => '+"a" +"b"',
+
+      // Functions must have search text.
+      '""' => false,
+      '-' => false,
     );
 
     $this->assertCompileQueries($tests);
@@ -56,7 +63,6 @@ final class PhutilSearchQueryCompilerTestCase
       'cat -dog' => '+[cat] -[dog]',
     );
     $this->assertCompileQueries($quote_tests, '+ -><()~*:[]&|');
-
   }
 
   public function testCompileQueriesWithStemming() {
@@ -133,6 +139,9 @@ final class PhutilSearchQueryCompilerTestCase
       '"'.$mao.'"' => array(
         array(null, $op_and, $mao),
       ),
+      'title:' => false,
+      'title:+' => false,
+      'title:+""' => false,
     );
 
     $this->assertCompileFunctionQueries($function_tests);
@@ -199,15 +208,19 @@ final class PhutilSearchQueryCompilerTestCase
       $compiler = id(new PhutilSearchQueryCompiler())
         ->setEnableFunctions(true);
 
-      $tokens = $compiler->newTokens($input);
+      try {
+        $tokens = $compiler->newTokens($input);
 
-      $result = array();
-      foreach ($tokens as $token) {
-        $result[] = array(
-          $token->getFunction(),
-          $token->getOperator(),
-          $token->getValue(),
-        );
+        $result = array();
+        foreach ($tokens as $token) {
+          $result[] = array(
+            $token->getFunction(),
+            $token->getOperator(),
+            $token->getValue(),
+          );
+        }
+      } catch (PhutilSearchQueryCompilerSyntaxException $ex) {
+        $result = false;
       }
 
       $this->assertEqual(