Merge PR #3728: Refuse redaction marks with no readable area instead of skipping them

crates/automation/tests/automation.rs

@@ -3445,6 +3445,48 @@ fn redacting_through_tools() {
     assert!(matches!(a.call("redact_mark", &json!({ "doc": doc })), Err(ToolError::InvalidArgs(_))));
 }
 
+/// One page with the text "Secret 1234" and a Redact annotation that has neither a usable
+/// QuadPoints array nor a Rect, so there is no area to redact.
+fn redact_pdf_with_unreadable_mark() -> Vec<u8> {
+    let body = "BT /F1 24 Tf 20 150 Td (Secret 1234) Tj ET";
+    let objs: Vec<String> = vec![
+        "<< /Type /Catalog /Pages 2 0 R >>".into(),
+        "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 300] >>".into(),
+        "<< /Type /Page /Parent 2 0 R /Contents 4 0 R /Resources << /Font << /F1 5 0 R >> >> /Annots [6 0 R] >>".into(),
+        format!("<< /Length {} >>\nstream\n{body}\nendstream", body.len()),
+        "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica >>".into(),
+        "<< /Type /Annot /Subtype /Redact /QuadPoints /oops >>".into(),
+    ];
+    let mut bytes = b"%PDF-1.7\n".to_vec();
+    let mut offsets = Vec::new();
+    for (i, o) in objs.iter().enumerate() {
+        offsets.push(bytes.len());
+        bytes.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes());
+    }
+    let xref = bytes.len();
+    bytes.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes());
+    for o in offsets {
+        bytes.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes());
+    }
+    bytes.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes());
+    bytes
+}
+
+/// A mark that can't be read is refused by `redact_apply`, with the reason in the reply, and the
+/// page keeps its text. (Before, the mark was skipped and the reply said there were no marks.)
+#[test]
+fn an_unreadable_redaction_mark_is_refused_with_the_reason() {
+    let dir = workdir("redact-unreadable");
+    std::fs::write(dir.join("broken.pdf"), redact_pdf_with_unreadable_mark()).unwrap();
+    let mut a = auto(&dir);
+    let doc = ok(&mut a, "doc_open", json!({ "path": "broken.pdf" }))["doc"].as_u64().unwrap();
+    let Err(ToolError::Failed(reason)) = a.call("redact_apply", &json!({ "doc": doc })) else {
+        panic!("redact_apply must refuse a mark with no readable area");
+    };
+    assert!(reason.contains("1 redaction mark(s) have no readable area") && reason.contains("nothing was redacted"), "{reason}");
+    assert!(page_text(&mut a, doc)[0].contains("Secret 1234"), "the page is unchanged");
+}
+
 #[test]
 fn redacting_with_codes_through_tools() {
     let dir = workdir("redact-codes");

crates/redact/README.md

@@ -32,7 +32,9 @@ clear_marks(&mut doc, None)?;                        // remove marks without app
 
 A verification pass re-reads each redacted page and fails the operation if any glyph or inline
 image is still under an area. Content that can't be decoded makes the operation fail rather than
-leave it unredacted.
+leave it unredacted. A Redact annotation with neither a usable `/QuadPoints` array nor a usable
+`/Rect` makes it fail with `RedactError::UnreadableMark` before anything changes, instead of
+skipping that mark. The count covers the marks on the pages being applied.
 
 Before changing any page, `apply` prepares the redaction appearances with a shared 8 MiB
 budget for their uncompressed PDF content across the selected pages. It bounds repeated-text

crates/redact/src/lib.rs

@@ -38,6 +38,12 @@ pub enum RedactError {
     NothingToApply,
     #[error("page {0} has content that can't be read, so it can't be redacted safely")]
     Unreadable(usize),
+    /// 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(
+        "{0} redaction mark(s) have no readable area (neither usable QuadPoints nor a usable Rect), so the text under them can't be known; nothing was redacted. Remove those marks or mark the text again, then apply again"
+    )]
+    UnreadableMark(usize),
     #[error("redaction could not be verified: {0} item(s) were still found under the marks, so nothing was changed")]
     Residue(usize),
     /// A mark's overlay text has a character Helvetica can't draw; nothing was redacted.
@@ -162,7 +168,13 @@ pub(crate) fn listed_annotations(doc: &Document) -> HashSet<ObjRef> {
 
 /// Every redaction mark in the document, page by page.
 pub fn marks(doc: &Document) -> Vec<Mark> {
-    let mut out = Vec::new();
+    marks_checked(doc).0
+}
+
+/// [`marks`], and the 0-based page of each Redact annotation that has no readable area, which
+/// [`apply`] refuses rather than skipping.
+pub(crate) fn marks_checked(doc: &Document) -> (Vec<Mark>, Vec<usize>) {
+    let (mut out, mut unreadable) = (Vec::new(), Vec::new());
     for (pi, p) in pdfcraft_model::pages(doc).iter().enumerate() {
         for a in annots_of(doc, &p.dict) {
             let Some(r) = a.as_ref() else { continue };
@@ -197,13 +209,14 @@ pub fn marks(doc: &Document) -> Vec<Mark> {
                 rects.push(r);
             }
             if rects.is_empty() {
+                unreadable.push(pi);
                 continue;
             }
             let overlay = d.get(b"OverlayText").and_then(|t| doc.resolve(t).as_string().map(|s| s.to_text())).unwrap_or_default();
             out.push(Mark { page: pi, obj: r, rects, fill: color(doc, d, b"IC"), overlay, look: overlay_look(doc, d) });
         }
     }
-    out
+    (out, unreadable)
 }
 
 /// A page's content streams: (the references or inline objects as listed, their decoded data).
@@ -326,7 +339,13 @@ fn localize_resources(doc: &mut Document, selected: &[usize]) -> Result<(), pdfc
 /// sensitive content still present. Call this on a working clone and replace the original only
 /// on success. On any error, discard the working clone; do not save or reuse it.
 pub fn apply(doc: &mut Document, pages: Option<&[usize]>) -> Result<Report, RedactError> {
-    let all_marks = marks(doc);
+    let (all_marks, unreadable) = marks_checked(doc);
+    // A mark with no readable area is not applied, and applying the others would look complete:
+    // refuse before anything changes. Only the pages being applied count.
+    let unreadable = unreadable.iter().filter(|page| pages.is_none_or(|p| p.contains(page))).count();
+    if unreadable > 0 {
+        return Err(RedactError::UnreadableMark(unreadable));
+    }
     let chosen: Vec<&Mark> = all_marks.iter().filter(|m| pages.is_none_or(|p| p.contains(&m.page))).collect();
     if chosen.is_empty() {
         return Err(RedactError::NothingToApply);

crates/redact/src/tests.rs

@@ -1057,6 +1057,75 @@ fn unreadable_content_fails_closed() {
     assert_eq!(apply(&mut doc, None), Err(RedactError::Unreadable(1)));
 }
 
+/// Leaves a mark with nothing readable: /QuadPoints becomes a name and /Rect is removed.
+fn break_area(doc: &mut Document, obj: ObjRef) {
+    doc.update_dict(obj, |d| {
+        d.set(b"QuadPoints".to_vec(), Object::name("oops"));
+        d.remove(b"Rect");
+    })
+    .unwrap();
+}
+
+/// A Redact annotation with no readable area (neither usable QuadPoints nor a usable Rect) used to
+/// vanish: `marks` skipped it, so applying the readable mark next to it reported success and left
+/// the text under the broken mark in place. Now the whole operation is refused before anything
+/// changes, and the count is of the unreadable marks.
+#[test]
+fn a_mark_with_no_readable_area_is_refused_instead_of_vanishing() {
+    let mut doc = one_page(b"BT /F1 10 Tf 10 100 Td (AB1234CD) Tj ET", "", vec![]);
+    mark(&mut doc, 0, &[[40.0, 95.0, 50.0, 110.0]], "");
+    mark(&mut doc, 0, &[[0.0, 95.0, 10.0, 110.0]], "");
+    for m in marks(&doc) {
+        break_area(&mut doc, m.obj);
+    }
+    mark(&mut doc, 0, &[[20.0, 95.0, 40.0, 110.0]], "");
+    assert_eq!(marks(&doc).len(), 1, "only the readable mark is listed");
+    assert_eq!(apply(&mut doc, None), Err(RedactError::UnreadableMark(2)));
+    // Nothing changed: the readable mark is still pending and its text is still drawn.
+    assert_eq!(marks(&doc).len(), 1);
+    assert!(content(&doc, 0).contains("(AB1234CD) Tj"), "{}", content(&doc, 0));
+}
+
+/// A mark whose QuadPoints can't be used still redacts through its Rect, as it always did.
+#[test]
+fn a_mark_with_only_a_usable_rect_still_redacts() {
+    let mut doc = one_page(b"BT /F1 10 Tf 10 100 Td (AB1234CD) Tj ET", "", vec![]);
+    mark(&mut doc, 0, &[[20.0, 95.0, 40.0, 110.0]], "");
+    let m = marks(&doc).swap_remove(0);
+    doc.update_dict(m.obj, |d| {
+        d.set(b"QuadPoints".to_vec(), Object::name("oops"));
+        d.set(b"Rect".to_vec(), Object::Array(vec![Object::Int(20), Object::Int(95), Object::Int(40), Object::Int(110)]));
+    })
+    .unwrap();
+    assert_eq!(marks(&doc).len(), 1, "the Rect keeps the mark readable");
+    let r = apply(&mut doc, None).unwrap();
+    assert_eq!((r.marks, r.glyphs), (1, 4));
+    assert!(!content(&doc, 0).contains("1234"));
+}
+
+/// Only the pages being applied decide the refusal: an unreadable mark on another page does not
+/// stop the marks of the selected page from being applied.
+#[test]
+fn an_unreadable_mark_on_another_page_does_not_stop_the_selected_page() {
+    let mut doc = pdf(vec![
+        b"<< /Type /Catalog /Pages 2 0 R >>".to_vec(),
+        b"<< /Type /Pages /Kids [3 0 R 7 0 R] /Count 2 >>".to_vec(),
+        b"<< /Type /Page /Parent 2 0 R /MediaBox [0 0 300 300] /Contents 4 0 R /Resources << /Font << /F1 5 0 R >> >> >>".to_vec(),
+        stream("", b"BT /F1 10 Tf 10 100 Td (AB1234CD) Tj ET"),
+        FONT.replace("95 0 R", "6 0 R").into_bytes(),
+        widths(),
+        b"<< /Type /Page /Parent 2 0 R /MediaBox [0 0 300 300] /Contents 8 0 R /Resources << /Font << /F1 5 0 R >> >> >>".to_vec(),
+        stream("", b"BT /F1 10 Tf 10 100 Td (XY) Tj ET"),
+    ]);
+    mark(&mut doc, 1, &[[10.0, 95.0, 30.0, 110.0]], "");
+    let broken = marks(&doc).swap_remove(0);
+    break_area(&mut doc, broken.obj);
+    mark(&mut doc, 0, &[[20.0, 95.0, 40.0, 110.0]], "");
+    assert_eq!(apply(&mut doc, None), Err(RedactError::UnreadableMark(1)));
+    assert_eq!(apply(&mut doc, Some(&[0])).map(|r| r.glyphs), Ok(4));
+    assert!(!content(&doc, 0).contains("1234"));
+}
+
 /// A document with something in every hidden-information category.
 fn hidden_fixture() -> Document {
     let content = b"BT /F1 10 Tf 10 100 Td (Visible) Tj 3 Tr (Hidden) Tj 0 Tr ET BT /F1 10 Tf 500 500 Td (Offpage) Tj ET /OC /L1 BDC BT /F1 10 Tf 10 50 Td (Layer) Tj ET EMC";