Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion system/Database/BaseConnection.php
Original file line number Diff line number Diff line change
Expand Up @@ -1338,7 +1338,7 @@ public function protectIdentifiers($item, bool $prefixSingle = false, ?bool $pro

private function protectDotItem(string $item, string $alias, bool $protectIdentifiers, bool $fieldExists): string
{
$parts = explode('.', $item);
$parts = array_map(fn (string $part): string => trim($part, $this->escapeChar), explode('.', $item));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With QUOTED_IDENTIFIER OFF, the SQLSRV driver sets $this->escapeChar to ['[', ']']. Passing this array as the second argument to trim() causes a TypeError when processing dotted identifiers. I verified that protectIdentifiers('jobs.id') returned [jobs].[id] before this change and now throws. We should handle the array form of escapeChar and add a regression test.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case also failed before the PR, but it appears to be a missing case within the intended fix. With an empty DBPrefix, the normalized $parts are not joined back into $item, so getFieldData('"public"."jobs"') still passes the quotes into the metadata query and returns no columns. Since Query Builder passes protected table names, could we cover this path too and add a test for a quoted, schema-qualified table name with an empty prefix?


// Does the first segment of the exploded item match
// one of the aliases previously identified? If so,
Expand Down
13 changes: 11 additions & 2 deletions system/Database/Postgre/Connection.php
Original file line number Diff line number Diff line change
Expand Up @@ -325,11 +325,20 @@ protected function _listColumns($table = ''): string
*/
protected function _fieldData(string $table): array
{
$parts = explode('.', $table);
$table = array_pop($parts);
$schema = array_pop($parts);

$sql = 'SELECT "column_name", "data_type", "character_maximum_length", "numeric_precision", "column_default", "is_nullable"
FROM "information_schema"."columns"
WHERE LOWER("table_name") = '
. $this->escape(strtolower($table))
. ' ORDER BY "ordinal_position"';
. $this->escape(strtolower($table));

if ($schema !== null) {
$sql .= ' AND LOWER("table_schema") = ' . $this->escape(strtolower($schema));
}

$sql .= ' ORDER BY "ordinal_position"';

if (($query = $this->query($sql)) === false) {
throw new DatabaseException(lang('Database.failGetFieldData'));
Expand Down
8 changes: 8 additions & 0 deletions system/Database/SQLSRV/Connection.php
Original file line number Diff line number Diff line change
Expand Up @@ -314,12 +314,20 @@ protected function _enableForeignKeyChecks()
*/
protected function _fieldData(string $table): array
{
$parts = explode('.', $table);
$table = array_pop($parts);
$schema = array_pop($parts);

$sql = 'SELECT
COLUMN_NAME, DATA_TYPE, CHARACTER_MAXIMUM_LENGTH, NUMERIC_PRECISION,
COLUMN_DEFAULT, IS_NULLABLE
FROM INFORMATION_SCHEMA.COLUMNS
WHERE TABLE_NAME= ' . $this->escape(($table));

if ($schema !== null) {
$sql .= ' AND TABLE_SCHEMA = ' . $this->escape($schema);
}

if (($query = $this->query($sql)) === false) {
throw new DatabaseException(lang('Database.failGetFieldData'));
}
Expand Down
7 changes: 6 additions & 1 deletion system/Database/SQLite3/Connection.php
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,12 @@ public function getFieldNames($tableName)
*/
protected function _fieldData(string $table): array
{
if (false === $query = $this->query('PRAGMA TABLE_INFO(' . $this->protectIdentifiers($table, true, null, false) . ')')) {
$parts = explode('.', $table);
$table = array_pop($parts);
$schema = array_pop($parts);
$pragma = ($schema === null ? '' : $this->escapeIdentifiers($schema) . '.') . 'TABLE_INFO(' . $this->protectIdentifiers($table, true, null, false) . ')';

if (false === $query = $this->query('PRAGMA ' . $pragma)) {
throw new DatabaseException(lang('Database.failGetFieldData'));
}

Expand Down
5 changes: 5 additions & 0 deletions tests/system/Database/BaseConnectionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,11 @@ public static function provideProtectIdentifiers(): iterable
'quoted table alias' => [false, true, false, '"jobs" "j"', '"jobs" "j"'],
'quoted table alias prefix' => [true, true, false, '"jobs" "j"', '"test_jobs" "j"'],

'schema.table' => [false, true, false, 'tenant.jobs', '"tenant"."test_jobs"'],
'schema.table prefix' => [true, true, false, 'tenant.jobs', '"tenant"."test_jobs"'],
'quoted schema.table prefix' => [true, true, false, '"tenant"."test_jobs"', '"tenant"."test_jobs"'],
'quoted schema.table no-protect' => [true, false, false, '"tenant"."test_jobs"', 'tenant.test_jobs'],

'table.*' => [false, true, true, 'jobs.*', '"test_jobs".*'], // Prefixed because it has segments
'table.* prefix' => [true, true, true, 'jobs.*', '"test_jobs".*'],
'table.column' => [false, true, true, 'users.id', '"test_users"."id"'], // Prefixed because it has segments
Expand Down
24 changes: 24 additions & 0 deletions tests/system/Database/Live/AbstractGetFieldDataTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,30 @@ protected function tearDown(): void
$this->forge->dropTable($this->table, true);
}

protected function qualifiedTableName(): string
{
return match ($this->db->DBDriver) {
'MySQLi' => $this->db->getDatabase() . '.' . $this->table,
'OCI8' => $this->db->username . '.' . $this->table,
'Postgre' => 'public.' . $this->table,
'SQLite3' => 'main.' . $this->table,
'SQLSRV' => 'dbo.' . $this->table,
default => $this->markTestSkipped('No qualified table name is defined for ' . $this->db->DBDriver . '.'),
};
}

public function testGetFieldDataWithQualifiedTableName(): void
{
$this->createTableForDefault();

$toArray = static fn (stdClass $field): array => (array) $field;

$this->assertSame(
array_map($toArray, $this->db->getFieldData($this->table)),
array_map($toArray, $this->db->getFieldData($this->qualifiedTableName())),
);
}

protected function createTableForDefault(): void
{
$this->forge->dropTable($this->table, true);
Expand Down
1 change: 1 addition & 0 deletions user_guide_src/source/changelogs/v4.7.5.rst
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ Bugs Fixed
- **Cookie:** Fixed a bug where ``Cookie`` instances allowed invalid characters in path, domain, and prefix attributes rejected by ``setcookie()`` and ``setrawcookie()``.
- **Database:** Fixed a bug where rebuilding a SQLite3 table (e.g., ``Forge::dropColumn()``, ``Forge::modifyColumn()``, ``Forge::dropForeignKey()`` and ``Forge::dropPrimaryKey()``) corrupted the table names referenced by its foreign keys when ``DBPrefix`` was set.
- **Database:** Fixed a bug where Postgre query failures were silently ignored when ``DBDebug`` was enabled and PHP warnings were disabled. A ``DatabaseException`` is now thrown.
- **Database:** Fixed a bug where ``getFieldData()`` failed for a schema-qualified table name on Postgre, SQLSRV and SQLite3, and where ``protectIdentifiers()`` prefixed an already protected segment of a dotted identifier a second time.
- **Debug:** Fixed a bug where ``Timer::start()`` treated ``0.0`` as an empty value and substituted the current time.
- **Files:** Fixed a bug where ``File::move()`` and ``UploadedFile::move()`` set executable and overly permissive file permissions (``0777 & ~umask()`` instead of ``0666 & ~umask()``), and ``UploadedFile::move()`` targeted the parent directory instead of the destination file for ``chmod()``.
- **Helpers:** Fixed a bug where ``get_dir_file_info()`` returned incomplete entries for subdirectories and missing files instead of omitting them.
Expand Down
Loading