Improve security

src/reader/sync.rs

@@ -1,4 +1,5 @@
 use super::feeds;
+use crate::auth::extractor::AuthUser;
 use crate::error::AppError;
 use crate::json_serialization::user::JsonUser;
 use crate::models::feed::rss_feed::Feed;
@@ -12,12 +13,13 @@ use crate::{
         feed_item,
     },
 };
-use actix_web::{web, HttpRequest, HttpResponse, Responder};
+use actix_web::{web, HttpRequest, HttpResponse};
 use chrono::{DateTime, Local, NaiveDateTime};
 use dateparser::parse;
 use diesel::prelude::*;
 use rss::Item;
 use scraper::{Html, Selector};
+use std::collections::{HashMap, HashSet};
 
 fn get_date(date_str: &str) -> Result<NaiveDateTime, chrono::ParseError> {
     if let Ok(result) = parse(date_str) {
@@ -47,6 +49,20 @@ fn escape_html_attr(value: &str) -> String {
         .replace('>', "&gt;")
 }
 
+// Feed-supplied `<img>` markup is rendered as-is (via `v-html`) in the frontend,
+// so strip everything except a harmless image tag before it's stored — in
+// particular event handlers like `onerror`/`onload` that a malicious feed
+// could use for XSS.
+fn sanitize_img_html(html: &str) -> String {
+    let allowed_attributes = HashSet::from(["src", "alt", "title"]);
+
+    ammonia::Builder::default()
+        .tags(HashSet::from(["img"]))
+        .tag_attributes(HashMap::from([("img", allowed_attributes)]))
+        .clean(html)
+        .to_string()
+}
+
 // Some feeds (e.g. Deutsche Welle) don't embed an <img> in the item content at
 // all — they carry the article image as an RSS <enclosure> instead. Build an
 // <img> tag from it so those feeds get a preview image too.
@@ -107,12 +123,12 @@ fn create_feed_item(item: Item, feed: &Feed, connection: &mut PgConnection) -> a
     let selector_img = Selector::parse("img").expect("\"img\" is a valid CSS selector");
     match frag.select(&selector_img).find(image_src_is_resolvable) {
         Some(image) => {
-            content.push_str(&image.html());
+            content.push_str(&sanitize_img_html(&image.html()));
             content.push_str("<br>");
         }
         None => {
             if let Some(image_html) = enclosure_image_html(&item) {
-                content.push_str(&image_html);
+                content.push_str(&sanitize_img_html(&image_html));
                 content.push_str("<br>");
             }
         }
@@ -155,11 +171,16 @@ fn create_feed_item(item: Item, feed: &Feed, connection: &mut PgConnection) -> a
 pub async fn sync(
     _req: HttpRequest,
     data: web::Json<JsonUser>,
-) -> Result<impl Responder, AppError> {
-    let mut connection: diesel::PgConnection = establish_connection();
-
+    auth_user: AuthUser,
+) -> Result<HttpResponse, AppError> {
     let req_user_id: i32 = data.user_id;
 
+    if auth_user.0 != req_user_id {
+        return Ok(HttpResponse::Forbidden().finish());
+    }
+
+    let mut connection: diesel::PgConnection = establish_connection();
+
     let feeds: Vec<Feed> = feed::table
         .filter(user_id.eq(req_user_id))
         .load::<Feed>(&mut connection)?;
@@ -183,7 +204,7 @@ pub async fn sync(
         }
     }
 
-    Ok(HttpResponse::Ok())
+    Ok(HttpResponse::Ok().finish())
 }
 
 #[cfg(test)]
@@ -281,6 +302,28 @@ mod tests {
         );
     }
 
+    #[test]
+    fn sanitize_img_html_strips_event_handlers() {
+        let sanitized = sanitize_img_html(r#"<img src="x" onerror="alert(1)">"#);
+
+        assert!(!sanitized.contains("onerror"));
+        assert!(!sanitized.contains("alert"));
+        assert!(sanitized.contains(r#"src="x""#));
+    }
+
+    #[test]
+    fn sanitize_img_html_keeps_only_allowed_attributes() {
+        let sanitized = sanitize_img_html(
+            r#"<img src="https://example.test/img.jpg" alt="desc" title="t" style="display:none" class="evil">"#,
+        );
+
+        assert!(sanitized.contains(r#"src="https://example.test/img.jpg""#));
+        assert!(sanitized.contains(r#"alt="desc""#));
+        assert!(sanitized.contains(r#"title="t""#));
+        assert!(!sanitized.contains("style"));
+        assert!(!sanitized.contains("class"));
+    }
+
     #[actix_web::test]
     async fn create_feed_item_inserts_articles_older_than_two_weeks() {
         let mut connection = establish_connection();
@@ -397,6 +440,62 @@ mod tests {
             .ok();
     }
 
+    #[actix_web::test]
+    async fn create_feed_item_strips_onerror_from_feed_image() {
+        let mut connection = establish_connection();
+        let suffix = unique_suffix();
+
+        let new_user = NewUser::new(
+            format!("xss_test_{suffix}"),
+            format!("xss_{suffix}@example.test"),
+            "secret".to_string(),
+        )
+        .unwrap();
+        let user: User = diesel::insert_into(users::table)
+            .values(&new_user)
+            .get_result(&mut connection)
+            .unwrap();
+
+        let new_feed = NewFeed::new(
+            format!("XSS test feed {suffix}"),
+            format!("https://example.test/feed/{suffix}"),
+            user.id,
+        );
+        let feed: Feed = diesel::insert_into(feed::table)
+            .values(&new_feed)
+            .get_result(&mut connection)
+            .unwrap();
+
+        let mut item = Item::default();
+        item.set_title(Some(format!("XSS article {suffix}")));
+        item.set_link(Some(format!("https://example.test/xss/{suffix}")));
+        item.set_content(Some(
+            r#"<img src="https://example.test/real.jpg" onerror="alert(1)"><p>text</p>"#
+                .to_string(),
+        ));
+
+        create_feed_item(item, &feed, &mut connection).unwrap();
+
+        let stored: FeedItem = feed_item::table
+            .filter(feed_id.eq(feed.id))
+            .first(&mut connection)
+            .unwrap();
+
+        assert!(!stored.content.contains("onerror"));
+        assert!(!stored.content.contains("alert"));
+        assert!(stored.content.contains(r#"src="https://example.test/real.jpg""#));
+
+        diesel::delete(feed_item::table.filter(feed_id.eq(feed.id)))
+            .execute(&mut connection)
+            .ok();
+        diesel::delete(feed::table.filter(feed::id.eq(feed.id)))
+            .execute(&mut connection)
+            .ok();
+        diesel::delete(users::table.filter(users::id.eq(user.id)))
+            .execute(&mut connection)
+            .ok();
+    }
+
     #[actix_web::test]
     async fn create_feed_item_strips_social_sharing_widget() {
         let mut connection = establish_connection();