fix(asset): close staging symlink/TOCTOU bypass, log swallowed blob-reclaim errors

crates/agentflare-store/src/blobs.rs

@@ -75,7 +75,19 @@ fn decompress_if_gzip(data: Vec<u8>) -> std::io::Result<Vec<u8>> {
 /// 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;

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

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(|| {