Merge pull request #238 from components-web-app/feature/237-or-search-filter-andwhere

AND the OrSearchFilter's clauses against the rest of the query (#237)

Author: silverbackdan

CLAUDE.md

@@ -1004,3 +1004,25 @@ The `email` sub-node had no default of its own, so `new_email_confirmation` and
 Tests: `tests/DependencyInjection/ConfigurationTest.php` and `tests/DependencyInjection/SilverbackApiComponentsExtensionTest.php` (bare `ContainerBuilder`, no kernel boot — see the Risky/Infection warning above), `tests/Serializer/MappingLoader/TimestampedLoaderTest.php`, `tests/Serializer/Normalizer/TimestampedNormalizerTest.php`, and `features/timestamped/timestamped.feature` (PATCH scenarios for both the guarded and unguarded entity shapes).
 
 > **Newly covering a large declarative file costs MSI.** These DI tests pulled `Configuration.php` and `SilverbackApiComponentsExtension.php` into the covered set for the first time, so their mutants started counting where `--only-covered` had been skipping them — MSI fell from 85% to 82% against an 80% gate. The fix was to broaden the extension test to assert the wiring it performs, not to exclude the files.
+
+---
+
+### #237 — `orWhere()` in a Doctrine filter silently discards every query-extension predicate ✓ **DONE**
+
+`OrSearchFilter::addWhereByStrategy()` built its clauses with `$queryBuilder->orWhere(...)`. **Doctrine's `orWhere()` ORs against the entire accumulated WHERE, not just among the filter's own clauses.** AP4 registers `FilterExtension` at priority `-16` while this bundle's query extensions carry no explicit priority and therefore default to `0` — higher priority runs first, so the extensions add their predicates **before** the filter runs and the filter then ORs them away:
+
+```
+(liveAt IS NOT NULL AND liveAt <= :now) OR (o.path LIKE :path)
+```
+
+An anonymous `GET /_/routes?path=launch` therefore listed a route scheduled for 2999. The filter parameter is attacker-controlled, so the predicate being defeated is a security one. Confirmed empirically to defeat `RouteExtension` (own `liveAt`), `RouteAncestorGateResolver` (ancestor gate, #234) **and** `PublishableExtension` (an anonymous filtered collection returned every draft alongside the published resources).
+
+**The general trap, not a one-off: never use `orWhere()` in an API Platform filter.** Anything already on the query — a publication gate, a draft exclusion, an application's own extension — is inside the left operand of that OR and is discarded the moment the filter matches. Collect the filter's own clauses and apply them with a **single `andWhere()`** wrapping one `Expr\Orx`, which is the shape API Platform's own `SearchFilter` uses.
+
+**The accumulator must be shared across `addWhereByStrategy()` calls.** `AbstractFilter::apply()` calls `filterProperty()` once per query parameter, and each of those calls `addWhereByStrategy()` once — so an `Orx` built *inside* `addWhereByStrategy()` would turn multi-field search into AND and destroy the filter's whole purpose. `apply()` is overridden to clear the accumulator, delegate to the parent, then apply the single `andWhere()`; the accumulator is cleared again in a `finally` so no state survives the request under worker mode.
+
+Doctrine's DDC-1237 handling in `Expr\Composite::processQueryPart()` parenthesises any part whose string contains ` OR ` / ` AND `, which is what keeps the `word_start` strategy (a two-`LIKE` string clause) from capturing the preceding predicate once it is ANDed on.
+
+**`addWhereByStrategy()`'s five single-value branches are unreachable from `filterProperty()`** — `normalizeValues((array) $value, ...)` always hands it an array — but they were fixed too, since the method is `protected`.
+
+Tests: `tests/Filter/OrSearchFilterTest.php` asserts the DQL shape across all five strategies (case-sensitive and `i`-prefixed, single and multi-value) plus cross-field OR, the `word_start` parenthesisation and non-leakage between two `apply()` calls; `features/main/or_search_filter.feature` covers the scheduled, draft and ancestor-gated route cases, the publishable draft case, and two guards proving the filter still ORs across fields and across multiple values for one field. `DummyOrSearchFilterable` had existed with no coverage of any kind, which is why this survived.

features/bootstrap/DoctrineContext.php

@@ -45,6 +45,7 @@
 use Silverback\ApiComponentsBundle\Repository\User\UserRepositoryInterface;
 use Silverback\ApiComponentsBundle\Tests\Functional\TestBundle\Entity\DummyComponent;
 use Silverback\ApiComponentsBundle\Tests\Functional\TestBundle\Entity\DummyCustomTimestamped;
+use Silverback\ApiComponentsBundle\Tests\Functional\TestBundle\Entity\DummyOrSearchFilterable;
 use Silverback\ApiComponentsBundle\Tests\Functional\TestBundle\Entity\DummyPublishableComponent;
 use Silverback\ApiComponentsBundle\Tests\Functional\TestBundle\Entity\DummyTimestamped;
 use Silverback\ApiComponentsBundle\Tests\Functional\TestBundle\Entity\DummyTimestampedWithSerializationGroups;
@@ -1085,6 +1086,20 @@ public function thereIsALayout(string $reference = 'no-reference', ?string $crea
         return $layout;
     }
 
+    /**
+     * @Given there is a DummyOrSearchFilterable with field1 :field1 and field2 :field2
+     */
+    public function thereIsADummyOrSearchFilterable(string $field1, string $field2): DummyOrSearchFilterable
+    {
+        $resource = new DummyOrSearchFilterable();
+        $resource->field1 = $field1;
+        $resource->field2 = $field2;
+        $this->manager->persist($resource);
+        $this->manager->flush();
+
+        return $resource;
+    }
+
     /**
      * @Given there is an empty PageData resource
      */

features/main/or_search_filter.feature

@@ -0,0 +1,58 @@
+Feature: A search filter combines its own clauses with OR and ANDs them against the rest of the query
+  In order that supplying a filter parameter cannot bypass a publication gate or a draft exclusion
+  As an anonymous API consumer
+  A filtered collection must apply the query extensions' predicates as well as the filter's own
+
+  Background:
+    Given I add "Accept" header equal to "application/ld+json"
+    And I add "Content-Type" header equal to "application/ld+json"
+
+  Scenario: A filtered anonymous route collection still excludes a scheduled route
+    Given there is a Route "/launch" with a page
+    And the Route "/launch" goes live at "2999-01-01T00:00:00+00:00"
+    When I send a "GET" request to "/_/routes?path=launch"
+    Then the response status code should be 200
+    And the JSON node "member[0]" should not exist
+
+  Scenario: A filtered anonymous route collection still excludes a draft route
+    Given there is a Route "/launch" with a page
+    And the Route "/launch" has no go-live date
+    When I send a "GET" request to "/_/routes?path=launch"
+    Then the response status code should be 200
+    And the JSON node "member[0]" should not exist
+
+  Scenario: A filtered anonymous route collection still excludes a route gated by its ancestor
+    Given there is a PageData resource with the route path "/conference/programme" nested within the route "/conference"
+    And the Route "/conference" goes live at "2999-01-01T00:00:00+00:00"
+    When I send a "GET" request to "/_/routes?path=programme"
+    Then the response status code should be 200
+    And the JSON node "member[0]" should not exist
+
+  Scenario: A filtered anonymous route collection still returns a live route
+    Given there is a Route "/launch" with a page
+    When I send a "GET" request to "/_/routes?path=launch"
+    Then the response status code should be 200
+    And the JSON node "totalItems" should be equal to "1"
+    And the JSON node "member[0].path" should be equal to the string "/launch"
+
+  Scenario: The filter still matches across fields with OR
+    Given there is a DummyOrSearchFilterable with field1 "alpha" and field2 "beta"
+    And there is a DummyOrSearchFilterable with field1 "gamma" and field2 "alpha"
+    When I send a "GET" request to "/dummy_or_search_filterables?field1=alpha&field2=alpha"
+    Then the response status code should be 200
+    And the JSON node "totalItems" should be equal to "2"
+
+  Scenario: The filter still matches multiple values for one field with OR
+    Given there is a DummyOrSearchFilterable with field1 "alpha" and field2 "beta"
+    And there is a DummyOrSearchFilterable with field1 "gamma" and field2 "delta"
+    And there is a DummyOrSearchFilterable with field1 "epsilon" and field2 "zeta"
+    When I send a "GET" request to "/dummy_or_search_filterables?field1[]=alpha&field1[]=gamma"
+    Then the response status code should be 200
+    And the JSON node "totalItems" should be equal to "2"
+
+  @loginUser
+  Scenario: A filtered collection does not expose a draft to a user without draft access
+    Given there are 2 draft and published resources available
+    When I send a "GET" request to "/component/dummy_publishable_components?reference=is"
+    Then the response status code should be 200
+    And the response should include the published resources only without the draftResources key

src/Filter/OrSearchFilter.php

@@ -37,6 +37,11 @@ final class OrSearchFilter extends AbstractFilter implements SearchFilterInterfa
 
     public const DOCTRINE_INTEGER_TYPE = Types::INTEGER;
 
+    /**
+     * @var list<string>
+     */
+    private array $orExpressions = [];
+
     public function __construct(
         ManagerRegistry $managerRegistry,
         IriConverterInterface $iriConverter,
@@ -51,6 +56,21 @@ public function __construct(
         $this->propertyAccessor = $propertyAccessor ?: PropertyAccess::createPropertyAccessor();
     }
 
+    public function apply(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, ?Operation $operation = null, array $context = []): void
+    {
+        $this->orExpressions = [];
+
+        try {
+            parent::apply($queryBuilder, $queryNameGenerator, $resourceClass, $operation, $context);
+
+            if ($this->orExpressions) {
+                $queryBuilder->andWhere($queryBuilder->expr()->orX(...$this->orExpressions));
+            }
+        } finally {
+            $this->orExpressions = [];
+        }
+    }
+
     /**
      * {@inheritdoc}
      */
@@ -182,75 +202,19 @@ protected function addWhereByStrategy(string $strategy, QueryBuilder $queryBuild
     {
         $wrapCase = $this->createWrapCase($caseSensitive);
         $valueParameter = $queryNameGenerator->generateParameterName($field);
-        switch ($strategy) {
-            case null:
-            case self::STRATEGY_EXACT:
-                if (\is_array($value)) {
-                    foreach ($value as $i => $v) {
-                        $queryBuilder
-                            ->orWhere(\sprintf($wrapCase('%s.%s') . ' = ' . $wrapCase(':%s'), $alias, $field, $valueParameter . $i))
-                            ->setParameter($valueParameter . $i, $v);
-                    }
-                } else {
-                    $queryBuilder
-                        ->orWhere(\sprintf($wrapCase('%s.%s') . ' = ' . $wrapCase(':%s'), $alias, $field, $valueParameter))
-                        ->setParameter($valueParameter, $value);
-                }
-                break;
-            case self::STRATEGY_PARTIAL:
-                if (\is_array($value)) {
-                    foreach ($value as $i => $v) {
-                        $queryBuilder
-                            ->orWhere(\sprintf($wrapCase('%s.%s') . ' LIKE ' . $wrapCase('CONCAT(\'%%\', :%s, \'%%\')'), $alias, $field, $valueParameter . $i))
-                            ->setParameter($valueParameter . $i, $v);
-                    }
-                } else {
-                    $queryBuilder
-                        ->orWhere(\sprintf($wrapCase('%s.%s') . ' LIKE ' . $wrapCase('CONCAT(\'%%\', :%s, \'%%\')'), $alias, $field, $valueParameter))
-                        ->setParameter($valueParameter, $value);
-                }
-                break;
-            case self::STRATEGY_START:
-                if (\is_array($value)) {
-                    foreach ($value as $i => $v) {
-                        $queryBuilder
-                            ->orWhere(\sprintf($wrapCase('%s.%s') . ' LIKE ' . $wrapCase('CONCAT(:%s, \'%%\')'), $alias, $field, $valueParameter . $i))
-                            ->setParameter($valueParameter . $i, $v);
-                    }
-                } else {
-                    $queryBuilder
-                        ->orWhere(\sprintf($wrapCase('%s.%s') . ' LIKE ' . $wrapCase('CONCAT(:%s, \'%%\')'), $alias, $field, $valueParameter))
-                        ->setParameter($valueParameter, $value);
-                }
-                break;
-            case self::STRATEGY_END:
-                if (\is_array($value)) {
-                    foreach ($value as $i => $v) {
-                        $queryBuilder
-                            ->orWhere(\sprintf($wrapCase('%s.%s') . ' LIKE ' . $wrapCase('CONCAT(\'%%\', :%s)'), $alias, $field, $valueParameter . $i))
-                            ->setParameter($valueParameter . $i, $v);
-                    }
-                } else {
-                    $queryBuilder
-                        ->orWhere(\sprintf($wrapCase('%s.%s') . ' LIKE ' . $wrapCase('CONCAT(\'%%\', :%s)'), $alias, $field, $valueParameter))
-                        ->setParameter($valueParameter, $value);
-                }
-                break;
-            case self::STRATEGY_WORD_START:
-                if (\is_array($value)) {
-                    foreach ($value as $i => $v) {
-                        $queryBuilder
-                            ->orWhere(\sprintf($wrapCase('%1$s.%2$s') . ' LIKE ' . $wrapCase('CONCAT(:%3$s, \'%%\')') . ' OR ' . $wrapCase('%1$s.%2$s') . ' LIKE ' . $wrapCase('CONCAT(\'%% \', :%3$s, \'%%\')'), $alias, $field, $valueParameter . $i))
-                            ->setParameter($valueParameter . $i, $v);
-                    }
-                } else {
-                    $queryBuilder
-                        ->orWhere(\sprintf($wrapCase('%1$s.%2$s') . ' LIKE ' . $wrapCase('CONCAT(:%3$s, \'%%\')') . ' OR ' . $wrapCase('%1$s.%2$s') . ' LIKE ' . $wrapCase('CONCAT(\'%% \', :%3$s, \'%%\')'), $alias, $field, $valueParameter))
-                        ->setParameter($valueParameter, $value);
-                }
-                break;
-            default:
-                throw new InvalidArgumentException(\sprintf('strategy %s does not exist.', $strategy));
+
+        $format = match ($strategy) {
+            null, self::STRATEGY_EXACT => $wrapCase('%1$s.%2$s') . ' = ' . $wrapCase(':%3$s'),
+            self::STRATEGY_PARTIAL => $wrapCase('%1$s.%2$s') . ' LIKE ' . $wrapCase('CONCAT(\'%%\', :%3$s, \'%%\')'),
+            self::STRATEGY_START => $wrapCase('%1$s.%2$s') . ' LIKE ' . $wrapCase('CONCAT(:%3$s, \'%%\')'),
+            self::STRATEGY_END => $wrapCase('%1$s.%2$s') . ' LIKE ' . $wrapCase('CONCAT(\'%%\', :%3$s)'),
+            self::STRATEGY_WORD_START => $wrapCase('%1$s.%2$s') . ' LIKE ' . $wrapCase('CONCAT(:%3$s, \'%%\')') . ' OR ' . $wrapCase('%1$s.%2$s') . ' LIKE ' . $wrapCase('CONCAT(\'%% \', :%3$s, \'%%\')'),
+            default => throw new InvalidArgumentException(\sprintf('strategy %s does not exist.', $strategy)),
+        };
+
+        foreach ((array) $value as $i => $v) {
+            $this->orExpressions[] = \sprintf($format, $alias, $field, $valueParameter . $i);
+            $queryBuilder->setParameter($valueParameter . $i, $v);
         }
     }
 }