Fix bug where a % in a serialized array can lead to the data being broken

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

diff --git a/CRM/Dedupe/Finder.php b/CRM/Dedupe/Finder.php
index dac4bf1359fbf63b00fec751f54248eebb3bd0ed..06863a2afabd7b7e5420fe78825e70db07d7304e 100644 (file)
--- a/CRM/Dedupe/Finder.php
+++ b/CRM/Dedupe/Finder.php
@@ -348,12 +348,12 @@ class CRM_Dedupe_Finder {
         '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'],
         ]
       );
     }

diff --git a/CRM/Dedupe/Merger.php b/CRM/Dedupe/Merger.php
index 00f2901fa5c9e22d93c25b8f3d158cb16c66a4a6..6c1d7d6bfd21d9e02fa7e29a19a0cdce2b3d8afc 100644 (file)
--- a/CRM/Dedupe/Merger.php
+++ b/CRM/Dedupe/Merger.php
@@ -775,10 +775,9 @@ INNER JOIN  civicrm_membership membership2 ON membership1.membership_type_id = m
       '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']]);
   }
 
   /**

diff --git a/tests/phpunit/api/v3/JobTest.php b/tests/phpunit/api/v3/JobTest.php
index 57b88cfa4aeb7b6f167ca9a64bb4d92ad6ee7b0f..869bf470e2b10c1dc45ae60ee93247898b12562c 100644 (file)
--- a/tests/phpunit/api/v3/JobTest.php
+++ b/tests/phpunit/api/v3/JobTest.php
@@ -828,6 +828,22 @@ class api_v3_JobTest extends CiviUnitTestCase {
 
   }
 
+  /**
+   * 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.
    *