Check the remembered path of a pathless doc_save against the root (#1791) (#1879)

crates/automation/src/documents.rs

@@ -1,7 +1,7 @@
 use super::error::{Result, ToolError, failed};
 use super::json::{Args, nest_rows, security, summary};
 use super::paths::{child, read_input, write_atomic};
-use super::{Arc, Automation, BlockKey, DocId, Document, Edit, Path, PathBuf, TextBlock, Value, comments, file_access, json};
+use super::{Arc, Automation, BlockKey, DocId, Document, Edit, Path, TextBlock, Value, comments, json};
 
 /// How far a watermark may be moved from the page centre, in points (a page is at most 14,400).
 const WATERMARK_MAX_OFFSET: f64 = 20_000.0;
@@ -135,18 +135,18 @@ impl Automation {
     pub(crate) fn doc_save_with(&mut self, a: &Args, write: impl FnOnce(&Path, &[u8]) -> Result<()>) -> Result<Value> {
         let doc = self.doc(a)?;
         let id = doc.id;
-        let target = match a.opt_str("path")? {
+        let explicit = a.opt_str("path")?;
+        let target = match explicit {
             Some(p) => self.resolve(p, true)?,
             None => {
                 // Saving in place writes where the file was opened from, which the read root
-                // allows and the write root may not.
+                // allows and the write root may not. It also goes through --root again: a
+                // folder on the remembered path may have become a link out since (#1791).
                 let own = doc.path.clone().ok_or_else(|| failed("the document has never been saved: pass a path"))?;
-                let own_path = PathBuf::from(&own);
-                self.check_roots(&own, &own_path, file_access::Access::Write)?;
-                own_path
+                self.resolve(&own, true)?
             }
         };
-        let same_file = doc.path.as_deref().is_some_and(|p| Path::new(p) == target);
+        let same_file = explicit.is_none() || doc.path.as_deref().is_some_and(|p| Path::new(p) == target);
         // Saving to a new file is a full rewrite unless asked otherwise, like Save As.
         let full = a.opt_bool("full")?.unwrap_or(!same_file);
         let flatten_fill_sign = a.opt_bool("flatten_fill_sign")?.unwrap_or(false);

crates/automation/src/tests.rs

@@ -270,6 +270,30 @@ fn a_bounded_text_find_reads_only_the_pages_it_needs_and_says_it_stopped() {
     let _ = std::fs::remove_dir_all(&dir);
 }
 
+#[test]
+fn a_save_without_a_path_stays_inside_the_root() {
+    let base = std::env::temp_dir().join(format!("pdfcraft-automation-save-root-{}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&base);
+    let root = base.join("root");
+    let outside = base.join("outside");
+    std::fs::create_dir_all(&root).unwrap();
+    std::fs::create_dir_all(&outside).unwrap();
+    let mut a = Automation::new().with_root(&root).unwrap();
+    let bytes = Arc::new(numbered_pages(1));
+    // Remembered path inside the root: still saves.
+    let inside = root.canonicalize().unwrap().join("in.pdf");
+    let id = a.session.open("in.pdf", Some(inside.to_string_lossy().into_owned()), bytes.clone(), None).unwrap();
+    json(&mut a, "doc_save", json!({ "doc": id.0 }));
+    assert!(inside.exists());
+    // Remembered path outside the root: refused, and nothing is written there.
+    let escaped = outside.join("out.pdf");
+    let id = a.session.open("out.pdf", Some(escaped.to_string_lossy().into_owned()), bytes, None).unwrap();
+    let error = a.call("doc_save", &json!({ "doc": id.0 })).unwrap_err();
+    assert!(error.to_string().contains("outside the allowed directory"), "{error}");
+    assert!(!escaped.exists());
+    let _ = std::fs::remove_dir_all(&base);
+}
+
 #[test]
 fn dot_dot_resolves_by_name_and_never_climbs_above_the_start() {
     let base = std::env::temp_dir();