diff --git a/src/ConnectionInternals.php b/src/ConnectionInternals.php index d7c44dd..d1c7816 100644 --- a/src/ConnectionInternals.php +++ b/src/ConnectionInternals.php @@ -348,15 +348,48 @@ private function assertSafeTemplate(string $sql): void /** * Reject LIMIT/OFFSET - these methods add their own LIMIT internally. + * + * Only a top-level clause counts: a column named `offset`, a LIMIT inside a subquery + * or a keyword in a comment are all fine (see topLevelSql()). The raw match runs + * first so the masking only happens when a keyword is present at all. + * * @throws InvalidArgumentException */ private function rejectLimitAndOffset(int|array|string $where): void { - if (is_string($where) && preg_match('/\b(LIMIT|OFFSET)\b/i', $where)) { + $regex = '/\b(LIMIT|OFFSET)\b/i'; + if (is_string($where) && preg_match($regex, $where) && preg_match($regex, $this->topLevelSql($where))) { throw new InvalidArgumentException("This method doesn't support LIMIT or OFFSET"); } } + /** + * Return the SQL with everything below the top level blanked out, for keyword guards. + * + * Quoted text, backtick-quoted identifiers, comments and parenthesised groups + * (subqueries, function calls, IN lists) are each replaced by a single space, so a + * keyword guard like /\bLIMIT\b/ can only match a real top-level clause: + * + * `offset` = ? AND note = 'no limit' -- see LIMIT => = ? AND note = + * id IN (SELECT id FROM t LIMIT 5) ORDER BY id => id IN ORDER BY id + * id IN (SELECT id FROM t) LIMIT 5 => id IN LIMIT 5 (still rejected) + * + * The first pass handles quotes, identifiers and comments together, so whichever + * opens first consumes the rest (a quote inside a comment, a # inside a literal). + * Quotes are rejected later by assertSafeTemplate() anyway; masking them here means + * that clearer error is the one the caller sees. An unterminated block comment runs + * to the end, as in replacePlaceholders(). Parentheses go in one recursive pass so + * nested groups are handled; an unbalanced group is left as is. The result is only + * ever matched against, never executed. + */ + private function topLevelSql(string $sql): string + { + $sql = preg_replace(<<<'REGEX' + ~'(?:[^'\\]|\\.|'')*'|"(?:[^"\\]|\\.|"")*"|`[^`]*`|/\*.*?(?:\*/|$)|(?:--(?=\s|$)|#)[^\r\n]*~s + REGEX, ' ', $sql); + return preg_replace('/\((?:[^()]++|(?R))*+\)/', ' ', $sql); + } + /** * Reject template patterns that conflict with the auto-appended `LIMIT 1`. * @@ -388,8 +421,10 @@ private function rejectPreLimitConflicts(int|array|string $where): void return; } - // Row-locking clauses - grammar requires LIMIT before these - if (preg_match('/\bFOR\s+(?:UPDATE|SHARE)\b|\bLOCK\s+IN\s+SHARE\s+MODE\b/i', $where, $m)) { + // Row-locking clauses - grammar requires LIMIT before these. Matched on topLevelSql() + // so a subquery or comment can't trigger it; the trailing checks below must see the + // raw template. + if (preg_match('/\bFOR\s+(?:UPDATE|SHARE)\b|\bLOCK\s+IN\s+SHARE\s+MODE\b/i', $this->topLevelSql($where), $m)) { $clause = preg_replace('/\s+/', ' ', strtoupper($m[0])); throw new InvalidArgumentException("This method doesn't support $clause. Use query(...)->first() instead."); } diff --git a/tests/DB/LimitOffsetGuardTest.php b/tests/DB/LimitOffsetGuardTest.php new file mode 100644 index 0000000..772f4fc --- /dev/null +++ b/tests/DB/LimitOffsetGuardTest.php @@ -0,0 +1,144 @@ +query("DROP TEMPORARY TABLE IF EXISTS test_keywords"); + DB::$mysqli->query("CREATE TEMPORARY TABLE test_keywords (id INT PRIMARY KEY, `offset` INT, `limit` INT)"); + DB::$mysqli->query("INSERT INTO test_keywords (id, `offset`, `limit`) VALUES (1, 0, 10), (2, 5, 20), (3, 9, 30)"); + } + + public static function tearDownAfterClass(): void + { + DB::$mysqli->query("DROP TEMPORARY TABLE IF EXISTS test_keywords"); + } + + //region keyword below the top level is allowed + + public function testSelectOneAllowsColumnNamedOffset(): void + { + $row = DB::selectOne('keywords', "`offset` = ?", 5); + $this->assertSame(['id' => 2, 'offset' => 5, 'limit' => 20], $row->toArray()); + } + + public function testCountAllowsColumnNamedOffset(): void + { + $this->assertSame(2, DB::count('keywords', "`offset` > ?", 0)); + } + + public function testSelectOneAllowsColumnNamedLimitInOrderBy(): void + { + $row = DB::selectOne('keywords', "id > ? ORDER BY `limit` DESC", 0); + $this->assertSame(3, $row->toArray()['id']); + } + + public function testQueryOneAllowsAliasNamedOffset(): void + { + $row = DB::queryOne("SELECT num AS `offset` FROM ::users WHERE num = ?", 1); + $this->assertSame(['offset' => 1], $row->toArray()); + } + + public function testQueryOneAllowsLimitInsideDerivedTable(): void + { + // the LIMIT belongs to the derived table; the outer query has none, so the appended LIMIT 1 is valid + $row = DB::queryOne("SELECT * FROM (SELECT num, status FROM ::users ORDER BY num DESC LIMIT ?) AS t WHERE t.status = ? ORDER BY t.num DESC", 5, 'Active'); + $this->assertSame(['num' => 19, 'status' => 'Active'], $row->toArray()); + } + + public function testSelectOneAllowsLimitInsideNestedSubquery(): void + { + // inner table is test_products: a regular table, so it can be reopened inside a derived table + $row = DB::selectOne('keywords', "id IN (SELECT id FROM (SELECT product_id AS id FROM ::products ORDER BY product_id LIMIT ?) AS x) ORDER BY id DESC", 2); + $this->assertSame(['id' => 2, 'offset' => 5, 'limit' => 20], $row->toArray()); + } + + public function testCountAllowsKeywordInsideBlockComment(): void + { + $this->assertSame(10, DB::count('users', "status = ? /* no LIMIT here */", 'Active')); + } + + public function testCountAllowsKeywordInsideTrailingLineComment(): void + { + // count() appends nothing, so a trailing line comment is harmless here + $this->assertSame(10, DB::count('users', "status = ? -- LIMIT", 'Active')); + } + + public function testQueryOneAllowsLockingKeywordInsideComment(): void + { + $row = DB::queryOne("SELECT num FROM ::users WHERE num = ? /* FOR UPDATE */", 1); + $this->assertSame(['num' => 1], $row->toArray()); + } + + //endregion + //region keyword at the top level is still rejected + + /** + * @dataProvider provideTopLevelLimitOrOffset + */ + public function testTopLevelLimitOrOffsetIsStillRejected(Closure $call): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage("This method doesn't support LIMIT or OFFSET"); + $call(); + } + + public static function provideTopLevelLimitOrOffset(): array + { + return [ + 'selectOne: LIMIT after a keyword column' => [fn() => DB::selectOne('keywords', "`offset` = ? LIMIT 5", 5)], + 'selectOne: LIMIT after a subquery' => [fn() => DB::selectOne('users', "num IN (SELECT user_id FROM ::orders) LIMIT ?", 5)], + 'queryOne: LIMIT and OFFSET' => [fn() => DB::queryOne("SELECT * FROM ::users ORDER BY num LIMIT 5 OFFSET 1")], + 'count: LIMIT' => [fn() => DB::count('users', "ORDER BY num LIMIT 5")], + ]; + } + + public function testSelectOneStillRejectsTopLevelForUpdateAfterSubquery(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage("doesn't support FOR UPDATE"); + DB::selectOne('users', "num IN (SELECT user_id FROM ::orders) FOR UPDATE"); + } + + //endregion + //region quoted text: the template guard's error wins, not a misleading keyword error + + public function testQuotedKeywordReportsQuotesErrorNotLimitError(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage("Quotes not allowed in template"); + DB::selectOne('users', "name = 'no limit'"); + } + + public function testQuotedLockingKeywordReportsQuotesErrorNotForUpdateError(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage("Quotes not allowed in template"); + DB::selectOne('users', "name = 'for update'"); + } + + //endregion +}