From c829a1ad44cb4128d68b71895238ae1e8941219d Mon Sep 17 00:00:00 2001 From: ffishman <20211677+ffishman@users.noreply.github.com> Date: Thu, 22 Oct 2026 17:03:37 -0300 Subject: [PATCH] fix(sign): a removed annotation is not hidden behind an added signature Review follow-up: an indirect /Annots array that drops a signed annotation while gaining a signature widget was judged by the widget alone, so a form-fill certification let a signed comment be deleted. A removed comment is now a comment change, and a removed widget is a change no permission covers. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/sign/src/pdf.rs | 17 ++++++++++++++--- crates/sign/tests/it/pdf.rs | 24 ++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/crates/sign/src/pdf.rs b/crates/sign/src/pdf.rs index cd555a989..0cd58d7f9 100644 --- a/crates/sign/src/pdf.rs +++ b/crates/sign/src/pdf.rs @@ -1338,12 +1338,23 @@ fn classify_changes(doc: &Document, old: Option, p: Option) -> Mod (Some(k), _) => k, // The Document Security Store's own arrays and dictionaries, when they only grew. (_, _) if dss.contains(&num) => kind::SECURITY_STORE, - // A signed page's indirect /Annots array, still an array: what it gained. + // A signed page's indirect /Annots array, still an array: what it lost, then what it + // gained. Dropping a signed annotation is never hidden behind an added signature. (_, Object::Array(now)) if signed_annots.contains(&r) && before.as_deref().is_some_and(|b| b.as_array().is_some()) => { let was: HashSet = before.as_deref().and_then(Object::as_array).map(|a| a.iter().filter_map(Object::as_ref).collect()).unwrap_or_default(); - let added: Vec = now.iter().filter_map(Object::as_ref).filter(|r| !was.contains(r)).collect(); - added_annots(doc, &added) + let kept: HashSet = now.iter().filter_map(Object::as_ref).collect(); + let removed: Vec = was.iter().filter(|r| !kept.contains(r)).copied().collect(); + if removed.is_empty() { + let added: Vec = now.iter().filter_map(Object::as_ref).filter(|r| !was.contains(r)).collect(); + added_annots(doc, &added) + } else if removed.iter().any(|r| old.get(*r).as_dict().is_some_and(|d| d.name(b"Subtype") == Some(b"Widget"))) { + // A form field or signature widget taken off the page: no permission covers it. + kind::OTHER + } else { + // Deleting comments is a comment change (permitted at P 3 only). + kind::COMMENTS + } } (_, Object::Dict(d)) => { let was = before.as_deref().and_then(Object::as_dict); diff --git a/crates/sign/tests/it/pdf.rs b/crates/sign/tests/it/pdf.rs index cd0b713a8..b3a025fc2 100644 --- a/crates/sign/tests/it/pdf.rs +++ b/crates/sign/tests/it/pdf.rs @@ -178,6 +178,30 @@ fn an_indirect_annots_array_is_judged_by_the_annotations_it_gains() { let certified = pdfcraft_sign::sign(&open(&unsigned), &id, &SignOptions { certify: Some(2), ..opts() }).unwrap(); let s = check(&edit_after(&certified, comment)); assert_eq!((s.status, s.modification), (Status::Invalid, Modification::Disallowed(vec![kind::COMMENTS]))); + // Under that certification, a signed comment swapped for a new signature widget is still a + // deleted comment, and a form field taken off the page is permitted by no level. + let commented = pdfcraft_sign::sign(&open(&edit_after(&unsigned, comment)), &id, &SignOptions { certify: Some(2), ..opts() }).unwrap(); + let swap = |drop: &dyn Fn(&Document, &Object) -> bool| { + edit_after(&commented, |doc| { + let mut w = pdfcraft_cos::Dict::new(); + w.set(b"Type".to_vec(), Object::name("Annot")); + w.set(b"Subtype".to_vec(), Object::name("Widget")); + w.set(b"FT".to_vec(), Object::name("Sig")); + w.set(b"T".to_vec(), PdfString::text("Later")); + w.set(b"Rect".to_vec(), Object::Array(vec![Object::Int(0); 4])); + let w = doc.add(Object::Dict(w)); + let mut a = doc.get(annots).as_array().cloned().unwrap(); + a.retain(|o| !drop(doc, o)); + a.push(Object::Ref(w)); + doc.set(annots, Object::Array(a)); + }) + }; + let subtype = + |doc: &Document, o: &Object, t: &[u8]| o.as_ref().is_some_and(|r| doc.get(r).as_dict().is_some_and(|d| d.name(b"Subtype") == Some(t))); + let s = check(&swap(&|doc, o| subtype(doc, o, b"Text"))); + assert_eq!((s.status, s.modification), (Status::Invalid, Modification::Disallowed(vec![kind::COMMENTS]))); + let s = check(&swap(&|doc, o| subtype(doc, o, b"Widget") && o.as_ref().is_some_and(|r| r.num == 6))); + assert_eq!((s.status, s.modification), (Status::Invalid, Modification::Disallowed(vec![kind::OTHER]))); } /// Issue #1328: a signature field whose /V is null, directly or through a reference, is unsigned: