From 3c972945880cb14189cc9e0b9db867e40ebd9dee Mon Sep 17 00:00:00 2001 From: Mattias Michaux Date: Wed, 25 May 2016 09:45:31 +0200 Subject: [PATCH] backport of 4.7. --- CRM/Contact/BAO/Query.php | 191 ++++++++++++-------- CRM/Core/Page/AJAX.php | 2 +- CRM/Utils/Rule.php | 15 +- CRM/Utils/Sort.php | 6 +- CRM/Utils/Type.php | 22 ++- tests/phpunit/CRM/Contact/BAO/QueryTest.php | 2 +- tests/phpunit/CRM/Utils/TypeTest.php | 39 ++-- 7 files changed, 160 insertions(+), 117 deletions(-) diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index 93a8dd35e7..9ab5622e9f 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -4534,7 +4534,7 @@ civicrm_relationship.is_permission_a_b = 0 * The offset for the query. * @param int $rowCount * The number of rows to return. - * @param string $sort + * @param string|CRM_Utils_Sort $sort * The order by string. * @param bool $count * Is this a count only query ?. @@ -4602,84 +4602,7 @@ civicrm_relationship.is_permission_a_b = 0 $order = $orderBy = $limit = ''; if (!$count) { - $config = CRM_Core_Config::singleton(); - if ($config->includeOrderByClause || - isset($this->_distinctComponentClause) - ) { - if ($sort) { - if (is_string($sort)) { - $orderBy = $sort; - } - else { - $orderBy = trim($sort->orderBy()); - } - if (!empty($orderBy)) { - // this is special case while searching for - // change log CRM-1718 - if (preg_match('/sort_name/i', $orderBy)) { - $orderBy = str_replace('sort_name', 'contact_a.sort_name', $orderBy); - } - - $orderBy = CRM_Utils_Type::escape($orderBy, 'String'); - $order = " ORDER BY $orderBy"; - - if ($sortOrder) { - $sortOrder = CRM_Utils_Type::escape($sortOrder, 'String'); - $order .= " $sortOrder"; - } - - // always add contact_a.id to the ORDER clause - // so the order is deterministic - if (strpos('contact_a.id', $order) === FALSE) { - $order .= ", contact_a.id"; - } - } - } - elseif ($sortByChar) { - $order = " ORDER BY UPPER(LEFT(contact_a.sort_name, 1)) asc"; - } - else { - $order = " ORDER BY contact_a.sort_name asc, contact_a.id"; - } - } - - // hack for order clause - if ($order) { - $fieldStr = trim(str_replace('ORDER BY', '', $order)); - $fieldOrder = explode(' ', $fieldStr); - $field = $fieldOrder[0]; - - if ($field) { - switch ($field) { - case 'city': - case 'postal_code': - $this->_whereTables["civicrm_address"] = 1; - $order = str_replace($field, "civicrm_address.{$field}", $order); - break; - - case 'country': - case 'state_province': - $this->_whereTables["civicrm_{$field}"] = 1; - $order = str_replace($field, "civicrm_{$field}.name", $order); - break; - - case 'email': - $this->_whereTables["civicrm_email"] = 1; - $order = str_replace($field, "civicrm_email.{$field}", $order); - break; - - default: - //CRM-12565 add "`" around $field if it is a pseudo constant - foreach ($this->_pseudoConstantsSelect as $key => $value) { - if (!empty($value['element']) && $value['element'] == $field) { - $order = str_replace($field, "`{$field}`", $order); - } - } - } - $this->_fromClause = self::fromClause($this->_tables, NULL, NULL, $this->_primaryLocation, $this->_mode); - $this->_simpleFromClause = self::fromClause($this->_whereTables, NULL, NULL, $this->_primaryLocation, $this->_mode); - } - } + list($order, $additionalFromClause) = $this->prepareOrderBy($sort, $sortByChar, $sortOrder, $additionalFromClause); if ($rowCount > 0 && $offset >= 0) { $offset = CRM_Utils_Type::escape($offset, 'Int'); @@ -5852,4 +5775,114 @@ AND displayRelType.is_active = 1 return array(CRM_Utils_Array::value($op, $qillOperators, $op), $fieldValue); } + /** + * Parse and assimilate the various sort options. + * + * Side-effect: if sorting on a common column from a related table (`city`, `postal_code`, + * `email`), the related table may be joined automatically. + * + * At time of writing, this code is deeply flawed and should be rewritten. For the moment, + * it's been extracted to a standalone function. + * + * @param string|CRM_Utils_Sort $sort + * The order by string. + * @param bool $sortByChar + * If true returns the distinct array of first characters for search results. + * @param null $sortOrder + * Who knows? Hu knows. He who knows Hu knows who. + * @param string $additionalFromClause + * Should be clause with proper joins, effective to reduce where clause load. + * @return array + * list(string $orderByClause, string $additionalFromClause). + */ + protected function prepareOrderBy($sort, $sortByChar, $sortOrder, $additionalFromClause) { + $config = CRM_Core_Config::singleton(); + if ($config->includeOrderByClause || + isset($this->_distinctComponentClause) + ) { + if ($sort) { + if (is_string($sort)) { + $orderBy = $sort; + } + else { + $orderBy = trim($sort->orderBy()); + } + if (!empty($orderBy)) { + // this is special case while searching for + // change log CRM-1718 + if (preg_match('/sort_name/i', $orderBy)) { + $orderBy = str_replace('sort_name', 'contact_a.sort_name', $orderBy); + } + + $orderBy = CRM_Utils_Type::escape($orderBy, 'String'); + $order = " ORDER BY $orderBy"; + + if ($sortOrder) { + $sortOrder = CRM_Utils_Type::escape($sortOrder, 'String'); + $order .= " $sortOrder"; + } + + // always add contact_a.id to the ORDER clause + // so the order is deterministic + if (strpos('contact_a.id', $order) === FALSE) { + $order .= ", contact_a.id"; + } + } + } + elseif ($sortByChar) { + $order = " ORDER BY UPPER(LEFT(contact_a.sort_name, 1)) asc"; + } + else { + $order = " ORDER BY contact_a.sort_name asc, contact_a.id"; + } + } + + // hack for order clause + if ($order) { + $fieldStr = trim(str_replace('ORDER BY', '', $order)); + $fieldOrder = explode(' ', $fieldStr); + $field = $fieldOrder[0]; + + if ($field) { + switch ($field) { + case 'city': + case 'postal_code': + $this->_whereTables["civicrm_address"] = 1; + $order = str_replace($field, "civicrm_address.{$field}", $order); + break; + + case 'country': + case 'state_province': + $this->_whereTables["civicrm_{$field}"] = 1; + $order = str_replace($field, "civicrm_{$field}.name", $order); + break; + + case 'email': + $this->_whereTables["civicrm_email"] = 1; + $order = str_replace($field, "civicrm_email.{$field}", $order); + break; + + default: + //CRM-12565 add "`" around $field if it is a pseudo constant + foreach ($this->_pseudoConstantsSelect as $key => $value) { + if (!empty($value['element']) && $value['element'] == $field) { + $order = str_replace($field, "`{$field}`", $order); + } + } + } + $this->_fromClause = self::fromClause($this->_tables, NULL, NULL, $this->_primaryLocation, $this->_mode); + $this->_simpleFromClause = self::fromClause($this->_whereTables, NULL, NULL, $this->_primaryLocation, $this->_mode); + } + } + + // The above code relies on crazy brittle string manipulation of a peculiarly-encoded ORDER BY + // clause. But this magic helper which forgivingly reescapes ORDER BY. + // Note: $sortByChar implies that $order was hard-coded/trusted, so it can do funky things. + if ($order && !$sortByChar) { + $order = ' ORDER BY ' . CRM_Utils_Type::escape(preg_replace('/^\s*ORDER BY\s*/', '', $order), 'MysqlOrderBy'); + return array($order, $additionalFromClause); + } + return array($order, $additionalFromClause); + } + } diff --git a/CRM/Core/Page/AJAX.php b/CRM/Core/Page/AJAX.php index 3116e26f51..e33dc666e6 100644 --- a/CRM/Core/Page/AJAX.php +++ b/CRM/Core/Page/AJAX.php @@ -219,7 +219,7 @@ class CRM_Core_Page_AJAX { $sortMapper = array(); foreach ($_GET['columns'] as $key => $value) { - $sortMapper[$key] = CRM_Utils_Type::validate($value['data'], 'MysqlColumnName'); + $sortMapper[$key] = CRM_Utils_Type::validate($value['data'], 'MysqlColumnNameOrAlias'); }; $offset = isset($_GET['start']) ? CRM_Utils_Type::validate($_GET['start'], 'Integer') : $defaultOffset; diff --git a/CRM/Utils/Rule.php b/CRM/Utils/Rule.php index 32cbd75919..aff62b8ac6 100644 --- a/CRM/Utils/Rule.php +++ b/CRM/Utils/Rule.php @@ -90,23 +90,22 @@ class CRM_Utils_Rule { } /** - * Validate an acceptable column name for sorting results. + * Validate that a string is a valid MySQL column name or alias. * * @param $str * * @return bool */ - public static function mysqlColumnName($str) { + public static function mysqlColumnNameOrAlias($str) { // Check not empty. if (empty($str)) { return FALSE; } - // Ensure it only contains valid characters (alphanumeric and underscores). - // - // MySQL permits column names that don't match this (eg containing spaces), - // but CiviCRM won't create those ... - if (!preg_match('/^\w{1,64}(\.\w{1,64})?$/i', $str)) { + // Ensure the string contains only valid characters: + // For column names: alphanumeric and underscores + // For aliases: backticks, alphanumeric hyphens and underscores. + if (!preg_match('/^((`[\w-]{1,64}`|[\w-]{1,64})\.)?(`[\w-]{1,64}`|[\w-]{1,64})$/i', $str)) { return FALSE; } @@ -140,7 +139,7 @@ class CRM_Utils_Rule { // at all, so we split and loop over. $parts = explode(',', $str); foreach ($parts as $part) { - if (!preg_match('/^((\w{1,64})((\.)(\w{1,64}))?( (asc|desc))?)$/i', trim($part))) { + if (!preg_match('/^((`[\w-]{1,64}`|[\w-]{1,64})\.)?(`[\w-]{1,64}`|[\w-]{1,64})( (asc|desc))?$/i', trim($part))) { return FALSE; } } diff --git a/CRM/Utils/Sort.php b/CRM/Utils/Sort.php index 69f0dc23be..d8dd68b1ea 100644 --- a/CRM/Utils/Sort.php +++ b/CRM/Utils/Sort.php @@ -129,7 +129,7 @@ class CRM_Utils_Sort { foreach ($vars as $weight => $value) { $this->_vars[$weight] = array( - 'name' => CRM_Utils_Type::validate($value['sort'], 'MysqlColumnName'), + 'name' => CRM_Utils_Type::validate($value['sort'], 'MysqlColumnNameOrAlias'), 'direction' => CRM_Utils_Array::value('direction', $value), 'title' => $value['name'], ); @@ -160,11 +160,11 @@ class CRM_Utils_Sort { $this->_vars[$this->_currentSortID]['direction'] == self::DONTCARE ) { $this->_vars[$this->_currentSortID]['name'] = str_replace(' ', '_', $this->_vars[$this->_currentSortID]['name']); - return $this->_vars[$this->_currentSortID]['name'] . ' asc'; + return CRM_Utils_Type::escape($this->_vars[$this->_currentSortID]['name'], 'MysqlColumnNameOrAlias') . ' asc'; } else { $this->_vars[$this->_currentSortID]['name'] = str_replace(' ', '_', $this->_vars[$this->_currentSortID]['name']); - return $this->_vars[$this->_currentSortID]['name'] . ' desc'; + return CRM_Utils_Type::escape($this->_vars[$this->_currentSortID]['name'], 'MysqlColumnNameOrAlias') . ' desc'; } } diff --git a/CRM/Utils/Type.php b/CRM/Utils/Type.php index db2aa35345..c1bd663f9c 100644 --- a/CRM/Utils/Type.php +++ b/CRM/Utils/Type.php @@ -258,8 +258,9 @@ class CRM_Utils_Type { } break; - case 'MysqlColumnName': - if (CRM_Utils_Rule::mysqlColumnName($data)) { + case 'MysqlColumnNameOrAlias': + if (CRM_Utils_Rule::mysqlColumnNameOrAlias($data)) { + $data = str_replace('`', '', $data); $parts = explode('.', $data); $data = '`' . implode('`.`', $parts) . '`'; @@ -277,7 +278,7 @@ class CRM_Utils_Type { 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)); + $part = preg_replace_callback('/^(?:(?:((?:`[\w-]{1,64}`|[\w-]{1,64}))(?:\.))?(`[\w-]{1,64}`|[\w-]{1,64})(?: (asc|desc))?)$/i', array('CRM_Utils_Type', 'mysqlOrderByCallback'), trim($part)); } return implode(', ', $parts); } @@ -386,8 +387,8 @@ class CRM_Utils_Type { } break; - case 'MysqlColumnName': - if (CRM_Utils_Rule::mysqlColumnName($data)) { + case 'MysqlColumnNameOrAlias': + if (CRM_Utils_Rule::mysqlColumnNameOrAlias($data)) { return $data; } break; @@ -422,13 +423,14 @@ class CRM_Utils_Type { */ public static function mysqlOrderByCallback($matches) { $output = ''; - // Column or table name. - if (isset($matches[1])) { - $output .= '`' . $matches[1] . '`'; + $matches = str_replace('`', '', $matches); + // Table name. + if (isset($matches[1]) && $matches[1]) { + $output .= '`' . $matches[1] . '`.'; } - // Column name in case there is a table. + // Column name. if (isset($matches[2]) && $matches[2]) { - $output .= '.`' . $matches[2] . '`'; + $output .= '`' . $matches[2] . '`'; } // Sort order. if (isset($matches[3]) && $matches[3]) { diff --git a/tests/phpunit/CRM/Contact/BAO/QueryTest.php b/tests/phpunit/CRM/Contact/BAO/QueryTest.php index f9ff18a887..c183636153 100644 --- a/tests/phpunit/CRM/Contact/BAO/QueryTest.php +++ b/tests/phpunit/CRM/Contact/BAO/QueryTest.php @@ -176,7 +176,7 @@ class CRM_Contact_BAO_QueryTest extends CiviUnitTestCase { 'contact_sub_type' => 1, 'sort_name' => 1, ); - $expectedSQL = "SELECT contact_a.id as contact_id, contact_a.contact_type as `contact_type`, contact_a.contact_sub_type as `contact_sub_type`, contact_a.sort_name as `sort_name`, civicrm_address.id as address_id, civicrm_address.city as `city` FROM civicrm_contact contact_a LEFT JOIN civicrm_address ON ( contact_a.id = civicrm_address.contact_id AND civicrm_address.is_primary = 1 ) WHERE ( ( LOWER(civicrm_address.city) = 'cool city' ) ) AND (contact_a.is_deleted = 0) ORDER BY contact_a.sort_name asc, contact_a.id "; + $expectedSQL = "SELECT contact_a.id as contact_id, contact_a.contact_type as `contact_type`, contact_a.contact_sub_type as `contact_sub_type`, contact_a.sort_name as `sort_name`, civicrm_address.id as address_id, civicrm_address.city as `city` FROM civicrm_contact contact_a LEFT JOIN civicrm_address ON ( contact_a.id = civicrm_address.contact_id AND civicrm_address.is_primary = 1 ) WHERE ( ( LOWER(civicrm_address.city) = 'cool city' ) ) AND (contact_a.is_deleted = 0) ORDER BY `contact_a`.`sort_name` asc, `contact_a`.`id` "; $queryObj = new CRM_Contact_BAO_Query($params, $returnProperties); try { $this->assertEquals($expectedSQL, $queryObj->searchQuery(0, 0, NULL, diff --git a/tests/phpunit/CRM/Utils/TypeTest.php b/tests/phpunit/CRM/Utils/TypeTest.php index 9c46b671b2..8a05e38685 100644 --- a/tests/phpunit/CRM/Utils/TypeTest.php +++ b/tests/phpunit/CRM/Utils/TypeTest.php @@ -38,21 +38,27 @@ 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(str_repeat('a', 64), 'MysqlColumnName', str_repeat('a', 64)), - array(str_repeat('a', 65), 'MysqlColumnName', NULL), - array(str_repeat('a', 64) . '.' . str_repeat('a', 64), 'MysqlColumnName', str_repeat('a', 64) . '.' . str_repeat('a', 64)), - array(str_repeat('a', 64) . '.' . str_repeat('a', 65), 'MysqlColumnName', NULL), - array(str_repeat('a', 65) . '.' . str_repeat('a', 64), 'MysqlColumnName', NULL), + array('civicrm_column_name', 'MysqlColumnNameOrAlias', 'civicrm_column_name'), + array('table.civicrm_column_name', 'MysqlColumnNameOrAlias', 'table.civicrm_column_name'), + array('table.civicrm_column_name.toomanydots', 'MysqlColumnNameOrAlias', NULL), + array('Home-street_address', 'MysqlColumnNameOrAlias', 'Home-street_address'), + array('`Home-street_address`', 'MysqlColumnNameOrAlias', '`Home-street_address`'), + array('`Home-street_address', 'MysqlColumnNameOrAlias', NULL), + array('table.`Home-street_address`', 'MysqlColumnNameOrAlias', 'table.`Home-street_address`'), + array('`table-alias`.`Home-street_address`', 'MysqlColumnNameOrAlias', '`table-alias`.`Home-street_address`'), + array('`table-alias`.column', 'MysqlColumnNameOrAlias', '`table-alias`.column'), + array('column_name, sleep(5)', 'MysqlColumnNameOrAlias', NULL), + array(str_repeat('a', 64), 'MysqlColumnNameOrAlias', str_repeat('a', 64)), + array(str_repeat('a', 65), 'MysqlColumnNameOrAlias', NULL), + array(str_repeat('a', 64) . '.' . str_repeat('a', 64), 'MysqlColumnNameOrAlias', str_repeat('a', 64) . '.' . str_repeat('a', 64)), + array(str_repeat('a', 64) . '.' . str_repeat('a', 65), 'MysqlColumnNameOrAlias', NULL), + array(str_repeat('a', 65) . '.' . str_repeat('a', 64), 'MysqlColumnNameOrAlias', 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'), + array('table.`Home-street_address` asc, `table-alias`.`Home-street_address` desc,`table-alias`.column', 'MysqlOrderBy', 'table.`Home-street_address` asc, `table-alias`.`Home-street_address` desc,`table-alias`.column'), ); } @@ -91,16 +97,19 @@ class CRM_Utils_TypeTest extends CiviUnitTestCase { array('-3', 'ContactReference', NULL), // Escape function is meant for sql, not xss array('

Hello

', 'Memo', '

Hello

'), - 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('civicrm_column_name', 'MysqlColumnNameOrAlias', '`civicrm_column_name`'), + array('table.civicrm_column_name', 'MysqlColumnNameOrAlias', '`table`.`civicrm_column_name`'), + array('table.civicrm_column_name.toomanydots', 'MysqlColumnNameOrAlias', NULL), + array('Home-street_address', 'MysqlColumnNameOrAlias', '`Home-street_address`'), + array('`Home-street_address`', 'MysqlColumnNameOrAlias', '`Home-street_address`'), + array('`Home-street_address', 'MysqlColumnNameOrAlias', NULL), + array('column_name, sleep(5)', 'MysqlColumnNameOrAlias', 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'), + array('table.`Home-street_address` asc, `table-alias`.`Home-street_address` desc,`table-alias`.column', 'MysqlOrderBy', '`table`.`Home-street_address` asc, `table-alias`.`Home-street_address` desc, `table-alias`.`column`'), ); } -- 2.25.1