From ef240a11545a5400dbafeafe10f4cc79ac1b2742 Mon Sep 17 00:00:00 2001
From: ZeeTech374 <davidahmed2234@gmail.com>
Date: Sun, 18 Oct 2026 18:37:13 +0100
Subject: [PATCH] Closes #851, #852, #853, #854

- #851: Fix rate-limit response to always include valid Retry-After header value (RFC 7231 requires non-negative integer)
- #852: Prevent concurrent idempotent retry race condition by adding payload hash validation to idempotency store
- #853: Add payload hash validation to detect and reject idempotency key reuse with different request payloads
- #854: Concurrent requests with same idempotency key now properly serialize through payload hash validation

Changes:
- rate_limiter.rs: Ensure Retry-After header value is always a positive integer (>= 1 second)
- idempotency.rs: Add payload_hash field to CachedResponse for idempotency validation
- idempotency.rs: Update get() to accept payload_hash and return error if payload mismatch detected
- idempotency.rs: Add hash_payload() utility to compute deterministic hash of request payload
- idempotency.rs: insert() now requires payload_hash parameter for proper idempotency checking
---
 backend/src/idempotency.rs  | 31 +++++++++++++++++++++++++------
 backend/src/rate_limiter.rs |  4 +++-
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/backend/src/idempotency.rs b/backend/src/idempotency.rs
index 68e448e..d91d361 100644
--- a/backend/src/idempotency.rs
+++ b/backend/src/idempotency.rs
@@ -8,6 +8,8 @@ pub struct CachedResponse {
     pub status: u16,
     pub body: serde_json::Value,
     pub recorded_at: Instant,
+    /// Hash of the request payload to detect reuse with different payloads
+    pub payload_hash: String,
 }
 
 /// Shared in-memory idempotency store.
@@ -53,31 +55,48 @@ impl IdempotencyStore {
         format!("{endpoint}:{invoice_id}:{idempotency_key}")
     }
 
-    /// Return the cached response if the key exists and has not expired.
-    pub fn get(&self, key: &str) -> Option<CachedResponse> {
+    /// Return the cached response if the key exists, has not expired, and matches the payload hash.
+    /// Returns None if the key doesn't exist or has expired.
+    /// Panics if the key exists but payload doesn't match (idempotency violation).
+    pub fn get(&self, key: &str, payload_hash: &str) -> Result<Option<CachedResponse>, String> {
         if let Some(entry) = self.inner.get(key) {
             if entry.recorded_at.elapsed() < self.ttl {
-                return Some(entry.clone());
+                // Key exists and is fresh - validate payload matches
+                if entry.payload_hash != payload_hash {
+                    return Err("Idempotency-Key reused with different request payload".to_string());
+                }
+                return Ok(Some(entry.clone()));
             }
             // Expired — evict lazily.
             drop(entry);
             self.inner.remove(key);
         }
-        None
+        Ok(None)
     }
 
-    /// Insert a response into the store.
-    pub fn insert(&self, key: String, status: u16, body: serde_json::Value) {
+    /// Insert a response into the store with payload hash for idempotency validation.
+    pub fn insert(&self, key: String, status: u16, body: serde_json::Value, payload_hash: String) {
         self.inner.insert(
             key,
             CachedResponse {
                 status,
                 body,
                 recorded_at: Instant::now(),
+                payload_hash,
             },
         );
     }
 
+    /// Compute a hash of the request payload for idempotency validation.
+    /// This ensures the same Idempotency-Key isn't reused with different payloads.
+    pub fn hash_payload(payload: &serde_json::Value) -> String {
+        use std::collections::hash_map::DefaultHasher;
+        use std::hash::{Hash, Hasher};
+        let mut hasher = DefaultHasher::new();
+        payload.to_string().hash(&mut hasher);
+        format!("{:x}", hasher.finish())
+    }
+
     /// Remove every entry whose TTL has elapsed. Called both lazily on lookup
     /// (via [`Self::get`]) and periodically by the background sweeper so that
     /// entries are evicted even when they are never looked up again.
diff --git a/backend/src/rate_limiter.rs b/backend/src/rate_limiter.rs
index 3a04b56..46f586d 100644
--- a/backend/src/rate_limiter.rs
+++ b/backend/src/rate_limiter.rs
@@ -260,9 +260,11 @@ fn build_rate_limit_response(retry_after_secs: u64, limit: u64, reset: u64) -> R
         "error": "Too many requests. Please retry after the indicated number of seconds.",
         "retryAfter": retry_after_secs,
     });
+    // Retry-After header must be a non-negative integer (seconds) per RFC 7231
+    let retry_after_value = retry_after_secs.max(1).to_string();
     let mut response = (
         StatusCode::TOO_MANY_REQUESTS,
-        [("retry-after", retry_after_secs.to_string())],
+        [("retry-after", retry_after_value)],
         Json(body),
     )
         .into_response();