From 589b2f176278e7d6ecf80d69f8ad568f6dd0f8c5 Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Tue, 3 Mar 2020 18:54:50 -0800
Subject: [PATCH] CIV-01-021 - Improve entity name sanitization

Before
------

* There exist two functions which purport to take an API entity name and sanitize it,
  producing a canonical API entity name. (`\Civi\API\Request::normalizeEntityName`
  and `_civicrm_api_get_camel_name`)
* The two functions are identical for typical inputs. Both call `convertStringToCamel()`.
* The difference relates to unusual/unspecified input characters like `/` or `.` or `+`.
   * `_civicrm_api_get_camel_name()` allows/returns unusual characters.
   * `normalizeEntityName()` filters them out via `\CRM_Utils_String::munge()`

After
-----

* `_civicrm_api_get_camel_name()` just calls `normalizeEntityName()`
* A unit-test provides some comparison/contrast between the old+new behaviors.

Comments
--------

I came into this because CIV-01-021 pointed out that `_civicrm_api_get_camel_name()` had
insufficient sanitization of wonky inputs and could potentially lead to unexpected file-reads.

You can potentially address those wonky inputs by filtering them out or by throwing an exception.
I initially started doing an exception... but it turned out that `normalizeEntityName()` was already
filtering out and didn't really need a change. Also, regardless of the policy, the functions should be
brought into alignment.

Anyway, it seemed like this was the simpler change - it keeps `normalizeEntityName()` working exactly
as before, and only changes `_civicrm_api_get_camel_name()` to match.
---
 api/api.php                        |  2 +-
 tests/phpunit/api/v3/UtilsTest.php | 45 ++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/api/api.php b/api/api.php
index 7cbc5d180d..43902e227c 100644
--- a/api/api.php
+++ b/api/api.php
@@ -200,7 +200,7 @@ function civicrm_error($result) {
  * @return string|null
  */
 function _civicrm_api_get_camel_name($entity) {
-  return is_string($entity) ? CRM_Utils_String::convertStringToCamel($entity) : NULL;
+  return is_string($entity) ? \Civi\API\Request::normalizeEntityName($entity) : NULL;
 }
 
 /**
diff --git a/tests/phpunit/api/v3/UtilsTest.php b/tests/phpunit/api/v3/UtilsTest.php
index 9988575b95..f472f3c0b7 100644
--- a/tests/phpunit/api/v3/UtilsTest.php
+++ b/tests/phpunit/api/v3/UtilsTest.php
@@ -82,6 +82,51 @@ class api_v3_UtilsTest extends CiviUnitTestCase {
     $this->assertTrue($this->runPermissionCheck('contact', 'create', $params), 'permission check should be skippable');
   }
 
+  public function getCamelCaseFuncs() {
+    // There have been two slightly different functions for normalizing names;
+    // _civicrm_api_get_camel_name() and \Civi\API\Request::normalizeEntityName().
+    return [
+      // These are the typical cases - where the two have always agreed.
+      ['Foo', 'Foo'],
+      ['foo', 'Foo'],
+      ['FooBar', 'FooBar'],
+      ['foo_bar', 'FooBar'],
+      ['fooBar', 'FooBar'],
+      ['Im', 'Im'],
+      ['ACL', 'Acl'],
+      ['HTTP', 'HTTP'],
+
+      // These are some atypical cases - where the two have always agreed.
+      ['foo__bar', 'FooBar'],
+      ['Foo_Bar', 'FooBar'],
+      ['one_two_three', 'OneTwoThree'],
+      ['oneTwo_three', 'OneTwoThree'],
+      ['Got2B', 'Got2B'],
+      ['got2_BGood', 'Got2BGood'],
+
+      // These are some atypical cases - where they have traditionally disagreed.
+      // _civicrm_api_get_camel_name() has now changed to match normalizeEntityName()
+      // because the latter is more defensive.
+      ['Foo-Bar', 'FooBar'],
+      ['Foo+Bar', 'FooBar'],
+      ['Foo.Bar', 'FooBar'],
+      ['Foo/../Bar/', 'FooBar'],
+      ['./Foo', 'Foo'],
+    ];
+  }
+
+  /**
+   * @param string $inputValue
+   *   The user-supplied/untrusted entity name.
+   * @param string $expectValue
+   *   The normalized/UpperCamelCase entity name.
+   * @dataProvider getCamelCaseFuncs
+   */
+  public function testCamelName($inputValue, $expectValue) {
+    $actualValue = _civicrm_api_get_camel_name($inputValue);
+    $this->assertEquals($expectValue, $actualValue);
+  }
+
   /**
    * @param string $entity
    * @param string $action
-- 
2.25.1