fix(security): close SQL injection in tree sort, TV filters and identifier quoting

core/functions/nodes.php

@@ -1,5 +1,40 @@
 <?php
 
+if (!function_exists('treeSortColumns')) {
+    /**
+     * Columns the manager tree may be sorted by.
+     *
+     * @return string[]
+     */
+    function treeSortColumns(): array
+    {
+        return [
+            'isfolder', 'pagetitle', 'longtitle', 'menutitle', 'id', 'menuindex', 'alias',
+            'createdon', 'editedon', 'publishedon', 'pub_date', 'unpub_date',
+        ];
+    }
+}
+
+if (!function_exists('normalizeTreeSortBy')) {
+    /**
+     * @param mixed $value
+     */
+    function normalizeTreeSortBy($value): string
+    {
+        return is_string($value) && in_array($value, treeSortColumns(), true) ? $value : 'menuindex';
+    }
+}
+
+if (!function_exists('normalizeTreeSortDir')) {
+    /**
+     * @param mixed $value
+     */
+    function normalizeTreeSortDir($value): string
+    {
+        return is_string($value) && strtoupper($value) === 'DESC' ? 'DESC' : 'ASC';
+    }
+}
+
 if (!function_exists('makeHTML')) {
     /**
      * @param int $indent
@@ -25,11 +60,9 @@ function makeHTML($indent, $parent, $expandAll, $hereid = '')
         $spacer .= '</span>';
 
         // manage order-by
-        if (!isset($_SESSION['tree_sortby']) && !isset($_SESSION['tree_sortdir'])) {
-            // This is the first startup, set default sort order
-            $_SESSION['tree_sortby'] = 'menuindex';
-            $_SESSION['tree_sortdir'] = 'ASC';
-        }
+        // Session values may come from the request or from stored user settings: never trust them in SQL
+        $_SESSION['tree_sortby'] = normalizeTreeSortBy($_SESSION['tree_sortby'] ?? null);
+        $_SESSION['tree_sortdir'] = normalizeTreeSortDir($_SESSION['tree_sortdir'] ?? null);
 
         $sc = evo()->getDatabase()->getFullTableName('site_content');
 
@@ -45,7 +78,7 @@ function makeHTML($indent, $parent, $expandAll, $hereid = '')
                 $sortby = $sc . '.' . $_SESSION['tree_sortby'];
         };
 
-        $orderBy = $sortby . ' ' . ($_SESSION['tree_sortdir'] ?? 'ASC');
+        $orderBy = $sortby . ' ' . $_SESSION['tree_sortdir'];
 
         // get document groups for current user
         if (isset($_SESSION['mgrDocgroups']) && is_array($_SESSION['mgrDocgroups'])) {

core/src/Core.php

@@ -5220,22 +5220,26 @@ public function getDocumentChildrenTVars($parentid = 0, $tvidnames = [], $publis
             foreach ($_ as $i => $v) {
                 if ($v === 'value') {
                     unset($_[$i]);
-                } else {
+                } elseif (preg_match('/^(\w+|\*)$/D', $v)) {
                     $_[$i] = 'tv.' . $v;
+                } else {
+                    unset($_[$i]);
                 }
             }
-            $fields = implode(',', $_);
+            $fields = $_ ? implode(',', $_) : 'tv.*';
         } else {
             $fields = "tv.*";
         }
 
-        if ($tvsort != '') {
-            $tvsort = 'tv.' . implode(',tv.', array_filter(array_map('trim', explode(',', $tvsort))));
-        }
+        $tvsortdir = strtoupper(trim((string)$tvsortdir)) === 'DESC' ? 'DESC' : 'ASC';
+        $tvsort = array_filter(array_map('trim', explode(',', (string)$tvsort)), function ($v) {
+            return preg_match('/^\w+$/D', $v);
+        });
+        $tvsort = $tvsort ? 'tv.' . implode(',tv.', $tvsort) : '';
         if ($tvidnames === "*") {
             $query = "tv.id<>0";
         } else {
-            $query = (is_numeric($tvidnames[0]) ? "tv.id" : "tv.name") . " IN ('" . implode("','", $tvidnames) . "')";
+            $query = (is_numeric($tvidnames[0]) ? "tv.id" : "tv.name") . " IN ('" . implode("','", $this->db->escape($tvidnames)) . "')";
         }
 
         foreach ($docs as $doc) {
@@ -5410,18 +5414,24 @@ public function getTemplateVars($idnames = [], $fields = '*', $docid = '', $publ
             if (\is_scalar($fields)) {
                 $fields = explode(',', $fields);
             }
-            $fields = array_filter(array_map('trim', $fields), function ($value) {
-                return $value !== 'value';
-            });
+            $fields = array_values(array_filter(array_map('trim', $fields), function ($value) {
+                return $value !== 'value' && preg_match('/^(\w+|\*)$/D', $value);
+            }));
+            if (!$fields) {
+                $fields = ['*'];
+            }
         } else {
             $fields = ['*'];
         }
-        $sort = ($sort == '') ? '' : $table . '.' . implode(',' . $table . '.', array_filter(array_map('trim', explode(',', $sort))));
+        $sort = array_filter(array_map('trim', explode(',', (string)$sort)), function ($v) {
+            return preg_match('/^\w+(\s+(ASC|DESC))?$/iD', $v);
+        });
+        $sort = $sort ? $table . '.' . implode(',' . $table . '.', $sort) : '';
 
         if ($idnames === '*') {
             $query = '' . $table . '.id<>0';
         } else {
-            $query = (is_numeric($idnames[0]) ? '' . $table . '.id' : '' . $table . '.name') . " IN ('" . implode("','", $idnames) . "')";
+            $query = (is_numeric($idnames[0]) ? '' . $table . '.id' : '' . $table . '.name') . " IN ('" . implode("','", $this->getDatabase()->escape($idnames)) . "')";
         }
 
         $rs = SiteTmplvar::query()

core/src/Database.php

@@ -343,7 +343,7 @@ protected function prepareFields($data, $ignoreAlias = false)
             $tmp = [];
             foreach ($data as $alias => $field) {
                 $tmp[] = ($alias !== $field && !\is_int($alias) && $ignoreAlias === false) ?
-                    ($field . ' as `' . $alias . '`') : $field;
+                    ($field . ' as `' . str_replace('`', '``', (string)$alias) . '`') : $field;
             }
 
             $data = implode(',', $tmp);
@@ -355,6 +355,23 @@ protected function prepareFields($data, $ignoreAlias = false)
         return $this->replacePrefixPlaceholderInTableName($data);
     }
 
+    /**
+     * Quotes a column name/alias for the active driver, doubling embedded quote characters
+     * so an untrusted array key cannot break out of the identifier.
+     *
+     * @param  string|int  $name
+     * @return string
+     */
+    protected function quoteIdentifier($name)
+    {
+        $name = (string)$name;
+        if ($this->getConfig('driver') === 'pgsql') {
+            return '"' . str_replace('"', '""', $name) . '"';
+        }
+
+        return '`' . str_replace('`', '``', $name) . '`';
+    }
+
     /**
      * @param  string|array  $data
      * @param  bool  $hasArray
@@ -628,19 +645,13 @@ public function insert($fields, $intotable, $fromfields = "*", $fromtable = "",
                 $this->query("INSERT INTO {$intotable} {$fields}");
             } else {
                 if (empty($fromtable)) {
-                    switch ($this->getConfig('driver')) {
-                        case 'pgsql':
-                            $fields = "(\"" . implode("\", \"", array_keys($fields)) . "\") VALUES('" . implode("', '",
-                                    array_values($fields)) . "')";
-                            break;
-                        default:
-                            $fields = "(`" . implode("`, `", array_keys($fields)) . "`) VALUES('" . implode("', '",
-                                    array_values($fields)) . "')";
-                            break;
-                    }
+                    $columns = implode(', ', array_map([$this, 'quoteIdentifier'], array_keys($fields)));
+                    $fields = "(" . $columns . ") VALUES('" . implode("', '", array_values($fields)) . "')";
                     $this->query("INSERT INTO {$intotable} {$fields}");
                 } else {
-                    $fields = "(" . implode(",", array_keys($fields)) . ")";
+                    $fields = "(" . implode(",", array_map(function ($column) {
+                        return preg_match('/^[\w.]+$/', (string)$column) ? $column : $this->quoteIdentifier($column);
+                    }, array_keys($fields))) . ")";
                     $where = trim($where);
                     $limit = trim($limit);
                     if ($where !== '' && stripos($where, 'WHERE') !== 0) {
@@ -691,14 +702,7 @@ public function update($fields, $table, $where = "")
                     } else {
                         $f = "'" . $value . "'";
                     }
-                    switch ($this->getConfig('driver')) {
-                        case 'pgsql':
-                            $fields[$key] = "\"{$key}\" = " . $f;
-                            break;
-                        default:
-                            $fields[$key] = "`{$key}` = " . $f;
-                            break;
-                    }
+                    $fields[$key] = $this->quoteIdentifier($key) . ' = ' . $f;
 
                 }
                 $fields = implode(',', $fields);

core/src/Models/SiteContent.php

@@ -95,6 +95,12 @@ class SiteContent extends Eloquent\Model
 
     const CHILDREN_RELATION_NAME = 'children';
 
+    /**
+     * Upper bound for the filters of one tvFilter() call and for the sort terms of one tvOrderBy()
+     * call: every one of them costs the database a join or a sort key, so an unbounded list is a DoS.
+     */
+    const MAX_TV_QUERY_TERMS = 20;
+
     /**
      * ClosureTable model instance.
      *
@@ -2224,6 +2230,10 @@ public function scopeTvFilter($query, $filters = '', $outerSep = ';', $innerSep
     {
         $prefix = evo()->getDatabase()->getConfig('prefix');
         $filters = explode($outerSep, trim($filters));
+        if (count($filters) > self::MAX_TV_QUERY_TERMS) {
+            // Fail closed, like a malformed filter: dropping the surplus would widen the result set
+            return $query->whereRaw('1 = 0');
+        }
         foreach ($filters as $filter) {
             if (empty($filter)) break;
             $parts = explode($innerSep, $filter, 5);
@@ -2232,6 +2242,14 @@ public function scopeTvFilter($query, $filters = '', $outerSep = ';', $innerSep
             $op = $parts[2];
             $value = !empty($parts[3]) ? $parts[3] : '';
             $cast = !empty($parts[4]) ? $parts[4] : '';
+            // The name, operator and cast end up in raw SQL below, so refuse anything that is not plain
+            if (!preg_match('/^[\w\-]+$/D', (string)$tvname)
+                || !preg_match('/^(=|!=|<>|<=|>=|<|>|[a-z_\-!]+)$/iD', (string)$op)
+                || !preg_match('/^([A-Za-z]+(\(\d+(,\d+)?\))?)?$/D', (string)$cast)) {
+                // Fail closed: dropping a malformed filter would widen the result set
+                $query = $query->whereRaw('1 = 0');
+                continue;
+            }
             $field = 'tv_' . $tvname . '.value';
             if ($type == 'tvd') {
                 $field = \DB::Raw("IFNULL(`" . $prefix . "tv_" . $tvname . "`.`value`, `" . $prefix . "tvd_" . $tvname . "`.`default_text`)");
@@ -2263,6 +2281,9 @@ public function scopeTvFilter($query, $filters = '', $outerSep = ';', $innerSep
                 case ($cast == 'UNSIGNED'):
                 case ($cast == 'SIGNED'):
                 case (strpos($cast, 'DECIMAL') !== false):
+                    if (!is_numeric($value)) {
+                        $value = 0;
+                    }
                     $numericCast = (in_array(evo()->getDatabase()->getConfig('driver'), ['sqlite', 'sqlite3'], true))
                         ? 'INTEGER'
                         : $cast;
@@ -2283,13 +2304,20 @@ public function scopeTvFilter($query, $filters = '', $outerSep = ';', $innerSep
     public function scopeTvOrderBy($query, $orderBy = '', $sep = ':')
     {
         $prefix = evo()->getDatabase()->getConfig('prefix');
-        $orderBy = explode(',', trim($orderBy));
+        $orderBy = array_slice(explode(',', trim($orderBy)), 0, self::MAX_TV_QUERY_TERMS);
         foreach ($orderBy as $parts) {
             if (empty(trim($parts))) return;
             $part = array_map('trim', explode(' ', trim($parts), 3));
             $tvname = $part[0];
             $sortDir = !empty($part[1]) ? $part[1] : 'desc';
             $cast = !empty($part[2]) ? $part[2] : '';
+            // The name, direction and cast end up in raw SQL below, so refuse anything that is not plain
+            $nameOnly = explode($sep, $tvname, 2)[0];
+            if (!preg_match('/^[\w\-]+$/D', $nameOnly)
+                || !preg_match('/^(asc|desc)$/iD', $sortDir)
+                || !preg_match('/^([A-Za-z]+(\(\d+(,\d+)?\))?)?$/D', $cast)) {
+                continue;
+            }
             $driver = evo()->getDatabase()->getConfig('driver');
             $castType = $cast;
             if (in_array($driver, ['sqlite', 'sqlite3'], true) && $castType !== '') {
@@ -2418,6 +2446,8 @@ public static function tvList($docs, $tvList = [])
 
     public function scopeOrderByDate($query, $sortDir = 'desc')
     {
+        $sortDir = strtolower(trim((string)$sortDir)) === 'asc' ? 'ASC' : 'DESC';
+
         return $query->orderByRaw('CASE WHEN pub_date != 0 THEN pub_date ELSE createdon END ' . $sortDir);
     }
 

core/tests/Fixtures/template-pinned-controller.php

@@ -0,0 +1,91 @@
+<?php
+
+namespace Tests\Unit\PinnedControllerFixtures {
+    class BaseController {}
+
+    class HomeController extends BaseController
+    {
+        public static int $constructed = 0;
+        public static int $mainCalls = 0;
+
+        public function __construct() { ++self::$constructed; }
+        public function main() { ++self::$mainCalls; }
+    }
+}
+
+namespace {
+    use EvolutionCMS\Core;
+    use EvolutionCMS\Models\SiteTemplate;
+    use EvolutionCMS\TemplateProcessor;
+    use Illuminate\Config\Repository;
+    use Tests\Unit\PinnedControllerFixtures\HomeController;
+
+    // Run in a separate process: it defines CMS constants and the $evo global.
+    define('EVO_CLASS', Core::class);
+    define('IN_MANAGER_MODE', false);
+    require dirname(__DIR__, 2) . '/vendor/autoload.php';
+
+    $assertSame = static function ($expected, $actual, string $message): void {
+        if ($expected !== $actual) {
+            throw new RuntimeException($message . ': expected ' . var_export($expected, true) . ', got ' . var_export($actual, true));
+        }
+    };
+
+    $directory = sys_get_temp_dir() . '/evo-controller-' . bin2hex(random_bytes(6));
+    mkdir($directory);
+    $path = $directory . '/home.blade.php';
+    file_put_contents($path, 'pinned');
+    $app = (new ReflectionClass(Core::class))->newInstanceWithoutConstructor();
+    $GLOBALS['evo'] = $app;
+    $app->instance('config', new Repository([
+        'cms' => ['settings' => ['ControllerNamespace' => substr(HomeController::class, 0, -strlen('HomeController'))]],
+        'view' => [
+            'paths' => [$directory],
+            'template_engines' => ['blade.php' => ['label' => 'Blade', 'processor' => 'blade']],
+        ],
+    ]));
+    $views = new class {
+        public array $found = [];
+        public array $calls = [];
+        public function exists($name) { $this->calls[] = $name; return in_array($name, $this->found, true); }
+        public function getExtensions() { return ['blade.php' => 'blade']; }
+    };
+    $app->instance('view', $views);
+    $app->documentObject = ['id' => 7, 'template' => 2, 'templatealias' => 'home', 'content' => 'page'];
+    $app->documentContent = 'template';
+
+    try {
+        foreach (['blade.php', ''] as $extension) {
+            HomeController::$constructed = 0;
+            HomeController::$mainCalls = 0;
+            // Pinned files must work without resolving the alias through the view finder.
+            $views->found = $extension === '' ? ['home'] : [];
+            $views->calls = [];
+            $row = new SiteTemplate();
+            $row->setRawAttributes(['id' => 2, 'templatesource' => 'file', 'templatefileextension' => $extension]);
+            $processor = new TemplateProcessor($app);
+            (new ReflectionProperty($processor, 'templateRows'))->setValue($processor, [2 => $row]);
+
+            $assertSame('home', $processor->getBladeDocumentContent(), "[$extension] document content");
+            $assertSame(1, HomeController::$constructed, "[$extension] controller constructed");
+            $assertSame(1, HomeController::$mainCalls, "[$extension] controller main calls");
+            $assertSame($extension === '' ? '' : $path, $processor->getDocumentViewPath(), "[$extension] view path");
+            $assertSame($extension === ''
+                ? ['tpl-2_doc-7', 'doc-7', 'tpl-2', 'home']
+                : ['tpl-2_doc-7', 'doc-7', 'tpl-2'], $views->calls, "[$extension] view probes");
+        }
+
+        $views->found = ['doc-7'];
+        $row->templatefileextension = 'blade.php';
+        $processor = new TemplateProcessor($app);
+        (new ReflectionProperty($processor, 'templateRows'))->setValue($processor, [2 => $row]);
+        $assertSame('doc-7', $processor->getBladeDocumentContent(), 'doc-7 override content');
+        $assertSame('', $processor->getDocumentViewPath(), 'doc-7 override view path');
+        $assertSame(1, HomeController::$constructed, 'doc-7 override controller constructed');
+        echo "Pinned and automatic template files run the controller once.\n";
+    } finally {
+        unlink($path);
+        rmdir($directory);
+        unset($GLOBALS['evo']);
+    }
+}

core/tests/Unit/Console/PackageDiscoverBootstrapCacheTest.php

@@ -2,7 +2,7 @@
 
 test('package discovery invalidates bootstrap configuration after generating providers and aliases', function () {
     $fixture = dirname(__DIR__, 2) . '/Fixtures/discover-bootstrap-cache.php';
-    exec(escapeshellarg(PHP_BINARY) . ' ' . escapeshellarg($fixture) . ' 2>&1', $output, $status);
+    $output = evoRunPhp($fixture, [], $status);
 
-    expect($status)->toBe(0, implode("\n", $output));
+    expect($status)->toBe(0, $output);
 });