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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding a captcha rate limit. Fixes #1755 #1941

Merged
merged 2 commits into from Nov 25, 2021
Merged

Adding a captcha rate limit. Fixes #1755 #1941

merged 2 commits into from Nov 25, 2021

Conversation

dessalines
Copy link
Member

No description provided.

// Handle captcha separately
web::resource("/user/get_captcha")
.wrap(rate_limit.image())
.route(web::get().to(route_get::<GetCaptcha>)),
Copy link
Member

Choose a reason for hiding this comment

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

This is the rate limit for image uploads, which is 6 per hour by default. Seems a bit low if someone requests a new captcha a few times in a row, and fails a few. Maybe the message rate limit would work.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was using the message rate limit before, check out these problems: https://git.radicallyopensecurity.com/nlnet/off-ngid-lemmy/-/issues/14

Copy link
Member

Choose a reason for hiding this comment

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

Then i think it needs a separate limit. Maybe reusing the post/comment limit (6 in 10 minutes) could work, but without burst it also seems a bit low. Plus it would be really confusing to use that setting for something completely different.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought image made sense, because it is an image being generated. But I'd be fine with the post limit, I'll change it to that.

Copy link
Member

Choose a reason for hiding this comment

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

Its not a great solution, because then a user might be unable to post if they requested a new captcha a couple times. I think it really needs a separate limit, i would say rougly 60 per hour?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its pry good enough for now, and if ppl run into this limit often, we could add another one to the config.

Copy link
Member

Choose a reason for hiding this comment

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

Alright. Federation tests are failing though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Its a yerba builder issue, they're passing on cloud drone: https://cloud.drone.io/LemmyNet/lemmy/2065

Copy link
Member

Choose a reason for hiding this comment

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

Weird.

@Nutomic Nutomic merged commit e765b42 into main Nov 25, 2021
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.

None yet

2 participants