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

reset_password API to always return success #5284

Merged
merged 2 commits into from
Jan 2, 2025
Merged

Conversation

anhcuky
Copy link
Contributor

@anhcuky anhcuky commented Dec 23, 2024

No description provided.

@phiresky
Copy link
Collaborator

Theoretically (for security), it would be better if this endpoint returned success regardless of registration state (with undistinguishable timing). The corresponding UI text as is common in other websites would be "If this email address has a registered account, it has been sent a password reset email."

Otherwise, as is, we have an email enumeration vulnerability. As in, you can find registered users by calling this endpoint and collecting success responses, which you can then use to target those users specifically using leaked password lists.

@anhcuky
Copy link
Contributor Author

anhcuky commented Dec 24, 2024

@phiresky Thanks, your suggestion aligns with my concern in #5277. (I closed it because I thought it was unnecessary, lol). Will make this change in this PR

@dessalines
Copy link
Member

We should also probably just remove the CouldntFindEmail error.

@SleeplessOne1917
Copy link
Member

Seconding Dessalines on removing the CouldntFindEmail error. Best to make it so that an attacker can't deduce whether an email or username is in use by looking at responses.

@anhcuky anhcuky changed the title Fix misleading error type in password reset reset_password API to always return success Dec 25, 2024
@anhcuky
Copy link
Contributor Author

anhcuky commented Dec 25, 2024

Updated this PR

Copy link
Collaborator

@dullbananas dullbananas left a comment

Choose a reason for hiding this comment

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

Since errors when sending the email are now ignored, the text shown in lemmy-ui should suggest trying again later if nothing happens

crates/api/src/local_user/reset_password.rs Outdated Show resolved Hide resolved
crates/api/src/local_user/reset_password.rs Show resolved Hide resolved

async fn try_reset_password(email: &str, context: &LemmyContext) -> LemmyResult<()> {
Copy link
Member

@dessalines dessalines Dec 27, 2024

Choose a reason for hiding this comment

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

Should be moved to utils like all the other helper functions. And you should add a function comment that it always returns Ok, and probably link to the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's a little better to keep the reset_password and try_reset_password functions together

@dessalines
Copy link
Member

You also need to remove the CouldntFindEmail error.

@dessalines dessalines merged commit c034229 into LemmyNet:main Jan 2, 2025
1 check passed
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.

5 participants