From e93769786c7669485a659ee471630740d3cea702 Mon Sep 17 00:00:00 2001 From: jarvarvarvis <53998846+jarvarvarvis@users.noreply.github.com> Date: Fri, 24 Apr 2026 04:02:28 +0000 Subject: [PATCH] feat: automatically sanitise usernames on create/update (#689) * feat: automatically sanitise usernames on create/update Signed-off-by: higgs01 <6546697+higgs01@users.noreply.github.com> * test: add tests for validation and sanitasion Signed-off-by: higgs01 <6546697+higgs01@users.noreply.github.com> * fix: Return only the sanitised string in sanitise_username (#424) * fix: Use as_str for role.id in insert_role * Run rustfmt for code changes Signed-off-by: jarvarvarvis * feat: Make minimum username length configurable in Revolt.toml (#424) * fix: Remove redundant call to is_match with blocked username regex Signed-off-by: jarvarvarvis * Update crates/core/database/src/models/users/model.rs Co-authored-by: Tom Signed-off-by: jarvarvarvis <53998846+jarvarvarvis@users.noreply.github.com> * Update crates/core/database/src/models/users/model.rs Co-authored-by: Tom Signed-off-by: jarvarvarvis <53998846+jarvarvarvis@users.noreply.github.com> * Update crates/core/database/src/models/users/model.rs Co-authored-by: Tom Signed-off-by: jarvarvarvis <53998846+jarvarvarvis@users.noreply.github.com> * Update crates/core/database/src/models/users/model.rs Co-authored-by: Tom Signed-off-by: jarvarvarvis <53998846+jarvarvarvis@users.noreply.github.com> * fix: Implement suggested changes and clean up last 4 commits Signed-off-by: jarvarvarvis * fix: Disallow stoat as username, update create_user test Signed-off-by: jarvarvarvis * fix: Use sanitised username to find updated discriminator Signed-off-by: jarvarvarvis * feat: Sanitise revolt.chat in username Signed-off-by: jarvarvarvis * fix: Implement discussed changes Signed-off-by: jarvarvarvis * test: Fix create_user test Signed-off-by: jarvarvarvis * fix: don't overflow the stack not entirely sure why this fixes it and I don't like it. But work it does. Signed-off-by: IAmTomahawkx * fix: revert odd file mode change Signed-off-by: jarvarvarvis --------- Signed-off-by: higgs01 <6546697+higgs01@users.noreply.github.com> Signed-off-by: jarvarvarvis Signed-off-by: jarvarvarvis <53998846+jarvarvarvis@users.noreply.github.com> Signed-off-by: IAmTomahawkx Co-authored-by: higgs01 <6546697+higgs01@users.noreply.github.com> Co-authored-by: Tom --- Cargo.lock | 8 +- Cargo.toml | 4 +- crates/core/config/Revolt.toml | 2 + crates/core/config/src/lib.rs | 1 + .../src/models/servers/ops/mongodb.rs | 2 +- .../core/database/src/models/users/model.rs | 187 ++++++++++++++---- 6 files changed, 165 insertions(+), 39 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7322d38f..ce648f79 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2113,9 +2113,13 @@ dependencies = [ [[package]] name = "decancer" -version = "1.6.5" +version = "3.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "080b09f6adad25c23d8c47c54e52e59b0dc09d079c4b23e0f147dac440359d0d" +checksum = "a9244323129647178bf41ac861a2cdb9d9c81b9b09d3d0d1de9cd302b33b8a1d" +dependencies = [ + "lazy_static", + "regex", +] [[package]] name = "der" diff --git a/Cargo.toml b/Cargo.toml index 23b4ceb0..f10bfb26 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -171,7 +171,7 @@ config = "0.13.3" cached = "0.44.0" rand = "0.8.5" base64 = "0.21.3" -decancer = "1.6.2" +decancer = "3.3.3" linkify = "0.8.1" url-escape = "0.1.1" revolt_optional_struct = "0.2.0" @@ -198,4 +198,4 @@ revolt-parser = { version = "0.12.0", path = "crates/core/parser" } revolt-permissions = { version = "0.12.0", path = "crates/core/permissions" } revolt-presence = { version = "0.12.0", path = "crates/core/presence" } revolt-ratelimits = { version = "0.12.0", path = "crates/core/ratelimits" } -revolt-result = { version = "0.12.0", path = "crates/core/result" } \ No newline at end of file +revolt-result = { version = "0.12.0", path = "crates/core/result" } diff --git a/crates/core/config/Revolt.toml b/crates/core/config/Revolt.toml index 3bb33111..30a4622e 100644 --- a/crates/core/config/Revolt.toml +++ b/crates/core/config/Revolt.toml @@ -78,6 +78,8 @@ call_ring_duration = 30 [api.livekit.nodes] [api.users] +# Minimum allowed length of usernames +min_username_length = 2 [pushd] # this changes the names of the queues to not overlap diff --git a/crates/core/config/src/lib.rs b/crates/core/config/src/lib.rs index b9e60a4f..a49b1cdf 100644 --- a/crates/core/config/src/lib.rs +++ b/crates/core/config/src/lib.rs @@ -231,6 +231,7 @@ pub struct LiveKitNode { #[derive(Deserialize, Debug, Clone)] pub struct ApiUsers { pub early_adopter_cutoff: Option, + pub min_username_length: usize, } #[derive(Deserialize, Debug, Clone)] diff --git a/crates/core/database/src/models/servers/ops/mongodb.rs b/crates/core/database/src/models/servers/ops/mongodb.rs index ecdff53b..d8ef26f2 100644 --- a/crates/core/database/src/models/servers/ops/mongodb.rs +++ b/crates/core/database/src/models/servers/ops/mongodb.rs @@ -77,7 +77,7 @@ impl AbstractServers for MongoDb { }, doc! { "$set": { - "roles.".to_owned() + &role.id: to_document(role) + "roles.".to_owned() + role.id.as_str(): to_document(role) .map_err(|_| create_database_error!("to_document", "role"))? } }, diff --git a/crates/core/database/src/models/users/model.rs b/crates/core/database/src/models/users/model.rs index 0c2b6cee..bea37fac 100644 --- a/crates/core/database/src/models/users/model.rs +++ b/crates/core/database/src/models/users/model.rs @@ -7,6 +7,7 @@ use futures::future::join_all; use iso8601_timestamp::Timestamp; use once_cell::sync::Lazy; use rand::seq::SliceRandom; +use regex::{Regex, RegexBuilder}; use revolt_config::{config, FeaturesLimits}; use revolt_models::v0::{self, UserBadges, UserFlags}; use revolt_presence::filter_online; @@ -163,6 +164,13 @@ pub static DISCRIMINATOR_SEARCH_SPACE: Lazy> = Lazy::new(|| { set.into_iter().collect() }); +static BLOCKED_USERNAME_PATTERNS: Lazy = Lazy::new(|| { + RegexBuilder::new("`{3}|(discord|rvlt|guilded|stt)\\.gg|(revolt|stoat)\\.chat|https?:\\/\\/") + .case_insensitive(true) + .build() + .unwrap() +}); + #[allow(clippy::derivable_impls)] impl Default for User { fn default() -> Self { @@ -198,11 +206,13 @@ impl User { I: Into>, D: Into>, { - let username = User::validate_username(username)?; + let new_username = User::sanitise_username(&username).await?; + User::validate_username(&new_username)?; + let mut user = User { id: account_id.into().unwrap_or_else(|| Ulid::new().to_string()), - discriminator: User::find_discriminator(db, &username, None).await?, - username, + discriminator: User::find_discriminator(db, &new_username, None).await?, + username: new_username.clone(), last_acknowledged_policy_change: Timestamp::now_utc(), ..Default::default() }; @@ -278,39 +288,40 @@ impl User { } } - /// Sanitise and validate a username can be used - pub fn validate_username(username: String) -> Result { - // Copy the username for validation + /// Validate a username + /// + /// This will check if the username is a blocked name or contains a blocked pattern. + fn validate_username(username: &str) -> Result<()> { let username_lowercase = username.to_lowercase(); - // Block homoglyphs - if decancer::cure(&username_lowercase).into_str() != username_lowercase { + const BLOCKED_USERNAMES: &[&str] = &["admin", "revolt", "stoat"]; + + if BLOCKED_USERNAMES.contains(&username_lowercase.as_str()) + || BLOCKED_USERNAME_PATTERNS.is_match(username) + { return Err(create_error!(InvalidUsername)); } - // Ensure the username itself isn't blocked - const BLOCKED_USERNAMES: &[&str] = &["admin", "revolt"]; + Ok(()) + } - for username in BLOCKED_USERNAMES { - if username_lowercase == *username { - return Err(create_error!(InvalidUsername)); - } - } + /// Sanitise a username + /// + /// This will clean up Unicode homoglyphs and pad to the min username length with underscores. + async fn sanitise_username(username: &str) -> Result { + let options = decancer::Options::default().retain_capitalization(); + let mut username = decancer::cure(username, options) + .map_err(|_| create_error!(InvalidUsername))? + .to_string(); - // Ensure none of the following substrings show up in the username - const BLOCKED_SUBSTRINGS: &[&str] = &[ - "```", - "discord.gg", - "rvlt.gg", - "guilded.gg", - "https://", - "http://", - ]; - - for substr in BLOCKED_SUBSTRINGS { - if username_lowercase.contains(substr) { - return Err(create_error!(InvalidUsername)); - } + let config = revolt_config::config().await; + let username_length_diff = config + .api + .users + .min_username_length + .saturating_sub(username.len()); + if username_length_diff > 0 { + username.push_str(&"_".repeat(username_length_diff)) } Ok(username) @@ -416,12 +427,14 @@ impl User { /// Update a user's username pub async fn update_username(&mut self, db: &Database, username: String) -> Result<()> { - let username = User::validate_username(username)?; - if self.username.to_lowercase() == username.to_lowercase() { + let new_username = User::sanitise_username(&username).await?; + User::validate_username(&new_username)?; + + if self.username.to_lowercase() == new_username.to_lowercase() { self.update( db, PartialUser { - username: Some(username), + username: Some(new_username), ..Default::default() }, vec![], @@ -434,12 +447,12 @@ impl User { discriminator: Some( User::find_discriminator( db, - &username, + &new_username, Some((self.discriminator.to_string(), self.id.clone())), ) .await?, ), - username: Some(username), + username: Some(new_username), ..Default::default() }, vec![], @@ -825,3 +838,109 @@ impl User { badges } } + +#[cfg(test)] +mod tests { + use crate::User; + + #[test] + fn username_validation_blocked_names() { + let username_admin = "Admin"; + let username_revolt = "Revolt"; + let username_stoat = "Stoat"; + let username_allowed = "Allowed"; + + assert!(User::validate_username(username_admin).is_err()); + assert!(User::validate_username(username_revolt).is_err()); + assert!(User::validate_username(username_stoat).is_err()); + assert!(User::validate_username(username_allowed).is_ok()); + } + + #[test] + fn username_validation_blocked_patterns() { + let username_grave = "```_test"; + let username_discord = "discord.gg_test"; + let username_rvlt = "rvlt.gg_test"; + let username_guilded = "guilded.gg_test"; + let username_stt = "stt.gg_test"; + let username_revolt = "revolt.chat_test"; + let username_stoat = "stoat.chat_test"; + let username_http = "http://_test"; + let username_https = "https://_test"; + + assert!(User::validate_username(username_grave).is_err()); + assert!(User::validate_username(username_discord).is_err()); + assert!(User::validate_username(username_rvlt).is_err()); + assert!(User::validate_username(username_guilded).is_err()); + assert!(User::validate_username(username_stt).is_err()); + assert!(User::validate_username(username_revolt).is_err()); + assert!(User::validate_username(username_stoat).is_err()); + assert!(User::validate_username(username_http).is_err()); + assert!(User::validate_username(username_https).is_err()); + } + + #[async_std::test] + async fn username_sanitisation_clean() { + let username_clean = "Test"; + + let username_clean_sanitised = User::sanitise_username(username_clean).await; + + assert!(username_clean_sanitised.is_ok()); + assert_eq!(username_clean, username_clean_sanitised.unwrap()); + } + + #[async_std::test] + async fn username_sanitisation_homoglyphs() { + let username_homoglyphs = "𝔽𝕌Ňℕｙ"; + + let username_homoglyphs_sanitised = + User::sanitise_username(username_homoglyphs).await.unwrap(); + + assert_ne!(username_homoglyphs, username_homoglyphs_sanitised); + assert_eq!("funny", username_homoglyphs_sanitised); + } + + #[async_std::test] + async fn username_sanitisation_padding() { + let username_padding = "a"; + + let username = User::sanitise_username(username_padding).await.unwrap(); + + assert_eq!("a_", username); + } + + #[async_std::test] + async fn create_user() { + use revolt_result::Result; + + database_test!(|db| async move { + let mut created_clean = User::create(&db, "Test".to_string(), None, None) + .await + .unwrap(); + + assert_eq!("Test", created_clean.username); + + created_clean + .update_username(&db, "Test2".to_string()) + .await + .unwrap(); + + assert_eq!("Test2", created_clean.username); + + let created_invalid_result: Result<_> = + User::create(&db, "stoat.chat".to_string(), None, None).await; + + assert!(created_invalid_result.is_err()); + + let mut updated_invalid = User::create(&db, "Test".to_string(), None, None) + .await + .unwrap(); + + let updated_invalid_update_result = updated_invalid + .update_username(&db, "http://test".to_string()) + .await; + + assert!(updated_invalid_update_result.is_err()); + }); + } +}