From fa7465e4709437f1aaf0c13add655763fe0bf956 Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Sun, 18 Jul 2021 13:23:23 -0400 Subject: [PATCH] APIv4 - Validate number of args passed to SQL functions --- Civi/Api4/Query/SqlFunction.php | 16 +++++++----- Civi/Api4/Query/SqlFunctionABS.php | 1 - Civi/Api4/Query/SqlFunctionAVG.php | 1 - Civi/Api4/Query/SqlFunctionCOALESCE.php | 2 +- Civi/Api4/Query/SqlFunctionCONCAT.php | 2 +- Civi/Api4/Query/SqlFunctionCOUNT.php | 2 +- Civi/Api4/Query/SqlFunctionGREATEST.php | 2 +- Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php | 6 ++--- Civi/Api4/Query/SqlFunctionIF.php | 3 ++- Civi/Api4/Query/SqlFunctionISNULL.php | 1 - Civi/Api4/Query/SqlFunctionLEAST.php | 2 +- Civi/Api4/Query/SqlFunctionLOWER.php | 1 - Civi/Api4/Query/SqlFunctionMAX.php | 1 - Civi/Api4/Query/SqlFunctionMIN.php | 1 - Civi/Api4/Query/SqlFunctionNULLIF.php | 3 ++- Civi/Api4/Query/SqlFunctionREPLACE.php | 3 --- Civi/Api4/Query/SqlFunctionROUND.php | 2 -- Civi/Api4/Query/SqlFunctionSUM.php | 1 - Civi/Api4/Query/SqlFunctionUPPER.php | 1 - .../phpunit/api/v4/Action/SqlFunctionTest.php | 25 ++++++++++++++++++- 20 files changed, 46 insertions(+), 30 deletions(-) diff --git a/Civi/Api4/Query/SqlFunction.php b/Civi/Api4/Query/SqlFunction.php index c6c4f93e4e..44923c6a56 100644 --- a/Civi/Api4/Query/SqlFunction.php +++ b/Civi/Api4/Query/SqlFunction.php @@ -53,8 +53,12 @@ abstract class SqlFunction extends SqlExpression { 'expr' => [], 'suffix' => NULL, ]; - if ($param['expr'] && isset($prefix) || in_array('', $param['prefix']) || !$param['optional']) { - $this->args[$idx]['expr'] = $this->captureExpressions($arg, $param['expr'], $param['must_be'], $param['cant_be']); + if ($param['max_expr'] && isset($prefix) || in_array('', $param['prefix']) || !$param['optional']) { + $exprs = $this->captureExpressions($arg, $param['must_be'], $param['cant_be']); + if (count($exprs) < $param['min_expr'] || count($exprs) > $param['max_expr']) { + throw new \API_Exception('Incorrect number of arguments for SQL function ' . static::getName()); + } + $this->args[$idx]['expr'] = $exprs; $this->args[$idx]['suffix'] = $this->captureKeyword($param['suffix'], $arg); } } @@ -82,13 +86,12 @@ abstract class SqlFunction extends SqlExpression { * Shifts 0 or more expressions off the argument string and returns them * * @param string $arg - * @param int $limit * @param array $mustBe * @param array $cantBe * @return array * @throws \API_Exception */ - private function captureExpressions(&$arg, $limit, $mustBe, $cantBe) { + private function captureExpressions(&$arg, $mustBe, $cantBe) { $captured = []; $arg = ltrim($arg); while ($arg) { @@ -98,7 +101,7 @@ abstract class SqlFunction extends SqlExpression { $this->fields = array_merge($this->fields, $expr->getFields()); $captured[] = $expr; // Keep going if we have a comma indicating another expression follows - if (count($captured) < $limit && substr($arg, 0, 1) === ',') { + if (substr($arg, 0, 1) === ',') { $arg = ltrim(substr($arg, 1)); } else { @@ -227,7 +230,8 @@ abstract class SqlFunction extends SqlExpression { // Merge in defaults to ensure each param has these properties $params[] = $param + [ 'prefix' => [], - 'expr' => 1, + 'min_expr' => 1, + 'max_expr' => 1, 'suffix' => [], 'optional' => FALSE, 'must_be' => [], diff --git a/Civi/Api4/Query/SqlFunctionABS.php b/Civi/Api4/Query/SqlFunctionABS.php index 96d682cd57..a5909ec818 100644 --- a/Civi/Api4/Query/SqlFunctionABS.php +++ b/Civi/Api4/Query/SqlFunctionABS.php @@ -20,7 +20,6 @@ class SqlFunctionABS extends SqlFunction { protected static $params = [ [ - 'expr' => 1, 'optional' => FALSE, 'must_be' => ['SqlField', 'SqlNumber'], ], diff --git a/Civi/Api4/Query/SqlFunctionAVG.php b/Civi/Api4/Query/SqlFunctionAVG.php index 577f05afc7..a9787d2e96 100644 --- a/Civi/Api4/Query/SqlFunctionAVG.php +++ b/Civi/Api4/Query/SqlFunctionAVG.php @@ -21,7 +21,6 @@ class SqlFunctionAVG extends SqlFunction { protected static $params = [ [ 'prefix' => ['', 'DISTINCT', 'ALL'], - 'expr' => 1, 'must_be' => ['SqlField'], ], ]; diff --git a/Civi/Api4/Query/SqlFunctionCOALESCE.php b/Civi/Api4/Query/SqlFunctionCOALESCE.php index 1173106e9c..324d62fecf 100644 --- a/Civi/Api4/Query/SqlFunctionCOALESCE.php +++ b/Civi/Api4/Query/SqlFunctionCOALESCE.php @@ -20,7 +20,7 @@ class SqlFunctionCOALESCE extends SqlFunction { protected static $params = [ [ - 'expr' => 99, + 'max_expr' => 99, 'optional' => FALSE, ], ]; diff --git a/Civi/Api4/Query/SqlFunctionCONCAT.php b/Civi/Api4/Query/SqlFunctionCONCAT.php index 5dc63348a1..fa06754078 100644 --- a/Civi/Api4/Query/SqlFunctionCONCAT.php +++ b/Civi/Api4/Query/SqlFunctionCONCAT.php @@ -20,7 +20,7 @@ class SqlFunctionCONCAT extends SqlFunction { protected static $params = [ [ - 'expr' => 99, + 'max_expr' => 99, 'optional' => FALSE, 'must_be' => ['SqlField', 'SqlString'], ], diff --git a/Civi/Api4/Query/SqlFunctionCOUNT.php b/Civi/Api4/Query/SqlFunctionCOUNT.php index 7a9cda1721..8d52c9c733 100644 --- a/Civi/Api4/Query/SqlFunctionCOUNT.php +++ b/Civi/Api4/Query/SqlFunctionCOUNT.php @@ -21,7 +21,7 @@ class SqlFunctionCOUNT extends SqlFunction { protected static $params = [ [ 'prefix' => ['', 'DISTINCT', 'ALL'], - 'expr' => 1, + 'max_expr' => 1, 'must_be' => ['SqlField', 'SqlWild'], 'cant_be' => [], ], diff --git a/Civi/Api4/Query/SqlFunctionGREATEST.php b/Civi/Api4/Query/SqlFunctionGREATEST.php index 5ce1b496a7..47212da6bb 100644 --- a/Civi/Api4/Query/SqlFunctionGREATEST.php +++ b/Civi/Api4/Query/SqlFunctionGREATEST.php @@ -22,7 +22,7 @@ class SqlFunctionGREATEST extends SqlFunction { protected static $params = [ [ - 'expr' => 99, + 'max_expr' => 99, 'optional' => FALSE, ], ]; diff --git a/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php b/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php index 0e0f7525b8..a570bfaf3f 100644 --- a/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php +++ b/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php @@ -23,20 +23,20 @@ class SqlFunctionGROUP_CONCAT extends SqlFunction { protected static $params = [ [ 'prefix' => ['', 'DISTINCT', 'ALL'], - 'expr' => 1, + 'max_expr' => 1, 'must_be' => ['SqlField', 'sqlFunction'], 'optional' => FALSE, ], [ 'prefix' => ['ORDER BY'], - 'expr' => 1, + 'max_expr' => 1, 'suffix' => ['', 'ASC', 'DESC'], 'must_be' => ['SqlField'], 'optional' => TRUE, ], [ 'prefix' => ['SEPARATOR'], - 'expr' => 1, + 'max_expr' => 1, 'must_be' => ['SqlString'], 'optional' => TRUE, // @see self::formatOutput() diff --git a/Civi/Api4/Query/SqlFunctionIF.php b/Civi/Api4/Query/SqlFunctionIF.php index 39f688a163..4a1e244c7d 100644 --- a/Civi/Api4/Query/SqlFunctionIF.php +++ b/Civi/Api4/Query/SqlFunctionIF.php @@ -20,7 +20,8 @@ class SqlFunctionIF extends SqlFunction { protected static $params = [ [ - 'expr' => 3, + 'min_expr' => 3, + 'max_expr' => 3, 'optional' => FALSE, ], ]; diff --git a/Civi/Api4/Query/SqlFunctionISNULL.php b/Civi/Api4/Query/SqlFunctionISNULL.php index 1568c1114a..7b614a5381 100644 --- a/Civi/Api4/Query/SqlFunctionISNULL.php +++ b/Civi/Api4/Query/SqlFunctionISNULL.php @@ -20,7 +20,6 @@ class SqlFunctionISNULL extends SqlFunction { protected static $params = [ [ - 'expr' => 1, 'optional' => FALSE, ], ]; diff --git a/Civi/Api4/Query/SqlFunctionLEAST.php b/Civi/Api4/Query/SqlFunctionLEAST.php index b0f315a988..89aa121e8c 100644 --- a/Civi/Api4/Query/SqlFunctionLEAST.php +++ b/Civi/Api4/Query/SqlFunctionLEAST.php @@ -22,7 +22,7 @@ class SqlFunctionLEAST extends SqlFunction { protected static $params = [ [ - 'expr' => 99, + 'max_expr' => 99, 'optional' => FALSE, ], ]; diff --git a/Civi/Api4/Query/SqlFunctionLOWER.php b/Civi/Api4/Query/SqlFunctionLOWER.php index 2a6b9686f5..0dc052123f 100644 --- a/Civi/Api4/Query/SqlFunctionLOWER.php +++ b/Civi/Api4/Query/SqlFunctionLOWER.php @@ -20,7 +20,6 @@ class SqlFunctionLOWER extends SqlFunction { protected static $params = [ [ - 'expr' => 1, 'optional' => FALSE, 'must_be' => ['SqlField', 'SqlString'], ], diff --git a/Civi/Api4/Query/SqlFunctionMAX.php b/Civi/Api4/Query/SqlFunctionMAX.php index c783d2cab7..f26a2c0aa5 100644 --- a/Civi/Api4/Query/SqlFunctionMAX.php +++ b/Civi/Api4/Query/SqlFunctionMAX.php @@ -23,7 +23,6 @@ class SqlFunctionMAX extends SqlFunction { protected static $params = [ [ 'prefix' => ['', 'DISTINCT', 'ALL'], - 'expr' => 1, 'must_be' => ['SqlField'], ], ]; diff --git a/Civi/Api4/Query/SqlFunctionMIN.php b/Civi/Api4/Query/SqlFunctionMIN.php index f5fe4e86bd..756aeb0392 100644 --- a/Civi/Api4/Query/SqlFunctionMIN.php +++ b/Civi/Api4/Query/SqlFunctionMIN.php @@ -23,7 +23,6 @@ class SqlFunctionMIN extends SqlFunction { protected static $params = [ [ 'prefix' => ['', 'DISTINCT', 'ALL'], - 'expr' => 1, 'must_be' => ['SqlField'], ], ]; diff --git a/Civi/Api4/Query/SqlFunctionNULLIF.php b/Civi/Api4/Query/SqlFunctionNULLIF.php index 0286a46056..f5602cd2cd 100644 --- a/Civi/Api4/Query/SqlFunctionNULLIF.php +++ b/Civi/Api4/Query/SqlFunctionNULLIF.php @@ -22,7 +22,8 @@ class SqlFunctionNULLIF extends SqlFunction { protected static $params = [ [ - 'expr' => 2, + 'min_expr' => 2, + 'max_expr' => 2, 'optional' => FALSE, ], ]; diff --git a/Civi/Api4/Query/SqlFunctionREPLACE.php b/Civi/Api4/Query/SqlFunctionREPLACE.php index d1cdef78e5..b43a7ab802 100644 --- a/Civi/Api4/Query/SqlFunctionREPLACE.php +++ b/Civi/Api4/Query/SqlFunctionREPLACE.php @@ -20,17 +20,14 @@ class SqlFunctionREPLACE extends SqlFunction { protected static $params = [ [ - 'expr' => 1, 'optional' => FALSE, 'must_be' => ['SqlField', 'SqlString'], ], [ - 'expr' => 1, 'optional' => FALSE, 'must_be' => ['SqlField', 'SqlString'], ], [ - 'expr' => 1, 'optional' => FALSE, 'must_be' => ['SqlField', 'SqlString'], ], diff --git a/Civi/Api4/Query/SqlFunctionROUND.php b/Civi/Api4/Query/SqlFunctionROUND.php index 4d5b2f084d..28ee78caab 100644 --- a/Civi/Api4/Query/SqlFunctionROUND.php +++ b/Civi/Api4/Query/SqlFunctionROUND.php @@ -20,12 +20,10 @@ class SqlFunctionROUND extends SqlFunction { protected static $params = [ [ - 'expr' => 1, 'optional' => FALSE, 'must_be' => ['SqlField', 'SqlNumber'], ], [ - 'expr' => 1, 'optional' => TRUE, 'must_be' => ['SqlNumber'], ], diff --git a/Civi/Api4/Query/SqlFunctionSUM.php b/Civi/Api4/Query/SqlFunctionSUM.php index b6b74ae786..81b49005f6 100644 --- a/Civi/Api4/Query/SqlFunctionSUM.php +++ b/Civi/Api4/Query/SqlFunctionSUM.php @@ -21,7 +21,6 @@ class SqlFunctionSUM extends SqlFunction { protected static $params = [ [ 'prefix' => ['', 'DISTINCT', 'ALL'], - 'expr' => 1, 'must_be' => ['SqlField'], ], ]; diff --git a/Civi/Api4/Query/SqlFunctionUPPER.php b/Civi/Api4/Query/SqlFunctionUPPER.php index b9a714e2d5..ed4590e290 100644 --- a/Civi/Api4/Query/SqlFunctionUPPER.php +++ b/Civi/Api4/Query/SqlFunctionUPPER.php @@ -20,7 +20,6 @@ class SqlFunctionUPPER extends SqlFunction { protected static $params = [ [ - 'expr' => 1, 'optional' => FALSE, 'must_be' => ['SqlField', 'SqlString'], ], diff --git a/tests/phpunit/api/v4/Action/SqlFunctionTest.php b/tests/phpunit/api/v4/Action/SqlFunctionTest.php index 72560a1d07..078b9eeaa8 100644 --- a/tests/phpunit/api/v4/Action/SqlFunctionTest.php +++ b/tests/phpunit/api/v4/Action/SqlFunctionTest.php @@ -34,7 +34,8 @@ class SqlFunctionTest extends UnitTestCase { $this->assertArrayHasKey('SUM', $functions); $this->assertArrayNotHasKey('', $functions); $this->assertArrayNotHasKey('SqlFunction', $functions); - $this->assertEquals(1, $functions['MAX']['params'][0]['expr']); + $this->assertEquals(1, $functions['MAX']['params'][0]['min_expr']); + $this->assertEquals(1, $functions['MAX']['params'][0]['max_expr']); } public function testGroupAggregates() { @@ -176,4 +177,26 @@ class SqlFunctionTest extends UnitTestCase { $this->assertEquals(FALSE, $result[$aids[2]]['duration_isnull']); } + public function testIncorrectNumberOfArguments() { + try { + Activity::get(FALSE) + ->addSelect('IF(is_deleted) AS whoops') + ->execute(); + $this->fail('Api should have thrown exception'); + } + catch (\API_Exception $e) { + $this->assertEquals('Incorrect number of arguments for SQL function IF', $e->getMessage()); + } + + try { + Activity::get(FALSE) + ->addSelect('NULLIF(is_deleted, 1, 2) AS whoops') + ->execute(); + $this->fail('Api should have thrown exception'); + } + catch (\API_Exception $e) { + $this->assertEquals('Incorrect number of arguments for SQL function NULLIF', $e->getMessage()); + } + } + } -- 2.25.1