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

Specify a maximum number of runs to limit the request params -> DoS. #191

Merged
merged 1 commit into from
May 24, 2024

Conversation

ioquatix
Copy link
Member

No description provided.

@Sim4n6
Copy link

Sim4n6 commented Apr 29, 2024

suppose i set maximum_runs = profiler_runs+1 and profiler_runs = 999_999_999 ?

@ioquatix
Copy link
Member Author

It's your own fault if you use this code incorrectly. I also think we should document that should avoid using this in production or in any environment where a denial of service would cause an outage.

Copy link

@Sim4n6 Sim4n6 left a comment

Choose a reason for hiding this comment

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

LGTM

lib/rack/contrib/profiler.rb Show resolved Hide resolved
Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

I prefer clamp, or at least something similar that sets both a low and high bound. What are your thoughts?

@mpalmer
Copy link
Contributor

mpalmer commented May 20, 2024

I guess in the interests of preventing people from trying to do "potato" profile runs, doing profiler_runs.to_i.clamp(1, @maximum_runs) would be advisable, but I wouldn't make it a blocker for merge. The important thing, for this PR, is to prevent the DoS, which the current implementation does.

@Sim4n6
Copy link

Sim4n6 commented May 23, 2024

Meanwhile, could you please consider enabling the private vulnerability reporting feature for this repo?

@mpalmer
Copy link
Contributor

mpalmer commented May 24, 2024

I've enabled that, @Sim4n6, and there's also the option to go through Tidelift for vulnerability disclosure, per the README.

@ioquatix ioquatix merged commit 0eec2a9 into main May 24, 2024
30 checks passed
@ioquatix ioquatix deleted the rack-profiler-maximum branch May 24, 2024 00:48
@ioquatix ioquatix added this to the v2.5.0 milestone May 24, 2024
@ioquatix
Copy link
Member Author

Okay, I released it, thanks everyone!

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.

4 participants