AI search: judge models (TypeSafe Jev) and natural language search - #5721 - make it secure

inc/classes/BxDolDb.php

@@ -1018,12 +1018,21 @@ class BxDolDb extends BxDolFactory implements iBxDolSingleton
         return $s;
     }
 
-    public function isValidFieldName($s)
+    /**
+     * @param bool $bStrict one identifier of letters, digits and underscores. The loose check also allows a qualified name and most characters inside backticks.
+     */
+    public function isValidFieldName($s, $bStrict = false)
     {
+        if ($bStrict)
+            return is_string($s) && (bool)preg_match('/^[A-Za-z0-9_]+$/', $s);
+
+        if (!is_string($s))
+            return false;
+
         if (false !== strpos($s, '`.`')) {
             $a = explode('`.`', $s, 2);
             foreach ($a as $sField) {
-                if (false === $this->isValidFieldName($sField))
+                if (false === $this->isValidFieldName($sField, $bStrict))
                     return false;
             }
             return true;

inc/classes/BxDolSearch.php

@@ -1225,7 +1225,15 @@ class BxDolSearchResult implements iBxDolReplaceable
                 $sqlCondition = '';
                 if (isset($aValue['operator']) && isset($aValue['value']) && $aValue['value'] !== '' && $aValue['value'] !== false && $aValue['value'] !== null) {
                    $sFieldTable = isset($aValue['table']) ? $aValue['table'] : $this->aCurrent['table'];
-                   $sqlCondition = "`{$sFieldTable}`.`{$aValue['field']}` ";
+                   $sFieldName = isset($aValue['field']) ? $aValue['field'] : '';
+                   if(!$oDb->isValidFieldName($sFieldTable, true))
+                       continue;
+                   if($sFieldName !== '' && !$oDb->isValidFieldName($sFieldName, true))
+                       continue;
+                   if($sFieldName === '' && ($aValue['operator'] ?? '') !== 'against')
+                       continue;
+
+                   $sqlCondition = "`{$sFieldTable}`.`{$sFieldName}` ";
                    switch ($aValue['operator']) {
                        case 'empty value':
                             $sqlCondition .= " = '' ";
@@ -1418,10 +1426,12 @@ class BxDolSearchResult implements iBxDolReplaceable
         if (!$sKeyword)
             return '';
 
-        $sTable = empty($this->aCurrent['tableSearch']) ? $this->aCurrent['table'] : $this->aCurrent['tableSearch'];
-
         $oDb = BxDolDb::getInstance();
 
+        $sTable = empty($this->aCurrent['tableSearch']) ? $this->aCurrent['table'] : $this->aCurrent['tableSearch'];
+        if(!$oDb->isValidFieldName($sTable, true))
+            return '';
+
         $bLike = getParam('useLikeOperator');
 
         if (!is_array($aFields))
@@ -1431,8 +1441,15 @@ class BxDolSearchResult implements iBxDolReplaceable
             $sKeyword = $oDb->escape('%' . preg_replace('/\s+/', '%', $sKeyword) . '%');
 
             $sSqlWhere = '';
-            foreach ($aFields as $sValue)
+            foreach ($aFields as $sValue) {
+                if(!$oDb->isValidFieldName($sValue, true))
+                    continue;
+
                 $sSqlWhere .= "`{$sTable}`.`$sValue` LIKE  " . $sKeyword . " OR ";
+            }
+
+            if($sSqlWhere === '')
+                return '';
 
             $sSqlWhere = '(' . trim($sSqlWhere, 'OR ') . ')';
 
@@ -1440,8 +1457,15 @@ class BxDolSearchResult implements iBxDolReplaceable
         	$sKeyword = $oDb->escape($sKeyword);
 
             $sSqlWhere = '';
-            foreach ($aFields as $sValue)
+            foreach ($aFields as $sValue) {
+                if(!$oDb->isValidFieldName($sValue, true))
+                    continue;
+
                 $sSqlWhere .= "`{$sTable}`.`$sValue`, ";
+            }
+
+            if($sSqlWhere === '')
+                return '';
 
             $sSqlWhere = trim($sSqlWhere, ', ');
             $sSqlWhere = " MATCH({$sSqlWhere}) AGAINST (" . $sKeyword . ") ";

modules/base/general/classes/BxBaseModGeneralModule.php

@@ -1406,9 +1406,13 @@ class BxBaseModGeneralModule extends BxDolModule
                 if(($sK = 'class_search_result') && isset($aParams['params'][$sK]))
                     $aParams[$sK] = $aParams['params'][$sK];
 
-                if(isset($aParams['params']['filters'])) {
-                    foreach($aParams['params']['filters'] as $sKey => $sValue)
+                if(isset($aParams['params']['filters']) && is_array($aParams['params']['filters'])) {
+                    foreach($aParams['params']['filters'] as $sKey => $sValue) {
+                        if(in_array($sKey, ['filter', 'condition', 'filters', 'class_search_result'], true))
+                            continue;
+
                         $aParams['params'][$sKey] = $sValue;
+                    }
 
                     $aParams['params']['filters'] = [
                         'values' => $aParams['params']['filters']
@@ -1420,6 +1424,12 @@ class BxBaseModGeneralModule extends BxDolModule
         if (isset($aParams['params']['condition']))
             unset($aParams['params']['condition']);
 
+        // **Unsafe on the API.** "filter" lets the caller pick both the column and the operator,
+        // so any real column of the module table can be queried, for example status or author.
+        // So 'filters' is safe, 'filter' isn't safe!
+        if($this->_bIsApi && isset($aParams['params']['filter']))
+            unset($aParams['params']['filter']);
+
         return $this->serviceBrowseWithCondition ($aParams);
     }
 

modules/base/general/classes/BxBaseModGeneralSearchResult.php

@@ -152,7 +152,7 @@ class BxBaseModGeneralSearchResult extends BxTemplSearchResult
             return;
 
         foreach($aValues as $sName => $mixedValue) {
-            if(!is_string($sName) || !isset($aFields[$sName]) || !preg_match('/^[A-Za-z0-9_]+$/', $sName))
+            if(!is_string($sName) || !isset($aFields[$sName]) || !$oDb->isValidFieldName($sName, true))
                 continue;
 
             $sOperator = $aFields[$sName];
@@ -248,34 +248,60 @@ class BxBaseModGeneralSearchResult extends BxTemplSearchResult
     protected function addConditionsForFilter($CNF, $sMode, $aParams)
     {
         $aFilter = $aParams['filter'];
-        
-        if(empty($aFilter['field']) || empty($aFilter['value']))
+        $oDb = BxDolDb::getInstance();
+
+        if(empty($aFilter['field']) || !$oDb->isValidFieldName($aFilter['field'], true) || empty($aFilter['value']))
             return;
-        
-        $aRestriction = [
-            'value' => $aFilter['value'],
-            'field' => $aFilter['field'],
-            'operator' => '=',
-        ];
 
-        if(isset($aFilter['operator']))
-            $aRestriction['operator'] = $aFilter['operator'];
+        $aOperators = ['=', '!=', '<>', '<', '>', '<=', '>=', 'like', 'in', 'not in', 'between'];
+        $sOperator = '=';
+        if(isset($aFilter['operator'])) {
+            if(!is_string($aFilter['operator']))
+                return;
+
+            $sOperator = strtolower(trim($aFilter['operator']));
+            if(!in_array($sOperator, $aOperators, true))
+                return;
+        }
+
+        $sTable = $this->aCurrent['table'] ?? '';
+        if(isset($aFilter['table'])) {
+            if(!is_string($aFilter['table']))
+                return;
 
-        if(isset($aFilter['table']))
             switch($aFilter['table']) {
                 case 'table':
-                    $aRestriction['table'] = $this->aCurrent['table'];
+                    $sTable = $this->aCurrent['table'] ?? '';
                     break;
 
                 case 'tableSearch':
-                    $aRestriction['table'] = $this->aCurrent['tableSearch'];
+                    $sTable = $this->aCurrent['tableSearch'] ?? '';
                     break;
 
-                default: 
-                    $aRestriction['table'] = $aFilter['table'];
+                default:
+                    return;
             }
+        }
+
+        if(!$oDb->isValidFieldName($sTable, true))
+            return;
 
-        $this->aCurrent['restriction']['filter'] = $aRestriction;
+        if(!$oDb->isFieldExists($sTable, $aFilter['field']))
+            return;
+
+        $mixedValue = $aFilter['value'];
+        if(in_array($sOperator, ['like', '=', '!=', '<>', '<', '>', '<=', '>='], true) && is_array($mixedValue))
+            return;
+
+        if($sOperator === 'between' && !is_array($mixedValue))
+            return;
+
+        $this->aCurrent['restriction']['filter'] = [
+            'value' => $mixedValue,
+            'field' => $aFilter['field'],
+            'operator' => $sOperator,
+            'table' => $sTable,
+        ];
     }
 
     /**

tests/units/inc/classes/BxDolDbTest.php

@@ -8,9 +8,9 @@ use PHPUnit\Framework\Attributes\DataProvider;
 class BxDolDbTest extends BxDolTestCase
 {
     #[DataProvider('providerForIsValidFieldName')]
-    public function testIsValidFieldName($s, $bRes)
+    public function testIsValidFieldName($s, $bRes, $bStrict = false)
     {
-        $this->assertEquals($bRes, (bool)BxDolDb::getInstance()->isValidFieldName($s));
+        $this->assertEquals($bRes, (bool)BxDolDb::getInstance()->isValidFieldName($s, $bStrict));
     }
 
     static public function providerForIsValidFieldName()
@@ -30,6 +30,14 @@ class BxDolDbTest extends BxDolTestCase
             array('💡', false),
             array('id;', true),
             array('`id`', false),
+            array('table`.`id', true),
+            array('name', true, true),
+            array('имя', false, true),
+            array('id;', false, true),
+            array('na``me2', false, true),
+            array('table`.`id', false, true),
+            array('12345', true, true),
+            array('', false, true),
         );
     }
 

tests/units/modules/boonex/posts/classes/BxPostsSearchResultTest.php

@@ -104,6 +104,35 @@ class BxPostsSearchResultTest extends BxPostsTestCase
         $this->assertSame('bx_posts_posts', $o->aCurrent['restriction']['filter']['table']);
     }
 
+    public function testAddConditionsForFilterRejectsRawTableAndBadNames()
+    {
+        $o = new BxPostsSearchResult('public');
+        $CNF = $this->_oModule->_oConfig->CNF;
+
+        $this->bxCallProtected($o, 'addConditionsForFilter', $CNF, 'public', [
+            'filter' => ['field' => 'id', 'value' => 1, 'operator' => '=', 'table' => 'sys_accounts'],
+        ]);
+        $this->assertArrayNotHasKey('filter', $o->aCurrent['restriction']);
+
+        $this->bxCallProtected($o, 'addConditionsForFilter', $CNF, 'public', [
+            'filter' => ['field' => 'id` OR 1', 'value' => 1, 'operator' => '=', 'table' => 'table'],
+        ]);
+        $this->assertArrayNotHasKey('filter', $o->aCurrent['restriction']);
+
+        $this->bxCallProtected($o, 'addConditionsForFilter', $CNF, 'public', [
+            'filter' => ['field' => 'cat', 'value' => 4, 'operator' => 'or 1=1', 'table' => 'table'],
+        ]);
+        $this->assertArrayNotHasKey('filter', $o->aCurrent['restriction']);
+
+        $o->aCurrent['restriction']['injected'] = [
+            'value' => 'x',
+            'field' => 'id` OR 1',
+            'operator' => '=',
+            'table' => 'bx_posts_posts',
+        ];
+        $this->assertStringNotContainsString('OR 1', $o->getRestriction());
+    }
+
     public function testGalleryUnitViewSwitchesTemplate()
     {
         $sPrev = bx_get('unit_view');