From 87aebd1b24618fa822d994f32209bff08e974056 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 14:47:29 -0500 Subject: [PATCH] fix(preflight): follow every /Next action and bound colour and intent work (#5926) * fix(preflight): follow every /Next action and bound colour and intent work Action chains followed an indirect /Next only when it named an array, and skipped references inside a /Next array, so a forbidden action one indirect hop behind an allowed one survived both verification and conversion; an untyped dictionary with /P and /K was exempt from action checks whatever its /S. Both are now followed and checked, and arrays are borrowed instead of deep-copied per chain. Conversion treats a /Subtype /Widget without /Type as an annotation, and takes forbidden annotations off their pages in one pass. Device-colour inspection decoded each content stream with the 1 GiB per-stream default and no total; it now shares one 1 GiB DecodeBudget and reports unread content instead of treating it as clean. Type 3 CharProcs shared by many fonts are read once, and declared() reads each output intent once (at most 8). * address review: no sRGB intent when colour couldn't all be read; count array reads in action chains; declare every output intent * address review: cut only an over-long identifier; test with a filter that can't be read * review: leave untyped structure elements out of the forbidden-action check * merge main, keep both * address review: a wide direct /Next array is refused before it is queued; queued references are resolved through aliases --- crates/preflight/src/lib.rs | 190 ++++++++++++++++++++++++++-------- crates/preflight/src/tests.rs | 124 ++++++++++++++++++++++ 2 files changed, 269 insertions(+), 45 deletions(-) diff --git a/crates/preflight/src/lib.rs b/crates/preflight/src/lib.rs index 7b6e960006..90c6d9db83 100644 --- a/crates/preflight/src/lib.rs +++ b/crates/preflight/src/lib.rs @@ -203,6 +203,7 @@ pub const WORDINGS: &[&str] = &[ "A graphics state has the rendering intent {}, which isn't a standard one", "A graphics state has a rendering intent that isn't a name", "The form asks viewers to make appearances (NeedAppearances)", + "Some content could not be read for device colour", // Pages, actions, annotations and fields. "Page {} has additional actions", "A {} action isn't allowed", @@ -229,7 +230,8 @@ pub struct Declared { /// `pdfaid:part` and `pdfaid:conformance`, e.g. (2, "B"). pub pdfa: Option<(u8, String)>, pub pdfua: Option, - /// Output intents' condition identifiers. + /// Output intents' condition identifiers: every intent the document lists (each object + /// once), each identifier cut to [`MAX_IDENTIFIER_BYTES`]. pub output_intents: Vec, } @@ -276,10 +278,27 @@ pub fn declared(doc: &Document) -> Declared { d.pdfua = xmp::identifier(&x, xmp::PDFUA_ID, "part").and_then(|p| p.parse().ok()); } let cat = catalog(doc); - if let Some(list) = cat.get(b"OutputIntents").map(|o| doc.resolve(o)).and_then(|o| o.as_array().cloned()) { - for oi in list { - if let Some(od) = doc.resolve(&oi).as_dict() { - let id = od.get(b"OutputConditionIdentifier").and_then(|v| doc.resolve(v).as_string().map(|s| s.to_text())).unwrap_or_default(); + let list = cat.get(b"OutputIntents").map(|o| doc.resolve(o)); + if let Some(list) = list.as_deref().and_then(Object::as_array) { + // One intent object listed again is one declaration: its identifier is not copied once + // per listing. Every distinct one is kept (verification reads them all too), and only + // the start of an identifier longer than any real one. + let mut seen = std::collections::HashSet::new(); + for oi in list.iter().filter(|oi| oi.as_ref().is_none_or(|r| seen.insert(r))) { + if let Some(od) = doc.resolve(oi).as_dict() { + let id = od + .get(b"OutputConditionIdentifier") + .and_then(|v| { + doc.resolve(v).as_string().map(|s| { + if s.bytes.len() <= MAX_IDENTIFIER_BYTES { + s.to_text() + } else { + // An even cut keeps UTF-16 text whole code units. + pdfcraft_cos::PdfString { bytes: s.bytes[..MAX_IDENTIFIER_BYTES & !1].to_vec(), hex: s.hex }.to_text() + } + }) + }) + .unwrap_or_default(); d.output_intents.push(id); } } @@ -287,6 +306,14 @@ pub fn declared(doc: &Document) -> Declared { d } +/// The most bytes of an output condition identifier [`declared`] keeps (a real one is a few +/// dozen). +const MAX_IDENTIFIER_BYTES: usize = 1024; + +/// Decoded bytes one verification reads looking for device colour, all content streams +/// together. Past it, content is not inspected, and the report says so. +const MAX_COLOUR_DECODED: usize = 1 << 30; + /// Every indirect object, with its reference. fn objects(doc: &Document) -> Vec<(ObjRef, std::sync::Arc)> { doc.object_numbers() @@ -361,6 +388,9 @@ struct DeviceColour { /// space, which PDF/A accepts whatever the output intent (ISO 19005-2 6.2.4.3). default_rgb: bool, default_cmyk: bool, + /// Content that could not be read (undecodable, or past [`MAX_COLOUR_DECODED`]) may use + /// device colour that was not seen. + incomplete: bool, } impl DeviceColour { @@ -388,27 +418,35 @@ const MAX_DIRECT_DEPTH: u8 = 8; /// The glyph procedures of every Type 3 font in `d`: `d` itself, or a font held directly inside /// it (a font can sit inline in a resource dictionary). -fn type3_procs(doc: &Document, d: &Dict, depth: u8, out: &mut std::collections::HashSet) { +fn type3_procs(doc: &Document, d: &Dict, depth: u8, read: &mut std::collections::HashSet, out: &mut std::collections::HashSet) { if d.name(b"Subtype") == Some(b"Type3") - && let Some(procs) = d.get(b"CharProcs").map(|c| doc.resolve(c)) - && let Some(procs) = procs.as_dict() + && let Some(c) = d.get(b"CharProcs") + // One glyph dictionary shared by many fonts is read once: its entries are the same. + && c.as_ref().is_none_or(|r| read.insert(r)) + && let Some(procs) = doc.resolve(c).as_dict() { out.extend(procs.iter().filter_map(|(_, p)| p.as_ref())); } for (_, v) in d.iter() { - type3_procs_within(doc, v, depth.saturating_add(1), out); + type3_procs_within(doc, v, depth.saturating_add(1), read, out); } } -fn type3_procs_within(doc: &Document, o: &Object, depth: u8, out: &mut std::collections::HashSet) { +fn type3_procs_within( + doc: &Document, + o: &Object, + depth: u8, + read: &mut std::collections::HashSet, + out: &mut std::collections::HashSet, +) { if depth > MAX_DIRECT_DEPTH { return; } match o { - Object::Dict(d) => type3_procs(doc, d, depth, out), + Object::Dict(d) => type3_procs(doc, d, depth, read, out), Object::Array(a) => { for v in a { - type3_procs_within(doc, v, depth.saturating_add(1), out); + type3_procs_within(doc, v, depth.saturating_add(1), read, out); } } _ => {} @@ -430,9 +468,10 @@ fn referenced_content(doc: &Document, objs: &[(ObjRef, std::sync::Arc)]) }; out.extend(contents.iter().filter_map(Object::as_ref)); } + let mut read = std::collections::HashSet::new(); for (_, o) in objs { if let Some(d) = dict_of(o) { - type3_procs(doc, d, 0, &mut out); + type3_procs(doc, d, 0, &mut read, &mut out); } } out @@ -560,6 +599,7 @@ fn mark_cs_object(doc: &Document, cs: &Object, used: &mut DeviceColour, depth: u fn uses_device_colour(doc: &Document, objs: &[(ObjRef, std::sync::Arc)]) -> DeviceColour { let mut used = DeviceColour::default(); let referenced = referenced_content(doc, objs); + let mut budget = pdfcraft_cos::DecodeBudget::new(MAX_COLOUR_DECODED, usize::MAX); for (r, o) in objs { let Some(d) = dict_of(o) else { continue }; // A resource dictionary's /ColorSpace, whether it is this object or the page's inline @@ -586,9 +626,14 @@ fn uses_device_colour(doc: &Document, objs: &[(ObjRef, std::sync::Arc)]) if !(used.gray && used.rgb && used.cmyk) && let Object::Stream(s) = &**o && (referenced.contains(r) || is_content(&s.dict)) - && let Ok(data) = s.decoded() { - let _ = content_colour(&data, &mut used); + match budget.decode(s) { + Ok(data) => { + let _ = content_colour(&data, &mut used); + } + // Not read: it may draw in a device space nothing above saw. + Err(_) => used.incomplete = true, + } } } used @@ -830,6 +875,9 @@ fn check_output_intent( issue: &mut impl FnMut(&'static str, &'static str, Option, Option, bool), ) { let used = uses_device_colour(doc, objs); + if used.incomplete { + issue("6.2.3", "Some content could not be read for device colour", None, None, false); + } match pdfa_intent_profile(doc) { None if used.any() => { issue("6.2.3", "Device colour is used, but there is no PDF/A output intent", None, None, !used.cmyk); @@ -1215,22 +1263,56 @@ const MAX_ACTION_CHAIN: usize = 1024; fn chain_forbidden(doc: &Document, a: &Dict) -> Option { let mut read = 0; let mut seen = std::collections::HashSet::new(); - // Actions written in an indirect `/Next` array, copied out of it. - let mut held: Vec = Vec::new(); + // Objects `/Next` refers to (an action, or an array of them), each read once and borrowed: + // one array shared by many chains is not copied per chain. + let mut held: Vec> = Vec::new(); if let Some(s) = walk_chain(doc, a, &mut read, &mut seen, &mut held) { return Some(s); } - while let Some(d) = held.pop() { - if let Some(s) = walk_chain(doc, &d, &mut read, &mut seen, &mut held) { - return Some(s); + while let Some(o) = held.pop() { + let dicts: Vec<&Dict> = match &*o { + Object::Dict(d) => vec![d], + Object::Array(list) => { + // An array read is work too, with its entries: a chain of arrays alone must + // still reach the limit. + read = read.saturating_add(1 + list.len().min(MAX_ACTION_CHAIN)); + if read > MAX_ACTION_CHAIN { + return Some("too deeply chained".into()); + } + // More than the chain limit can't be read anyway. + hold_referenced(doc, list, &mut seen, &mut held); + list.iter().filter_map(Object::as_dict).take(MAX_ACTION_CHAIN).collect() + } + _ => continue, + }; + for d in dicts { + if let Some(s) = walk_chain(doc, d, &mut read, &mut seen, &mut held) { + return Some(s); + } } } None } -/// [`chain_forbidden`] for the actions reachable from `a` through direct `/Next` entries. The -/// actions of each indirect `/Next` array (each read once) go to `held`. -fn walk_chain(doc: &Document, a: &Dict, read: &mut usize, seen: &mut std::collections::HashSet, held: &mut Vec) -> Option { +/// Queue the objects the references in a `/Next` array name, each once. +fn hold_referenced(doc: &Document, list: &[Object], seen: &mut std::collections::HashSet, held: &mut Vec>) { + for r in list.iter().filter_map(Object::as_ref).take(MAX_ACTION_CHAIN) { + if seen.insert(r) { + // Resolved through any alias: a reference to a reference names its object's value. + held.push(doc.resolve(&Object::Ref(r))); + } + } +} + +/// [`chain_forbidden`] for the actions reachable from `a` through direct `/Next` entries. What +/// an indirect `/Next`, or a reference inside a `/Next` array, names goes to `held`. +fn walk_chain( + doc: &Document, + a: &Dict, + read: &mut usize, + seen: &mut std::collections::HashSet, + held: &mut Vec>, +) -> Option { let mut stack = vec![a]; while let Some(d) = stack.pop() { *read += 1; @@ -1242,13 +1324,16 @@ fn walk_chain(doc: &Document, a: &Dict, read: &mut usize, seen: &mut std::collec } match d.get(b"Next") { Some(Object::Dict(next)) => stack.push(next), - Some(Object::Array(list)) => stack.extend(list.iter().filter_map(Object::as_dict)), - Some(Object::Ref(r)) if seen.insert(*r) => { - if let Object::Array(list) = &*doc.get(*r) { - // More than the chain limit can't be read anyway. - held.extend(list.iter().filter_map(Object::as_dict).take(MAX_ACTION_CHAIN).cloned()); + Some(Object::Array(list)) => { + *read = read.saturating_add(list.len().min(MAX_ACTION_CHAIN)); + // A wide array is refused before any of it is queued. + if *read > MAX_ACTION_CHAIN { + return Some("too deeply chained".into()); } + hold_referenced(doc, list, seen, held); + stack.extend(list.iter().filter_map(Object::as_dict)); } + Some(Object::Ref(r)) if seen.insert(*r) => held.push(doc.resolve(&Object::Ref(*r))), _ => {} } } @@ -1291,6 +1376,26 @@ fn strip_actions(doc: &Document, d: &mut Dict) -> bool { changed } +/// Take the annotations `gone` off every page that lists them, each page's list rewritten once, and +/// replace their indirect objects with `Null`. +fn remove_from_pages(doc: &mut Document, gone: &std::collections::HashSet) -> Result<(), pdfcraft_cos::CosError> { + if gone.is_empty() { + return Ok(()); + } + for p in pdfcraft_model::pages(doc) { + let annots = p.dict.get(b"Annots").map(|a| doc.resolve(a)).and_then(|a| a.as_array().cloned()).unwrap_or_default(); + let listed = |a: &Object| a.as_ref().is_some_and(|r| gone.contains(&r)); + if annots.iter().any(listed) { + let kept: Vec = annots.into_iter().filter(|a| !listed(a)).collect(); + doc.update_dict(p.obj, |pd| pd.set(b"Annots".to_vec(), Object::Array(kept)))?; + } + } + for r in gone { + doc.set(*r, Object::Null); + } + Ok(()) +} + /// Collects every indirect annotation reference listed in any page's `/Annots` array. fn page_annot_refs(doc: &Document) -> std::collections::HashSet { let mut refs = std::collections::HashSet::new(); @@ -1304,19 +1409,6 @@ fn page_annot_refs(doc: &Document) -> std::collections::HashSet { refs } -/// Removes annotation `r` from every page's `/Annots` list and replaces its indirect object with `Null`. -fn remove_annot_from_pages(doc: &mut Document, r: ObjRef) -> Result<(), pdfcraft_cos::CosError> { - for p in pdfcraft_model::pages(doc) { - let annots = p.dict.get(b"Annots").map(|a| doc.resolve(a)).and_then(|a| a.as_array().cloned()).unwrap_or_default(); - if annots.iter().any(|a| a.as_ref() == Some(r)) { - let kept: Vec = annots.into_iter().filter(|a| a.as_ref() != Some(r)).collect(); - doc.update_dict(p.obj, |pd| pd.set(b"Annots".to_vec(), Object::Array(kept)))?; - } - } - doc.set(r, Object::Null); - Ok(()) -} - fn fix_annot_or_field(doc: &Document, nd: &mut Dict, is_annot: bool, sub: Option<&[u8]>, actions: &mut usize, flags: &mut usize) -> bool { let mut changed = strip_actions(doc, nd); *actions += changed as usize; @@ -1413,8 +1505,10 @@ pub fn convert(doc: &mut Document, level: Level) -> Result Result::to_vec); let sub = d.name(b"Subtype").map(<[u8]>::to_vec); - let is_annot = matches!(&*o, Object::Dict(_)) && is_annotation(doc, &d, ty.as_deref(), sub.as_deref(), page_annots.contains(&r)); + let is_annot = matches!(&*o, Object::Dict(_)) + // A widget is an annotation whether or not it says `/Type /Annot` (the form reader + // finds it by its field), so its actions go the same way. + && (is_annotation(doc, &d, ty.as_deref(), sub.as_deref(), page_annots.contains(&r)) || sub.as_deref() == Some(b"Widget")); if is_annot && sub.as_deref().is_some_and(|s| FORBIDDEN_ANNOTS.contains(&s)) { - remove_annot_from_pages(doc, r)?; + // Taken off its page below, all of them in one pass over the pages. + forbidden_annots.insert(r); removed += 1; continue; } @@ -1489,6 +1588,7 @@ pub fn convert(doc: &mut Document, level: Level) -> Result>", pages, page, &link(&format!("<< /S /GoTo /D [3 0 R /Fit] /Next {js} >>"))], ""), // `/Next` refers to an array that holds the action directly. build(&["<< /Type /Catalog /Pages 2 0 R >>", pages, page, &link("<< /S /GoTo /D [3 0 R /Fit] /Next 5 0 R >>"), &format!("[{js}]")], ""), + // `/Next` refers to the action itself, which carries `/P` and `/K` as a structure + // element would: those keys must not exempt it. + build( + &[ + "<< /Type /Catalog /Pages 2 0 R >>", + pages, + page, + &link("<< /S /GoTo /D [3 0 R /Fit] /Next 5 0 R >>"), + "<< /S /JavaScript /JS (app.alert(1)) /P 3 0 R /K 0 >>", + ], + "", + ), + // A `/Next` array refers to the action. + build(&["<< /Type /Catalog /Pages 2 0 R >>", pages, page, &link("<< /S /GoTo /D [3 0 R /Fit] /Next [5 0 R] >>"), js], ""), ]; for (i, mut doc) in cases.into_iter().enumerate() { let found = messages(&verify(&doc, Level::A2b)).contains(&"A JavaScript action isn't allowed"); @@ -495,6 +509,116 @@ fn an_output_intent_profile_header_is_read_within_a_size_cap() { } } +/// A widget the form reader finds by its field is an annotation whether or not it writes +/// `/Type /Annot`: conversion takes its additional actions off all the same. +#[test] +fn a_widget_without_a_type_loses_its_additional_actions() { + let pages = "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 200] >>"; + let page = "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>"; + let widget = "<< /Subtype /Widget /Rect [0 0 10 10] /F 4 /T (f) /AA << /K << /S /JavaScript /JS (1) >> >> >>"; + let mut doc = build(&["<< /Type /Catalog /Pages 2 0 R >>", pages, page, widget], ""); + convert(&mut doc, Level::A2b).unwrap(); + let w = doc.get(pdfcraft_cos::ObjRef::new(4, 0)); + assert!(w.as_dict().is_some_and(|d| d.get(b"AA").is_none()), "{w:?}"); +} + +/// Forbidden annotations on one page are taken off it in one pass, all of them. +#[test] +fn forbidden_annotations_leave_their_page_together() { + let n = 300; + let annots: Vec = (0..n).map(|i| format!("{} 0 R", i + 4)).collect(); + let page = format!("<< /Type /Page /Parent 2 0 R /Annots [{} 2 0 R] >>", annots.join(" ")); + let mut objs = + vec!["<< /Type /Catalog /Pages 2 0 R >>".to_string(), "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 200] >>".into(), page]; + objs.extend((0..n).map(|_| "<< /Type /Annot /Subtype /Sound /Rect [0 0 0 0] /P 3 0 R >>".to_string())); + let refs: Vec<&str> = objs.iter().map(String::as_str).collect(); + let mut doc = build(&refs, ""); + let r = convert(&mut doc, Level::A2b).unwrap(); + assert!(r.fixed.iter().any(|f| f.starts_with(&format!("{n}: removed multimedia"))), "{:?}", r.fixed); + let left = pdfcraft_model::pages(&doc)[0].dict.get(b"Annots").and_then(|a| doc.resolve(a).as_array().map(Vec::len)); + assert_eq!(left, Some(1)); +} + +/// Content that can't be read may draw in DeviceCMYK: convert then adds no sRGB output intent. +#[test] +fn convert_adds_no_srgb_intent_when_colour_could_not_all_be_read() { + let mut doc = build( + &[ + "<< /Type /Catalog /Pages 2 0 R >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 200] >>", + "<< /Type /Page /Parent 2 0 R /Contents 4 0 R /Resources << >> >>", + // A /Filter that isn't a name can't be decoded: the content isn't read. + "<< /Length 9 /Filter 5 >>\nstream\nnot flate\nendstream", + ], + "", + ); + convert(&mut doc, Level::A2b).unwrap(); + assert!(declared(&doc).output_intents.is_empty()); +} + +/// A `/Next` chain of arrays alone (each array naming the next) still reaches the chain limit, +/// so the walk's work is bounded however the arrays are linked. +#[test] +fn an_array_only_next_chain_is_too_deeply_chained() { + let mut doc = Document::new_empty(); + let mut next = Object::Array(Vec::new()); + for _ in 0..2 * MAX_ACTION_CHAIN { + next = Object::Array(vec![Object::Ref(doc.add(next))]); + } + let mut action = Dict::new(); + action.set(b"S".to_vec(), Object::name("URI")); + action.set(b"Next".to_vec(), Object::Ref(doc.add(next))); + assert_eq!(chain_forbidden(&doc, &action).as_deref(), Some("too deeply chained")); + // A short chain of arrays is read through. + let mut short = Object::Array(Vec::new()); + for _ in 0..10 { + short = Object::Array(vec![Object::Ref(doc.add(short))]); + } + action.set(b"Next".to_vec(), Object::Ref(doc.add(short))); + assert_eq!(chain_forbidden(&doc, &action), None); +} + +/// A direct `/Next` array wider than the chain limit is refused as soon as it is charged, and a +/// `/Next` that reaches its array through a reference alias is read through it. +#[test] +fn a_wide_direct_next_array_is_refused_and_an_alias_is_followed() { + let uri = || { + let mut d = Dict::new(); + d.set(b"S".to_vec(), Object::name("URI")); + d + }; + let mut action = uri(); + action.set(b"Next".to_vec(), Object::Array((0..=MAX_ACTION_CHAIN).map(|_| Object::Dict(uri())).collect())); + let doc = Document::new_empty(); + assert_eq!(chain_forbidden(&doc, &action).as_deref(), Some("too deeply chained")); + // `/Next` -> 1 0 R -> 2 0 R -> [ << /S /JavaScript >> ] + let mut doc = Document::new_empty(); + let mut js = Dict::new(); + js.set(b"S".to_vec(), Object::name("JavaScript")); + let list = doc.add(Object::Array(vec![Object::Dict(js)])); + let alias = doc.add(Object::Ref(list)); + let mut action = uri(); + action.set(b"Next".to_vec(), Object::Ref(alias)); + assert_eq!(chain_forbidden(&doc, &action).as_deref(), Some("JavaScript")); +} + +/// Every distinct output intent is declared, past eight; an identifier longer than any real one +/// keeps its start. +#[test] +fn every_output_intent_is_declared_with_its_identifier_cut_to_the_bound() { + let long = "x".repeat(4 * MAX_IDENTIFIER_BYTES); + let intents: Vec = (0..9) + .map(|i| { + format!("<< /Type /OutputIntent /S /GTS_PDFA1 /OutputConditionIdentifier ({}) >>", if i == 8 { long.clone() } else { format!("c{i}") }) + }) + .collect(); + let catalog = format!("<< /Type /Catalog /Pages 2 0 R /OutputIntents [{}] >>", intents.join(" ")); + let doc = build(&[catalog.as_str(), "<< /Type /Pages /Kids [] /Count 0 >>"], ""); + let d = declared(&doc); + assert_eq!(d.output_intents.len(), 9); + assert_eq!(d.output_intents[8].len(), MAX_IDENTIFIER_BYTES); +} + #[test] fn typeless_annotations_indexed_cmyk_and_indirect_filter_elements_are_handled() { // Annotations in /Annots that omit optional /Type /Annot and /P (while keeping required /Subtype