NEXT-37397 - Security picks

shopware · Aug 7, 2024 · a784aa1 · a784aa1
1 parent 5e7a9dd
commit a784aa1
Show file tree

Hide file tree

Showing 8 changed files with 87 additions and 11 deletions.
diff --git a/Framework/Adapter/Twig/Node/FeatureCallSilentToken.php b/Framework/Adapter/Twig/Node/FeatureCallSilentToken.php
@@ -24,7 +24,9 @@ public function compile(Compiler $compiler): void
     {
         $compiler
             ->addDebugInfo($this)
-            ->raw('\Shopware\Core\Framework\Feature::callSilentIfInactive(\'' . $this->flag . '\', function () use(&$context) { ')
+            ->raw('\Shopware\Core\Framework\Feature::callSilentIfInactive(')
+            ->string($this->flag)
+            ->raw(', function () use(&$context) { ')
             ->subcompile($this->getNode('body'))
             ->raw('});');
     }

diff --git a/Framework/Context.php b/Framework/Context.php
@@ -149,11 +149,11 @@ public function createWithVersionId(string $versionId): self
     /**
      * @template TReturn of mixed
      *
-     * @param callable(Context): TReturn $callback
+     * @param \Closure(Context): TReturn $callback
      *
      * @return TReturn the return value of the provided callback function
      */
-    public function scope(string $scope, callable $callback)
+    public function scope(string $scope, \Closure $callback)
     {
         $currentScope = $this->getScope();
         $this->scope = $scope;
@@ -216,11 +216,11 @@ public function setRuleIds(array $ruleIds): void
     /**
      * @template TReturn of mixed
      *
-     * @param callable(Context): TReturn $function
+     * @param \Closure(Context): TReturn $function
      *
      * @return TReturn
      */
-    public function enableInheritance(callable $function)
+    public function enableInheritance(\Closure $function)
     {
         $previous = $this->considerInheritance;
         $this->considerInheritance = true;
@@ -233,11 +233,11 @@ public function enableInheritance(callable $function)
     /**
      * @template TReturn of mixed
      *
-     * @param callable(Context): TReturn $function
+     * @param \Closure(Context): TReturn $function
      *
      * @return TReturn
      */
-    public function disableInheritance(callable $function)
+    public function disableInheritance(\Closure $function)
     {
         $previous = $this->considerInheritance;
         $this->considerInheritance = false;

diff --git a/Framework/DataAbstractionLayer/DataAbstractionLayerException.php b/Framework/DataAbstractionLayer/DataAbstractionLayerException.php
@@ -44,6 +44,7 @@ class DataAbstractionLayerException extends HttpException
     public const INVALID_WRITE_INPUT = 'FRAMEWORK__INVALID_WRITE_INPUT';
     public const DECODE_HANDLED_BY_HYDRATOR = 'FRAMEWORK__DECODE_HANDLED_BY_HYDRATOR';
     public const ATTRIBUTE_NOT_FOUND = 'FRAMEWORK__ATTRIBUTE_NOT_FOUND';
+    public const INVALID_AGGREGATION_NAME = 'FRAMEWORK__INVALID_AGGREGATION_NAME';
 
     public static function invalidSerializerField(string $expectedClass, Field $field): self
     {
@@ -402,4 +403,14 @@ public static function canNotFindAttribute(string $attribute, string $property):
             ['attribute' => $attribute, 'property' => $property]
         );
     }
+
+    public static function invalidAggregationName(string $name): self
+    {
+        return new self(
+            Response::HTTP_BAD_REQUEST,
+            self::INVALID_AGGREGATION_NAME,
+            'Invalid aggregation name "{{ name }}", cannot contain question marks und colon.',
+            ['name' => $name]
+        );
+    }
 }
diff --git a/Framework/DataAbstractionLayer/Dbal/EntityAggregator.php b/Framework/DataAbstractionLayer/Dbal/EntityAggregator.php
@@ -4,6 +4,7 @@
 
 use Doctrine\DBAL\Connection;
 use Shopware\Core\Framework\Context;
+use Shopware\Core\Framework\DataAbstractionLayer\DataAbstractionLayerException;
 use Shopware\Core\Framework\DataAbstractionLayer\DefinitionInstanceRegistry;
 use Shopware\Core\Framework\DataAbstractionLayer\EntityCollection;
 use Shopware\Core\Framework\DataAbstractionLayer\EntityDefinition;
@@ -116,6 +117,10 @@ private function fetchAggregation(
         Criteria $criteria,
         Context $context
     ): AggregationResult {
+        if (str_contains($aggregation->getName(), '?') || str_contains($aggregation->getName(), ':')) {
+            throw DataAbstractionLayerException::invalidAggregationName($aggregation->getName());
+        }
+
         $clone = clone $criteria;
         $clone->resetAggregations();
         $clone->resetSorting();

diff --git a/Framework/DataAbstractionLayer/Search/Parser/AggregationParser.php b/Framework/DataAbstractionLayer/Search/Parser/AggregationParser.php
@@ -199,6 +199,12 @@ private function parseAggregation(int $index, EntityDefinition $definition, arra
             return null;
         }
 
+        if (str_contains($name, '?') || str_contains($name, ':')) {
+            $exceptions->add(new InvalidAggregationQueryException('The aggregation name should not contain a question mark or colon.'), '/aggregations/' . $index);
+
+            return null;
+        }
+
         $type = $aggregation['type'] ?? null;
 
         if (!\is_string($type) || empty($type) || is_numeric($type)) {

diff --git a/Framework/Test/DataAbstractionLayer/Search/EntityAggregatorTest.php b/Framework/Test/DataAbstractionLayer/Search/EntityAggregatorTest.php
@@ -11,6 +11,7 @@
 use Shopware\Core\Defaults;
 use Shopware\Core\DevOps\Environment\EnvironmentHelper;
 use Shopware\Core\Framework\Context;
+use Shopware\Core\Framework\DataAbstractionLayer\DataAbstractionLayerException;
 use Shopware\Core\Framework\DataAbstractionLayer\Exception\InvalidAggregationQueryException;
 use Shopware\Core\Framework\DataAbstractionLayer\Search\Aggregation\Bucket\DateHistogramAggregation;
 use Shopware\Core\Framework\DataAbstractionLayer\Search\Aggregation\Bucket\FilterAggregation;
@@ -1338,6 +1339,18 @@ public function testAggregationWithBacktickInName(): void
         $this->aggregator->aggregate($this->getContainer()->get(TaxDefinition::class), $criteria, $context);
     }
 
+    public function testAggregationNameWithDisallowedName(): void
+    {
+        $context = Context::createDefaultContext();
+
+        $criteria = new Criteria();
+        $criteria->addAggregation(new SumAggregation('foo?foo', 'taxRate'));
+
+        static::expectExceptionObject(DataAbstractionLayerException::invalidAggregationName('foo?foo'));
+
+        $this->aggregator->aggregate($this->getContainer()->get(TaxDefinition::class), $criteria, $context);
+    }
+
     private function insertData(): void
     {
         $repository = $this->getContainer()->get('product.repository');

diff --git a/Framework/Test/DataAbstractionLayer/Search/Parser/AggregationParserTest.php b/Framework/Test/DataAbstractionLayer/Search/Parser/AggregationParserTest.php
@@ -318,4 +318,33 @@ public function testICanCreateARangeAggregation(): void
         static::assertEquals($expectedRanges[0] + ['key' => '1-2'], $computedRanges[0]);
         static::assertEquals($expectedRanges[1] + ['key' => '2-3'], $computedRanges[1]);
     }
+
+    public function testQuestionMarkNotAllowedInAggregationName(): void
+    {
+        $criteria = new Criteria();
+        $searchRequestException = new SearchRequestException();
+        $this->parser->buildAggregations(
+            self::getContainer()->get(ProductDefinition::class),
+            [
+                'aggregations' => [
+                    [
+                        'name' => 'max?agg',
+                        'type' => 'max',
+                        'field' => 'tax.taxRate',
+                    ],
+                ],
+            ],
+            $criteria,
+            $searchRequestException
+        );
+
+        $errors = iterator_to_array($searchRequestException->getErrors(), false);
+        static::assertCount(1, $errors);
+
+        $error = array_shift($errors);
+
+        static::assertNotNull($error);
+
+        static::assertSame('The aggregation name should not contain a question mark or colon.', $error['detail']);
+    }
 }
diff --git a/System/SalesChannel/Entity/SalesChannelRepository.php b/System/SalesChannel/Entity/SalesChannelRepository.php
@@ -9,6 +9,7 @@
 use Shopware\Core\Framework\DataAbstractionLayer\Event\EntitySearchResultLoadedEvent;
 use Shopware\Core\Framework\DataAbstractionLayer\Exception\InconsistentCriteriaIdsException;
 use Shopware\Core\Framework\DataAbstractionLayer\Field\AssociationField;
+use Shopware\Core\Framework\DataAbstractionLayer\Field\ManyToManyAssociationField;
 use Shopware\Core\Framework\DataAbstractionLayer\Read\EntityReaderInterface;
 use Shopware\Core\Framework\DataAbstractionLayer\RepositorySearchDetector;
 use Shopware\Core\Framework\DataAbstractionLayer\Search\AggregationResult\AggregationResultCollection;
@@ -203,7 +204,7 @@ private function processCriteria(Criteria $topCriteria, SalesChannelContext $sal
         }
 
         $queue = [
-            ['definition' => $this->definition, 'criteria' => $topCriteria],
+            ['definition' => $this->definition, 'criteria' => $topCriteria, 'path' => ''],
         ];
 
         $maxCount = 100;
@@ -216,8 +217,10 @@ private function processCriteria(Criteria $topCriteria, SalesChannelContext $sal
 
             $definition = $cur['definition'];
             $criteria = $cur['criteria'];
+            $path = $cur['path'];
+            $processedKey = $path . $definition::class;
 
-            if (isset($processed[$definition::class])) {
+            if (isset($processed[$processedKey])) {
                 continue;
             }
 
@@ -230,7 +233,7 @@ private function processCriteria(Criteria $topCriteria, SalesChannelContext $sal
                 $this->eventDispatcher->dispatch($event, $eventName);
             }
 
-            $processed[$definition::class] = true;
+            $processed[$processedKey] = true;
 
             foreach ($criteria->getAssociations() as $associationName => $associationCriteria) {
                 // find definition
@@ -240,7 +243,14 @@ private function processCriteria(Criteria $topCriteria, SalesChannelContext $sal
                 }
 
                 $referenceDefinition = $field->getReferenceDefinition();
-                $queue[] = ['definition' => $referenceDefinition, 'criteria' => $associationCriteria];
+                $queue[] = ['definition' => $referenceDefinition, 'criteria' => $associationCriteria, 'path' => $path . '.' . $associationName];
+
+                if (!$field instanceof ManyToManyAssociationField) {
+                    continue;
+                }
+
+                $referenceDefinition = $field->getToManyReferenceDefinition();
+                $queue[] = ['definition' => $referenceDefinition, 'criteria' => $associationCriteria, 'path' => $path . '.' . $associationName];
             }
         }
     }