From fee586c49037aef63b34b75f3b88b653746c27ea Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 14 Feb 2018 19:27:43 +1300 Subject: [PATCH] CRM-21753 - addition of json validation Per https://github.com/civicrm/civicrm-core/pull/11658 the dedupe code has a mechanism to handle a nuanced array of criteria. This PR is purely to support the passing of that json through the url criteria with a proposed validation rule. The criteria in the url might look like criteria={"contact" : {"first_name" :{"IN" : ["Peter", "Paul", "Mary"]]]} This is passed to the contact api to limit the contacts to look for matches for (before actually finding the matches). In discussion the biggest risk seemed to be the possibility of chaining so I have added a filter on the criteria & set check_permissions (acls are applied in the next step but having it on the api call is clearer & may reduce matches for the next step). --- CRM/Contact/Page/DedupeFind.php | 7 +++-- CRM/Core/BAO/PrevNextCache.php | 11 +++++++- CRM/Utils/Rule.php | 39 ++++++++++++++++++++++++++++ CRM/Utils/Type.php | 7 +++++ tests/phpunit/CRM/Utils/TypeTest.php | 2 ++ 5 files changed, 63 insertions(+), 3 deletions(-) diff --git a/CRM/Contact/Page/DedupeFind.php b/CRM/Contact/Page/DedupeFind.php index 701dd8e13540..3eacb5f91d11 100644 --- a/CRM/Contact/Page/DedupeFind.php +++ b/CRM/Contact/Page/DedupeFind.php @@ -63,8 +63,9 @@ public function run() { $limit = CRM_Utils_Request::retrieve('limit', 'Integer', $this); $rgid = CRM_Utils_Request::retrieve('rgid', 'Positive', $this); $cid = CRM_Utils_Request::retrieve('cid', 'Positive', $this, FALSE, 0); - // Using a placeholder for criteria as it is intended to be able to pass this later. - $criteria = array(); + $criteria = CRM_Utils_Request::retrieve('criteria', 'Json', $this, FALSE); + $this->assign('criteria', $criteria); + $isConflictMode = ($context == 'conflicts'); if ($cid) { $this->_cid = $cid; @@ -79,8 +80,10 @@ public function run() { 'rgid' => $rgid, 'gid' => $gid, 'limit' => $limit, + 'criteria' => $criteria, ); $this->assign('urlQuery', CRM_Utils_System::makeQueryString($urlQry)); + $criteria = json_decode($criteria, TRUE); if ($context == 'search') { $context = 'search'; diff --git a/CRM/Core/BAO/PrevNextCache.php b/CRM/Core/BAO/PrevNextCache.php index 0112a921b8e7..157783dddd2e 100644 --- a/CRM/Core/BAO/PrevNextCache.php +++ b/CRM/Core/BAO/PrevNextCache.php @@ -398,8 +398,17 @@ public static function refillCache($rgid, $gid, $cacheKeyString, $criteria, $che } elseif ($rgid) { $contactIDs = array(); + // The thing we really need to filter out is any chaining that would 'DO SOMETHING' to the DB. + // criteria could be passed in via url so we want to ensure nothing could be in that url that + // would chain to a delete. Limiting to getfields for 'get' limits us to declared fields, + // although we might wish to revisit later to allow joins. + $validFieldsForRetrieval = civicrm_api3('Contact', 'getfields', ['action' => 'get'])['values']; if (!empty($criteria)) { - $contacts = civicrm_api3('Contact', 'get', array_merge(array('options' => array('limit' => 0), 'return' => 'id'), $criteria['contact'])); + $contacts = civicrm_api3('Contact', 'get', array_merge([ + 'options' => ['limit' => 0], + 'return' => 'id', + 'check_permissions' => TRUE, + ], array_intersect_key($criteria['contact'], $validFieldsForRetrieval))); $contactIDs = array_keys($contacts['values']); } $foundDupes = CRM_Dedupe_Finder::dupes($rgid, $contactIDs, $checkPermissions, $searchLimit); diff --git a/CRM/Utils/Rule.php b/CRM/Utils/Rule.php index ada29634f103..ef4587c59b5f 100644 --- a/CRM/Utils/Rule.php +++ b/CRM/Utils/Rule.php @@ -813,6 +813,25 @@ public static function xssString($value) { } } + /** + * Validate json string for xss + * + * @param string $value + * + * @return bool + * False if invalid, true if valid / safe. + */ + public static function json($value) { + if (!self::xssString($value)) { + return FALSE; + } + $array = json_decode($value, TRUE); + if (!$array || !is_array($array)) { + return FALSE; + } + return self::arrayValue($array); + } + /** * @param $path * @@ -942,4 +961,24 @@ public static function checkExtesnionKeyIsValid($key = NULL) { return TRUE; } + /** + * Validate array recursively checking keys and values. + * + * @param array $array + * @return bool + */ + protected static function arrayValue($array) { + foreach ($array as $key => $item) { + if (is_array($item)) { + if (!self::xssString($key) || !self::arrayValue($item)) { + return FALSE; + } + } + if (!self::xssString($key) || !self::xssString($item)) { + return FALSE; + } + } + return TRUE; + } + } diff --git a/CRM/Utils/Type.php b/CRM/Utils/Type.php index d96a0c4a1309..75bc081b5698 100644 --- a/CRM/Utils/Type.php +++ b/CRM/Utils/Type.php @@ -422,6 +422,7 @@ public static function validate($data, $type, $abort = TRUE, $name = 'One of par 'MysqlOrderByDirection', 'MysqlOrderBy', 'ExtensionKey', + 'Json', ); if (!in_array($type, $possibleTypes)) { if ($isThrowException) { @@ -530,6 +531,12 @@ public static function validate($data, $type, $abort = TRUE, $name = 'One of par return $data; } break; + + case 'Json': + if (CRM_Utils_Rule::json($data)) { + return $data; + } + break; } if ($abort) { diff --git a/tests/phpunit/CRM/Utils/TypeTest.php b/tests/phpunit/CRM/Utils/TypeTest.php index ad9ff03738fe..a9b1bb36bf41 100644 --- a/tests/phpunit/CRM/Utils/TypeTest.php +++ b/tests/phpunit/CRM/Utils/TypeTest.php @@ -74,6 +74,8 @@ public function validateDataProvider() { 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'), array('a string', 'String', 'a string'), + array('{"contact":{"contact_id":205}}', 'Json', '{"contact":{"contact_id":205}}'), + array('{"contact":{"contact_id":!n†rude®}}', 'Json', NULL), ); }