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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26bd408
Adding a captcha rate limit. Fixes #1755
dessalines Nov 22, 2021
e0bf244
Changing to post rate limit.
dessalines Nov 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion crates/websocket/src/chat_server.rs
View file
Open in desktop
Expand Up @@ -491,7 +491,10 @@ impl ChatServer {
} else {
let user_operation = UserOperation::from_str(op)?;
let fut = (message_handler)(context, msg.id, user_operation.clone(), data);
rate_limiter.message().wrap(ip, fut).await
match user_operation {
UserOperation::GetCaptcha => rate_limiter.post().wrap(ip, fut).await,
_ => rate_limiter.message().wrap(ip, fut).await,
}
}
}
}
Expand Down
7 changes: 6 additions & 1 deletion src/api_routes.rs
View file
Open in desktop
Expand Up @@ -156,6 +156,12 @@ pub fn config(cfg: &mut web::ServiceConfig, rate_limit: &RateLimit) {
.wrap(rate_limit.register())
.route(web::post().to(route_post_crud::<Register>)),
)
.service(
// Handle captcha separately
web::resource("/user/get_captcha")
.wrap(rate_limit.post())
.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.

)
// User actions
.service(
web::scope("/user")
Expand All @@ -173,7 +179,6 @@ pub fn config(cfg: &mut web::ServiceConfig, rate_limit: &RateLimit) {
.route("/block", web::post().to(route_post::<BlockPerson>))
// Account actions. I don't like that they're in /user maybe /accounts
.route("/login", web::post().to(route_post::<Login>))
.route("/get_captcha", web::get().to(route_get::<GetCaptcha>))
.route(
"/delete_account",
web::post().to(route_post_crud::<DeleteAccount>),
Expand Down