From 882514391de1955e7bbc89611f28e95424979715 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 701dd8e135..3eacb5f91d 100644 --- a/CRM/Contact/Page/DedupeFind.php +++ b/CRM/Contact/Page/DedupeFind.php @@ -63,8 +63,9 @@ class CRM_Contact_Page_DedupeFind extends CRM_Core_Page_Basic { $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 @@ class CRM_Contact_Page_DedupeFind extends CRM_Core_Page_Basic { '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 0112a921b8..157783dddd 100644 --- a/CRM/Core/BAO/PrevNextCache.php +++ b/CRM/Core/BAO/PrevNextCache.php @@ -398,8 +398,17 @@ WHERE (pn.cacheKey $op %1 OR pn.cacheKey $op %2) } 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 ada29634f1..ef4587c59b 100644 --- a/CRM/Utils/Rule.php +++ b/CRM/Utils/Rule.php @@ -813,6 +813,25 @@ class CRM_Utils_Rule { } } + /** + * 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 @@ class CRM_Utils_Rule { 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 d96a0c4a13..75bc081b56 100644 --- a/CRM/Utils/Type.php +++ b/CRM/Utils/Type.php @@ -422,6 +422,7 @@ class CRM_Utils_Type { 'MysqlOrderByDirection', 'MysqlOrderBy', 'ExtensionKey', + 'Json', ); if (!in_array($type, $possibleTypes)) { if ($isThrowException) { @@ -530,6 +531,12 @@ class CRM_Utils_Type { 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 ad9ff03738..a9b1bb36bf 100644 --- a/tests/phpunit/CRM/Utils/TypeTest.php +++ b/tests/phpunit/CRM/Utils/TypeTest.php @@ -74,6 +74,8 @@ class CRM_Utils_TypeTest extends CiviUnitTestCase { 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), ); } -- 2.25.1