Avoid simple text replacements in SQL if possible

MAILPOET-4986
mailpoet · Mar 17, 2023 · 6f153b6 · 6f153b6
1 parent c6c0143
commit 6f153b6
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 9 deletions.
diff --git a/mailpoet/lib/Segments/DynamicSegments/Filters/FilterHelper.php b/mailpoet/lib/Segments/DynamicSegments/Filters/FilterHelper.php
@@ -3,6 +3,7 @@
 namespace MailPoet\Segments\DynamicSegments\Filters;
 
 use MailPoet\Entities\SubscriberEntity;
+use MailPoet\Util\Security;
 use MailPoetVendor\Doctrine\DBAL\Query\QueryBuilder;
 use MailPoetVendor\Doctrine\ORM\EntityManager;
 
@@ -34,4 +35,27 @@ public function getSubscribersTable(): string {
       ->getClassMetadata(SubscriberEntity::class)
       ->getTableName();
   }
+
+  public function getInterpolatedSQL(QueryBuilder $query): string {
+    $sql = $query->getSQL();
+    $params = $query->getParameters();
+    $search = array_map(function($key) {
+      return ":$key";
+    }, array_keys($params));
+    $replace = array_map(function($value) use ($query) {
+      if (is_array($value)) {
+        $quotedValues = array_map(function($arrayValue) use ($query) {
+          return $query->expr()->literal($arrayValue);
+        }, $value);
+        return implode(',', $quotedValues);
+      }
+      return $query->expr()->literal($value);
+    }, array_values($params));
+    return str_replace($search, $replace, $sql);
+  }
+
+  public function getUniqueParameterName(string $parameter): string {
+    $suffix = Security::generateRandomString();
+    return sprintf("%s_%s", $parameter, $suffix);
+  }
 }
diff --git a/mailpoet/lib/Segments/DynamicSegments/Filters/WooCommercePurchaseDate.php b/mailpoet/lib/Segments/DynamicSegments/Filters/WooCommercePurchaseDate.php
@@ -37,7 +37,7 @@ public function apply(QueryBuilder $queryBuilder, DynamicSegmentFilterEntity $fi
     if (in_array($operator, [DateFilterHelper::NOT_ON, DateFilterHelper::NOT_IN_THE_LAST])) {
       $subQuery = $this->filterHelper->getNewSubscribersQueryBuilder();
       $this->applyConditionsToQueryBuilder($operator, $date, $subQuery);
-      $queryBuilder->andWhere($queryBuilder->expr()->notIn("{$subscribersTable}.id", $subQuery->getSQL()));
+      $queryBuilder->andWhere($queryBuilder->expr()->notIn("{$subscribersTable}.id", $this->filterHelper->getInterpolatedSQL($subQuery)));
     } else {
       $this->applyConditionsToQueryBuilder($operator, $date, $queryBuilder);
     }
@@ -47,27 +47,29 @@ public function apply(QueryBuilder $queryBuilder, DynamicSegmentFilterEntity $fi
 
   private function applyConditionsToQueryBuilder(string $operator, string $date, QueryBuilder $queryBuilder): QueryBuilder {
     $orderStatsAlias = $this->wooFilterHelper->applyOrderStatusFilter($queryBuilder);
-    $quotedDate = $queryBuilder->expr()->literal($date);
+    $dateParam = $this->filterHelper->getUniqueParameterName('date');
 
     switch ($operator) {
       case DateFilterHelper::BEFORE:
-        $queryBuilder->andWhere("DATE($orderStatsAlias.date_created) < $quotedDate");
+        $queryBuilder->andWhere("DATE($orderStatsAlias.date_created) < :$dateParam");
         break;
       case DateFilterHelper::AFTER:
-        $queryBuilder->andWhere("DATE($orderStatsAlias.date_created) > $quotedDate");
+        $queryBuilder->andWhere("DATE($orderStatsAlias.date_created) > :$dateParam");
         break;
       case DateFilterHelper::IN_THE_LAST:
       case DateFilterHelper::NOT_IN_THE_LAST:
-        $queryBuilder->andWhere("DATE($orderStatsAlias.date_created) >= $quotedDate");
+        $queryBuilder->andWhere("DATE($orderStatsAlias.date_created) >= :$dateParam");
         break;
       case DateFilterHelper::ON:
       case DateFilterHelper::NOT_ON:
-        $queryBuilder->andWhere("DATE($orderStatsAlias.date_created) = $quotedDate");
+        $queryBuilder->andWhere("DATE($orderStatsAlias.date_created) = :$dateParam");
         break;
       default:
         throw new InvalidFilterException('Incorrect value for operator', InvalidFilterException::MISSING_VALUE);
     }
 
+    $queryBuilder->setParameter($dateParam, $date);
+
     return $queryBuilder;
   }
 }
diff --git a/mailpoet/lib/Segments/DynamicSegments/Filters/WooFilterHelper.php b/mailpoet/lib/Segments/DynamicSegments/Filters/WooFilterHelper.php
@@ -3,6 +3,7 @@
 namespace MailPoet\Segments\DynamicSegments\Filters;
 
 use MailPoet\Util\DBCollationChecker;
+use MailPoetVendor\Doctrine\DBAL\Connection;
 use MailPoetVendor\Doctrine\DBAL\Query\QueryBuilder;
 
 class WooFilterHelper {
@@ -76,10 +77,10 @@ public function applyOrderStatusFilter(QueryBuilder $queryBuilder, array $allowe
       $allowedStatuses = $this->defaultIncludedStatuses();
     }
 
+    $statusParam = $this->filterHelper->getUniqueParameterName('status');
     $orderStatsAlias = $this->applyCustomerOrderJoin($queryBuilder);
-    $quotedStatus = array_map([$queryBuilder->expr(), 'literal'], $allowedStatuses);
-    $queryBuilder->andWhere($queryBuilder->expr()->in("$orderStatsAlias.status", $quotedStatus));
-
+    $queryBuilder->andWhere("$orderStatsAlias.status IN (:$statusParam)");
+    $queryBuilder->setParameter($statusParam, $allowedStatuses, Connection::PARAM_STR_ARRAY);
     return $orderStatsAlias;
   }
 

diff --git a/mailpoet/tests/integration/Segments/DynamicSegments/Filters/FilterHelperTest.php b/mailpoet/tests/integration/Segments/DynamicSegments/Filters/FilterHelperTest.php
@@ -0,0 +1,47 @@
+<?php declare(strict_types = 1);
+
+namespace MailPoet\Segments\DynamicSegments\Filters;
+
+use MailPoet\Entities\SubscriberEntity;
+use MailPoetVendor\Doctrine\DBAL\Query\QueryBuilder;
+
+class FilterHelperTest extends \MailPoetTest {
+  /** @var FilterHelper */
+  private $filterHelper;
+
+  /** @var string */
+  private $subscribersTable;
+
+  public function _before() {
+    parent::_before();
+    $this->filterHelper = $this->diContainer->get(FilterHelper::class);
+    $this->subscribersTable = $this->entityManager
+      ->getClassMetadata(SubscriberEntity::class)
+      ->getTableName();
+  }
+
+  public function testItCanReturnSQLThatDoesNotIncludeParams(): void {
+    $queryBuilder = $this->getSubscribersQueryBuilder();
+    $defaultResult = $queryBuilder->getSQL();
+    expect($defaultResult)->equals("SELECT id FROM $this->subscribersTable");
+    expect($this->filterHelper->getInterpolatedSQL($queryBuilder))->equals($defaultResult);
+  }
+
+  public function testItCanReturnInterpolatedSQL(): void {
+    $queryBuilder = $this->getSubscribersQueryBuilder();
+    $queryBuilder->where("$this->subscribersTable.created_at < :date");
+    $queryBuilder->setParameter('date', '2023-03-09');
+    expect($this->filterHelper->getInterpolatedSQL($queryBuilder))->equals("SELECT id FROM $this->subscribersTable WHERE $this->subscribersTable.created_at < '2023-03-09'");
+  }
+
+  public function testItProperlyInterpolatesArrayValues(): void {
+    $queryBuilder = $this->getSubscribersQueryBuilder();
+    $queryBuilder->where("$this->subscribersTable.status IN (:statuses)");
+    $queryBuilder->setParameter('statuses', ['subscribed', 'inactive']);
+    expect($this->filterHelper->getInterpolatedSQL($queryBuilder))->equals("SELECT id FROM $this->subscribersTable WHERE $this->subscribersTable.status IN ('subscribed','inactive')");
+  }
+
+  private function getSubscribersQueryBuilder(): QueryBuilder {
+    return $this->entityManager->getConnection()->createQueryBuilder()->select('id')->from($this->subscribersTable);
+  }
+}