fix(sign): a signature field with a null /V can be signed by name (#1328) (#1374)

The signature list counts a field as signed only when its /V resolves
to a dictionary, but signing an existing field refused any field that
had a /V key. A field with `/V null`, or a /V pointing at a null object,
was listed as unsigned and then refused with "the field is already
signed".

Both now ask the field for its signature dictionary through one
predicate. A field with a signature is still refused, and a field
without /V still signs as before.

Adds a_field_with_a_null_value_is_unsigned_and_can_be_signed, which
fails before this change.

crates/sign/src/pdf.rs

@@ -485,6 +485,14 @@ struct Field {
     widgets: Vec<(Option<ObjRef>, Dict)>,
 }
 
+impl Field {
+    /// The signature dictionary in /V. A field without one is unsigned, whether /V is missing,
+    /// null or not a dictionary.
+    fn signature(&self, doc: &Document) -> Option<Dict> {
+        self.dict.get(b"V").map(|v| doc.resolve(v)).and_then(|v| v.as_dict().cloned())
+    }
+}
+
 /// Every signature field, depth first in `/Fields` order.
 fn sig_fields(doc: &Document) -> Vec<Field> {
     let mut out = Vec::new();
@@ -566,7 +574,7 @@ pub fn list_cached(doc: &Document, bytes: &[u8], trust: &TrustStore, cache: &Dig
             .filter(|r| r.len() == 4)
             .map(|r| [r[0].min(r[2]), r[1].min(r[3]), r[0].max(r[2]), r[1].max(r[3])]);
         let visible = rect.is_some_and(|r| r[2] - r[0] > 0.5 && r[3] - r[1] > 0.5);
-        let v = f.dict.get(b"V").map(|v| doc.resolve(v)).and_then(|v| v.as_dict().cloned());
+        let v = f.signature(doc);
         let mut info = SignatureInfo {
             field: f.name.clone(),
             signed: v.is_some(),
@@ -1711,7 +1719,7 @@ fn sign_inner(
     let (widget, rect) = match existing {
         Some(f) => {
             let f = f?;
-            if f.dict.contains(b"V") {
+            if f.signature(&doc).is_some() {
                 return Err(SignError::Pdf(format!("the field {:?} is already signed", f.name)));
             }
             let fr = f.r.ok_or_else(|| SignError::Pdf("the field is not an indirect object".into()))?;

crates/sign/tests/pdf.rs

@@ -132,6 +132,30 @@ fn signing_an_existing_field_and_counter_signing() {
     ));
 }
 
+/// Issue #1328: a signature field whose /V is null, directly or through a reference, is unsigned:
+/// it is listed as unsigned and can be signed by name.
+#[test]
+fn a_field_with_a_null_value_is_unsigned_and_can_be_signed() {
+    let id = pkcs12::open(&data("ec-p256.p12"), "test").unwrap();
+    let field = pdfcraft_cos::ObjRef { num: 6, generation: 0 };
+    let direct = edit_after(&fixture(), |doc| doc.update_dict(field, |d| d.set(b"V".to_vec(), Object::Null)).unwrap());
+    let indirect = edit_after(&fixture(), |doc| {
+        let null = doc.add(Object::Null);
+        doc.update_dict(field, |d| d.set(b"V".to_vec(), Object::Ref(null))).unwrap();
+    });
+    for (what, unsigned) in [("direct null", direct), ("indirect null", indirect)] {
+        let doc = open(&unsigned);
+        assert!(doc.get(field).as_dict().is_some_and(|d| d.contains(b"V")), "{what}: the fixture keeps its /V");
+        let listed = signatures(&doc, &unsigned, &TrustStore::default());
+        assert!(listed.iter().any(|s| s.field == "Approval" && !s.signed), "{what}");
+        let signed =
+            pdfcraft_sign::sign(&doc, &id, &SignOptions { field: Some("Approval".into()), ..opts() }).unwrap_or_else(|e| panic!("{what}: {e:?}"));
+        let listed = signatures(&open(&signed), &signed, &TrustStore::default());
+        assert!(listed.iter().any(|s| s.field == "Approval" && s.signed), "{what}");
+        assert!(!listed.iter().any(|s| s.field == "Signature1"), "{what}: no new field");
+    }
+}
+
 /// Append a revision that changes object `num` with `f`.
 fn edit_after(signed: &[u8], f: impl FnOnce(&mut Document)) -> Vec<u8> {
     let mut doc = open(signed);