OR filter bypasses all doctrine extensions -> Potential security problem

[yobottehg]

First of all thanks for this great module project, it helps us a lot.

One thing you should perhaps note in the readme is that the per default activated OR filter can bypass all existing doctrine extensions for the collection endpoints.

So if you have an extension which adds where X = 1 to the collection query you can add or WHERE X = 2 with this filter for all already activated filters.

My suggestion would be to either deactivate the OR filter as a default to not have other run into this potential problem this easily or at least add a big warning in the readme regarding the security problem.

[metaclass-nl]

Auw, that is defenetly a problem! Thanks for bringing it to my attention!

I would say that the problem will only occur if the extensions are run before the built in FilterExtension. If they run afterwards them calling andWhere to add their criteria to the QueryBuilder will basically place the entire query from the FilterExtension between brackets and then AND its own criteria.

https://api-platform.com/docs/core/extensions/ states: "Note that your extensions should have a positive priority if defined. Internal extensions have negative priorities". api_platform.doctrine.orm.query_extension.filter has priority -16. The higher the number, the earlier the tagged service will be located in the collection. So i guess custom extionsions will run before the built in ones. It may be possible to influence that by configuring a priority from the config but this may interfere with the working of the eager loading extensions and IMHO it can only be a temporary workaround.

When i first built the FilterLogic i wondered wheather to implicitly AND all criteria produced by it, or to reflect the upmost and and or directly into andWhere and orWhere calls. I chose the latter because that allows for more flexibility for combining with existing filters, but did not think of other extensions being called before the FilterExtension. Now i guess i will have to switch the first solution. It will still be possible to use OR but only by nesting the criteria in the query string inside or, all criteria that are not nested in and, or or not (and thus are added by the other filters directly) will be combined with AND.

Downside of this solution is that it will change the behavior of exising apps that are using LogicFilter. This makes it backwards incompatible so a new major version will be required.

Developing this will take some time. For the time being a warning will be added to the existing FilterBundles readme.

[metaclass-nl]

The docs of API Platform about making a custom ORM Filter do not warn about this either. They simply ignore the possibility to use orWhere instead of andWhere, but because it is left to the Filters themselves to call andWhere, IMHO it should be expected that some will call orWhere instead. Maybe we should make another issue at API Platform that their docs should warn about this too.

[metaclass-nl]

The innerJoinsLeft option may also cause a security problem if the working of any of the extionsions
used relies on INNER JOINS selecting only rows with NOT NULL values. Similar for AddFakeLeftJoin with extensions that use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryBuilderHelper::addJoinOnce
or ApiPlatform\Core\Bridge\Doctrine\Orm\PropertyHelperTrait::addJoinsForNestedProperty,
they may not have been intended to generate left joins instead of inner joins. I added warnings about both to the readme.

[benblub]

good to know. i disabled the or condition with some kind of decorator pattern. Disabling options in this bundle would be nice. I need to take a look into the inner joins combined with not null.

[metaclass-nl]

This issue is solved in v2.0.0.rc1 but it changes the behavior with OR. Your client may have to change the queries they send to get the same results.

A workaround is in v1.0.2 but that does throw an Exception if it can not solve the problem.

[benblub]

We are using/testing v2.0.0.rc1. Thanks for your commitment.