From 94149b774aeb21dc43061308e7c969ce97e6a8a6 Mon Sep 17 00:00:00 2001 From: silverbackdan Date: Mon, 21 Sep 2026 14:15:33 +0100 Subject: [PATCH] AND the OrSearchFilter's clauses against the rest of the query (#237) addWhereByStrategy() built its clauses with orWhere(), which ORs against the entire accumulated WHERE rather than among the filter's own clauses. Query extensions default to priority 0 while FilterExtension is -16, so extensions add their predicates first and the filter ORed them away. A filter parameter is attacker-controlled, so the discarded predicate is a security one. Verified on main: anonymous GET /_/routes?path=launch returned a route scheduled for 2999, and a user without draft permission saw every draft in a filtered publishable collection. Clauses now accumulate into one Orx applied with a single andWhere(). The accumulator is shared across the per-field calls, since the method runs once per query parameter and building it per call would turn multi-field search into AND. Each strategy had a duplicate single-value branch that normalizeValues() made unreachable, so values are normalised to an array once and each strategy is a single format string. --- CLAUDE.md | 22 +++ features/bootstrap/DoctrineContext.php | 15 ++ features/main/or_search_filter.feature | 58 ++++++++ src/Filter/OrSearchFilter.php | 102 +++++-------- tests/Filter/OrSearchFilterTest.php | 138 ++++++++++++++++++ .../Entity/DummyPublishableComponent.php | 3 + 6 files changed, 269 insertions(+), 69 deletions(-) create mode 100644 features/main/or_search_filter.feature create mode 100644 tests/Filter/OrSearchFilterTest.php diff --git a/CLAUDE.md b/CLAUDE.md index 91dd1a33d..516c4d1c6 100644 --- a/CLAUDE.md +++ b/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. diff --git a/features/bootstrap/DoctrineContext.php b/features/bootstrap/DoctrineContext.php index b9e519da3..59f46937c 100644 --- a/features/bootstrap/DoctrineContext.php +++ b/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 */ diff --git a/features/main/or_search_filter.feature b/features/main/or_search_filter.feature new file mode 100644 index 000000000..2496ba152 --- /dev/null +++ b/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 diff --git a/src/Filter/OrSearchFilter.php b/src/Filter/OrSearchFilter.php index fb7730cae..3b610c97c 100644 --- a/src/Filter/OrSearchFilter.php +++ b/src/Filter/OrSearchFilter.php @@ -37,6 +37,11 @@ final class OrSearchFilter extends AbstractFilter implements SearchFilterInterfa public const DOCTRINE_INTEGER_TYPE = Types::INTEGER; + /** + * @var list + */ + 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); } } } diff --git a/tests/Filter/OrSearchFilterTest.php b/tests/Filter/OrSearchFilterTest.php new file mode 100644 index 000000000..e2217936b --- /dev/null +++ b/tests/Filter/OrSearchFilterTest.php @@ -0,0 +1,138 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Silverback\ApiComponentsBundle\Tests\Filter; + +use ApiPlatform\Doctrine\Orm\Util\QueryNameGenerator; +use ApiPlatform\Metadata\IriConverterInterface; +use Doctrine\ORM\EntityManagerInterface; +use Doctrine\ORM\Mapping\ClassMetadata; +use Doctrine\ORM\Query\Expr; +use Doctrine\ORM\QueryBuilder; +use Doctrine\Persistence\ManagerRegistry; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\TestCase; +use Silverback\ApiComponentsBundle\Filter\OrSearchFilter; +use Silverback\ApiComponentsBundle\Tests\Functional\TestBundle\Entity\DummyOrSearchFilterable; + +class OrSearchFilterTest extends TestCase +{ + private const EXISTING_PREDICATE = 'o.publishedAt IS NOT NULL'; + + private EntityManagerInterface $entityManager; + private ManagerRegistry $managerRegistry; + + protected function setUp(): void + { + $classMetadata = $this->createStub(ClassMetadata::class); + $classMetadata->method('hasField')->willReturnCallback(static fn (string $field): bool => \in_array($field, ['field1', 'field2'], true)); + $classMetadata->method('hasAssociation')->willReturn(false); + $classMetadata->method('getTypeOfField')->willReturn('string'); + + $this->entityManager = $this->createStub(EntityManagerInterface::class); + $this->entityManager->method('getClassMetadata')->willReturn($classMetadata); + $this->entityManager->method('getExpressionBuilder')->willReturn(new Expr()); + + $this->managerRegistry = $this->createStub(ManagerRegistry::class); + $this->managerRegistry->method('getManagerForClass')->willReturn($this->entityManager); + } + + public static function strategyProvider(): iterable + { + foreach (['exact', 'iexact', 'partial', 'ipartial', 'start', 'istart', 'end', 'iend', 'word_start', 'iword_start'] as $strategy) { + yield $strategy . ' with a single value' => [$strategy, 'alpha']; + yield $strategy . ' with multiple values' => [$strategy, ['alpha', 'gamma']]; + } + } + + #[DataProvider('strategyProvider')] + public function test_clauses_are_combined_with_or_among_themselves_and_and_against_the_query(string $strategy, string|array $value): void + { + $queryBuilder = $this->createQueryBuilder(); + $this->createFilter(['field1' => $strategy])->apply($queryBuilder, new QueryNameGenerator(), DummyOrSearchFilterable::class, null, ['filters' => ['field1' => $value]]); + + $dql = $queryBuilder->getDQL(); + + self::assertStringContainsString(self::EXISTING_PREDICATE . ' AND', $dql); + self::assertStringNotContainsString(self::EXISTING_PREDICATE . ' OR', $dql); + } + + public function test_clauses_for_different_fields_are_combined_with_or_within_a_single_and(): void + { + $queryBuilder = $this->createQueryBuilder(); + $this->createFilter(['field1' => 'exact', 'field2' => 'exact'])->apply($queryBuilder, new QueryNameGenerator(), DummyOrSearchFilterable::class, null, ['filters' => ['field1' => 'alpha', 'field2' => 'alpha']]); + + self::assertSame(self::EXISTING_PREDICATE . ' AND (o.field1 = :field1_p10 OR o.field2 = :field2_p20)', $this->getWhere($queryBuilder)); + } + + public function test_multiple_values_for_one_field_are_combined_with_or_within_a_single_and(): void + { + $queryBuilder = $this->createQueryBuilder(); + $this->createFilter(['field1' => 'exact'])->apply($queryBuilder, new QueryNameGenerator(), DummyOrSearchFilterable::class, null, ['filters' => ['field1' => ['alpha', 'gamma']]]); + + self::assertSame(self::EXISTING_PREDICATE . ' AND (o.field1 = :field1_p10 OR o.field1 = :field1_p11)', $this->getWhere($queryBuilder)); + } + + public function test_a_word_start_clause_is_parenthesised_so_it_cannot_capture_a_preceding_predicate(): void + { + $queryBuilder = $this->createQueryBuilder(); + $this->createFilter(['field1' => 'word_start'])->apply($queryBuilder, new QueryNameGenerator(), DummyOrSearchFilterable::class, null, ['filters' => ['field1' => 'alpha']]); + + self::assertSame(self::EXISTING_PREDICATE . " AND (o.field1 LIKE CONCAT(:field1_p10, '%') OR o.field1 LIKE CONCAT('% ', :field1_p10, '%'))", $this->getWhere($queryBuilder)); + } + + public function test_a_case_insensitive_partial_clause_ands_against_the_query(): void + { + $queryBuilder = $this->createQueryBuilder(); + $this->createFilter(['field1' => 'ipartial'])->apply($queryBuilder, new QueryNameGenerator(), DummyOrSearchFilterable::class, null, ['filters' => ['field1' => 'alpha']]); + + self::assertSame(self::EXISTING_PREDICATE . " AND LOWER(o.field1) LIKE LOWER(CONCAT('%', :field1_p10, '%'))", $this->getWhere($queryBuilder)); + } + + public function test_a_filter_with_no_matching_property_leaves_the_query_untouched(): void + { + $queryBuilder = $this->createQueryBuilder(); + $this->createFilter(['field1' => 'exact'])->apply($queryBuilder, new QueryNameGenerator(), DummyOrSearchFilterable::class, null, ['filters' => ['unmapped' => 'alpha']]); + + self::assertSame(self::EXISTING_PREDICATE, $this->getWhere($queryBuilder)); + } + + public function test_clauses_from_one_request_do_not_leak_into_the_next(): void + { + $filter = $this->createFilter(['field1' => 'exact']); + + $first = $this->createQueryBuilder(); + $filter->apply($first, new QueryNameGenerator(), DummyOrSearchFilterable::class, null, ['filters' => ['field1' => 'alpha']]); + + $second = $this->createQueryBuilder(); + $filter->apply($second, new QueryNameGenerator(), DummyOrSearchFilterable::class, null, ['filters' => ['field1' => 'gamma']]); + + self::assertSame(self::EXISTING_PREDICATE . ' AND o.field1 = :field1_p10', $this->getWhere($second)); + } + + private function createFilter(array $properties): OrSearchFilter + { + return new OrSearchFilter($this->managerRegistry, $this->createStub(IriConverterInterface::class), null, null, $properties); + } + + private function createQueryBuilder(): QueryBuilder + { + return (new QueryBuilder($this->entityManager)) + ->select('o') + ->from(DummyOrSearchFilterable::class, 'o') + ->andWhere(self::EXISTING_PREDICATE); + } + + private function getWhere(QueryBuilder $queryBuilder): string + { + return (string) $queryBuilder->getDQLPart('where'); + } +} diff --git a/tests/Functional/TestBundle/Entity/DummyPublishableComponent.php b/tests/Functional/TestBundle/Entity/DummyPublishableComponent.php index 0f7b6ed43..03f5658d5 100644 --- a/tests/Functional/TestBundle/Entity/DummyPublishableComponent.php +++ b/tests/Functional/TestBundle/Entity/DummyPublishableComponent.php @@ -11,17 +11,20 @@ namespace Silverback\ApiComponentsBundle\Tests\Functional\TestBundle\Entity; +use ApiPlatform\Metadata\ApiFilter; use ApiPlatform\Metadata\ApiResource; use Doctrine\ORM\Mapping as ORM; use Silverback\ApiComponentsBundle\Annotation as Silverback; use Silverback\ApiComponentsBundle\Entity\Core\AbstractComponent; use Silverback\ApiComponentsBundle\Entity\Utility\PublishableTrait; +use Silverback\ApiComponentsBundle\Filter\OrSearchFilter; /** * @author Daniel West */ #[Silverback\Publishable] #[ApiResource(mercure: true)] +#[ApiFilter(OrSearchFilter::class, properties: ['reference' => 'ipartial'])] #[ORM\Entity] class DummyPublishableComponent extends AbstractComponent {