mirror of
https://github.com/storytold/photocraft.git
synced 2026-11-02 06:17:22 -06:00
Automation: refuse to replace a non-image file in the write root (#9439)
`doc.save {"path": "src/main.rs", "format": "png"}` and `doc.render
{"path": "Cargo.toml"}` renamed image bytes over whatever file was there.
`AuthorizedWorkspace::write` now replaces an existing file only when its
extension is a format PhotoCraft writes or its bytes are an image; new
files of any name are still created. docs/mcp.md says what a granted root
allows and recommends a dedicated output folder.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
b0622d20ca
commit
fa4269af98
@@ -771,6 +771,44 @@ mod tests {
|
||||
std::fs::remove_dir_all(base).unwrap();
|
||||
}
|
||||
|
||||
/// A save or render never turns a file PhotoCraft doesn't write (source, settings, notes)
|
||||
/// into image bytes, whatever `format` says; images and new files are written as before (#9439).
|
||||
#[test]
|
||||
fn workspace_writes_refuse_to_replace_a_file_that_is_not_an_image() {
|
||||
let root = std::env::temp_dir().join(format!("pc-no-replace-foreign-{}", std::process::id()));
|
||||
let _ = std::fs::remove_dir_all(&root);
|
||||
std::fs::create_dir_all(root.join("src")).unwrap();
|
||||
let foreign = [("src/main.rs", "fn main() {}\n"), ("Cargo.toml", "[package]\n"), (".env", "TOKEN=x\n"), ("Makefile", "all:\n")];
|
||||
for (name, text) in foreign {
|
||||
std::fs::write(root.join(name), text).unwrap();
|
||||
}
|
||||
std::fs::write(root.join("old.png"), b"not yet a png").unwrap();
|
||||
let mut h = Headless::with_workspace(crate::AuthorizedWorkspace::new(Some(&root), Some(&root)).unwrap());
|
||||
h.handle("doc.new", json!({"width": 8, "height": 8, "background": "white"})).unwrap();
|
||||
for (name, text) in foreign {
|
||||
let saved = h.handle("doc.save", json!({"path": name, "format": "png"})).unwrap_err().to_string();
|
||||
assert!(saved.contains("already exists"), "doc.save over {name}: {saved}");
|
||||
let rendered = h.handle("doc.render", json!({"path": name})).unwrap_err().to_string();
|
||||
assert!(rendered.contains("already exists"), "doc.render over {name}: {rendered}");
|
||||
assert_eq!(std::fs::read_to_string(root.join(name)).unwrap(), text, "{name} is untouched");
|
||||
}
|
||||
// An existing image is replaced, and a new file of any name is created.
|
||||
h.handle("doc.save", json!({"path": "old.png"})).unwrap();
|
||||
assert!(std::fs::read(root.join("old.png")).unwrap().starts_with(b"\x89PNG"));
|
||||
h.handle("doc.save", json!({"path": "fresh.txt", "format": "png"})).unwrap();
|
||||
assert!(std::fs::read(root.join("fresh.txt")).unwrap().starts_with(b"\x89PNG"));
|
||||
h.handle("doc.render", json!({"path": "old.png"})).unwrap();
|
||||
std::fs::write(root.join("OLD.TIF"), b"x").unwrap();
|
||||
h.handle("doc.save", json!({"path": "OLD.TIF"})).unwrap();
|
||||
// An image saved under a name without an image extension is recognised by its bytes.
|
||||
h.handle("doc.save", json!({"path": "out", "format": "png"})).unwrap();
|
||||
h.handle("doc.save", json!({"path": "out", "format": "png"})).unwrap();
|
||||
h.handle("doc.render", json!({"path": "fresh.txt"})).unwrap();
|
||||
// Release directory capabilities before cleanup on Windows.
|
||||
drop(h);
|
||||
std::fs::remove_dir_all(root).unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn doc_replace_smart_object_reads_through_the_read_root_and_never_outside_it() {
|
||||
let base = std::env::temp_dir().join(format!("pc-doc-replace-so-{}", std::process::id()));
|
||||
|
||||
@@ -88,6 +88,15 @@ impl AuthorizedWorkspace {
|
||||
let relative = request_path(path, self.write.as_ref())?;
|
||||
let root = self.write.as_ref().ok_or_else(|| AutomationError::BadRequest(format!("{DENIED}: write authority is absent")))?;
|
||||
let leaf = relative.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default();
|
||||
// The bytes are an image or document whatever the name says (`format` picks them), so
|
||||
// only a file of a kind PhotoCraft writes is replaced: never source, settings or notes
|
||||
// that share the root (`src/main.rs`, `.env`; #9439). A new file may have any name, and
|
||||
// an image saved under one (`out` with `format: png`) can be saved over again.
|
||||
if root.dir.symlink_metadata(&relative).is_ok() && !written_kind(&leaf) && !holds_image(&root.dir, &relative) {
|
||||
return Err(AutomationError::BadRequest(format!(
|
||||
"automation write refused: `{path}` already exists and is not an image or document file PhotoCraft writes; save to a new name"
|
||||
)));
|
||||
}
|
||||
let parent = relative.parent().map(Path::to_path_buf).unwrap_or_default();
|
||||
let tmp = parent.join(photocraft_format::atomic::temp_name(&leaf));
|
||||
let mut options = OpenOptions::new();
|
||||
@@ -124,6 +133,23 @@ impl AuthorizedWorkspace {
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether `name`'s extension is one of the formats PhotoCraft saves or exports.
|
||||
fn written_kind(name: &str) -> bool {
|
||||
let Some((_, ext)) = name.rsplit_once('.') else { return false };
|
||||
let ext = ext.to_ascii_lowercase();
|
||||
["psd", "psb", "ora", "pdf", photocraft_format::EXTENSION].contains(&ext.as_str()) || photocraft_codecs::from_extension(&ext).is_some()
|
||||
}
|
||||
|
||||
/// Whether the file at `relative` starts like an image PhotoCraft reads (PSD/PSB or a codec's
|
||||
/// magic). Containers that also hold other things (zip, PDF) are judged by name only.
|
||||
fn holds_image(dir: &Dir, relative: &Path) -> bool {
|
||||
let mut head = [0u8; 64];
|
||||
let Ok(mut file) = dir.open(relative) else { return false };
|
||||
let Ok(n) = file.read(&mut head) else { return false };
|
||||
let head = &head[..n];
|
||||
head.starts_with(b"8BPS") || photocraft_codecs::detect(head).is_some()
|
||||
}
|
||||
|
||||
fn read_to_observed_length(reader: &mut impl Read, observed_length: u64) -> Result<Vec<u8>, AutomationError> {
|
||||
// Do not reserve from a potentially stale or attacker-controlled metadata length. Read in
|
||||
// chunks, and reject growth beyond the length observed on the opened file before appending.
|
||||
|
||||
+10
-1
@@ -57,6 +57,15 @@ Paths are relative to the corresponding root, or absolute paths beneath it; with
|
||||
filesystem authority is absent. Bridge mode uses `--bridge` and `--control-token-file` as
|
||||
described in [development.md](development.md). UI tools require a running desktop app.
|
||||
|
||||
A client has the full rights of the roots it is given: it can read any file below the read
|
||||
root, and `doc_save`, `doc_export` and the export tools can create files anywhere below the
|
||||
write root. `doc_save` and `doc_export` replace an existing file only when its extension is an
|
||||
image or document format PhotoCraft writes (PNG, JPEG, TIFF, PSD, PSB, .pcraft, OpenRaster,
|
||||
PDF, …) or its bytes are an image; anything else (`src/main.rs`, `.env`) is refused and left
|
||||
untouched. The multi-file exports (layers, data sets) never replace a file. An agent reads
|
||||
text from documents (layer names, metadata) that may try to steer it, so prefer a dedicated
|
||||
output folder as the write root over a whole project.
|
||||
|
||||
Use absolute root paths in MCP client configurations, since a relative root is resolved from
|
||||
the client's launch directory. In bridge mode, configure roots on the **desktop app**; root
|
||||
flags on `photocraft-cli mcp --bridge` are rejected. Explicit `--control-token` and
|
||||
@@ -89,7 +98,7 @@ protocol messages. For `serve`, explicit credential flags require `--port` (or
|
||||
Existing document/session/job/UI tools remain listed, including the documented
|
||||
`doc_render_preview` spelling. There are no hidden legacy aliases. Every tool has a title
|
||||
and all four annotation hints. Generic command/control tools conservatively advertise writes;
|
||||
file saves may replace existing targets. Hints do not grant permission.
|
||||
file saves may replace existing image and document files. Hints do not grant permission.
|
||||
|
||||
After a failed batch, `completed` and per-step `results` report successful steps, not a
|
||||
rolled-back transaction. Inspect the document and its history with `doc_inspect`; commands
|
||||
|
||||
Reference in New Issue
Block a user