fix(xfdf): bound field text, look up names, guard tab cells, protect fields

XFDF import built every full field name from its parent's with no total; it
is held to 64 MiB of names and values. FDF field arrays charge each decoded
value before keeping it instead of after decoding all of them. merge_csv,
XFDF field export and apply_values looked names up by linear scan per field;
they use maps, keeping first-match behaviour. Tab-delimited export guards
formula-like cells with a leading ' as CSV does, and import takes it off.
A same-name comment import no longer replaces a widget whose subtype is a
reference or whose /FT is inherited from its parent, existing annotations
are borrowed instead of deep-copied per import, and an imported redaction
mark's /Rect is grown to hold its /QuadPoints so nothing is removed outside
its review outline.

crates/xfdf/src/common.rs

@@ -8,6 +8,9 @@ use super::{DataError, Dict, Document, Object};
 const MAX_NODES: u32 = 2_000_000;
 
 pub(crate) const MAX_DEPTH: usize = 64;
+/// Most bytes of full field names and values one XFDF import expands to: a long parent name is
+/// written into every descendant's full name, so a small file can name far more than it holds.
+pub(crate) const MAX_XFDF_TEXT: usize = 64 << 20;
 
 /// The deepest element nesting accepted before the text reaches the XML parser.
 ///
@@ -240,7 +243,13 @@ pub(crate) fn xml_name(name: &str) -> String {
     s
 }
 
-pub(crate) fn walk_xfdf(node: roxmltree::Node, prefix: &str, out: &mut Vec<(String, Vec<String>)>, depth: usize) -> Result<(), DataError> {
+pub(crate) fn walk_xfdf(
+    node: roxmltree::Node,
+    prefix: &str,
+    out: &mut Vec<(String, Vec<String>)>,
+    depth: usize,
+    bytes: &mut usize,
+) -> Result<(), DataError> {
     if depth > MAX_DEPTH {
         return Err(DataError::Malformed(format!("XFDF fields nest more than {MAX_DEPTH} deep")));
     }
@@ -258,9 +267,13 @@ pub(crate) fn walk_xfdf(node: roxmltree::Node, prefix: &str, out: &mut Vec<(Stri
         // leaf holding only `<value-richtext>` is not read yet, so it leaves the field alone.
         let names_kids_or_rich = f.children().any(|c| c.has_tag_name("field") || c.has_tag_name("value-richtext"));
         if !vals.is_empty() || !names_kids_or_rich {
+            *bytes = bytes.saturating_add(full.len()).saturating_add(vals.iter().map(String::len).fold(0, usize::saturating_add));
+            if *bytes > MAX_XFDF_TEXT {
+                return Err(DataError::Malformed(format!("the file expands to more than {} MiB of field names and values", MAX_XFDF_TEXT >> 20)));
+            }
             out.push((full.clone(), vals));
         }
-        walk_xfdf(f, &full, out, depth + 1)?;
+        walk_xfdf(f, &full, out, depth + 1, bytes)?;
     }
     Ok(())
 }

crates/xfdf/src/fdf.rs

@@ -419,16 +419,27 @@ pub(crate) fn walk_fdf(
         let full = if prefix.is_empty() { t } else { format!("{prefix}.{t}") };
         budget.spend(0, full.len())?;
         if let Some(v) = d.get(b"V").map(|v| fdf.resolve(v)) {
-            let vals: Vec<String> = match &*v {
-                Object::String(s) => vec![s.to_text()],
-                Object::Name(n) => vec![pdfcraft_forms::name_text(n)],
+            // Each value is charged as it is decoded, before the next: an array of references to
+            // one long string is not decoded whole and then refused.
+            let mut vals: Vec<String> = Vec::new();
+            let keep = |vals: &mut Vec<String>, s: String, budget: &mut ImportBudget| -> Result<(), DataError> {
+                budget.spend(1, s.len())?;
+                vals.push(s);
+                Ok(())
+            };
+            match &*v {
+                Object::String(s) => keep(&mut vals, s.to_text(), budget)?,
+                Object::Name(n) => keep(&mut vals, pdfcraft_forms::name_text(n), budget)?,
                 Object::Array(a) => {
                     budget.spend(a.len(), 0)?;
-                    a.iter().filter_map(|x| fdf.resolve(x).as_string().map(|s| s.to_text())).collect()
+                    for x in a {
+                        if let Some(s) = fdf.resolve(x).as_string() {
+                            keep(&mut vals, s.to_text(), budget)?;
+                        }
+                    }
                 }
-                _ => Vec::new(),
-            };
-            budget.spend(vals.len(), vals.iter().map(String::len).fold(0, usize::saturating_add))?;
+                _ => {}
+            }
             out.push((full.clone(), vals));
         }
         if let Some(kids) = d.get(b"Kids").map(|k| fdf.resolve(k))

crates/xfdf/src/lib.rs

@@ -124,7 +124,11 @@ pub fn export_data(doc: &Document, format: Format) -> String {
             )
         }
         _ => {
-            let clean = |s: &str| s.replace(['\t', '\n', '\r'], " ");
+            // A formula-like cell gets a leading `'`, as in CSV; `tab_delimited` takes it off.
+            let clean = |s: &str| {
+                let s = if csv_guarded(s) { format!("'{s}") } else { s.to_string() };
+                s.replace(['\t', '\n', '\r'], " ")
+            };
             format!(
                 "{}\n{}\n",
                 fields.iter().map(|f| clean(&f.0)).collect::<Vec<_>>().join("\t"),
@@ -171,7 +175,7 @@ pub fn data_values(bytes: &[u8]) -> Result<Vec<(String, Vec<String>)>, DataError
     }
     let mut values = Vec::new();
     if let Some(fields) = root.children().find(|c| c.has_tag_name("fields")) {
-        walk_xfdf(fields, "", &mut values, 0)?;
+        walk_xfdf(fields, "", &mut values, 0, &mut 0)?;
     }
     Ok(values)
 }
@@ -184,9 +188,10 @@ pub fn data_values(bytes: &[u8]) -> Result<Vec<(String, Vec<String>)>, DataError
 /// is written with a leading `'`, which keeps it text; a plain number such as `-12.5` is not.
 pub fn merge_csv(files: &[Vec<(String, Vec<String>)>]) -> String {
     let mut columns: Vec<&str> = Vec::new();
+    let mut known: HashSet<&str> = HashSet::new();
     for f in files {
         for (k, _) in f {
-            if !columns.contains(&k.as_str()) {
+            if known.insert(k.as_str()) {
                 columns.push(k);
             }
         }
@@ -199,7 +204,12 @@ pub fn merge_csv(files: &[Vec<(String, Vec<String>)>]) -> String {
     let mut out = columns.iter().map(|c| q(c)).collect::<Vec<_>>().join(",");
     out.push('\n');
     for f in files {
-        let row: Vec<String> = columns.iter().map(|c| f.iter().find(|(k, _)| k == c).map_or(String::new(), |(_, v)| q(&v.join(", ")))).collect();
+        // The first value of a name repeated in one file wins, as the scan it replaces found it.
+        let mut by_name: HashMap<&str, &Vec<String>> = HashMap::new();
+        for (k, v) in f {
+            by_name.entry(k.as_str()).or_insert(v);
+        }
+        let row: Vec<String> = columns.iter().map(|c| by_name.get(c).map_or(String::new(), |v| q(&v.join(", ")))).collect();
         out.push_str(&row.join(","));
         out.push('\n');
     }
@@ -326,7 +336,7 @@ fn import_table(doc: &mut Document, text: &str) -> Result<Report, DataError> {
 fn tab_delimited(t: &str) -> Result<Vec<(String, Vec<String>)>, DataError> {
     let mut lines = t.lines();
     let (Some(head), Some(row)) = (lines.next(), lines.next()) else { return Err(DataError::UnknownFormat) };
-    Ok(head.split('\t').map(str::to_string).zip(row.split('\t')).map(|(k, v)| (k, vec![v.to_string()])).collect())
+    Ok(head.split('\t').map(|k| csv_text(k.to_string())).zip(row.split('\t')).map(|(k, v)| (k, vec![csv_text(v.to_string())])).collect())
 }
 
 /// Import comments and/or field values from XFDF, FDF, XML, CSV or tab-delimited text

crates/xfdf/src/tests.rs

@@ -2099,3 +2099,80 @@ fn an_fdf_comment_popup_without_a_rect_is_dropped() {
     let popups = page.iter().filter(|a| dst.resolve(a).as_dict().is_some_and(|d| d.name(b"Subtype") == Some(b"Popup".as_slice()))).count();
     assert_eq!(popups, 0);
 }
+
+/// Tab-delimited export guards a formula-like cell as CSV does, and the import takes the guard
+/// off, so a spreadsheet never runs form data and the value survives the round trip.
+#[test]
+fn tab_delimited_export_guards_formula_cells_and_round_trips() {
+    let mut doc = blank();
+    set_value(&mut doc, "Name", &FieldValue::Text("=SUM(A1:A9)".into())).unwrap();
+    let text = export_data(&doc, Format::Txt);
+    let row = text.lines().nth(1).unwrap();
+    assert!(row.starts_with("'=SUM(A1:A9)"), "{row:?}");
+    let mut again = blank();
+    import(&mut again, text.as_bytes()).unwrap();
+    assert_eq!(values(&again).iter().find(|(k, _)| k == "Name").map(|(_, v)| v.clone()), Some(vec!["=SUM(A1:A9)".to_string()]));
+}
+
+/// Merging many files with many distinct names stays linear: every column once, in first-seen
+/// order, and a row looks its names up rather than scanning them.
+#[test]
+fn merge_csv_lists_each_column_once_in_order() {
+    let files: Vec<Vec<(String, Vec<String>)>> =
+        (0..3).map(|f| (0..4).map(|i| (format!("c{}", (i + f) % 5), vec![format!("{f}-{i}")])).collect()).collect();
+    let csv = merge_csv(&files);
+    let mut lines = csv.lines();
+    assert_eq!(lines.next(), Some("c0,c1,c2,c3,c4"));
+    assert_eq!(lines.next(), Some("0-0,0-1,0-2,0-3,"));
+    assert_eq!(lines.next(), Some(",1-0,1-1,1-2,1-3"));
+    assert_eq!(lines.next(), Some("2-3,,2-0,2-1,2-2"));
+}
+
+/// A long parent name is written into every descendant's full name: past the import's text
+/// bound the file is refused rather than expanded.
+#[test]
+fn xfdf_field_names_past_the_text_bound_are_refused() {
+    let parent = "p".repeat(1 << 20);
+    let kids: String = (0..100).map(|i| format!("<field name=\"k{i}\"><value>v</value></field>")).collect();
+    let xfdf =
+        format!("<?xml version=\"1.0\"?><xfdf xmlns=\"http://ns.adobe.com/xfdf/\"><fields><field name=\"{parent}\">{kids}</field></fields></xfdf>");
+    let mut doc = blank();
+    match import(&mut doc, xfdf.as_bytes()) {
+        Err(DataError::Malformed(m)) => assert!(m.contains("64 MiB"), "{m}"),
+        other => panic!("{other:?}"),
+    }
+}
+
+/// A same-name import never replaces a widget whose field type is inherited, or whose subtype is
+/// written as a reference: the field keeps its dictionary and the import goes beside it.
+#[test]
+fn same_name_import_keeps_a_widget_with_an_inherited_field_type() {
+    let mut doc = blank();
+    let page = pdfcraft_model::pages(&doc)[0].obj;
+    // A field with one widget kid: /FT on the field, the widget names it as /Parent.
+    let mut field = Dict::new();
+    field.set(b"FT".to_vec(), Object::name("Tx"));
+    field.set(b"T".to_vec(), Object::String(PdfString::text("inherited")));
+    let field = doc.add(field);
+    let subtype = doc.add(Object::name("Widget"));
+    let mut widget = Dict::new();
+    widget.set(b"Type".to_vec(), Object::name("Annot"));
+    widget.set(b"Subtype".to_vec(), Object::Ref(subtype));
+    widget.set(b"Rect".to_vec(), arr(&[10.0, 10.0, 60.0, 30.0]));
+    widget.set(b"NM".to_vec(), Object::String(PdfString::text("w1")));
+    widget.set(b"Parent".to_vec(), Object::Ref(field));
+    let widget = doc.add(widget);
+    doc.update_dict(field, |d| d.set(b"Kids".to_vec(), Object::Array(vec![Object::Ref(widget)]))).unwrap();
+    doc.update_dict(page, |p| p.set(b"Annots".to_vec(), Object::Array(vec![Object::Ref(widget)]))).unwrap();
+    let mut note = Dict::new();
+    note.set(b"Type".to_vec(), Object::name("Annot"));
+    note.set(b"Subtype".to_vec(), Object::name("Text"));
+    note.set(b"Rect".to_vec(), arr(&[10.0, 10.0, 30.0, 30.0]));
+    note.set(b"NM".to_vec(), Object::String(PdfString::text("w1")));
+    let added = xfdf::place(&mut doc, page, note).unwrap();
+    assert_ne!(added, widget);
+    assert_eq!(doc.get(widget).as_dict().unwrap().name(b"Type"), Some(b"Annot".as_slice()));
+    assert!(doc.get(widget).as_dict().unwrap().reference(b"Parent").is_some(), "the widget was replaced");
+    let annots = doc.get(page).as_dict().unwrap().get(b"Annots").unwrap().as_array().unwrap().len();
+    assert_eq!(annots, 2);
+}

crates/xfdf/src/xfdf.rs

@@ -148,6 +148,8 @@ fn xfdf_fields(doc: &Document, out: &mut String) {
     struct Node {
         values: Vec<String>,
         kids: Vec<(String, Node)>,
+        /// Each child's place in `kids`, by name: a wide level is not scanned per field.
+        index: std::collections::HashMap<String, usize>,
     }
     let mut root = Node::default();
     for f in pdfcraft_forms::fields(doc) {
@@ -162,10 +164,11 @@ fn xfdf_fields(doc: &Document, out: &mut String) {
         // the last level stay together in the deepest element's name, and the import joins them
         // back with periods into the same full name.
         for part in f.name.splitn(MAX_DEPTH, '.') {
-            let i = match node.kids.iter().position(|(k, _)| k == part) {
-                Some(i) => i,
+            let i = match node.index.get(part) {
+                Some(&i) => i,
                 None => {
                     node.kids.push((part.to_string(), Node::default()));
+                    node.index.insert(part.to_string(), node.kids.len() - 1);
                     node.kids.len() - 1
                 }
             };
@@ -214,8 +217,13 @@ pub fn export_xfdf(doc: &Document, comments: bool, fields: bool, file: &str) ->
 /// Set the values of fields named in `values` (full names), counting what took.
 pub(crate) fn apply_values(doc: &mut Document, values: &[(String, Vec<String>)], report: &mut Report) {
     let all = pdfcraft_forms::fields(doc);
+    // The first field of a name, as the scan it replaces found it.
+    let mut by_name: std::collections::HashMap<&str, usize> = std::collections::HashMap::new();
+    for (i, f) in all.iter().enumerate() {
+        by_name.entry(f.name.as_str()).or_insert(i);
+    }
     for (name, vals) in values {
-        let Some(f) = all.iter().find(|f| &f.name == name) else { continue };
+        let Some(f) = by_name.get(name.as_str()).and_then(|&i| all.get(i)) else { continue };
         let first = vals.first().cloned().unwrap_or_default();
         let v = match f.kind {
             FieldKind::Text => FieldValue::Text(first),
@@ -369,7 +377,27 @@ pub(crate) const LOCKED: i64 = 128;
 /// widget or field (replacing it would set the field's value and policy around the form setter).
 fn import_protected(doc: &Document, x: &Dict) -> bool {
     let flags = x.get(b"F").and_then(|f| doc.resolve(f).as_int()).unwrap_or(0);
-    flags & LOCKED != 0 || matches!(x.name(b"Subtype"), Some(b"Redact" | b"Widget")) || x.contains(b"FT")
+    let subtype = x.get(b"Subtype").map(|s| doc.resolve(s));
+    flags & LOCKED != 0 || matches!(subtype.as_deref().and_then(Object::as_name), Some(b"Redact" | b"Widget")) || is_field(doc, x)
+}
+
+/// Whether `x` is a form field or a widget of one: it has `/FT`, or a `/Parent` up the field
+/// tree does (a widget inherits its field's type).
+fn is_field(doc: &Document, x: &Dict) -> bool {
+    if x.contains(b"FT") {
+        return true;
+    }
+    let mut parent = x.reference(b"Parent");
+    let mut seen = std::collections::HashSet::new();
+    while let Some(r) = parent.filter(|r| seen.len() < 32 && seen.insert(*r)) {
+        let o = doc.get(r);
+        let Some(d) = o.as_dict() else { return false };
+        if d.contains(b"FT") {
+            return true;
+        }
+        parent = d.reference(b"Parent");
+    }
+    false
 }
 
 pub(crate) fn place(doc: &mut Document, page_ref: ObjRef, mut d: Dict) -> Result<ObjRef, DataError> {
@@ -389,7 +417,19 @@ pub(crate) fn place(doc: &mut Document, page_ref: ObjRef, mut d: Dict) -> Result
             let (quads, rest) = v.as_chunks::<8>();
             v.len() == q.len() && rest.is_empty() && !quads.is_empty() && quads.iter().all(quad_has_area)
         });
-        if !quads_drawable {
+        if quads_drawable {
+            // The rectangle holds all the mark removes: its review outline is clipped to the
+            // rectangle, so a quad outside it would be removed unseen.
+            let v: Vec<f64> = d.get(b"QuadPoints").and_then(Object::as_array).into_iter().flatten().filter_map(Object::as_f64).collect();
+            let mut r = rect_strict(doc, d.get(b"Rect"))
+                .map_or([f64::INFINITY, f64::INFINITY, f64::NEG_INFINITY, f64::NEG_INFINITY], |[x0, y0, x1, y1]| {
+                    [x0.min(x1), y0.min(y1), x0.max(x1), y0.max(y1)]
+                });
+            for [x, y] in v.as_chunks::<2>().0 {
+                r = [r[0].min(*x), r[1].min(*y), r[2].max(*x), r[3].max(*y)];
+            }
+            d.set(b"Rect".to_vec(), arr(&r));
+        } else {
             // Quads the outline can't draw, or any quad that encloses no area, are replaced by the
             // /Rect's, or dropped when there is no usable /Rect either; such a mark then removes
             // nothing.
@@ -418,9 +458,11 @@ pub(crate) fn place(doc: &mut Document, page_ref: ObjRef, mut d: Dict) -> Result
     // and add the imported dict beside it under no name of its own.
     let mut clash = false;
     for a in annots {
-        let existing = a.as_ref().and_then(|r| doc.get(r).as_dict().cloned());
-        let same = nm.is_some() && existing.as_ref().and_then(|x| x.get(b"NM").and_then(|n| doc.resolve(n).as_string().map(|s| s.to_text()))) == nm;
-        if same && existing.as_ref().is_some_and(|x| import_protected(doc, x)) {
+        // Borrowed: the page's annotations are read once per imported one, never copied.
+        let held = a.as_ref().map(|r| doc.get(r));
+        let existing = held.as_deref().and_then(Object::as_dict);
+        let same = nm.is_some() && existing.and_then(|x| x.get(b"NM").and_then(|n| doc.resolve(n).as_string().map(|s| s.to_text()))) == nm;
+        if same && existing.is_some_and(|x| import_protected(doc, x)) {
             clash = true;
             kept.push(a);
         } else if same {
@@ -434,7 +476,8 @@ pub(crate) fn place(doc: &mut Document, page_ref: ObjRef, mut d: Dict) -> Result
     }
     // The replaced note's pop-up goes with it; otherwise it stays on the page pointing at a note that is no longer there.
     kept.retain(|a| {
-        let Some(x) = a.as_ref().map(|r| doc.get(r)).and_then(|o| o.as_dict().cloned()) else { return true };
+        let Some(held) = a.as_ref().map(|r| doc.get(r)) else { return true };
+        let Some(x) = held.as_dict() else { return true };
         x.name(b"Subtype") != Some(b"Popup".as_slice()) || x.reference(b"Parent").is_none_or(|p| !removed.contains(&p))
     });
     // Keep the replaced comment's identity: replies and review statuses not included in a
@@ -516,7 +559,7 @@ pub(crate) fn import_xfdf(doc: &mut Document, text: &str) -> Result<Report, Data
     // Fields.
     if let Some(fields) = root.children().find(|c| c.has_tag_name("fields")) {
         let mut values = Vec::new();
-        walk_xfdf(fields, "", &mut values, 0)?;
+        walk_xfdf(fields, "", &mut values, 0, &mut 0)?;
         apply_values(doc, &values, &mut report);
     }
     Ok(report)