From e60889adf13fe853a8ad8bda729e492e25f5c598 Mon Sep 17 00:00:00 2001 From: henk Date: Mon, 8 Aug 2022 21:48:26 +0200 Subject: [PATCH] #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 --- README.md | 17 +- src/Filter/FilterLogic.php | 71 +++++++-- .../Filter/FilterLogicWithAnnotationTest.php | 145 +++++++++++++++++- 3 files changed, 213 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 92b393f..c618ce4 100644 --- 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 { // ... } ``` +SECURITY WARNING: 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 { // ... } ``` +SECURITY WARNING: 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 index a7f3d66..028ef42 100644 --- 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 index a6e63dc..4b19f26 100644 --- 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¬[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¬[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). '/';