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

Stricter rate limit for login #4718

Merged
merged 1 commit into from
May 15, 2024
Merged

Stricter rate limit for login #4718

merged 1 commit into from
May 15, 2024

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented May 14, 2024

Login only uses the message rate limit which allows a lot of requests, so an attacker can try to brute force it. Better to use the register rate limit which is much stricter.

.service(
// Handle /user/login separately to add the register() rate limiter
// TODO: pretty annoying way to apply rate limits for register and login, we should
// group them under a common path so that rate limit is only applied once (eg under /account).
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps that path could be called /auth and only have signup and login under it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or /account/auth, we can do that as part of #4428

@Nothing4You
Copy link
Contributor

I don't think it makes sense to group login and signup under the same rate limit.
Login rate limits are frequently desired to be much more lenient than signup rate limits.

A lot of the current rate limits are overly broad, is there a downside to separating them?

@Nutomic Nutomic removed this from the 0.19.4 milestone May 14, 2024
@Nutomic
Copy link
Member Author

Nutomic commented May 14, 2024

Adding another rate limit is possible, but much more complicated because it needs a db migration and then the setting also needs to be added to lemmy-ui. Generally I prefer sensible defaults for configuration, rather than countless tiny settings that need to be tuned to get the instance working well.

@dullbananas
Copy link
Collaborator

This can be a problem for users who don't stay logged in for long periods of time (e.g. when a public/shared device is used) if the instance allows something like 7 registrations in 14 days.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

I agree this specific rate limit could be a bit restrictive, but we can always add another one for the DB in the future. iirc there was another rate limit someone was requesting, but I'd have to look through the issues. Either way I'll make another issue for a login-specific rate limit.

@dessalines dessalines merged commit 49bb17b into main May 15, 2024
2 checks passed
@Nutomic Nutomic mentioned this pull request May 20, 2024
4 tasks
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

5 participants