From 9adfd1d61d4669d1d33a318cec35b838c906bf8a Mon Sep 17 00:00:00 2001 From: Arif Hoque Date: Mon, 19 Oct 2026 15:35:52 +0600 Subject: [PATCH 1/3] Fix SQL injection via unvalidated table, view and column identifiers --- src/Phaseolies/Database/Database.php | 18 +- .../Database/Entity/Query/Grammar.php | 33 +- .../DatabaseIdentifierSafetyBehavior.php | 298 ++++++++++++++ .../DatabaseIdentifierSafetyMysqlTest.php | 17 + .../DatabaseIdentifierSafetyPgsqlTest.php | 17 + .../DatabaseIdentifierSafetySqliteTest.php | 13 + .../Builder/DatabaseIdentifierSafetyTest.php | 368 ++++++++++++++++++ tests/Builder/GrammarIdentifierTest.php | 198 ++++++++++ 8 files changed, 956 insertions(+), 6 deletions(-) create mode 100644 tests/Builder/DatabaseIdentifierSafetyBehavior.php create mode 100644 tests/Builder/DatabaseIdentifierSafetyMysqlTest.php create mode 100644 tests/Builder/DatabaseIdentifierSafetyPgsqlTest.php create mode 100644 tests/Builder/DatabaseIdentifierSafetySqliteTest.php create mode 100644 tests/Builder/DatabaseIdentifierSafetyTest.php create mode 100644 tests/Builder/GrammarIdentifierTest.php diff --git a/src/Phaseolies/Database/Database.php b/src/Phaseolies/Database/Database.php index 12ab9387..7db7417a 100644 --- a/src/Phaseolies/Database/Database.php +++ b/src/Phaseolies/Database/Database.php @@ -483,15 +483,29 @@ public function procedure(string $procedureName, array $params = [], array $outp * @param array $params * @return array * @throws PDOException + * @throws \InvalidArgumentException */ public static function view(string $viewName, array $where = [], array $params = []): array { - $sql = "SELECT * FROM {$viewName}"; + if (!static::isValidIdentifier($viewName)) { + throw new \InvalidArgumentException("Invalid view name \"{$viewName}\"."); + } + + $quote = static::getPdoInstance()->getAttribute(PDO::ATTR_DRIVER_NAME) === 'pgsql' ? '"' : '`'; + $quoteId = fn(string $id): string => $quote . str_replace('.', "{$quote}.{$quote}", $id) . $quote; + + $sql = "SELECT * FROM " . $quoteId($viewName); if (!empty($where)) { $conditions = []; foreach ($where as $column => $value) { - $conditions[] = "{$column} = :{$column}"; + $column = (string) $column; + + if (!static::isValidIdentifier($column, false)) { + throw new \InvalidArgumentException("Invalid column name \"{$column}\"."); + } + + $conditions[] = $quoteId($column) . " = :{$column}"; $params[":{$column}"] = $value; } $sql .= " WHERE " . implode(' AND ', $conditions); diff --git a/src/Phaseolies/Database/Entity/Query/Grammar.php b/src/Phaseolies/Database/Entity/Query/Grammar.php index ff7464f5..e17cec19 100644 --- a/src/Phaseolies/Database/Entity/Query/Grammar.php +++ b/src/Phaseolies/Database/Entity/Query/Grammar.php @@ -9,7 +9,29 @@ trait Grammar * * @var string */ - private const VALID_IDENTIFIER_PATTERN = '/^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?$/'; + private const VALID_IDENTIFIER_PATTERN = '/^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?\z/'; + + /** + * Matches a plain, unqualified identifier + * + * @var string + */ + private const VALID_SIMPLE_IDENTIFIER_PATTERN = '/^[A-Za-z_][A-Za-z0-9_]*\z/'; + + /** + * Determine if a value is a safe identifier. + * + * @param string $identifier + * @param bool $allowQualified + * @return bool + */ + protected static function isValidIdentifier(string $identifier, bool $allowQualified = true): bool + { + return (bool) preg_match( + $allowQualified ? self::VALID_IDENTIFIER_PATTERN : self::VALID_SIMPLE_IDENTIFIER_PATTERN, + $identifier + ); + } /** * Ensure a value used as a column identifier is a safe @@ -20,7 +42,7 @@ trait Grammar */ protected function assertValidIdentifier(string $identifier): void { - if (!preg_match(self::VALID_IDENTIFIER_PATTERN, $identifier)) { + if (!static::isValidIdentifier($identifier)) { throw new \InvalidArgumentException( "Invalid column identifier \"{$identifier}\". " . "Use whereRaw()/orderByRaw()/groupByRaw() to build conditions from raw SQL expressions." @@ -72,13 +94,16 @@ public function rand(): string * * @param string $tableName * @return string + * @throws \InvalidArgumentException */ public function getTableColumnsSql(string $tableName): string { + $this->assertValidIdentifier($tableName); + return match ($this->getDriver()) { - 'sqlite' => "PRAGMA table_info({$tableName})", + 'sqlite' => "PRAGMA table_info({$this->quoteIdentifier($tableName)})", 'pgsql' => "SELECT column_name FROM information_schema.columns WHERE table_name = '{$tableName}'", - default => "DESCRIBE {$tableName}", + default => "DESCRIBE {$this->quoteIdentifier($tableName)}", }; } diff --git a/tests/Builder/DatabaseIdentifierSafetyBehavior.php b/tests/Builder/DatabaseIdentifierSafetyBehavior.php new file mode 100644 index 00000000..f737cc11 --- /dev/null +++ b/tests/Builder/DatabaseIdentifierSafetyBehavior.php @@ -0,0 +1,298 @@ + [ + ['name' => 'id', 'type' => 'integer'], + ['name' => 'name', 'type' => 'string'], + ['name' => 'order', 'type' => 'string', 'nullable' => true], + ], + self::AUDIT => [ + ['name' => 'id', 'type' => 'integer'], + ], + self::RESERVED => [ + ['name' => 'id', 'type' => 'integer'], + ['name' => 'label', 'type' => 'string', 'nullable' => true], + ], + ]; + } + + protected function seedData(): array + { + return [ + self::USERS => [ + ['id' => 1, 'name' => 'alice', 'order' => 'a'], + ['id' => 2, 'name' => 'bob', 'order' => 'b'], + ['id' => 3, 'name' => 'carol', 'order' => 'a'], + ], + ]; + } + + protected function bootContainer(): void + { + parent::bootContainer(); + + $config = new \ReflectionClass(Config::class); + foreach (['config' => [], 'cacheFile' => null, 'loadedFromCache' => false, 'fileHashes' => []] as $name => $value) { + if ($config->hasProperty($name)) { + $config->getProperty($name)->setValue(null, $value); + } + } + + // Database::view() resolves its connection from this + Config::set('database.default', 'default'); + } + + /** + * Views depend on their tables; PostgreSQL refuses to drop a table that + * still has a view on it. + */ + protected function dropTablesIfExist(array $tables): void + { + $this->dropView(); + parent::dropTablesIfExist($tables); + } + + protected function setupDatabaseConnections(): void + { + parent::setupDatabaseConnections(); + + $this->dropView(); + $this->pdo->exec(sprintf( + 'CREATE VIEW %s AS SELECT %s, %s, %s FROM %s', + $this->quoteIdentifier(self::VIEW), + $this->quoteIdentifier('id'), + $this->quoteIdentifier('name'), + $this->quoteIdentifier('order'), + $this->quoteIdentifier(self::USERS) + )); + } + + protected function tearDownDatabaseConnections(): void + { + if (isset($this->pdo)) { + $this->dropView(); + } + + parent::tearDownDatabaseConnections(); + } + + private function dropView(): void + { + $this->pdo->exec('DROP VIEW IF EXISTS ' . $this->quoteIdentifier(self::VIEW)); + } + + private function rowCount(string $table): int + { + return (int) $this->pdo->query('SELECT COUNT(*) FROM ' . $this->quoteIdentifier($table))->fetchColumn(); + } + + /** + * The schema/database that qualifies objects on this server + */ + private function currentSchema(): string + { + return (string) $this->pdo->query(match (static::driverName()) { + 'sqlite' => "SELECT 'main'", + 'mysql' => 'SELECT DATABASE()', + 'pgsql' => 'SELECT current_schema()', + })->fetchColumn(); + } + + /** + * @return array + */ + public static function maliciousNames(): array + { + return [ + 'empty' => [''], + 'space' => [self::VIEW . ' u'], + 'trailing newline' => [self::VIEW . "\n"], + 'null byte' => [self::VIEW . "\0"], + 'stacked statement' => [self::VIEW . '; DROP TABLE ' . self::AUDIT . ';--'], + 'union' => [self::VIEW . ' UNION SELECT * FROM ' . self::USERS], + 'single quote' => [self::VIEW . "'"], + 'double quote' => [self::VIEW . '"'], + 'backtick' => [self::VIEW . '`'], + 'backtick breakout' => [self::VIEW . '` WHERE 1=1; --'], + 'double quote breakout' => [self::VIEW . '" WHERE 1=1; --'], + 'comment' => [self::VIEW . '--'], + 'subselect' => ['(SELECT 1)'], + 'too many dots' => ['a.b.c'], + 'unicode quote' => [self::VIEW . "\u{FF07}"], + ]; + } + + /** + * @return array + */ + public static function injectionValues(): array + { + return [ + 'tautology' => ["x' OR '1'='1"], + 'stacked' => ["x'; DROP TABLE " . self::AUDIT . ';--'], + 'union' => ["x' UNION SELECT 1,2,3--"], + 'comment' => ["x'--"], + 'backslash' => ["x\\' OR 1=1--"], + 'double' => ['x" OR "1"="1'], + ]; + } + + // -- Database::view() ---------------------------------------------------- + + public function testViewReturnsAllRows(): void + { + $this->assertCount(3, Database::view(self::VIEW)); + } + + public function testViewReturnsAssociativeRows(): void + { + $this->assertEquals( + ['id' => 1, 'name' => 'alice', 'order' => 'a'], + Database::view(self::VIEW, ['id' => 1])[0] + ); + } + + public function testViewFiltersAndCombinesConditionsWithAnd(): void + { + $this->assertCount(1, Database::view(self::VIEW, ['name' => 'alice'])); + $this->assertCount(2, Database::view(self::VIEW, ['order' => 'a'])); + $this->assertCount(1, Database::view(self::VIEW, ['order' => 'a', 'name' => 'carol'])); + $this->assertSame([], Database::view(self::VIEW, ['order' => 'b', 'name' => 'carol'])); + } + + public function testViewQuotesReservedWordColumn(): void + { + // `order` is a reserved word on every supported server; this only + // works because the column identifier is quoted. + $this->assertCount(2, Database::view(self::VIEW, ['order' => 'a'])); + } + + public function testViewAcceptsSchemaQualifiedName(): void + { + $this->assertCount(3, Database::view($this->currentSchema() . '.' . self::VIEW)); + } + + public function testViewReturnsEmptyArrayWhenNothingMatches(): void + { + $this->assertSame([], Database::view(self::VIEW, ['name' => 'nobody'])); + } + + #[DataProvider('injectionValues')] + public function testViewTreatsValuesAsDataNotSql(string $value): void + { + $this->assertSame([], Database::view(self::VIEW, ['name' => $value])); + $this->assertSame(3, $this->rowCount(self::USERS)); + $this->assertSame(0, $this->rowCount(self::AUDIT)); + } + + #[DataProvider('maliciousNames')] + public function testViewRejectsMaliciousViewName(string $name): void + { + $this->expectException(\InvalidArgumentException::class); + Database::view($name); + } + + #[DataProvider('maliciousNames')] + public function testViewRejectsMaliciousColumnName(string $column): void + { + $this->expectException(\InvalidArgumentException::class); + Database::view(self::VIEW, [$column => 'x']); + } + + public function testViewRejectsQualifiedColumnAndIntegerKey(): void + { + foreach ([[self::VIEW . '.name' => 'alice'], ['alice']] as $where) { + try { + Database::view(self::VIEW, $where); + $this->fail('Expected InvalidArgumentException for ' . json_encode($where)); + } catch (\InvalidArgumentException) { + $this->addToAssertionCount(1); + } + } + } + + public function testRejectedNamesLeaveTheDatabaseUntouched(): void + { + foreach (self::maliciousNames() as [$name]) { + foreach ([fn() => Database::view($name), fn() => Database::view(self::VIEW, [$name => 'x'])] as $call) { + try { + $call(); + } catch (\InvalidArgumentException) { + } + } + } + + $this->assertSame(3, $this->rowCount(self::USERS)); + $this->assertSame(0, $this->rowCount(self::AUDIT)); + $this->assertCount(3, Database::view(self::VIEW)); + } + + // -- Database::getTableColumns() ----------------------------------------- + + public function testGetTableColumnsReturnsColumnNames(): void + { + $columns = (new Database('default'))->getTableColumns(self::USERS); + + $this->assertEqualsCanonicalizing(['id', 'name', 'order'], $columns); + } + + public function testGetTableColumnsReturnsEmptyOrFailsCleanlyForUnknownTable(): void + { + try { + $this->assertSame([], (new Database('default'))->getTableColumns('dpr_ident_missing')); + } catch (\PDOException $e) { + // MySQL's DESCRIBE errors on a missing table; that is a database + // error, never an identifier-validation one. + $this->assertNotInstanceOf(\InvalidArgumentException::class, $e); + } + } + + #[DataProvider('maliciousNames')] + public function testGetTableColumnsRejectsMaliciousTableName(string $table): void + { + try { + (new Database('default'))->getTableColumns($table); + $this->fail('Expected InvalidArgumentException for ' . json_encode($table)); + } catch (\PDOException) { + $this->fail('Validation failure must not reach the database'); + } catch (\InvalidArgumentException) { + $this->assertSame(0, $this->rowCount(self::AUDIT)); + } + } + + public function testGetTableColumnsRejectsNull(): void + { + $this->expectException(\InvalidArgumentException::class); + (new Database('default'))->getTableColumns(null); + } + + public function testGetTableColumnsHandlesReservedWordTableName(): void + { + // The table is dpr_ident_group, so this stays on a prefixed object + // while still exercising quoting of a multi-part identifier. + $this->assertEqualsCanonicalizing(['id', 'label'], (new Database('default'))->getTableColumns(self::RESERVED)); + } +} diff --git a/tests/Builder/DatabaseIdentifierSafetyMysqlTest.php b/tests/Builder/DatabaseIdentifierSafetyMysqlTest.php new file mode 100644 index 00000000..5c74d8e5 --- /dev/null +++ b/tests/Builder/DatabaseIdentifierSafetyMysqlTest.php @@ -0,0 +1,17 @@ + [], 'cacheFile' => null, 'loadedFromCache' => false, 'fileHashes' => []] as $name => $value) { + if ($config->hasProperty($name)) { + $config->getProperty($name)->setValue(null, $value); + } + } + Config::set('database.default', self::CONNECTION); + + $this->pdo = new PDO('sqlite::memory:', null, null, [ + PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, + PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, + ]); + $this->pdo->exec('CREATE TABLE users (id INTEGER, name TEXT, "order" TEXT)'); + $this->pdo->exec("INSERT INTO users VALUES (1, 'alice', 'a'), (2, 'bob', 'b'), (3, 'carol', 'a')"); + $this->pdo->exec('CREATE VIEW user_view AS SELECT id, name, "order" FROM users'); + $this->pdo->exec('CREATE TABLE audit (id INTEGER)'); + + $this->useConnection($this->pdo); + } + + protected function tearDown(): void + { + $this->useConnection(null); + } + + private function useConnection(?PDO $pdo): void + { + (new \ReflectionClass(Database::class)) + ->getProperty('connections') + ->setValue(null, $pdo === null ? [] : [self::CONNECTION => $pdo]); + } + + private function tableCount(string $table): int + { + return (int) $this->pdo->query("SELECT COUNT(*) FROM {$table}")->fetchColumn(); + } + + /** + * @return array + */ + public static function maliciousNames(): array + { + return [ + 'empty' => [''], + 'space' => ['user_view u'], + 'trailing newline' => ["user_view\n"], + 'null byte' => ["user_view\0"], + 'stacked statement' => ['user_view; DROP TABLE audit;--'], + 'union' => ['user_view UNION SELECT * FROM users'], + 'quote' => ["user_view'"], + 'double quote' => ['user_view"'], + 'backtick' => ['user_view`'], + 'backtick breakout' => ['user_view` WHERE 1=1; --'], + 'comment' => ['user_view--'], + 'subselect' => ['(SELECT 1)'], + 'function' => ['randomblob(10)'], + 'too many dots' => ['a.b.c'], + 'unicode quote' => ["user_view\u{FF07}"], + ]; + } + + // -- Database::view(): behaviour ---------------------------------------- + + public function testViewReturnsAllRows(): void + { + $this->assertCount(3, Database::view('user_view')); + } + + public function testViewReturnsAssociativeRows(): void + { + $this->assertSame( + ['id' => 1, 'name' => 'alice', 'order' => 'a'], + Database::view('user_view', ['id' => 1])[0] + ); + } + + public function testViewFiltersBySingleColumn(): void + { + $rows = Database::view('user_view', ['name' => 'alice']); + + $this->assertCount(1, $rows); + $this->assertSame('alice', $rows[0]['name']); + } + + public function testViewCombinesMultipleConditionsWithAnd(): void + { + $this->assertCount(2, Database::view('user_view', ['order' => 'a'])); + $this->assertCount(1, Database::view('user_view', ['order' => 'a', 'name' => 'carol'])); + $this->assertSame([], Database::view('user_view', ['order' => 'b', 'name' => 'carol'])); + } + + public function testViewWorksWithReservedWordColumnBecauseItIsQuoted(): void + { + $this->assertCount(2, Database::view('user_view', ['order' => 'a'])); + } + + public function testViewAcceptsUnderscoreAndDigitNames(): void + { + $this->pdo->exec("CREATE VIEW _v2 AS SELECT 'x' AS _c1"); + + $this->assertSame([['_c1' => 'x']], Database::view('_v2', ['_c1' => 'x'])); + } + + public function testViewAcceptsSchemaQualifiedViewName(): void + { + $this->assertCount(3, Database::view('main.user_view')); + } + + public function testViewReturnsEmptyArrayWhenNothingMatches(): void + { + $this->assertSame([], Database::view('user_view', ['name' => 'nobody'])); + } + + public function testViewSupportsNullAndNumericValues(): void + { + $this->assertSame([], Database::view('user_view', ['name' => null])); + $this->assertCount(1, Database::view('user_view', ['id' => 2])); + } + + public function testViewPassesCallerSuppliedParamsThroughWithGeneratedOnes(): void + { + $recorder = $this->recordingPdo('mysql'); + $this->useConnection($recorder->pdo); + + Database::view('v', ['a' => 1], [':extra' => 'e']); + + $this->assertSame([[':extra' => 'e', ':a' => 1]], $recorder->params); + } + + // -- Database::view(): values are data ----------------------------------- + + #[DataProvider('injectionValues')] + public function testViewTreatsValuesAsDataNotSql(string $value): void + { + $this->assertSame([], Database::view('user_view', ['name' => $value])); + $this->assertSame(3, $this->tableCount('users')); + $this->assertSame(0, $this->tableCount('audit')); + } + + /** + * @return array + */ + public static function injectionValues(): array + { + return [ + 'tautology' => ["x' OR '1'='1"], + 'stacked' => ["x'; DROP TABLE users;--"], + 'union' => ["x' UNION SELECT 1,2,3--"], + 'comment' => ["x'--"], + 'backslash' => ["x\\' OR 1=1--"], + 'null byte' => ["x\0' OR '1'='1"], + ]; + } + + // -- Database::view(): identifiers are validated ------------------------- + + #[DataProvider('maliciousNames')] + public function testViewRejectsMaliciousViewName(string $name): void + { + $this->expectException(\InvalidArgumentException::class); + Database::view($name); + } + + #[DataProvider('maliciousNames')] + public function testViewRejectsMaliciousColumnName(string $column): void + { + $this->expectException(\InvalidArgumentException::class); + Database::view('user_view', [$column => 'x']); + } + + public function testViewRejectsQualifiedColumnName(): void + { + $this->expectException(\InvalidArgumentException::class); + Database::view('user_view', ['user_view.name' => 'alice']); + } + + public function testViewRejectsIntegerKeyedWhere(): void + { + $this->expectException(\InvalidArgumentException::class); + Database::view('user_view', ['alice']); + } + + public function testViewRejectsAnyBadColumnEvenAfterGoodOnes(): void + { + $this->expectException(\InvalidArgumentException::class); + Database::view('user_view', ['name' => 'alice', '1=1; DROP TABLE audit;--' => 1]); + } + + public function testViewDoesNotExecuteAnythingWhenRejected(): void + { + foreach (self::maliciousNames() as [$name]) { + try { + Database::view('user_view', [$name => 'x']); + } catch (\InvalidArgumentException) { + } + try { + Database::view($name); + } catch (\InvalidArgumentException) { + } + } + + $this->assertSame(3, $this->tableCount('users')); + $this->assertSame(0, $this->tableCount('audit')); + $this->assertCount(3, Database::view('user_view')); + } + + public function testViewRejectsBeforeTouchingTheConnection(): void + { + $this->useConnection(null); + + // No connection is configured; a rejected name must fail on validation + // (InvalidArgumentException), not on connecting (RuntimeException). + $this->expectException(\InvalidArgumentException::class); + Database::view('users; DROP TABLE users'); + } + + // -- Database::view(): SQL shape per driver ------------------------------ + + /** + * @return array + */ + public static function viewSqlPerDriver(): array + { + return [ + 'sqlite' => ['sqlite', 'SELECT * FROM `v` WHERE `a` = :a AND `b` = :b', 'SELECT * FROM `v`'], + 'mysql' => ['mysql', 'SELECT * FROM `v` WHERE `a` = :a AND `b` = :b', 'SELECT * FROM `v`'], + 'pgsql' => ['pgsql', 'SELECT * FROM "v" WHERE "a" = :a AND "b" = :b', 'SELECT * FROM "v"'], + ]; + } + + #[DataProvider('viewSqlPerDriver')] + public function testViewQuotesIdentifiersPerDriver(string $driver, string $withWhere, string $withoutWhere): void + { + $recorder = $this->recordingPdo($driver); + $this->useConnection($recorder->pdo); + + Database::view('v'); + Database::view('v', ['a' => 1, 'b' => 2]); + + $this->assertSame([$withoutWhere, $withWhere], $recorder->sql); + $this->assertSame([[], [':a' => 1, ':b' => 2]], $recorder->params); + } + + public function testViewQuotesEachPartOfAQualifiedNameSeparately(): void + { + $mysql = $this->recordingPdo('mysql'); + $this->useConnection($mysql->pdo); + Database::view('app.v'); + $this->assertSame(['SELECT * FROM `app`.`v`'], $mysql->sql); + + $pgsql = $this->recordingPdo('pgsql'); + $this->useConnection($pgsql->pdo); + Database::view('app.v'); + $this->assertSame(['SELECT * FROM "app"."v"'], $pgsql->sql); + } + + public function testViewNeverPlacesValuesInTheSqlText(): void + { + $recorder = $this->recordingPdo('mysql'); + $this->useConnection($recorder->pdo); + + Database::view('v', ['a' => "x' OR '1'='1"]); + + $this->assertStringNotContainsString('OR', $recorder->sql[0]); + $this->assertSame([':a' => "x' OR '1'='1"], $recorder->params[0]); + } + + private function recordingPdo(string $driver): object + { + $recorder = new class { + public array $sql = []; + public array $params = []; + public PDO $pdo; + }; + + $pdo = $this->createStub(PDO::class); + $pdo->method('getAttribute')->willReturn($driver); + $pdo->method('prepare')->willReturnCallback(function (string $sql) use ($recorder) { + $recorder->sql[] = $sql; + + $stmt = $this->createStub(PDOStatement::class); + $stmt->method('execute')->willReturnCallback(function (?array $params = null) use ($recorder) { + $recorder->params[] = $params ?? []; + + return true; + }); + $stmt->method('fetchAll')->willReturn([]); + + return $stmt; + }); + $recorder->pdo = $pdo; + + return $recorder; + } + + // -- Database::getTableColumns() ----------------------------------------- + + public function testGetTableColumnsReturnsColumnNames(): void + { + $this->assertSame(['id', 'name', 'order'], (new Database(self::CONNECTION))->getTableColumns('users')); + } + + public function testGetTableColumnsWorksForReservedWordTableNames(): void + { + $this->pdo->exec('CREATE TABLE "group" (id INTEGER, label TEXT)'); + + $this->assertSame(['id', 'label'], (new Database(self::CONNECTION))->getTableColumns('group')); + } + + #[DataProvider('maliciousNames')] + public function testGetTableColumnsRejectsMaliciousTableName(string $table): void + { + $this->expectException(\InvalidArgumentException::class); + (new Database(self::CONNECTION))->getTableColumns($table); + } + + public function testGetTableColumnsRejectionIsNotWrappedAsPdoException(): void + { + try { + (new Database(self::CONNECTION))->getTableColumns('users; DROP TABLE audit'); + $this->fail('Expected InvalidArgumentException'); + } catch (\PDOException) { + $this->fail('Validation failure must not surface as a database error'); + } catch (\InvalidArgumentException) { + $this->assertSame(0, $this->tableCount('audit')); + } + } + + public function testGetTableColumnsStillRejectsNull(): void + { + $this->expectException(\InvalidArgumentException::class); + (new Database(self::CONNECTION))->getTableColumns(null); + } + + public function testGetTableColumnsReturnsEmptyForUnknownTable(): void + { + $this->assertSame([], (new Database(self::CONNECTION))->getTableColumns('does_not_exist')); + } +} diff --git a/tests/Builder/GrammarIdentifierTest.php b/tests/Builder/GrammarIdentifierTest.php new file mode 100644 index 00000000..5f2bae4b --- /dev/null +++ b/tests/Builder/GrammarIdentifierTest.php @@ -0,0 +1,198 @@ +createStub(PDO::class); + $pdo->method('getAttribute')->willReturn($driver); + + return new class($pdo) { + use Grammar; + + public function __construct(public PDO $pdo) + { + } + + public function assertIdentifier(string $identifier): void + { + $this->assertValidIdentifier($identifier); + } + + public static function valid(string $identifier, bool $qualified = true): bool + { + return static::isValidIdentifier($identifier, $qualified); + } + }; + } + + /** + * @return array + */ + public static function validIdentifiers(): array + { + return [ + 'plain' => ['users'], + 'leading underscore' => ['_users'], + 'digits' => ['users2024'], + 'mixed case' => ['UserAccounts'], + 'single char' => ['a'], + 'underscore only' => ['_'], + 'qualified' => ['main.users'], + 'qualified mixed' => ['App_1.user_view'], + ]; + } + + /** + * Payloads that must never reach SQL as an identifier + * + * @return array + */ + public static function maliciousIdentifiers(): array + { + return [ + 'empty' => [''], + 'whitespace' => [' '], + 'leading digit' => ['1users'], + 'space' => ['users u'], + 'leading space' => [' users'], + 'trailing space' => ['users '], + 'trailing newline' => ["users\n"], + 'trailing CRLF' => ["users\r\n"], + 'embedded newline' => ["users\nDROP TABLE users"], + 'tab' => ["users\t"], + 'null byte' => ["users\0"], + 'null byte mid' => ["us\0ers"], + 'semicolon' => ['users;'], + 'stacked statement' => ['users; DROP TABLE users;--'], + 'line comment' => ['users--'], + 'block comment' => ['users/**/'], + 'hash comment' => ['users#'], + 'single quote' => ["users'"], + 'double quote' => ['users"'], + 'backtick' => ['users`'], + 'backtick breakout' => ['users` WHERE 1=1; --'], + 'quote tautology' => ["x' OR '1'='1"], + 'union' => ['users UNION SELECT 1'], + 'subselect' => ['(SELECT 1)'], + 'parentheses' => ['users()'], + 'function call' => ['SLEEP(5)'], + 'operator' => ['id=1'], + 'wildcard' => ['*'], + 'comma list' => ['a,b'], + 'brackets' => ['[users]'], + 'backslash' => ['users\\'], + 'too many dots' => ['a.b.c'], + 'leading dot' => ['.users'], + 'trailing dot' => ['users.'], + 'double dot' => ['a..b'], + 'dot then digit' => ['a.1b'], + 'hyphen' => ['user-name'], + 'unicode letter' => ['usérs'], + 'unicode fullwidth quote' => ["users\u{FF07}"], + 'unicode fullwidth semi' => ["users\u{FF1B}"], + 'right-to-left override' => ["users\u{202E}"], + 'zero width space' => ["users\u{200B}"], + 'emoji' => ["users\u{1F600}"], + 'invalid utf8' => ["users\xC3\x28"], + ]; + } + + // -- isValidIdentifier / assertValidIdentifier --------------------------- + + #[DataProvider('validIdentifiers')] + public function testAcceptsValidIdentifiers(string $identifier): void + { + $grammar = $this->grammarFor('sqlite'); + + $this->assertTrue($grammar::valid($identifier)); + $grammar->assertIdentifier($identifier); + } + + #[DataProvider('maliciousIdentifiers')] + public function testRejectsMaliciousIdentifiers(string $identifier): void + { + $grammar = $this->grammarFor('sqlite'); + + $this->assertFalse($grammar::valid($identifier)); + $this->expectException(\InvalidArgumentException::class); + $grammar->assertIdentifier($identifier); + } + + public function testUnqualifiedModeRejectsDottedNames(): void + { + $grammar = $this->grammarFor('sqlite'); + + $this->assertTrue($grammar::valid('users', false)); + $this->assertFalse($grammar::valid('main.users', false)); + } + + #[DataProvider('maliciousIdentifiers')] + public function testUnqualifiedModeRejectsMaliciousIdentifiers(string $identifier): void + { + $this->assertFalse($this->grammarFor('sqlite')::valid($identifier, false)); + } + + public function testRejectsVeryLongIdentifierWithoutBacktrackingBlowup(): void + { + $grammar = $this->grammarFor('sqlite'); + $start = microtime(true); + + $this->assertFalse($grammar::valid(str_repeat('a', 100000) . '!')); + $this->assertLessThan(1.0, microtime(true) - $start); + } + + // -- getTableColumnsSql -------------------------------------------------- + + /** + * @return array + */ + public static function expectedSql(): array + { + return [ + 'sqlite' => ['sqlite', 'PRAGMA table_info(`users`)'], + 'mysql' => ['mysql', 'DESCRIBE `users`'], + 'mariadb' => ['mariadb', 'DESCRIBE `users`'], + 'pgsql' => ['pgsql', "SELECT column_name FROM information_schema.columns WHERE table_name = 'users'"], + ]; + } + + #[DataProvider('expectedSql')] + public function testGetTableColumnsSqlPerDriver(string $driver, string $expected): void + { + $this->assertSame($expected, $this->grammarFor($driver)->getTableColumnsSql('users')); + } + + #[DataProvider('maliciousIdentifiers')] + public function testGetTableColumnsSqlRejectsMaliciousTableNameOnEveryDriver(string $table): void + { + foreach (['sqlite', 'mysql', 'mariadb', 'pgsql'] as $driver) { + try { + $this->grammarFor($driver)->getTableColumnsSql($table); + $this->fail("[{$driver}] accepted malicious table name " . json_encode($table)); + } catch (\InvalidArgumentException) { + $this->addToAssertionCount(1); + } + } + } + + public function testQuoteIdentifierUsesDriverSpecificQuoteCharacter(): void + { + $this->assertSame('`users`', $this->grammarFor('mysql')->quoteIdentifier('users')); + $this->assertSame('`users`', $this->grammarFor('sqlite')->quoteIdentifier('users')); + $this->assertSame('"users"', $this->grammarFor('pgsql')->quoteIdentifier('users')); + $this->assertSame('"main"."users"', $this->grammarFor('pgsql')->quoteIdentifier('main.users')); + } +} From 295d96e044ef60e83e331ee0a6831fb7ca3e74bf Mon Sep 17 00:00:00 2001 From: Arif Hoque Date: Mon, 19 Oct 2026 15:38:50 +0600 Subject: [PATCH 2/3] Fix PostgreSQL temporal lookups reading times in the session timezone --- .../Temporal/InteractsWithTemporal.php | 14 +-- .../Database/Temporal/TemporalBuilder.php | 21 +--- .../Database/Temporal/TemporalManager.php | 38 ++++++ .../TemporalDateTimeNormalizationTest.php | 109 ++++++++++++++++++ 4 files changed, 151 insertions(+), 31 deletions(-) create mode 100644 tests/Model/TemporalBranch/TemporalDateTimeNormalizationTest.php diff --git a/src/Phaseolies/Database/Temporal/InteractsWithTemporal.php b/src/Phaseolies/Database/Temporal/InteractsWithTemporal.php index cedf03bf..29b8bd77 100644 --- a/src/Phaseolies/Database/Temporal/InteractsWithTemporal.php +++ b/src/Phaseolies/Database/Temporal/InteractsWithTemporal.php @@ -543,24 +543,14 @@ private static function snapshotScalar(mixed $value): string } /** - * Expand a partial datetime to the end of the given day, minute or second. + * Normalize a datetime for comparison with `valid_from` * * @param string $datetime * @return string */ private function normalizeTemporalDateTime(string $datetime): string { - $datetime = trim($datetime); - if (preg_match('/^\d{4}-\d{2}-\d{2}$/', $datetime)) { - return $datetime . ' 23:59:59.999999'; - } - if (preg_match('/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}$/', $datetime)) { - return $datetime . ':59.999999'; - } - if (preg_match('/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/', $datetime)) { - return $datetime . '.999999'; - } - return $datetime; + return TemporalManager::normalizeDateTime($datetime, $this->getConnection()->getAttribute(PDO::ATTR_DRIVER_NAME)); } /** diff --git a/src/Phaseolies/Database/Temporal/TemporalBuilder.php b/src/Phaseolies/Database/Temporal/TemporalBuilder.php index 6e9ef960..ba6a50a2 100644 --- a/src/Phaseolies/Database/Temporal/TemporalBuilder.php +++ b/src/Phaseolies/Database/Temporal/TemporalBuilder.php @@ -441,31 +441,14 @@ private function applyPhpLimit(array $models): array } /** - * Normalize various human-friendly datetime strings into a sortable + * Normalize a datetime for comparison with `valid_from` * * @param string $datetime * @return string */ private function normalizeDateTime(string $datetime): string { - $datetime = trim($datetime); - - // Date-only: append end-of-day - if (preg_match('/^\d{4}-\d{2}-\d{2}$/', $datetime)) { - return $datetime . ' 23:59:59.999999'; - } - - // Date + HH:MM only - if (preg_match('/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}$/', $datetime)) { - return $datetime . ':59.999999'; - } - - // Date + HH:MM:SS only - if (preg_match('/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/', $datetime)) { - return $datetime . '.999999'; - } - - return $datetime; + return TemporalManager::normalizeDateTime($datetime, $this->pdo->getAttribute(\PDO::ATTR_DRIVER_NAME)); } /** diff --git a/src/Phaseolies/Database/Temporal/TemporalManager.php b/src/Phaseolies/Database/Temporal/TemporalManager.php index 577b3fa0..8facb5d0 100644 --- a/src/Phaseolies/Database/Temporal/TemporalManager.php +++ b/src/Phaseolies/Database/Temporal/TemporalManager.php @@ -347,6 +347,44 @@ private static function insertHistory(PDO $pdo, string $table, array $data): voi $stmt->execute(array_values($data)); } + /** + * Normalize a user supplied point in time for comparison with `valid_from`. + * + * Partial datetimes are expanded to the end of the given day, minute or + * second. On PostgreSQL `valid_from` is a TIMESTAMPTZ written with an + * explicit offset, so a value without one would be read in the session + * time zone instead of the application's; the application offset is + * appended to keep reads and writes in the same zone. + * + * @param string $datetime + * @param string $driver + * @return string + */ + public static function normalizeDateTime(string $datetime, string $driver): string + { + $datetime = trim($datetime); + + if (preg_match('/^\d{4}-\d{2}-\d{2}$/', $datetime)) { + $datetime .= ' 23:59:59.999999'; + } elseif (preg_match('/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}$/', $datetime)) { + $datetime .= ':59.999999'; + } elseif (preg_match('/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/', $datetime)) { + $datetime .= '.999999'; + } + + $hasOffset = (bool) preg_match('/(Z|[+-]\d{2}(:?\d{2})?)$/i', $datetime); + + if ($driver === 'pgsql' && !$hasOffset) { + try { + return \Carbon\Carbon::parse($datetime)->format('Y-m-d H:i:s.uP'); + } catch (\Throwable) { + return $datetime; + } + } + + return $datetime; + } + /** * Generate a high-resolution timestamp string for the current driver. * diff --git a/tests/Model/TemporalBranch/TemporalDateTimeNormalizationTest.php b/tests/Model/TemporalBranch/TemporalDateTimeNormalizationTest.php new file mode 100644 index 00000000..858bb6f4 --- /dev/null +++ b/tests/Model/TemporalBranch/TemporalDateTimeNormalizationTest.php @@ -0,0 +1,109 @@ +originalTimezone = date_default_timezone_get(); + date_default_timezone_set('UTC'); + } + + protected function tearDown(): void + { + date_default_timezone_set($this->originalTimezone); + } + + /** + * @return array + */ + public static function partialDatetimes(): array + { + return [ + 'date only' => ['2026-02-01', '2026-02-01 23:59:59.999999'], + 'to the minute' => ['2026-02-01 10:30', '2026-02-01 10:30:59.999999'], + 'to the second' => ['2026-02-01 10:30:15', '2026-02-01 10:30:15.999999'], + 'with micros' => ['2026-02-01 10:30:15.123456', '2026-02-01 10:30:15.123456'], + 'surrounding ws' => [' 2026-02-01 ', '2026-02-01 23:59:59.999999'], + ]; + } + + #[DataProvider('partialDatetimes')] + public function testExpandsPartialDatetimesOnDriversWithoutTimezones(string $input, string $expected): void + { + $this->assertSame($expected, TemporalManager::normalizeDateTime($input, 'mysql')); + $this->assertSame($expected, TemporalManager::normalizeDateTime($input, 'sqlite')); + } + + #[DataProvider('partialDatetimes')] + public function testPgsqlAppendsTheApplicationOffset(string $input, string $expected): void + { + $this->assertSame($expected . '+00:00', TemporalManager::normalizeDateTime($input, 'pgsql')); + } + + public function testPgsqlUsesTheApplicationTimezoneNotUtc(): void + { + date_default_timezone_set('Asia/Dhaka'); + Carbon::setTestNow(); + + $this->assertSame( + '2026-02-01 10:30:59.999999+06:00', + TemporalManager::normalizeDateTime('2026-02-01 10:30', 'pgsql') + ); + } + + public function testPgsqlAppliesTheOffsetInEffectOnThatDateAcrossDst(): void + { + date_default_timezone_set('America/New_York'); + + $this->assertSame('2026-01-15 12:00:00.999999-05:00', TemporalManager::normalizeDateTime('2026-01-15 12:00:00', 'pgsql')); + $this->assertSame('2026-07-15 12:00:00.999999-04:00', TemporalManager::normalizeDateTime('2026-07-15 12:00:00', 'pgsql')); + } + + /** + * @return array + */ + public static function explicitOffsets(): array + { + return [ + 'positive' => ['2026-02-01 10:30:00.000000+06:00'], + 'negative' => ['2026-02-01 10:30:00-05:00'], + 'short offset' => ['2026-02-01 10:30:00+06'], + 'compact' => ['2026-02-01 10:30:00+0600'], + 'zulu' => ['2026-02-01T10:30:00Z'], + ]; + } + + #[DataProvider('explicitOffsets')] + public function testExplicitOffsetsAreNeverRewritten(string $input): void + { + date_default_timezone_set('Asia/Dhaka'); + + $this->assertSame($input, TemporalManager::normalizeDateTime($input, 'pgsql')); + } + + public function testNormalizingTwiceIsStable(): void + { + $once = TemporalManager::normalizeDateTime('2026-02-01 10:30', 'pgsql'); + + $this->assertSame($once, TemporalManager::normalizeDateTime($once, 'pgsql')); + } + + public function testUnparseableInputIsReturnedUntouchedForTheDatabaseToReject(): void + { + $this->assertSame('not a date', TemporalManager::normalizeDateTime('not a date', 'pgsql')); + } +} From e0a45ec915e9f9579337926d80349c69a5afeedc Mon Sep 17 00:00:00 2001 From: Arif Hoque Date: Mon, 19 Oct 2026 15:48:42 +0600 Subject: [PATCH 3/3] Escape table name as a string literal in the PostgreSQL column lookup --- src/Phaseolies/Database/Entity/Query/Grammar.php | 6 +++++- tests/Builder/GrammarIdentifierTest.php | 11 +++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/Phaseolies/Database/Entity/Query/Grammar.php b/src/Phaseolies/Database/Entity/Query/Grammar.php index e17cec19..c7733ae0 100644 --- a/src/Phaseolies/Database/Entity/Query/Grammar.php +++ b/src/Phaseolies/Database/Entity/Query/Grammar.php @@ -100,9 +100,13 @@ public function getTableColumnsSql(string $tableName): string { $this->assertValidIdentifier($tableName); + // information_schema compares table_name as a string value, so it is + // escaped as a literal rather than quoted as an identifier + $literal = str_replace("'", "''", $tableName); + return match ($this->getDriver()) { 'sqlite' => "PRAGMA table_info({$this->quoteIdentifier($tableName)})", - 'pgsql' => "SELECT column_name FROM information_schema.columns WHERE table_name = '{$tableName}'", + 'pgsql' => "SELECT column_name FROM information_schema.columns WHERE table_name = '{$literal}'", default => "DESCRIBE {$this->quoteIdentifier($tableName)}", }; } diff --git a/tests/Builder/GrammarIdentifierTest.php b/tests/Builder/GrammarIdentifierTest.php index 5f2bae4b..a09b57e1 100644 --- a/tests/Builder/GrammarIdentifierTest.php +++ b/tests/Builder/GrammarIdentifierTest.php @@ -188,6 +188,17 @@ public function testGetTableColumnsSqlRejectsMaliciousTableNameOnEveryDriver(str } } + public function testPgsqlTableNameIsAnEscapedLiteralNotAnIdentifier(): void + { + $sql = $this->grammarFor('pgsql')->getTableColumnsSql('main.users'); + + $this->assertSame( + "SELECT column_name FROM information_schema.columns WHERE table_name = 'main.users'", + $sql + ); + $this->assertStringNotContainsString('"', $sql); + } + public function testQuoteIdentifierUsesDriverSpecificQuoteCharacter(): void { $this->assertSame('`users`', $this->grammarFor('mysql')->quoteIdentifier('users'));