Disallow strings in limit() and offset()

Back in 68a08a2 int casts were removed in limit() and offset(). This opened up the possibility for applications to provide unsafe user input into these methods and create an injection vector. We've been trying to narrow the accepted types, so instead of adding casting I've added an exception when strings that are not number shaped are provided.
cakephp · Jan 3, 2023 · 3f463e7 · 3f463e7
1 parent d229619
commit 3f463e7
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 0 deletions.
diff --git a/src/Database/Query.php b/src/Database/Query.php
@@ -1534,6 +1534,9 @@ public function page(int $num, ?int $limit = null)
      */
     public function limit($limit)
     {
+        if (is_string($limit) && !is_numeric($limit)) {
+            throw new InvalidArgumentException('Invalid value for `limit()`');
+        }
         $this->_dirty();
         $this->_parts['limit'] = $limit;
 
@@ -1560,6 +1563,9 @@ public function limit($limit)
      */
     public function offset($offset)
     {
+        if (is_string($offset) && !is_numeric($offset)) {
+            throw new InvalidArgumentException('Invalid value for `offset()`');
+        }
         $this->_dirty();
         $this->_parts['offset'] = $offset;
 

diff --git a/tests/TestCase/Database/QueryTest.php b/tests/TestCase/Database/QueryTest.php
@@ -2218,6 +2218,32 @@ public function testSelectLimit(): void
         $this->assertCount(2, $result);
     }
 
+    /**
+     * Tests selecting rows with string offset/limit
+     */
+    public function testSelectLimitInvalid(): void
+    {
+        $query = new Query($this->connection);
+        $this->expectException(InvalidArgumentException::class);
+        $query->select('id')->from('comments')
+            ->limit('1 --more')
+            ->order(['id' => 'ASC'])
+            ->execute();
+    }
+
+    /**
+     * Tests selecting rows with string offset/limit
+     */
+    public function testSelectOffsetInvalid(): void
+    {
+        $query = new Query($this->connection);
+        $this->expectException(InvalidArgumentException::class);
+        $query->select('id')->from('comments')
+            ->offset('1 --more')
+            ->order(['id' => 'ASC'])
+            ->execute();
+    }
+
     /**
      * Tests selecting rows combining a limit and offset clause
      */