Repository: lldap/lldap (language: rust)

Issue:
[BUG] password reset: mail server error discloses user email
**Describe the bug**
An error while sending the password reset email discloses the email address associated with a user name.

![Screenshot 2024-03-29 123448](https://github.com/lldap/lldap/assets/3942997/be25caa9-e034-49f9-947b-325c3acee770)

**To Reproduce**
Setup the mail server to reject the email and start the password reset sequence.

**Expected behavior**
Sensitive user data should not be exposed.

**Additional context**
v0.5.0 docker image (lldap/lldap:stable)


Discussion / hints:
Hello! I just started using this project, and am fairly fluent in Rust. I would like to take this one on, but I'm curious what you see as an acceptable solution. Should we redact the email specifically from the error message or hide it entirely, instead logging it to a file? My vote would be for hiding the error message entirely. I'd rather have errors explicitly whitelisted before being logged to the client, as I'm sure others may disclose sensitive information as well.
Hey! Welcome to the project! Feel free to join the discord and ask for help there if you need. 

For this issue, I think it makes sense to "sanitize" the error sent to the frontend (e.g. "error sending the confirmation email"), and log the full error on the server.
I think the existence of a username or email address shouldn't be disclosed at all, e.g. "if we have a user with this name / email address, a password reset email has been sent. Please check your spam folder as well.". No matter if it was sent, or couldn't be sent, or if there was such a user at all. Everything else should go to the log.

If the request is legit and the email doesn't arrive, the user has to contact admin anyway, no matter where the delivery problem was.

Also, it might need checking if the account is still enabled and such (don't know if disabling account has made into development).
Yeah, I guess it makes sense. It's not ideal to have a failure without feedback, but given that it's an unauthorized entry point I think it makes sense. 

While we're at it, we should also include a sleep(3s) before returning an error, otherwise the timing is too suspect.

Fix patch:
diff --git a/server/src/infra/mail.rs b/server/src/infra/mail.rs
index 6bccf04f..d1c67d0f 100644
--- a/server/src/infra/mail.rs
+++ b/server/src/infra/mail.rs
@@ -4,6 +4,8 @@ use lettre::{
     message::Mailbox, transport::smtp::authentication::Credentials, AsyncSmtpTransport,
     AsyncTransport, Message, Tokio1Executor,
 };
+use std::time::Duration;
+use tokio::time::sleep;
 use tracing::debug;
 
 async fn send_email(
@@ -58,11 +60,16 @@ async fn send_email(
     }
 
     if let Err(e) = mailer.port(options.port).build().send(email).await {
-        if e.to_string().contains("CorruptMessage") {
-            Err(anyhow!("CorruptMessage returned by lettre, this usually means the SMTP encryption setting is wrong.").context(e))
-        } else {
-            Err(e.into())
-        }
+        debug!("Error sending email: {:?}", e);
+        Err(anyhow!(
+            "{}: SMTP error {}",
+            if e.to_string().contains("CorruptMessage") {
+                "CorruptMessage returned by lettre, this usually means the SMTP encryption setting is wrong"
+            } else {
+                "Error sending email"
+            },
+            e.status().map(|s| s.to_string()).unwrap_or_default()
+        ))
     } else {
         Ok(())
     }
@@ -92,14 +99,18 @@ To reset your password please visit the following URL: {}
 Please contact an administrator if you did not initiate the process.",
         username, reset_url
     );
-    send_email(
+    let res = send_email(
         to,
         "[LLDAP] Password reset requested",
         body,
         options,
         server_url,
     )
-    .await
+    .await;
+    if res.is_err() {
+        sleep(Duration::from_secs(3)).await;
+    }
+    res
 }
 
 pub async fn send_test_email(to: Mailbox, options: &MailOptions) -> Result<()> {


Test patch: