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

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Apr 16, 2023

Fixes #3627

Changes proposed in this pull request:
This PR along with a tag v0.5.2 in SychO9/json-api-php@728047e fixes a lack of validation for query parameters that would lead to multiple server errors.

Reviewers should focus on:
I'm surprised we never noticed the lack of validation for filters, that said the interface forces a string type, whereas a filter can be an array, unfortunately, we can only change that in 2.0, but at least, implementations of the interface can do without the string type.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9 SychO9 requested a review from a team as a code owner April 16, 2023 11:00
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
/**
* @throws FlarumValidationException
*/
protected function asArray($filterValue, $multidimensional = false): array
Copy link
Member

Choose a reason for hiding this comment

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

Should these require string? / why not add type annotations?

Copy link
Member Author

Choose a reason for hiding this comment

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

filterValue is either a string or an array, w can't use mixed and we can't use sting|array yet.

The second parameter does need to be typed though.

framework/core/src/Filter/ValidateFilterTrait.php Outdated Show resolved Hide resolved
framework/core/src/Filter/ValidateFilterTrait.php Outdated Show resolved Hide resolved
framework/core/src/Api/Controller/ListPostsController.php Outdated Show resolved Hide resolved
SychO9 added 2 commits April 16, 2023 20:34
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9 SychO9 requested a review from askvortsov1 April 16, 2023 19:40
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@imorland imorland added this to the 1.8 milestone Apr 22, 2023
Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Another question, sorry. If we're removing the string type annotation for $filterValue, would it be possible to specify a union type in the doc block so we still get some kind of typechecking?

$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.

framework/core/src/Post/PostRepository.php Outdated Show resolved Hide resolved
SychO9 added 2 commits April 24, 2023 09:37
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9
Copy link
Member Author

SychO9 commented Apr 24, 2023

Another question, sorry. If we're removing the string type annotation for $filterValue, would it be possible to specify a union type in the doc block so we still get some kind of typechecking?

We can't add a more open type than the code type string s that is incorrect so phpstan errors with it. Unless you meant doing so on every filter implementation, in wich case I don't think it would actually be useful since the interface is what's picked up when dynamically applying each individual filter.

@SychO9 SychO9 merged commit 9363682 into main May 7, 2023
@SychO9 SychO9 deleted the sm/3627-some-query-param-validation branch May 7, 2023 17:37
@github-actions github-actions bot mentioned this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Insufficient request validation - some bots generates fatal errors
3 participants