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 c879543 commit 2e09a60
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 4 deletions.
4 changes: 2 additions & 2 deletions CRM/Dedupe/Finder.php
Original file line number Diff line number Diff line change
Expand Up @@ -348,12 +348,12 @@ public static function parseAndStoreDupePairs($foundDupes, $cacheKeyString) {
'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 => [serialize($row), 'String'],
]
);
}
Expand Down
3 changes: 1 addition & 2 deletions CRM/Dedupe/Merger.php
Original file line number Diff line number Diff line change
Expand Up @@ -775,10 +775,9 @@ public static function updateMergeStats($cacheKeyString, $result = []) {
'merged' => (int) $merged,
'skipped' => (int) $skipped,
];
$data = serialize($data);

CRM_Core_DAO::executeQuery("INSERT INTO civicrm_prevnext_cache (entity_table, entity_id1, entity_id2, cacheKey, data) VALUES
('civicrm_contact', 0, 0, %1, '{$data}')", [1 => [$cacheKeyString . '_stats', 'String']]);
('civicrm_contact', 0, 0, %1, %2)", [1 => [$cacheKeyString . '_stats', 'String'], 2 => [serialize($data), 'String']]);
}

/**
Expand Down
16 changes: 16 additions & 0 deletions tests/phpunit/api/v3/JobTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -828,6 +828,22 @@ 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', []);

$this->individualCreate();
$this->individualCreate(['first_name' => '[foo\\bar\'baz']);
$this->callAPISuccess('Job', 'process_batch_merge', []);
$this->callAPISuccessGetSingle('Contact', ['first_name' => '[foo\\bar\'baz']);
}

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

0 comments on commit 2e09a60

Please sign in to comment.