From 5f7e50ef4ab19dc784528fa578f31ad8ddcb6af9 Mon Sep 17 00:00:00 2001 From: Douwe Osinga Date: Wed, 2 Jul 2025 16:42:32 -0400 Subject: [PATCH] Defend against invalid sessions (#3229) Co-authored-by: Douwe Osinga --- crates/goose-cli/src/commands/session.rs | 8 ++--- crates/goose-cli/src/commands/web.rs | 38 +++++++++++++---------- crates/goose-cli/src/session/mod.rs | 1 - crates/goose-server/src/routes/session.rs | 7 ++--- crates/goose/src/session/info.rs | 18 +++++------ crates/goose/src/session/mod.rs | 2 +- crates/goose/src/session/storage.rs | 1 - 7 files changed, 37 insertions(+), 38 deletions(-) diff --git a/crates/goose-cli/src/commands/session.rs b/crates/goose-cli/src/commands/session.rs index e1ffe46f..c02a2ad9 100644 --- a/crates/goose-cli/src/commands/session.rs +++ b/crates/goose-cli/src/commands/session.rs @@ -1,7 +1,7 @@ use crate::session::message_to_markdown; use anyhow::{Context, Result}; use cliclack::{confirm, multiselect, select}; -use goose::session::info::{get_session_info, SessionInfo, SortOrder}; +use goose::session::info::{get_valid_sorted_sessions, SessionInfo, SortOrder}; use goose::session::{self, Identifier}; use regex::Regex; use std::fs; @@ -75,7 +75,7 @@ fn prompt_interactive_session_removal(sessions: &[SessionInfo]) -> Result, regex_string: Option) -> Result<()> { - let all_sessions = match get_session_info(SortOrder::Descending) { + let all_sessions = match get_valid_sorted_sessions(SortOrder::Descending) { Ok(sessions) => sessions, Err(e) => { tracing::error!("Failed to retrieve sessions: {:?}", e); @@ -125,7 +125,7 @@ pub fn handle_session_list(verbose: bool, format: String, ascending: bool) -> Re SortOrder::Descending }; - let sessions = match get_session_info(sort_order) { + let sessions = match get_valid_sorted_sessions(sort_order) { Ok(sessions) => sessions, Err(e) => { tracing::error!("Failed to list sessions: {:?}", e); @@ -294,7 +294,7 @@ fn export_session_to_markdown( /// Shows a list of available sessions and lets the user select one pub fn prompt_interactive_session_selection() -> Result { // Get sessions sorted by modification date (newest first) - let sessions = match get_session_info(SortOrder::Descending) { + let sessions = match get_valid_sorted_sessions(SortOrder::Descending) { Ok(sessions) => sessions, Err(e) => { tracing::error!("Failed to list sessions: {:?}", e); diff --git a/crates/goose-cli/src/commands/web.rs b/crates/goose-cli/src/commands/web.rs index 33e0d34d..3ca4f77d 100644 --- a/crates/goose-cli/src/commands/web.rs +++ b/crates/goose-cli/src/commands/web.rs @@ -225,14 +225,15 @@ async fn list_sessions() -> Json { Ok(sessions) => { let session_info: Vec = sessions .into_iter() - .map(|(name, path)| { - let metadata = session::read_metadata(&path).unwrap_or_default(); - serde_json::json!({ - "name": name, - "path": path, - "description": metadata.description, - "message_count": metadata.message_count, - "working_dir": metadata.working_dir + .filter_map(|(name, path)| { + session::read_metadata(&path).ok().map(|metadata| { + serde_json::json!({ + "name": name, + "path": path, + "description": metadata.description, + "message_count": metadata.message_count, + "working_dir": metadata.working_dir + }) }) }) .collect(); @@ -246,7 +247,6 @@ async fn list_sessions() -> Json { })), } } - async fn get_session( axum::extract::Path(session_id): axum::extract::Path, ) -> Json { @@ -259,17 +259,21 @@ async fn get_session( } }; + let error_response = |e: Box| { + Json(serde_json::json!({ + "error": e.to_string() + })) + }; + match session::read_messages(&session_file) { - Ok(messages) => { - let metadata = session::read_metadata(&session_file).unwrap_or_default(); - Json(serde_json::json!({ + Ok(messages) => match session::read_metadata(&session_file) { + Ok(metadata) => Json(serde_json::json!({ "metadata": metadata, "messages": messages - })) - } - Err(e) => Json(serde_json::json!({ - "error": e.to_string() - })), + })), + Err(e) => error_response(e.into()), + }, + Err(e) => error_response(e.into()), } } diff --git a/crates/goose-cli/src/session/mod.rs b/crates/goose-cli/src/session/mod.rs index 15ba617f..b9db39be 100644 --- a/crates/goose-cli/src/session/mod.rs +++ b/crates/goose-cli/src/session/mod.rs @@ -1259,7 +1259,6 @@ impl Session { ); } - /// Get the session metadata pub fn get_metadata(&self) -> Result { if !self.session_file.as_ref().is_some_and(|f| f.exists()) { return Err(anyhow::anyhow!("Session file does not exist")); diff --git a/crates/goose-server/src/routes/session.rs b/crates/goose-server/src/routes/session.rs index c5d91876..ca0d5970 100644 --- a/crates/goose-server/src/routes/session.rs +++ b/crates/goose-server/src/routes/session.rs @@ -10,7 +10,7 @@ use axum::{ }; use goose::message::Message; use goose::session; -use goose::session::info::{get_session_info, SessionInfo, SortOrder}; +use goose::session::info::{get_valid_sorted_sessions, SessionInfo, SortOrder}; use goose::session::SessionMetadata; use serde::Serialize; use utoipa::ToSchema; @@ -53,8 +53,8 @@ async fn list_sessions( ) -> Result, StatusCode> { verify_secret_key(&headers, &state)?; - let sessions = - get_session_info(SortOrder::Descending).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; + let sessions = get_valid_sorted_sessions(SortOrder::Descending) + .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; Ok(Json(SessionListResponse { sessions })) } @@ -89,7 +89,6 @@ async fn get_session_history( Err(_) => return Err(StatusCode::BAD_REQUEST), }; - // Read metadata let metadata = session::read_metadata(&session_path).map_err(|_| StatusCode::NOT_FOUND)?; let messages = match session::read_messages(&session_path) { diff --git a/crates/goose/src/session/info.rs b/crates/goose/src/session/info.rs index 6056257a..a772af05 100644 --- a/crates/goose/src/session/info.rs +++ b/crates/goose/src/session/info.rs @@ -18,7 +18,7 @@ pub enum SortOrder { Descending, } -pub fn get_session_info(sort_order: SortOrder) -> Result> { +pub fn get_valid_sorted_sessions(sort_order: SortOrder) -> Result> { let sessions = match session::list_sessions() { Ok(sessions) => sessions, Err(e) => { @@ -26,10 +26,9 @@ pub fn get_session_info(sort_order: SortOrder) -> Result> { return Err(anyhow::anyhow!("Failed to list sessions")); } }; - let mut session_infos = sessions + let mut session_infos: Vec = sessions .into_iter() - .map(|(id, path)| { - // Get last modified time as string + .filter_map(|(id, path)| { let modified = path .metadata() .and_then(|m| m.modified()) @@ -38,19 +37,18 @@ pub fn get_session_info(sort_order: SortOrder) -> Result> { .format("%Y-%m-%d %H:%M:%S UTC") .to_string() }) - .unwrap_or_else(|_| "Unknown".to_string()); + .ok()?; - // Get session description - let metadata = session::read_metadata(&path).expect("Failed to read session metadata"); + let metadata = session::read_metadata(&path).ok()?; - SessionInfo { + Some(SessionInfo { id, path: path.to_string_lossy().to_string(), modified, metadata, - } + }) }) - .collect::>(); + .collect(); // Sort sessions by modified date // Since all dates are in ISO format (YYYY-MM-DD HH:MM:SS UTC), we can just use string comparison diff --git a/crates/goose/src/session/mod.rs b/crates/goose/src/session/mod.rs index 99b6bb08..5f4537fe 100644 --- a/crates/goose/src/session/mod.rs +++ b/crates/goose/src/session/mod.rs @@ -9,4 +9,4 @@ pub use storage::{ SessionMetadata, }; -pub use info::{get_session_info, SessionInfo}; +pub use info::{get_valid_sorted_sessions, SessionInfo}; diff --git a/crates/goose/src/session/storage.rs b/crates/goose/src/session/storage.rs index 29cf057d..b176511f 100644 --- a/crates/goose/src/session/storage.rs +++ b/crates/goose/src/session/storage.rs @@ -1309,7 +1309,6 @@ pub async fn generate_description_with_schedule_id( description }; - // Read current metadata let mut metadata = read_metadata(&secure_path)?; // Update description and schedule_id