security(orm): prevent passing callable strings to find()/query()

zenstruck · Jul 14, 2023 · f4b1c48 · f4b1c48
1 parent d0ee92a
commit f4b1c48
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 6 deletions.
diff --git a/src/Collection/Doctrine/ORM/EntityRepository.php b/src/Collection/Doctrine/ORM/EntityRepository.php
@@ -33,7 +33,7 @@ public function __construct(private EntityManagerInterface $em, private string $
     }
 
     /**
-     * @param mixed|Criteria|array<string,mixed>|callable(QueryBuilder):void $specification
+     * @param mixed|Criteria|array<string,mixed>|(object&callable(QueryBuilder):void) $specification
      */
     public function find(mixed $specification): ?object
     {
@@ -46,7 +46,7 @@ public function find(mixed $specification): ?object
                 return $this->em()->getUnitOfWork()->getEntityPersister($this->class)->load($specification, limit: 1); // @phpstan-ignore-line
             }
 
-            if (\is_callable($specification)) {
+            if (\is_callable($specification) && \is_object($specification)) {
                 $specification($qb = $this->qb(), 'e');
 
                 return $qb->getQuery()->getSingleResult();
@@ -59,7 +59,7 @@ public function find(mixed $specification): ?object
     }
 
     /**
-     * @param Criteria|array<string,mixed>|callable(QueryBuilder):void $specification
+     * @param Criteria|array<string,mixed>|(object&callable(QueryBuilder):void) $specification
      *
      * @return EntityResult<V>
      */
@@ -71,7 +71,7 @@ public function query(mixed $specification): EntityResult
             return $qb->addCriteria($specification)->result();
         }
 
-        if (\is_callable($specification)) {
+        if (\is_callable($specification) && \is_object($specification)) {
             $specification($qb, 'e');
 
             return $qb->result();

diff --git a/src/Collection/Doctrine/ORM/EntityRepositoryBridge.php b/src/Collection/Doctrine/ORM/EntityRepositoryBridge.php
@@ -25,7 +25,7 @@ trait EntityRepositoryBridge
     private EntityRepository $collectionRepo;
 
     /**
-     * @param mixed|Criteria|array<string,mixed>|callable(QueryBuilder):void $specification
+     * @param mixed|Criteria|array<string,mixed>|(object&callable(QueryBuilder):void) $specification
      */
     public function find($specification, $lockMode = null, $lockVersion = null): ?object
     {
@@ -38,7 +38,7 @@ public function find($specification, $lockMode = null, $lockVersion = null): ?ob
     }
 
     /**
-     * @param Criteria|array<string,mixed>|callable(QueryBuilder):void $specification
+     * @param Criteria|array<string,mixed>|(object&callable(QueryBuilder):void) $specification
      *
      * @return EntityResult<V>
      */

diff --git a/tests/Doctrine/ORM/EntityRepositoryTest.php b/tests/Doctrine/ORM/EntityRepositoryTest.php
@@ -155,6 +155,29 @@ public function can_filter_with_callable(): void
         $this->assertSame('value 2', \iterator_to_array($objects)[0]->value);
     }
 
+    /**
+     * @test
+     */
+    public function cannot_find_with_callable_strings(): void
+    {
+        $this->assertIsCallable('system');
+        $this->assertNull($this->repo()->find('system'));
+    }
+
+    /**
+     * @test
+     */
+    public function cannot_query_with_callable_strings(): void
+    {
+        $this->assertIsCallable('system');
+
+        $repo = $this->repo();
+
+        $this->expectException(\InvalidArgumentException::class);
+
+        $repo->query('system');
+    }
+
     /**
      * @test
      */