fix(engine): run the renderer redaction check on redactions applied inside a batch (#5977)

* fix(engine): run the renderer redaction check on redactions applied inside a batch

* Resolve redaction marks by page object and measure the edited document

The renderer check looked marks up by the page index they had when they
were drawn, so a MovePages later in the same batch pointed them at another
page's text, and it measured glyphs with page geometry from before the edit.

Each mark now carries the page object it was placed on, and the geometry is
read back from the document as the batch wrote it.

* review: skip marks whose page the batch deleted

* review: group marks by page once and refuse an ambiguous page tree

* address review: refuse a batch that deletes a marked page while adding pages

* merge main, keep both

* address review: a batch that applies redactions and then copies a page is refused

crates/engine/src/document.rs

@@ -958,41 +958,60 @@ pub(crate) fn recovered_render_password(cos: &pdfcraft_cos::Document) -> Option<
 /// the document as it was.
 pub(crate) fn shown_under_redactions(
     after: &pdfcraft_cos::Document,
-    marks: &[pdfcraft_redact::Mark],
+    marks: &[(pdfcraft_cos::ObjRef, pdfcraft_redact::Mark)],
     overlays: &[pdfcraft_cos::ObjRef],
-    pages: &[pdfcraft_render::PageInfo],
     password: Option<&str>,
 ) -> Result<(), EditError> {
     use pdfcraft_cos::Object;
     use pdfcraft_redact::RedactError;
-    let mut marked: Vec<usize> = marks.iter().map(|m| m.page).collect();
+    let mut shown = after.clone();
+    let model = pdfcraft_model::pages(&shown);
+    // A batch can move pages after the marks were collected: a mark is found by the page object it
+    // was placed on, not by the index that page had when it was drawn. A page the batch deleted
+    // takes its marks with it, so they are not checked against whatever page now holds the index.
+    // One index, built once: each mark is resolved against it instead of scanning every page,
+    // and the marks are grouped by resolved page in the same pass.
+    let mut index: std::collections::HashMap<pdfcraft_cos::ObjRef, usize> = std::collections::HashMap::new();
+    for (i, p) in model.iter().enumerate() {
+        index.entry(p.obj).or_insert(i);
+    }
+    let mut marked: Vec<usize> = Vec::new();
+    let mut rects_of: std::collections::HashMap<usize, Vec<[f64; 4]>> = std::collections::HashMap::new();
+    for (obj, m) in marks {
+        let Some(&pi) = index.get(obj) else { continue };
+        if !rects_of.contains_key(&pi) {
+            marked.push(pi);
+        }
+        rects_of.entry(pi).or_default().extend(m.rects.iter().map(|r| [r[0].min(r[2]), r[1].min(r[3]), r[0].max(r[2]), r[1].max(r[3])]));
+    }
     marked.sort_unstable();
-    marked.dedup();
     if marked.is_empty() {
         return Ok(());
     }
-    let mut shown = after.clone();
-    let model = pdfcraft_model::pages(&shown);
     for &pi in &marked {
         let Some(page) = model.get(pi) else { continue };
         let Some(Object::Array(list)) = page.dict.get(b"Contents").map(|c| (*shown.resolve(c)).clone()) else { continue };
         let kept: Vec<Object> = list.into_iter().filter(|o| !o.as_ref().is_some_and(|r| overlays.contains(&r))).collect();
         shown.update_dict(page.obj, |d| d.set(b"Contents".to_vec(), Object::Array(kept))).map_err(|e| EditError::Write(e.to_string()))?;
     }
-    let bytes = write_incremental(&shown, &SaveOptions::default()).map_err(|e| EditError::Write(e.to_string()))?;
+    let bytes = Arc::new(write_incremental(&shown, &SaveOptions::default()).map_err(|e| EditError::Write(e.to_string()))?);
+    // Page geometry comes from the document as it is now written: a page moved earlier in the batch
+    // brings its own size and rotation with it.
+    let pages = pdfcraft_render::inspect(bytes.clone(), password).map_err(|e| EditError::Write(e.to_string()))?.pages;
+    // The model counts one entry per `/Kids` entry; the renderer one per page it draws. If they
+    // disagree, a mark's index could land on another page's text, and the check cannot confirm
+    // the redaction.
+    if pages.len() != model.len() {
+        return Err(RedactError::AmbiguousPages.into());
+    }
     let config = RenderConfig { password: password.map(Arc::from), ..Default::default() };
-    let mut renderer = pdfcraft_render::PageRenderer::new(Arc::new(bytes), config);
+    let mut renderer = pdfcraft_render::PageRenderer::new(bytes, config);
     let mut residue = 0;
     for &pi in &marked {
         let request = pdfcraft_render::RenderRequest { page: pi, kind: pdfcraft_render::RequestKind::Text, scale: 1.0, ..Default::default() };
         // A page that cannot be read back cannot be confirmed clean.
         let (Some(info), Some(text)) = (pages.get(pi), renderer.render(request).text) else { return Err(RedactError::Unreadable(pi + 1).into()) };
-        let rects: Vec<[f64; 4]> = marks
-            .iter()
-            .filter(|m| m.page == pi)
-            .flat_map(|m| m.rects.iter())
-            .map(|r| [r[0].min(r[2]), r[1].min(r[3]), r[0].max(r[2]), r[1].max(r[3])])
-            .collect();
+        let rects = rects_of.get(&pi).cloned().unwrap_or_default();
         for glyph in text.glyphs.iter().filter(|g| !g.text.trim().is_empty()) {
             let [x0, y0, x1, y1] = info.view_rect_to_user(glyph.rect).map(f64::from);
             let area = (x1 - x0) * (y1 - y0);

crates/engine/src/edit/run.rs

@@ -253,6 +253,11 @@ pub(crate) struct EditCtx {
     /// The overlay streams the edit's applied redactions added, which alone the renderer's check
     /// leaves out.
     pub(crate) redaction_overlays: Vec<pdfcraft_cos::ObjRef>,
+    /// The marks the edit's applied redactions took out (a batch's included), as they were when
+    /// each apply ran, for the renderer's check.
+    /// Each applied mark with the page object it was drawn on, which a later page move changes the
+    /// index of but not the identity.
+    pub(crate) redaction_marks: Vec<(pdfcraft_cos::ObjRef, pdfcraft_redact::Mark)>,
     /// What the edit's applied redactions left as it was that still holds the removed text.
     pub(crate) redaction_elsewhere: Option<pdfcraft_redact::Elsewhere>,
     /// What became of the PDF/A and PDF/UA identification when the edit's applied redactions
@@ -310,6 +315,7 @@ impl EditCtx {
             rng: getrandom::fill,
             renamed_fields: Vec::new(),
             redaction_overlays: Vec::new(),
+            redaction_marks: Vec::new(),
             redaction_elsewhere: None,
             redaction_identification: pdfcraft_redact::Identification::default(),
             prepared: None,
@@ -529,7 +535,17 @@ pub(crate) fn run_edit(doc: &mut pdfcraft_cos::Document, edit: &Edit, cx: &mut E
             pdfcraft_organize::insert_blank_page(doc, *at, *width, *height)?;
         }
         Edit::SetInfo { key, value } => pdfcraft_organize::set_info(doc, key, value)?,
-        Edit::DuplicatePages { pages, at } => pdfcraft_organize::duplicate_pages(doc, pages, *at)?,
+        Edit::DuplicatePages { pages, at } => {
+            // A copy of a page cannot be tied back to the marks an earlier redaction of this edit
+            // applied: text put back under a mark would sit on the copy, unchecked. Fail closed.
+            if !cx.redaction_marks.is_empty() {
+                return Err(EditError::Invalid(
+                    "a batch that applies redactions can't also copy pages: the copies can't be checked against the marks; the document was not changed"
+                        .into(),
+                ));
+            }
+            pdfcraft_organize::duplicate_pages(doc, pages, *at)?
+        }
         Edit::ReplacePages { pages, name, bytes, src_pages } => {
             let src = crate::sources::open_source_into(name, bytes, doc)?;
             pdfcraft_organize::replace_pages(doc, pages, &src, src_pages)?;
@@ -991,8 +1007,15 @@ pub(crate) fn run_edit(doc: &mut pdfcraft_cos::Document, edit: &Edit, cx: &mut E
             pdfcraft_edit::add_content(doc, *page, &content)?;
         }
         Edit::ApplyRedactions { pages } => {
+            let model = pdfcraft_model::pages(doc);
+            cx.redaction_marks.extend(
+                pdfcraft_redact::marks(doc)
+                    .into_iter()
+                    .filter(|m| pages.as_ref().is_none_or(|p| p.contains(&m.page)))
+                    .filter_map(|m| model.get(m.page).map(|p| (p.obj, m))),
+            );
             let report = pdfcraft_redact::apply(doc, pages.as_deref())?;
-            cx.redaction_overlays = report.overlays;
+            cx.redaction_overlays.extend(report.overlays);
             cx.redaction_elsewhere = Some(report.still_holding);
             cx.redaction_identification = report.identification;
         }

crates/engine/src/session/editing.rs

@@ -185,6 +185,7 @@ impl Session {
         }
         cx.renamed_fields.clear();
         cx.redaction_overlays.clear();
+        cx.redaction_marks.clear();
         // `next` is a copy: if the edit fails or crashes, the document is unchanged.
         run_checked_edit(&mut next, &edit, &mut cx)?;
         // A field's enter or exit scripts that changed nothing leave no edit behind (no undo step,
@@ -195,11 +196,20 @@ impl Session {
             return Ok(None);
         }
         // Redaction removed what its own interpreter places under the marks. The marks were drawn
-        // over the page as the renderer shows it, so the renderer has the last word.
-        if let Edit::ApplyRedactions { pages } = &edit {
-            let marks: Vec<pdfcraft_redact::Mark> =
-                pdfcraft_redact::marks(&editor.cos).into_iter().filter(|m| pages.as_ref().is_none_or(|p| p.contains(&m.page))).collect();
-            guard(|| shown_under_redactions(&next, &marks, &cx.redaction_overlays, &doc.info.pages, doc.password.as_deref()))
+        // over the page as the renderer shows it, so the renderer has the last word: on every
+        // apply the edit ran, a batch's included.
+        if !cx.redaction_marks.is_empty() {
+            // A batch that deletes a marked page and adds pages cannot be tied back to the marks:
+            // a copy of a page may carry the text the redaction removed from the original only.
+            // Fail closed.
+            let before: std::collections::HashSet<_> = pdfcraft_model::pages(&editor.cos).iter().map(|p| p.obj).collect();
+            let after: std::collections::HashSet<_> = pdfcraft_model::pages(&next).iter().map(|p| p.obj).collect();
+            if cx.redaction_marks.iter().any(|(o, _)| !after.contains(o)) && after.iter().any(|o| !before.contains(o)) {
+                return Err(EditError::Invalid(
+                    "a batch that deletes a page with a redaction mark and adds pages can't be verified; the document was not changed".into(),
+                ));
+            }
+            guard(|| shown_under_redactions(&next, &cx.redaction_marks, &cx.redaction_overlays, doc.password.as_deref()))
                 .unwrap_or_else(|m| Err(EditError::Invalid(format!("{} failed unexpectedly ({m}); the document was not changed", edit.label()))))?;
         }
         // Renamed fields are said, not slipped in: a repair-log note (with the caveat that

crates/engine/src/tests.rs

@@ -3542,9 +3542,10 @@ fn the_renderer_check_refuses_text_still_shown_under_a_mark() {
     mark_shown_text(&mut s, id, "SECRET42");
     let doc = s.get(id).unwrap();
     let mut cos = doc.editor.as_ref().unwrap().cos.clone();
-    let marks = pdfcraft_redact::marks(&cos);
+    let model = pdfcraft_model::pages(&cos);
+    let marks: Vec<_> = pdfcraft_redact::marks(&cos).into_iter().filter_map(|m| model.get(m.page).map(|p| (p.obj, m))).collect();
     let refused = |cos: &pdfcraft_cos::Document| {
-        let shown = shown_under_redactions(cos, &marks, &[], &doc.info.pages, None);
+        let shown = shown_under_redactions(cos, &marks, &[], None);
         matches!(shown, Err(EditError::Redact(pdfcraft_redact::RedactError::Residue(n))) if n > 0)
     };
     assert!(refused(&cos));
@@ -13778,6 +13779,66 @@ fn image_transform_engine_refuses_singular_matrices_without_mutation() {
     }
 }
 
+/// The renderer's check covers a redaction applied inside a batch too, with the marks as they
+/// were when it ran: one the batch itself added is checked, and text a later edit of the batch
+/// puts back under a mark refuses the whole batch.
+#[test]
+fn redactions_applied_in_a_batch_get_the_renderer_check() {
+    let mut s = Session::new().with_clock(|| 1_700_000_000);
+    let page = one_line_page_with("/Font << /F1 4 0 R >>", "", "BT /F1 16 Tf 20 150 Td (Public words then SECRET42) Tj ET");
+    let id = s.open("batch.pdf", None, Arc::new(page), None).unwrap();
+    mark_shown_text(&mut s, id, "SECRET42");
+    let apply = Edit::ApplyRedactions { pages: None };
+    // Text added where the mark was shows under it again: the renderer sees it and refuses.
+    let under = AddedText { rect: [150.0, 130.0, 300.0, 160.0], text: "SECRET42 SECRET42".into(), ..Default::default() };
+    let refused = s.apply(id, Edit::Batch { label: "Redact then write".into(), edits: vec![apply.clone(), Edit::AddText { page: 0, text: under }] });
+    assert!(matches!(refused, Err(EditError::Redact(pdfcraft_redact::RedactError::Residue(n))) if n > 0), "{refused:?}");
+    assert!(page_texts(&s, id)[0].contains("SECRET42"), "nothing changed");
+    assert_eq!(s.get(id).unwrap().redaction_marks(), 1);
+    // Applied alone in a batch, the redaction goes through, and the text is gone.
+    s.apply(id, Edit::Batch { label: "Redact".into(), edits: vec![apply] }).unwrap();
+    assert!(!page_texts(&s, id)[0].contains("SECRET42"), "{:?}", page_texts(&s, id));
+    assert_eq!(s.get(id).unwrap().redaction_marks(), 0);
+}
+
+/// A batch that deletes a page holding a redaction mark and adds pages is refused whole: the
+/// marks cannot be tied to copies, so redaction fails closed. Without added pages it goes through.
+#[test]
+fn a_batch_that_deletes_a_marked_page_and_adds_pages_is_refused() {
+    let mut s = Session::new().with_clock(|| 1_700_000_000);
+    let page = one_line_page_with("/Font << /F1 4 0 R >>", "", "BT /F1 16 Tf 20 150 Td (Public words then SECRET42) Tj ET");
+    let id = s.open("batch.pdf", None, Arc::new(page), None).unwrap();
+    mark_shown_text(&mut s, id, "SECRET42");
+    let edits = vec![
+        Edit::ApplyRedactions { pages: None },
+        Edit::InsertBlankPage { at: 1, width: 612.0, height: 792.0 },
+        Edit::DeletePages { pages: vec![0] },
+    ];
+    let refused = s.apply(id, Edit::Batch { label: "x".into(), edits });
+    assert!(matches!(refused, Err(EditError::Invalid(_))), "{refused:?}");
+    assert!(page_texts(&s, id)[0].contains("SECRET42"), "nothing changed");
+    assert_eq!(s.get(id).unwrap().redaction_marks(), 1);
+}
+
+/// A batch that applies redactions and then copies a page is refused whole: the copy can't be tied
+/// to the marks, so text restored under one would be unchecked. Copying first, or alone, is fine.
+#[test]
+fn a_batch_that_applies_redactions_and_copies_a_page_is_refused() {
+    let mut s = Session::new().with_clock(|| 1_700_000_000);
+    let page = one_line_page_with("/Font << /F1 4 0 R >>", "", "BT /F1 16 Tf 20 150 Td (Public words then SECRET42) Tj ET");
+    let id = s.open("batch.pdf", None, Arc::new(page), None).unwrap();
+    mark_shown_text(&mut s, id, "SECRET42");
+    let apply = Edit::ApplyRedactions { pages: None };
+    let copy = Edit::DuplicatePages { pages: vec![0], at: None };
+    let refused = s.apply(id, Edit::Batch { label: "x".into(), edits: vec![apply.clone(), copy.clone()] });
+    assert!(matches!(&refused, Err(EditError::Invalid(m)) if m.contains("copy pages")), "{refused:?}");
+    assert!(page_texts(&s, id)[0].contains("SECRET42"), "nothing changed");
+    assert_eq!(s.get(id).unwrap().redaction_marks(), 1);
+    // Copying before the redaction is applied puts the mark on the copy too, and both are checked.
+    s.apply(id, Edit::Batch { label: "y".into(), edits: vec![copy, apply] }).unwrap();
+    assert!(page_texts(&s, id).iter().all(|t| !t.contains("SECRET42")), "{:?}", page_texts(&s, id));
+}
+
 /// A reset in an opening script reaches a field of a row an earlier opening script added, before
 /// the form is laid out again with the row's widgets: its data goes back to its default and stays
 /// so when the form is saved.

crates/redact/src/lib.rs

@@ -58,6 +58,12 @@ pub enum RedactError {
     NothingToApply,
     #[error("page {0} has content that can't be read, so it can't be redacted safely")]
     Unreadable(usize),
+    /// The page tree and the renderer count a different number of pages (a page repeated in
+    /// `/Kids`, say), so a mark's page can't be matched to the page the renderer drew.
+    #[error(
+        "the document's page tree and its rendered pages disagree on how many pages there are, so the redaction could not be verified; nothing was changed"
+    )]
+    AmbiguousPages,
     /// Redact annotations with no readable area (neither usable `/QuadPoints` nor a usable `/Rect`).
     /// Skipping them would leave the text under them in the file while the others report success.
     #[error(