diff --git a/crates/agentflare-store/src/blobs.rs b/crates/agentflare-store/src/blobs.rs index cf29faef..17d6096d 100644 --- a/crates/agentflare-store/src/blobs.rs +++ b/crates/agentflare-store/src/blobs.rs @@ -75,7 +75,19 @@ fn decompress_if_gzip(data: Vec) -> std::io::Result> { /// deleting one another store needs is not. fn delete_disk_blob(dir: &Path, hash: &str) { let path = blob_disk_path(dir, hash); - let _ = std::fs::remove_file(&path); + // The row is already gone (blob_unref) or was never inserted (blob_store, + // cleaning up after a failed metadata insert) by the time this runs, so a + // failure here can't be retried through the database — log it (unless the + // file was simply already absent) so an orphaned file is at least + // discoverable instead of silently unaccounted for. + if let Err(e) = std::fs::remove_file(&path) + && e.kind() != std::io::ErrorKind::NotFound + { + eprintln!( + "[store] failed to reclaim blob file {}: {e}", + path.display() + ); + } } use std::path::PathBuf; diff --git a/src/mcp_server/asset.rs b/src/mcp_server/asset.rs index 0e9e5e60..c45f9815 100644 --- a/src/mcp_server/asset.rs +++ b/src/mcp_server/asset.rs @@ -39,30 +39,76 @@ impl AgentflareMcp { } let staging_dir = crate::paths::home().join(".agentflare").join("staging"); let staged = staging_dir.join(&fn_val); - if !staged.exists() { - return Err(ErrorData::invalid_params( - format!( - "file not found at staging path: {} — write the file there before calling attach", - staged.display() - ), - None, - )); + + // symlink_metadata (not metadata) never follows the link, so a + // symlink dropped into staging pointing outside it is rejected + // here rather than silently read through. + match std::fs::symlink_metadata(&staged) { + Ok(m) if m.file_type().is_symlink() => { + return Err(ErrorData::invalid_params( + format!("staged file '{fn_val}' is a symlink — not allowed"), + None, + )); + } + // Reject FIFOs/devices/sockets too: opening one and + // blocking on read_to_end below would hang the calling + // thread waiting for a writer that may never come — same + // malicious-staged-file threat model as the symlink check. + Ok(m) if !m.file_type().is_file() => { + return Err(ErrorData::invalid_params( + format!("staged file '{fn_val}' is not a regular file — not allowed"), + None, + )); + } + Ok(_) => {} + Err(_) => { + return Err(ErrorData::invalid_params( + format!( + "file not found at staging path: {} — write the file there before calling attach", + staged.display() + ), + None, + )); + } } - let size = std::fs::metadata(&staged) - .map_err(|e| ErrorData::internal_error(e.to_string(), None))? - .len(); + let max_attach = Self::asset_max_attach_bytes(); + let mut open_opts = std::fs::OpenOptions::new(); + open_opts.read(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + // O_NOFOLLOW: a symlink swapped in between the + // symlink_metadata check above and this open must never be + // followed (TOCTOU). flare-code: no Windows-side reparse-point + // guard yet — Windows symlink creation needs an elevated + // privilege the staging dir's normal writers won't have. + open_opts.custom_flags(libc::O_NOFOLLOW); + } + let mut file = open_opts.open(&staged).map_err(|e| { + ErrorData::internal_error(format!("opening staged file: {e}"), None) + })?; + + // Size and content come from the same handle now, capped at + // max_attach+1: a file grown after the open (instead of + // symlink-swapped) is caught by the length check below instead + // of being read unbounded. + use std::io::Read; + let mut bytes = Vec::new(); + (&mut file) + .take(max_attach + 1) + .read_to_end(&mut bytes) + .map_err(|e| ErrorData::internal_error(e.to_string(), None))?; + let size = bytes.len() as u64; if size > max_attach { return Err(ErrorData::invalid_params( format!( - "file is {} bytes, exceeds the {} byte attach limit", + "file is at least {} bytes, exceeds the {} byte attach limit", size, max_attach ), None, )); } - let bytes = std::fs::read(&staged) - .map_err(|e| ErrorData::internal_error(e.to_string(), None))?; let meta = metadata.unwrap_or_else(|| "{}".to_string()); // attach always nests with_store inside with_backend_db below, which diff --git a/src/mcp_server/tests/asset_tests.rs b/src/mcp_server/tests/asset_tests.rs index 713a6535..22e8a9ae 100644 --- a/src/mcp_server/tests/asset_tests.rs +++ b/src/mcp_server/tests/asset_tests.rs @@ -454,6 +454,54 @@ fn asset_attach_rejects_oversized_file() { }); } +#[test] +#[cfg(unix)] +fn asset_attach_rejects_symlink() { + crate::paths::test_support::with_temp_home(|| { + let (_tmp, s) = harness(); + let home = crate::paths::home(); + let staging = home.join(".agentflare").join("staging"); + std::fs::create_dir_all(&staging).unwrap(); + let outside = home.join("outside-secret.txt"); + std::fs::write(&outside, b"not for attaching").unwrap(); + std::os::unix::fs::symlink(&outside, staging.join("link.txt")).unwrap(); + let err = s + .asset(Parameters(AssetRequest { + action: "attach".into(), + id: None, + item_id: Some("item-1".into()), + project_id: None, + filename: Some("link.txt".into()), + metadata: None, + })) + .unwrap_err(); + assert_eq!(err.code, rmcp::model::ErrorCode::INVALID_PARAMS); + }); +} + +#[test] +fn asset_attach_rejects_non_regular_file() { + // A directory sharing the guard's "not is_file()" branch with FIFOs and + // other special files, without the hang risk of actually opening one. + crate::paths::test_support::with_temp_home(|| { + let (_tmp, s) = harness(); + let home = crate::paths::home(); + let staging = home.join(".agentflare").join("staging"); + std::fs::create_dir_all(staging.join("dir.txt")).unwrap(); + let err = s + .asset(Parameters(AssetRequest { + action: "attach".into(), + id: None, + item_id: Some("item-1".into()), + project_id: None, + filename: Some("dir.txt".into()), + metadata: None, + })) + .unwrap_err(); + assert_eq!(err.code, rmcp::model::ErrorCode::INVALID_PARAMS); + }); +} + #[test] fn asset_get_over_max_inline_omits_content() { crate::paths::test_support::with_temp_home(|| {