From 5e207917861c6a886febb5105b4559c5e13b764c Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Sat, 26 Sep 2026 23:06:49 +0200 Subject: [PATCH 1/3] fix(install): make the database identifier validators reject bad input validateDbName(), validateDbCollation() and validateTablePrefix() failed only when preg_match() returned false, which happens on a regex error, not on a mismatch - so they accepted anything. The values reach CREATE DATABASE (the collation unquoted on MySQL), SELECT ... FROM {prefix}site_content and the generated connection config. - The three validators now require a match. Database names may start with a digit and contain $ so existing databases still upgrade; collations may contain @ (sr_RS@latin) or be empty. - The connection test validates the collation, which it read raw, and reports a rejected value as a failed check instead of a PHP fatal. - install.php validates the table prefix it writes into the config. validateDbUser() and validateAdminUsername() share the bug but only reach PDO and the ORM; enforcing them would reject names in use today. Co-Authored-By: Claude Opus 5.5 --- .../InstallerIdentifierValidationTest.php | 120 ++++++++++++++++++ .../controllers/connection/databasetest.php | 22 ++-- install/src/controllers/install.php | 2 +- install/src/functions.php | 20 +-- 4 files changed, 146 insertions(+), 18 deletions(-) create mode 100644 core/tests/Unit/Security/InstallerIdentifierValidationTest.php diff --git a/core/tests/Unit/Security/InstallerIdentifierValidationTest.php b/core/tests/Unit/Security/InstallerIdentifierValidationTest.php new file mode 100644 index 0000000000..363005f331 --- /dev/null +++ b/core/tests/Unit/Security/InstallerIdentifierValidationTest.php @@ -0,0 +1,120 @@ +toBe('evo') + ->and(validateDbName('my_site-2'))->toBe('my_site-2') + ->and(validateDbName('1site'))->toBe('1site') + ->and(validateDbName('app$db'))->toBe('app$db') + ->and(validateDbName(' `evo` '))->toBe('evo'); + }); + + test('rejects names that could leave their quotes', function (string $name) { + expect(fn () => validateDbName($name))->toThrow(InvalidArgumentException::class); + })->with([ + 'backtick' => ['evo`; DROP DATABASE mysql; -- '], + 'double quote' => ['evo" WITH OWNER postgres'], + 'single quote' => ["evo' OR 1=1"], + 'space' => ['evo site'], + 'semicolon' => ['evo;'], + 'newline' => ["evo\nDROP"], + 'path' => ['../evo'], + 'empty' => [''], + ]); + + test('rejects names of 64 characters or more', function () { + expect(fn () => validateDbName(str_repeat('a', 64)))->toThrow(InvalidArgumentException::class); + }); +}); + +describe('validateDbCollation()', function () { + + test('accepts MySQL, PostgreSQL and SQLite collations', function (string $collation) { + expect(validateDbCollation($collation))->toBe($collation); + })->with([ + 'utf8mb4_unicode_ci', 'utf8mb4_0900_ai_ci', 'en_US.UTF-8', 'en_US.utf8', 'C', 'C.UTF-8', + 'en-US-x-icu', 'sr_RS@latin', 'utf8', '', + ]); + + test('rejects collations carrying SQL', function (string $collation) { + expect(fn () => validateDbCollation($collation))->toThrow(InvalidArgumentException::class); + })->with([ + 'unquoted clause' => ['utf8mb4_general_ci; DROP DATABASE mysql'], + 'quote break' => ["en_US.utf8' TEMPLATE template1 --"], + 'comment' => ['utf8mb4_bin/**/'], + 'leading digit' => ['1collation'], + ]); +}); + +describe('validateTablePrefix()', function () { + + test('accepts the prefixes the installer generates or allows', function () { + expect(validateTablePrefix('evo_'))->toBe('evo_') + ->and(validateTablePrefix('a1b2_'))->toBe('a1b2_') + ->and(validateTablePrefix(''))->toBe('') + ->and(validateTablePrefix(null))->toBe(''); + }); + + test('rejects prefixes that would change the statement or the config file', function (string $prefix) { + expect(fn () => validateTablePrefix($prefix))->toThrow(InvalidArgumentException::class); + })->with([ + 'statement' => ['x; DROP TABLE users; --'], + 'subquery' => ['(SELECT 1)'], + 'quote' => ["evo_'"], + 'php' => ['evo_\'.phpinfo().\''], + 'too long' => [str_repeat('a', 45)], + ]); +}); + +describe('call sites', function () { + + test('the database test validates the collation before it reaches CREATE DATABASE', function () { + expect(installerRoot('install/src/controllers/connection/databasetest.php')) + ->toContain("\$database_collation = validateDbCollation(\$_POST['database_collation'] ?? '');") + ->not->toContain("\$database_collation = \$_POST['database_collation'];"); + }); + + test('the database test reports a rejected value instead of dying on it', function () { + // The validators never threw before, so nothing here caught them. + expect(installerRoot('install/src/controllers/connection/databasetest.php')) + ->toMatch('/try \{\s+\$driver = validateDbType.*?\} catch \(InvalidArgumentException \$e\) \{\s+exit\(\$output \. \'\'/s'); + }); + + test('the installer validates the prefix it writes into the connection config', function () { + expect(installerRoot('install/src/controllers/install.php')) + ->toContain("addslashes(validateTablePrefix(\$_POST['tableprefix'] ?? ''))"); + }); + + test('the legacy store processor casts ids and escapes event names', function () { + $source = installerRoot('assets/modules/store/installer/instprocessor.php'); + + expect($source) + ->toContain('$id = (int) $row["id"];') + ->toContain("\$templateId = (int) \$tRow['id'];") + ->toContain('$prev_id = (int) $prev_id;') + ->toContain("evo()->getDatabase()->escape(array_map('trim', \$events))") + ->not->toContain("WHERE id='{\$row['id']}'") + ->not->toContain('DELETE FROM $dbase.'); + }); +}); diff --git a/install/src/controllers/connection/databasetest.php b/install/src/controllers/connection/databasetest.php index 14b01e6117..1b831be7d4 100644 --- a/install/src/controllers/connection/databasetest.php +++ b/install/src/controllers/connection/databasetest.php @@ -1,14 +1,18 @@ ' . $_lang['status_failed'] . ' ' + . htmlspecialchars($e->getMessage(), ENT_QUOTES, 'UTF-8') . ''); +} +$installMode = (int)$_POST['installMode']; $database_charset = getDatabaseCharset($database_collation, $driver); try { diff --git a/install/src/controllers/install.php b/install/src/controllers/install.php index 7fbf3194b5..5ac0e06380 100644 --- a/install/src/controllers/install.php +++ b/install/src/controllers/install.php @@ -100,7 +100,7 @@ $confph['connection_collation'] = addslashes($database_collation); $confph['database_name'] = addslashes( $database_type === 'sqlite' ? sqliteDbNameToPath($database_name) : $database_name); - $confph['table_prefix'] = addslashes($_POST['tableprefix'] ?? ''); + $confph['table_prefix'] = addslashes(validateTablePrefix($_POST['tableprefix'] ?? '')); $confph['lastInstallTime'] = time(); $confph['site_sessionname'] = addslashes($site_sessionname); $confph['database_engine'] = ''; diff --git a/install/src/functions.php b/install/src/functions.php index a5c373d3d2..83530a00c2 100644 --- a/install/src/functions.php +++ b/install/src/functions.php @@ -258,9 +258,11 @@ function validateDbName($name) if (strlen($name) >= 64) { throw new InvalidArgumentException("Database name should be shorter than 64 characters"); } - if (preg_match('/^[a-zA-Z][a-zA-Z0-9_-]{0,63}$/', $name) === false) { - throw new InvalidArgumentException("Database name should begin with letter and contain only letters," - . " numbers, dashes, and underscores"); + // The name is spliced into CREATE DATABASE and the config file. preg_match() returns 0, not + // false, for a mismatch, so the old "=== false" test let everything through. + if (preg_match('/^[A-Za-z0-9_$-]+$/', $name) !== 1) { + throw new InvalidArgumentException("Database name should contain only letters, numbers, dashes," + . " dollar signs and underscores"); } return $name; } @@ -268,9 +270,11 @@ function validateDbName($name) function validateDbCollation($collation) { $collation = trim($collation ?? ''); - if (preg_match('/^[a-zA-Z][a-zA-Z0-9_.-]{0,63}$/', $collation) === false) { - throw new InvalidArgumentException("Database name should begin with letter and contain only letters," - . " numbers, dots, dashes, and underscores"); + // Also spliced into CREATE DATABASE, unquoted on MySQL. "@" covers PostgreSQL locale + // modifiers such as sr_RS@latin. + if (preg_match('/^(?:[A-Za-z][A-Za-z0-9_.@-]{0,63})?$/', $collation) !== 1) { + throw new InvalidArgumentException("Database collation should begin with letter and contain only letters," + . " numbers, dots, dashes, at signs and underscores"); } return $collation; } @@ -319,8 +323,8 @@ function validateDbHost($host, $type) function validateTablePrefix($prefix) { - $prefix = trim($prefix); - if (preg_match('/^[a-zA-Z0-9_]{0,40}_?$/', $prefix) === false) { + $prefix = trim((string) ($prefix ?? '')); + if (preg_match('/^[a-zA-Z0-9_]{0,40}_?$/', $prefix) !== 1) { throw new InvalidArgumentException('Invalid table prefix'); } return $prefix; From bb85d96542a6dd06ef5646f26f69c4d04b4826cc Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Sat, 26 Sep 2026 23:06:49 +0200 Subject: [PATCH 2/3] fix(store): cast ids and escape event names in the legacy installer processor instprocessor.php is deprecated and no longer included (the store uses instprocessor-fast.php), but it still interpolated row ids and package event names into SQL. Ids are cast to int, event names escaped, and an undefined $dbase that made the tmplvar-templates DELETE invalid is removed. Co-Authored-By: Claude Opus 5.5 --- .../modules/store/installer/instprocessor.php | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/assets/modules/store/installer/instprocessor.php b/assets/modules/store/installer/instprocessor.php index f5d9574d8a..be41474537 100644 --- a/assets/modules/store/installer/instprocessor.php +++ b/assets/modules/store/installer/instprocessor.php @@ -149,7 +149,7 @@ function propertiesNameValue($propertyString) { if (evo()->getDatabase()->getRecordCount($rs)) { $insert = true; while($row = evo()->getDatabase()->getRow($rs,'assoc')) { - if (!@evo()->getDatabase()->query("UPDATE `" . $table_prefix . "site_tmplvars` SET type='$input_type', caption='$caption', description='$desc', category='$category', locked='$locked', elements='$input_options', display='$output_widget', display_params='$output_widget_params', default_text='$input_default' WHERE id='{$row['id']}';")) { + if (!@evo()->getDatabase()->query("UPDATE `" . $table_prefix . "site_tmplvars` SET type='$input_type', caption='$caption', description='$desc', category='$category', locked='$locked', elements='$input_options', display='$output_widget', display_params='$output_widget_params', default_text='$input_default' WHERE id='" . (int) $row['id'] . "';")) { echo "

" . mysql_error() . "

"; return; } @@ -175,8 +175,8 @@ function propertiesNameValue($propertyString) { // remove existing tv -> template assignments $ds = evo()->getDatabase()->query("SELECT id FROM `".$table_prefix."site_tmplvars` WHERE name='$name' AND description='$desc';" ); $row = evo()->getDatabase()->getRow($ds,'assoc'); - $id = $row["id"]; - evo()->getDatabase()->query("DELETE FROM $dbase.`" . $table_prefix . "site_tmplvar_templates` WHERE tmplvarid = '$id';"); + $id = (int) $row["id"]; + evo()->getDatabase()->query("DELETE FROM `" . $table_prefix . "site_tmplvar_templates` WHERE tmplvarid = '$id';"); // add tv -> template assignments foreach ($assignments as $assignment) { @@ -186,7 +186,7 @@ function propertiesNameValue($propertyString) { $ts = evo()->getDatabase()->query("SELECT id FROM `".$table_prefix."site_templates` $where;" ); if ($ds && $ts) { $tRow = evo()->getDatabase()->getRow($ts,'assoc'); - $templateId = $tRow['id']; + $templateId = (int) $tRow['id']; evo()->getDatabase()->query("INSERT INTO `" . $table_prefix . "site_tmplvar_templates` (tmplvarid, templateid) VALUES('$id', '$templateId');"); } } @@ -338,13 +338,13 @@ function propertiesNameValue($propertyString) { while($row = evo()->getDatabase()->getRow($rs,'assoc')) { $props = evo()->getDatabase()->escape(propUpdate($properties,$row['properties'])); if ($row['description'] == $desc) { - if (!@evo()->getDatabase()->query("UPDATE `" . $table_prefix . "site_plugins` SET plugincode='$plugin', description='$desc', properties='$props' WHERE id='{$row['id']}';")) { + if (!@evo()->getDatabase()->query("UPDATE `" . $table_prefix . "site_plugins` SET plugincode='$plugin', description='$desc', properties='$props' WHERE id='" . (int) $row['id'] . "';")) { echo "

" . mysql_error() . "

"; return; } $insert = false; } else { - if (!@evo()->getDatabase()->query("UPDATE `" . $table_prefix . "site_plugins` SET disabled='1' WHERE id='{$row['id']}';")) { + if (!@evo()->getDatabase()->query("UPDATE `" . $table_prefix . "site_plugins` SET disabled='1' WHERE id='" . (int) $row['id'] . "';")) { echo "

".mysql_error()."

"; return; } @@ -371,12 +371,13 @@ function propertiesNameValue($propertyString) { $ds = evo()->getDatabase()->query("SELECT id FROM `".$table_prefix."site_plugins` WHERE name='$name' AND description='$desc' ORDER BY id DESC LIMIT 1;" ); if ($ds) { $row = evo()->getDatabase()->getRow($ds,'assoc'); - $id = $row["id"]; - $_events = implode("','", $events); + $id = (int) $row["id"]; + // Event names come from the package's docblock, not from this site. + $_events = implode("','", evo()->getDatabase()->escape(array_map('trim', $events))); // add new events if ($prev_id) { - $prev_id = EvolutionCms()->getDatabase()->escape($prev_id); + $prev_id = (int) $prev_id; evo()->getDatabase()->query("INSERT OR IGNORE INTO `{$table_prefix}site_plugin_events` (`pluginid`, `evtid`, `priority`) SELECT {$id} as 'pluginid', `se`.`id` AS `evtid`, COALESCE(`spe`.`priority`, MAX(`spe2`.`priority`) + 1, 0) AS `priority` From 63e3568889fd1c73329a5bf48994e100f67aedf1 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Sat, 26 Sep 2026 23:25:05 +0200 Subject: [PATCH 3/3] fix(install): accept only shipped installer and manager languages Co-Authored-By: Claude Opus 5.5 --- .../InstallerLanguageSelectionTest.php | 83 +++++++++++++++++++ install/src/lang.php | 35 ++++---- 2 files changed, 104 insertions(+), 14 deletions(-) create mode 100644 core/tests/Unit/Install/InstallerLanguageSelectionTest.php diff --git a/core/tests/Unit/Install/InstallerLanguageSelectionTest.php b/core/tests/Unit/Install/InstallerLanguageSelectionTest.php new file mode 100644 index 0000000000..bc05834821 --- /dev/null +++ b/core/tests/Unit/Install/InstallerLanguageSelectionTest.php @@ -0,0 +1,83 @@ + $install_language, "manager" => $manager_language,' + . ' "hasStrings" => count($_lang) > 0]);'; + + $tmp = tempnam(sys_get_temp_dir(), 'evo-lang-'); + file_put_contents($tmp, $script); + try { + $output = shell_exec(escapeshellarg(PHP_BINARY) . ' ' . escapeshellarg($tmp) . ' 2>&1'); + } finally { + unlink($tmp); + } + + $result = json_decode((string) $output, true); + self::assertIsArray($result, 'lang.php failed: ' . $output); + + return $result; + } + + public function testShippedLanguageIsSelected(): void + { + $result = $this->resolve(['language' => 'de', 'managerlanguage' => 'uk']); + + self::assertSame('de', $result['install']); + self::assertSame('uk', $result['manager']); + self::assertTrue($result['hasStrings']); + } + + public function testUnknownLanguageFallsBackInsteadOfFailingTheInclude(): void + { + $result = $this->resolve(['language' => 'xx', 'managerlanguage' => 'xx']); + + self::assertSame('en', $result['install']); + self::assertSame('en', $result['manager']); + self::assertTrue($result['hasStrings']); + } + + public function testPathLikeLanguageValuesAreRejected(): void + { + $result = $this->resolve( + ['language' => '../../core/config/app', 'managerlanguage' => '..'], + ['language' => ['de']], + '..' + ); + + self::assertSame('en', $result['install']); + self::assertSame('en', $result['manager']); + } + + public function testPostTakesPrecedenceOverQueryAndAcceptLanguage(): void + { + $result = $this->resolve(['language' => 'fr'], ['language' => 'it'], 'de-DE,de;q=0.9'); + + self::assertSame('it', $result['install']); + self::assertSame('it', $result['manager']); + } + + public function testAcceptLanguageIsUsedWhenNothingIsRequested(): void + { + $result = $this->resolve([], [], 'nl-NL,nl;q=0.9'); + + self::assertSame('nl', $result['install']); + } +} diff --git a/install/src/lang.php b/install/src/lang.php index 9bcdd34ecc..8fcee11b83 100644 --- a/install/src/lang.php +++ b/install/src/lang.php @@ -15,32 +15,39 @@ #default fallback language file - english $install_language = 'en'; +// Language codes arrive from the request and are interpolated into include paths, so a code is +// only accepted when it is a plain alphabetic stem that names a shipped locale file/directory. +$installLanguageExists = static function ($code): bool { + return is_string($code) && ctype_alpha($code) && is_file(__DIR__ . '/lang/' . $code . '.inc.php'); +}; +$managerLanguageExists = static function ($code): bool { + return is_string($code) && ctype_alpha($code) && is_dir(dirname(__DIR__, 2) . '/core/lang/' . $code); +}; + $_langISO6391 = substr($_SERVER['HTTP_ACCEPT_LANGUAGE'] ?? '', 0, 2); -if (ctype_alpha($_langISO6391) && file_exists(__DIR__ . '/lang/' . $_langISO6391 . '.inc.php')) { +if ($installLanguageExists($_langISO6391)) { $install_language = $_langISO6391; } -if (isset($_POST['language']) && ctype_alpha($_POST['language'])) { +if ($installLanguageExists($_POST['language'] ?? null)) { $install_language = $_POST['language']; -} else { - if (isset($_GET['language']) && ctype_alpha($_GET['language'])) { - $install_language = $_GET['language']; - } +} elseif ($installLanguageExists($_GET['language'] ?? null)) { + $install_language = $_GET['language']; } # load language file -require_once 'lang/en.inc.php'; // As fallback +require __DIR__ . '/lang/en.inc.php'; // As fallback $fallbackLang = $_lang; -require_once 'lang/' . $install_language . '.inc.php'; +if ($install_language !== 'en') { + require __DIR__ . '/lang/' . $install_language . '.inc.php'; +} $_lang += $fallbackLang; -$manager_language = $install_language; +$manager_language = $managerLanguageExists($install_language) ? $install_language : 'en'; -if (isset($_POST['managerlanguage']) && ctype_alpha($_POST['managerlanguage'])) { +if ($managerLanguageExists($_POST['managerlanguage'] ?? null)) { $manager_language = $_POST['managerlanguage']; -} else { - if (isset($_GET['managerlanguage']) && ctype_alpha($_GET['managerlanguage'])) { - $manager_language = $_GET['managerlanguage']; - } +} elseif ($managerLanguageExists($_GET['managerlanguage'] ?? null)) { + $manager_language = $_GET['managerlanguage']; } foreach ($_lang as $k => $v) {