From ca8ebb0bb7681bc5695f6f3efe156dbf0d64dd96 Mon Sep 17 00:00:00 2001 From: Christoph Ortenburg Date: Thu, 15 Oct 2026 20:32:20 -0400 Subject: [PATCH 1/2] fix(automation): tools on a file the viewer reads fewer pages of answer instead of panicking `link_list` and `form_set_props` indexed the viewer's pages with a page number from the editing layer. On a file whose page tree has a dangling `/Kids` entry (#2660) the viewer sees three pages and the editing layer six, so `link_list` panicked ("the len is 3 but the index is 3") and so did moving a field on one of the later pages. Rectangles on a page the viewer could not read are now reported unchanged, as `doc_info` already reports them (`link_list`, `comment_list`, `content_list`); `comment_list` no longer borrows the last page's geometry for them. Placing a rect on such a page is an error (`form_set_props`, and `sign` when the viewer has no page at all). Co-Authored-By: Claude Opus 5.5 --- crates/automation/src/comments.rs | 4 +- crates/automation/src/content.rs | 6 +- crates/automation/src/forms.rs | 2 +- crates/automation/src/links.rs | 5 +- crates/automation/src/signing.rs | 2 +- crates/automation/tests/hostile_args.rs | 111 ++++++++++++++++++++++++ 6 files changed, 120 insertions(+), 10 deletions(-) diff --git a/crates/automation/src/comments.rs b/crates/automation/src/comments.rs index 302688f72..4effebf11 100644 --- a/crates/automation/src/comments.rs +++ b/crates/automation/src/comments.rs @@ -173,7 +173,6 @@ impl Automation { let only = a.opt_int("page")?.map(|p| (p.max(1) - 1) as usize); let all = self.comments(a)?; let view = |c: &Annotation| -> Value { - let p = &doc.info.pages[c.page.min(doc.info.pages.len().saturating_sub(1))]; json!({ "id": c.name, "page": c.page + 1, @@ -182,7 +181,8 @@ impl Automation { "author": c.author, "contents": c.contents, "modified": c.modified, - "rect": rect_to_view(p, c.rect), + // Unchanged on a page the viewer could not read (#2660), as `doc_info` reports it. + "rect": crate::view_rect(&doc.info, c.page, c.rect), "color": c.color.map(hex), }) }; diff --git a/crates/automation/src/content.rs b/crates/automation/src/content.rs index faefc5fa7..6d1c86002 100644 --- a/crates/automation/src/content.rs +++ b/crates/automation/src/content.rs @@ -73,9 +73,9 @@ impl Automation { .iter() .map(|it| { let index = per_page.entry(it.page).or_default(); - let ph = doc.info.pages[it.page].height as f64; - let mut v = - json!({ "page": it.page + 1, "index": *index + 1, "rect": to_view(it.content.rect(), ph).map(|x| (x * 100.0).round() / 100.0) }); + // Unchanged on a page the viewer could not read (#2660), like `crate::view_rect`. + let rect = doc.info.pages.get(it.page).map_or(it.content.rect(), |p| to_view(it.content.rect(), p.height as f64)); + let mut v = json!({ "page": it.page + 1, "index": *index + 1, "rect": rect.map(|x| (x * 100.0).round() / 100.0) }); *index += 1; match &it.content { AddedContent::Text(t) => { diff --git a/crates/automation/src/forms.rs b/crates/automation/src/forms.rs index f2eab5ddc..054303d4a 100644 --- a/crates/automation/src/forms.rs +++ b/crates/automation/src/forms.rs @@ -405,7 +405,7 @@ impl Automation { let r: Vec = v.as_array().map(|x| x.iter().filter_map(Value::as_f64).collect()).unwrap_or_default(); let r = <[f64; 4]>::try_from(r).map_err(|_| ToolError::InvalidArgs("rect must be 4 numbers".into()))?; let page = f.widgets.first().and_then(|w| w.page).ok_or_else(|| failed(format!("{name} is not on a page")))?; - let p = &doc.info.pages[page]; + let p = doc.info.pages.get(page).ok_or_else(|| failed(format!("{name} is on page {}, which could not be read", page + 1)))?; let (u0, u1) = (p.view_to_user(r[0] as f32, r[1] as f32), p.view_to_user(r[2] as f32, r[3] as f32)); Some((0, [u0[0].min(u1[0]) as f64, u0[1].min(u1[1]) as f64, u0[0].max(u1[0]) as f64, u0[1].max(u1[1]) as f64])) } diff --git a/crates/automation/src/links.rs b/crates/automation/src/links.rs index cd0fb9da0..8058d5639 100644 --- a/crates/automation/src/links.rs +++ b/crates/automation/src/links.rs @@ -70,9 +70,8 @@ impl Automation { .links() .iter() .map(|l| { - let p = &doc.info.pages[l.page]; - let (v0, v1) = (p.user_to_view(l.rect[0] as f32, l.rect[1] as f32), p.user_to_view(l.rect[2] as f32, l.rect[3] as f32)); - let rect = [v0[0].min(v1[0]), v0[1].min(v1[1]), v0[0].max(v1[0]), v0[1].max(v1[1])].map(|x| (x * 100.0).round() / 100.0); + // The link's page may be one the viewer could not read (#2660): rect unchanged then. + let rect = crate::view_rect(&doc.info, l.page, l.rect.map(|x| x as f32)); let mut v = json!({ "page": l.page + 1, "index": l.index + 1, "rect": rect, "visible": l.style.visible }); // Where an in-document link goes, as the viewer resolves it: named destinations // included, with the position on the page. diff --git a/crates/automation/src/signing.rs b/crates/automation/src/signing.rs index 94fc4bdec..c7a043c89 100644 --- a/crates/automation/src/signing.rs +++ b/crates/automation/src/signing.rs @@ -208,7 +208,7 @@ impl Automation { }; let rect = match a.nums::<4>("rect")? { Some(r) => { - let p = &doc.info.pages[page]; + let p = doc.info.pages.get(page).ok_or_else(|| bad("the document has no page to put the signature on"))?; let (u0, u1) = (p.view_to_user(r[0] as f32, r[1] as f32), p.view_to_user(r[2] as f32, r[3] as f32)); let u = [u0[0].min(u1[0]) as f64, u0[1].min(u1[1]) as f64, u0[0].max(u1[0]) as f64, u0[1].max(u1[1]) as f64]; if u[2] - u[0] < 4.0 || u[3] - u[1] < 4.0 { diff --git a/crates/automation/tests/hostile_args.rs b/crates/automation/tests/hostile_args.rs index 607696899..b7fd0f164 100644 --- a/crates/automation/tests/hostile_args.rs +++ b/crates/automation/tests/hostile_args.rs @@ -336,3 +336,114 @@ fn listing(dir: &Path) -> Vec<(PathBuf, u64)> { out.sort(); out } + +/// A six-page file whose fourth `/Kids` entry points at an object that does not exist (#2660), +/// with a link, a URI link and a comment on every page and a form field on the last. Readers +/// disagree on such a file: the viewer stopped at the dangling entry and saw three pages, while +/// the editing layer sees all six, so annotations come back on pages the viewer does not have. +fn dangling_kid_pdf() -> Vec { + let pages: Vec = (0..6).map(|i| 10 + 2 * i).collect(); + let mut kids: Vec = pages[..3].to_vec(); + kids.push(77); // does not exist + kids.extend(&pages[3..]); + let kids: Vec = kids.iter().map(|k| format!("{k} 0 R")).collect(); + let mut objs: Vec<(u32, String)> = vec![ + (1, "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [50 0 R] >> >>".into()), + (2, format!("<< /Type /Pages /Kids [{}] /Count 6 >>", kids.join(" "))), + (90, "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica >>".into()), + ]; + for (i, &page) in pages.iter().enumerate() { + let text = format!("BT /F1 24 Tf 50 300 Td (Page {}) Tj ET", i + 1); + let field = if i == 5 { " 50 0 R" } else { "" }; + objs.push(( + page, + format!( + "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 400 400] /Resources << /Font << /F1 90 0 R >> >> /Contents {} 0 R \ + /Annots [<< /Type /Annot /Subtype /Link /Rect [10 10 100 40] /Dest [10 0 R /Fit] >> \ + << /Type /Annot /Subtype /Link /Rect [10 50 100 80] /A << /S /URI /URI (https://example.com/) >> >> \ + << /Type /Annot /Subtype /Text /Rect [200 200 220 220] /Contents (note) /T (author) >>{field}] >>", + page + 1 + ), + )); + objs.push((page + 1, format!("<< /Length {} >>\nstream\n{text}\nendstream", text.len()))); + } + objs.push((50, "<< /FT /Tx /T (name) /Type /Annot /Subtype /Widget /Rect [50 100 250 130] /P 20 0 R >>".into())); + objs.sort_by_key(|(n, _)| *n); + + let size = 100; + let mut out = b"%PDF-1.7\n".to_vec(); + let mut offsets = vec![None; size]; + for (num, body) in &objs { + if let Some(slot) = offsets.get_mut(*num as usize) { + *slot = Some(out.len()); + } + out.extend_from_slice(format!("{num} 0 obj\n{body}\nendobj\n").as_bytes()); + } + let xref = out.len(); + out.extend_from_slice(format!("xref\n0 {size}\n0000000000 65535 f \n").as_bytes()); + for offset in offsets.iter().skip(1) { + out.extend_from_slice( + match offset { + Some(o) => format!("{o:010} 00000 n \n"), + None => "0000000000 65535 f \n".to_string(), + } + .as_bytes(), + ); + } + out.extend_from_slice(format!("trailer << /Size {size} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n").as_bytes()); + out +} + +#[test] +fn no_tool_panics_when_the_viewer_sees_fewer_pages_than_the_file_has() { + // #2660: `link_list` indexed the viewer's pages with a page number from the editing layer + // ("index out of bounds: the len is 3 but the index is 3"). Every tool, on a fresh copy of a + // file where the two disagree, must answer with a result or an error. + let root = tmp("dangling-kid"); + let pdf = dangling_kid_pdf(); + let mut failures = Vec::new(); + for def in tools() { + // Written again for every tool: tools that save write to `in.pdf`, the plausible path. + std::fs::write(root.join("in.pdf"), &pdf).expect("fixture"); + let mut a = Automation::new().with_root(&root).expect("root"); + let opened = a.call("doc_open", &json!({ "path": "in.pdf" })).expect("open the damaged file"); + let doc = opened + .into_iter() + .find_map(|c| match c { + pdfcraft_automation::Content::Json(v) => v["doc"].as_u64(), + pdfcraft_automation::Content::Png { .. } => None, + }) + .expect("the opened document has an id"); + if let Err(e) = guarded(&mut a, def.name, &Value::Object(plausible_with(&def, Some(doc)))) { + failures.push(e); + } + match def.name { + // The field sits on the sixth page; a rect is placed on the field's own page. + "form_set_props" => { + if let Err(e) = guarded(&mut a, def.name, &json!({ "doc": doc, "field": "name", "rect": [10, 10, 60, 30] })) { + failures.push(e); + } + } + // Listing still reports what is on the pages the viewer lacks, not just survives it. + "link_list" => { + let listed = catch_unwind(AssertUnwindSafe(|| a.call("link_list", &json!({ "doc": doc })))); + let count = match listed { + Ok(Ok(content)) => content.into_iter().find_map(|c| match c { + pdfcraft_automation::Content::Json(v) => v["count"].as_u64(), + pdfcraft_automation::Content::Png { .. } => None, + }), + _ => None, + }; + if count != Some(12) { + failures.push(format!("link_list listed {count:?} links, not the file's 12")); + } + } + _ => {} + } + } + for f in &failures { + eprintln!(" {f}"); + } + let _ = std::fs::remove_dir_all(&root); + assert!(failures.is_empty(), "{} tool(s) panicked; the first is: {}", failures.len(), failures.first().map_or("", String::as_str)); +} From 85da56ed244b9427ff6211d3e363c559f47036fe Mon Sep 17 00:00:00 2001 From: Christoph Ortenburg Date: Mon, 19 Oct 2026 14:43:33 -0400 Subject: [PATCH 2/2] test(automation): pin the content_list guard and form_set_props' refusal (#2660) Review on #2715: reverting only the content_list guard still passed. The fixture now has text added by PdfCraft (`/PCAdded`) on page 6, and content_list must report it at its unchanged rect [70 70 150 100]. form_set_props must refuse a rect for the field on page 6 with "name is on page 6, which could not be read" and leave form_fields unchanged. Reverting any one of the link, content or form guards now fails the test. Co-Authored-By: Claude Opus 5.5 --- crates/automation/tests/hostile_args.rs | 76 ++++++++++++++++++------- 1 file changed, 54 insertions(+), 22 deletions(-) diff --git a/crates/automation/tests/hostile_args.rs b/crates/automation/tests/hostile_args.rs index b7fd0f164..d393e3726 100644 --- a/crates/automation/tests/hostile_args.rs +++ b/crates/automation/tests/hostile_args.rs @@ -338,7 +338,8 @@ fn listing(dir: &Path) -> Vec<(PathBuf, u64)> { } /// A six-page file whose fourth `/Kids` entry points at an object that does not exist (#2660), -/// with a link, a URI link and a comment on every page and a form field on the last. Readers +/// with a link, a URI link and a comment on every page, and a form field and text added by +/// PdfCraft (its `/PCAdded` marker) on the last. Readers /// disagree on such a file: the viewer stopped at the dangling entry and saw three pages, while /// the editing layer sees all six, so annotations come back on pages the viewer does not have. fn dangling_kid_pdf() -> Vec { @@ -354,20 +355,27 @@ fn dangling_kid_pdf() -> Vec { ]; for (i, &page) in pages.iter().enumerate() { let text = format!("BT /F1 24 Tf 50 300 Td (Page {}) Tj ET", i + 1); - let field = if i == 5 { " 50 0 R" } else { "" }; + let (field, contents) = if i == 5 { (" 50 0 R", format!("[{} 0 R 60 0 R]", page + 1)) } else { ("", format!("{} 0 R", page + 1)) }; objs.push(( page, format!( - "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 400 400] /Resources << /Font << /F1 90 0 R >> >> /Contents {} 0 R \ + "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 400 400] /Resources << /Font << /F1 90 0 R >> >> /Contents {contents} \ /Annots [<< /Type /Annot /Subtype /Link /Rect [10 10 100 40] /Dest [10 0 R /Fit] >> \ << /Type /Annot /Subtype /Link /Rect [10 50 100 80] /A << /S /URI /URI (https://example.com/) >> >> \ - << /Type /Annot /Subtype /Text /Rect [200 200 220 220] /Contents (note) /T (author) >>{field}] >>", - page + 1 + << /Type /Annot /Subtype /Text /Rect [200 200 220 220] /Contents (note) /T (author) >>{field}] >>" ), )); objs.push((page + 1, format!("<< /Length {} >>\nstream\n{text}\nendstream", text.len()))); } objs.push((50, "<< /FT /Tx /T (name) /Type /Annot /Subtype /Widget /Rect [50 100 250 130] /P 20 0 R >>".into())); + let added = "BT /F1 12 Tf 72 80 Td (AddedSample) Tj ET"; + objs.push(( + 60, + format!( + "<< /PCMark /Added /PCAdded << /Kind /Text /Rect [70 70 150 100] /Text (AddedSample) /Size 12 >> /Length {} >>\nstream\n{added}\nendstream", + added.len() + ), + )); objs.sort_by_key(|(n, _)| *n); let size = 100; @@ -400,6 +408,20 @@ fn no_tool_panics_when_the_viewer_sees_fewer_pages_than_the_file_has() { // ("index out of bounds: the len is 3 but the index is 3"). Every tool, on a fresh copy of a // file where the two disagree, must answer with a result or an error. let root = tmp("dangling-kid"); + // The JSON a call answers with, or the error it gave. + let answer = |a: &mut Automation, tool: &str, args: &Value| -> std::result::Result { + match catch_unwind(AssertUnwindSafe(|| a.call(tool, args))) { + Ok(Ok(content)) => content + .into_iter() + .find_map(|c| match c { + pdfcraft_automation::Content::Json(v) => Some(v), + pdfcraft_automation::Content::Png { .. } => None, + }) + .ok_or_else(|| format!("{tool} answered without JSON")), + Ok(Err(e)) => Err(format!("{tool} failed: {e:?}")), + Err(_) => Err(format!("{tool} panicked")), + } + }; let pdf = dangling_kid_pdf(); let mut failures = Vec::new(); for def in tools() { @@ -418,26 +440,36 @@ fn no_tool_panics_when_the_viewer_sees_fewer_pages_than_the_file_has() { failures.push(e); } match def.name { - // The field sits on the sixth page; a rect is placed on the field's own page. + // The field sits on the sixth page; a rect is placed on the field's own page. That is + // refused, saying why, and the field is left as it was. "form_set_props" => { - if let Err(e) = guarded(&mut a, def.name, &json!({ "doc": doc, "field": "name", "rect": [10, 10, 60, 30] })) { - failures.push(e); + let fields = |a: &mut Automation| answer(a, "form_fields", &json!({ "doc": doc })); + let before = fields(&mut a); + match catch_unwind(AssertUnwindSafe(|| a.call(def.name, &json!({ "doc": doc, "field": "name", "rect": [10, 10, 60, 30] })))) { + Ok(Err(ToolError::Failed(m))) if m == "name is on page 6, which could not be read" => {} + Ok(Err(e)) => failures.push(format!("form_set_props gave the wrong error: {e:?}")), + Ok(Ok(_)) => failures.push("form_set_props placed a rect on a page the viewer could not read".into()), + Err(_) => failures.push("form_set_props panicked on a rect for the field on page 6".into()), + } + let after = fields(&mut a); + if before.is_err() || before != after { + failures.push(format!("form_set_props changed the field: {before:?} became {after:?}")); } } // Listing still reports what is on the pages the viewer lacks, not just survives it. - "link_list" => { - let listed = catch_unwind(AssertUnwindSafe(|| a.call("link_list", &json!({ "doc": doc })))); - let count = match listed { - Ok(Ok(content)) => content.into_iter().find_map(|c| match c { - pdfcraft_automation::Content::Json(v) => v["count"].as_u64(), - pdfcraft_automation::Content::Png { .. } => None, - }), - _ => None, - }; - if count != Some(12) { - failures.push(format!("link_list listed {count:?} links, not the file's 12")); - } - } + "link_list" => match answer(&mut a, def.name, &json!({ "doc": doc })) { + Ok(v) if v["count"] == 12 => {} + other => failures.push(format!("link_list did not list the file's 12 links: {other:?}")), + }, + // The added text on page 6 is reported, its rect unchanged (user space), as + // `doc_info` reports rects on pages the viewer could not read. + "content_list" => match answer(&mut a, def.name, &json!({ "doc": doc })) { + Ok(v) + if v["items"].as_array().is_some_and(|items| { + items.iter().any(|it| it["page"] == 6 && it["type"] == "text" && it["rect"] == json!([70.0, 70.0, 150.0, 100.0])) + }) => {} + other => failures.push(format!("content_list did not report the added text on page 6 at [70, 70, 150, 100]: {other:?}")), + }, _ => {} } } @@ -445,5 +477,5 @@ fn no_tool_panics_when_the_viewer_sees_fewer_pages_than_the_file_has() { eprintln!(" {f}"); } let _ = std::fs::remove_dir_all(&root); - assert!(failures.is_empty(), "{} tool(s) panicked; the first is: {}", failures.len(), failures.first().map_or("", String::as_str)); + assert!(failures.is_empty(), "{} failure(s); the first is: {}", failures.len(), failures.first().map_or("", String::as_str)); }