Replace ammonia lib with manual html escaping (fixes #3774) (#3938)

* Replace ammonia lib with manual html escaping (fixes #3774)

* prettier

* clippy

* remove sanitize unit test

* fix tests

* fix schema

api_tests/src/post.spec.ts

@@ -513,13 +513,22 @@ test("Sanitize HTML", async () => {
   }
 
   let name = randomString(5);
-  let body = "<script>alert('xss');</script> hello";
+  let body = "<script>alert('xss');</script> hello &\"'";
   let form: CreatePost = {
     name,
     body,
     auth: beta.auth,
     community_id: betaCommunity.community.id,
   };
   let post = await beta.client.createPost(form);
-  expect(post.post_view.post.body).toBe(" hello");
+  // first escaping for the api
+  expect(post.post_view.post.body).toBe(
+    "&lt;script>alert(&#x27;xss&#x27;);&lt;/script> hello &amp;&quot;&#x27;",
+  );
+
+  let alphaPost = (await resolvePost(alpha, post.post_view.post)).post;
+  // second escaping over federation, avoid double escape of &
+  expect(alphaPost?.post.body).toBe(
+    "&lt;script>alert(&#x27;xss&#x27;);&lt;/script> hello &amp;&quot;&#x27;",
+  );
 });

crates/api/src/comment_report/create.rs

@@ -8,7 +8,7 @@ use lemmy_api_common::{
   utils::{
     check_community_ban,
     local_user_view_from_jwt,
-    sanitize_html,
+    sanitize_html_api,
     send_new_report_email_to_admins,
   },
 };
@@ -31,7 +31,7 @@ pub async fn create_comment_report(
   let local_user_view = local_user_view_from_jwt(&data.auth, &context).await?;
   let local_site = LocalSite::read(&mut context.pool()).await?;
 
-  let reason = sanitize_html(data.reason.trim());
+  let reason = sanitize_html_api(data.reason.trim());
   check_report_reason(&reason, &local_site)?;
 
   let person_id = local_user_view.person.id;

crates/api/src/community/ban.rs

@@ -8,7 +8,7 @@ use lemmy_api_common::{
     is_mod_or_admin,
     local_user_view_from_jwt,
     remove_user_data_in_community,
-    sanitize_html_opt,
+    sanitize_html_api_opt,
   },
 };
 use lemmy_db_schema::{
@@ -86,7 +86,7 @@ pub async fn ban_from_community(
     mod_person_id: local_user_view.person.id,
     other_person_id: data.person_id,
     community_id: data.community_id,
-    reason: sanitize_html_opt(&data.reason),
+    reason: sanitize_html_api_opt(&data.reason),
     banned: Some(data.ban),
     expires,
   };

crates/api/src/community/hide.rs

@@ -5,7 +5,7 @@ use lemmy_api_common::{
   community::{CommunityResponse, HideCommunity},
   context::LemmyContext,
   send_activity::{ActivityChannel, SendActivityData},
-  utils::{is_admin, local_user_view_from_jwt, sanitize_html_opt},
+  utils::{is_admin, local_user_view_from_jwt, sanitize_html_api_opt},
 };
 use lemmy_db_schema::{
   source::{
@@ -33,7 +33,7 @@ pub async fn hide_community(
   let mod_hide_community_form = ModHideCommunityForm {
     community_id: data.community_id,
     mod_person_id: local_user_view.person.id,
-    reason: sanitize_html_opt(&data.reason),
+    reason: sanitize_html_api_opt(&data.reason),
     hidden: Some(data.hidden),
   };
 

crates/api/src/local_user/ban_person.rs

@@ -4,7 +4,7 @@ use lemmy_api_common::{
   context::LemmyContext,
   person::{BanPerson, BanPersonResponse},
   send_activity::{ActivityChannel, SendActivityData},
-  utils::{is_admin, local_user_view_from_jwt, remove_user_data, sanitize_html_opt},
+  utils::{is_admin, local_user_view_from_jwt, remove_user_data, sanitize_html_api_opt},
 };
 use lemmy_db_schema::{
   source::{
@@ -54,7 +54,7 @@ pub async fn ban_from_site(
   let form = ModBanForm {
     mod_person_id: local_user_view.person.id,
     other_person_id: data.person_id,
-    reason: sanitize_html_opt(&data.reason),
+    reason: sanitize_html_api_opt(&data.reason),
     banned: Some(data.ban),
     expires,
   };

crates/api/src/local_user/save_settings.rs

@@ -2,7 +2,7 @@ use actix_web::web::{Data, Json};
 use lemmy_api_common::{
   context::LemmyContext,
   person::{LoginResponse, SaveUserSettings},
-  utils::{local_user_view_from_jwt, sanitize_html_opt, send_verification_email},
+  utils::{local_user_view_from_jwt, sanitize_html_api_opt, send_verification_email},
 };
 use lemmy_db_schema::{
   source::{
@@ -34,8 +34,8 @@ pub async fn save_user_settings(
   let local_user_view = local_user_view_from_jwt(&data.auth, &context).await?;
   let site_view = SiteView::read_local(&mut context.pool()).await?;
 
-  let bio = sanitize_html_opt(&data.bio);
-  let display_name = sanitize_html_opt(&data.display_name);
+  let bio = sanitize_html_api_opt(&data.bio);
+  let display_name = sanitize_html_api_opt(&data.display_name);
 
   let avatar = diesel_option_overwrite_to_url(&data.avatar)?;
   let banner = diesel_option_overwrite_to_url(&data.banner)?;
@@ -85,7 +85,7 @@ pub async fn save_user_settings(
   let person_id = local_user_view.person.id;
   let default_listing_type = data.default_listing_type;
   let default_sort_type = data.default_sort_type;
-  let theme = sanitize_html_opt(&data.theme);
+  let theme = sanitize_html_api_opt(&data.theme);
 
   let person_form = PersonUpdateForm {
     display_name,

crates/api/src/post_report/create.rs

@@ -8,7 +8,7 @@ use lemmy_api_common::{
   utils::{
     check_community_ban,
     local_user_view_from_jwt,
-    sanitize_html,
+    sanitize_html_api,
     send_new_report_email_to_admins,
   },
 };
@@ -31,7 +31,7 @@ pub async fn create_post_report(
   let local_user_view = local_user_view_from_jwt(&data.auth, &context).await?;
   let local_site = LocalSite::read(&mut context.pool()).await?;
 
-  let reason = sanitize_html(data.reason.trim());
+  let reason = sanitize_html_api(data.reason.trim());
   check_report_reason(&reason, &local_site)?;
 
   let person_id = local_user_view.person.id;

crates/api/src/private_message_report/create.rs

@@ -3,7 +3,7 @@ use actix_web::web::{Data, Json};
 use lemmy_api_common::{
   context::LemmyContext,
   private_message::{CreatePrivateMessageReport, PrivateMessageReportResponse},
-  utils::{local_user_view_from_jwt, sanitize_html, send_new_report_email_to_admins},
+  utils::{local_user_view_from_jwt, sanitize_html_api, send_new_report_email_to_admins},
 };
 use lemmy_db_schema::{
   source::{
@@ -24,7 +24,7 @@ pub async fn create_pm_report(
   let local_user_view = local_user_view_from_jwt(&data.auth, &context).await?;
   let local_site = LocalSite::read(&mut context.pool()).await?;
 
-  let reason = sanitize_html(data.reason.trim());
+  let reason = sanitize_html_api(data.reason.trim());
   check_report_reason(&reason, &local_site)?;
 
   let person_id = local_user_view.person.id;

crates/api/src/site/purge/comment.rs

@@ -2,7 +2,7 @@ use actix_web::web::{Data, Json};
 use lemmy_api_common::{
   context::LemmyContext,
   site::{PurgeComment, PurgeItemResponse},
-  utils::{is_admin, local_user_view_from_jwt, sanitize_html_opt},
+  utils::{is_admin, local_user_view_from_jwt, sanitize_html_api_opt},
 };
 use lemmy_db_schema::{
   source::{
@@ -35,7 +35,7 @@ pub async fn purge_comment(
   Comment::delete(&mut context.pool(), comment_id).await?;
 
   // Mod tables
-  let reason = sanitize_html_opt(&data.reason);
+  let reason = sanitize_html_api_opt(&data.reason);
   let form = AdminPurgeCommentForm {
     admin_person_id: local_user_view.person.id,
     reason,

crates/api/src/site/purge/community.rs

@@ -3,7 +3,12 @@ use lemmy_api_common::{
   context::LemmyContext,
   request::purge_image_from_pictrs,
   site::{PurgeCommunity, PurgeItemResponse},
-  utils::{is_admin, local_user_view_from_jwt, purge_image_posts_for_community, sanitize_html_opt},
+  utils::{
+    is_admin,
+    local_user_view_from_jwt,
+    purge_image_posts_for_community,
+    sanitize_html_api_opt,
+  },
 };
 use lemmy_db_schema::{
   source::{
@@ -42,7 +47,7 @@ pub async fn purge_community(
   Community::delete(&mut context.pool(), community_id).await?;
 
   // Mod tables
-  let reason = sanitize_html_opt(&data.reason);
+  let reason = sanitize_html_api_opt(&data.reason);
   let form = AdminPurgeCommunityForm {
     admin_person_id: local_user_view.person.id,
     reason,

crates/api/src/site/purge/person.rs

@@ -3,7 +3,7 @@ use lemmy_api_common::{
   context::LemmyContext,
   request::delete_image_from_pictrs,
   site::{PurgeItemResponse, PurgePerson},
-  utils::{is_admin, local_user_view_from_jwt, sanitize_html_opt},
+  utils::{is_admin, local_user_view_from_jwt, sanitize_html_api_opt},
 };
 use lemmy_db_schema::{
   source::{
@@ -42,7 +42,7 @@ pub async fn purge_person(
   Person::delete(&mut context.pool(), person_id).await?;
 
   // Mod tables
-  let reason = sanitize_html_opt(&data.reason);
+  let reason = sanitize_html_api_opt(&data.reason);
   let form = AdminPurgePersonForm {
     admin_person_id: local_user_view.person.id,
     reason,

crates/api/src/site/purge/post.rs

@@ -3,7 +3,7 @@ use lemmy_api_common::{
   context::LemmyContext,
   request::purge_image_from_pictrs,
   site::{PurgeItemResponse, PurgePost},
-  utils::{is_admin, local_user_view_from_jwt, sanitize_html_opt},
+  utils::{is_admin, local_user_view_from_jwt, sanitize_html_api_opt},
 };
 use lemmy_db_schema::{
   source::{
@@ -43,7 +43,7 @@ pub async fn purge_post(
   Post::delete(&mut context.pool(), post_id).await?;
 
   // Mod tables
-  let reason = sanitize_html_opt(&data.reason);
+  let reason = sanitize_html_api_opt(&data.reason);
   let form = AdminPurgePostForm {
     admin_person_id: local_user_view.person.id,
     reason,

crates/api_common/Cargo.toml

@@ -34,7 +34,6 @@ full = [
   "actix-web",
   "futures",
   "once_cell",
-  "ammonia",
 ]
 
 [dependencies]
@@ -67,4 +66,3 @@ once_cell = { workspace = true, optional = true }
 actix-web = { workspace = true, optional = true }
 # necessary for wasmt compilation
 getrandom = { version = "0.2.10", features = ["js"] }
-ammonia = { version = "3.3.0", optional = true }

crates/api_common/src/utils.rs

@@ -798,29 +798,43 @@ pub fn generate_moderators_url(community_id: &DbUrl) -> Result<DbUrl, LemmyError
   Ok(Url::parse(&format!("{community_id}/moderators"))?.into())
 }
 
-/// Sanitize HTML with default options. Additionally, dont allow bypassing markdown
-/// links and images
-pub fn sanitize_html(data: &str) -> String {
-  ammonia::Builder::default()
-    .rm_tags(&["a", "img"])
-    .clean(data)
-    .to_string()
-    // restore markdown quotes
-    .replace("&gt;", ">")
-    // restore white space
-    .replace("&nbsp;", " ")
+/// Replace special HTML characters in API parameters to prevent XSS attacks.
+///
+/// Taken from https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.md#output-encoding-for-html-contexts
+///
+/// `>` is left in place because it is interpreted as markdown quote.
+pub fn sanitize_html_api(data: &str) -> String {
+  data
+    .replace('&', "&amp;")
+    .replace('<', "&lt;")
+    .replace('\"', "&quot;")
+    .replace('\'', "&#x27;")
 }
 
-pub fn sanitize_html_opt(data: &Option<String>) -> Option<String> {
-  data.as_ref().map(|d| sanitize_html(d))
+pub fn sanitize_html_api_opt(data: &Option<String>) -> Option<String> {
+  data.as_ref().map(|d| sanitize_html_api(d))
+}
+
+/// Replace special HTML characters in federation parameters to prevent XSS attacks.
+///
+/// Unlike [sanitize_html_api()] it leaves `&` in place to avoid double escaping.
+pub fn sanitize_html_federation(data: &str) -> String {
+  data
+    .replace('<', "&lt;")
+    .replace('\"', "&quot;")
+    .replace('\'', "&#x27;")
+}
+
+pub fn sanitize_html_federation_opt(data: &Option<String>) -> Option<String> {
+  data.as_ref().map(|d| sanitize_html_federation(d))
 }
 
 #[cfg(test)]
 mod tests {
   #![allow(clippy::unwrap_used)]
   #![allow(clippy::indexing_slicing)]
 
-  use crate::utils::{honeypot_check, password_length_check, sanitize_html};
+  use crate::utils::{honeypot_check, password_length_check};
 
   #[test]
   #[rustfmt::skip]
@@ -838,14 +852,4 @@ mod tests {
     assert!(honeypot_check(&Some("1".to_string())).is_err());
     assert!(honeypot_check(&Some("message".to_string())).is_err());
   }
-
-  #[test]
-  fn test_sanitize_html() {
-    let sanitized = sanitize_html("<script>alert(1);</script> hello");
-    assert_eq!(sanitized, " hello");
-    let sanitized = sanitize_html("<img src='http://example.com'> test");
-    assert_eq!(sanitized, " test");
-    let sanitized = sanitize_html("Hello&nbsp;World");
-    assert_eq!(sanitized, "Hello World");
-  }
 }

crates/api_crud/src/comment/create.rs

@@ -13,7 +13,7 @@ use lemmy_api_common::{
     get_post,
     local_site_to_slur_regex,
     local_user_view_from_jwt,
-    sanitize_html,
+    sanitize_html_api,
     EndpointType,
   },
 };
@@ -52,7 +52,7 @@ pub async fn create_comment(
     &local_site_to_slur_regex(&local_site),
   );
   is_valid_body_field(&Some(content.clone()), false)?;
-  let content = sanitize_html(&content);
+  let content = sanitize_html_api(&content);
 
   // Check for a community ban
   let post_id = data.post_id;