Repository: LemmyNet/lemmy (language: rust)

Issue:
[Bug]: Comment requests are not idempotent
### Requirements

- [X] Is this a bug report? For questions or discussions use https://lemmy.ml/c/lemmy_support
- [X] Did you check to see if this issue already exists?
- [X] Is this only a single bug? Do not put multiple bugs in one issue.
- [X] Do you agree to follow the rules in our [Code of Conduct](https://join-lemmy.org/docs/code_of_conduct.html)?
- [X] Is this a backend issue? Use the [lemmy-ui](https://github.com/LemmyNet/lemmy-ui) repo for UI / frontend issues.

### Summary

Duplicate comment requests result in multiple copies of the comment being posted. The requests should be idempotent, so that sending the same request multiple times does not result in multiple comments. If a Lemmy client does not get a response due to a network issue, the user will be told the comment wasn't posted and will retry. If the original request did in fact succeed, this will result in a duplicate comment being posted.

### Steps to Reproduce

1. Send the same comment request multiple times.
2. Multiple comments are posted, instead of one.



### Technical Details

Reproduced in the Boost for Lemmy Android app.

### Version

BE: 0.19.3

### Lemmy Instance URL

_No response_


Discussion / hints:
How can we tell that its the same request? The [comment form](https://github.com/LemmyNet/lemmy/blob/main/crates/api_common/src/comment.rs#L17) doesnt have any sort of unique identifier. I suppose we could check for existing comments with identical content, post_id and parent_id, but that might give some false positives.
I would think this would require adding a `request_id`/`comment_id` field that the client includes by sending a random value, and the backend ignores duplicate requests with the same id. The client would then change the id if it's a different comment, or keep it the same if it's retrying the same comment.
There's already a `comment.id` field that's returned after the comment has been created, IE `CommentResponse.comment_view.comment.id`

Anyways this problem would affect not just comment, but any create operation, from posts, private messages, likes, etc. The back-end can't know about requests that failed, and shouldn't be storing extra "client-side" ids just to check for dupes. 

> If a Lemmy client does not get a response due to a network issue, the user will be told the comment wasn't posted and will retry. If the original request did in fact succeed, this will result in a duplicate comment being posted.

The real culprit here is how clients are incorrectly handling failed requests. Issues need to be opened up on those app's repos for their failure logic.

The only other way I could see this being fixed, is to move ID generation to the client side and use UUIDs or something, which isn't a good idea. 

> here's already a comment.id field that's returned after the comment has been created

Sorry if I'm missing something, but that does not seem relevant for this issue? If I client does not receive a response, it doesn't matter what the response contains.

> The real culprit here is how clients are incorrectly handling failed requests.

What would be the correct behavior in your opinion, when a client sends a request and does not receive a response? It's impossible to tell for the client if the request actually succeeded or not. I'd gladly open an issue in client projects but I don't see any incorrect behavior on their side, correct me if I'm wrong.

This is a real issue that got me banned from an instance. I posted a comment while on a train with a spotty mobile connection, the request timed out, and my client correctly told me the request timed out and offered to retry, which I did a few times. Hours later I discovered all the timed out requests actually worked and I got banned for spamming. I'm sure I'm not unique in this.

It's a common problem that's being solved in a lot of APIs, here are a few examples:
https://api-docs.deliveroo.com/docs/signature-idempotent-requests
https://docs.stripe.com/api/idempotent_requests
https://shopify.dev/docs/api/usage/idempotent-requests
https://developer.squareup.com/docs/build-basics/common-api-patterns/idempotency

The description from one of the above links sounds reasonable to me:

>To perform an idempotent request, provide an Idempotency-Key: <key> header in the request.
>
>An idempotency key is any unique client-generated value with sufficient entropy to avoid collisions. We recommend using V4 UUIDs as your keys.
>
>For idempotent requests, the API saves the status code and body of the response of the initial request and, when the same idempotency key is used in another request, it replays the saved response. 

> Anyways this problem would affect not just comment, but any create operation, from posts, private messages, likes, etc.

That's right, ideally the idempotency header would be supported by all of them.
Enforcing uniqueness for client-side generated UUIDs could have some benefits, but adding these for every create operation would be a lot of work. I'll re-open if you or anyone else wants to work on it tho.
The server could prevent creating a comment with the same content, parent comment, and creator as an existing comment, unless the check is disabled using an optional api parameter
If we had something like that, It'd be nice if it could be made as a DB constraint. 
Reopening this because its definitely a real issue.

A better way to solve it might be through a middleware, with an HTTP header containing the request id. That would work similar to the existing session middleware which handles logins. Then it would automatically work for all requests and wouldnt need any database changes. The request ids can be stored in memory.
> The request ids can be stored in memory

that is not reliable when running more than one lemmy process
Depends on the load balancing method. (If based on IP) It can be pretty reliable. Could be worthy tradeoff (fast + simple) . 
All though you could easily persist this as [RID + SID+ Resource type] Where RID is request id, SID session id+ Resource Type (Post/Comment/Dm). You can auto delete entries that are older than 1 hour as they don't matter anymore. 

Fix patch:
diff --git a/crates/utils/src/rate_limit/rate_limiter.rs b/crates/utils/src/rate_limit/rate_limiter.rs
index 68f248d6c5..7fe239f79c 100644
--- a/crates/utils/src/rate_limit/rate_limiter.rs
+++ b/crates/utils/src/rate_limit/rate_limiter.rs
@@ -13,9 +13,9 @@ static START_TIME: LazyLock<Instant> = LazyLock::new(Instant::now);
 
 /// Smaller than `std::time::Instant` because it uses a smaller integer for seconds and doesn't
 /// store nanoseconds
-#[derive(PartialEq, Debug, Clone, Copy)]
+#[derive(PartialEq, Debug, Clone, Copy, Hash)]
 pub struct InstantSecs {
-  secs: u32,
+  pub secs: u32,
 }
 
 #[allow(clippy::expect_used)]
diff --git a/src/idempotency_middleware.rs b/src/idempotency_middleware.rs
new file mode 100644
index 0000000000..2598cd12eb
--- /dev/null
+++ b/src/idempotency_middleware.rs
@@ -0,0 +1,176 @@
+use actix_web::{
+  body::EitherBody,
+  dev::{forward_ready, Service, ServiceRequest, ServiceResponse, Transform},
+  http::Method,
+  Error,
+  HttpMessage,
+  HttpResponse,
+};
+use futures_util::future::LocalBoxFuture;
+use lemmy_api_common::lemmy_db_views::structs::LocalUserView;
+use lemmy_db_schema::newtypes::LocalUserId;
+use lemmy_utils::rate_limit::rate_limiter::InstantSecs;
+use std::{
+  collections::HashSet,
+  future::{ready, Ready},
+  hash::{Hash, Hasher},
+  sync::{Arc, RwLock},
+  time::Duration,
+};
+
+/// https://www.ietf.org/archive/id/draft-ietf-httpapi-idempotency-key-header-01.html
+const IDEMPOTENCY_HEADER: &str = "Idempotency-Key";
+
+/// Delete idempotency keys older than this
+const CLEANUP_INTERVAL_SECS: u32 = 120;
+
+#[derive(Debug)]
+struct Entry {
+  user_id: LocalUserId,
+  key: String,
+  // Creation time is ignored for Eq, Hash and only used to cleanup old entries
+  created: InstantSecs,
+}
+
+impl PartialEq for Entry {
+  fn eq(&self, other: &Self) -> bool {
+    self.user_id == other.user_id && self.key == other.key
+  }
+}
+impl Eq for Entry {}
+
+impl Hash for Entry {
+  fn hash<H: Hasher>(&self, state: &mut H) {
+    self.user_id.hash(state);
+    self.key.hash(state);
+  }
+}
+
+#[derive(Clone)]
+pub struct IdempotencySet {
+  set: Arc<RwLock<HashSet<Entry>>>,
+}
+
+impl Default for IdempotencySet {
+  fn default() -> Self {
+    let set: Arc<RwLock<HashSet<Entry>>> = Default::default();
+
+    let set_ = set.clone();
+    tokio::spawn(async move {
+      let interval = Duration::from_secs(CLEANUP_INTERVAL_SECS.into());
+      let state_weak_ref = Arc::downgrade(&set_);
+
+      // Run at every interval to delete entries older than the interval.
+      // This loop stops when all other references to `state` are dropped.
+      while let Some(state) = state_weak_ref.upgrade() {
+        tokio::time::sleep(interval).await;
+        let now = InstantSecs::now();
+        #[allow(clippy::expect_used)]
+        let mut lock = state.write().expect("lock failed");
+        lock.retain(|e| e.created.secs > now.secs.saturating_sub(CLEANUP_INTERVAL_SECS));
+        lock.shrink_to_fit();
+      }
+    });
+    Self { set }
+  }
+}
+
+pub struct IdempotencyMiddleware {
+  idempotency_set: IdempotencySet,
+}
+
+impl IdempotencyMiddleware {
+  pub fn new(idempotency_set: IdempotencySet) -> Self {
+    Self { idempotency_set }
+  }
+}
+
+impl<S, B> Transform<S, ServiceRequest> for IdempotencyMiddleware
+where
+  S: Service<ServiceRequest, Response = ServiceResponse<B>, Error = Error>,
+  S::Future: 'static,
+  B: 'static,
+{
+  type Response = ServiceResponse<EitherBody<B>>;
+  type Error = Error;
+  type InitError = ();
+  type Transform = IdempotencyService<S>;
+  type Future = Ready<Result<Self::Transform, Self::InitError>>;
+
+  fn new_transform(&self, service: S) -> Self::Future {
+    ready(Ok(IdempotencyService {
+      service,
+      idempotency_set: self.idempotency_set.clone(),
+    }))
+  }
+}
+
+pub struct IdempotencyService<S> {
+  service: S,
+  idempotency_set: IdempotencySet,
+}
+
+impl<S, B> Service<ServiceRequest> for IdempotencyService<S>
+where
+  S: Service<ServiceRequest, Response = ServiceResponse<B>, Error = Error>,
+  S::Future: 'static,
+  B: 'static,
+{
+  type Response = ServiceResponse<EitherBody<B>>;
+  type Error = Error;
+  type Future = LocalBoxFuture<'static, Result<Self::Response, Self::Error>>;
+
+  forward_ready!(service);
+
+  #[allow(clippy::expect_used)]
+  fn call(&self, req: ServiceRequest) -> Self::Future {
+    let is_post_or_put = req.method() == Method::POST || req.method() == Method::PUT;
+    let idempotency = req
+      .headers()
+      .get(IDEMPOTENCY_HEADER)
+      .map(|i| i.to_str().unwrap_or_default().to_string())
+      // Ignore values longer than 32 chars
+      .and_then(|i| (i.len() <= 32).then_some(i))
+      // Only use idempotency for POST and PUT requests
+      .and_then(|i| is_post_or_put.then_some(i));
+
+    let user_id = {
+      let ext = req.extensions();
+      ext.get().map(|u: &LocalUserView| u.local_user.id)
+    };
+
+    if let (Some(key), Some(user_id)) = (idempotency, user_id) {
+      let value = Entry {
+        user_id,
+        key,
+        created: InstantSecs::now(),
+      };
+      if self
+        .idempotency_set
+        .set
+        .read()
+        .expect("lock failed")
+        .contains(&value)
+      {
+        // Duplicate request, return error
+        let (req, _pl) = req.into_parts();
+        let response = HttpResponse::UnprocessableEntity()
+          .finish()
+          .map_into_right_body();
+        return Box::pin(async { Ok(ServiceResponse::new(req, response)) });
+      } else {
+        // New request, store key and continue
+        self
+          .idempotency_set
+          .set
+          .write()
+          .expect("lock failed")
+          .insert(value);
+      }
+    }
+
+    let fut = self.service.call(req);
+
+    Box::pin(async move { fut.await.map(ServiceResponse::map_into_left_body) })
+  }
+}
diff --git a/src/lib.rs b/src/lib.rs
index bd84d02640..2de3b106ba 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1,6 +1,7 @@
 pub mod api_routes_v3;
 pub mod api_routes_v4;
 pub mod code_migrations;
+pub mod idempotency_middleware;
 pub mod prometheus_metrics;
 pub mod scheduled_tasks;
 pub mod session_middleware;
@@ -18,6 +19,7 @@ use actix_web::{
 };
 use actix_web_prom::PrometheusMetricsBuilder;
 use clap::{Parser, Subcommand};
+use idempotency_middleware::{IdempotencyMiddleware, IdempotencySet};
 use lemmy_api::sitemap::get_sitemap;
 use lemmy_api_common::{
   context::LemmyContext,
@@ -334,6 +336,9 @@ fn create_http_server(
     .build()
     .map_err(|e| LemmyErrorType::Unknown(format!("Should always be buildable: {e}")))?;
 
+  // Must create this outside of HTTP server so that duplicate requests get detected across threads.
+  let idempotency_set = IdempotencySet::default();
+
   // Create Http server
   let bind = (settings.bind, settings.port);
   let server = HttpServer::new(move || {
@@ -355,6 +360,7 @@ fn create_http_server(
       .app_data(Data::new(context.clone()))
       .app_data(Data::new(rate_limit_cell.clone()))
       .wrap(FederationMiddleware::new(federation_config.clone()))
+      .wrap(IdempotencyMiddleware::new(idempotency_set.clone()))
       .wrap(SessionMiddleware::new(context.clone()))
       .wrap(Condition::new(
         SETTINGS.prometheus.is_some(),


Test patch: