security/core#49 Ensure that only intergers are passed to the IN build options in...

Fix Rule checking and add a unit test

Add in unit test on building country_id options too

Add in a unit test for building county options with a state_province_id filter

diff --git a/CRM/Core/BAO/Address.php b/CRM/Core/BAO/Address.php
index b5ec97d74863f4ae97c01a1364d571eddc120270..d94ecf2800dd5f65a255db3d9b89b67ab95744fe 100644 (file)
--- a/CRM/Core/BAO/Address.php
+++ b/CRM/Core/BAO/Address.php
@@ -1319,6 +1319,9 @@ SELECT is_primary,
           }
         }
         if (!empty($props['country_id'])) {
+          if (!CRM_Utils_Rule::commaSeparatedIntegers(implode(',', (array) $props['country_id']))) {
+            throw new CRM_Core_Exception(ts('Province limit or default country setting is incorrect'));
+          }
           $params['condition'] = 'country_id IN (' . implode(',', (array) $props['country_id']) . ')';
         }
         break;
@@ -1331,6 +1334,9 @@ SELECT is_primary,
         if ($context != 'get' && $context != 'validate') {
           $config = CRM_Core_Config::singleton();
           if (!empty($config->countryLimit) && is_array($config->countryLimit)) {
+            if (!CRM_Utils_Rule::commaSeparatedIntegers(implode(',', $config->countryLimit))) {
+              throw new CRM_Core_Exception(ts('Available Country setting is incorrect'));
+            }
             $params['condition'] = 'id IN (' . implode(',', $config->countryLimit) . ')';
           }
         }
@@ -1339,6 +1345,9 @@ SELECT is_primary,
       // Filter county list based on chosen state
       case 'county_id':
         if (!empty($props['state_province_id'])) {
+          if (!CRM_Utils_Rule::commaSeparatedIntegers(implode(',', (array) $props['state_province_id']))) {
+            throw new CRM_Core_Exception(ts('Can only accept Integers for state_province_id filtering'));
+          }
           $params['condition'] = 'state_province_id IN (' . implode(',', (array) $props['state_province_id']) . ')';
         }
         break;

diff --git a/CRM/Utils/Rule.php b/CRM/Utils/Rule.php
index 50d72d38352e74e4d484241548f7464638a20c15..940128d7156c9487cd53d365c2b60faa2cc62a36 100644 (file)
--- a/CRM/Utils/Rule.php
+++ b/CRM/Utils/Rule.php
@@ -488,6 +488,8 @@ class CRM_Utils_Rule {
    */
   public static function commaSeparatedIntegers($value) {
     foreach (explode(',', $value) as $val) {
+      // Remove any Whitespace around the key.
+      $val = trim($val);
       if (!self::positiveInteger($val)) {
         return FALSE;
       }

diff --git a/tests/phpunit/api/v3/AddressTest.php b/tests/phpunit/api/v3/AddressTest.php
index bbbc21cf9a7d2cac2018d3bc7ddd524e65ad888c..308b012d60e9fb2bba3ecd8b391fa2dee76d4b29 100644 (file)
--- a/tests/phpunit/api/v3/AddressTest.php
+++ b/tests/phpunit/api/v3/AddressTest.php
@@ -479,4 +479,47 @@ class api_v3_AddressTest extends CiviUnitTestCase {
     $this->assertEquals($expectState, $created['state_province_id']);
   }
 
+  public function testBuildStateProvinceOptionsWithDodgyProvinceLimit() {
+    $provinceLimit = [1228, "abcd;ef"];
+    $this->callAPISuccess('setting', 'create', [
+     'provinceLimit' => $provinceLimit,
+    ]);
+    $result = $this->callAPIFailure('address', 'getoptions', ['field' => 'state_province_id']);
+    // confirm that we hit our error not a SQLI.
+    $this->assertEquals('Province limit or default country setting is incorrect', $result['error_message']);
+    $this->callAPISuccess('setting', 'create', [
+     'provinceLimit' => [1228],
+    ]);
+    // Now confirm with a correct province setting it works fine
+    $this->callAPISuccess('address', 'getoptions', ['field' => 'state_province_id']);
+  }
+
+  public function testBuildCountryWithDodgyCountryLimitSetting() {
+    $countryLimit = [1228, "abcd;ef"];
+    $this->callAPISuccess('setting', 'create', [
+     'countryLimit' => $countryLimit,
+    ]);
+    $result = $this->callAPIFailure('address', 'getoptions', ['field' => 'country_id']);
+    // confirm that we hit our error not a SQLI.
+    $this->assertEquals('Available Country setting is incorrect', $result['error_message']);
+    $this->callAPISuccess('setting', 'create', [
+     'countryLimit' => [1228],
+    ]);
+    // Now confirm with a correct province setting it works fine
+    $this->callAPISuccess('address', 'getoptions', ['field' => 'country_id']);
+  }
+
+  public function testBuildCountyWithDodgeStateProvinceFiltering() {
+    $result = $this->callAPIFailure('Address', 'getoptions', [
+      'field' => 'county_id',
+      'state_province_id' => "abcd;ef",
+    ]);
+    $this->assertEquals('Can only accept Integers for state_province_id filtering', $result['error_message']);
+    $goodResult = $this->callAPISuccess('Address', 'getoptions', [
+      'field' => 'county_id',
+      'state_province_id' => 1004,
+    ]);
+    $this->assertEquals('San Francisco', $goodResult['values'][4]);
+  }
+
 }