CVE-2023-37473: Improper Neutralization of Special Elements in Output Used by a Downstream Component ('Injection') (CWE-74)
value: zenstruck/collections is a set of helpers for iterating/paginating/filtering collections. Passing _callable strings_ (ie `system`) caused the function to be executed. This would result in a limited subset of specific user input being executed as if it were code. This issue has been addressed in commit `f4b1c48820` and included in release version 0.2.1. Users are advised to upgrade. Users unable to upgrade should ensure that user input is not passed to either `EntityRepository::find()` or `query()`.
CWE description: The product constructs all or part of a command, data structure, or record using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify how it is parsed or interpreted when it is sent to a downstream component.
CVSS v3 base score: 8.8
Published: 2023-07-14T21:15Z
Repository: https://github.com/zenstruck/collection
Language: PHP
Files changed: src/Collection/Doctrine/ORM/EntityRepository.php, src/Collection/Doctrine/ORM/EntityRepositoryBridge.php, tests/Doctrine/ORM/EntityRepositoryTest.php
Commit date: 2023-07-14 10:21:50 -0400
Commit message: security(orm): prevent passing callable strings to `find()`/`query()`

Diff: diff --git a/src/Collection/Doctrine/ORM/EntityRepository.php b/src/Collection/Doctrine/ORM/EntityRepository.php
index c94bbee..e675446 100644
--- a/src/Collection/Doctrine/ORM/EntityRepository.php
+++ b/src/Collection/Doctrine/ORM/EntityRepository.php
@@ -31,11 +31,11 @@ class EntityRepository implements ObjectRepository
     public function __construct(private EntityManagerInterface $em, private string $class)
     {
     }
 
     /**
-     * @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
     {
         try {
             if ($specification instanceof Criteria) {
@@ -44,11 +44,11 @@ class EntityRepository implements ObjectRepository
 
             if (\is_array($specification) && !array_is_list($specification)) {
                 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();
             }
 
@@ -57,11 +57,11 @@ class EntityRepository implements ObjectRepository
             return null;
         }
     }
 
     /**
-     * @param Criteria|array<string,mixed>|callable(QueryBuilder):void $specification
+     * @param Criteria|array<string,mixed>|(object&callable(QueryBuilder):void) $specification
      *
      * @return EntityResult<V>
      */
     public function query(mixed $specification): EntityResult
     {
@@ -69,11 +69,11 @@ class EntityRepository implements ObjectRepository
 
         if ($specification instanceof Criteria) {
             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
index 66ddc1f..b98a059 100644
--- a/src/Collection/Doctrine/ORM/EntityRepositoryBridge.php
+++ b/src/Collection/Doctrine/ORM/EntityRepositoryBridge.php
@@ -23,11 +23,11 @@ trait EntityRepositoryBridge
 {
     /** @var EntityRepository<V> */
     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
     {
         if ($lockMode || $lockVersion) {
             // @phpstan-ignore-next-line
@@ -36,11 +36,11 @@ trait EntityRepositoryBridge
 
         return $this->collectionRepo()->find($specification);
     }
 
     /**
-     * @param Criteria|array<string,mixed>|callable(QueryBuilder):void $specification
+     * @param Criteria|array<string,mixed>|(object&callable(QueryBuilder):void) $specification
      *
      * @return EntityResult<V>
      */
     public function query(mixed $specification): EntityResult
     {
diff --git a/tests/Doctrine/ORM/EntityRepositoryTest.php b/tests/Doctrine/ORM/EntityRepositoryTest.php
index a2deddd..b478db8 100644
--- a/tests/Doctrine/ORM/EntityRepositoryTest.php
+++ b/tests/Doctrine/ORM/EntityRepositoryTest.php
@@ -153,10 +153,33 @@ class EntityRepositoryTest extends TestCase
 
         $this->assertCount(1, $objects);
         $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
      */
     public function can_filter_for_no_results(): void
     {

Vulnerable code:      * @param mixed|Criteria|array<string,mixed>|callable(QueryBuilder):void $specification
            if (\is_callable($specification)) {
     * @param Criteria|array<string,mixed>|callable(QueryBuilder):void $specification
        if (\is_callable($specification)) {
     * @param mixed|Criteria|array<string,mixed>|callable(QueryBuilder):void $specification
     * @param Criteria|array<string,mixed>|callable(QueryBuilder):void $specification
Fixed code:      * @param mixed|Criteria|array<string,mixed>|(object&callable(QueryBuilder):void) $specification
            if (\is_callable($specification) && \is_object($specification)) {
     * @param Criteria|array<string,mixed>|(object&callable(QueryBuilder):void) $specification
        if (\is_callable($specification) && \is_object($specification)) {
     * @param mixed|Criteria|array<string,mixed>|(object&callable(QueryBuilder):void) $specification
     * @param Criteria|array<string,mixed>|(object&callable(QueryBuilder):void) $specification
    /**
     * @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');
    }