From 44dda08b136750000fb646b9e411293da31a3025 Mon Sep 17 00:00:00 2001
From: Nutomic <me@nutomic.com>
Date: Tue, 1 Oct 2024 02:27:14 +0200
Subject: [PATCH] Avoid stack overflow when fetching nested comments, reduce
 max comment depth to 50 (#5009)

* Avoid stack overflow when fetching deeply nested comments

* add test case

* reduce comment depth, add docs

* decrease

* reduce max comment depth to 50

* fmt

* clippy

* cleanup
---
 Cargo.toml                               |  2 +-
 api_tests/src/comment.spec.ts            | 23 +++++++++++++++++++++++
 crates/api_crud/src/comment/create.rs    |  3 +--
 crates/apub/src/protocol/objects/note.rs | 19 ++++++++++++++-----
 crates/utils/src/lib.rs                  |  2 ++
 5 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 49a8e1d61..6b594a140 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -90,7 +90,7 @@ lemmy_db_views = { version = "=0.19.6-beta.7", path = "./crates/db_views" }
 lemmy_db_views_actor = { version = "=0.19.6-beta.7", path = "./crates/db_views_actor" }
 lemmy_db_views_moderator = { version = "=0.19.6-beta.7", path = "./crates/db_views_moderator" }
 lemmy_federate = { version = "=0.19.6-beta.7", path = "./crates/federate" }
-activitypub_federation = { version = "0.6.0-alpha1", default-features = false, features = [
+activitypub_federation = { version = "0.6.0-alpha2", default-features = false, features = [
   "actix-web",
 ] }
 diesel = "2.1.6"
diff --git a/api_tests/src/comment.spec.ts b/api_tests/src/comment.spec.ts
index c18469e33..5f2059e4f 100644
--- a/api_tests/src/comment.spec.ts
+++ b/api_tests/src/comment.spec.ts
@@ -858,3 +858,26 @@ test("Dont send a comment reply to a blocked community", async () => {
   blockRes = await blockCommunity(beta, newCommunityId, false);
   expect(blockRes.blocked).toBe(false);
 });
+
+/// Fetching a deeply nested comment can lead to stack overflow as all parent comments are also
+/// fetched recursively. Ensure that it works properly.
+test("Fetch a deeply nested comment", async () => {
+  let lastComment;
+  for (let i = 0; i < 50; i++) {
+    let commentRes = await createComment(
+      alpha,
+      postOnAlphaRes.post_view.post.id,
+      lastComment?.comment_view.comment.id,
+    );
+    expect(commentRes.comment_view.comment).toBeDefined();
+    lastComment = commentRes;
+  }
+
+  let betaComment = await resolveComment(
+    beta,
+    lastComment!.comment_view.comment,
+  );
+
+  expect(betaComment!.comment!.comment).toBeDefined();
+  expect(betaComment?.comment?.post).toBeDefined();
+});
diff --git a/crates/api_crud/src/comment/create.rs b/crates/api_crud/src/comment/create.rs
index 795c27b35..273ab7a5f 100644
--- a/crates/api_crud/src/comment/create.rs
+++ b/crates/api_crud/src/comment/create.rs
@@ -30,10 +30,9 @@ use lemmy_db_views::structs::{LocalUserView, PostView};
 use lemmy_utils::{
   error::{LemmyErrorExt, LemmyErrorType, LemmyResult},
   utils::{mention::scrape_text_for_mentions, validation::is_valid_body_field},
+  MAX_COMMENT_DEPTH_LIMIT,
 };
 
-const MAX_COMMENT_DEPTH_LIMIT: usize = 100;
-
 #[tracing::instrument(skip(context))]
 pub async fn create_comment(
   data: Json<CreateComment>,
diff --git a/crates/apub/src/protocol/objects/note.rs b/crates/apub/src/protocol/objects/note.rs
index a092cec9f..e3e204254 100644
--- a/crates/apub/src/protocol/objects/note.rs
+++ b/crates/apub/src/protocol/objects/note.rs
@@ -20,10 +20,9 @@ use lemmy_db_schema::{
   source::{community::Community, post::Post},
   traits::Crud,
 };
-use lemmy_utils::error::LemmyResult;
+use lemmy_utils::{error::LemmyResult, LemmyErrorType, MAX_COMMENT_DEPTH_LIMIT};
 use serde::{Deserialize, Serialize};
 use serde_with::skip_serializing_none;
-use std::ops::Deref;
 use url::Url;
 
 #[skip_serializing_none]
@@ -58,9 +57,19 @@ impl Note {
     &self,
     context: &Data<LemmyContext>,
   ) -> LemmyResult<(ApubPost, Option<ApubComment>)> {
-    // Fetch parent comment chain in a box, otherwise it can cause a stack overflow.
-    let parent = Box::pin(self.in_reply_to.dereference(context).await?);
-    match parent.deref() {
+    // We use recursion here to fetch the entire comment chain up to the top-level parent. This is
+    // necessary because we need to know the post and parent comment in order to insert a new
+    // comment. However it can also lead to stack overflow when fetching many comments recursively.
+    // To avoid this we check the request count against max comment depth, which based on testing
+    // can be handled without risking stack overflow. This is not a perfect solution, because in
+    // some cases we have to fetch user profiles too, and reach the limit after only 25 comments
+    // or so.
+    // A cleaner solution would be converting the recursion into a loop, but that is tricky.
+    if context.request_count() > MAX_COMMENT_DEPTH_LIMIT as u32 {
+      Err(LemmyErrorType::MaxCommentDepthReached)?;
+    }
+    let parent = self.in_reply_to.dereference(context).await?;
+    match parent {
       PostOrComment::Post(p) => Ok((p.clone(), None)),
       PostOrComment::Comment(c) => {
         let post_id = c.post_id;
diff --git a/crates/utils/src/lib.rs b/crates/utils/src/lib.rs
index 5a5e76d2a..7f0691496 100644
--- a/crates/utils/src/lib.rs
+++ b/crates/utils/src/lib.rs
@@ -29,6 +29,8 @@ pub const CACHE_DURATION_FEDERATION: Duration = Duration::from_secs(60);
 
 pub const CACHE_DURATION_API: Duration = Duration::from_secs(1);
 
+pub const MAX_COMMENT_DEPTH_LIMIT: usize = 50;
+
 #[macro_export]
 macro_rules! location_info {
   () => {