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

Add RangeValidator for take paramater #2169

Closed
wants to merge 1 commit into from

Conversation

bnematzadeh
Copy link
Contributor

Description

In the user.get.js file, no validator has been implemented for the take field. A client could send a request to the following endpoint and set takes to a large value, which could lead to reduced performance and a potential DoS attack on the application. I have added a new validator called RangeError.

In all controllers, it’s better to check that the value provided by the client does not exceed a certain range before passing this parameter to the database. For this purpose, I wrote two different tests: one for when the takes value is out of range, and another for when it is within the valid range.

if (optionsWithDefault.take) {
    if (optionsWithDefault.take > DEFAULT_OPTIONS.limit) {
      throw new RangeError('The value of the limit parameter is out of range');
    }
    queryParams.limit = optionsWithDefault.take;
  }

Copy link

codecov bot commented Nov 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.52%. Comparing base (216f637) to head (071e253).
Report is 19 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2169      +/-   ##
==========================================
+ Coverage   98.50%   98.52%   +0.02%     
==========================================
  Files         867      867              
  Lines       14243    14277      +34     
==========================================
+ Hits        14030    14067      +37     
+ Misses        213      210       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

relativeci bot commented Nov 10, 2024

#2867 Bundle Size — 10.26MiB (0%).

071e253(current) vs 2540927 master#2858(baseline)

Warning

Bundle contains 3 duplicate packages – View duplicate packages

Bundle metrics  no changes
                 Current
#2867
     Baseline
#2858
No change  Initial JS 5.56MiB 5.56MiB
No change  Initial CSS 304.68KiB 304.68KiB
Change  Cache Invalidation 0% 54.06%
No change  Chunks 51 51
No change  Assets 171 171
No change  Modules 1495 1495
No change  Duplicate Modules 21 21
No change  Duplicate Code 0.84% 0.84%
No change  Packages 124 124
No change  Duplicate Packages 3 3
Bundle size by type  no changes
                 Current
#2867
     Baseline
#2858
No change  JS 7.35MiB 7.35MiB
No change  IMG 2.48MiB 2.48MiB
No change  CSS 321.47KiB 321.47KiB
No change  Fonts 93.55KiB 93.55KiB
No change  Other 17.62KiB 17.62KiB
No change  HTML 13.58KiB 13.58KiB

Bundle analysis reportBranch bnematzadeh:gladys-sec-4Project dashboard


Generated by RelativeCIDocumentationReport issue

@Pierre-Gilles
Copy link
Contributor

@bnematzadeh Why not, but it's a bit of cherrypicking in the case of Gladys :) Don't forget that Gladys is a self-hosted/local software, and when you talk about the "client", it's the same person that started Gladys server, so if the "client" is asking for a big ?take, the user could ddos... his own server! :D

@bnematzadeh
Copy link
Contributor Author

@bnematzadeh Why not, but it's a bit of cherrypicking in the case of Gladys :) Don't forget that Gladys is a self-hosted/local software, and when you talk about the "client", it's the same person that started Gladys server, so if the "client" is asking for a big ?take, the user could ddos... his own server! :D

You're right. In any case, this can also be added to the code to ensure the parameter is sent within a specific range. Overall, I agree with you.

@Pierre-Gilles
Copy link
Contributor

Pierre-Gilles commented Nov 11, 2024

In any case, this can also be added to the code to ensure the parameter is sent within a specific range.

Yes, but I don't think it's worth it to put it everywhere.

For example, the "GET user" should return only a few rows, as most people have from 1 to 5 users at home (depending on how many people are living in this home), so I don't think this route is a good candidate.

Routes with millions of rows are better candidates, but I still think that in some case it could be useful to query millions of rows.

For example, the other day, I wanted to know the minimum temperature recorded during this summer on my outside temperature sensor, so I asked for the last 200k rows, and I received them without any issues (it was 4.6Mb, retrieved in 500ms).

If there was a low limit, I couldn't have done it

@Pierre-Gilles
Copy link
Contributor

@bnematzadeh So what so you think?

@Pierre-Gilles
Copy link
Contributor

I'm closing this, don't hesitate if you have any other feedback :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants