#10: OR filter bypasses all doctrine extensions -> Potential security…

… problem - made workaround assuming that eventual extensions do not use QueryNameGenerator, otherwise throws Exception, - added security warnings with respect to innerJoinsLeft and AddFakeLeftJoin
metaclass-nl · Aug 8, 2022 · e60889a · e60889a
1 parent 912d8aa
commit e60889a
Show file tree

Hide file tree

Showing 3 changed files with 213 additions and 20 deletions.
diff --git a/README.md b/README.md
@@ -7,11 +7,6 @@ Combines API Platform ORM Filters with AND, OR and NOT according to client reque
 - works with built in filters of Api Platform, except for DateFilter
   with EXCLUDE_NULL. A DateFilter subclass is provided to correct this.
 
-SECURIY WARNING: The current version of LogicFilter allows clients 
-to bypass criteria set by custom Extensions to limit their access to certain data,
-like the examples do in the docs on [Custom Doctrine ORM Extension](https://api-platform.com/docs/core/extensions/#custom-doctrine-orm-extension) 
-see [Issue 10](https://github.com/metaclass-nl/filter-bundle/issues/10).
-
 Usage
 -----
 Once the FilterLogic class and service configuration have been installed in you app,
@@ -89,6 +84,8 @@ class Article {
 // ...
 }
 ```
+<b>SECURITY WARNING</b>: do not use this option if the working of any of the extionsions 
+you use relies on INNER JOINS selecting only rows with NOT NULL values!
 
 In case you do not like FilterLogic messing with the joins you can make
 the built-in filters of Api Platform generate left joins themselves by first adding
@@ -103,6 +100,12 @@ class Article {
 // ...
 }
 ```
+<b>SECURITY WARNING</b>: Extensions that use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryBuilderHelper::addJoinOnce
+or ApiPlatform\Core\Bridge\Doctrine\Orm\PropertyHelperTrait::addJoinsForNestedProperty
+may not have been intended to generate left joins instead of inner joins. Of course this would
+technically be their error, not yours, but still it is better to prevent an eventual breach of security
+then having to deal with the consequences.
+
 
 With one fo these workarounds the following will find Articles whose title contains 'pro'
 as well as those whose keywords contain one whose word is 'php'.
@@ -142,6 +145,10 @@ with the others by either Doctrine\ORM\Query\Expr\Andx or Doctrine\ORM\Query\Exp
 
 May Fail if a filter or extension uses QueryBuilder::where or ::add. 
 
+May fail if boths extensions and filters add joins or both use the QueryNameGenerator 
+because the workaround for [Issue 10](https://github.com/metaclass-nl/filter-bundle/issues/10)
+can not reconize wich ones come from the extensions and not correctly initialize the QueryNameGenerator.
+
 You are advised to check the code of all custom and third party Filters and
 not to combine those that use QueryBuilder::where or ::add with FilterLogic
 or that produce complex logic that is not semantically complete. For an

diff --git a/src/Filter/FilterLogic.php b/src/Filter/FilterLogic.php
@@ -6,6 +6,7 @@
 use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\AbstractContextAwareFilter;
 use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\ContextAwareFilterInterface;
 use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\OrderFilter;
+use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGenerator;
 use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface;
 use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
 use ApiPlatform\Core\Exception\ResourceClassNotFoundException;
@@ -77,32 +78,79 @@ public function apply(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $q
 
         $this->filters = $this->getFilters($resourceClass, $operationName);
 
+
+        // Issue #10 workaround, tries to AND with criteria from extensions
+        $existingWhere = (string) $queryBuilder->getDQLPart('where');
+
+        $newQb = new QueryBuilder($queryBuilder->getEntityManager());
+        $newQb->add('select', $queryBuilder->getDQLPart('select'));
+        $newQb->add('from', $queryBuilder->getDQLPart('from'));
+        $newQng = new QueryNameGenerator();
+        // Problem: too hard to add the joins from the extensions and correctly initialize the QueryNameGenerator
+        // Workaround may fail if extensions did any joins and filters also, or if both use the QueryNameGenerator
+
+         $filters = $this->getFilters($resourceClass, $operationName, true);
+        foreach ($filters as $filter) {
+            $filter->apply($newQb, $newQng, $resourceClass, $operationName, $context);
+        }
+        $filterWhere = (string) $newQb->getDQLPart('where');
+
+        $logicExp = new Expr\Andx();
+        $combinator = 'AND';
         if (isset($context['filters']['and']) ) {
             $expressions = $this->filterProperty('and', $context['filters']['and'], $queryBuilder, $queryNameGenerator, $resourceClass, $operationName, $context);
-            foreach($expressions as $exp) {
-                $queryBuilder->andWhere($exp);
-            };
+            //var_export($expressions);
+            $logicExp->addMultiple($expressions);
         }
         if (isset($context['filters']['not']) ) {
             // NOT expressions are combined by parent logic, here defaulted to AND
             $expressions = $this->filterProperty('not', $context['filters']['not'], $queryBuilder, $queryNameGenerator, $resourceClass, $operationName, $context);
             foreach($expressions as $exp) {
-                $queryBuilder->andWhere(new Expr\Func('NOT', [$exp]));
+                $logicExp->add(new Expr\Func('NOT', [$exp]));
             };
         }
         if (isset($context['filters']['or'])) {
-            $expressions = $this->filterProperty('or', $context['filters']['or'], $queryBuilder, $queryNameGenerator, $resourceClass, $operationName, $context);
-            foreach($expressions as $exp) {
-                $queryBuilder->orWhere($exp);
-            };
+            $orExpressions = $this->filterProperty('or', $context['filters']['or'], $queryBuilder, $queryNameGenerator, $resourceClass, $operationName, $context);
+            if (!empty($orExpressions)) {
+                $logicExp = new Expr\Orx([$logicExp]);
+                $logicExp->addMultiple($orExpressions);
+                $combinator = 'OR';
+            }
         }
 
         if ($this->innerJoinsLeft) {
             $this->replaceInnerJoinsByLeftJoins($queryBuilder);
         }
+
+        // if $existingWhere empty no problem
+        // if  $filterWhere empty nest OR in an extra AND
+        if (empty($existingWhere) || empty($filterWhere) ) {
+            $queryBuilder->andWhere($logicExp);
+            return;
+        }
+        // elseif only criteria from filters, apply according to operator
+        if ($existingWhere == $filterWhere) {
+            if ($combinator == 'OR') {
+                $queryBuilder->orWhere($logicExp);
+            } else {
+                $queryBuilder->andWhere($logicExp);
+            }
+            return;
+        }
+        // elseif criteria from filters follow AND, replace them
+        if(false!==strpos($existingWhere, " AND $filterWhere")) {
+            $queryBuilder->add('where',
+                str_replace($filterWhere, "($filterWhere $combinator ($logicExp))", $existingWhere)
+            );
+            return;
+        }
+
+        // Could not replace criteria from filters, probably an extension used the QueryNameGenerator
+        //throw new \RuntimeException("Could not replace '$filterWhere' in '$existingWhere'");
+        throw new \RuntimeException("Could not replace criteria from filters");
     }
 
-    /**
+     /**
      * @return array of Doctrine\ORM\Query\Expr\* and/or string (DQL),
      * each of which must be self-contained in the sense that the intended
      * logic is not compromised if it is combined with the others and other
@@ -249,10 +297,11 @@ private function getAppliedExpressions($where, $marker)
     /**
      * @param string $resourceClass
      * @param string $operationName
+     * @param bool $ignoreClassExp
      * @return ContextAwareFilterInterface[] From resource except $this and OrderFilters
      * @throws ResourceClassNotFoundException
      */
-    protected function getFilters($resourceClass, $operationName)
+    protected function getFilters($resourceClass, $operationName, $ignoreClassExp=false)
     {
         $resourceMetadata = $this->resourceMetadataFactory->create($resourceClass);
         $resourceFilters = $resourceMetadata->getCollectionOperationAttribute($operationName, 'filters', [], true);
@@ -265,7 +314,7 @@ protected function getFilters($resourceClass, $operationName)
             if ($filter instanceof ContextAwareFilterInterface
                 && !($filter instanceof OrderFilter)
                 && $filter !== $this
-                && preg_match($this->classExp, get_class($filter))
+                && ($ignoreClassExp || preg_match($this->classExp, get_class($filter)))
             ) {
                 $result[$filterId] = $filter;
             }

diff --git a/tests/Filter/FilterLogicWithAnnotationTest.php b/tests/Filter/FilterLogicWithAnnotationTest.php
@@ -56,10 +56,10 @@ public function setUp(): void
         self::assertNotNull($this->filterLogic, "this->filterLogic");
     }
 
-    public function testDdFilter()
+    public function testDdFilterAnd()
     {
         $reqData = null;
-        parse_str('&and[or][dd][after]=2021-01-01&and[or][exists][bool]=true', $reqData);
+        parse_str('exists[bool]=true&and[or][dd][after]=2021-01-01', $reqData);
         // var_dump($reqData);
         $context = ['filters' => $reqData];
         foreach ($this->filters as $filter) {
@@ -69,8 +69,8 @@ public function testDdFilter()
         $this->assertEquals(
             str_replace('
 ', '', "SELECT o FROM Metaclass\FilterBundle\Entity\TestEntity o WHERE 
-(o.dd >= :dd_p1 OR o.dd IS NULL) 
-OR o.bool IS NOT NULL"),
+o.bool IS NOT NULL 
+AND (o.dd >= :dd_p1 OR o.dd IS NULL)"),
             $this->testEntityQb->getDQL(),
             'DQL');
         $this->assertEquals(
@@ -80,6 +80,143 @@ public function testDdFilter()
 
     }
 
+    public function testDdFilterNot()
+    {
+        $reqData = null;
+        parse_str('exists[bool]=true&not[dd][after]=2021-01-01', $reqData);
+        // var_dump($reqData);
+        $context = ['filters' => $reqData];
+        foreach ($this->filters as $filter) {
+            $filter->apply($this->testEntityQb, $this->queryNameGen, TestEntity::class, 'get', $context);
+        }
+
+        $this->assertEquals(
+            str_replace('
+', '', "SELECT o FROM Metaclass\FilterBundle\Entity\TestEntity o WHERE 
+o.bool IS NOT NULL 
+AND (NOT(o.dd >= :dd_p1 OR o.dd IS NULL))"),
+            $this->testEntityQb->getDQL(),
+            'DQL');
+        $this->assertEquals(
+            '2021-01-01',
+            $this->testEntityQb->getParameter('dd_p1')->getValue()->format('Y-m-d'),
+            'Parameter dd_p1');
+    }
+
+    public function testDdFilterOr()
+    {
+        $reqData = null;
+        parse_str('exists[bool]=true&or[dd][after]=2021-01-01&or[dd][before]=2010-02-02', $reqData);
+        // var_dump($reqData);
+        $context = ['filters' => $reqData];
+        foreach ($this->filters as $filter) {
+            $filter->apply($this->testEntityQb, $this->queryNameGen, TestEntity::class, 'get', $context);
+        }
+
+        $this->assertEquals(
+            str_replace('
+', '', "SELECT o FROM Metaclass\FilterBundle\Entity\TestEntity o WHERE 
+o.bool IS NOT NULL 
+OR (
+(o.dd <= :dd_p1 AND o.dd IS NOT NULL) 
+OR (o.dd >= :dd_p2 OR o.dd IS NULL)
+)"),
+            $this->testEntityQb->getDQL(),
+            'DQL');
+        $this->assertEquals(
+            '2010-02-02',
+            $this->testEntityQb->getParameter('dd_p1')->getValue()->format('Y-m-d'),
+            'Parameter dd_p1');
+        $this->assertEquals(
+            '2021-01-01',
+            $this->testEntityQb->getParameter('dd_p2')->getValue()->format('Y-m-d'),
+            'Parameter dd_p2');
+    }
+
+    public function testDdFilterAndWithExtsionCriterium()
+    {
+        $this->testEntityQb->andWhere('o.numb >= 0');
+        $reqData = null;
+        parse_str('exists[bool]=true&and[or][dd][after]=2021-01-01', $reqData);
+        // var_dump($reqData);
+        $context = ['filters' => $reqData];
+        foreach ($this->filters as $filter) {
+            $filter->apply($this->testEntityQb, $this->queryNameGen, TestEntity::class, 'get', $context);
+        }
+
+        $this->assertEquals(
+            str_replace('
+', '', "SELECT o FROM Metaclass\FilterBundle\Entity\TestEntity o WHERE 
+o.numb >= 0 AND (
+o.bool IS NOT NULL 
+AND (o.dd >= :dd_p1 OR o.dd IS NULL)
+)"),
+            $this->testEntityQb->getDQL(),
+            'DQL');
+        $this->assertEquals(
+            '2021-01-01',
+            $this->testEntityQb->getParameter('dd_p1')->getValue()->format('Y-m-d'),
+            'Parameter dd_p1');
+
+    }
+
+    public function testDdFilterNotWithExtsionCriterium()
+    {
+        $this->testEntityQb->andWhere('o.numb >= 0');
+        $reqData = null;
+        parse_str('exists[bool]=true&not[dd][after]=2021-01-01', $reqData);
+        // var_dump($reqData);
+        $context = ['filters' => $reqData];
+        foreach ($this->filters as $filter) {
+            $filter->apply($this->testEntityQb, $this->queryNameGen, TestEntity::class, 'get', $context);
+        }
+
+        $this->assertEquals(
+            str_replace('
+', '', "SELECT o FROM Metaclass\FilterBundle\Entity\TestEntity o WHERE 
+o.numb >= 0 AND (
+o.bool IS NOT NULL 
+AND (NOT(o.dd >= :dd_p1 OR o.dd IS NULL)))"),
+            $this->testEntityQb->getDQL(),
+            'DQL');
+        $this->assertEquals(
+            '2021-01-01',
+            $this->testEntityQb->getParameter('dd_p1')->getValue()->format('Y-m-d'),
+            'Parameter dd_p1');
+    }
+
+    public function testDdFilterOrWithExtsionCriterium()
+    {
+        $this->testEntityQb->andWhere('o.numb >= 0');
+        $reqData = null;
+        parse_str('exists[bool]=true&or[dd][after]=2021-01-01&or[dd][before]=2010-02-02', $reqData);
+        // var_dump($reqData);
+        $context = ['filters' => $reqData];
+        foreach ($this->filters as $filter) {
+            $filter->apply($this->testEntityQb, $this->queryNameGen, TestEntity::class, 'get', $context);
+        }
+
+        $this->assertEquals(
+            str_replace('
+', '', "SELECT o FROM Metaclass\FilterBundle\Entity\TestEntity o WHERE 
+o.numb >= 0 AND (
+o.bool IS NOT NULL 
+OR (
+(o.dd <= :dd_p1 AND o.dd IS NOT NULL) 
+OR (o.dd >= :dd_p2 OR o.dd IS NULL)
+))"),
+            $this->testEntityQb->getDQL(),
+            'DQL');
+        $this->assertEquals(
+            '2010-02-02',
+            $this->testEntityQb->getParameter('dd_p1')->getValue()->format('Y-m-d'),
+            'Parameter dd_p1');
+        $this->assertEquals(
+            '2021-01-01',
+            $this->testEntityQb->getParameter('dd_p2')->getValue()->format('Y-m-d'),
+            'Parameter dd_p2');
+    }
+
     public function testRexExp()
     {
         $regExp = '/'. str_replace('\\', '\\\\', DateFilter::class). '/';