Align API behaviour with upstream and return errors on malformed input (#7810)

- Attachments: reject a declared v2 attachment size above the upload limit,
  keep display sizes within the largest unit, and make download links valid
  for 1 minute like upstream.
- Ciphers: moving a personal cipher into an organization needs collection
  write access or full access, like upstream.
- Events: /events/collect only accepts the client event types upstream accepts.
- Client IP: for list headers like X-Forwarded-For, use the rightmost address
  that isn't in IP_HEADER_TRUSTED_PROXIES, reading every occurrence.
- Prelogin: unknown emails get stable KDF settings from upstream's list.
- Login with device: an auth request can be used for one login, like upstream.
- Directory Connector import: never revokes, restores or removes Owners,
  like upstream.
- SSO: check the PKCE verifier again when the code exchange is reused during
  2FA, and accept the same redirect URIs as upstream (desktop localhost
  callback for AppImage and dev builds, CLI on ports 8065-8070).
- Identity: send the merged master password policy in the 2FA response and
  use PascalCase policy keys in identity responses, like upstream.
  Organization API key login no longer needs device fields. Recovery code
  login normalises the code, uses upstream's error message, logs a failed 2FA
  event and sends a "two-step login recovered" email.
- Add GET /api/accounts/keys.
- Return errors instead of panicking on malformed event dates, cipher data,
  key rotation cipher ids, bulk confirm entries and YubiKey OTPs.

.env.template

@@ -338,6 +338,8 @@
 ## "local" accepts it from any non global address, which covers a reverse proxy running on the same
 ## host or container network. Use "all" to accept it from anywhere, or list the addresses of your
 ## proxy as IPs and CIDR ranges if it connects from a public address.
+## For a list header like X-Forwarded-For, the client IP is the rightmost address that isn't one of
+## these proxies, so with several proxies in a row (a CDN in front of nginx, for example) list all of them.
 # IP_HEADER_TRUSTED_PROXIES=local
 
 ## Icon service

src/api/core/accounts.rs

@@ -42,6 +42,7 @@ pub fn routes() -> Vec<rocket::Route> {
         post_profile,
         put_avatar,
         get_public_keys,
+        get_keys,
         post_keys,
         post_password,
         post_set_password,
@@ -568,6 +569,24 @@ async fn get_public_keys(user_id: UserId, _headers: Headers, conn: DbConn) -> Js
     })))
 }
 
+#[get("/accounts/keys")]
+fn get_keys(headers: Headers) -> JsonResult {
+    let user = headers.user;
+
+    // The SDK reads a 404 as the user having no key pair yet
+    if user.private_key.is_none() || user.public_key.is_none() {
+        err_code!("User has no key pair", Status::NotFound.code)
+    }
+
+    Ok(Json(json!({
+        "key": (!user.akey.is_empty()).then_some(&user.akey),
+        "publicKey": user.public_key,
+        "privateKey": user.private_key,
+        "accountKeys": user.account_keys_json(),
+        "object": "keys",
+    })))
+}
+
 #[post("/accounts/keys", data = "<data>")]
 async fn post_keys(data: Json<KeysData>, headers: Headers, conn: DbConn) -> JsonResult {
     let data: KeysData = data.into_inner();
@@ -994,7 +1013,8 @@ async fn post_rotatekey(data: Json<KeyData>, headers: Headers, conn: DbConn, nt:
     // Update cipher data
     for cipher_data in data.account_data.ciphers {
         if cipher_data.organization_id.is_none() {
-            let Some(saved_cipher) = existing_ciphers.iter_mut().find(|c| &c.uuid == cipher_data.id.as_ref().unwrap())
+            let Some(saved_cipher) =
+                cipher_data.id.as_ref().and_then(|id| existing_ciphers.iter_mut().find(|c| &c.uuid == id))
             else {
                 err!("Cipher doesn't exist")
             };
@@ -1360,14 +1380,33 @@ pub struct PreloginData {
     email: String,
 }
 
+/// Like upstream, an unknown email gets KDF settings picked from a list of common ones by a keyed hash of the email,
+/// so they stay the same between requests. The salt stays null, as for existing users.
+fn unknown_email_kdf(email: &str) -> (i32, i32, Option<i32>, Option<i32>) {
+    const PBKDF2: i32 = UserKdfType::Pbkdf2 as i32;
+    const ARGON2ID: i32 = UserKdfType::Argon2id as i32;
+    // Same list as upstream, the default is in it twice to give it more weight
+    const KDF_SETTINGS: [(i32, i32, Option<i32>, Option<i32>); 6] = [
+        (User::CLIENT_KDF_TYPE_DEFAULT, User::CLIENT_KDF_ITER_DEFAULT, None, None),
+        (User::CLIENT_KDF_TYPE_DEFAULT, User::CLIENT_KDF_ITER_DEFAULT, None, None),
+        (PBKDF2, 100_000, None, None),
+        (PBKDF2, 5_000, None, None),
+        (ARGON2ID, 3, Some(64), Some(4)),
+        (ARGON2ID, 6, Some(32), Some(4)),
+    ];
+    KDF_SETTINGS[crate::auth::prelogin_kdf_index(email, KDF_SETTINGS.len())]
+}
+
 pub async fn prelogin(data: Json<PreloginData>, ip: ClientIp, conn: DbConn) -> JsonResult {
     crate::ratelimit::check_limit_unauthenticated(&ip.ip)?;
 
     let data: PreloginData = data.into_inner();
+    // Normalized once, so the lookup and the settings for an unknown email use the same value
+    let email = data.email.trim().to_lowercase();
 
-    let (kdf_type, kdf_iter, kdf_mem, kdf_para) = match User::find_by_mail(&data.email, &conn).await {
+    let (kdf_type, kdf_iter, kdf_mem, kdf_para) = match User::find_by_mail(&email, &conn).await {
         Some(user) => (user.client_kdf_type, user.client_kdf_iter, user.client_kdf_memory, user.client_kdf_parallelism),
-        None => (User::CLIENT_KDF_TYPE_DEFAULT, User::CLIENT_KDF_ITER_DEFAULT, None, None),
+        None => unknown_email_kdf(&email),
     };
 
     Ok(Json(json!({

src/api/core/ciphers.rs

@@ -29,7 +29,7 @@ use crate::{
     util::{NumberOrString, deser_opt_nonempty_str, save_temp_file},
 };
 
-use super::folders::FolderData;
+use super::{folders::FolderData, sends::SIZE_525_MB};
 
 pub fn routes() -> Vec<Route> {
     // Note that many routes have an `admin` variant; this seems to be
@@ -441,10 +441,10 @@ pub async fn update_cipher_from_data(
     // We do not mind which data is in it, the keep our model more flexible when there are upstream changes.
     // But, we at least know we do not need to store and return this specific key.
     fn clean_cipher_data(mut json_data: Value) -> Value {
-        if json_data.is_array() {
-            json_data.as_array_mut().unwrap().iter_mut().for_each(|ref mut f| {
-                f.as_object_mut().unwrap().remove("response");
-            });
+        if let Some(items) = json_data.as_array_mut() {
+            for item in items.iter_mut().filter_map(Value::as_object_mut) {
+                item.remove("response");
+            }
         }
         json_data
     }
@@ -486,9 +486,10 @@ pub async fn update_cipher_from_data(
             Some(member) => {
                 // A non-empty list of collections implies the caller already validated the user's write
                 // access to them, so we can move the cipher into the organization on that basis.
+                // Write access to the cipher itself only counts when it already belongs to this organization.
                 if shared_to_collections.as_ref().is_some_and(|cols| !cols.is_empty())
                     || member.has_full_access()
-                    || cipher.is_write_accessible_to_user(&headers.user.uuid, conn).await
+                    || (!transfer_cipher && cipher.is_write_accessible_to_user(&headers.user.uuid, conn).await)
                 {
                     cipher.organization_uuid = Some(org_id);
                     // After some discussion in PR #1329 re-added the user_uuid = None again.
@@ -552,7 +553,9 @@ pub async fn update_cipher_from_data(
 
     let type_data = if let Some(mut data) = type_data_opt {
         // Remove the 'Response' key from the base object.
-        data.as_object_mut().unwrap().remove("response");
+        if let Some(data_obj) = data.as_object_mut() {
+            data_obj.remove("response");
+        }
         // Remove the 'Response' key from every Uri.
         if data["uris"].is_array() {
             data["uris"] = clean_cipher_data(data["uris"].clone());
@@ -1182,6 +1185,10 @@ async fn post_attachment_v2(
     if file_size < 0 {
         err!("Attachment size can't be negative")
     }
+    // Matches the "file" upload limit in main.rs, nothing larger can be uploaded
+    if file_size > SIZE_525_MB {
+        err!("Max file size is 525 MB.")
+    }
     let attachment_id = crypto::generate_attachment_id();
     let attachment =
         Attachment::new(attachment_id.clone(), cipher.uuid.clone(), data.file_name, file_size, Some(data.key));

src/api/core/emergency_access.rs

@@ -225,7 +225,7 @@ async fn send_invite(data: Json<EmergencyAccessInviteData>, headers: Headers, co
     let (grantee_user, new_user) = match User::find_by_mail(&email, &conn).await {
         None => {
             if !CONFIG.invitations_allowed() {
-                err!(format!("Grantee user does not exist: {email}"))
+                err!(format!("Grantee user does not exist: {}", email.escape_debug()))
             }
 
             if !CONFIG.is_email_domain_allowed(&email) {

src/api/core/events.rs

@@ -6,12 +6,13 @@ use serde_json::Value;
 
 use crate::{
     CONFIG,
-    api::{EmptyResult, JsonResult},
+    api::{ApiResult, EmptyResult, JsonResult},
     auth::{AdminHeaders, Headers},
     db::{
         DbConn, DbPool,
         models::{Cipher, CipherId, Event, EventType, Membership, MembershipId, OrganizationId, UserId},
     },
+    error::MapResult,
     util::parse_date,
 };
 
@@ -29,6 +30,14 @@ struct EventRange {
     continuation_token: Option<String>,
 }
 
+impl EventRange {
+    fn date_range(&self) -> ApiResult<(NaiveDateTime, NaiveDateTime)> {
+        let start_date = parse_date(&self.start).map_res("Invalid start date")?;
+        let end_date = parse_date(self.continuation_token.as_ref().unwrap_or(&self.end)).map_res("Invalid end date")?;
+        Ok((start_date, end_date))
+    }
+}
+
 // Upstream: https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Api/AdminConsole/Controllers/EventsController.cs#L87
 #[get("/organizations/<org_id>/events?<data..>")]
 async fn get_org_events(org_id: OrganizationId, data: EventRange, headers: AdminHeaders, conn: DbConn) -> JsonResult {
@@ -39,12 +48,7 @@ async fn get_org_events(org_id: OrganizationId, data: EventRange, headers: Admin
     // Return an empty vec when we org events are disabled.
     // This prevents client errors
     let events_json: Vec<Value> = if CONFIG.org_events_enabled() {
-        let start_date = parse_date(&data.start);
-        let end_date = if let Some(before_date) = &data.continuation_token {
-            parse_date(before_date)
-        } else {
-            parse_date(&data.end)
-        };
+        let (start_date, end_date) = data.date_range()?;
 
         Event::find_by_organization_uuid(&org_id, &start_date, &end_date, &conn)
             .await
@@ -69,12 +73,7 @@ async fn get_cipher_events(cipher_id: CipherId, data: EventRange, headers: Heade
     let events_json: Vec<Value> = if CONFIG.org_events_enabled()
         && Membership::user_has_ge_admin_access_to_cipher(&headers.user.uuid, &cipher_id, &conn).await
     {
-        let start_date = parse_date(&data.start);
-        let end_date = if let Some(before_date) = &data.continuation_token {
-            parse_date(before_date)
-        } else {
-            parse_date(&data.end)
-        };
+        let (start_date, end_date) = data.date_range()?;
 
         Event::find_by_cipher_uuid(&cipher_id, &start_date, &end_date, &conn).await.iter().map(Event::to_json).collect()
     } else {
@@ -102,12 +101,7 @@ async fn get_user_events(
     // Return an empty vec when we org events are disabled.
     // This prevents client errors
     let events_json: Vec<Value> = if CONFIG.org_events_enabled() {
-        let start_date = parse_date(&data.start);
-        let end_date = if let Some(before_date) = &data.continuation_token {
-            parse_date(before_date)
-        } else {
-            parse_date(&data.end)
-        };
+        let (start_date, end_date) = data.date_range()?;
 
         Event::find_by_org_and_member(&org_id, &member_id, &start_date, &end_date, &conn)
             .await
@@ -168,9 +162,10 @@ async fn post_events_collect(data: Json<Vec<EventCollection>>, headers: Headers,
     }
 
     for event in data.iter() {
-        let event_date = parse_date(&event.date);
+        let event_date = parse_date(&event.date).map_res("Invalid event date")?;
+        // Only the event types upstream accepts from clients, the rest are written server-side.
         match event.r#type {
-            1000..=1099 => {
+            t if t == EventType::UserClientExportedVault as i32 => {
                 log_user_event_impl(
                     event.r#type,
                     &headers.user.uuid,
@@ -181,7 +176,7 @@ async fn post_events_collect(data: Json<Vec<EventCollection>>, headers: Headers,
                 )
                 .await;
             }
-            1600..=1699 => {
+            t if t == EventType::OrganizationClientExportedVault as i32 => {
                 // Only allow logging events for an organization the user is actually a member of.
                 if let Some(org_id) = &event.organization_id
                     && Membership::find_confirmed_by_user_and_org(&headers.user.uuid, org_id, &conn).await.is_some()
@@ -219,7 +214,8 @@ async fn post_events_collect(data: Json<Vec<EventCollection>>, headers: Headers,
                     .await;
                 }
             }
-            _ => {
+            // Client-side cipher events (viewed, copied, toggled, autofilled).
+            1107..=1114 | 1117 | 1119..=1132 => {
                 // The cipher determines the organization the event is logged to, so make sure the
                 // user can actually access it instead of trusting the provided cipher uuid.
                 if let Some(cipher_uuid) = &event.cipher_id
@@ -240,6 +236,7 @@ async fn post_events_collect(data: Json<Vec<EventCollection>>, headers: Headers,
                     .await;
                 }
             }
+            _ => {}
         }
     }
     Ok(())

src/api/core/organizations.rs

@@ -1028,7 +1028,7 @@ async fn send_invite(
         let user = match User::find_by_mail(email, &conn).await {
             None => {
                 if !CONFIG.invitations_allowed() {
-                    err!(format!("User does not exist: {email}"))
+                    err!(format!("User does not exist: {}", email.escape_debug()))
                 }
 
                 if !CONFIG.is_email_domain_allowed(email) {
@@ -1046,7 +1046,7 @@ async fn send_invite(
             }
             Some(user) => {
                 if Membership::find_by_user_and_org(&user.uuid, &org_id, &conn).await.is_some() {
-                    err!(format!("User already in organization: {email}"))
+                    err!(format!("User already in organization: {}", email.escape_debug()))
                 }
 
                 if !CONFIG.mail_enabled() {
@@ -1321,7 +1321,9 @@ async fn bulk_confirm_invite(
     match data.keys {
         Some(keys) => {
             for invite in keys {
-                let member_id = invite.id.unwrap();
+                let Some(member_id) = invite.id else {
+                    err!("Member id is required")
+                };
                 let user_key = invite.key.unwrap_or_default();
                 let err_msg = match confirm_invite_impl(&org_id, &member_id, &user_key, &headers, &conn, &nt).await {
                     Ok(()) => String::new(),

src/api/core/public.rs

@@ -58,25 +58,13 @@ async fn ldap_import(data: Json<OrgImportData>, token: PublicToken, conn: DbConn
     let org_id = token.0;
     let data = data.into_inner();
 
+    // Like upstream, the import never removes Owners, so it doesn't revoke or restore them either.
     for user_data in &data.members {
         let mut user_created: bool = false;
         if user_data.deleted {
             // If user is marked for deletion and it exists, revoke it
             if let Some(mut member) = Membership::find_by_email_and_org(&user_data.email, &org_id, &conn).await {
-                // Only revoke a user if it is not the last confirmed owner
-                let revoked = if member.atype == MembershipType::Owner
-                    && member.status == MembershipStatus::Confirmed as i32
-                {
-                    if Membership::count_confirmed_by_org_and_type(&org_id, MembershipType::Owner, &conn).await <= 1 {
-                        warn!("Can't revoke the last owner");
-                        false
-                    } else {
-                        member.revoke()
-                    }
-                } else {
-                    member.revoke()
-                };
-
+                let revoked = member.atype != MembershipType::Owner && member.revoke();
                 let ext_modified = member.set_external_id(Some(user_data.external_id.clone()));
                 if revoked || ext_modified {
                     member.save(&conn).await?;
@@ -84,7 +72,7 @@ async fn ldap_import(data: Json<OrgImportData>, token: PublicToken, conn: DbConn
             }
         // If user is part of the organization, restore it
         } else if let Some(mut member) = Membership::find_by_email_and_org(&user_data.email, &org_id, &conn).await {
-            let mut restored = member.restore();
+            let mut restored = member.atype != MembershipType::Owner && member.restore();
             let ext_modified = member.set_external_id(Some(user_data.external_id.clone()));
             // Enforce org policies as every other restore path does.
             // If the user is not allowed, we revoke again and continue so the external_id is still updated.
@@ -180,14 +168,8 @@ async fn ldap_import(data: Json<OrgImportData>, token: PublicToken, conn: DbConn
         for member in Membership::find_by_org(&org_id, &conn).await {
             if let Some(ref user_external_id) = member.external_id
                 && !sync_members.contains(user_external_id)
+                && member.atype != MembershipType::Owner
             {
-                if member.atype == MembershipType::Owner && member.status == MembershipStatus::Confirmed as i32 {
-                    // Removing owner, check that there is at least one other confirmed owner
-                    if Membership::count_confirmed_by_org_and_type(&org_id, MembershipType::Owner, &conn).await <= 1 {
-                        warn!("Can't delete the last owner");
-                        continue;
-                    }
-                }
                 member.delete(&conn).await?;
             }
         }

src/api/core/sends.rs

@@ -23,7 +23,7 @@ use crate::{
 
 const SEND_INACCESSIBLE_MSG: &str = "Send does not exist or is no longer available";
 static ANON_PUSH_DEVICE: LazyLock<Device> = LazyLock::new(|| {
-    let dt = crate::util::parse_date("1970-01-01T00:00:00.000000Z");
+    let dt = DateTime::UNIX_EPOCH.naive_utc();
     Device {
         uuid: String::from("00000000-0000-0000-0000-000000000000").into(),
         created_at: dt,
@@ -39,7 +39,7 @@ static ANON_PUSH_DEVICE: LazyLock<Device> = LazyLock::new(|| {
 });
 
 // The max file size allowed by Bitwarden clients and add an extra 5% to avoid issues
-const SIZE_525_MB: i64 = 550_502_400;
+pub(crate) const SIZE_525_MB: i64 = 550_502_400;
 
 pub fn routes() -> Vec<rocket::Route> {
     routes![
@@ -328,8 +328,8 @@ async fn post_send_file_v2_data(
             "Send file name does not match.",
             format!(
                 "Expected file name '{}' got '{}'",
-                send_data.fileName,
-                raw_file_name.dangerous_unsafe_unsanitized_raw()
+                send_data.fileName.escape_debug(),
+                raw_file_name.dangerous_unsafe_unsanitized_raw().as_str().escape_debug()
             )
         ),
         _ => err!("Send file name does not match or is not provided."),

src/api/core/two_factor/yubikey.rs

@@ -217,7 +217,9 @@ pub async fn validate_yubikey_login(response: &str, twofactor_data: &str) -> Emp
     }
 
     let yubikey_metadata: YubikeyMetadata = serde_json::from_str(twofactor_data).expect("Can't parse Yubikey Metadata");
-    let response_id = &response[..12];
+    let Some(response_id) = response.get(..12) else {
+        err!("Invalid Yubikey OTP");
+    };
 
     if !yubikey_metadata.keys.contains(&response_id.to_owned()) {
         err!("Given Yubikey is not registered");

src/api/icons.rs

@@ -85,7 +85,7 @@ static ICON_SIZE_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"(?x)(\d+
 #[get("/<host>/icon.png")]
 fn icon_external(host: &str) -> Cached<Option<Redirect>> {
     let Ok(host) = get_valid_host(host) else {
-        warn!("Invalid host: {host}");
+        warn!("Invalid host: {}", host.escape_debug());
         return Cached::ttl(None, CONFIG.icon_cache_negttl(), true);
     };
 
@@ -113,7 +113,7 @@ async fn icon_internal(host: &str) -> Cached<(ContentType, Vec<u8>)> {
     const FALLBACK_ICON: &[u8] = include_bytes!("../static/images/fallback-icon.png");
 
     let Ok(host) = get_valid_host(host) else {
-        warn!("Invalid host: {host}");
+        warn!("Invalid host: {}", host.escape_debug());
         return Cached::ttl(
             (ContentType::new("image", "png"), FALLBACK_ICON.to_vec()),
             CONFIG.icon_cache_negttl(),

src/api/identity.rs

@@ -20,7 +20,7 @@ use crate::{
                 authenticator, duo_oidc, email, enforce_2fa_policy, is_twofactor_provider_usable, webauthn, yubikey,
             },
         },
-        master_password_policy,
+        identity_master_password_policy,
         push::register_push_device,
     },
     auth,
@@ -83,10 +83,6 @@ async fn login(data: Form<ConnectData>, client_header: ClientHeaders, conn: DbCo
             check_is_some(data.client_secret.as_ref(), "client_secret cannot be blank")?;
             check_is_some(data.scope.as_ref(), "scope cannot be blank")?;
 
-            check_is_some(data.device_identifier.as_ref(), "device_identifier cannot be blank")?;
-            check_is_some(data.device_name.as_ref(), "device_name cannot be blank")?;
-            check_is_some(data.device_type.as_ref(), "device_type cannot be blank")?;
-
             api_key_login(data, &mut user_id, &conn, &client_header.ip).await
         }
         "authorization_code" if CONFIG.sso_enabled() => {
@@ -408,6 +404,7 @@ async fn password_login(data: ConnectData, user_id: &mut Option<UserId>, conn: &
 
         if auth_request.user_uuid != user.uuid
             || !auth_request.approved.unwrap_or(false)
+            || auth_request.authentication_date.is_some()
             || request_expired
             || ip.ip.to_string() != auth_request.request_ip
             || !auth_request.check_access_code(password)
@@ -484,6 +481,19 @@ async fn password_login(data: ConnectData, user_id: &mut Option<UserId>, conn: &
 
     let twofactor_token = twofactor_auth(&mut user, &data, &mut device, ip, conn).await?;
 
+    // Like upstream, an auth request can be used for one login only. Marked after 2FA so a 2FA prompt doesn't use it up.
+    if let Some(ref auth_request_id) = data.auth_request
+        && !AuthRequest::set_authentication_date(auth_request_id, conn).await?
+    {
+        err!(
+            "Username or access code is incorrect. Try again",
+            format!("IP: {}. Username: {log_username}.", ip.ip),
+            ErrorEvent {
+                event: EventType::UserFailedLogIn,
+            }
+        )
+    }
+
     let auth_tokens = auth::AuthTokens::new(&device, &user, AuthMethod::Password, data.client_id);
 
     authenticated_response(&user, &mut device, auth_tokens, twofactor_token, conn, ip).await
@@ -521,7 +531,7 @@ async fn authenticated_response(
     // Save to update `device.updated_at` to track usage and toggle new status
     device.save(true, conn).await?;
 
-    let master_password_policy = master_password_policy(user, conn).await;
+    let master_password_policy = identity_master_password_policy(user, conn).await;
 
     let has_master_password = !user.password_hash.is_empty();
     let master_password_unlock = if has_master_password {
@@ -594,7 +604,14 @@ async fn api_key_login(data: ConnectData, user_id: &mut Option<UserId>, conn: &D
 
     // Validate scope
     match data.scope.as_ref() {
-        Some(scope) if scope == &AuthMethod::UserApiKey.scope() => user_api_key_login(data, user_id, conn, ip).await,
+        Some(scope) if scope == &AuthMethod::UserApiKey.scope() => {
+            // Like upstream, only the user API key logs in a device
+            check_is_some(data.device_identifier.as_ref(), "device_identifier cannot be blank")?;
+            check_is_some(data.device_name.as_ref(), "device_name cannot be blank")?;
+            check_is_some(data.device_type.as_ref(), "device_type cannot be blank")?;
+
+            user_api_key_login(data, user_id, conn, ip).await
+        }
         Some(scope) if scope == &AuthMethod::OrgApiKey.scope() => organization_api_key_login(data, conn, ip).await,
         _ => err!("Scope not supported"),
     }
@@ -811,12 +828,12 @@ async fn twofactor_auth(
     if ![TwoFactorType::Remember as i32, TwoFactorType::RecoveryCode as i32].contains(&selected_id)
         && !twofactor_ids.contains(&selected_id)
     {
-        err_json!(json_err_twofactor(&twofactor_ids, &user.uuid, data, conn).await?, "Invalid two factor provider")
+        err_json!(json_err_twofactor(&twofactor_ids, user, data, conn).await?, "Invalid two factor provider")
     }
 
     // Like upstream, a blank token counts as not provided
     let Some(twofactor_code) = data.two_factor_token.as_deref().filter(|t| !t.trim().is_empty()) else {
-        err_json!(json_err_twofactor(&twofactor_ids, &user.uuid, data, conn).await?, "2FA token not provided")
+        err_json!(json_err_twofactor(&twofactor_ids, user, data, conn).await?, "2FA token not provided")
     };
 
     let selected_twofactor = twofactors.into_iter().find(|tf| tf.atype == selected_id && tf.enabled);
@@ -859,16 +876,22 @@ async fn twofactor_auth(
                         device.save(true, conn).await?;
                     }
                     err_json!(
-                        json_err_twofactor(&twofactor_ids, &user.uuid, data, conn).await?,
+                        json_err_twofactor(&twofactor_ids, user, data, conn).await?,
                         "2FA Remember token not provided or expired"
                     )
                 }
             }
         }
         Some(TwoFactorType::RecoveryCode) => {
-            // Check if recovery code is correct
-            if !user.check_valid_recovery_code(twofactor_code) {
-                err!("Recovery code is incorrect. Try again.")
+            // Like upstream, spaces and case don't matter
+            let recovery_code = twofactor_code.replace(' ', "").trim().to_lowercase();
+            if !user.check_valid_recovery_code(&recovery_code) {
+                err!(
+                    "Two-step token is invalid. Try again.",
+                    ErrorEvent {
+                        event: EventType::UserFailedLogIn2fa
+                    }
+                )
             }
 
             // Remove all twofactors from the user
@@ -883,6 +906,13 @@ async fn twofactor_auth(
 
             log_user_event(EventType::UserRecovered2fa as i32, &user.uuid, device.atype, &ip.ip, conn).await;
 
+            if CONFIG.mail_enabled()
+                && let Err(e) =
+                    mail::send_recover_twofactor(&user.email, &ip.ip.to_string(), &Utc::now().naive_utc()).await
+            {
+                error!("Error sending two-step login recovered email: {e:#?}");
+            }
+
             // Remove the recovery code, not needed without twofactors
             user.totp_recover = None;
             user.save(conn).await?;
@@ -910,20 +940,14 @@ fn selected_data(tf: Option<TwoFactor>) -> ApiResult<String> {
     tf.map(|t| t.data).map_res("Two factor doesn't exist")
 }
 
-async fn json_err_twofactor(
-    providers: &[i32],
-    user_id: &UserId,
-    data: &ConnectData,
-    conn: &DbConn,
-) -> ApiResult<Value> {
+async fn json_err_twofactor(providers: &[i32], user: &User, data: &ConnectData, conn: &DbConn) -> ApiResult<Value> {
+    let user_id = &user.uuid;
     let mut result = json!({
         "error" : "invalid_grant",
         "error_description" : "Two factor required.",
         "TwoFactorProviders" : providers.iter().map(ToString::to_string).collect::<Vec<String>>(),
         "TwoFactorProviders2" : {}, // { "0" : null }
-        "MasterPasswordPolicy": {
-            "Object": "masterPasswordPolicy"
-        }
+        "MasterPasswordPolicy": identity_master_password_policy(user, conn).await,
     });
 
     for provider in providers {
@@ -936,14 +960,8 @@ async fn json_err_twofactor(
             }
 
             Some(TwoFactorType::Duo) => {
-                let email = if let Some(u) = User::find_by_uuid(user_id, conn).await {
-                    u.email
-                } else {
-                    err!("User does not exist")
-                };
-
                 let auth_url = duo_oidc::get_duo_auth_url(
-                    &email,
+                    &user.email,
                     data.client_id.as_ref().unwrap(),
                     data.device_identifier.as_ref().unwrap(),
                     conn,
@@ -1212,7 +1230,7 @@ async fn oidcsignin_redirect(
     let state = sso::decode_state(&base64_state)?;
 
     let Some(mut sso_auth) = SsoAuth::find(&state, conn).await else {
-        err!(format!("Cannot retrieve sso_auth for {state}"))
+        err!(format!("Cannot retrieve sso_auth for {}", state.escape_debug()))
     };
 
     // Browser-binding check
@@ -1221,7 +1239,7 @@ async fn oidcsignin_redirect(
     let provided_hash = cookie_value.as_deref().map(|v| crypto::sha256_hex(v.as_bytes()));
     match (sso_auth.binding_hash.as_deref(), provided_hash.as_deref()) {
         (Some(expected), Some(actual)) if crypto::ct_eq(expected, actual) => {}
-        _ => err!(format!("SSO session binding mismatch for {state}")),
+        _ => err!(format!("SSO session binding mismatch for {}", state.escape_debug())),
     }
     cookies
         .remove(Cookie::build(SSO_BINDING_COOKIE).path(format!("{}/identity/connect/", CONFIG.domain_path())).build());

src/api/mod.rs

@@ -124,3 +124,21 @@ async fn master_password_policy(user: &User, conn: &DbConn) -> Value {
     mpp_json["Object"] = json!("masterPasswordPolicy");
     mpp_json
 }
+
+// Identity responses use PascalCase keys like upstream, some clients (Android) only read those
+async fn identity_master_password_policy(user: &User, conn: &DbConn) -> Value {
+    match master_password_policy(user, conn).await {
+        Value::Object(policy) => Value::Object(
+            policy
+                .into_iter()
+                .map(|(mut key, value)| {
+                    if let Some(first) = key.get_mut(..1) {
+                        first.make_ascii_uppercase();
+                    }
+                    (key, value)
+                })
+                .collect(),
+        ),
+        policy => policy,
+    }
+}

src/auth.rs

@@ -14,6 +14,7 @@ use ipnet::IpNet;
 use jsonwebtoken::{Algorithm, DecodingKey, EncodingKey, Header, errors::ErrorKind};
 use num_traits::FromPrimitive;
 use openssl::rsa::Rsa;
+use ring::hmac;
 use serde::{de::DeserializeOwned, ser::Serialize};
 
 use rocket::{
@@ -65,6 +66,7 @@ static JWT_2FA_REMEMBER_ISSUER: LazyLock<String> = LazyLock::new(|| format!("{}|
 
 static PRIVATE_RSA_KEY: OnceLock<EncodingKey> = OnceLock::new();
 static PUBLIC_RSA_KEY: OnceLock<DecodingKey> = OnceLock::new();
+static PRELOGIN_KDF_KEY: OnceLock<hmac::Key> = OnceLock::new();
 
 pub async fn initialize_keys() -> Result<(), Error> {
     use std::io::Error as IoError;
@@ -99,9 +101,22 @@ pub async fn initialize_keys() -> Result<(), Error> {
     if PUBLIC_RSA_KEY.set(dec).is_err() {
         err!("PUBLIC_RSA_KEY must only be initialized once")
     }
+
+    // Derived from the RSA key, so it stays the same across restarts without storing another secret
+    let prelogin_kdf_key = hmac::sign(&hmac::Key::new(hmac::HMAC_SHA256, &priv_key_buffer), b"prelogin-kdf");
+    if PRELOGIN_KDF_KEY.set(hmac::Key::new(hmac::HMAC_SHA256, prelogin_kdf_key.as_ref())).is_err() {
+        err!("PRELOGIN_KDF_KEY must only be initialized once")
+    }
     Ok(())
 }
 
+/// Returns an index in `0..len` derived from the email, the same for every request
+pub fn prelogin_kdf_index(email: &str, len: usize) -> usize {
+    let tag = hmac::sign(PRELOGIN_KDF_KEY.wait(), email.as_bytes());
+    let tag = tag.as_ref();
+    usize::from(u16::from_be_bytes([tag[0], tag[1]])) % len
+}
+
 pub fn encode_jwt<T: Serialize>(claims: &T) -> String {
     match jsonwebtoken::encode(&JWT_HEADER, claims, PRIVATE_RSA_KEY.wait()) {
         Ok(token) => token,
@@ -422,7 +437,7 @@ pub fn generate_file_download_claims(cipher_id: CipherId, file_id: AttachmentId)
     let time_now = Utc::now();
     FileDownloadClaims {
         nbf: time_now.timestamp(),
-        exp: (time_now + TimeDelta::try_minutes(5).unwrap()).timestamp(),
+        exp: (time_now + TimeDelta::try_minutes(1).unwrap()).timestamp(),
         iss: JWT_FILE_DOWNLOAD_ISSUER.to_string(),
         sub: cipher_id,
         file_id,
@@ -1091,6 +1106,31 @@ fn ip_header_is_trusted(remote: Option<IpAddr>) -> bool {
     trusted.split(',').filter_map(parse_trusted_proxy).any(|net| net.contains(&remote))
 }
 
+/// Each proxy appends the address it received the request from to a list header (`X-Forwarded-For: client, proxy1`).
+/// Like ASP.NET's ForwardedHeaders, walk the entries of every occurrence of the header from the right and take the
+/// first one that isn't a trusted proxy. If they all are, the leftmost one is the client.
+fn client_ip_from_header(req: &Request<'_>, header: &str, is_trusted: impl Fn(IpAddr) -> bool) -> Option<IpAddr> {
+    let values: Vec<&str> = req.headers().get(header).collect();
+    client_ip_from_header_values(&values, header, is_trusted)
+}
+
+fn client_ip_from_header_values(values: &[&str], header: &str, is_trusted: impl Fn(IpAddr) -> bool) -> Option<IpAddr> {
+    let entries: Vec<&str> = values.iter().flat_map(|value| value.split(',')).map(str::trim).collect();
+
+    let mut client_ip = None;
+    for entry in entries.into_iter().rev() {
+        let Ok(ip) = entry.parse() else {
+            warn!("'{header}' header is malformed: {entry}");
+            break;
+        };
+        client_ip = Some(ip);
+        if !is_trusted(ip) {
+            break;
+        }
+    }
+    client_ip
+}
+
 #[rocket::async_trait]
 impl<'r> FromRequest<'r> for ClientIp {
     type Error = ();
@@ -1099,15 +1139,7 @@ impl<'r> FromRequest<'r> for ClientIp {
         let remote = req.remote().map(|r| r.ip());
 
         let ip = if CONFIG._ip_header_enabled() && ip_header_is_trusted(remote) {
-            req.headers().get_one(&CONFIG.ip_header()).and_then(|ip| {
-                match ip.find(',') {
-                    Some(idx) => &ip[..idx],
-                    None => ip,
-                }
-                .parse()
-                .map_err(|_| warn!("'{}' header is malformed: {ip}", CONFIG.ip_header()))
-                .ok()
-            })
+            client_ip_from_header(req, &CONFIG.ip_header(), |ip| ip_header_is_trusted(Some(ip)))
         } else {
             if CONFIG._ip_header_enabled() && req.headers().get_one(&CONFIG.ip_header()).is_some() {
                 // Log the canonical IP, which is what the user filter will need to match against
@@ -1226,7 +1258,7 @@ impl AuthMethod {
         match scope {
             None => err!("Missing scope"),
             Some(scope) if scope == &method_scope => Ok(method_scope),
-            Some(scope) => err!(format!("Scope ({scope}) not supported")),
+            Some(scope) => err!(format!("Scope ({}) not supported", scope.escape_debug())),
         }
     }
 }
@@ -1345,3 +1377,52 @@ pub async fn refresh_tokens(
 
     Ok((device, auth_tokens))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn client_ip(values: &[&str]) -> Option<IpAddr> {
+        let is_trusted = |ip: IpAddr| match ip {
+            IpAddr::V4(v4) => v4.is_private() || v4.is_loopback(),
+            IpAddr::V6(v6) => v6.is_loopback(),
+        };
+        client_ip_from_header_values(values, "X-Forwarded-For", is_trusted)
+    }
+
+    fn ip(s: &str) -> Option<IpAddr> {
+        Some(s.parse().unwrap())
+    }
+
+    #[test]
+    fn ip_header_single_value() {
+        assert_eq!(client_ip(&["203.0.113.5"]), ip("203.0.113.5"));
+        assert_eq!(client_ip(&["10.0.0.7"]), ip("10.0.0.7"));
+        assert_eq!(client_ip(&["2001:db8::1"]), ip("2001:db8::1"));
+    }
+
+    #[test]
+    fn ip_header_uses_rightmost_untrusted_entry() {
+        // Entries already in the request come before the one the proxy appended
+        assert_eq!(client_ip(&["198.51.100.1, 203.0.113.5"]), ip("203.0.113.5"));
+        // Trusted proxies on the right are skipped
+        assert_eq!(client_ip(&["198.51.100.1, 203.0.113.5, 10.0.0.2, 127.0.0.1"]), ip("203.0.113.5"));
+        // All trusted, so the leftmost entry is the client
+        assert_eq!(client_ip(&["192.168.1.10, 10.0.0.2"]), ip("192.168.1.10"));
+    }
+
+    #[test]
+    fn ip_header_reads_every_occurrence() {
+        assert_eq!(client_ip(&["198.51.100.1", "203.0.113.5"]), ip("203.0.113.5"));
+        assert_eq!(client_ip(&["198.51.100.1", "203.0.113.5, 10.0.0.2"]), ip("203.0.113.5"));
+    }
+
+    #[test]
+    fn ip_header_malformed_entries() {
+        assert_eq!(client_ip(&[]), None);
+        assert_eq!(client_ip(&[""]), None);
+        assert_eq!(client_ip(&["203.0.113.5, not-an-ip"]), None);
+        // Stops at the malformed entry and keeps the closest proxy
+        assert_eq!(client_ip(&["not-an-ip, 10.0.0.2"]), ip("10.0.0.2"));
+    }
+}

src/config.rs

@@ -675,7 +675,8 @@ make_config! {
         /// other address use the remote IP instead, so a client can't spoof the header.
         /// Either the string "local" (the default, any non-global address, which covers a reverse proxy
         /// running on the same host or container network), the string "all" to accept it from anywhere,
-        /// or a comma separated list of IPs and CIDR ranges.
+        /// or a comma separated list of IPs and CIDR ranges. For a list header like X-Forwarded-For, the
+        /// client IP is the rightmost address that isn't a trusted proxy, so list every proxy in the chain.
         ip_header_trusted_proxies: String, true, def,    "local".to_owned();
         /// Icon service |> The predefined icon services are: internal, bitwarden, duckduckgo, google.
         /// To specify a custom icon service, set a URL template with exactly one instance of `{}`,
@@ -1726,6 +1727,7 @@ where
     reg!("email/protected_action", ".html");
     reg!("email/pw_hint_none", ".html");
     reg!("email/pw_hint_some", ".html");
+    reg!("email/recover_twofactor", ".html");
     reg!("email/register_verify_email", ".html");
     reg!("email/send_2fa_removed_from_org", ".html");
     reg!("email/send_emergency_access_invite", ".html");

src/db/models/attachment.rs

@@ -61,7 +61,7 @@ impl Attachment {
             let token = encode_jwt(&generate_file_download_claims(self.cipher_uuid.clone(), self.id.clone()));
             Ok(format!("{host}/attachments/{}/{}?token={token}", self.cipher_uuid, self.id))
         } else {
-            Ok(operator.presign_read(&self.get_file_path(), Duration::from_mins(5)).await?.uri().to_string())
+            Ok(operator.presign_read(&self.get_file_path(), Duration::from_mins(1)).await?.uri().to_string())
         }
     }
 

src/db/models/auth_request.rs

@@ -4,7 +4,7 @@ use diesel::prelude::*;
 use serde_json::Value;
 
 use crate::{
-    api::EmptyResult,
+    api::{ApiResult, EmptyResult},
     crypto::ct_eq,
     db::{DbConn, schema::auth_requests},
     error::MapResult,
@@ -166,6 +166,21 @@ impl AuthRequest {
         .await
     }
 
+    /// Marks the request as used to log in. Returns false when it already was, so it only works once.
+    pub async fn set_authentication_date(uuid: &AuthRequestId, conn: &DbConn) -> ApiResult<bool> {
+        let now = Utc::now().naive_utc();
+        conn.run(move |conn| {
+            diesel::update(auth_requests::table)
+                .filter(auth_requests::uuid.eq(uuid))
+                .filter(auth_requests::authentication_date.is_null())
+                .set(auth_requests::authentication_date.eq(now))
+                .execute(conn)
+                .map(|updated| updated == 1)
+                .map_res("Error updating auth request")
+        })
+        .await
+    }
+
     pub fn check_access_code(&self, access_code: &str) -> bool {
         ct_eq(&self.access_code, access_code)
     }

src/db/models/organization.rs

@@ -342,7 +342,7 @@ impl OrganizationApiKey {
 impl Organization {
     pub async fn save(&self, conn: &DbConn) -> EmptyResult {
         if !crate::util::is_valid_email(&self.billing_email) {
-            err!(format!("BillingEmail {} is not a valid email address", self.billing_email))
+            err!(format!("BillingEmail {} is not a valid email address", self.billing_email.escape_debug()))
         }
 
         for member in &Membership::find_by_org(&self.uuid, conn).await {

src/db/models/user.rs

@@ -261,6 +261,25 @@ impl User {
         !CONFIG.mail_enabled() || self.verified_at.is_some()
     }
 
+    /// The `accountKeys` object (upstream's `PrivateKeysResponseModel`), null without a key pair
+    pub fn account_keys_json(&self) -> Value {
+        if self.private_key.is_some() {
+            json!({
+                "publicKeyEncryptionKeyPair": {
+                    "wrappedPrivateKey": self.private_key,
+                    "publicKey": self.public_key,
+                    "signedPublicKey": null,
+                    "object": "publicKeyEncryptionKeyPair",
+                },
+                "securityState": null,
+                "signatureKeyPair": null,
+                "object": "privateKeys"
+            })
+        } else {
+            Value::Null
+        }
+    }
+
     pub async fn to_json(&self, conn: &DbConn) -> Value {
         let mut orgs_json = Vec::new();
         for c in Membership::find_confirmed_by_user(&self.uuid, conn).await {
@@ -281,21 +300,7 @@ impl User {
             UserStatus::Enabled
         };
 
-        let account_keys = if self.private_key.is_some() {
-            json!({
-                "publicKeyEncryptionKeyPair": {
-                    "wrappedPrivateKey": self.private_key,
-                    "publicKey": self.public_key,
-                    "signedPublicKey": null,
-                    "object": "publicKeyEncryptionKeyPair",
-                },
-                "securityState": null,
-                "signatureKeyPair": null,
-                "object": "privateKeys"
-            })
-        } else {
-            Value::Null
-        };
+        let account_keys = self.account_keys_json();
 
         json!({
             "_status": status as i32,
@@ -325,7 +330,7 @@ impl User {
 
     pub async fn save(&mut self, conn: &DbConn) -> EmptyResult {
         if !crate::util::is_valid_email(&self.email) {
-            err!(format!("User email {} is not a valid email address", self.email))
+            err!(format!("User email {} is not a valid email address", self.email.escape_debug()))
         }
 
         self.updated_at = Utc::now().naive_utc();
@@ -473,7 +478,7 @@ impl Invitation {
 
     pub async fn save(&self, conn: &DbConn) -> EmptyResult {
         if !crate::util::is_valid_email(&self.email) {
-            err!(format!("Invitation email {} is not a valid email address", self.email))
+            err!(format!("Invitation email {} is not a valid email address", self.email.escape_debug()))
         }
 
         db_run! { conn:

src/mail.rs

@@ -531,6 +531,21 @@ pub async fn send_new_device_logged_in(address: &str, ip: &str, dt: &NaiveDateTi
     send_email(address, &subject, body_html, body_text).await
 }
 
+pub async fn send_recover_twofactor(address: &str, ip: &str, dt: &NaiveDateTime) -> EmptyResult {
+    let fmt = "%A, %B %_d, %Y at %r %Z";
+    let (subject, body_html, body_text) = get_text(
+        "email/recover_twofactor",
+        json!({
+            "url": CONFIG.domain(),
+            "img_src": CONFIG._smtp_img_src(),
+            "ip": ip,
+            "datetime": crate::util::format_naive_datetime_local(dt, fmt),
+        }),
+    )?;
+
+    send_email(address, &subject, body_html, body_text).await
+}
+
 pub async fn send_incomplete_2fa_login(
     address: &str,
     ip: &str,

src/sso.rs

@@ -2,7 +2,6 @@ use std::{sync::LazyLock, time::Duration};
 
 use chrono::Utc;
 use derive_more::{AsRef, Deref, Display, From, Into};
-use regex::Regex;
 use url::Url;
 
 use crate::{
@@ -175,16 +174,16 @@ pub fn decode_state(base64_state: &str) -> ApiResult<OIDCState> {
         if let Ok(valid) = String::from_utf8(vec) {
             OIDCState(valid)
         } else {
-            err!(format!("Invalid utf8 chars in {base64_state} after base64 decoding"))
+            err!(format!("Invalid utf8 chars in {} after base64 decoding", base64_state.escape_debug()))
         }
     } else {
-        err!(format!("Failed to decode {base64_state} using base64"))
+        err!(format!("Failed to decode {} using base64", base64_state.escape_debug()))
     };
 
     Ok(state)
 }
 
-// redirect_uri from: https://github.com/bitwarden/server/blob/main/src/Identity/IdentityServer/ApiClient.cs
+// redirect_uri from: https://github.com/bitwarden/server/blob/main/src/Identity/IdentityServer/StaticClientStore.cs
 pub async fn authorize_url(
     state: OIDCState,
     client_challenge: OIDCCodeChallenge,
@@ -193,20 +192,21 @@ pub async fn authorize_url(
     binding_hash: Option<String>,
     conn: DbConn,
 ) -> ApiResult<Url> {
+    // The desktop app uses the localhost callback when it can't register the custom scheme (AppImage, dev builds)
+    let localhost_redirect_uri =
+        (8065..=8070).map(|port| format!("http://localhost:{port}")).find(|uri| uri == raw_redirect_uri);
+
     let redirect_uri = match client_id {
         "web" | "browser" => format!("{}/sso-connector.html", CONFIG.domain()),
-        "desktop" | "mobile" => "bitwarden://sso-callback".to_owned(),
+        "desktop" => localhost_redirect_uri.unwrap_or_else(|| "bitwarden://sso-callback".to_owned()),
+        "mobile" => "bitwarden://sso-callback".to_owned(),
         "cli" => {
-            let port_regex = Regex::new(r"^http://localhost:([0-9]{4})$").unwrap();
-            if let Some(port) =
-                port_regex.captures(raw_redirect_uri).and_then(|captures| captures.get(1).map(|c| c.as_str()))
-            {
-                format!("http://localhost:{port}")
-            } else {
-                err!("Failed to extract port number")
-            }
+            let Some(uri) = localhost_redirect_uri else {
+                err!(format!("Unsupported redirect uri {}", raw_redirect_uri.escape_debug()))
+            };
+            uri
         }
-        _ => err!(format!("Unsupported client {client_id}")),
+        _ => err!(format!("Unsupported client {}", client_id.escape_debug())),
     };
 
     let (auth_url, sso_auth) = Client::authorize_url(state, client_challenge, redirect_uri, binding_hash).await?;
@@ -242,7 +242,8 @@ impl OIDCIdentifier {
 
 // During the 2FA flow we will
 //  - retrieve the user information and then only discover he needs 2FA.
-//  - second time we will rely on `SsoAuth.auth_response` since the `code` has already been exchanged.
+//  - second time we will rely on `SsoAuth.auth_response` since the `code` has already been exchanged,
+//    after checking the PKCE verifier again like upstream does on every redemption.
 // The `SsoAuth` will ensure that the user is authorized only once.
 pub async fn exchange_code(
     code: &OIDCCode,
@@ -256,6 +257,7 @@ pub async fn exchange_code(
     };
 
     if let Some(authenticated_user) = sso_auth.auth_response.clone() {
+        Client::check_client_verifier(&client_verifier, &sso_auth)?;
         return Ok((sso_auth, authenticated_user));
     }
 

src/sso_client.rs

@@ -212,6 +212,16 @@ impl Client {
         Ok((auth_url, SsoAuth::new(state, client_challenge, nonce.secret().clone(), redirect_uri, binding_hash)))
     }
 
+    // Check the client's PKCE verifier against the challenge it sent when starting the flow
+    pub fn check_client_verifier(client_verifier: &OIDCCodeVerifier, sso_auth: &SsoAuth) -> EmptyResult {
+        let verifier = PkceCodeVerifier::new(client_verifier.to_string());
+        let challenge = PkceCodeChallenge::from_code_verifier_sha256(&verifier);
+        if challenge.as_str() != &*sso_auth.client_challenge {
+            err!("PKCE client challenge failed")
+        }
+        Ok(())
+    }
+
     pub async fn exchange_code(
         &self,
         code: OIDCCode,
@@ -234,15 +244,10 @@ impl Client {
 
         let mut exchange = self.core_client.exchange_code(oidc_code);
 
-        let verifier = PkceCodeVerifier::new(client_verifier.into());
         if CONFIG.sso_pkce() {
-            exchange = exchange.set_pkce_verifier(verifier);
+            exchange = exchange.set_pkce_verifier(PkceCodeVerifier::new(client_verifier.into()));
         } else {
-            let challenge = PkceCodeChallenge::from_code_verifier_sha256(&verifier);
-            if challenge.as_str() != String::from(sso_auth.client_challenge.clone()) {
-                err!("PKCE client challenge failed")
-                // Might need to notify admin ? how ?
-            }
+            Self::check_client_verifier(&client_verifier, sso_auth)?;
         }
 
         match exchange.request_async(&self.http_client).await {

src/static/templates/email/recover_twofactor.hbs

@@ -0,0 +1,9 @@
+Two-Step Login Recovered From {{ip}}
+<!---------------->
+Two-step login on your account has been disabled by using the account's recovery code.
+
+* Date: {{datetime}}
+* IP Address: {{ip}}
+
+If you did not perform this action, you should immediately take steps to secure your account.
+{{> email/email_footer_text }}

src/static/templates/email/recover_twofactor.html.hbs

@@ -0,0 +1,26 @@
+Two-Step Login Recovered From {{ip}}
+<!---------------->
+{{> email/email_header }}
+<table width="100%" cellpadding="0" cellspacing="0" style="margin: 0; font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; -webkit-font-smoothing: antialiased; -webkit-text-size-adjust: none;">
+   <tr style="margin: 0; font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; -webkit-font-smoothing: antialiased; -webkit-text-size-adjust: none;">
+      <td class="content-block" style="font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; margin: 0; -webkit-font-smoothing: antialiased; padding: 0 0 10px; -webkit-text-size-adjust: none;" valign="top">
+         Two-step login on your account has been disabled by using the account's recovery code.
+      </td>
+   </tr>
+   <tr style="margin: 0; font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; -webkit-font-smoothing: antialiased; -webkit-text-size-adjust: none;">
+      <td class="content-block" style="font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; margin: 0; -webkit-font-smoothing: antialiased; padding: 0 0 10px; -webkit-text-size-adjust: none;" valign="top">
+         <b>Date:</b> {{datetime}}
+      </td>
+   </tr>
+   <tr style="margin: 0; font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; -webkit-font-smoothing: antialiased; -webkit-text-size-adjust: none;">
+      <td class="content-block" style="font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; margin: 0; -webkit-font-smoothing: antialiased; padding: 0 0 10px; -webkit-text-size-adjust: none;" valign="top">
+         <b>IP Address:</b> {{ip}}
+      </td>
+   </tr>
+   <tr style="margin: 0; font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; -webkit-font-smoothing: antialiased; -webkit-text-size-adjust: none;">
+      <td class="content-block last" style="font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif; box-sizing: border-box; font-size: 16px; color: #333; line-height: 25px; margin: 0; -webkit-font-smoothing: antialiased; padding: 0; -webkit-text-size-adjust: none;" valign="top">
+         If you did not perform this action, you should immediately take steps to secure your account.
+      </td>
+   </tr>
+</table>
+{{> email/email_footer }}

src/util.rs

@@ -380,7 +380,7 @@ pub fn get_display_size(size: i64) -> String {
     let mut unit_counter = 0;
 
     loop {
-        if size > 1024. {
+        if size > 1024. && unit_counter < UNITS.len() - 1 {
             size /= 1024.;
             unit_counter += 1;
         } else {
@@ -522,8 +522,8 @@ pub fn format_datetime_http(dt: &DateTime<Local>) -> String {
     expiry_time.to_rfc2822().replace("+0000", "GMT")
 }
 
-pub fn parse_date(date: &str) -> NaiveDateTime {
-    DateTime::parse_from_rfc3339(date).unwrap().naive_utc()
+pub fn parse_date(date: &str) -> Option<NaiveDateTime> {
+    DateTime::parse_from_rfc3339(date).ok().map(|dt| dt.naive_utc())
 }
 
 /// Returns true or false if an email address is valid or not