diff --git a/crates/engine/src/document.rs b/crates/engine/src/document.rs index 602410922f..805e089dd7 100644 --- a/crates/engine/src/document.rs +++ b/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 = 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 = std::collections::HashMap::new(); + for (i, p) in model.iter().enumerate() { + index.entry(p.obj).or_insert(i); + } + let mut marked: Vec = Vec::new(); + let mut rects_of: std::collections::HashMap> = 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 = 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); diff --git a/crates/engine/src/edit/run.rs b/crates/engine/src/edit/run.rs index 57e5729fc8..0a7a348fca 100644 --- a/crates/engine/src/edit/run.rs +++ b/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, + /// 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, /// 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; } diff --git a/crates/engine/src/session/editing.rs b/crates/engine/src/session/editing.rs index 4a6f372fb4..a039f56798 100644 --- a/crates/engine/src/session/editing.rs +++ b/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::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 diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 4e24a4f608..91b00df4cc 100644 --- a/crates/engine/src/tests.rs +++ b/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. diff --git a/crates/redact/src/lib.rs b/crates/redact/src/lib.rs index 8f54cefdc6..819cae99cd 100644 --- a/crates/redact/src/lib.rs +++ b/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(