From 0bb9a2e57945efdfeb3b09a6caadf733d21236e6 Mon Sep 17 00:00:00 2001 From: Rich Date: Tue, 31 May 2022 22:58:34 +0000 Subject: [PATCH] Fix issue #116 api3 chain check_permissions --- Civi/API/Subscriber/ChainSubscriber.php | 44 ++++++----- tests/phpunit/CRM/Utils/RestTest.php | 98 ++++++++++++++++++++++++- 2 files changed, 123 insertions(+), 19 deletions(-) diff --git a/Civi/API/Subscriber/ChainSubscriber.php b/Civi/API/Subscriber/ChainSubscriber.php index e76220ca0d..dfbfa458e8 100644 --- a/Civi/API/Subscriber/ChainSubscriber.php +++ b/Civi/API/Subscriber/ChainSubscriber.php @@ -89,8 +89,11 @@ class ChainSubscriber implements EventSubscriberInterface { $oldResult = $result; $result = ['values' => [0 => $oldResult]]; } + + // Scan the params for chain calls. foreach ($params as $field => $newparams) { if ((is_array($newparams) || $newparams === 1) && $field <> 'api.has_parent' && substr($field, 0, 3) == 'api') { + // This param is a chain call, e.g. api.. // 'api.participant.delete' => 1 is a valid options - handle 1 // instead of an array @@ -105,9 +108,13 @@ class ChainSubscriber implements EventSubscriberInterface { $subAPI = explode($separator, $field); $subaction = empty($subAPI[2]) ? $action : $subAPI[2]; - $subParams = [ + /** @var array of parameters that will be applied to every chained request. */ + $enforcedSubParams = [ 'debug' => $params['debug'] ?? NULL, ]; + /** @var array of parameters that provide defaults to every chained request, but which may be overridden by parameters in the chained request. */ + $defaultSubParams = []; + $subEntity = _civicrm_api_get_entity_name_from_camel($subAPI[1]); // Hard coded list of entitys that have fields starting api_ and shouldn't be automatically @@ -131,8 +138,8 @@ class ChainSubscriber implements EventSubscriberInterface { //from the parent call. in this case 'contact_id' will also be //set to the parent's id if (!($subEntity == 'line_item' && $lowercase_entity == 'contribution' && $action != 'create')) { - $subParams["entity_id"] = $parentAPIValues['id']; - $subParams['entity_table'] = 'civicrm_' . $lowercase_entity; + $defaultSubParams["entity_id"] = $parentAPIValues['id']; + $defaultSubParams['entity_table'] = 'civicrm_' . $lowercase_entity; } $addEntityId = TRUE; @@ -150,38 +157,39 @@ class ChainSubscriber implements EventSubscriberInterface { } } if ($addEntityId) { - $subParams[$lowercase_entity . "_id"] = $parentAPIValues['id']; + $defaultSubParams[$lowercase_entity . "_id"] = $parentAPIValues['id']; } } + // @todo remove strtolower: $subEntity is already lower case if ($entity != 'Contact' && \CRM_Utils_Array::value(strtolower($subEntity . "_id"), $parentAPIValues)) { //e.g. if event_id is in the values returned & subentity is event //then pass in event_id as 'id' don't do this for contact as it //does some weird things like returning primary email & //thus limiting the ability to chain email //TODO - this might need the camel treatment - $subParams['id'] = $parentAPIValues[$subEntity . "_id"]; + $defaultSubParams['id'] = $parentAPIValues[$subEntity . "_id"]; } if (\CRM_Utils_Array::value('entity_table', $result['values'][$idIndex]) == $subEntity) { - $subParams['id'] = $result['values'][$idIndex]['entity_id']; + $defaultSubParams['id'] = $result['values'][$idIndex]['entity_id']; } // if we are dealing with the same entity pass 'id' through // (useful for get + delete for example) if ($lowercase_entity == $subEntity) { - $subParams['id'] = $result['values'][$idIndex]['id']; + $defaultSubParams['id'] = $result['values'][$idIndex]['id']; } - $subParams['version'] = $version; - if (!empty($params['check_permissions'])) { - $subParams['check_permissions'] = $params['check_permissions']; - } - $subParams['sequential'] = 1; - $subParams['api.has_parent'] = 1; + $enforcedSubParams['version'] = $version; + // Copy check_permissions from parent. + $enforcedSubParams['check_permissions'] = $params['check_permissions'] ?? NULL; + $enforcedSubParams['sequential'] = 1; + $enforcedSubParams['api.has_parent'] = 1; + // Inspect $newparams, the passed in params for the chain call. if (array_key_exists(0, $newparams)) { - $genericParams = $subParams; - // it is a numerically indexed array - ie. multiple creates + // It is a numerically indexed array - ie. multiple creates foreach ($newparams as $entityparams) { - $subParams = array_merge($genericParams, $entityparams); + // Defaults, overridden by request params, overridden by enforced params. + $subParams = array_merge($defaultSubParams, $entityparams, $enforcedSubParams); _civicrm_api_replace_variables($subParams, $result['values'][$idIndex], $separator); $result['values'][$idIndex][$field][] = $apiKernel->runSafe($subEntity, $subaction, $subParams); if ($result['is_error'] === 1) { @@ -190,8 +198,8 @@ class ChainSubscriber implements EventSubscriberInterface { } } else { - - $subParams = array_merge($subParams, $newparams); + // Defaults, overridden by request params, overridden by enforced params. + $subParams = array_merge($defaultSubParams, $newparams, $enforcedSubParams); _civicrm_api_replace_variables($subParams, $result['values'][$idIndex], $separator); $result['values'][$idIndex][$field] = $apiKernel->runSafe($subEntity, $subaction, $subParams); if (!empty($result['is_error'])) { diff --git a/tests/phpunit/CRM/Utils/RestTest.php b/tests/phpunit/CRM/Utils/RestTest.php index b2e5aab5e4..4fa1aa3148 100644 --- a/tests/phpunit/CRM/Utils/RestTest.php +++ b/tests/phpunit/CRM/Utils/RestTest.php @@ -4,7 +4,12 @@ * Class CRM_Utils_RestTest * @group headless */ -class CRM_Utils_RestTest extends CiviUnitTestCase { +class CRM_Utils_RestTest extends CiviUnitTestCase { + + public function setUp() :void { + parent::setUp(); + $this->useTransaction(TRUE); + } public function testProcessMultiple() { $_SERVER['REQUEST_METHOD'] = 'POST'; @@ -32,4 +37,95 @@ class CRM_Utils_RestTest extends CiviUnitTestCase { $this->assertGreaterThan($output['cow']['id'], $output['sheep']['id']); } + /** + * Check that check_permissions passed in in chained api calls is ignored. + */ + public function testSecurityIssue116() { + $this->hookClass->setHook('civicrm_alterAPIPermissions', [$this, 'alterAPIPermissions']); + + $config = CRM_Core_Config::singleton(); + $config->userPermissionClass->permissions = []; + + $contactID = \Civi\Api4\Contact::create(FALSE) + ->addValue('display_name', 'Wilma') + ->addValue('contact_type', 'Individual') + ->execute()->first()['id']; + + $jobLogID = civicrm_api3('JobLog', 'create', [ + 'name' => 'test', + 'domain_id' => 1, + ])['id']; + $params = [ 'id' => $jobLogID, 'version' => 3, 'sequential' => 1, 'check_permissions' => 0 ]; + $args = ['civicrm', 'JobLog', 'get']; + + // Check we can load the email without checking perms. + $r = civicrm_api('JobLog', 'get', $params); + $this->assertEquals(1, $r['count']); + + // Check we can still load it with checking permission (because we allow it in hook) + $r = civicrm_api('JobLog', 'get', ['check_permissions' => 1] + $params); + $this->assertEquals(1, $r['count']); + + // Now check we can load it via the rest endpoint which should enforce permissions. + $output = CRM_Utils_REST::process($args, $params); + $this->assertEquals(1, $output['count']); + + // Now add a chain, naughtily passing in a check_permissions + // We do not have permission to access this contact. + $params['api.contact.get'] = [ + 'id' => $contactID, + 'check_permissions' => 0, + 'return' => 'display_name', + ]; + $output = CRM_Utils_REST::process($args, $params); + $this->assertEquals($jobLogID, $output['id']); + $chain = $output['values'][0]['api.contact.get']; + $this->assertEquals(0, $chain['count'], "Vulnerable."); + + // There is a different codepath when the chained api call is an array + // (This is designed for multiple chained create/delete calls, but + // we can just use get for testing.) + $params['api.contact.get'] = [$params['api.contact.get']]; + $output = CRM_Utils_REST::process($args, $params); + $this->assertEquals($jobLogID, $output['id']); + $chainResult = $output['values'][0]['api.contact.get']; + $this->assertIsArray($chainResult); + $this->assertCount(1, $chainResult); + $this->assertEquals(0, $chainResult[0]['count'], "Vulnerable."); + + // Try create call AND using different api chain syntax. + unset($params['api.contact.get']); + $params['api_contact_create'] = [ + ['contact_type' => 'Individual', 'display_name' => 'Sad Face', 'check_permissions' => 0] + ]; + $output = CRM_Utils_REST::process($args, $params); + $this->assertEquals(1, $output['is_error']); + $this->assertEquals('unauthorized', $output['error_code']); + + // Test that a nested chain is also forced to use permissions. + unset($params['api_contact_create']); + $params['api.job_log.get'] = [ + 'id' => $jobLogID, + 'check_permissions' => 0, + 'api.contact.get' => [ + 'id' => $contactID, + 'check_permissions' => 0, + 'return' => 'display_name', + ]]; + $output = CRM_Utils_REST::process($args, $params); + $this->assertEquals($jobLogID, $output['id']); + $chain = $output['values'][0]['api.job_log.get']; + $this->assertEquals(1, $chain['count'], "Expected the first chain to work."); + // Check the inner contact.get returned nothing + $chain = $chain['values'][0]['api.contact.get']; + $this->assertEquals(0, $chain['count'], "Vulnerable."); + } + + /** + */ + public function alterAPIPermissions($entity, $action, &$params, &$permissions) { + if ($entity === 'job_log' && $action === 'get') { + $permissions['job_log']['get'] = []; + } + } } -- 2.25.1