Added tests + validate and escape checks for mysql order by.

diff --git a/CRM/Utils/Rule.php b/CRM/Utils/Rule.php
index 08cbcc4a0b4f7278231c2fdee36e7e58311f3209..73b978967bcb94416ca53dbca2333231bc5a4f73 100644 (file)
--- a/CRM/Utils/Rule.php
+++ b/CRM/Utils/Rule.php
@@ -143,6 +143,25 @@ class CRM_Utils_Rule {
     return TRUE;
   }
 
+  /**
+   * Validate that a string is valid order by clause.
+   *
+   * @param $str
+   * @return bool
+   */
+  public static function mysqlOrderBy($str) {
+    // Making a regex for a comma separated list is quite hard and not readable
+    // at all, so we split and loop over.
+    $parts = explode(',', $str);
+    foreach ($parts as $part) {
+      if (!preg_match('/^(([\w_]+)((\.)([\w_]+))?( (asc|desc))?)$/i', trim($part))) {
+        return FALSE;
+      }
+    }
+
+    return TRUE;
+  }
+
   /**
    * @param $str
    *

diff --git a/CRM/Utils/Type.php b/CRM/Utils/Type.php
index 81e8274f91f12f6f378ad9dca1e76eb83959ccb7..7f9ddafddba463021860808c2f37e851c00bb6eb 100644 (file)
--- a/CRM/Utils/Type.php
+++ b/CRM/Utils/Type.php
@@ -152,6 +152,18 @@ class CRM_Utils_Type {
     return $data;
   }
 
+  /**
+   * Helper function to call validate on arrays
+   *
+   * @see validate
+   */
+  public static function validateAll($data, $type, $abort = TRUE) {
+    foreach ($data as $key => $value) {
+      $data[$key] = CRM_Utils_Type::validate($value, $type, $abort);
+    }
+    return $data;
+  }
+
   /**
    * Verify that a variable is of a given type, and apply a bit of processing.
    *
@@ -258,19 +270,35 @@ class CRM_Utils_Type {
 
       case 'MysqlColumnNameLoose':
         if (CRM_Utils_Rule::mysqlColumnNameLoose($data)) {
-          return str_replace('`', '``', $data);
+          $parts = explode('.', str_replace('`', '``', $data));
+          $data = '`'.implode('`.`', $parts).'`';
+
+          return $data;
         }
         break;
 
       case 'MysqlColumnName':
         if (CRM_Utils_Rule::mysqlColumnName($data)) {
+          $parts = explode('.', $data);
+          $data = '`'.implode('`.`', $parts).'`';
+
           return $data;
         }
         break;
 
       case 'MysqlOrderByDirection':
         if (CRM_Utils_Rule::mysqlOrderByDirection($data)) {
-          return $data;
+          return strtolower($data);
+        }
+        break;
+
+      case 'MysqlOrderBy':
+        if (CRM_Utils_Rule::mysqlOrderBy($data)) {
+          $parts = explode(',', $data);
+          foreach ($parts as &$part) {
+            $part = preg_replace_callback('/(?:([\w_]+)(?:(?:\.)([\w_]+))?(?: (asc|desc))?)/i', array('CRM_Utils_Type', 'mysqlOrderByCallback'), trim($part));
+          }
+          return implode(', ', $parts);
         }
         break;
 
@@ -391,7 +419,7 @@ class CRM_Utils_Type {
 
       case 'MysqlOrderByDirection':
         if (CRM_Utils_Rule::mysqlOrderByDirection($data)) {
-          return $data;
+          return strtolower($data);
         }
         break;
 
@@ -414,4 +442,28 @@ class CRM_Utils_Type {
     return NULL;
   }
 
+  /**
+   * preg_replace_callback for MysqlOrderBy escape.
+   */
+  public static function mysqlOrderByCallback($matches) {
+    $output = '';
+
+    // Column or table name.
+    if (isset($matches[1])) {
+      $output .= '`' . $matches[1] . '`';
+    }
+
+    // Column name in case there is a table.
+    if (isset($matches[2]) && $matches[2]) {
+      $output .= '.`' . $matches[2] . '`';
+    }
+
+    // Sort order.
+    if (isset($matches[3]) && $matches[3]) {
+      $output .= ' ' . $matches[3];
+    }
+
+    return $output;
+  }
+
 }

diff --git a/tests/phpunit/CRM/Utils/TypeTest.php b/tests/phpunit/CRM/Utils/TypeTest.php
index 679841fb472d974cc79eb6a41b5bd49310efd5d2..ae131aa268ad52d3006110afcc569c06585120bd 100644 (file)
--- a/tests/phpunit/CRM/Utils/TypeTest.php
+++ b/tests/phpunit/CRM/Utils/TypeTest.php
@@ -37,6 +37,16 @@ class CRM_Utils_TypeTest extends CiviUnitTestCase {
       array(-10, 'Positive', NULL),
       array('-10', 'Positive', NULL),
       array('-10foo', 'Positive', NULL),
+      array('civicrm_column_name', 'MysqlColumnName', 'civicrm_column_name'),
+      array('table.civicrm_column_name', 'MysqlColumnName', 'table.civicrm_column_name'),
+      array('table.civicrm_column_name.toomanydots', 'MysqlColumnName', NULL),
+      array('invalid-column-name', 'MysqlColumnName', NULL),
+      array('column_name, sleep(5)', 'MysqlColumnName', NULL),
+      array('asc', 'MysqlOrderByDirection', 'asc'),
+      array('DESC', 'MysqlOrderByDirection', 'desc'),
+      array('DESCc', 'MysqlOrderByDirection', NULL),
+      array('table.civicrm_column_name desc', 'MysqlOrderBy', 'table.civicrm_column_name desc'),
+      array('table.civicrm_column_name desc,other_column,another_column desc', 'MysqlOrderBy', 'table.civicrm_column_name desc, other_column, another_column desc'),
     );
   }
 
@@ -75,6 +85,16 @@ class CRM_Utils_TypeTest extends CiviUnitTestCase {
       array('-3', 'ContactReference', NULL),
       // Escape function is meant for sql, not xss
       array('<p onclick="alert(\'xss\');">Hello</p>', 'Memo', '<p onclick=\\"alert(\\\'xss\\\');\\">Hello</p>'),
+      array('civicrm_column_name', 'MysqlColumnName', '`civicrm_column_name`'),
+      array('table.civicrm_column_name', 'MysqlColumnName', '`table`.`civicrm_column_name`'),
+      array('table.civicrm_column_name.toomanydots', 'MysqlColumnName', NULL),
+      array('invalid-column-name', 'MysqlColumnName', NULL),
+      array('column_name, sleep(5)', 'MysqlColumnName', NULL),
+      array('asc', 'MysqlOrderByDirection', 'asc'),
+      array('DESC', 'MysqlOrderByDirection', 'desc'),
+      array('DESCc', 'MysqlOrderByDirection', NULL),
+      array('table.civicrm_column_name desc', 'MysqlOrderBy', '`table`.`civicrm_column_name` desc'),
+      array('table.civicrm_column_name desc,other_column,another_column desc', 'MysqlOrderBy', '`table`.`civicrm_column_name` desc, `other_column`, `another_column` desc'),
     );
   }