Skip to content

Commit

Permalink
Fix bug where a % in a serialized array can lead to the data being br…
Browse files Browse the repository at this point in the history
…oken

It turns out that is a field in a serialized array has a %2 (for example) this gets swapped in executeQuery for the
%2 value (in this case srcID - rendering the serialized array invalid. This proposes that we
explicitly handle arrays as  a data type in compose query.

Some thoughts
1) we could make serialized arrays valid types in validate (not done here)
2) we could iterate through the  array keys  & values escaping them -
at this stage it's left in the calling function
3) there are whole bikeshed factories to keep in business on discussion of whether
'Array-1', 'Array-2' etc are the right format
  • Loading branch information
eileenmcnaughton committed Mar 11, 2020
1 parent ae90f9a commit 3cb0d57
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 5 deletions.
14 changes: 13 additions & 1 deletion CRM/Core/DAO.php
Original file line number Diff line number Diff line change
Expand Up @@ -1505,12 +1505,24 @@ public static function &singleValueQuery(
*
* @return string
* @throws CRM_Core_Exception
* @throws \Exception
*/
public static function composeQuery($query, $params = [], $abort = TRUE) {
$tr = [];
foreach ($params as $key => $item) {
if (is_numeric($key)) {
if (CRM_Utils_Type::validate($item[0], $item[1]) !== NULL) {
if (in_array($item[1], [
'Array-' . self::SERIALIZE_COMMA,
'Array-' . self::SERIALIZE_PHP,
'Array-' . self::SERIALIZE_SEPARATOR_BOOKEND,
'Array-' . self::SERIALIZE_JSON,
'Array-' . self::SERIALIZE_SEPARATOR_TRIMMED,
], TRUE)) {
$serializeType = substr($item[1], -1);
$serializedArray = CRM_Core_DAO::serializeField($item[0], $serializeType);
$tr['%' . $key] = "'{$serializedArray}'";
}
elseif (CRM_Utils_Type::validate($item[0], $item[1]) !== NULL) {
$item[0] = self::escapeString($item[0]);
if ($item[1] == 'String' ||
$item[1] == 'Memo' ||
Expand Down
8 changes: 4 additions & 4 deletions CRM/Dedupe/Finder.php
Original file line number Diff line number Diff line change
Expand Up @@ -341,19 +341,19 @@ public static function parseAndStoreDupePairs($foundDupes, $cacheKeyString) {

$mainContacts[] = $row = [
'dstID' => (int) $dstID,
'dstName' => $displayNames[$dstID],
'dstName' => CRM_Core_DAO::escapeString($displayNames[$dstID]),
'srcID' => (int) $srcID,
'srcName' => $displayNames[$srcID],
'srcName' => CRM_Core_DAO::escapeString($displayNames[$srcID]),
'weight' => $dupes[2],
'canMerge' => TRUE,
];

$data = CRM_Core_DAO::escapeString(serialize($row));
CRM_Core_DAO::executeQuery("INSERT INTO civicrm_prevnext_cache (entity_table, entity_id1, entity_id2, cacheKey, data) VALUES
('civicrm_contact', %1, %2, %3, '{$data}')", [
('civicrm_contact', %1, %2, %3, %4)", [
1 => [$dstID, 'Integer'],
2 => [$srcID, 'Integer'],
3 => [$cacheKeyString, 'String'],
4 => [$row, 'Array-' . CRM_Core_DAO::SERIALIZE_PHP],
]
);
}
Expand Down
17 changes: 17 additions & 0 deletions tests/phpunit/api/v3/JobTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -828,6 +828,23 @@ public function getMergeLocationData() {

}

/**
* Test weird characters don't mess with merge & cause a fatal.
*
* @throws \CRM_Core_Exception
*/
public function testNoErrorOnOdd() {
$this->individualCreate();
$this->individualCreate(['first_name' => 'Gerrit%0a%2e%0a']);
$this->callAPISuccess('Job', 'process_batch_merge', []);
// As serialized
//a:6:{s:5:"dstID";s:1:"4";s:7:"dstName";s:23:"Mr. Anthony Anderson II";s:5:"srcID";s:1:"5";s:7:"srcName";s:31:"Mr. Gerrit%0a4e%0a Anderson II";s:6:"weight";s:2:"10";s:8:"canMerge";b:1;}
// As stored to db
//a:6:{s:5:"dstID";s:1:"4";s:7:"dstName";s:23:"Mr. Anthony Anderson II";s:5:"srcID";s:1:"5";s:7:"srcName";s:31:"Mr. Gerrit%0a4e%0a Anderson II";s:6:"weight";s:2:"10";s:8:"canMerge";b:1;}
// as retrieved
//a:6:{s:5:"dstID";s:1:"4";s:7:"dstName";s:23:"Mr. Anthony Anderson II";s:5:"srcID";s:1:"5";s:7:"srcName";s:31:"Mr. Gerrit%0a%2e%0a Anderson II";s:6:"weight";s:2:"10";s:8:"canMerge";b:1;}
}

/**
* Test the batch merge does not create duplicate emails.
*
Expand Down

0 comments on commit 3cb0d57

Please sign in to comment.