fix(redact,sign,cos): close high-severity findings from the agent scan

Redact: refuse Redact marks written in place; remove replies and pop-ups listed on
other pages; delete form fields by object, not name; resolve Do by its last operand
like the renderer; sanitize drops PCAdded and flattens widgets before hidden-content
cleanup. Sign: freed signed objects and edits to an already-signed widget are
modifications. Cos: only reading trusts /Type /XRef to skip encryption.

crates/content/src/lib.rs

@@ -52,6 +52,12 @@ impl Op {
     pub fn name(&self, i: usize) -> Option<&[u8]> {
         self.operands.get(i).and_then(Object::as_name)
     }
+
+    /// The last operand as a name: the one `Do` and the other one-operand operators act on, since
+    /// the renderer takes operands from the end and ignores any surplus before them.
+    pub fn last_name(&self) -> Option<&[u8]> {
+        self.operands.last().and_then(Object::as_name)
+    }
 }
 
 /// A parsed stream.

crates/cos/src/security.rs

@@ -178,7 +178,10 @@ fn walk(o: &Object, strings: &dyn Fn(&PdfString) -> PdfString, h: &SecurityHandl
         Object::Stream(s) => {
             let mut dict: Dict = s.dict.iter().map(|(k, v)| (k.clone(), walk(v, strings, h, num, generation, pass))).collect();
             let ty = s.dict.name(b"Type");
-            if ty == Some(b"XRef") || (ty == Some(b"Metadata") && !h.encrypts_metadata()) {
+            // Cross-reference streams are never encrypted (§7.5.8), but only reading may trust the
+            // label: a stream a producer calls /XRef stays plaintext if writing believes it, and the
+            // writer builds its own cross-reference streams outside this transform.
+            if (decrypt && ty == Some(b"XRef")) || (ty == Some(b"Metadata") && !h.encrypts_metadata()) {
                 return Object::Stream(Stream { dict, raw: s.raw.clone() });
             }
             let mut named = named_crypt_filter(&s.dict);

crates/cos/tests/it/encryption.rs

@@ -477,3 +477,20 @@ fn removing_an_inline_encrypt_dictionary_leaves_an_unencrypted_file() {
         assert!(hayro_syntax::Pdf::new(plain).is_ok(), "{alg:?}");
     }
 }
+
+/// A content stream that calls itself `/Type /XRef` is still encrypted: only reading trusts the label.
+#[test]
+fn a_stream_labelled_xref_is_encrypted_when_saved() {
+    let mut doc = Document::open(Arc::new(fixture_with(" /Type /XRef"))).unwrap();
+    doc.set_encryption(&NewEncryption {
+        algorithm: Algorithm::Aes256,
+        user_password: "u",
+        owner_password: "o",
+        permissions: -4,
+        encrypt_metadata: true,
+        seed: [9; 32],
+    })
+    .unwrap();
+    let bytes = write_full(&doc, &SaveOptions { object_streams: false, ..SaveOptions::default() }).unwrap();
+    assert!(!bytes.windows(SECRET.len()).any(|w| w == SECRET.as_bytes()), "the labelled stream was written in the clear");
+}

crates/forms/src/author.rs

@@ -1239,8 +1239,17 @@ fn rename_in_calculations(doc: &mut Document, all: &[Field], old: &str, new: &st
 
 /// Delete a field: its widgets leave their pages and the field leaves the form.
 pub fn delete_field(doc: &mut Document, name: &str) -> Result<(), FormError> {
-    let all = fields(doc);
-    let f = all.iter().find(|f| f.name == name).ok_or_else(|| FormError::NoSuchField(name.into()))?.clone();
+    let f = fields(doc).into_iter().find(|f| f.name == name).ok_or_else(|| FormError::NoSuchField(name.into()))?;
+    delete_found_field(doc, f)
+}
+
+/// [`delete_field`] for the field object `obj`, when separate fields share one name.
+pub fn delete_field_object(doc: &mut Document, obj: ObjRef) -> Result<(), FormError> {
+    let f = fields(doc).into_iter().find(|f| f.obj == obj).ok_or_else(|| FormError::NoSuchField(format!("{obj:?}")))?;
+    delete_found_field(doc, f)
+}
+
+fn delete_found_field(doc: &mut Document, f: Field) -> Result<(), FormError> {
     let widgets: std::collections::HashSet<ObjRef> = f.widgets.iter().map(|w| w.obj).collect();
     for p in page_refs(doc) {
         let Some(a) = doc.get(p).as_dict().and_then(|d| d.get(b"Annots").cloned()) else { continue };

crates/forms/src/lib.rs

@@ -26,8 +26,8 @@ mod scripting;
 pub mod table;
 pub use actions::{FieldAction, Trigger, field_actions, set_field_actions};
 pub use author::{
-    BorderStyle, CheckStyle, FieldFont, FieldProps, Look, LookPatch, MIN_FIELD_SIDE, NewField, add_field, check_style, delete_field, duplicate_field,
-    look, redraw_field, set_button_icon, set_props,
+    BorderStyle, CheckStyle, FieldFont, FieldProps, Look, LookPatch, MIN_FIELD_SIDE, NewField, add_field, check_style, delete_field,
+    delete_field_object, duplicate_field, look, redraw_field, set_button_icon, set_props,
 };
 pub use calc_order::{CalculatedField, CalculationIssue, CalculationOrder, calculation_order, set_calculation_order};
 pub use scripting::{

crates/redact/src/interp.rs

@@ -415,7 +415,7 @@ pub(crate) fn prune(xo: &mut Dict, replaced: &[Vec<u8>], content: &[Vec<u8>]) {
     }
     let joined = Pieces::join(content);
     let ops = joined.parse().ops;
-    let still_drawn = |n: &[u8]| ops.iter().any(|op| op.is("Do") && op.name(0) == Some(n));
+    let still_drawn = |n: &[u8]| ops.iter().any(|op| op.is("Do") && op.last_name() == Some(n));
     for n in replaced {
         if !still_drawn(n) {
             xo.remove(n);
@@ -430,7 +430,7 @@ fn content_draws(doc: &Document, content: &[Vec<u8>], xo: &Dict, name: &[u8], de
     let joined = Pieces::join(content);
     let ops = joined.parse().ops;
     for op in ops.iter().filter(|op| op.is("Do")) {
-        let Some(n) = op.name(0) else { continue };
+        let Some(n) = op.last_name() else { continue };
         if n == name {
             return Some(true);
         }
@@ -672,7 +672,7 @@ fn resource_ops(doc: &Document, res: &Dict, content: &[Vec<u8>]) -> Result<Vec<O
     while cursor < ops.len() {
         let op = &ops[cursor];
         if op.is("Do")
-            && let Some(r) = op.name(0).and_then(|n| xobjects.get(n)).and_then(Object::as_ref)
+            && let Some(r) = op.last_name().and_then(|n| xobjects.get(n)).and_then(Object::as_ref)
             && seen.insert(r)
             && let Object::Stream(s) = &*doc.get(r)
             && s.dict.name(b"Subtype") == Some(b"Form")
@@ -717,7 +717,7 @@ fn prune_form_resources(doc: &Document, res: &mut Dict, content: &[Vec<u8>]) ->
     let mut xo = res_dict(doc, res, b"XObject");
     let names: Vec<Vec<u8>> = xo.iter().map(|(n, _)| n.to_vec()).collect();
     for name in names {
-        if !ops.iter().any(|op| op.is("Do") && op.name(0) == Some(&name)) {
+        if !ops.iter().any(|op| op.is("Do") && op.last_name() == Some(&name)) {
             xo.remove(&name);
         }
     }
@@ -1204,7 +1204,7 @@ fn xobject(
     xobjects: &mut Dict,
     out: &mut Output,
 ) -> Option<Option<Op>> {
-    let name = op.name(0)?.to_vec();
+    let name = op.last_name()?.to_vec();
     let r = xobjects.get(&name)?.as_ref()?;
     let obj = doc.get(r);
     let Object::Stream(s) = &*obj else { return None };

crates/redact/src/lib.rs

@@ -185,7 +185,14 @@ 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 };
+            let Some(r) = a.as_ref() else {
+                // A mark written in place has no object to find, apply or remove: refuse it
+                // rather than apply the others as if it were not there.
+                if doc.resolve(&a).as_dict().is_some_and(|d| d.name(b"Subtype") == Some(b"Redact")) {
+                    unreadable.push(pi);
+                }
+                continue;
+            };
             let obj = doc.get(r);
             let Some(d) = obj.as_dict() else { continue };
             if d.name(b"Subtype") != Some(b"Redact") {
@@ -400,13 +407,14 @@ fn apply_to(doc: &mut Document, pages: Option<&[usize]>) -> Result<Report, Redac
         .collect::<Result<Vec<_>, _>>()?;
     report.pages = by_page.len();
     localize_resources(doc, &by_page)?;
-    let mut doomed_fields: Vec<String> = Vec::new();
+    let mut doomed_fields: Vec<ObjRef> = Vec::new();
     // The original image and form XObjects the rewrite drops or replaces, on any page.
     let mut doomed_xobjects: Vec<ObjRef> = Vec::new();
     let mut retired_properties = Vec::new();
-    let widget_owner: Vec<(ObjRef, String)> =
-        pdfcraft_forms::fields(doc).into_iter().flat_map(|f| f.widgets.iter().map(|w| (w.obj, f.name.clone())).collect::<Vec<_>>()).collect();
+    let widget_owner: Vec<(ObjRef, ObjRef)> =
+        pdfcraft_forms::fields(doc).into_iter().flat_map(|f| f.widgets.iter().map(|w| (w.obj, f.obj)).collect::<Vec<_>>()).collect();
     let listed_before = listed_annotations(doc);
+    let mut removed_everywhere: HashSet<ObjRef> = HashSet::new();
 
     for &(pi, page_marks) in &groups {
         let page = pdfcraft_model::pages(doc).swap_remove(pi);
@@ -521,10 +529,10 @@ fn apply_to(doc: &mut Document, pages: Option<&[usize]>) -> Result<Report, Redac
             if rect_of(doc, d.get(b"Rect")).is_some_and(|b| rects.iter().any(|x| pdfcraft_content::overlaps(*x, b, 0.0))) {
                 removed.push(r);
                 if subtype == b"Widget" {
-                    if let Some((_, name)) = widget_owner.iter().find(|(w, _)| *w == r)
-                        && !doomed_fields.contains(name)
+                    if let Some((_, field)) = widget_owner.iter().find(|(w, _)| *w == r)
+                        && !doomed_fields.contains(field)
                     {
-                        doomed_fields.push(name.clone());
+                        doomed_fields.push(*field);
                     }
                 } else {
                     report.annotations += 1;
@@ -557,6 +565,7 @@ fn apply_to(doc: &mut Document, pages: Option<&[usize]>) -> Result<Report, Redac
                 break;
             }
         }
+        removed_everywhere.extend(removed.iter().copied());
         let kept: Vec<Object> = annots
             .into_iter()
             .filter(|a| {
@@ -578,10 +587,56 @@ fn apply_to(doc: &mut Document, pages: Option<&[usize]>) -> Result<Report, Redac
         })?;
     }
 
+    // 2b. Replies and pop-ups of a removed annotation can be listed on another page than their
+    // parent: their `/IRT` or `/Parent` would keep the removed comment reachable.
+    loop {
+        let mut grew = false;
+        for p in pdfcraft_model::pages(doc) {
+            let annots = annots_of(doc, &p.dict);
+            let mut kept = Vec::with_capacity(annots.len());
+            for a in &annots {
+                let Some(r) = a.as_ref() else {
+                    kept.push(a.clone());
+                    continue;
+                };
+                let obj = doc.get(r);
+                let Some(d) = obj.as_dict() else {
+                    kept.push(a.clone());
+                    continue;
+                };
+                let is_reply =
+                    d.get(b"IRT").and_then(Object::as_ref).is_some_and(|x| removed_everywhere.contains(&x)) && d.name(b"Subtype") != Some(b"Widget");
+                let is_popup = d.get(b"Parent").and_then(Object::as_ref).is_some_and(|x| removed_everywhere.contains(&x));
+                if removed_everywhere.contains(&r) || is_reply || is_popup {
+                    if removed_everywhere.insert(r) {
+                        grew = true;
+                        if is_reply {
+                            report.annotations += 1;
+                        }
+                    }
+                } else {
+                    kept.push(a.clone());
+                }
+            }
+            if kept.len() != annots.len() {
+                doc.update_dict(p.obj, |d| {
+                    if kept.is_empty() {
+                        d.remove(b"Annots");
+                    } else {
+                        d.set(b"Annots".to_vec(), Object::Array(kept));
+                    }
+                })?;
+            }
+        }
+        if !grew {
+            break;
+        }
+    }
+
     // 3. Form fields with a widget under a mark (all their widgets go).
-    for name in &doomed_fields {
-        if pdfcraft_forms::fields(doc).iter().any(|f| &f.name == name) {
-            pdfcraft_forms::delete_field(doc, name)?;
+    for field in &doomed_fields {
+        if pdfcraft_forms::fields(doc).iter().any(|f| f.obj == *field) {
+            pdfcraft_forms::delete_field_object(doc, *field)?;
             report.fields += 1;
         }
     }

crates/redact/src/sanitize.rs

@@ -400,6 +400,8 @@ fn content_pass(doc: &mut Document, text: bool, layers: bool, write: bool) -> Re
                 _ => Dict::new(),
             };
             dict.remove(b"Length");
+            // The source text of an added item (Edit ▸ Add content) would keep what was removed.
+            dict.remove(b"PCAdded");
             new_list[i] = Object::Ref(doc.add(Object::Stream(pdfcraft_cos::Stream::flate(dict, &bytes))));
             changed = true;
         }
@@ -505,13 +507,6 @@ pub fn remove_hidden(doc: &mut Document, which: &[Hidden]) -> Result<Vec<(Hidden
     let count = |h: Hidden| counts.iter().find(|c| c.0 == h).map_or(0, |c| c.1);
     let (root, _) = catalog(doc).ok_or(RedactError::NothingToApply)?;
     let mut done = Vec::new();
-    // Content first (it needs the layers and the form still in place).
-    let (text, layers) = (which.contains(&Hidden::HiddenText), which.contains(&Hidden::HiddenLayers));
-    let retained_off_layers = if (text && count(Hidden::HiddenText) > 0) || (layers && count(Hidden::HiddenLayers) > 0) {
-        content_pass(doc, text, layers, true)?.1
-    } else {
-        std::collections::HashSet::new()
-    };
     // Every annotation the pages list now: whatever is no longer listed at the end (flattened
     // widgets included) leaves the structure tree too.
     let listed_before = listed_annotations(doc);
@@ -529,6 +524,14 @@ pub fn remove_hidden(doc: &mut Document, which: &[Hidden]) -> Result<Vec<(Hidden
             d.remove(b"AcroForm");
         })?;
     }
+    // Content next (it needs the layers still in place). Flattened appearances are page content
+    // by now, so hidden text or layers inside them are removed with the rest.
+    let (text, layers) = (which.contains(&Hidden::HiddenText), which.contains(&Hidden::HiddenLayers));
+    let retained_off_layers = if (text && (fields || count(Hidden::HiddenText) > 0)) || (layers && (fields || count(Hidden::HiddenLayers) > 0)) {
+        content_pass(doc, text, layers, true)?.1
+    } else {
+        std::collections::HashSet::new()
+    };
     // Annotations: comments, attachments, links, actions, and the widgets flattening kept (hidden
     // ones, and ones with nothing to draw), which still hold their field's value and parent.
     let drop_annot = |a: &Dict| -> bool {

crates/redact/src/tests.rs

@@ -2762,3 +2762,62 @@ fn a_failure_on_a_later_page_leaves_the_document_unchanged() {
     assert_eq!(after, before);
     assert_eq!(content(&doc, 0), text);
 }
+
+#[test]
+fn a_reply_and_pop_up_on_another_page_go_with_the_removed_comment() {
+    let objs: Vec<Vec<u8>> = vec![
+        b"<< /Type /Catalog /Pages 2 0 R >>".to_vec(),
+        b"<< /Type /Pages /Kids [3 0 R 4 0 R] /Count 2 >>".to_vec(),
+        b"<< /Type /Page /Parent 2 0 R /MediaBox [0 0 300 300] /Contents 5 0 R /Annots [6 0 R] >>".to_vec(),
+        b"<< /Type /Page /Parent 2 0 R /MediaBox [0 0 300 300] /Contents 5 0 R /Annots [7 0 R 8 0 R] >>".to_vec(),
+        stream("", b"BT ET"),
+        b"<< /Type /Annot /Subtype /Text /Rect [20 20 40 40] /Contents (PARENT-NOTE) /Popup 8 0 R >>".to_vec(),
+        b"<< /Type /Annot /Subtype /Text /Rect [250 250 270 270] /IRT 6 0 R /Contents (OTHER-PAGE-REPLY) >>".to_vec(),
+        b"<< /Type /Annot /Subtype /Popup /Rect [200 150 290 190] /Parent 6 0 R >>".to_vec(),
+    ];
+    let mut doc = pdf(objs);
+    mark(&mut doc, 0, &[[10.0, 10.0, 70.0, 50.0]], "");
+    apply(&mut doc, None).unwrap();
+    let second = pdfcraft_model::pages(&doc).swap_remove(1);
+    assert!(annots_of(&doc, &second.dict).is_empty(), "the reply and the pop-up leave page 2");
+    let bytes = write_full(&doc, &SaveOptions { object_streams: false, ..SaveOptions::default() }).unwrap();
+    assert!(!bytes.windows(10).any(|w| w == b"PARENT-NOT"), "the removed comment survived the save");
+}
+
+#[test]
+fn a_redact_mark_written_in_place_is_refused() {
+    let objs: Vec<Vec<u8>> = vec![
+        b"<< /Type /Catalog /Pages 2 0 R >>".to_vec(),
+        b"<< /Type /Pages /Kids [3 0 R] /Count 1 >>".to_vec(),
+        b"<< /Type /Page /Parent 2 0 R /MediaBox [0 0 300 300] /Contents 4 0 R /Annots [<< /Type /Annot /Subtype /Redact /Rect [10 10 70 40] >>] >>"
+            .to_vec(),
+        stream("", b"BT ET"),
+    ];
+    let mut doc = pdf(objs);
+    mark(&mut doc, 0, &[[100.0, 100.0, 120.0, 120.0]], "");
+    assert!(matches!(apply(&mut doc, None), Err(RedactError::UnreadableMark(1))));
+}
+
+#[test]
+fn do_acts_on_the_operand_before_the_operator_like_the_renderer() {
+    // `/Missing /Im Do`: the renderer draws /Im, so redaction must find the image there.
+    let img = stream("/Type /XObject /Subtype /Image /Width 1 /Height 1 /ColorSpace /DeviceGray /BitsPerComponent 8", b"\x00");
+    let mut doc = one_page(b"q 100 0 0 100 10 10 cm /Missing /Im Do Q", "/XObject << /Im 7 0 R >>", vec![img]);
+    mark(&mut doc, 0, &[[0.0, 0.0, 300.0, 300.0]], "");
+    let r = apply(&mut doc, None).unwrap();
+    assert_eq!(r.images_removed, 1, "{r:?}");
+}
+
+#[test]
+fn hidden_text_removal_drops_the_source_text_of_added_content() {
+    let mut doc = one_page(b"BT /F1 12 Tf 3 Tr 20 20 Td (SECRET) Tj ET", "", vec![]);
+    let content_ref = pdfcraft_model::pages(&doc)[0].dict.get(b"Contents").and_then(Object::as_ref).unwrap();
+    doc.update_dict(content_ref, |d| {
+        d.set(b"PCAdded".to_vec(), Object::Dict(Dict::new()));
+    })
+    .unwrap();
+    sanitize::remove_hidden(&mut doc, &[sanitize::Hidden::HiddenText]).unwrap();
+    let p = &pdfcraft_model::pages(&doc)[0];
+    let (list, _) = page_streams(&doc, &p.dict, 0).unwrap();
+    assert!(list.iter().all(|o| doc.resolve(o).as_dict().is_some_and(|d| !d.contains(b"PCAdded"))));
+}

crates/sign/src/pdf.rs

@@ -1464,6 +1464,22 @@ fn classify_changes(doc: &Document, old: Option<Document>, p: Option<u8>) -> Mod
             list.push(what);
         }
     }
+    // An object the signed revision had and this one frees is a change too, and the loop above
+    // only sees objects that still exist. File plumbing (cross-reference and object streams) is
+    // rewritten by every save and is not a change.
+    let current: HashSet<u32> = doc.object_numbers().into_iter().collect();
+    for num in old.object_numbers().into_iter().filter(|n| !current.contains(n)) {
+        let r = ObjRef { num, generation: old.generation(num) };
+        let was = old.get(r);
+        if matches!(&*was, Object::Stream(s) if matches!(s.dict.name(b"Type"), Some(b"XRef" | b"ObjStm"))) {
+            continue;
+        }
+        let what =
+            if old_content.contains(&r) || signed_pages.contains(&r) || signed_resources.contains_key(&r) { kind::PAGE_CONTENT } else { kind::OTHER };
+        if !disallowed.contains(&what) {
+            disallowed.push(what);
+        }
+    }
     if disallowed.is_empty() { Modification::Allowed(allowed) } else { Modification::Disallowed(disallowed) }
 }
 
@@ -1804,6 +1820,11 @@ fn change_kind(doc: &Document, old: &Document, d: &Dict, before: Option<&Dict>)
         _ => {}
     }
     match d.name(b"Subtype") {
+        // A signature widget that already held a signature is what the signature shows: moving
+        // it or swapping its appearance can hide the signed page under it.
+        Some(b"Widget") if before.is_some_and(|b| b.contains(b"V") && b.name(b"FT") == Some(b"Sig")) && before != Some(d) => {
+            return kind::PAGE_CONTENT;
+        }
         Some(b"Widget") => return if d.name(b"FT") == Some(b"Sig") && signature_ok { kind::SIGNATURE } else { kind::FORM_FILL },
         Some(b"Link") => return kind::LINKS,
         Some(_) if ty == Some(b"Annot") || d.contains(b"Rect") => return kind::COMMENTS,

crates/sign/tests/it/pdf.rs

@@ -2576,3 +2576,29 @@ fn a_timestamp_from_a_certificate_not_issued_for_time_stamping_is_not_trusted_ti
     assert_eq!(s.timestamp_time, None, "{:?}", s.details);
     assert!(s.details.iter().any(|d| d.message.contains("not issued for time stamping")), "{:?}", s.details);
 }
+
+/// A signed revision's objects can't be freed or its signature widget rewritten without the
+/// change being seen: the classifier used to look only at objects still present, and let a
+/// signature widget's geometry and appearance change count as a signature.
+#[test]
+fn freeing_a_signed_object_or_rewriting_the_signed_widget_is_a_page_content_change() {
+    let id = pkcs12::open(&data("rsa-aes.p12"), "test").unwrap();
+    let trust = TrustStore { certs: vec![id.certificate.clone()] };
+    let check = |bytes: &[u8]| signatures(&open(bytes), bytes, &trust).into_iter().find(|s| s.signed).unwrap();
+    let signed = pdfcraft_sign::sign(&open(&fixture()), &id, &opts()).unwrap();
+    let freed = check(&edit_after(&signed, |doc| doc.free(pdfcraft_cos::ObjRef { num: 5, generation: 0 })));
+    assert_eq!(freed.status, Status::Invalid, "{:?}", freed.modification);
+    assert!(matches!(&freed.modification, Modification::Disallowed(k) if k.contains(&kind::PAGE_CONTENT)), "{:?}", freed.modification);
+    let widget = open(&signed)
+        .object_numbers()
+        .into_iter()
+        .map(|num| pdfcraft_cos::ObjRef { num, generation: 0 })
+        .find(|r| open(&signed).get(*r).as_dict().is_some_and(|d| d.name(b"Subtype") == Some(b"Widget") && d.contains(b"V")))
+        .unwrap();
+    let moved = check(&edit_after(&signed, |doc| {
+        doc.update_dict(widget, |d| d.set(b"Rect".to_vec(), Object::Array(vec![Object::Int(0), Object::Int(0), Object::Int(300), Object::Int(300)])))
+            .unwrap();
+    }));
+    assert_eq!(moved.status, Status::Invalid, "{:?}", moved.modification);
+    assert!(matches!(&moved.modification, Modification::Disallowed(k) if k.contains(&kind::PAGE_CONTENT)), "{:?}", moved.modification);
+}