fix: filter values are not validated (#3795)

flarum · May 7, 2023 · 9363682 · 9363682
1 parent c766881
commit 9363682
Show file tree

Hide file tree

Showing 27 changed files with 215 additions and 57 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -11,13 +11,16 @@
 
 use Flarum\Filter\FilterInterface;
 use Flarum\Filter\FilterState;
+use Flarum\Filter\ValidateFilterTrait;
 use Flarum\Search\AbstractRegexGambit;
 use Flarum\Search\SearchState;
 use Flarum\User\User;
 use Illuminate\Database\Query\Builder;
 
 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
@@ -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
@@ -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);
+            ->whereIn('discussion_tag.tag_id', $ids, 'and', $negate);
     }
 }
diff --git 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->asStringArray($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
@@ -717,6 +717,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
@@ -145,7 +145,7 @@ protected function extractOffset(ServerRequestInterface $request)
                 );
             }
 
-            $offset = $this->posts->getIndexForNumber($filter['discussion'], $near, $actor);
+            $offset = $this->posts->getIndexForNumber((int) $filter['discussion'], $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
@@ -11,13 +11,16 @@
 
 use Flarum\Filter\FilterInterface;
 use Flarum\Filter\FilterState;
+use Flarum\Filter\ValidateFilterTrait;
 use Flarum\Search\AbstractRegexGambit;
 use Flarum\Search\SearchState;
 use Flarum\User\UserRepository;
 use Illuminate\Database\Query\Builder;
 
 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->asStringArray($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
@@ -11,13 +11,16 @@
 
 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;
 use Illuminate\Support\Arr;
 
 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
@@ -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
@@ -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
@@ -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
@@ -0,0 +1,94 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Filter;
+
+use Flarum\Foundation\ValidationException as FlarumValidationException;
+use Flarum\Locale\Translator;
+
+trait ValidateFilterTrait
+{
+    /**
+     * @throws FlarumValidationException
+     * @return array<string>|array<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);
+                }
+            }, $filterValue);
+        } else {
+            $value = explode(',', $this->asString($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
+     * @return array<int>
+     */
+    protected function asIntArray($filterValue): array
+    {
+        return array_map(function ($value) {
+            return $this->asInt($value);
+        }, $this->asStringArray($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
@@ -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);
     }
 }
-Original file line number
+Diff line change
@@ Expand Up @@
                     );
                 }
-                $offset = $this->posts->getIndexForNumber($filter['discussion'], $near, $actor);
+                $offset = $this->posts->getIndexForNumber((int) $filter['discussion'], $near, $actor);
                 return max(0, $offset - $limit / 2);
             }
@@ Expand Down @@