From 31eb004eb5cbe4cfe9c185dd536840630f4d5f7a Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Sun, 16 Apr 2023 11:52:34 +0100 Subject: [PATCH 1/7] fix: filter values are not validated Signed-off-by: Sami Mazouz --- extensions/likes/src/Query/LikedByFilter.php | 7 +- .../lock/src/Query/LockedFilterGambit.php | 2 +- .../mentions/src/Filter/MentionedFilter.php | 7 +- .../sticky/src/Query/StickyFilterGambit.php | 2 +- .../src/Query/SubscriptionFilterGambit.php | 7 +- .../src/Query/SuspendedFilterGambit.php | 2 +- extensions/tags/src/Filter/PostTagFilter.php | 9 +- extensions/tags/src/Query/TagFilterGambit.php | 7 +- framework/core/locale/core.yml | 4 + .../Api/Controller/ListPostsController.php | 5 +- .../Discussion/Query/AuthorFilterGambit.php | 13 ++- .../Discussion/Query/CreatedFilterGambit.php | 7 +- .../Discussion/Query/HiddenFilterGambit.php | 2 +- .../Discussion/Query/UnreadFilterGambit.php | 2 +- framework/core/src/Filter/FilterInterface.php | 2 + .../core/src/Filter/ValidateFilterTrait.php | 90 +++++++++++++++++++ .../core/src/Group/Filter/HiddenFilter.php | 9 +- framework/core/src/Http/Filter/UserFilter.php | 7 +- .../core/src/Post/Filter/AuthorFilter.php | 11 ++- .../core/src/Post/Filter/DiscussionFilter.php | 7 +- framework/core/src/Post/Filter/IdFilter.php | 8 +- .../core/src/Post/Filter/NumberFilter.php | 7 +- framework/core/src/Post/Filter/TypeFilter.php | 7 +- .../core/src/User/Query/EmailFilterGambit.php | 7 +- .../core/src/User/Query/GroupFilterGambit.php | 11 +-- framework/core/src/User/UserRepository.php | 7 ++ 26 files changed, 201 insertions(+), 48 deletions(-) create mode 100644 framework/core/src/Filter/ValidateFilterTrait.php diff --git a/extensions/likes/src/Query/LikedByFilter.php b/extensions/likes/src/Query/LikedByFilter.php index 09207473ad..5952e63fa7 100644 --- a/extensions/likes/src/Query/LikedByFilter.php +++ b/extensions/likes/src/Query/LikedByFilter.php @@ -11,17 +11,20 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; class LikedByFilter implements FilterInterface { + use ValidateFilterTrait; + public function getFilterKey(): string { return 'likedBy'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { - $likedId = trim($filterValue, '"'); + $likedId = $this->asInt($filterValue); $filterState ->getQuery() diff --git a/extensions/lock/src/Query/LockedFilterGambit.php b/extensions/lock/src/Query/LockedFilterGambit.php index 30caa38ca6..42abbfddd2 100644 --- a/extensions/lock/src/Query/LockedFilterGambit.php +++ b/extensions/lock/src/Query/LockedFilterGambit.php @@ -32,7 +32,7 @@ public function getFilterKey(): string return 'locked'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { $this->constrain($filterState->getQuery(), $negate); } diff --git a/extensions/mentions/src/Filter/MentionedFilter.php b/extensions/mentions/src/Filter/MentionedFilter.php index de7edd58ba..5652da21bc 100644 --- a/extensions/mentions/src/Filter/MentionedFilter.php +++ b/extensions/mentions/src/Filter/MentionedFilter.php @@ -11,17 +11,20 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; class MentionedFilter implements FilterInterface { + use ValidateFilterTrait; + public function getFilterKey(): string { return 'mentioned'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { - $mentionedId = trim($filterValue, '"'); + $mentionedId = $this->asInt($filterValue); $filterState ->getQuery() diff --git a/extensions/sticky/src/Query/StickyFilterGambit.php b/extensions/sticky/src/Query/StickyFilterGambit.php index a52b0a41a1..5ce083e085 100644 --- a/extensions/sticky/src/Query/StickyFilterGambit.php +++ b/extensions/sticky/src/Query/StickyFilterGambit.php @@ -32,7 +32,7 @@ public function getFilterKey(): string return 'sticky'; } - public function filter(FilterState $filterState, string $filterValue, $negate) + public function filter(FilterState $filterState, $filterValue, $negate) { $this->constrain($filterState->getQuery(), $negate); } diff --git a/extensions/subscriptions/src/Query/SubscriptionFilterGambit.php b/extensions/subscriptions/src/Query/SubscriptionFilterGambit.php index 447a31d83e..b94ddfc5c8 100644 --- a/extensions/subscriptions/src/Query/SubscriptionFilterGambit.php +++ b/extensions/subscriptions/src/Query/SubscriptionFilterGambit.php @@ -11,6 +11,7 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; use Flarum\Search\AbstractRegexGambit; use Flarum\Search\SearchState; use Flarum\User\User; @@ -18,6 +19,8 @@ class SubscriptionFilterGambit extends AbstractRegexGambit implements FilterInterface { + use ValidateFilterTrait; + protected function getGambitPattern() { return 'is:(follow|ignor)(?:ing|ed)'; @@ -33,8 +36,10 @@ public function getFilterKey(): string return 'subscription'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { + $filterValue = $this->asString($filterValue); + preg_match('/^'.$this->getGambitPattern().'$/i', 'is:'.$filterValue, $matches); $this->constrain($filterState->getQuery(), $filterState->getActor(), $matches[1], $negate); diff --git a/extensions/suspend/src/Query/SuspendedFilterGambit.php b/extensions/suspend/src/Query/SuspendedFilterGambit.php index ebb2771131..ae33ded2d7 100644 --- a/extensions/suspend/src/Query/SuspendedFilterGambit.php +++ b/extensions/suspend/src/Query/SuspendedFilterGambit.php @@ -63,7 +63,7 @@ public function getFilterKey(): string return 'suspended'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { if (! $filterState->getActor()->can('suspend', new Guest())) { return false; diff --git a/extensions/tags/src/Filter/PostTagFilter.php b/extensions/tags/src/Filter/PostTagFilter.php index 5b80ed614f..c2854a7661 100644 --- a/extensions/tags/src/Filter/PostTagFilter.php +++ b/extensions/tags/src/Filter/PostTagFilter.php @@ -11,18 +11,23 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; class PostTagFilter implements FilterInterface { + use ValidateFilterTrait; + public function getFilterKey(): string { return 'tag'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { + $ids = $this->asIntArray($filterValue); + $filterState->getQuery() ->join('discussion_tag', 'discussion_tag.discussion_id', '=', 'posts.discussion_id') - ->where('discussion_tag.tag_id', $negate ? '!=' : '=', $filterValue); + ->where('discussion_tag.tag_id', $negate ? '!=' : '=', $ids); } } diff --git a/extensions/tags/src/Query/TagFilterGambit.php b/extensions/tags/src/Query/TagFilterGambit.php index 6d8a6719e8..4cbc4b6ce2 100644 --- a/extensions/tags/src/Query/TagFilterGambit.php +++ b/extensions/tags/src/Query/TagFilterGambit.php @@ -11,6 +11,7 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; use Flarum\Http\SlugManager; use Flarum\Search\AbstractRegexGambit; use Flarum\Search\SearchState; @@ -21,6 +22,8 @@ class TagFilterGambit extends AbstractRegexGambit implements FilterInterface { + use ValidateFilterTrait; + /** * @var SlugManager */ @@ -46,14 +49,14 @@ public function getFilterKey(): string return 'tag'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { $this->constrain($filterState->getQuery(), $filterValue, $negate, $filterState->getActor()); } protected function constrain(Builder $query, $rawSlugs, $negate, User $actor) { - $slugs = explode(',', trim($rawSlugs, '"')); + $slugs = $this->asArray($rawSlugs); $query->where(function (Builder $query) use ($slugs, $negate, $actor) { foreach ($slugs as $slug) { diff --git a/framework/core/locale/core.yml b/framework/core/locale/core.yml index c35d0964e8..966314eb31 100644 --- a/framework/core/locale/core.yml +++ b/framework/core/locale/core.yml @@ -715,6 +715,10 @@ core: # Translations in this namespace are used in messages output by the API. api: invalid_username_message: "The username may only contain letters, numbers, and dashes." + invalid_filter_type: + must_be_numeric_message: "The {filter} filter must be numeric." + must_not_be_array_message: "The {filter} filter must not be an array." + must_not_be_multidimensional_array_message: "The {filter} filter must not be a multidimensional array." # Translations in this namespace are used in emails sent by the forum. email: diff --git a/framework/core/src/Api/Controller/ListPostsController.php b/framework/core/src/Api/Controller/ListPostsController.php index f38cd71abe..3cf5c4b68a 100644 --- a/framework/core/src/Api/Controller/ListPostsController.php +++ b/framework/core/src/Api/Controller/ListPostsController.php @@ -10,6 +10,7 @@ namespace Flarum\Api\Controller; use Flarum\Api\Serializer\PostSerializer; +use Flarum\Discussion\Discussion; use Flarum\Http\RequestUtil; use Flarum\Http\UrlGenerator; use Flarum\Post\Filter\PostFilterer; @@ -145,7 +146,9 @@ protected function extractOffset(ServerRequestInterface $request) ); } - $offset = $this->posts->getIndexForNumber($filter['discussion'], $near, $actor); + $discussionId = Discussion::findOrFail((int) $filter['discussion'])->id; + + $offset = $this->posts->getIndexForNumber($discussionId, $near, $actor); return max(0, $offset - $limit / 2); } diff --git a/framework/core/src/Discussion/Query/AuthorFilterGambit.php b/framework/core/src/Discussion/Query/AuthorFilterGambit.php index 7f4426c7b6..703c947826 100644 --- a/framework/core/src/Discussion/Query/AuthorFilterGambit.php +++ b/framework/core/src/Discussion/Query/AuthorFilterGambit.php @@ -11,6 +11,7 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; use Flarum\Search\AbstractRegexGambit; use Flarum\Search\SearchState; use Flarum\User\UserRepository; @@ -18,6 +19,8 @@ class AuthorFilterGambit extends AbstractRegexGambit implements FilterInterface { + use ValidateFilterTrait; + /** * @var \Flarum\User\UserRepository */ @@ -52,20 +55,16 @@ public function getFilterKey(): string return 'author'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { $this->constrain($filterState->getQuery(), $filterValue, $negate); } protected function constrain(Builder $query, $rawUsernames, $negate) { - $usernames = trim($rawUsernames, '"'); - $usernames = explode(',', $usernames); + $usernames = $this->asArray($rawUsernames); - $ids = []; - foreach ($usernames as $username) { - $ids[] = $this->users->getIdForUsername($username); - } + $ids = $this->users->getIdsForUsernames($usernames); $query->whereIn('discussions.user_id', $ids, 'and', $negate); } diff --git a/framework/core/src/Discussion/Query/CreatedFilterGambit.php b/framework/core/src/Discussion/Query/CreatedFilterGambit.php index 66bd1d69b8..ed2198be38 100644 --- a/framework/core/src/Discussion/Query/CreatedFilterGambit.php +++ b/framework/core/src/Discussion/Query/CreatedFilterGambit.php @@ -11,6 +11,7 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; use Flarum\Search\AbstractRegexGambit; use Flarum\Search\SearchState; use Illuminate\Database\Query\Builder; @@ -18,6 +19,8 @@ class CreatedFilterGambit extends AbstractRegexGambit implements FilterInterface { + use ValidateFilterTrait; + /** * {@inheritdoc} */ @@ -39,8 +42,10 @@ public function getFilterKey(): string return 'created'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { + $filterValue = $this->asString($filterValue); + preg_match('/^'.$this->getGambitPattern().'$/i', 'created:'.$filterValue, $matches); $this->constrain($filterState->getQuery(), Arr::get($matches, 1), Arr::get($matches, 3), $negate); diff --git a/framework/core/src/Discussion/Query/HiddenFilterGambit.php b/framework/core/src/Discussion/Query/HiddenFilterGambit.php index 2072428d9a..b56af32987 100644 --- a/framework/core/src/Discussion/Query/HiddenFilterGambit.php +++ b/framework/core/src/Discussion/Query/HiddenFilterGambit.php @@ -38,7 +38,7 @@ public function getFilterKey(): string return 'hidden'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { $this->constrain($filterState->getQuery(), $negate); } diff --git a/framework/core/src/Discussion/Query/UnreadFilterGambit.php b/framework/core/src/Discussion/Query/UnreadFilterGambit.php index b995b2c74b..0423bda583 100644 --- a/framework/core/src/Discussion/Query/UnreadFilterGambit.php +++ b/framework/core/src/Discussion/Query/UnreadFilterGambit.php @@ -53,7 +53,7 @@ public function getFilterKey(): string return 'unread'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { $this->constrain($filterState->getQuery(), $filterState->getActor(), $negate); } diff --git a/framework/core/src/Filter/FilterInterface.php b/framework/core/src/Filter/FilterInterface.php index edfdf5ce8a..4411826cca 100644 --- a/framework/core/src/Filter/FilterInterface.php +++ b/framework/core/src/Filter/FilterInterface.php @@ -18,6 +18,8 @@ public function getFilterKey(): string; /** * Filters a query. + * + * @todo: 2.0 change the $filterValue type to mixed, as it can be an array. */ public function filter(FilterState $filterState, string $filterValue, bool $negate); } diff --git a/framework/core/src/Filter/ValidateFilterTrait.php b/framework/core/src/Filter/ValidateFilterTrait.php new file mode 100644 index 0000000000..924b979557 --- /dev/null +++ b/framework/core/src/Filter/ValidateFilterTrait.php @@ -0,0 +1,90 @@ +throwValidationException('core.api.invalid_filter_type.must_not_be_multidimensional_array_message'); + } + + return trim($subValue, '"'); + }, $filterValue); + } else { + $value = explode(',', trim($filterValue, '"')); + } + + return $value; + } + + /** + * @throws FlarumValidationException + */ + protected function asString($filterValue): string + { + if (is_array($filterValue)) { + $this->throwValidationException('core.api.invalid_filter_type.must_not_be_array_message'); + } + + return trim($filterValue, '"'); + } + + /** + * @throws FlarumValidationException + */ + protected function asInt($filterValue): int + { + if (! is_numeric($filterValue)) { + $this->throwValidationException('core.api.invalid_filter_type.must_be_numeric_message'); + } + + return (int) $this->asString($filterValue); + } + + /** + * @throws FlarumValidationException + */ + protected function asIntArray($filterValue): array + { + return array_map(function ($value) { + return $this->asInt($value); + }, $this->asArray($filterValue)); + } + + /** + * @throws FlarumValidationException + */ + protected function asBool($filterValue): bool + { + return $this->asString($filterValue) === '1'; + } + + /** + * @throws FlarumValidationException + */ + private function throwValidationException(string $messageCode): void + { + $translator = resolve(Translator::class); + + throw new FlarumValidationException([ + 'message' => $translator->trans($messageCode, ['{filter}' => $this->getFilterKey()]), + ]); + } +} diff --git a/framework/core/src/Group/Filter/HiddenFilter.php b/framework/core/src/Group/Filter/HiddenFilter.php index 653c50655d..e811f45e85 100644 --- a/framework/core/src/Group/Filter/HiddenFilter.php +++ b/framework/core/src/Group/Filter/HiddenFilter.php @@ -11,16 +11,21 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; class HiddenFilter implements FilterInterface { + use ValidateFilterTrait; + public function getFilterKey(): string { return 'hidden'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { - $filterState->getQuery()->where('is_hidden', $negate ? '!=' : '=', $filterValue); + $hidden = $this->asBool($filterValue); + + $filterState->getQuery()->where('is_hidden', $negate ? '!=' : '=', $hidden); } } diff --git a/framework/core/src/Http/Filter/UserFilter.php b/framework/core/src/Http/Filter/UserFilter.php index d206fbaf97..12268042ab 100644 --- a/framework/core/src/Http/Filter/UserFilter.php +++ b/framework/core/src/Http/Filter/UserFilter.php @@ -12,6 +12,7 @@ use Flarum\Api\Controller\ListAccessTokensController; use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; /** * Filters an access tokens request by the related user. @@ -20,6 +21,8 @@ */ class UserFilter implements FilterInterface { + use ValidateFilterTrait; + /** * @inheritDoc */ @@ -31,8 +34,10 @@ public function getFilterKey(): string /** * @inheritDoc */ - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { + $filterValue = $this->asInt($filterValue); + $filterState->getQuery()->where('user_id', $negate ? '!=' : '=', $filterValue); } } diff --git a/framework/core/src/Post/Filter/AuthorFilter.php b/framework/core/src/Post/Filter/AuthorFilter.php index 493db3da90..398789d08c 100644 --- a/framework/core/src/Post/Filter/AuthorFilter.php +++ b/framework/core/src/Post/Filter/AuthorFilter.php @@ -11,18 +11,18 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; use Flarum\User\UserRepository; class AuthorFilter implements FilterInterface { + use ValidateFilterTrait; + /** * @var \Flarum\User\UserRepository */ protected $users; - /** - * @param \Flarum\User\UserRepository $users - */ public function __construct(UserRepository $users) { $this->users = $users; @@ -33,10 +33,9 @@ public function getFilterKey(): string return 'author'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { - $usernames = trim($filterValue, '"'); - $usernames = explode(',', $usernames); + $usernames = $this->asArray($filterValue); $ids = $this->users->query()->whereIn('username', $usernames)->pluck('id'); diff --git a/framework/core/src/Post/Filter/DiscussionFilter.php b/framework/core/src/Post/Filter/DiscussionFilter.php index 9ebb809a1f..c10436bcef 100644 --- a/framework/core/src/Post/Filter/DiscussionFilter.php +++ b/framework/core/src/Post/Filter/DiscussionFilter.php @@ -11,17 +11,20 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; class DiscussionFilter implements FilterInterface { + use ValidateFilterTrait; + public function getFilterKey(): string { return 'discussion'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { - $discussionId = trim($filterValue, '"'); + $discussionId = $this->asInt($filterValue); $filterState->getQuery()->where('posts.discussion_id', $negate ? '!=' : '=', $discussionId); } diff --git a/framework/core/src/Post/Filter/IdFilter.php b/framework/core/src/Post/Filter/IdFilter.php index 9835e51130..f9928adc4d 100644 --- a/framework/core/src/Post/Filter/IdFilter.php +++ b/framework/core/src/Post/Filter/IdFilter.php @@ -11,18 +11,20 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; class IdFilter implements FilterInterface { + use ValidateFilterTrait; + public function getFilterKey(): string { return 'id'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { - $idString = trim($filterValue, '"'); - $ids = explode(',', $idString); + $ids = $this->asIntArray($filterValue); $filterState->getQuery()->whereIn('posts.id', $ids, 'and', $negate); } diff --git a/framework/core/src/Post/Filter/NumberFilter.php b/framework/core/src/Post/Filter/NumberFilter.php index 8b82fe4e3a..e2dff477b9 100644 --- a/framework/core/src/Post/Filter/NumberFilter.php +++ b/framework/core/src/Post/Filter/NumberFilter.php @@ -11,17 +11,20 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; class NumberFilter implements FilterInterface { + use ValidateFilterTrait; + public function getFilterKey(): string { return 'number'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { - $number = trim($filterValue, '"'); + $number = $this->asInt($filterValue); $filterState->getQuery()->where('posts.number', $negate ? '!=' : '=', $number); } diff --git a/framework/core/src/Post/Filter/TypeFilter.php b/framework/core/src/Post/Filter/TypeFilter.php index 64a02d79c9..a3120bd910 100644 --- a/framework/core/src/Post/Filter/TypeFilter.php +++ b/framework/core/src/Post/Filter/TypeFilter.php @@ -11,17 +11,20 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; class TypeFilter implements FilterInterface { + use ValidateFilterTrait; + public function getFilterKey(): string { return 'type'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { - $type = trim($filterValue, '"'); + $type = $this->asString($filterValue); $filterState->getQuery()->where('posts.type', $negate ? '!=' : '=', $type); } diff --git a/framework/core/src/User/Query/EmailFilterGambit.php b/framework/core/src/User/Query/EmailFilterGambit.php index 0da13ce5d6..bf65f6b8af 100644 --- a/framework/core/src/User/Query/EmailFilterGambit.php +++ b/framework/core/src/User/Query/EmailFilterGambit.php @@ -11,12 +11,15 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; use Flarum\Search\AbstractRegexGambit; use Flarum\Search\SearchState; use Illuminate\Database\Query\Builder; class EmailFilterGambit extends AbstractRegexGambit implements FilterInterface { + use ValidateFilterTrait; + /** * {@inheritdoc} */ @@ -50,7 +53,7 @@ public function getFilterKey(): string return 'email'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { if (! $filterState->getActor()->hasPermission('user.edit')) { return; @@ -61,7 +64,7 @@ public function filter(FilterState $filterState, string $filterValue, bool $nega protected function constrain(Builder $query, $rawEmail, bool $negate) { - $email = trim($rawEmail, '"'); + $email = $this->asString($rawEmail); $query->where('email', $negate ? '!=' : '=', $email); } diff --git a/framework/core/src/User/Query/GroupFilterGambit.php b/framework/core/src/User/Query/GroupFilterGambit.php index 4dbb7f875e..b864bbc547 100644 --- a/framework/core/src/User/Query/GroupFilterGambit.php +++ b/framework/core/src/User/Query/GroupFilterGambit.php @@ -11,6 +11,7 @@ use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Filter\ValidateFilterTrait; use Flarum\Group\Group; use Flarum\Search\AbstractRegexGambit; use Flarum\Search\SearchState; @@ -19,6 +20,8 @@ class GroupFilterGambit extends AbstractRegexGambit implements FilterInterface { + use ValidateFilterTrait; + /** * {@inheritdoc} */ @@ -40,15 +43,13 @@ public function getFilterKey(): string return 'group'; } - public function filter(FilterState $filterState, string $filterValue, bool $negate) + public function filter(FilterState $filterState, $filterValue, bool $negate) { - $this->constrain($filterState->getQuery(), $filterState->getActor(), $filterValue, $negate); + $this->constrain($filterState->getQuery(), $filterState->getActor(), $this->asArray($filterValue), $negate); } - protected function constrain(Builder $query, User $actor, string $rawQuery, bool $negate) + protected function constrain(Builder $query, User $actor, array $groupIdentifiers, bool $negate) { - $groupIdentifiers = explode(',', trim($rawQuery, '"')); - $groupQuery = Group::whereVisibleTo($actor); $ids = []; diff --git a/framework/core/src/User/UserRepository.php b/framework/core/src/User/UserRepository.php index 46df8ecf66..b0c1e965ef 100644 --- a/framework/core/src/User/UserRepository.php +++ b/framework/core/src/User/UserRepository.php @@ -95,6 +95,13 @@ public function getIdForUsername($username, User $actor = null) return $this->scopeVisibleTo($query, $actor)->value('id'); } + public function getIdsForUsernames(array $usernames, User $actor = null): array + { + $query = $this->query()->whereIn('username', $usernames); + + return $this->scopeVisibleTo($query, $actor)->pluck('id')->all(); + } + /** * Find users by matching a string of words against their username, * optionally making sure they are visible to a certain user. From a97729be52ef78a8a18ae258a7a3e5bdb93afda8 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Sun, 16 Apr 2023 12:04:03 +0100 Subject: [PATCH 2/7] fix(regression): group gambit broken Signed-off-by: Sami Mazouz --- framework/core/src/User/Query/GroupFilterGambit.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/framework/core/src/User/Query/GroupFilterGambit.php b/framework/core/src/User/Query/GroupFilterGambit.php index b864bbc547..4f56c7c84c 100644 --- a/framework/core/src/User/Query/GroupFilterGambit.php +++ b/framework/core/src/User/Query/GroupFilterGambit.php @@ -45,11 +45,12 @@ public function getFilterKey(): string public function filter(FilterState $filterState, $filterValue, bool $negate) { - $this->constrain($filterState->getQuery(), $filterState->getActor(), $this->asArray($filterValue), $negate); + $this->constrain($filterState->getQuery(), $filterState->getActor(), $filterValue, $negate); } - protected function constrain(Builder $query, User $actor, array $groupIdentifiers, bool $negate) + protected function constrain(Builder $query, User $actor, $rawQuery, bool $negate) { + $groupIdentifiers = $this->asArray($rawQuery); $groupQuery = Group::whereVisibleTo($actor); $ids = []; From 00fad1322cc99e76e1f2532bf15f233a52345ac4 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Sun, 16 Apr 2023 20:34:03 +0100 Subject: [PATCH 3/7] chore(review): improve code Signed-off-by: Sami Mazouz --- extensions/tags/src/Query/TagFilterGambit.php | 2 +- .../core/src/Discussion/Query/AuthorFilterGambit.php | 2 +- framework/core/src/Filter/ValidateFilterTrait.php | 12 +++++++----- framework/core/src/Post/Filter/AuthorFilter.php | 2 +- framework/core/src/User/Query/GroupFilterGambit.php | 2 +- 5 files changed, 11 insertions(+), 9 deletions(-) diff --git a/extensions/tags/src/Query/TagFilterGambit.php b/extensions/tags/src/Query/TagFilterGambit.php index 4cbc4b6ce2..294d4924a3 100644 --- a/extensions/tags/src/Query/TagFilterGambit.php +++ b/extensions/tags/src/Query/TagFilterGambit.php @@ -56,7 +56,7 @@ public function filter(FilterState $filterState, $filterValue, bool $negate) protected function constrain(Builder $query, $rawSlugs, $negate, User $actor) { - $slugs = $this->asArray($rawSlugs); + $slugs = $this->asStringArray($rawSlugs); $query->where(function (Builder $query) use ($slugs, $negate, $actor) { foreach ($slugs as $slug) { diff --git a/framework/core/src/Discussion/Query/AuthorFilterGambit.php b/framework/core/src/Discussion/Query/AuthorFilterGambit.php index 703c947826..d1008604ff 100644 --- a/framework/core/src/Discussion/Query/AuthorFilterGambit.php +++ b/framework/core/src/Discussion/Query/AuthorFilterGambit.php @@ -62,7 +62,7 @@ public function filter(FilterState $filterState, $filterValue, bool $negate) protected function constrain(Builder $query, $rawUsernames, $negate) { - $usernames = $this->asArray($rawUsernames); + $usernames = $this->asStringArray($rawUsernames); $ids = $this->users->getIdsForUsernames($usernames); diff --git a/framework/core/src/Filter/ValidateFilterTrait.php b/framework/core/src/Filter/ValidateFilterTrait.php index 924b979557..80886d07e9 100644 --- a/framework/core/src/Filter/ValidateFilterTrait.php +++ b/framework/core/src/Filter/ValidateFilterTrait.php @@ -17,18 +17,20 @@ trait ValidateFilterTrait /** * @throws FlarumValidationException */ - protected function asArray($filterValue, $multidimensional = false): array + protected function asStringArray($filterValue, bool $multidimensional = false): array { if (is_array($filterValue)) { $value = array_map(function ($subValue) use ($multidimensional) { if (is_array($subValue) && ! $multidimensional) { $this->throwValidationException('core.api.invalid_filter_type.must_not_be_multidimensional_array_message'); + } elseif (is_array($subValue)) { + return $this->asStringArray($subValue, true); + } else { + return $this->asString($subValue); } - - return trim($subValue, '"'); }, $filterValue); } else { - $value = explode(',', trim($filterValue, '"')); + $value = explode(',', $this->asString($filterValue)); } return $value; @@ -65,7 +67,7 @@ protected function asIntArray($filterValue): array { return array_map(function ($value) { return $this->asInt($value); - }, $this->asArray($filterValue)); + }, $this->asStringArray($filterValue)); } /** diff --git a/framework/core/src/Post/Filter/AuthorFilter.php b/framework/core/src/Post/Filter/AuthorFilter.php index 398789d08c..761969e294 100644 --- a/framework/core/src/Post/Filter/AuthorFilter.php +++ b/framework/core/src/Post/Filter/AuthorFilter.php @@ -35,7 +35,7 @@ public function getFilterKey(): string public function filter(FilterState $filterState, $filterValue, bool $negate) { - $usernames = $this->asArray($filterValue); + $usernames = $this->asStringArray($filterValue); $ids = $this->users->query()->whereIn('username', $usernames)->pluck('id'); diff --git a/framework/core/src/User/Query/GroupFilterGambit.php b/framework/core/src/User/Query/GroupFilterGambit.php index 4f56c7c84c..a511c2daa1 100644 --- a/framework/core/src/User/Query/GroupFilterGambit.php +++ b/framework/core/src/User/Query/GroupFilterGambit.php @@ -50,7 +50,7 @@ public function filter(FilterState $filterState, $filterValue, bool $negate) protected function constrain(Builder $query, User $actor, $rawQuery, bool $negate) { - $groupIdentifiers = $this->asArray($rawQuery); + $groupIdentifiers = $this->asStringArray($rawQuery); $groupQuery = Group::whereVisibleTo($actor); $ids = []; From cfb5c66a88eac6a7dfb1088383ad2c83e5e70b56 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Sun, 16 Apr 2023 20:38:39 +0100 Subject: [PATCH 4/7] chore(review): return a `0` offset rather than quitting Signed-off-by: Sami Mazouz --- .../Api/Controller/ListPostsController.php | 5 +-- framework/core/src/Post/PostRepository.php | 37 +++++++++++-------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/framework/core/src/Api/Controller/ListPostsController.php b/framework/core/src/Api/Controller/ListPostsController.php index 3cf5c4b68a..ef12729c9d 100644 --- a/framework/core/src/Api/Controller/ListPostsController.php +++ b/framework/core/src/Api/Controller/ListPostsController.php @@ -10,7 +10,6 @@ namespace Flarum\Api\Controller; use Flarum\Api\Serializer\PostSerializer; -use Flarum\Discussion\Discussion; use Flarum\Http\RequestUtil; use Flarum\Http\UrlGenerator; use Flarum\Post\Filter\PostFilterer; @@ -146,9 +145,7 @@ protected function extractOffset(ServerRequestInterface $request) ); } - $discussionId = Discussion::findOrFail((int) $filter['discussion'])->id; - - $offset = $this->posts->getIndexForNumber($discussionId, $near, $actor); + $offset = $this->posts->getIndexForNumber((int) $filter['discussion'], $near, $actor); return max(0, $offset - $limit / 2); } diff --git a/framework/core/src/Post/PostRepository.php b/framework/core/src/Post/PostRepository.php index 5d03ce0622..3d56c87808 100644 --- a/framework/core/src/Post/PostRepository.php +++ b/framework/core/src/Post/PostRepository.php @@ -105,22 +105,27 @@ public function filterVisibleIds(array $ids, User $actor) */ public function getIndexForNumber($discussionId, $number, User $actor = null) { - $query = Discussion::find($discussionId) - ->posts() - ->whereVisibleTo($actor) - ->where('created_at', '<', function ($query) use ($discussionId, $number) { - $query->select('created_at') - ->from('posts') - ->where('discussion_id', $discussionId) - ->whereNotNull('number') - ->take(1) - - // We don't add $number as a binding because for some - // reason doing so makes the bindings go out of order. - ->orderByRaw('ABS(CAST(number AS SIGNED) - '.(int) $number.')'); - }); - - return $query->count(); + $discussion = Discussion::find($discussionId); + + if ($discussion) { + $query = $discussion->posts() + ->whereVisibleTo($actor) + ->where('created_at', '<', function ($query) use ($discussionId, $number) { + $query->select('created_at') + ->from('posts') + ->where('discussion_id', $discussionId) + ->whereNotNull('number') + ->take(1) + + // We don't add $number as a binding because for some + // reason doing so makes the bindings go out of order. + ->orderByRaw('ABS(CAST(number AS SIGNED) - '.(int) $number.')'); + }); + + return $query->count(); + } + + return 0; } /** From 1ae4ea2dcf80877a3612faad591ef327927dd739 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Sun, 16 Apr 2023 21:36:37 +0100 Subject: [PATCH 5/7] chore: phpstan return types Signed-off-by: Sami Mazouz --- framework/core/src/Filter/ValidateFilterTrait.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/framework/core/src/Filter/ValidateFilterTrait.php b/framework/core/src/Filter/ValidateFilterTrait.php index 80886d07e9..ae8ac42c2c 100644 --- a/framework/core/src/Filter/ValidateFilterTrait.php +++ b/framework/core/src/Filter/ValidateFilterTrait.php @@ -16,6 +16,7 @@ trait ValidateFilterTrait { /** * @throws FlarumValidationException + * @return array|array */ protected function asStringArray($filterValue, bool $multidimensional = false): array { @@ -62,6 +63,7 @@ protected function asInt($filterValue): int /** * @throws FlarumValidationException + * @return array */ protected function asIntArray($filterValue): array { From c0b4addd9a4b597f2aa40c72c87b5d0d65f05cf7 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Mon, 24 Apr 2023 09:37:20 +0100 Subject: [PATCH 6/7] feat: filtering posts per many tags Signed-off-by: Sami Mazouz --- extensions/tags/src/Filter/PostTagFilter.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/tags/src/Filter/PostTagFilter.php b/extensions/tags/src/Filter/PostTagFilter.php index c2854a7661..7501fedfc7 100644 --- a/extensions/tags/src/Filter/PostTagFilter.php +++ b/extensions/tags/src/Filter/PostTagFilter.php @@ -28,6 +28,6 @@ public function filter(FilterState $filterState, $filterValue, bool $negate) $filterState->getQuery() ->join('discussion_tag', 'discussion_tag.discussion_id', '=', 'posts.discussion_id') - ->where('discussion_tag.tag_id', $negate ? '!=' : '=', $ids); + ->whereIn('discussion_tag.tag_id', $ids, 'and', $negate); } } From b97ae968054ff6ae0a827abeda42a2081ba7be42 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Mon, 24 Apr 2023 09:39:53 +0100 Subject: [PATCH 7/7] chore: avoid nesting Signed-off-by: Sami Mazouz --- framework/core/src/Post/PostRepository.php | 36 ++++++++++------------ 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/framework/core/src/Post/PostRepository.php b/framework/core/src/Post/PostRepository.php index 3d56c87808..11ad9f3085 100644 --- a/framework/core/src/Post/PostRepository.php +++ b/framework/core/src/Post/PostRepository.php @@ -105,27 +105,25 @@ public function filterVisibleIds(array $ids, User $actor) */ public function getIndexForNumber($discussionId, $number, User $actor = null) { - $discussion = Discussion::find($discussionId); - - if ($discussion) { - $query = $discussion->posts() - ->whereVisibleTo($actor) - ->where('created_at', '<', function ($query) use ($discussionId, $number) { - $query->select('created_at') - ->from('posts') - ->where('discussion_id', $discussionId) - ->whereNotNull('number') - ->take(1) - - // We don't add $number as a binding because for some - // reason doing so makes the bindings go out of order. - ->orderByRaw('ABS(CAST(number AS SIGNED) - '.(int) $number.')'); - }); - - return $query->count(); + if (! ($discussion = Discussion::find($discussionId))) { + return 0; } - return 0; + $query = $discussion->posts() + ->whereVisibleTo($actor) + ->where('created_at', '<', function ($query) use ($discussionId, $number) { + $query->select('created_at') + ->from('posts') + ->where('discussion_id', $discussionId) + ->whereNotNull('number') + ->take(1) + + // We don't add $number as a binding because for some + // reason doing so makes the bindings go out of order. + ->orderByRaw('ABS(CAST(number AS SIGNED) - '.(int) $number.')'); + }); + + return $query->count(); } /**