From 3f543ca17f516a37ed49d44a72ddcab05e75e852 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Mon, 19 Oct 2026 00:09:17 +0800 Subject: [PATCH 1/6] M8: redaction drops OBJRs of elements reached only through the parent or ID tree (#2447) Follow-up to #2532 (review by aiongg): the structure-tree walk started only at the root's /K, so an element listed only in /ParentTree or /IDTree kept its object reference to a removed link or comment, and the root kept that annotation reachable in a full rewrite. The walk now also starts from both trees and follows their nodes (/Nums, /Names, /Kids), written in place or as their own objects. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Gv2WwdgjmBz33GWkwgNn3Q --- crates/redact/src/tags.rs | 14 +++++++++++++- crates/redact/src/tests.rs | 31 +++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/crates/redact/src/tags.rs b/crates/redact/src/tags.rs index b70cf23ab..7228d9ed6 100644 --- a/crates/redact/src/tags.rs +++ b/crates/redact/src/tags.rs @@ -210,6 +210,12 @@ fn refers_to(doc: &Document, o: &Object, gone: &HashSet) -> bool { .is_some_and(|d| d.name(b"Type") == Some(b"OBJR") && d.get(b"Obj").and_then(Object::as_ref).is_some_and(|r| gone.contains(&r))) } +/// What a number or name tree node leads on to: its `/Nums` or `/Names` array (keys and values +/// alike; the walk ignores numbers and strings) and its `/Kids`. +fn tree_node_entries(d: &Dict) -> Vec { + [&b"Nums"[..], b"Names", b"Kids"].iter().filter_map(|key| d.get(key).cloned()).collect() +} + /// Drop the structure tree's object references (OBJR) to `gone`, annotations removed from their /// pages (#2447). A tagged PDF's tree points at each link and comment it tags; left there, the /// reference keeps the annotation, its URL or text included, in a full rewrite after the page @@ -236,12 +242,16 @@ pub(crate) fn drop_object_refs_within(doc: &mut Document, gone: &HashSet else { return Ok(0); }; - let mut todo: Vec = root.get(b"K").cloned().into_iter().collect(); + // Elements are reached from the root's /K, and also from the parent tree and the ID tree: + // one listed only there (left behind by an editor, say) still points at its annotation. + let mut todo: Vec = [&b"K"[..], b"ParentTree", b"IDTree"].iter().filter_map(|key| root.get(key).cloned()).collect(); let mut seen = HashSet::new(); let mut hits: Vec = Vec::new(); while let Some(k) = todo.pop() { match k { Object::Array(a) => todo.extend(a), + // A number or name tree node written in place (§7.9.6, §7.9.7): its values and kids. + Object::Dict(d) => todo.extend(tree_node_entries(&d)), Object::Ref(r) => { if !seen.insert(r) { continue; @@ -261,6 +271,8 @@ pub(crate) fn drop_object_refs_within(doc: &mut Document, gone: &HashSet } let Some(d) = object.as_dict() else { continue }; if !d.contains(b"S") { + // A parent tree or ID tree node: its values may be elements. + todo.extend(tree_node_entries(d)); continue; } let kids: Vec = match d.get(b"K").map(|k| doc.resolve(k)) { diff --git a/crates/redact/src/tests.rs b/crates/redact/src/tests.rs index a7e11bcaf..47b4d19da 100644 --- a/crates/redact/src/tests.rs +++ b/crates/redact/src/tests.rs @@ -842,6 +842,37 @@ fn a_structure_tree_too_large_to_walk_is_removed_whole() { assert!(!bytes.windows(14).any(|w| w == b"secret.example"), "the removed link can't be reached any more"); } +/// #2532 follow-up: an element reached only through the parent tree or the ID tree, not from +/// the root's /K, still points at a removed annotation, and the root keeps it reachable in a full +/// rewrite. Both trees are walked too, written in place or as their own objects. +#[test] +fn elements_listed_only_in_the_parent_tree_or_the_id_tree_lose_their_references() { + let objs: Vec> = vec![ + b"<< /Type /Catalog /Pages 2 0 R /MarkInfo << /Marked true >> /StructTreeRoot 5 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] >>".to_vec(), + // 4: the removed link; 8: the removed comment + b"<< /Type /Annot /Subtype /Link /Rect [0 0 10 10] /A << /S /URI /URI (https://secret.example) >> >>".to_vec(), + // 5: the root; its /K reaches only the empty document element 6. The parent tree is + // written in place, the ID tree's kid (9) as its own object. + b"<< /Type /StructTreeRoot /K 6 0 R /ParentTree << /Nums [0 7 0 R] >> /IDTree << /Kids [9 0 R] >> >>".to_vec(), + b"<< /Type /StructElem /S /Document /P 5 0 R >>".to_vec(), + // 7: a link element only the parent tree lists + b"<< /Type /StructElem /S /Link /P 6 0 R /K << /Type /OBJR /Obj 4 0 R >> /Alt (secret.example link) >>".to_vec(), + b"<< /Type /Annot /Subtype /Text /Rect [0 0 10 10] /Contents (hushhush note) >>".to_vec(), + b"<< /Names [(note1) 10 0 R] >>".to_vec(), + // 10: an annotation element only the ID tree lists + b"<< /Type /StructElem /S /Annot /P 6 0 R /ID (note1) /K [<< /Type /OBJR /Obj 8 0 R >>] >>".to_vec(), + ]; + let gone: std::collections::HashSet = [ObjRef::new(4, 0), ObjRef::new(8, 0)].into(); + let mut doc = pdf(objs); + assert_eq!(crate::tags::drop_object_refs(&mut doc, &gone).unwrap(), 2, "both references go"); + let bytes = write_full(&doc, &SaveOptions { object_streams: false, ..SaveOptions::default() }).unwrap(); + for secret in [&b"secret.example"[..], b"hushhush"] { + assert!(!bytes.windows(secret.len()).any(|w| w == secret), "{} survived the rewrite", String::from_utf8_lossy(secret)); + } +} + /// Issue #1325: removing comments keeps the annotations that are not comments: the multimedia /// and print-production ones (Watermark, PrinterMark, TrapNet, Screen, Movie, RichMedia, 3D). #[test] From e6de108972faceb0e799b7a5310d740c6311780e Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Mon, 19 Oct 2026 00:44:58 +0800 Subject: [PATCH 2/6] M8: note that structure elements written in place keep their references (#2447) Review note from Losminek on #3285. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Gv2WwdgjmBz33GWkwgNn3Q --- crates/redact/src/tags.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/redact/src/tags.rs b/crates/redact/src/tags.rs index 7228d9ed6..2923061b0 100644 --- a/crates/redact/src/tags.rs +++ b/crates/redact/src/tags.rs @@ -220,6 +220,10 @@ fn tree_node_entries(d: &Dict) -> Vec { /// pages (#2447). A tagged PDF's tree points at each link and comment it tags; left there, the /// reference keeps the annotation, its URL or text included, in a full rewrite after the page /// no longer lists it. Returns how many references were dropped. +/// +/// Only elements that are objects of their own are edited: a structure element written in place +/// (a direct dictionary in a `/K` array or a tree) keeps its references, since changing it means +/// rewriting the object that holds it. Producers rarely write elements that way. pub(crate) fn drop_object_refs(doc: &mut Document, gone: &HashSet) -> Result { drop_object_refs_within(doc, gone, MAX_STRUCT_REFS) } From ce3c0e46c9831dc1ce2b0c215daa31764b1f3e8c Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Mon, 19 Oct 2026 00:52:18 +0800 Subject: [PATCH 3/6] M8: redaction clears tag text of elements reached only through the parent or ID tree, and fails closed past the walk limit (#3302) tags::clean, which drops /Alt, /ActualText and /E that repeat redacted text, walked the structure tree from the root's /K only, so an element listed only in /ParentTree or /IDTree kept the redacted words. It now starts from both trees too, as drop_object_refs does since 3f543ca1. Past its 2,000,000-reference cap the walk used to skip the rest and report success. It now fails closed: every structure element in the document, found by reading each object rather than through the tree, loses its alternate and actual text. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Gv2WwdgjmBz33GWkwgNn3Q --- crates/redact/src/tags.rs | 54 ++++++++++++++++++++++++++++++++++-- crates/redact/src/tests.rs | 57 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 3 deletions(-) diff --git a/crates/redact/src/tags.rs b/crates/redact/src/tags.rs index 2923061b0..a0883ad6a 100644 --- a/crates/redact/src/tags.rs +++ b/crates/redact/src/tags.rs @@ -103,6 +103,20 @@ pub(crate) fn clean( stream: Option<(ObjRef, ObjRef)>, changed: &HashSet, empty: &HashSet, +) -> Result { + clean_within(doc, page, stream, changed, empty, MAX_STRUCT_REFS) +} + +/// [`clean`], visiting at most `limit` structure-tree references. A tree too large to walk in full +/// could hide an element that repeats the redacted text, so then every element in the document +/// loses its alternate and actual text instead (fail-closed, #3302). +pub(crate) fn clean_within( + doc: &mut Document, + page: ObjRef, + stream: Option<(ObjRef, ObjRef)>, + changed: &HashSet, + empty: &HashSet, + limit: usize, ) -> Result { if changed.is_empty() && stream.is_none() { return Ok(0); @@ -114,17 +128,25 @@ pub(crate) fn clean( else { return Ok(0); }; - // Indirect elements with the page they inherit. - let mut todo: Vec<(Object, Option)> = root.get(b"K").map(|k| vec![(k.clone(), None)]).unwrap_or_default(); + // Indirect elements with the page they inherit. Elements are reached from the root's /K, and + // also from the parent tree and the ID tree: one listed only there still owns marked content + // (#3302). + let mut todo: Vec<(Object, Option)> = + [&b"K"[..], b"ParentTree", b"IDTree"].iter().filter_map(|key| root.get(key).cloned()).map(|k| (k, None)).collect(); let mut seen = HashSet::new(); let mut hits: Vec<(ObjRef, Dict, Option, bool)> = Vec::new(); while let Some((k, pg)) = todo.pop() { match k { Object::Array(a) => todo.extend(a.into_iter().map(|x| (x, pg))), + // A number or name tree node written in place. + Object::Dict(d) if !d.contains(b"S") => todo.extend(tree_node_entries(&d).into_iter().map(|x| (x, pg))), Object::Ref(r) => { - if !seen.insert(r) || seen.len() > 2_000_000 { + if !seen.insert(r) { continue; } + if seen.len() > limit { + return clear_every_element(doc); + } let object = doc.get(r); if let Object::Array(a) = &*object { todo.extend(a.iter().cloned().map(|x| (x, pg))); @@ -132,6 +154,8 @@ pub(crate) fn clean( } let Some(d) = object.as_dict().cloned() else { continue }; if !d.contains(b"S") { + // A parent tree or ID tree node: its values may be elements. + todo.extend(tree_node_entries(&d).into_iter().map(|x| (x, pg))); continue; } let pg = d.get(b"Pg").and_then(Object::as_ref).or(pg); @@ -203,6 +227,30 @@ pub(crate) fn clean( Ok(n) } +/// The fail-closed end of a walk that couldn't finish: every structure element in the document, +/// found by reading each object rather than through the tree, loses the text that could repeat +/// redacted words. Returns how many elements changed. +fn clear_every_element(doc: &mut Document) -> Result { + let keys = [&b"Alt"[..], b"ActualText", b"E"]; + let elements: Vec = doc + .scan_objects() + .filter(|(_, o)| { + o.as_dict().is_some_and(|d| { + (d.name(b"Type") == Some(b"StructElem") || (d.contains(b"S") && d.contains(b"P"))) && keys.iter().any(|k| d.contains(k)) + }) + }) + .map(|(r, _)| r) + .collect(); + for &r in &elements { + doc.update_dict(r, |e| { + for key in keys { + e.remove(key); + } + })?; + } + Ok(elements.len()) +} + /// Whether `o` is an object reference (`/Type /OBJR`, direct or indirect) to one of `gone`. fn refers_to(doc: &Document, o: &Object, gone: &HashSet) -> bool { doc.resolve(o) diff --git a/crates/redact/src/tests.rs b/crates/redact/src/tests.rs index 47b4d19da..f25ac197a 100644 --- a/crates/redact/src/tests.rs +++ b/crates/redact/src/tests.rs @@ -842,6 +842,63 @@ fn a_structure_tree_too_large_to_walk_is_removed_whole() { assert!(!bytes.windows(14).any(|w| w == b"secret.example"), "the removed link can't be reached any more"); } +/// #3302: an element listed only in the parent tree (not under the root's /K) still owns the +/// marked content redaction changed, so its /ActualText and /Alt, which repeat the redacted +/// words, go as they do for an element under /K. +#[test] +fn an_element_listed_only_in_the_parent_tree_loses_its_redacted_text() { + let objs: Vec> = vec![ + b"<< /Type /Catalog /Pages 2 0 R /StructTreeRoot 5 0 R /MarkInfo << /Marked true >> >>".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 /StructParents 0 /Resources << /Font << /F1 9 0 R >> >> >>".to_vec(), + stream("", b"/P <> BDC BT /F1 14 Tf 20 150 Td (Codename Bluefin) Tj ET EMC"), + // The root's /K reaches only the empty document element 6; the paragraph 7 is in the + // parent tree only. + b"<< /Type /StructTreeRoot /K 6 0 R /ParentTree << /Nums [0 [7 0 R]] >> /ParentTreeNextKey 1 >>".to_vec(), + b"<< /Type /StructElem /S /Document /P 5 0 R >>".to_vec(), + b"<< /Type /StructElem /S /P /P 6 0 R /Pg 3 0 R /K 0 /ActualText (Codename Bluefin) /Alt (Codename Bluefin) >>".to_vec(), + b"<< /Type /StructElem /S /Note /P 6 0 R >>".to_vec(), + b"<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>".to_vec(), + ]; + let mut doc = pdf(objs); + mark(&mut doc, 0, &[[10.0, 140.0, 290.0, 170.0]], ""); + let r = apply(&mut doc, None).unwrap(); + assert!(r.glyphs > 0 && r.tags >= 1, "{r:?}"); + let element = doc.get(ObjRef::new(7, 0)).as_dict().cloned().unwrap(); + assert!(!element.contains(b"ActualText") && !element.contains(b"Alt"), "{element:?}"); +} + +/// #3302: a structure tree too large to walk in full fails closed: every element loses its +/// alternate and actual text, found by reading each object, instead of the walk stopping and +/// reporting success with elements never checked. +#[test] +fn a_tag_walk_cut_short_clears_every_element() { + let objs: Vec> = vec![ + b"<< /Type /Catalog /Pages 2 0 R /StructTreeRoot 5 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] /StructParents 0 >>".to_vec(), + b"<< >>".to_vec(), + b"<< /Type /StructTreeRoot /K 6 0 R >>".to_vec(), + b"<< /Type /StructElem /S /Document /P 5 0 R /K [7 0 R 8 0 R] >>".to_vec(), + b"<< /Type /StructElem /S /P /P 6 0 R /Pg 3 0 R /K 0 /ActualText (Codename Bluefin) >>".to_vec(), + b"<< /Type /StructElem /S /Figure /P 6 0 R /Alt (A chart) /E (chart) >>".to_vec(), + ]; + let page = ObjRef::new(3, 0); + let changed: std::collections::HashSet = [0].into(); + let mut doc = pdf(objs.clone()); + crate::tags::clean_within(&mut doc, page, None, &changed, &Default::default(), 100).unwrap(); + let figure = doc.get(ObjRef::new(8, 0)).as_dict().cloned().unwrap(); + assert!(figure.contains(b"Alt"), "a walk that finishes keeps an unrelated element's text"); + + let mut doc = pdf(objs); + let n = crate::tags::clean_within(&mut doc, page, None, &changed, &Default::default(), 1).unwrap(); + assert_eq!(n, 2, "both elements with text change"); + for r in [7, 8] { + let e = doc.get(ObjRef::new(r, 0)).as_dict().cloned().unwrap(); + assert!(!e.contains(b"ActualText") && !e.contains(b"Alt") && !e.contains(b"E"), "{r}: {e:?}"); + } +} + /// #2532 follow-up: an element reached only through the parent tree or the ID tree, not from /// the root's /K, still points at a removed annotation, and the root keeps it reachable in a full /// rewrite. Both trees are walked too, written in place or as their own objects. From f62bf71a7797be38b5d98b0a7a03e0b7d9500a64 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Mon, 19 Oct 2026 01:33:42 +0800 Subject: [PATCH 4/6] M8: tag cleanup keeps inherited pages, fails closed over replaced streams and in-place elements (#3302) Review follow-ups on #3285: walk /K before the parent and ID trees and look up an element's inherited /Pg through /P; drop the structure tree when a cut-short walk could leave MCRs naming a replaced stream; drop it when an element written in place tags a removed annotation. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Gv2WwdgjmBz33GWkwgNn3Q --- crates/redact/src/tags.rs | 71 ++++++++++++++++++++++++++++++------ crates/redact/src/tests.rs | 75 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 12 deletions(-) diff --git a/crates/redact/src/tags.rs b/crates/redact/src/tags.rs index a0883ad6a..2b117fcf4 100644 --- a/crates/redact/src/tags.rs +++ b/crates/redact/src/tags.rs @@ -130,9 +130,10 @@ pub(crate) fn clean_within( }; // Indirect elements with the page they inherit. Elements are reached from the root's /K, and // also from the parent tree and the ID tree: one listed only there still owns marked content - // (#3302). + // (#3302). The stack pops /K first, so an element reached through /K gets the page it inherits + // from its ancestors before a tree reaches it without one. let mut todo: Vec<(Object, Option)> = - [&b"K"[..], b"ParentTree", b"IDTree"].iter().filter_map(|key| root.get(key).cloned()).map(|k| (k, None)).collect(); + [&b"IDTree"[..], b"ParentTree", b"K"].iter().filter_map(|key| root.get(key).cloned()).map(|k| (k, None)).collect(); let mut seen = HashSet::new(); let mut hits: Vec<(ObjRef, Dict, Option, bool)> = Vec::new(); while let Some((k, pg)) = todo.pop() { @@ -145,7 +146,13 @@ pub(crate) fn clean_within( continue; } if seen.len() > limit { - return clear_every_element(doc); + let n = clear_every_element(doc)?; + if stream.is_some() { + // Marked-content references the walk didn't reach still name the + // original stream through /Stm, which a full save would then keep. + drop_tree(doc)?; + } + return Ok(n); } let object = doc.get(r); if let Object::Array(a) = &*object { @@ -158,7 +165,7 @@ pub(crate) fn clean_within( todo.extend(tree_node_entries(&d).into_iter().map(|x| (x, pg))); continue; } - let pg = d.get(b"Pg").and_then(Object::as_ref).or(pg); + let pg = d.get(b"Pg").and_then(Object::as_ref).or(pg).or_else(|| inherited_page(doc, &d)); let kids: Vec = match d.get(b"K").map(|k| doc.resolve(k)) { Some(o) => match &*o { Object::Array(a) => a.clone(), @@ -251,6 +258,33 @@ fn clear_every_element(doc: &mut Document) -> Result Option { + let mut parent = d.get(b"P").and_then(Object::as_ref); + for _ in 0..MAX_PARENTS { + let p = doc.get(parent?); + let p = p.as_dict()?; + if let Some(pg) = p.get(b"Pg").and_then(Object::as_ref) { + return Some(pg); + } + parent = p.get(b"P").and_then(Object::as_ref); + } + None +} + +/// The most ancestors [`inherited_page`] looks through; deeper trees, and cycles, go unanswered. +const MAX_PARENTS: usize = 256; + +/// Remove the structure tree, with `/MarkInfo`, which would claim tags there no longer are. +fn drop_tree(doc: &mut Document) -> Result<(), pdfcraft_cos::CosError> { + let Some(catalog) = doc.root() else { return Ok(()) }; + doc.update_dict(catalog, |c| { + c.remove(b"StructTreeRoot"); + c.remove(b"MarkInfo"); + }) +} + /// Whether `o` is an object reference (`/Type /OBJR`, direct or indirect) to one of `gone`. fn refers_to(doc: &Document, o: &Object, gone: &HashSet) -> bool { doc.resolve(o) @@ -269,9 +303,10 @@ fn tree_node_entries(d: &Dict) -> Vec { /// reference keeps the annotation, its URL or text included, in a full rewrite after the page /// no longer lists it. Returns how many references were dropped. /// -/// Only elements that are objects of their own are edited: a structure element written in place -/// (a direct dictionary in a `/K` array or a tree) keeps its references, since changing it means -/// rewriting the object that holds it. Producers rarely write elements that way. +/// Only elements that are objects of their own are edited: changing a structure element written in +/// place (a direct dictionary in a `/K` array or a tree) means rewriting the object that holds it, +/// so when one of those refers to a removed annotation the whole tree goes instead (fail-closed). +/// Producers rarely write elements that way. pub(crate) fn drop_object_refs(doc: &mut Document, gone: &HashSet) -> Result { drop_object_refs_within(doc, gone, MAX_STRUCT_REFS) } @@ -302,6 +337,22 @@ pub(crate) fn drop_object_refs_within(doc: &mut Document, gone: &HashSet while let Some(k) = todo.pop() { match k { Object::Array(a) => todo.extend(a), + // A structure element written in place: its kids, and if it tags a removed + // annotation, the whole tree (see above). + Object::Dict(d) if d.contains(b"S") => { + let kids: Vec = match d.get(b"K").map(|k| doc.resolve(k)) { + Some(o) => match &*o { + Object::Array(a) => a.clone(), + _ => d.get(b"K").cloned().into_iter().collect(), + }, + None => Vec::new(), + }; + if kids.iter().any(|x| refers_to(doc, x, gone)) { + drop_tree(doc)?; + return Ok(0); + } + todo.extend(kids); + } // A number or name tree node written in place (§7.9.6, §7.9.7): its values and kids. Object::Dict(d) => todo.extend(tree_node_entries(&d)), Object::Ref(r) => { @@ -309,11 +360,7 @@ pub(crate) fn drop_object_refs_within(doc: &mut Document, gone: &HashSet continue; } if seen.len() > limit { - let Some(catalog) = doc.root() else { return Ok(0) }; - doc.update_dict(catalog, |c| { - c.remove(b"StructTreeRoot"); - c.remove(b"MarkInfo"); - })?; + drop_tree(doc)?; return Ok(0); } let object = doc.get(r); diff --git a/crates/redact/src/tests.rs b/crates/redact/src/tests.rs index f25ac197a..c721c5e6d 100644 --- a/crates/redact/src/tests.rs +++ b/crates/redact/src/tests.rs @@ -930,6 +930,81 @@ fn elements_listed_only_in_the_parent_tree_or_the_id_tree_lose_their_references( } } +/// #3285 review: an element without its own /Pg takes its ancestor's page, also when the parent +/// tree lists it. One under /K must not be reached first through the parent tree without that +/// page, and one only the parent tree lists finds it through /P. +#[test] +fn an_element_inheriting_its_page_loses_its_redacted_text_when_the_parent_tree_lists_it() { + let objs: Vec> = vec![ + b"<< /Type /Catalog /Pages 2 0 R /StructTreeRoot 5 0 R /MarkInfo << /Marked true >> >>".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 /StructParents 0 /Resources << /Font << /F1 9 0 R >> >> >>".to_vec(), + stream("", b"/P <> BDC BT /F1 14 Tf 20 150 Td (Codename Bluefin) Tj ET EMC /P <> BDC BT /F1 14 Tf 20 100 Td (Codename Bluefin) Tj ET EMC"), + // The document element 6 carries the page; 7 sits under it, 8 only points at it. + b"<< /Type /StructTreeRoot /K 6 0 R /ParentTree << /Nums [0 [7 0 R 8 0 R]] >> /ParentTreeNextKey 1 >>".to_vec(), + b"<< /Type /StructElem /S /Document /P 5 0 R /Pg 3 0 R /K [7 0 R] >>".to_vec(), + b"<< /Type /StructElem /S /P /P 6 0 R /K 0 /ActualText (Codename Bluefin) >>".to_vec(), + b"<< /Type /StructElem /S /P /P 6 0 R /K 1 /Alt (Codename Bluefin) >>".to_vec(), + b"<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>".to_vec(), + ]; + let mut doc = pdf(objs); + mark(&mut doc, 0, &[[10.0, 90.0, 290.0, 170.0]], ""); + let r = apply(&mut doc, None).unwrap(); + assert!(r.glyphs > 0, "{r:?}"); + for n in [7, 8] { + let element = doc.get(ObjRef::new(n, 0)).as_dict().cloned().unwrap(); + assert!(!element.contains(b"ActualText") && !element.contains(b"Alt"), "{n}: {element:?}"); + } +} + +/// #3285 review: when the tag walk is cut short while redaction replaced a form's stream, the +/// marked-content references it never reached still name the original stream through /Stm, so +/// the tree goes, as it does for removed annotations. Without a replaced stream it stays. +#[test] +fn a_tag_walk_cut_short_over_a_replaced_stream_removes_the_tree() { + let objs: Vec> = vec![ + b"<< /Type /Catalog /Pages 2 0 R /StructTreeRoot 5 0 R /MarkInfo << /Marked true >> >>".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] /StructParents 0 >>".to_vec(), + stream("/Type /XObject /Subtype /Form /BBox [0 0 10 10]", b"/P <> BDC EMC"), + b"<< /Type /StructTreeRoot /K 6 0 R >>".to_vec(), + b"<< /Type /StructElem /S /Document /P 5 0 R /K [7 0 R] >>".to_vec(), + b"<< /Type /StructElem /S /P /P 6 0 R /Pg 3 0 R /K << /Type /MCR /MCID 0 /Stm 4 0 R >> >>".to_vec(), + stream("/Type /XObject /Subtype /Form /BBox [0 0 10 10]", b""), + ]; + let page = ObjRef::new(3, 0); + let streams = Some((ObjRef::new(4, 0), ObjRef::new(8, 0))); + let has_tree = |doc: &Document| doc.get(doc.root().unwrap()).as_dict().is_some_and(|c| c.contains(b"StructTreeRoot") && c.contains(b"MarkInfo")); + let mut doc = pdf(objs.clone()); + crate::tags::clean_within(&mut doc, page, None, &[0].into(), &Default::default(), 1).unwrap(); + assert!(has_tree(&doc), "without a replaced stream the tree stays"); + let mut doc = pdf(objs); + crate::tags::clean_within(&mut doc, page, streams, &Default::default(), &Default::default(), 1).unwrap(); + assert!(!has_tree(&doc), "a cut-short walk over a replaced stream drops the tree"); +} + +/// #3285 review: a structure element written in place, here as an ID tree value, can't be +/// edited without rewriting what holds it; when it tags a removed annotation the tree goes +/// instead of keeping the annotation reachable in a full rewrite. +#[test] +fn an_element_written_in_place_that_tags_a_removed_annotation_removes_the_tree() { + let objs: Vec> = vec![ + b"<< /Type /Catalog /Pages 2 0 R /MarkInfo << /Marked true >> /StructTreeRoot 5 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] >>".to_vec(), + b"<< /Type /Annot /Subtype /Text /Rect [0 0 10 10] /Contents (hushhush note) >>".to_vec(), + b"<< /Type /StructTreeRoot /K 6 0 R /IDTree << /Names [(note1) << /Type /StructElem /S /Annot /P 6 0 R /K [<< /Type /OBJR /Obj 4 0 R >>] >>] >> >>".to_vec(), + b"<< /Type /StructElem /S /Document /P 5 0 R >>".to_vec(), + ]; + let gone: std::collections::HashSet = [ObjRef::new(4, 0)].into(); + let mut doc = pdf(objs); + crate::tags::drop_object_refs(&mut doc, &gone).unwrap(); + let catalog = doc.get(doc.root().unwrap()).as_dict().cloned().unwrap(); + assert!(!catalog.contains(b"StructTreeRoot") && !catalog.contains(b"MarkInfo"), "{catalog:?}"); + let bytes = write_full(&doc, &SaveOptions { object_streams: false, ..SaveOptions::default() }).unwrap(); + assert!(!bytes.windows(8).any(|w| w == b"hushhush"), "the removed comment can't be reached any more"); +} + /// Issue #1325: removing comments keeps the annotations that are not comments: the multimedia /// and print-production ones (Watermark, PrinterMark, TrapNet, Screen, Movie, RichMedia, 3D). #[test] From 3ef3c9efa9216fa67e0878cc79f1302391f8d833 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Mon, 19 Oct 2026 03:20:34 +0800 Subject: [PATCH 5/6] M8: tag cleanup refuses what it can't finish instead of removing the tree (#3302) Maintainer review on #3285: an element written in place that owns redacted marked content or tags a removed annotation, an inherited page the lookup gave up on (over 256 ancestors or a loop), and a walk past its limit are now RedactError::Tags, so nothing is changed. Removing only /StructTreeRoot left outline /SE and GoTo /SD reaching the elements. Inherited pages are cached per ancestor. clear_every_element and drop_tree are gone. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Gv2WwdgjmBz33GWkwgNn3Q --- crates/redact/src/lib.rs | 7 ++ crates/redact/src/tags.rs | 164 ++++++++++++++++++------------------- crates/redact/src/tests.rs | 137 +++++++++++++++++++------------ 3 files changed, 168 insertions(+), 140 deletions(-) diff --git a/crates/redact/src/lib.rs b/crates/redact/src/lib.rs index a8dbf64d9..78be3096f 100644 --- a/crates/redact/src/lib.rs +++ b/crates/redact/src/lib.rs @@ -51,6 +51,13 @@ pub enum RedactError { "redaction appearance has invalid geometry or exceeds the 8 MiB budget; reduce the marked area, turn off repeated text or apply fewer pages at once (nothing was redacted)" )] OverlayLimit, + /// The structure tree couldn't be cleaned completely (#3302): an element written in place that + /// would need changing, an element whose inherited page can't be found, or a tree too large to + /// walk. Tags that kept the removed text or annotation would undo the redaction. + #[error( + "the document's tags can't be cleaned completely (an element written in place, or a structure tree too large or too deep to follow), so nothing was changed" + )] + Tags, #[error(transparent)] Edit(#[from] pdfcraft_edit::EditError), #[error(transparent)] diff --git a/crates/redact/src/tags.rs b/crates/redact/src/tags.rs index 2b117fcf4..ab5735e06 100644 --- a/crates/redact/src/tags.rs +++ b/crates/redact/src/tags.rs @@ -9,6 +9,8 @@ use std::collections::{HashMap, HashSet}; use pdfcraft_cos::{Dict, Document, ObjRef, Object}; +use crate::RedactError; + /// Each MCID's operators (serialized), and whether anything is still drawn inside it. fn by_mcid(doc: &Document, resources: &Dict, data: &[u8]) -> HashMap, bool)> { let mut out: HashMap, bool)> = HashMap::new(); @@ -103,13 +105,15 @@ pub(crate) fn clean( stream: Option<(ObjRef, ObjRef)>, changed: &HashSet, empty: &HashSet, -) -> Result { +) -> Result { clean_within(doc, page, stream, changed, empty, MAX_STRUCT_REFS) } -/// [`clean`], visiting at most `limit` structure-tree references. A tree too large to walk in full -/// could hide an element that repeats the redacted text, so then every element in the document -/// loses its alternate and actual text instead (fail-closed, #3302). +/// [`clean`], visiting at most `limit` structure-tree references. Fail-closed (#3302): when the +/// cleanup can't be shown complete, it is [`RedactError::Tags`] and the redaction isn't made. That +/// is a tree too large to walk in full (it could hide an element that repeats the redacted text), +/// an element whose inherited page can't be found (its ancestry too deep, or looping), and an +/// element written in place that would need changing (only the object holding it could be). pub(crate) fn clean_within( doc: &mut Document, page: ObjRef, @@ -117,7 +121,7 @@ pub(crate) fn clean_within( changed: &HashSet, empty: &HashSet, limit: usize, -) -> Result { +) -> Result { if changed.is_empty() && stream.is_none() { return Ok(0); } @@ -135,24 +139,36 @@ pub(crate) fn clean_within( let mut todo: Vec<(Object, Option)> = [&b"IDTree"[..], b"ParentTree", b"K"].iter().filter_map(|key| root.get(key).cloned()).map(|k| (k, None)).collect(); let mut seen = HashSet::new(); + let mut pages = HashMap::new(); let mut hits: Vec<(ObjRef, Dict, Option, bool)> = Vec::new(); + // Whether marked content `x` (a kid on page `pg`) is one this cleanup must change. + let touches = |doc: &Document, x: &Object, pg: Option| { + mcid_of(doc, x, pg, page, stream).is_some_and(|m| stream.is_some() || changed.contains(&m) || empty.contains(&m)) + }; while let Some((k, pg)) = todo.pop() { match k { Object::Array(a) => todo.extend(a.into_iter().map(|x| (x, pg))), // A number or name tree node written in place. Object::Dict(d) if !d.contains(b"S") => todo.extend(tree_node_entries(&d).into_iter().map(|x| (x, pg))), + // An element written in place: its kids are followed, but if it owns marked content + // this cleanup changes, only rewriting what holds it could change it. + Object::Dict(d) => { + let pg = match d.get(b"Pg").and_then(Object::as_ref).or(pg) { + Some(p) => Some(p), + None => inherited_page(doc, &d, &mut pages)?, + }; + let kids = kids_of(doc, &d); + if kids.iter().any(|x| touches(doc, x, pg)) { + return Err(RedactError::Tags); + } + todo.extend(kids.into_iter().map(|x| (x, pg))); + } Object::Ref(r) => { if !seen.insert(r) { continue; } if seen.len() > limit { - let n = clear_every_element(doc)?; - if stream.is_some() { - // Marked-content references the walk didn't reach still name the - // original stream through /Stm, which a full save would then keep. - drop_tree(doc)?; - } - return Ok(n); + return Err(RedactError::Tags); } let object = doc.get(r); if let Object::Array(a) = &*object { @@ -165,14 +181,11 @@ pub(crate) fn clean_within( todo.extend(tree_node_entries(&d).into_iter().map(|x| (x, pg))); continue; } - let pg = d.get(b"Pg").and_then(Object::as_ref).or(pg).or_else(|| inherited_page(doc, &d)); - let kids: Vec = match d.get(b"K").map(|k| doc.resolve(k)) { - Some(o) => match &*o { - Object::Array(a) => a.clone(), - _ => d.get(b"K").cloned().into_iter().collect(), - }, - None => Vec::new(), + let pg = match d.get(b"Pg").and_then(Object::as_ref).or(pg) { + Some(p) => Some(p), + None => inherited_page(doc, &d, &mut pages)?, }; + let kids = kids_of(doc, &d); let sensitive = kids.iter().any(|x| mcid_of(doc, x, pg, page, stream).is_some_and(|m| changed.contains(&m))); let remap = stream.is_some() && kids.iter().any(|x| mcid_of(doc, x, pg, page, stream).is_some()); if sensitive || remap { @@ -234,57 +247,49 @@ pub(crate) fn clean_within( Ok(n) } -/// The fail-closed end of a walk that couldn't finish: every structure element in the document, -/// found by reading each object rather than through the tree, loses the text that could repeat -/// redacted words. Returns how many elements changed. -fn clear_every_element(doc: &mut Document) -> Result { - let keys = [&b"Alt"[..], b"ActualText", b"E"]; - let elements: Vec = doc - .scan_objects() - .filter(|(_, o)| { - o.as_dict().is_some_and(|d| { - (d.name(b"Type") == Some(b"StructElem") || (d.contains(b"S") && d.contains(b"P"))) && keys.iter().any(|k| d.contains(k)) - }) - }) - .map(|(r, _)| r) - .collect(); - for &r in &elements { - doc.update_dict(r, |e| { - for key in keys { - e.remove(key); - } - })?; - } - Ok(elements.len()) -} - /// The page an element inherits (§14.7.4.4): the nearest `/Pg` among its ancestors, following -/// `/P` at most [`MAX_PARENTS`] steps. -fn inherited_page(doc: &Document, d: &Dict) -> Option { +/// `/P`. `Ok(None)` when none has one; [`RedactError::Tags`] when the lookup gives up (more than +/// [`MAX_PARENTS`] ancestors, or a loop), which isn't the same as having no page. `pages` keeps +/// each ancestor's answer, so siblings don't climb the same chain again. +fn inherited_page(doc: &Document, d: &Dict, pages: &mut HashMap>) -> Result, RedactError> { + let mut chain: Vec = Vec::new(); let mut parent = d.get(b"P").and_then(Object::as_ref); - for _ in 0..MAX_PARENTS { - let p = doc.get(parent?); - let p = p.as_dict()?; + let found = loop { + let Some(r) = parent else { break None }; + if let Some(known) = pages.get(&r) { + break *known; + } + if chain.len() >= MAX_PARENTS || chain.contains(&r) { + return Err(RedactError::Tags); + } + chain.push(r); + let object = doc.get(r); + let Some(p) = object.as_dict() else { break None }; if let Some(pg) = p.get(b"Pg").and_then(Object::as_ref) { - return Some(pg); + break Some(pg); } parent = p.get(b"P").and_then(Object::as_ref); + }; + for r in chain { + pages.insert(r, found); + } + Ok(found) +} + +/// An element's kids: its `/K` array, or the one kid it has. +fn kids_of(doc: &Document, d: &Dict) -> Vec { + match d.get(b"K").map(|k| doc.resolve(k)) { + Some(o) => match &*o { + Object::Array(a) => a.clone(), + _ => d.get(b"K").cloned().into_iter().collect(), + }, + None => Vec::new(), } - None } /// The most ancestors [`inherited_page`] looks through; deeper trees, and cycles, go unanswered. const MAX_PARENTS: usize = 256; -/// Remove the structure tree, with `/MarkInfo`, which would claim tags there no longer are. -fn drop_tree(doc: &mut Document) -> Result<(), pdfcraft_cos::CosError> { - let Some(catalog) = doc.root() else { return Ok(()) }; - doc.update_dict(catalog, |c| { - c.remove(b"StructTreeRoot"); - c.remove(b"MarkInfo"); - }) -} - /// Whether `o` is an object reference (`/Type /OBJR`, direct or indirect) to one of `gone`. fn refers_to(doc: &Document, o: &Object, gone: &HashSet) -> bool { doc.resolve(o) @@ -305,20 +310,21 @@ fn tree_node_entries(d: &Dict) -> Vec { /// /// Only elements that are objects of their own are edited: changing a structure element written in /// place (a direct dictionary in a `/K` array or a tree) means rewriting the object that holds it, -/// so when one of those refers to a removed annotation the whole tree goes instead (fail-closed). +/// so when one of those refers to a removed annotation it is [`RedactError::Tags`] (fail-closed). /// Producers rarely write elements that way. -pub(crate) fn drop_object_refs(doc: &mut Document, gone: &HashSet) -> Result { +pub(crate) fn drop_object_refs(doc: &mut Document, gone: &HashSet) -> Result { drop_object_refs_within(doc, gone, MAX_STRUCT_REFS) } /// The most structure-tree references [`drop_object_refs`] visits. const MAX_STRUCT_REFS: usize = 2_000_000; -/// [`drop_object_refs`], visiting at most `limit` references. A tree too large to walk in full -/// could keep a reference the walk never reached, so then the whole structure tree goes instead -/// (with `/MarkInfo`, which would claim tags there no longer are): fail-closed, at the cost of -/// the document's tags. -pub(crate) fn drop_object_refs_within(doc: &mut Document, gone: &HashSet, limit: usize) -> Result { +/// [`drop_object_refs`], visiting at most `limit` references. Fail-closed: a tree too large to +/// walk in full could keep a reference the walk never reached, and an element written in place +/// can only be changed by rewriting what holds it, so either is [`RedactError::Tags`] rather than +/// a cleanup that leaves the annotation reachable (removing only the tree would not: an outline +/// item's `/SE` or a link's `/SD` still reaches its elements). +pub(crate) fn drop_object_refs_within(doc: &mut Document, gone: &HashSet, limit: usize) -> Result { if gone.is_empty() { return Ok(0); } @@ -337,19 +343,12 @@ pub(crate) fn drop_object_refs_within(doc: &mut Document, gone: &HashSet while let Some(k) = todo.pop() { match k { Object::Array(a) => todo.extend(a), - // A structure element written in place: its kids, and if it tags a removed - // annotation, the whole tree (see above). + // A structure element written in place: its kids are followed; one that tags a + // removed annotation can't be changed here (see above). Object::Dict(d) if d.contains(b"S") => { - let kids: Vec = match d.get(b"K").map(|k| doc.resolve(k)) { - Some(o) => match &*o { - Object::Array(a) => a.clone(), - _ => d.get(b"K").cloned().into_iter().collect(), - }, - None => Vec::new(), - }; + let kids = kids_of(doc, &d); if kids.iter().any(|x| refers_to(doc, x, gone)) { - drop_tree(doc)?; - return Ok(0); + return Err(RedactError::Tags); } todo.extend(kids); } @@ -360,8 +359,7 @@ pub(crate) fn drop_object_refs_within(doc: &mut Document, gone: &HashSet continue; } if seen.len() > limit { - drop_tree(doc)?; - return Ok(0); + return Err(RedactError::Tags); } let object = doc.get(r); if let Object::Array(a) = &*object { @@ -374,13 +372,7 @@ pub(crate) fn drop_object_refs_within(doc: &mut Document, gone: &HashSet todo.extend(tree_node_entries(d)); continue; } - let kids: Vec = match d.get(b"K").map(|k| doc.resolve(k)) { - Some(o) => match &*o { - Object::Array(a) => a.clone(), - _ => d.get(b"K").cloned().into_iter().collect(), - }, - None => Vec::new(), - }; + let kids = kids_of(doc, d); if kids.iter().any(|x| refers_to(doc, x, gone)) { hits.push(r); } diff --git a/crates/redact/src/tests.rs b/crates/redact/src/tests.rs index c721c5e6d..8af417dcc 100644 --- a/crates/redact/src/tests.rs +++ b/crates/redact/src/tests.rs @@ -810,11 +810,12 @@ fn flattened_widgets_and_far_pop_ups_leave_the_structure_tree() { } /// #2532 review: a structure tree larger than the walk's limit can't be searched in full, so a -/// reference the walk never reached could keep a removed annotation. Then the whole tree goes -/// (with `/MarkInfo`), rather than reporting success with references left behind; under the -/// limit only the reference goes. +/// reference the walk never reached could keep a removed annotation. Then it is an error, and +/// nothing is changed, rather than success with references left behind (removing only the tree +/// would leave an outline's `/SE` reaching them, review on #3285); under the limit only the +/// reference goes. #[test] -fn a_structure_tree_too_large_to_walk_is_removed_whole() { +fn a_structure_tree_too_large_to_walk_is_refused() { let objs: Vec> = vec![ b"<< /Type /Catalog /Pages 2 0 R /MarkInfo << /Marked true >> /StructTreeRoot 5 0 R >>".to_vec(), b"<< /Type /Pages /Kids [3 0 R] /Count 1 >>".to_vec(), @@ -835,11 +836,8 @@ fn a_structure_tree_too_large_to_walk_is_removed_whole() { assert!(catalog(&doc).contains(b"StructTreeRoot") && catalog(&doc).contains(b"MarkInfo"), "under the limit the tree stays"); let mut doc = pdf(objs); - crate::tags::drop_object_refs_within(&mut doc, &gone, 2).unwrap(); - assert!(!catalog(&doc).contains(b"StructTreeRoot"), "a tree the walk couldn't finish goes whole"); - assert!(!catalog(&doc).contains(b"MarkInfo")); - let bytes = write_full(&doc, &SaveOptions { object_streams: false, ..SaveOptions::default() }).unwrap(); - assert!(!bytes.windows(14).any(|w| w == b"secret.example"), "the removed link can't be reached any more"); + assert!(matches!(crate::tags::drop_object_refs_within(&mut doc, &gone, 2), Err(RedactError::Tags)), "a tree the walk couldn't finish is refused"); + assert!(catalog(&doc).contains(b"StructTreeRoot"), "nothing changed"); } /// #3302: an element listed only in the parent tree (not under the root's /K) still owns the @@ -868,11 +866,10 @@ fn an_element_listed_only_in_the_parent_tree_loses_its_redacted_text() { assert!(!element.contains(b"ActualText") && !element.contains(b"Alt"), "{element:?}"); } -/// #3302: a structure tree too large to walk in full fails closed: every element loses its -/// alternate and actual text, found by reading each object, instead of the walk stopping and -/// reporting success with elements never checked. +/// #3302: a structure tree too large to walk in full fails closed: an error, so the redaction +/// isn't made, instead of the walk stopping and reporting success with elements never checked. #[test] -fn a_tag_walk_cut_short_clears_every_element() { +fn a_tag_walk_cut_short_is_refused() { let objs: Vec> = vec![ b"<< /Type /Catalog /Pages 2 0 R /StructTreeRoot 5 0 R >>".to_vec(), b"<< /Type /Pages /Kids [3 0 R] /Count 1 >>".to_vec(), @@ -886,17 +883,12 @@ fn a_tag_walk_cut_short_clears_every_element() { let page = ObjRef::new(3, 0); let changed: std::collections::HashSet = [0].into(); let mut doc = pdf(objs.clone()); - crate::tags::clean_within(&mut doc, page, None, &changed, &Default::default(), 100).unwrap(); + assert_eq!(crate::tags::clean_within(&mut doc, page, None, &changed, &Default::default(), 100).unwrap(), 1); let figure = doc.get(ObjRef::new(8, 0)).as_dict().cloned().unwrap(); assert!(figure.contains(b"Alt"), "a walk that finishes keeps an unrelated element's text"); let mut doc = pdf(objs); - let n = crate::tags::clean_within(&mut doc, page, None, &changed, &Default::default(), 1).unwrap(); - assert_eq!(n, 2, "both elements with text change"); - for r in [7, 8] { - let e = doc.get(ObjRef::new(r, 0)).as_dict().cloned().unwrap(); - assert!(!e.contains(b"ActualText") && !e.contains(b"Alt") && !e.contains(b"E"), "{r}: {e:?}"); - } + assert!(matches!(crate::tags::clean_within(&mut doc, page, None, &changed, &Default::default(), 1), Err(RedactError::Tags))); } /// #2532 follow-up: an element reached only through the parent tree or the ID tree, not from @@ -957,37 +949,11 @@ fn an_element_inheriting_its_page_loses_its_redacted_text_when_the_parent_tree_l } } -/// #3285 review: when the tag walk is cut short while redaction replaced a form's stream, the -/// marked-content references it never reached still name the original stream through /Stm, so -/// the tree goes, as it does for removed annotations. Without a replaced stream it stays. -#[test] -fn a_tag_walk_cut_short_over_a_replaced_stream_removes_the_tree() { - let objs: Vec> = vec![ - b"<< /Type /Catalog /Pages 2 0 R /StructTreeRoot 5 0 R /MarkInfo << /Marked true >> >>".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] /StructParents 0 >>".to_vec(), - stream("/Type /XObject /Subtype /Form /BBox [0 0 10 10]", b"/P <> BDC EMC"), - b"<< /Type /StructTreeRoot /K 6 0 R >>".to_vec(), - b"<< /Type /StructElem /S /Document /P 5 0 R /K [7 0 R] >>".to_vec(), - b"<< /Type /StructElem /S /P /P 6 0 R /Pg 3 0 R /K << /Type /MCR /MCID 0 /Stm 4 0 R >> >>".to_vec(), - stream("/Type /XObject /Subtype /Form /BBox [0 0 10 10]", b""), - ]; - let page = ObjRef::new(3, 0); - let streams = Some((ObjRef::new(4, 0), ObjRef::new(8, 0))); - let has_tree = |doc: &Document| doc.get(doc.root().unwrap()).as_dict().is_some_and(|c| c.contains(b"StructTreeRoot") && c.contains(b"MarkInfo")); - let mut doc = pdf(objs.clone()); - crate::tags::clean_within(&mut doc, page, None, &[0].into(), &Default::default(), 1).unwrap(); - assert!(has_tree(&doc), "without a replaced stream the tree stays"); - let mut doc = pdf(objs); - crate::tags::clean_within(&mut doc, page, streams, &Default::default(), &Default::default(), 1).unwrap(); - assert!(!has_tree(&doc), "a cut-short walk over a replaced stream drops the tree"); -} - /// #3285 review: a structure element written in place, here as an ID tree value, can't be -/// edited without rewriting what holds it; when it tags a removed annotation the tree goes -/// instead of keeping the annotation reachable in a full rewrite. +/// edited without rewriting what holds it; when it tags a removed annotation, removing hidden +/// information is refused instead of keeping the annotation reachable in a full rewrite. #[test] -fn an_element_written_in_place_that_tags_a_removed_annotation_removes_the_tree() { +fn an_element_written_in_place_that_tags_a_removed_annotation_is_refused() { let objs: Vec> = vec![ b"<< /Type /Catalog /Pages 2 0 R /MarkInfo << /Marked true >> /StructTreeRoot 5 0 R >>".to_vec(), b"<< /Type /Pages /Kids [3 0 R] /Count 1 >>".to_vec(), @@ -998,11 +964,74 @@ fn an_element_written_in_place_that_tags_a_removed_annotation_removes_the_tree() ]; let gone: std::collections::HashSet = [ObjRef::new(4, 0)].into(); let mut doc = pdf(objs); - crate::tags::drop_object_refs(&mut doc, &gone).unwrap(); - let catalog = doc.get(doc.root().unwrap()).as_dict().cloned().unwrap(); - assert!(!catalog.contains(b"StructTreeRoot") && !catalog.contains(b"MarkInfo"), "{catalog:?}"); - let bytes = write_full(&doc, &SaveOptions { object_streams: false, ..SaveOptions::default() }).unwrap(); - assert!(!bytes.windows(8).any(|w| w == b"hushhush"), "the removed comment can't be reached any more"); + assert!(matches!(crate::tags::drop_object_refs(&mut doc, &gone), Err(RedactError::Tags))); +} + +/// Whether a full rewrite of `doc` still holds the redacted word. +fn bluefin_survives(doc: &Document) -> bool { + let bytes = write_full(doc, &SaveOptions { object_streams: false, ..SaveOptions::default() }).unwrap(); + bytes.windows(7).any(|w| w == b"Bluefin") +} + +/// #3285 review (the maintainer's probes): an element written in place in the parent tree that +/// owns the redacted text can't be changed, so applying is refused; the same element as an object +/// of its own is cleaned, and nothing of the word is left in a full rewrite. +#[test] +fn an_element_written_in_place_owning_redacted_text_is_refused() { + let objs = |element: &str| -> Vec> { + vec![ + b"<< /Type /Catalog /Pages 2 0 R /StructTreeRoot 5 0 R /MarkInfo << /Marked true >> >>".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 /StructParents 0 /Resources << /Font << /F1 9 0 R >> >> >>" + .to_vec(), + stream("", b"/P <> BDC BT /F1 14 Tf 20 150 Td (Codename Bluefin) Tj ET EMC"), + format!("<< /Type /StructTreeRoot /K 6 0 R /ParentTree << /Nums [0 [{element}]] >> /ParentTreeNextKey 1 >>").into_bytes(), + b"<< /Type /StructElem /S /Document /P 5 0 R >>".to_vec(), + b"<< /Type /StructElem /S /P /P 6 0 R /Pg 3 0 R /K 0 /ActualText (Codename Bluefin) >>".to_vec(), + b"<< >>".to_vec(), + b"<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>".to_vec(), + ] + }; + let mut doc = pdf(objs("<< /Type /StructElem /S /P /P 6 0 R /Pg 3 0 R /K 0 /ActualText (Codename Bluefin) >>")); + mark(&mut doc, 0, &[[10.0, 140.0, 290.0, 170.0]], ""); + assert!(matches!(apply(&mut doc, None), Err(RedactError::Tags))); + let mut doc = pdf(objs("7 0 R")); + mark(&mut doc, 0, &[[10.0, 140.0, 290.0, 170.0]], ""); + let r = apply(&mut doc, None).unwrap(); + assert!(r.glyphs > 0 && !bluefin_survives(&doc), "{r:?}"); +} + +/// #3285 review: an element whose inherited page can't be found (more ancestors than the lookup +/// follows) isn't taken for one with no page: applying is refused. Within the limit the page is +/// found and the text goes. +#[test] +fn an_element_whose_inherited_page_is_out_of_reach_is_refused() { + let objs = |depth: u32| -> Vec> { + let mut objs: Vec> = vec![ + b"<< /Type /Catalog /Pages 2 0 R /StructTreeRoot 5 0 R /MarkInfo << /Marked true >> >>".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 /StructParents 0 /Resources << /Font << /F1 9 0 R >> >> >>" + .to_vec(), + stream("", b"/P <> BDC BT /F1 14 Tf 20 150 Td (Codename Bluefin) Tj ET EMC"), + b"<< /Type /StructTreeRoot /K 6 0 R /ParentTree << /Nums [0 [7 0 R]] >> /ParentTreeNextKey 1 >>".to_vec(), + b"<< /Type /StructElem /S /Document /P 5 0 R /Pg 3 0 R >>".to_vec(), + format!("<< /Type /StructElem /S /P /P {} 0 R /K 0 /ActualText (Codename Bluefin) >>", 10 + depth - 1).into_bytes(), + b"<< >>".to_vec(), + b"<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>".to_vec(), + ]; + for i in 0..depth { + let parent = if i == 0 { 6 } else { 10 + i - 1 }; + objs.push(format!("<< /Type /StructElem /S /Div /P {parent} 0 R >>").into_bytes()); + } + objs + }; + let mut doc = pdf(objs(300)); + mark(&mut doc, 0, &[[10.0, 140.0, 290.0, 170.0]], ""); + assert!(matches!(apply(&mut doc, None), Err(RedactError::Tags))); + let mut doc = pdf(objs(100)); + mark(&mut doc, 0, &[[10.0, 140.0, 290.0, 170.0]], ""); + apply(&mut doc, None).unwrap(); + assert!(!bluefin_survives(&doc)); } /// Issue #1325: removing comments keeps the annotations that are not comments: the multimedia From cece9f5ccc05c0e3b01f11da83d07875a98c3df9 Mon Sep 17 00:00:00 2001 From: skylar taylor-barrick Date: Sun, 18 Oct 2026 16:45:35 -0600 Subject: [PATCH 6/6] M8: document discard-on-error for public redaction calls (#3302) --- crates/redact/src/lib.rs | 8 +++++++- crates/redact/src/sanitize.rs | 6 ++++++ crates/redact/src/tests.rs | 27 ++++++++++++++++++++++----- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/crates/redact/src/lib.rs b/crates/redact/src/lib.rs index 78be3096f..fdac37dbe 100644 --- a/crates/redact/src/lib.rs +++ b/crates/redact/src/lib.rs @@ -55,7 +55,7 @@ pub enum RedactError { /// would need changing, an element whose inherited page can't be found, or a tree too large to /// walk. Tags that kept the removed text or annotation would undo the redaction. #[error( - "the document's tags can't be cleaned completely (an element written in place, or a structure tree too large or too deep to follow), so nothing was changed" + "the document's tags can't be cleaned completely (an element written in place, or a structure tree too large or too deep to follow); the operation was refused" )] Tags, #[error(transparent)] @@ -312,6 +312,12 @@ fn localize_resources(doc: &mut Document, selected: &[usize]) -> Result<(), pdfc /// Apply every redaction mark (or only those on `pages`, 0-based). Irreversible for the saved /// file; callers keep the previous document for undo. +/// +/// # Errors +/// +/// This operation is not transactional: an error may leave `doc` partially modified, with +/// 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 { let all_marks = marks(doc); let chosen: Vec<&Mark> = all_marks.iter().filter(|m| pages.is_none_or(|p| p.contains(&m.page))).collect(); diff --git a/crates/redact/src/sanitize.rs b/crates/redact/src/sanitize.rs index 0c9228ac0..1940e376a 100644 --- a/crates/redact/src/sanitize.rs +++ b/crates/redact/src/sanitize.rs @@ -261,6 +261,12 @@ pub fn scan(doc: &Document) -> Vec<(Hidden, usize)> { } /// Remove the chosen categories. Returns what was removed per category. +/// +/// # Errors +/// +/// This operation is not transactional: an error may leave `doc` partially modified, with +/// 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 remove_hidden(doc: &mut Document, which: &[Hidden]) -> Result, RedactError> { let counts = scan(doc); let count = |h: Hidden| counts.iter().find(|c| c.0 == h).map_or(0, |c| c.1); diff --git a/crates/redact/src/tests.rs b/crates/redact/src/tests.rs index 8af417dcc..9a18f5574 100644 --- a/crates/redact/src/tests.rs +++ b/crates/redact/src/tests.rs @@ -952,19 +952,24 @@ fn an_element_inheriting_its_page_loses_its_redacted_text_when_the_parent_tree_l /// #3285 review: a structure element written in place, here as an ID tree value, can't be /// edited without rewriting what holds it; when it tags a removed annotation, removing hidden /// information is refused instead of keeping the annotation reachable in a full rewrite. +/// Direct callers discard the failed working copy to preserve the original document. #[test] fn an_element_written_in_place_that_tags_a_removed_annotation_is_refused() { let objs: Vec> = vec![ b"<< /Type /Catalog /Pages 2 0 R /MarkInfo << /Marked true >> /StructTreeRoot 5 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] >>".to_vec(), + b"<< /Type /Page /Parent 2 0 R /MediaBox [0 0 300 300] /Annots [4 0 R] >>".to_vec(), b"<< /Type /Annot /Subtype /Text /Rect [0 0 10 10] /Contents (hushhush note) >>".to_vec(), b"<< /Type /StructTreeRoot /K 6 0 R /IDTree << /Names [(note1) << /Type /StructElem /S /Annot /P 6 0 R /K [<< /Type /OBJR /Obj 4 0 R >>] >>] >> >>".to_vec(), b"<< /Type /StructElem /S /Document /P 5 0 R >>".to_vec(), ]; - let gone: std::collections::HashSet = [ObjRef::new(4, 0)].into(); - let mut doc = pdf(objs); - assert!(matches!(crate::tags::drop_object_refs(&mut doc, &gone), Err(RedactError::Tags))); + let original = pdf(objs); + let before = write_full(&original, &SaveOptions::default()).unwrap(); + let mut working = original.clone(); + assert!(matches!(sanitize::remove_hidden(&mut working, &[sanitize::Hidden::Comments]), Err(RedactError::Tags))); + assert!(!working.get(ObjRef::new(3, 0)).as_dict().unwrap().contains(b"Annots"), "the failed working copy was partially changed"); + drop(working); + assert_eq!(write_full(&original, &SaveOptions::default()).unwrap(), before, "discarding the working copy preserves the original"); } /// Whether a full rewrite of `doc` still holds the redacted word. @@ -976,6 +981,7 @@ fn bluefin_survives(doc: &Document) -> bool { /// #3285 review (the maintainer's probes): an element written in place in the parent tree that /// owns the redacted text can't be changed, so applying is refused; the same element as an object /// of its own is cleaned, and nothing of the word is left in a full rewrite. +/// Refusal does not promise rollback: direct callers discard the failed working copy. #[test] fn an_element_written_in_place_owning_redacted_text_is_refused() { let objs = |element: &str| -> Vec> { @@ -994,7 +1000,18 @@ fn an_element_written_in_place_owning_redacted_text_is_refused() { }; let mut doc = pdf(objs("<< /Type /StructElem /S /P /P 6 0 R /Pg 3 0 R /K 0 /ActualText (Codename Bluefin) >>")); mark(&mut doc, 0, &[[10.0, 140.0, 290.0, 170.0]], ""); - assert!(matches!(apply(&mut doc, None), Err(RedactError::Tags))); + let before = write_full(&doc, &SaveOptions::default()).unwrap(); + let mut working = doc.clone(); + let error = apply(&mut working, None).unwrap_err(); + assert!(matches!(error, RedactError::Tags)); + assert_eq!( + error.to_string(), + "the document's tags can't be cleaned completely (an element written in place, or a structure tree too large or too deep to follow); the operation was refused" + ); + assert!(!content(&working, 0).contains("Bluefin"), "the failed working copy was partially changed"); + assert!(bluefin_survives(&working), "the failed working copy still holds the word in its tags"); + drop(working); + assert_eq!(write_full(&doc, &SaveOptions::default()).unwrap(), before, "discarding the working copy preserves the original"); let mut doc = pdf(objs("7 0 R")); mark(&mut doc, 0, &[[10.0, 140.0, 290.0, 170.0]], ""); let r = apply(&mut doc, None).unwrap();