fix: confine MCP agents' files to allowed folders; recovery discard deletes only recovery entries (#205) (#578)

Agent tool calls (MCP, and bridged control requests carrying allow_paths) reach files only in the open design's folder (strictly inside home or the temporary folder, not in a hidden folder, and never its hidden files), the app's recovery folder, the server's private scratch folder and each --allow-path; --allow-any-path lifts the limit. Paths are resolved first, so .. and symlinks that lead out (including a save through a link, #845) are refused. On Windows, UNC, device, NT, verbatim non-drive, drive-relative, alternate-stream, reserved-device and 8.3 short-name paths are refused before any file system call, and the resolved path must be on a local drive. doc.recovery_discard and doc.recover {discard} delete only real recovery entries. Docs: mcp.md, control-protocol.md, README.
This commit is contained in:
Kevin Ravensberg authored and GitHub committed 2026-10-13 18:24:08 -04:00
1 parent 3d3f9b626f
commit 96f6424aec
23 files changed
+1000 -37

No files matched your search

+61 -8
View File
@@ -130,13 +130,19 @@ pub fn load(dir: &Path, id: &str) -> Result<(Document, Entry)> {
Ok((solidcraft_io::read_design(&bytes)?, entry))
}
/// Delete an entry.
pub fn remove(dir: &Path, id: &str) {
if valid_id(id) {
for ext in ["json", solidcraft_io::DESIGN_EXT, solidcraft_io::LEGACY_DESIGN_EXT, "lock"] {
let _ = fs::remove_file(file(dir, id, ext));
}
/// Delete an entry. Only a real one: `<id>.json` must describe entry `id` and its design must
/// be there, so a folder and id that merely name someone's `part.solidcraft` delete nothing.
pub fn remove(dir: &Path, id: &str) -> Result<()> {
let is_entry = valid_id(id)
&& fs::read(file(dir, id, "json")).ok().and_then(|b| serde_json::from_slice::<Entry>(&b).ok()).is_some_and(|e| e.id == id)
&& design_file(dir, id).is_file();
if !is_entry {
return Err(EngineError::Other(format!("no recovered design `{id}`")));
}
for ext in ["json", solidcraft_io::DESIGN_EXT, solidcraft_io::LEGACY_DESIGN_EXT, "lock"] {
let _ = fs::remove_file(file(dir, id, ext));
}
Ok(())
}
/// One app's autosaver.
@@ -241,7 +247,7 @@ mod tests {
assert_eq!(orphans(&d).iter().map(|e| e.id.as_str()).collect::<Vec<_>>(), [id]);
let (_, loaded) = load(&d, id).unwrap();
assert_eq!(loaded, entry);
remove(&d, id);
remove(&d, id).unwrap();
assert!(orphans(&d).is_empty() && !d.join(format!("{id}.solvecraft")).exists());
let _ = fs::remove_dir_all(&d);
}
@@ -282,7 +288,7 @@ mod tests {
let (doc, entry) = load(&d, &found[0].id).unwrap();
assert_eq!(doc, *s.doc);
assert_eq!(entry.features, 1);
remove(&d, &entry.id);
remove(&d, &entry.id).unwrap();
assert!(orphans(&d).is_empty());
let _ = fs::remove_dir_all(&d);
}
@@ -302,6 +308,53 @@ mod tests {
let _ = fs::remove_dir_all(&d);
}
/// `doc.recovery_discard` deletes recovery entries only (#205): a folder and id that merely
/// name someone's design, or a `<id>.json` describing another entry, delete nothing.
#[test]
fn discarding_refuses_what_is_not_a_recovery_entry() {
let d = dir("discard");
fs::create_dir_all(&d).unwrap();
let part = d.join("part.solidcraft");
fs::write(&part, solidcraft_io::write_design(&Document::default())).unwrap();
let mut s = Session::default();
let discard = |s: &mut Session, id: &str| s.execute("doc.recovery_discard", &json!({"dir": d.to_string_lossy(), "id": id}));
let e = discard(&mut s, "part").unwrap_err();
assert!(e.to_string().contains("no recovered design `part`"), "{e}");
let other = Entry { id: "other".into(), path: None, name: "x".into(), saved_at: 1, features: 0 };
fs::write(d.join("part.json"), serde_json::to_vec(&other).unwrap()).unwrap();
assert!(discard(&mut s, "part").is_err());
assert!(discard(&mut s, "../part").is_err());
assert!(part.exists() && d.join("part.json").exists(), "nothing deleted");
// A real entry still goes.
let mine = Entry { id: "part".into(), ..other };
fs::write(d.join("part.json"), serde_json::to_vec(&mine).unwrap()).unwrap();
assert_eq!(discard(&mut s, "part").unwrap()["discarded"], json!(["part"]));
assert!(!part.exists());
let _ = fs::remove_dir_all(&d);
}
/// `doc.recover {discard: true}` opens a real entry and then deletes it; given a folder and
/// id that only name someone's design, it deletes nothing (#205 review).
#[test]
fn recover_with_discard_deletes_only_the_entry_it_opened() {
let d = dir("recover-discard");
let mut s = Session::default();
let mut a = Autosaver::new(&d, Duration::from_secs(3600)).unwrap();
s.execute("solid.box", &json!({"length": 10, "width": 10, "height": 10})).unwrap();
assert!(a.save(&s).unwrap());
let id = a.id.clone();
drop(a);
let mut t = Session::default();
let r = t.execute("doc.recover", &json!({"dir": d.to_string_lossy(), "id": id, "discard": true})).unwrap();
assert_eq!(r["features"], 1, "{r}");
assert!(orphans(&d).is_empty() && !file(&d, &id, "json").exists() && !file(&d, &id, "solidcraft").exists());
let part = d.join("part.solidcraft");
fs::write(&part, solidcraft_io::write_design(&Document::default())).unwrap();
assert!(t.execute("doc.recover", &json!({"dir": d.to_string_lossy(), "id": "part", "discard": true})).is_err());
assert!(part.exists());
let _ = fs::remove_dir_all(&d);
}
#[test]
fn saving_drops_the_entry_and_a_clean_close_leaves_nothing() {
let d = dir("clean");