From 9e024ee50432625ee58863307a5e281db5247621 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Sun, 7 Jun 2026 00:02:40 +0200 Subject: [PATCH 1/9] fix: escapeIdentifiers and protectIdentifiers SQL injection bypass Implemented strict regex validation for SQL function calls and string literals in BaseConnection::isSafeToBypassEscape() to prevent SQL injection vulnerabilities. Updated tests to match the new secure behavior and added explicit SQL injection test cases. --- system/Database/BaseConnection.php | 39 ++++++++++-- tests/system/Database/BaseConnectionTest.php | 67 ++++++++++++++++---- 2 files changed, 88 insertions(+), 18 deletions(-) diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index 273e4c60c4d5..d88506041b55 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -1281,8 +1281,9 @@ public function protectIdentifiers($item, bool $prefixSingle = false, ?bool $pro // Added exception for single quotes as well, we don't want to alter // literal strings. if (strcspn($item, "()'") !== strlen($item)) { - /** @psalm-suppress NoValue I don't know why ERROR. */ - return $item; + if ($this->isSafeToBypassEscape($item)) { + return $item; + } } // Do not protect identifiers and do not prefix, no swap prefix, there is nothing to do @@ -1433,6 +1434,34 @@ public function escapeIdentifier($item): string . $this->escapeChar; } + /** + * Checks if an identifier with parentheses or quotes is a safe function call or a string literal. + */ + private function isSafeToBypassEscape(string $item): bool + { + $item = trim($item); + + // String literals starting and ending with single quotes + if ($item[0] === "'" && preg_match('/^\'(?:[^\']|\'\')*\'$/s', $item)) { + return true; + } + + // String literals (for PostgreSQL it can be double quotes) + if ($this->escapeChar !== '"' && $item[0] === '"' && preg_match('/^"(?:[^"]|"")*"$/s', $item)) { + return true; + } + + // SQL functions or subqueries (e.g. MAX(id), (SELECT ...)) with an optional alias + if (str_contains($item, '(')) { + // Regex matching balanced parentheses (from start to end or with a safe alias) + if (preg_match('/^(?:[a-zA-Z0-9_.]+\s*)?(?P\((?:[^()]+|(?&parens))*\))(?:\s+(?:AS\s+)?(?:[a-zA-Z0-9_.]+|"[^"]*"|\'[^\']*\'|`[^`]*`))?$/is', $item)) { + return true; + } + } + + return false; + } + /** * Returns escaped table name with alias. */ @@ -1467,11 +1496,7 @@ public function escapeIdentifiers($item) return $item; } - // Avoid breaking functions and literal values inside queries - if (ctype_digit($item) - || $item[0] === "'" - || ($this->escapeChar !== '"' && $item[0] === '"') - || str_contains($item, '(')) { + if (ctype_digit($item) || $this->isSafeToBypassEscape($item)) { return $item; } diff --git a/tests/system/Database/BaseConnectionTest.php b/tests/system/Database/BaseConnectionTest.php index 4d6d1f76173d..8d416da0c038 100644 --- a/tests/system/Database/BaseConnectionTest.php +++ b/tests/system/Database/BaseConnectionTest.php @@ -367,12 +367,16 @@ public static function provideProtectIdentifiers(): iterable 'table.column' => [false, true, true, 'users.id', '"test_users"."id"'], // Prefixed because it has segments 'table.column prefix' => [true, true, true, 'users.id', '"test_users"."id"'], 'table.column AS' => [ - false, true, true, + false, + true, + true, 'users.id AS user_id', '"test_users"."id" AS "user_id"', // Prefixed because it has segments ], 'table.column AS prefix' => [ - true, true, true, + true, + true, + true, 'users.id AS user_id', '"test_users"."id" AS "user_id"', ], @@ -384,42 +388,78 @@ public static function provideProtectIdentifiers(): iterable 'function column' => [false, true, true, 'SUM(id)', 'SUM(id)'], 'function column AS' => [ - false, true, true, + false, + true, + true, 'COUNT(payments) AS myAlias', 'COUNT(payments) AS myAlias', ], 'function column AS prefix' => [ - true, true, true, + true, + true, + true, 'COUNT(payments) AS myAlias', 'COUNT(payments) AS myAlias', ], 'function quoted table column AS' => [ - false, true, true, + false, + true, + true, 'MAX("db"."payments") AS "payments"', 'MAX("db"."payments") AS "payments"', ], 'quoted column operator AS' => [ - false, true, true, + false, + true, + true, '"numericValue1" + "numericValue2" AS "numericResult"', '"numericValue1"" + ""numericValue2" AS "numericResult"', // Cannot process correctly ], 'quoted column operator AS no-protect' => [ - false, false, true, + false, + false, + true, '"numericValue1" + "numericValue2" AS "numericResult"', '"numericValue1" + "numericValue2" AS "numericResult"', ], 'sub query' => [ - false, true, true, - '(SELECT SUM(payments.amount) FROM payments WHERE payments.invoice_id=4) AS amount_paid)', + false, + true, + true, '(SELECT SUM(payments.amount) FROM payments WHERE payments.invoice_id=4) AS amount_paid)', + '(SELECT SUM(payments.test_amount) FROM payments WHERE payments.invoice_id=4) AS "amount_paid)"', ], 'sub query with missing `)` at the end' => [ - false, true, true, - '(SELECT MAX(advance_amount) FROM "orders" WHERE "id" > 2', + false, + true, + true, '(SELECT MAX(advance_amount) FROM "orders" WHERE "id" > 2', + '"(SELECT MAX(advance_amount) FROM ""orders"" WHERE ""id"" >" 2', + ], + + 'SQLi: Trailing operators after valid function' => [ + false, + true, + true, + 'COUNT(id) OR 1=1', + 'COUNT(id) OR "1=1"', + ], + 'SQLi: Unbalanced parenthesis attack' => [ + false, + true, + true, + 'id) OR (1=1', + '"id) OR" "(1=1"', + ], + 'SQLi: String literal bypass attempt' => [ + false, + true, + true, + "'admin' OR 1=1", + '"\'admin\' OR" "1=1"', ], ]; } @@ -446,6 +486,11 @@ public static function provideEscapeIdentifiers(): iterable // $item, $expected 'simple' => ['test', '"test"'], 'with dots' => ['com.sitedb.web', '"com"."sitedb"."web"'], + + // Security Tests: SQL Injection Bypasses in escapeIdentifiers + 'SQLi: Trailing operators after valid function' => ['COUNT(id) OR 1=1', '"COUNT(id) OR 1=1"'], + 'SQLi: Unbalanced parenthesis attack' => ['id) OR (1=1', '"id) OR (1=1"'], + 'SQLi: String literal bypass attempt' => ["'admin' OR 1=1", '"\'admin\' OR 1=1"'], ]; } From a9793853b4d7accb18d9b275693fe7d045ed9b9d Mon Sep 17 00:00:00 2001 From: Michal Sniatala Date: Sun, 7 Jun 2026 19:27:03 +0200 Subject: [PATCH 2/9] fix: nested transformer request scope leakage (#10281) --- system/API/BaseTransformer.php | 46 +++-- tests/_support/API/ChildTransformer.php | 31 ++++ tests/_support/API/ParentTransformer.php | 67 ++++++++ tests/system/API/TransformerTest.php | 160 ++++++++++++++++++ user_guide_src/source/changelogs/v4.7.4.rst | 1 + .../source/outgoing/api_transformers.rst | 15 ++ 6 files changed, 306 insertions(+), 14 deletions(-) create mode 100644 tests/_support/API/ChildTransformer.php create mode 100644 tests/_support/API/ParentTransformer.php diff --git a/system/API/BaseTransformer.php b/system/API/BaseTransformer.php index 540d443d1cdc..4badb8267139 100644 --- a/system/API/BaseTransformer.php +++ b/system/API/BaseTransformer.php @@ -54,6 +54,11 @@ */ abstract class BaseTransformer implements TransformerInterface { + /** + * Nesting depth of the transformation currently in progress (0 = root). + */ + private static int $depth = 0; + /** * @var list|null */ @@ -69,17 +74,24 @@ abstract class BaseTransformer implements TransformerInterface public function __construct( private ?IncomingRequest $request = null, ) { + // An explicitly provided request is always honored - its scope is + // intentional. Only the implicit global fallback is suppressed for + // nested transformers, which is the actual leak vector. + $explicitRequest = $request instanceof IncomingRequest; + $this->request = $request ?? request(); - $fields = $this->request->getGet('fields'); - $this->fields = is_string($fields) - ? array_map(trim(...), explode(',', $fields)) - : $fields; + if ($explicitRequest || self::$depth === 0) { + $fields = $this->request->getGet('fields'); + $this->fields = is_string($fields) + ? array_map(trim(...), explode(',', $fields)) + : $fields; - $includes = $this->request->getGet('include'); - $this->includes = is_string($includes) - ? array_map(trim(...), explode(',', $includes)) - : $includes; + $includes = $this->request->getGet('include'); + $this->includes = is_string($includes) + ? array_map(trim(...), explode(',', $includes)) + : $includes; + } } /** @@ -207,13 +219,19 @@ private function insertIncludes(array $data): array } } - foreach ($this->includes as $include) { - $method = 'include' . ucfirst($include); - if (method_exists($this, $method)) { - $data[$include] = $this->{$method}(); - } else { - throw ApiException::forMissingInclude($include); + self::$depth++; + + try { + foreach ($this->includes as $include) { + $method = 'include' . ucfirst($include); + if (method_exists($this, $method)) { + $data[$include] = $this->{$method}(); + } else { + throw ApiException::forMissingInclude($include); + } } + } finally { + self::$depth--; } return $data; diff --git a/tests/_support/API/ChildTransformer.php b/tests/_support/API/ChildTransformer.php new file mode 100644 index 000000000000..a226dad97608 --- /dev/null +++ b/tests/_support/API/ChildTransformer.php @@ -0,0 +1,31 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace Tests\Support\API; + +use CodeIgniter\API\BaseTransformer; + +/** + * Nested transformer used to verify that the root request's scope + * (fields/includes) does not leak into related resources. + */ +class ChildTransformer extends BaseTransformer +{ + public function toArray(mixed $resource): array + { + return [ + 'child_id' => $resource['id'] ?? null, + 'status' => 'transformed', + ]; + } +} diff --git a/tests/_support/API/ParentTransformer.php b/tests/_support/API/ParentTransformer.php new file mode 100644 index 000000000000..803b4dbb353f --- /dev/null +++ b/tests/_support/API/ParentTransformer.php @@ -0,0 +1,67 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace Tests\Support\API; + +use CodeIgniter\API\BaseTransformer; + +/** + * Root transformer used to verify that nested transformers do not inherit + * the root request's `fields`/`include` query state. + */ +class ParentTransformer extends BaseTransformer +{ + public function toArray(mixed $resource): array + { + return [ + 'parent_id' => $resource['id'] ?? null, + ]; + } + + /** + * Includes a single related child resource. + * + * @return array + */ + protected function includeChildren(): array + { + return (new ChildTransformer())->transform(['id' => 99]); + } + + /** + * Includes a collection of related child resources. + * + * @return list> + */ + protected function includeChildrenCollection(): array + { + return (new ChildTransformer())->transformMany([ + ['id' => 77], + ['id' => 88], + ]); + } + + /** + * Includes a single child while explicitly forwarding the request, opting + * the child into the request-derived scope even though it is nested. + * + * @return array + */ + protected function includeExplicitChild(): array + { + $childRequest = clone request(); + $childRequest->setGlobal('get', ['fields' => 'child_id']); + + return (new ChildTransformer($childRequest))->transform(['id' => 99]); + } +} diff --git a/tests/system/API/TransformerTest.php b/tests/system/API/TransformerTest.php index 8c6f50857d23..11f37f0fd859 100644 --- a/tests/system/API/TransformerTest.php +++ b/tests/system/API/TransformerTest.php @@ -22,6 +22,8 @@ use Config\Services; use PHPUnit\Framework\Attributes\Group; use stdClass; +use Tests\Support\API\ChildTransformer; +use Tests\Support\API\ParentTransformer; /** * @internal @@ -33,12 +35,14 @@ protected function setUp(): void { parent::setUp(); + Services::resetSingle('request'); Services::superglobals()->setGetArray([]); } protected function tearDown(): void { Services::superglobals()->setGetArray([]); + Services::resetSingle('request'); parent::tearDown(); } @@ -641,4 +645,160 @@ protected function includePosts(): array $this->assertArrayHasKey('posts', $result); $this->assertSame([['id' => 1, 'title' => 'Post 1']], $result['posts']); } + + public function testNestedTransformerDoesNotInheritIncludeState(): void + { + // The child transformer has no includeChildren() method. If the root + // request's `include=children` leaked into it, transforming the child + // would raise an ApiException for the missing include method. + $request = $this->createMockRequest('include=children'); + Services::injectMock('request', $request); + + $transformer = new ParentTransformer($request); + + $result = $transformer->transform(['id' => 1]); + + $this->assertSame([ + 'parent_id' => 1, + 'children' => ['child_id' => 99, 'status' => 'transformed'], + ], $result); + } + + public function testNestedTransformerDoesNotInheritFieldFilter(): void + { + // `fields=parent_id` applies to the root only. If it leaked into the + // child, array_intersect_key would strip every child field, leaving []. + $request = $this->createMockRequest('include=children&fields=parent_id'); + Services::injectMock('request', $request); + + $transformer = new ParentTransformer($request); + + $result = $transformer->transform(['id' => 1]); + + $this->assertSame([ + 'parent_id' => 1, + 'children' => ['child_id' => 99, 'status' => 'transformed'], + ], $result); + } + + public function testNestedCollectionTransformerDoesNotInheritState(): void + { + $request = $this->createMockRequest('include=childrenCollection&fields=parent_id'); + Services::injectMock('request', $request); + + $transformer = new ParentTransformer($request); + + $result = $transformer->transform(['id' => 1]); + + $this->assertSame([ + 'parent_id' => 1, + 'childrenCollection' => [ + ['child_id' => 77, 'status' => 'transformed'], + ['child_id' => 88, 'status' => 'transformed'], + ], + ], $result); + } + + public function testBareNestedInstantiationDoesNotInheritState(): void + { + // Reproduces the exact leak vector: a child created with a bare + // `new ChildTransformer()` (no request passed) inside an include + // method must not pick up the root request's scope from request(). + $request = $this->createMockRequest('include=children&fields=parent_id'); + Services::injectMock('request', $request); + + $root = new ParentTransformer(); + + $result = $root->transform(['id' => 5]); + + $this->assertSame([ + 'parent_id' => 5, + 'children' => ['child_id' => 99, 'status' => 'transformed'], + ], $result); + } + + public function testNestedTransformerHonorsExplicitRequest(): void + { + // A child created with an explicitly passed request must honor that + // request's scope even while nested - the isolation only suppresses + // the implicit global fallback, not deliberate developer intent. + $request = $this->createMockRequest('include=explicitChild&fields=child_id,parent_id'); + Services::injectMock('request', $request); + + $transformer = new ParentTransformer($request); + + $result = $transformer->transform(['id' => 1]); + + $this->assertSame([ + 'parent_id' => 1, + 'explicitChild' => ['child_id' => 99], + ], $result); + } + + public function testRootScopeStillAppliesAfterNesting(): void + { + // Sanity check that the root transformer keeps applying its own scope + // while nested children are isolated. + $request = $this->createMockRequest('include=children&fields=parent_id'); + Services::injectMock('request', $request); + + $transformer = new ParentTransformer($request); + + $result = $transformer->transform(['id' => 1, 'secret' => 'hidden']); + + // Root keeps only parent_id (plus the include key), the child is intact. + $this->assertArrayHasKey('parent_id', $result); + $this->assertArrayNotHasKey('secret', $result); + $this->assertSame(['child_id' => 99, 'status' => 'transformed'], $result['children']); + } + + public function testDepthIsRestoredAfterIncludeThrows(): void + { + $request = $this->createMockRequest('include=nonexistent'); + Services::injectMock('request', $request); + + $throwingRoot = new class ($request) extends BaseTransformer { + public function toArray(mixed $resource): array + { + return $resource; + } + }; + + try { + $throwingRoot->transform(['id' => 1, 'name' => 'Test']); + $this->fail('Expected ApiException was not thrown.'); + } catch (ApiException) { + // expected + } + + // The nesting depth must be balanced after the exception, so a fresh + // root transformer still applies the request scope (depth back to 0). + $fieldsRequest = $this->createMockRequest('fields=id'); + Services::injectMock('request', $fieldsRequest); + + $nextRoot = new class ($fieldsRequest) extends BaseTransformer { + public function toArray(mixed $resource): array + { + return $resource; + } + }; + + $result = $nextRoot->transform(['id' => 1, 'name' => 'Test']); + + $this->assertSame(['id' => 1], $result); + } + + public function testBareNestedTransformerStillUsedByChildTransformerDirectly(): void + { + // When ChildTransformer is itself the root (no parent), it must apply + // request scope as usual - the isolation only affects nesting. + $request = $this->createMockRequest('fields=child_id'); + Services::injectMock('request', $request); + + $transformer = new ChildTransformer($request); + + $result = $transformer->transform(['id' => 42]); + + $this->assertSame(['child_id' => 42], $result); + } } diff --git a/user_guide_src/source/changelogs/v4.7.4.rst b/user_guide_src/source/changelogs/v4.7.4.rst index daf64bbda631..7a38176717c0 100644 --- a/user_guide_src/source/changelogs/v4.7.4.rst +++ b/user_guide_src/source/changelogs/v4.7.4.rst @@ -30,6 +30,7 @@ Deprecations Bugs Fixed ********** +- **API:** Fixed a bug in Transformers where the root request's ``fields`` and ``include`` query parameters leaked into nested transformers created inside ``include*()`` methods, causing incorrect field filtering, unexpected includes, or infinite recursion. - **Database:** Fixed a bug where ``updateBatch()`` could be called after Query Builder ``where()`` conditions, even though it's not supported. In this situation, now the ``DatabaseException`` is thrown. - **HTTP:** Fixed a bug where the User Agent library reported Safari's WebKit version instead of the browser version from the ``Version`` token. diff --git a/user_guide_src/source/outgoing/api_transformers.rst b/user_guide_src/source/outgoing/api_transformers.rst index 5e2ab90ca5cb..38ce1de1fc14 100644 --- a/user_guide_src/source/outgoing/api_transformers.rst +++ b/user_guide_src/source/outgoing/api_transformers.rst @@ -174,6 +174,14 @@ The response would include: ] } +.. note:: The ``fields`` and ``include`` query parameters describe the **root** resource only. + Transformers instantiated inside an ``include*()`` method (such as ``PostTransformer`` above) + are treated as nested resources: when created **without an explicit request** they do **not** + inherit the root request's ``fields``/``include`` state. This prevents the parent's query + parameters from leaking into related resources, which could otherwise cause incorrect field + filtering or unexpected/recursive includes. If you deliberately pass a request to a nested + transformer, that request's scope is honored. + Restricting Available Includes =============================== @@ -262,6 +270,13 @@ Class Reference Initializes the transformer and extracts the ``fields`` and ``include`` query parameters from the request. + .. note:: When no request is explicitly passed, the global request's ``fields`` and ``include`` + are read **only for the root transformer**. A transformer constructed during a nested + transformation (i.e. inside an ``include*()`` method) without an explicit request still uses + the global request object, but does not read ``fields``/``include`` from it, to avoid leaking + the root's scope. Passing an explicit request always applies that request's scope, regardless + of nesting. + .. php:method:: toArray(mixed $resource) :param mixed $resource: The resource being transformed (Entity, array, object, or null) From 1bc3a948f3ece3be20eb7bbc322380d76d98df78 Mon Sep 17 00:00:00 2001 From: Bogdan Lambarski Date: Sun, 7 Jun 2026 19:48:32 +0200 Subject: [PATCH 3/9] Update system/Database/BaseConnection.php Co-authored-by: Michal Sniatala --- system/Database/BaseConnection.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index d88506041b55..125ff227b292 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -1440,6 +1440,10 @@ public function escapeIdentifier($item): string private function isSafeToBypassEscape(string $item): bool { $item = trim($item); + + if ($item === '') { + return false; + } // String literals starting and ending with single quotes if ($item[0] === "'" && preg_match('/^\'(?:[^\']|\'\')*\'$/s', $item)) { From ae4e6ea7d26b5c83e5bb614b729adc5564c3bac8 Mon Sep 17 00:00:00 2001 From: Bogdan Lambarski Date: Sun, 7 Jun 2026 19:48:44 +0200 Subject: [PATCH 4/9] Update system/Database/BaseConnection.php Co-authored-by: Michal Sniatala --- system/Database/BaseConnection.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index 125ff227b292..a42450dc76c7 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -1437,7 +1437,7 @@ public function escapeIdentifier($item): string /** * Checks if an identifier with parentheses or quotes is a safe function call or a string literal. */ - private function isSafeToBypassEscape(string $item): bool + private function isIdentifierEscapeExempt()(string $item): bool { $item = trim($item); From 9353fe82df8e0cec23c91e5b02495a786387c553 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Mon, 8 Jun 2026 23:16:46 +0200 Subject: [PATCH 5/9] fix: resolve syntax error and method names for escapeIdentifiers bypass --- system/Database/BaseConnection.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index a42450dc76c7..b5ef6d88b94d 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -1281,7 +1281,7 @@ public function protectIdentifiers($item, bool $prefixSingle = false, ?bool $pro // Added exception for single quotes as well, we don't want to alter // literal strings. if (strcspn($item, "()'") !== strlen($item)) { - if ($this->isSafeToBypassEscape($item)) { + if ($this->isIdentifierEscapeExempt($item)) { return $item; } } @@ -1437,7 +1437,7 @@ public function escapeIdentifier($item): string /** * Checks if an identifier with parentheses or quotes is a safe function call or a string literal. */ - private function isIdentifierEscapeExempt()(string $item): bool + private function isIdentifierEscapeExempt(string $item): bool { $item = trim($item); @@ -1500,7 +1500,7 @@ public function escapeIdentifiers($item) return $item; } - if (ctype_digit($item) || $this->isSafeToBypassEscape($item)) { + if (ctype_digit($item) || $this->isIdentifierEscapeExempt($item)) { return $item; } From a3df98fe423c53bffa2f5601e81e5223ffbdd05e Mon Sep 17 00:00:00 2001 From: Bogdan Date: Mon, 8 Jun 2026 23:20:53 +0200 Subject: [PATCH 6/9] fix: require AS keyword for function/subquery aliases in isIdentifierEscapeExempt to prevent SQL injection bypass --- system/Database/BaseConnection.php | 4 ++-- tests/system/Database/BaseConnectionTest.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index b5ef6d88b94d..9aee6fc160f3 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -1440,7 +1440,7 @@ public function escapeIdentifier($item): string private function isIdentifierEscapeExempt(string $item): bool { $item = trim($item); - + if ($item === '') { return false; } @@ -1458,7 +1458,7 @@ private function isIdentifierEscapeExempt(string $item): bool // SQL functions or subqueries (e.g. MAX(id), (SELECT ...)) with an optional alias if (str_contains($item, '(')) { // Regex matching balanced parentheses (from start to end or with a safe alias) - if (preg_match('/^(?:[a-zA-Z0-9_.]+\s*)?(?P\((?:[^()]+|(?&parens))*\))(?:\s+(?:AS\s+)?(?:[a-zA-Z0-9_.]+|"[^"]*"|\'[^\']*\'|`[^`]*`))?$/is', $item)) { + if (preg_match('/^(?:[a-zA-Z0-9_.]+\s*)?(?P\((?:[^()]+|(?&parens))*\))(?:\s+AS\s+(?:[a-zA-Z0-9_.]+|"[^"]*"|\'[^\']*\'|`[^`]*`))?$/is', $item)) { return true; } } diff --git a/tests/system/Database/BaseConnectionTest.php b/tests/system/Database/BaseConnectionTest.php index 8d416da0c038..eb6e342395f6 100644 --- a/tests/system/Database/BaseConnectionTest.php +++ b/tests/system/Database/BaseConnectionTest.php @@ -445,7 +445,7 @@ public static function provideProtectIdentifiers(): iterable true, true, 'COUNT(id) OR 1=1', - 'COUNT(id) OR "1=1"', + '"COUNT(id) OR" "1=1"', ], 'SQLi: Unbalanced parenthesis attack' => [ false, From b11412d4884b682134f8164472779b95e5c2b61f Mon Sep 17 00:00:00 2001 From: Bogdan Date: Mon, 8 Jun 2026 23:33:42 +0200 Subject: [PATCH 7/9] refactor: simplify conditions in BaseConnection using Rector --- system/Database/BaseConnection.php | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index 9aee6fc160f3..d773d7a2d123 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -1280,10 +1280,8 @@ public function protectIdentifiers($item, bool $prefixSingle = false, ?bool $pro // // Added exception for single quotes as well, we don't want to alter // literal strings. - if (strcspn($item, "()'") !== strlen($item)) { - if ($this->isIdentifierEscapeExempt($item)) { - return $item; - } + if (strcspn($item, "()'") !== strlen($item) && $this->isIdentifierEscapeExempt($item)) { + return $item; } // Do not protect identifiers and do not prefix, no swap prefix, there is nothing to do @@ -1456,14 +1454,8 @@ private function isIdentifierEscapeExempt(string $item): bool } // SQL functions or subqueries (e.g. MAX(id), (SELECT ...)) with an optional alias - if (str_contains($item, '(')) { - // Regex matching balanced parentheses (from start to end or with a safe alias) - if (preg_match('/^(?:[a-zA-Z0-9_.]+\s*)?(?P\((?:[^()]+|(?&parens))*\))(?:\s+AS\s+(?:[a-zA-Z0-9_.]+|"[^"]*"|\'[^\']*\'|`[^`]*`))?$/is', $item)) { - return true; - } - } - - return false; + // Regex matching balanced parentheses (from start to end or with a safe alias) + return str_contains($item, '(') && preg_match('/^(?:[a-zA-Z0-9_.]+\s*)?(?P\((?:[^()]+|(?&parens))*\))(?:\s+AS\s+(?:[a-zA-Z0-9_.]+|"[^"]*"|\'[^\']*\'|`[^`]*`))?$/is', $item); } /** From 1a9882aba69c21f5d100afeadfd7c59db0559666 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Mon, 8 Jun 2026 23:43:16 +0200 Subject: [PATCH 8/9] fix: resolve subquery bare alias escape bypass regression and restore test expectations --- system/Database/BaseConnection.php | 4 ++++ tests/system/Database/BaseConnectionTest.php | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index d773d7a2d123..c6a49bb12788 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -1306,6 +1306,10 @@ public function protectIdentifiers($item, bool $prefixSingle = false, ?bool $pro $alias = ''; } + if ($alias !== '' && strcspn($item, "()'") !== strlen($item) && $this->isIdentifierEscapeExempt($item)) { + return $item . $alias; + } + // Break the string apart if it contains periods, then insert the table prefix // in the correct location, assuming the period doesn't indicate that we're dealing // with an alias. While we're at it, we will escape the components diff --git a/tests/system/Database/BaseConnectionTest.php b/tests/system/Database/BaseConnectionTest.php index eb6e342395f6..7115e7d16abe 100644 --- a/tests/system/Database/BaseConnectionTest.php +++ b/tests/system/Database/BaseConnectionTest.php @@ -430,7 +430,7 @@ public static function provideProtectIdentifiers(): iterable true, true, '(SELECT SUM(payments.amount) FROM payments WHERE payments.invoice_id=4) AS amount_paid)', - '(SELECT SUM(payments.test_amount) FROM payments WHERE payments.invoice_id=4) AS "amount_paid)"', + '(SELECT SUM(payments.amount) FROM payments WHERE payments.invoice_id=4) AS "amount_paid)"', ], 'sub query with missing `)` at the end' => [ false, From 00ef2a62a358abbbb966d541310683c540c17d25 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Mon, 8 Jun 2026 23:56:47 +0200 Subject: [PATCH 9/9] fix: resolve PostgreSQL select function with comma escape protection regression --- system/Database/BaseBuilder.php | 2 +- system/Database/BaseConnection.php | 3 +++ tests/system/Database/BaseConnectionTest.php | 8 ++++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/system/Database/BaseBuilder.php b/system/Database/BaseBuilder.php index 0a27b0b33abc..0b4e341ba5f6 100644 --- a/system/Database/BaseBuilder.php +++ b/system/Database/BaseBuilder.php @@ -410,7 +410,7 @@ public function select($select = '*', ?bool $escape = null) } if (is_string($select)) { - $select = ($escape === false) ? [$select] : explode(',', $select); + $select = ($escape === false) ? [$select] : preg_split('/,(?![^(]*\))/', $select); } foreach ($select as $val) { diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index c6a49bb12788..85d6cc0ba444 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -1280,6 +1280,7 @@ public function protectIdentifiers($item, bool $prefixSingle = false, ?bool $pro // // Added exception for single quotes as well, we don't want to alter // literal strings. + /** @psalm-suppress NoValue */ if (strcspn($item, "()'") !== strlen($item) && $this->isIdentifierEscapeExempt($item)) { return $item; } @@ -1306,6 +1307,7 @@ public function protectIdentifiers($item, bool $prefixSingle = false, ?bool $pro $alias = ''; } + /** @psalm-suppress NoValue */ if ($alias !== '' && strcspn($item, "()'") !== strlen($item) && $this->isIdentifierEscapeExempt($item)) { return $item . $alias; } @@ -1496,6 +1498,7 @@ public function escapeIdentifiers($item) return $item; } + /** @psalm-suppress NoValue */ if (ctype_digit($item) || $this->isIdentifierEscapeExempt($item)) { return $item; } diff --git a/tests/system/Database/BaseConnectionTest.php b/tests/system/Database/BaseConnectionTest.php index 7115e7d16abe..bda195c8616f 100644 --- a/tests/system/Database/BaseConnectionTest.php +++ b/tests/system/Database/BaseConnectionTest.php @@ -410,6 +410,14 @@ public static function provideProtectIdentifiers(): iterable 'MAX("db"."payments") AS "payments"', ], + 'function with multiple arguments AS' => [ + false, + true, + true, + "CONCAT(first_name, ' ', last_name) AS name", + "CONCAT(first_name, ' ', last_name) AS name", + ], + 'quoted column operator AS' => [ false, true,