From b573c92a190fc9ee7dbfcef68c6ff80c100028ee Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Thu, 16 Jan 2025 13:14:55 +0100 Subject: [PATCH] Add validation for article title and user/displayname --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/backend/api/article.rs | 29 ++++++-------- src/backend/api/user.rs | 7 +++- src/backend/database/article.rs | 6 +-- src/backend/federation/objects/article.rs | 5 ++- src/backend/utils/mod.rs | 1 + src/backend/utils/validate.rs | 47 +++++++++++++++++++++++ src/frontend/markdown/mod.rs | 4 +- tests/test.rs | 7 ++-- 10 files changed, 79 insertions(+), 31 deletions(-) create mode 100644 src/backend/utils/validate.rs diff --git a/Cargo.lock b/Cargo.lock index b40c9f7..19b9d83 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2031,9 +2031,9 @@ dependencies = [ "markdown-it-sub", "markdown-it-sup", "mime_guess", - "once_cell", "pretty_assertions", "rand", + "regex", "reqwest", "retry_future", "send_wrapper", diff --git a/Cargo.toml b/Cargo.toml index 985e4eb..be65d2a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,7 +44,6 @@ serde = { version = "1.0.217", features = ["derive"] } url = { version = "2.5.4", features = ["serde"] } log = "0.4" tracing = "0.1.41" -once_cell = "1.20.2" console_error_panic_hook = "0.1.7" time = "0.3.37" markdown-it = "0.6.1" @@ -107,6 +106,7 @@ include_dir = "0.7.4" mime_guess = "2.0.5" clokwerk = "0.4.0" fmtm = "0.0.3" +regex = "1.11.1" [dev-dependencies] pretty_assertions = "1.4.1" diff --git a/src/backend/api/article.rs b/src/backend/api/article.rs index 336fc77..0b760c8 100644 --- a/src/backend/api/article.rs +++ b/src/backend/api/article.rs @@ -8,7 +8,7 @@ use crate::{ IbisData, }, federation::activities::{create_article::CreateArticle, submit_article_update}, - utils::{error::MyResult, generate_article_version}, + utils::{error::MyResult, generate_article_version, validate::validate_article_title}, }, common::{ article::{ @@ -46,25 +46,19 @@ use diffy::create_patch; pub(in crate::backend::api) async fn create_article( user: Extension, data: Data, - Form(create_article): Form, + Form(mut params): Form, ) -> MyResult> { - if create_article.title.is_empty() { - return Err(anyhow!("Title must not be empty").into()); - } - if create_article.title.contains('/') { - return Err(anyhow!("Invalid character `/`").into()); - } + params.title = validate_article_title(¶ms.title)?; let local_instance = DbInstance::read_local_instance(&data)?; - let escaped_title = create_article.title.replace(' ', "_"); let ap_id = ObjectId::parse(&format!( "{}://{}/article/{}", http_protocol_str(), extract_domain(&local_instance.ap_id), - escaped_title + params.title ))?; let form = DbArticleForm { - title: create_article.title, + title: params.title, text: String::new(), ap_id, instance_id: local_instance.id, @@ -76,8 +70,8 @@ pub(in crate::backend::api) async fn create_article( let edit_data = EditArticleForm { article_id: article.id, - new_text: create_article.text, - summary: create_article.summary, + new_text: params.text, + summary: params.summary, previous_version_id: article.latest_edit_version(&data)?, resolve_conflict_id: None, }; @@ -204,20 +198,21 @@ pub(in crate::backend::api) async fn list_articles( pub(in crate::backend::api) async fn fork_article( Extension(_user): Extension, data: Data, - Form(fork_form): Form, + Form(mut params): Form, ) -> MyResult> { // TODO: lots of code duplicated from create_article(), can move it into helper - let original_article = DbArticle::read_view(fork_form.article_id, &data)?; + let original_article = DbArticle::read_view(params.article_id, &data)?; + params.new_title = validate_article_title(¶ms.new_title)?; let local_instance = DbInstance::read_local_instance(&data)?; let ap_id = ObjectId::parse(&format!( "{}://{}/article/{}", http_protocol_str(), extract_domain(&local_instance.ap_id), - &fork_form.new_title + ¶ms.new_title ))?; let form = DbArticleForm { - title: fork_form.new_title, + title: params.new_title, text: original_article.article.text.clone(), ap_id, instance_id: local_instance.id, diff --git a/src/backend/api/user.rs b/src/backend/api/user.rs index baa19b2..e818096 100644 --- a/src/backend/api/user.rs +++ b/src/backend/api/user.rs @@ -2,7 +2,10 @@ use super::{check_is_admin, empty_to_none}; use crate::{ backend::{ database::{conflict::DbConflict, read_jwt_secret, IbisData}, - utils::error::MyResult, + utils::{ + error::MyResult, + validate::{validate_display_name, validate_user_name}, + }, }, common::{ article::DbArticle, @@ -83,6 +86,7 @@ pub(in crate::backend::api) async fn register_user( if !data.config.options.registration_open { return Err(anyhow!("Registration is closed").into()); } + validate_user_name(&form.username)?; let user = DbPerson::create_local(form.username, form.password, false, &data)?; let token = generate_login_token(&user.person, &data)?; let jar = jar.add(create_cookie(token, &data)); @@ -153,6 +157,7 @@ pub(in crate::backend::api) async fn update_user_profile( ) -> MyResult> { empty_to_none(&mut params.display_name); empty_to_none(&mut params.bio); + validate_display_name(¶ms.display_name)?; DbPerson::update_profile(¶ms, &data)?; Ok(Json(SuccessResponse::default())) } diff --git a/src/backend/database/article.rs b/src/backend/database/article.rs index 4567983..30cdb54 100644 --- a/src/backend/database/article.rs +++ b/src/backend/database/article.rs @@ -45,16 +45,14 @@ impl DbArticle { Ok(CollectionId::parse(&format!("{}/edits", self.ap_id))?) } - pub fn create(mut form: DbArticleForm, data: &IbisData) -> MyResult { - form.title = form.title.replace(' ', "_"); + pub fn create(form: DbArticleForm, data: &IbisData) -> MyResult { let mut conn = data.db_pool.get()?; Ok(insert_into(article::table) .values(form) .get_result(conn.deref_mut())?) } - pub fn create_or_update(mut form: DbArticleForm, data: &IbisData) -> MyResult { - form.title = form.title.replace(' ', "_"); + pub fn create_or_update(form: DbArticleForm, data: &IbisData) -> MyResult { let mut conn = data.db_pool.get()?; Ok(insert_into(article::table) .values(&form) diff --git a/src/backend/federation/objects/article.rs b/src/backend/federation/objects/article.rs index 21ef3cd..9bbf694 100644 --- a/src/backend/federation/objects/article.rs +++ b/src/backend/federation/objects/article.rs @@ -2,7 +2,7 @@ use crate::{ backend::{ database::{article::DbArticleForm, IbisData}, federation::objects::edits_collection::DbEditCollection, - utils::error::Error, + utils::{error::Error, validate::validate_article_title}, }, common::{ article::{DbArticle, EditVersion}, @@ -75,7 +75,7 @@ impl Object for DbArticle { async fn from_json(json: Self::Kind, data: &Data) -> Result { let instance = json.attributed_to.dereference(data).await?; - let form = DbArticleForm { + let mut form = DbArticleForm { title: json.name, text: json.content, ap_id: json.id, @@ -84,6 +84,7 @@ impl Object for DbArticle { protected: json.protected, approved: true, }; + form.title = validate_article_title(&form.title)?; let article = DbArticle::create_or_update(form, data)?; json.edits.dereference(&article, data).await?; diff --git a/src/backend/utils/mod.rs b/src/backend/utils/mod.rs index fbd99ab..858f47a 100644 --- a/src/backend/utils/mod.rs +++ b/src/backend/utils/mod.rs @@ -17,6 +17,7 @@ use url::{ParseError, Url}; pub mod error; pub(super) mod scheduled_tasks; +pub(super) mod validate; pub(super) fn generate_activity_id(data: &Data) -> Result { let domain = &data.config.federation.domain; diff --git a/src/backend/utils/validate.rs b/src/backend/utils/validate.rs new file mode 100644 index 0000000..16adb56 --- /dev/null +++ b/src/backend/utils/validate.rs @@ -0,0 +1,47 @@ +use super::error::MyResult; +use anyhow::anyhow; +use regex::Regex; +use std::sync::LazyLock; + +pub fn validate_article_title(title: &str) -> MyResult { + #[expect(clippy::expect_used)] + static TITLE_REGEX: LazyLock = + LazyLock::new(|| Regex::new(r"^[a-zA-Z0-9_]{3,100}$").expect("compile regex")); + let title = title.replace(' ', "_"); + if !TITLE_REGEX.is_match(&title) { + return Err(anyhow!("Invalid title").into()); + } + Ok(title) +} + +pub fn validate_user_name(name: &str) -> MyResult<()> { + #[allow(clippy::expect_used)] + static VALID_ACTOR_NAME_REGEX: LazyLock = + LazyLock::new(|| Regex::new(r"^[a-zA-Z0-9_]{3,20}$").expect("compile regex")); + + if VALID_ACTOR_NAME_REGEX.is_match(name) { + Ok(()) + } else { + Err(anyhow!("Invalid username").into()) + } +} + +pub fn validate_display_name(name: &Option) -> MyResult<()> { + if let Some(name) = name { + if name.contains('@') || name.len() < 3 || name.len() > 20 { + return Err(anyhow!("Invalid displayname").into()); + } + } + Ok(()) +} + +#[test] +#[expect(clippy::unwrap_used)] +fn test_validate_article_title() { + assert_eq!( + validate_article_title("With space 123").unwrap(), + "With_space_123" + ); + assert!(validate_article_title(&"long".to_string().repeat(100)).is_err()); + assert!(validate_article_title("a").is_err()); +} diff --git a/src/frontend/markdown/mod.rs b/src/frontend/markdown/mod.rs index b680d03..8a74609 100644 --- a/src/frontend/markdown/mod.rs +++ b/src/frontend/markdown/mod.rs @@ -6,14 +6,14 @@ use markdown_it::{ MarkdownIt, }; use math_equation::MathEquationScanner; -use once_cell::sync::OnceCell; +use std::sync::OnceLock; pub mod article_link; pub mod math_equation; pub mod toc; pub fn render_markdown(text: &str) -> String { - static INSTANCE: OnceCell = OnceCell::new(); + static INSTANCE: OnceLock = OnceLock::new(); let mut parsed = INSTANCE.get_or_init(markdown_parser).parse(text); // Make markdown headings one level smaller, so that h1 becomes h2 etc, and markdown titles diff --git a/tests/test.rs b/tests/test.rs index 71557a1..7b903e8 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -28,13 +28,14 @@ async fn test_create_read_and_edit_local_article() -> Result<()> { let TestData(alpha, beta, gamma) = TestData::start(false).await; // create article + const TITLE: &'static str = "Manu_Chao"; let create_form = CreateArticleForm { - title: "Manu_Chao".to_string(), + title: "Manu Chao".to_string(), text: TEST_ARTICLE_DEFAULT_TEXT.to_string(), summary: "create article".to_string(), }; let create_res = alpha.create_article(&create_form).await.unwrap(); - assert_eq!(create_form.title, create_res.article.title); + assert_eq!(TITLE, create_res.article.title); assert!(create_res.article.local); // now article can be read @@ -44,7 +45,7 @@ async fn test_create_read_and_edit_local_article() -> Result<()> { id: None, }; let get_res = alpha.get_article(get_article_data.clone()).await.unwrap(); - assert_eq!(create_form.title, get_res.article.title); + assert_eq!(TITLE, get_res.article.title); assert_eq!(TEST_ARTICLE_DEFAULT_TEXT, get_res.article.text); assert!(get_res.article.local);