fix(security): eliminate TOCTOU race in approval request_id check (#1486)

Hold session lock for the entire take-verify sequence so pending approval
cannot be lost if a concurrent operation modifies the thread between
take and restore.

Closes #1486

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

src/agent/agent_loop.rs

@@ -1052,11 +1052,10 @@ impl Agent {
             } else {
                 drop(sess);
                 self.session_manager
-                    .resolve_thread_with_parsed_uuid(
+                    .resolve_thread(
                         &message.user_id,
                         &message.channel,
                         message.conversation_scope(),
-                        approval_thread_uuid,
                     )
                     .await
             }

src/agent/session_manager.rs

@@ -107,20 +107,6 @@ impl SessionManager {
         user_id: &str,
         channel: &str,
         external_thread_id: Option<&str>,
-    ) -> (Arc<Mutex<Session>>, Uuid) {
-        self.resolve_thread_with_parsed_uuid(user_id, channel, external_thread_id, None)
-            .await
-    }
-
-    /// Like [`resolve_thread`](Self::resolve_thread), but accepts a pre-parsed
-    /// UUID to skip redundant parsing when the caller has already validated
-    /// the external thread ID as a UUID (e.g. the approval routing path).
-    pub async fn resolve_thread_with_parsed_uuid(
-        &self,
-        user_id: &str,
-        channel: &str,
-        external_thread_id: Option<&str>,
-        parsed_uuid: Option<Uuid>,
     ) -> (Arc<Mutex<Session>>, Uuid) {
         let session = self.get_or_create_session(user_id).await;
 
@@ -147,11 +133,9 @@ impl SessionManager {
         // (e.g. created by chat_new_thread_handler or hydrated from DB).
         // We only adopt it if no thread_map entry maps to this UUID —
         // otherwise it belongs to a different channel scope.
-        // Use pre-parsed UUID if available, otherwise parse from string.
-        let ext_uuid = parsed_uuid
-            .or_else(|| external_thread_id.and_then(|ext_tid| Uuid::parse_str(ext_tid).ok()));
-
-        if let Some(ext_uuid) = ext_uuid {
+        if let Some(ext_tid) = external_thread_id
+            && let Ok(ext_uuid) = Uuid::parse_str(ext_tid)
+        {
             let thread_map = self.thread_map.read().await;
             let mapped_elsewhere = thread_map.values().any(|&v| v == ext_uuid);
             drop(thread_map);
@@ -963,54 +947,4 @@ mod tests {
             "should have exactly 1 thread, not a duplicate"
         );
     }
-
-    #[tokio::test]
-    async fn test_resolve_thread_with_pre_parsed_uuid_adopts_thread() {
-        use crate::agent::session::Thread;
-
-        let manager = SessionManager::new();
-        let (session, _) = manager.resolve_thread("user1", "chan1", None).await;
-
-        // Manually insert a thread with a known UUID
-        let known_id = Uuid::new_v4();
-        {
-            let mut sess = session.lock().await;
-            let thread = Thread::with_id(known_id, sess.id);
-            sess.threads.insert(known_id, thread);
-        }
-
-        // Resolve with pre-parsed UUID -- should adopt it without re-parsing
-        let (_, resolved) = manager
-            .resolve_thread_with_parsed_uuid(
-                "user1",
-                "chan1",
-                Some(&known_id.to_string()),
-                Some(known_id),
-            )
-            .await;
-        assert_eq!(resolved, known_id);
-    }
-
-    #[tokio::test]
-    async fn test_resolve_thread_with_parsed_uuid_none_delegates_to_parse() {
-        use crate::agent::session::Thread;
-
-        let manager = SessionManager::new();
-        let (session, _) = manager.resolve_thread("user2", "chan2", None).await;
-
-        // Insert a thread with a known UUID
-        let known_id = Uuid::new_v4();
-        {
-            let mut sess = session.lock().await;
-            let thread = Thread::with_id(known_id, sess.id);
-            sess.threads.insert(known_id, thread);
-        }
-
-        // Resolve with parsed_uuid=None but a valid UUID string -- should
-        // fall back to parsing the string and still adopt the thread
-        let (_, resolved) = manager
-            .resolve_thread_with_parsed_uuid("user2", "chan2", Some(&known_id.to_string()), None)
-            .await;
-        assert_eq!(resolved, known_id);
-    }
 }

src/agent/thread_ops.rs

@@ -954,7 +954,10 @@ impl Agent {
         approved: bool,
         always: bool,
     ) -> Result<SubmissionResult, Error> {
-        // Get pending approval for this thread
+        // Get pending approval for this thread.
+        // The take-verify sequence is atomic under a single lock acquisition
+        // to prevent a TOCTOU race where a concurrent operation could modify
+        // or delete the thread between take and restore (#1486).
         let pending = {
             let mut sess = session.lock().await;
             let thread = sess
@@ -972,33 +975,31 @@ impl Agent {
                 return Ok(SubmissionResult::ok_with_message(""));
             }
 
-            thread.take_pending_approval()
-        };
+            let taken = match thread.take_pending_approval() {
+                Some(p) => p,
+                None => {
+                    tracing::debug!(
+                        %thread_id,
+                        "Ignoring stale approval: no pending approval found"
+                    );
+                    return Ok(SubmissionResult::ok_with_message(""));
+                }
+            };
 
-        let pending = match pending {
-            Some(p) => p,
-            None => {
-                tracing::debug!(
-                    %thread_id,
-                    "Ignoring stale approval: no pending approval found"
-                );
-                return Ok(SubmissionResult::ok_with_message(""));
+            // Verify request ID while still holding the lock — atomic with take
+            if let Some(req_id) = request_id
+                && req_id != taken.request_id
+            {
+                // Restore atomically under same lock
+                thread.await_approval(taken);
+                return Ok(SubmissionResult::error(
+                    "Request ID mismatch. Use the correct request ID.",
+                ));
             }
-        };
 
-        // Verify request ID if provided
-        if let Some(req_id) = request_id
-            && req_id != pending.request_id
-        {
-            // Put it back and return error
-            let mut sess = session.lock().await;
-            if let Some(thread) = sess.threads.get_mut(&thread_id) {
-                thread.await_approval(pending);
-            }
-            return Ok(SubmissionResult::error(
-                "Request ID mismatch. Use the correct request ID.",
-            ));
-        }
+            taken
+            // Lock dropped here — pending approval validated
+        };
 
         if approved {
             // If always, add to auto-approved set
@@ -2204,6 +2205,47 @@ mod tests {
         assert!(t.pending_messages.is_empty());
     }
 
+    #[test]
+    fn test_approval_request_id_mismatch_restores_pending() {
+        // Regression test for #1486: after a request_id mismatch, the pending
+        // approval must still be intact (take + verify + restore is atomic).
+        use crate::agent::session::{PendingApproval, Thread, ThreadState};
+        use uuid::Uuid;
+
+        let session_id = Uuid::new_v4();
+        let thread_id = Uuid::new_v4();
+        let mut thread = Thread::with_id(thread_id, session_id);
+
+        let correct_request_id = Uuid::new_v4();
+        let pending = PendingApproval {
+            request_id: correct_request_id,
+            tool_name: "shell".to_string(),
+            parameters: serde_json::json!({}),
+            display_parameters: serde_json::json!({}),
+            description: "test".to_string(),
+            tool_call_id: "call_0".to_string(),
+            context_messages: vec![],
+            deferred_tool_calls: vec![],
+            user_timezone: None,
+            allow_always: true,
+        };
+        thread.await_approval(pending);
+        assert_eq!(thread.state, ThreadState::AwaitingApproval);
+
+        // Simulate: take, verify mismatch, restore -- all must be atomic
+        let taken = thread.take_pending_approval().unwrap();
+        assert_eq!(taken.request_id, correct_request_id);
+        // On mismatch, restore
+        thread.await_approval(taken);
+        // Must still be in AwaitingApproval with pending intact
+        assert_eq!(thread.state, ThreadState::AwaitingApproval);
+        assert!(thread.pending_approval.is_some());
+        assert_eq!(
+            thread.pending_approval.as_ref().unwrap().request_id,
+            correct_request_id
+        );
+    }
+
     // Helper function to extract the approval message without needing a full Agent instance
     fn extract_approval_message(
         session: &crate::agent::session::Session,