From f10c941ce2a4e8c55501bbfe2c28bfd99023250b Mon Sep 17 00:00:00 2001 From: Patrick Rathje Date: Tue, 7 May 2024 08:03:20 +0200 Subject: [PATCH] Update QueryManager.php Due to a bug in string comparison, custom DQL can be added to the orderBy, potentially leading to SQL injection. This pull request fixes the original comparison and further filter special characters from the given property paths --- Service/QueryManager.php | 104 ++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 51 deletions(-) diff --git a/Service/QueryManager.php b/Service/QueryManager.php index 7ba337b..fb65641 100644 --- a/Service/QueryManager.php +++ b/Service/QueryManager.php @@ -2,35 +2,31 @@ namespace BrauneDigital\QueryFilterBundle\Service; -use Doctrine\ORM\QueryBuilder; use BrauneDigital\QueryFilterBundle\Exception\InvalidConfigException; use BrauneDigital\QueryFilterBundle\Query\Filter\FilterInterface; use BrauneDigital\QueryFilterBundle\Query\QueryBuilderJoinWrapperInterface; use BrauneDigital\QueryFilterBundle\Query\QueryBuilderTranslationJoinWrapper; +use Doctrine\ORM\QueryBuilder; -class QueryManager { - +class QueryManager +{ protected $filters; - /** - * - */ public function __construct() { - $this->filters = array(); + $this->filters = []; } - /** - * @param FilterInterface $filter - * @param $alias - */ public function addFilter(FilterInterface $filter, $alias) { $this->filters[$alias] = $filter; } + public static function removeSpecialCharacters(string $string) : ?string + { + return preg_replace('/[^A-Za-z0-9\-_\.]/', '', $string); + } /** - * @param $alias * @return FilterInterface */ protected function getFilter($alias) @@ -38,42 +34,45 @@ protected function getFilter($alias) if (array_key_exists($alias, $this->filters)) { return $this->filters[$alias]; } else { - throw new \Exception('Filter ' . $alias . " not found."); + throw new \Exception('Filter '.$alias.' not found.'); } } - public function getAliasProperty(QueryBuilderJoinWrapperInterface $qbWrapper, $path, $optional = false) { + public function getAliasProperty(QueryBuilderJoinWrapperInterface $qbWrapper, $path, $optional = false) + { $path = $this->toCamelCase($path); + $path = self::removeSpecialCharacters($path); $alias = $qbWrapper->getAlias($path, $optional); $pos = strrpos($path, '.'); - if($pos !== false) { + if ($pos !== false) { $property = substr($path, $pos + 1); } else { $property = $path; } - return array($alias, $property); - } - public function getExpr(QueryBuilderJoinWrapperInterface $qbWrapper, $data, $alias = null, $property = null, $optional = false) { + return [$alias, $property]; + } - if(empty($data)) { + public function getExpr(QueryBuilderJoinWrapperInterface $qbWrapper, $data, $alias = null, $property = null, $optional = false) + { + if (empty($data)) { return null; } if (array_key_exists('property', $data)) { - list($alias, $property) = $this->getAliasProperty($qbWrapper, $data['property'], $optional); + [$alias, $property] = $this->getAliasProperty($qbWrapper, $data['property'], $optional); } $filterType = null; if (array_key_exists('type', $data)) { $filterType = $data['type']; - } else if (array_key_exists('filter', $data)) { + } elseif (array_key_exists('filter', $data)) { $filterType = $data['filter']; } else { - throw new \Exception('No filter specified in ' . $alias . '.' . $property); + throw new \Exception('No filter specified in '.$alias.'.'.$property); } $filter = $this->getFilter($filterType); @@ -82,28 +81,31 @@ public function getExpr(QueryBuilderJoinWrapperInterface $qbWrapper, $data, $ali } /** - * @param QueryBuilder $queryBuilder * @param array $filterConfig - * @param null $locale + * @param null $locale + * * @deprecated */ - public function filter(QueryBuilder $queryBuilder, $filterConfig = array(), $locale = null) { + public function filter(QueryBuilder $queryBuilder, $filterConfig = [], $locale = null) + { $qbWrapper = new QueryBuilderTranslationJoinWrapper($queryBuilder, $locale); $this->filterWithWrapperOnly($qbWrapper, $filterConfig); } - public function filterWithWrapperOnly(QueryBuilderJoinWrapperInterface $qbWrapper, $filterConfig = array()) { + public function filterWithWrapperOnly(QueryBuilderJoinWrapperInterface $qbWrapper, $filterConfig = []) + { $this->filterWithWrapper($qbWrapper->getQueryBuilder(), $qbWrapper, $filterConfig); } /** - * @param QueryBuilder $queryBuilder - * @param QueryBuilderJoinWrapperInterface $qbWrapper * @param array $filterConfig + * * @throws InvalidConfigException + * * @deprecated */ - public function filterWithWrapper(QueryBuilder $queryBuilder, QueryBuilderJoinWrapperInterface $qbWrapper, $filterConfig = array()) { + public function filterWithWrapper(QueryBuilder $queryBuilder, QueryBuilderJoinWrapperInterface $qbWrapper, $filterConfig = []) + { if ($filterConfig === false) { return; // NOOP } @@ -113,15 +115,14 @@ public function filterWithWrapper(QueryBuilder $queryBuilder, QueryBuilderJoinWr } if (count($filterConfig) == 0) { - //NOOP + // NOOP return; } if (array_keys($filterConfig) === range(0, count($filterConfig) - 1)) { - - //build filters - foreach($filterConfig as $property => $filterData) { - + // build filters + foreach ($filterConfig as $property => $filterData) { + // $property = self::removeSpecialChars($property); $expr = $this->getExpr($qbWrapper, $filterData); if ($expr != null) { @@ -138,61 +139,62 @@ public function filterWithWrapper(QueryBuilder $queryBuilder, QueryBuilderJoinWr } /** - * @param QueryBuilder $queryBuilder * @param array $orderConfig - * @param null $locale + * @param null $locale */ - public function order(QueryBuilder $queryBuilder, $orderConfig = array(), $locale = null) { + public function order(QueryBuilder $queryBuilder, $orderConfig = [], $locale = null) + { $qbWrapper = new QueryBuilderTranslationJoinWrapper($queryBuilder, $locale); $this->orderWithWrapper($queryBuilder, $qbWrapper, $orderConfig); } /** - * @param QueryBuilder $queryBuilder * @param array $orderConfig - * @param null $locale */ - public function orderWithWrapper(QueryBuilder $queryBuilder, QueryBuilderJoinWrapperInterface $qbWrapper, $orderConfig = array()) { - + public function orderWithWrapper(QueryBuilder $queryBuilder, QueryBuilderJoinWrapperInterface $qbWrapper, $orderConfig = []) + { if (!is_array($orderConfig)) { throw new InvalidConfigException('The order config must be an array.'); } - foreach($orderConfig as $path => $order) { - - if(!is_string($path)) { + foreach ($orderConfig as $path => $order) { + if (!is_string($path)) { throw new InvalidConfigException('The order config must be an array of valid paths'); } $path = $this->toCamelCase($path); + $path = self::removeSpecialCharacters($path); $pos = strrpos($path, '.'); - if($pos !== false) { + if ($pos !== false) { $property = substr($path, $pos + 1); } else { $property = $path; } - if(strcasecmp($order, "ASC") || strcasecmp($order, "DESC")) { - $qbWrapper->getQueryBuilder()->addOrderBy($qbWrapper->getAlias($path, true) . "." . $property, $order); + $order = strtoupper($order); + if (in_array($order, ['ASC', 'DESC'])) { + $qbWrapper->getQueryBuilder()->addOrderBy($qbWrapper->getAlias($path, true).'.'.$property, $order); } else { - throw new InvalidConfigException("Invalid sortBy Value for order" . $path); + throw new InvalidConfigException('Invalid sortBy Value for order'.$path); } } } /** * @param $str - * TODO: This is not the best place here :D + * TODO: This is not the best place here :D */ - public function toCamelCase($str) { + public function toCamelCase($str) + { $parts = explode('_', strtolower($str)); $size = count($parts); $str = $parts[0]; - for($i = 1; $i < $size; $i++) { + for ($i = 1; $i < $size; ++$i) { $str .= ucfirst(trim($parts[$i])); } + return $str; } }