Skip to content

Commit

Permalink
CRM-21753 - addition of json validation
Browse files Browse the repository at this point in the history
Per #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).
  • Loading branch information
eileenmcnaughton committed Mar 27, 2018
1 parent 671e1ad commit 8825143
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 3 deletions.
7 changes: 5 additions & 2 deletions CRM/Contact/Page/DedupeFind.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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';
Expand Down
11 changes: 10 additions & 1 deletion CRM/Core/BAO/PrevNextCache.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
39 changes: 39 additions & 0 deletions CRM/Utils/Rule.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down Expand Up @@ -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;
}

}
7 changes: 7 additions & 0 deletions CRM/Utils/Type.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 2 additions & 0 deletions tests/phpunit/CRM/Utils/TypeTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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),
);
}

Expand Down

0 comments on commit 8825143

Please sign in to comment.