Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: filter values are not validated #3795

Merged
merged 7 commits into from
May 7, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions extensions/likes/src/Query/LikedByFilter.php
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion extensions/lock/src/Query/LockedFilterGambit.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
7 changes: 5 additions & 2 deletions extensions/mentions/src/Filter/MentionedFilter.php
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion extensions/sticky/src/Query/StickyFilterGambit.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)';
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion extensions/suspend/src/Query/SuspendedFilterGambit.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
9 changes: 7 additions & 2 deletions extensions/tags/src/Filter/PostTagFilter.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is an intArray, shouldn't it be whereIn?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch! technically I should have kept it an int rather than an array<int>. But nothing wrong with allowing filtering per many tags.

}
}
7 changes: 5 additions & 2 deletions extensions/tags/src/Query/TagFilterGambit.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -21,6 +22,8 @@

class TagFilterGambit extends AbstractRegexGambit implements FilterInterface
{
use ValidateFilterTrait;

/**
* @var SlugManager
*/
Expand All @@ -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) {
Expand Down
4 changes: 4 additions & 0 deletions framework/core/locale/core.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
13 changes: 6 additions & 7 deletions framework/core/src/Discussion/Query/AuthorFilterGambit.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}
*/
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
2 changes: 2 additions & 0 deletions framework/core/src/Filter/FilterInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
94 changes: 94 additions & 0 deletions framework/core/src/Filter/ValidateFilterTrait.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
<?php
SychO9 marked this conversation as resolved.
Show resolved Hide resolved

/*
* 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) {
SychO9 marked this conversation as resolved.
Show resolved Hide resolved
$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';
SychO9 marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* @throws FlarumValidationException
*/
private function throwValidationException(string $messageCode): void
{
$translator = resolve(Translator::class);

throw new FlarumValidationException([
'message' => $translator->trans($messageCode, ['{filter}' => $this->getFilterKey()]),
]);
}
}
9 changes: 7 additions & 2 deletions framework/core/src/Group/Filter/HiddenFilter.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Loading