From 619a239cebe25054ba59486ce2b878b66613c0ce Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 12:12:27 -0500 Subject: [PATCH 01/22] ci: run Harden-Runner on every job (#7003) Adds step-security/harden-runner (audit, pinned to v2.22.4 like craft-common-actions) as the first step of each job that did not have it, so every workflow in the org's public repos reports its egress to StepSecurity. Audit, not block: block-mode allow lists come from each job's observed traffic. --- .github/workflows/ci.yml | 4 ++++ .github/workflows/fuzz.yml | 4 ++++ .github/workflows/packaging-lint.yml | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 94a4c545fe..38ee53caf3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,6 +29,10 @@ jobs: permissions: contents: read steps: + - name: Harden runner + uses: step-security/harden-runner@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4 + with: + egress-policy: audit - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml index b7b1d409c8..fe0a0074d7 100644 --- a/.github/workflows/fuzz.yml +++ b/.github/workflows/fuzz.yml @@ -24,6 +24,10 @@ jobs: name: fuzz (mutation, 15 min) runs-on: ubuntu-latest steps: + - name: Harden runner + uses: step-security/harden-runner@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4 + with: + egress-policy: audit - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false diff --git a/.github/workflows/packaging-lint.yml b/.github/workflows/packaging-lint.yml index ea61d84303..1771a35fea 100644 --- a/.github/workflows/packaging-lint.yml +++ b/.github/workflows/packaging-lint.yml @@ -27,6 +27,10 @@ jobs: name: Windows installation policy (no app build) runs-on: windows-latest steps: + - name: Harden runner + uses: step-security/harden-runner@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4 + with: + egress-policy: audit - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false From d4608308591dc566dd31bd1486801cc48d14209f Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 12:44:55 -0500 Subject: [PATCH 02/22] Updates: take craft-update from craft-libs main at 6e03f65bf --- Cargo.lock | 3 ++- Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9cf008f02e..5d4d1fb2b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1289,7 +1289,7 @@ dependencies = [ [[package]] name = "craft-update" version = "0.1.0" -source = "git+https://github.com/storytold/craft-libs?rev=1b34eba41e62138fc2438d2734523b302840b45d#1b34eba41e62138fc2438d2734523b302840b45d" +source = "git+https://github.com/storytold/craft-libs?rev=6e03f65bf45181db77ba6229716e6624db9eb25b#6e03f65bf45181db77ba6229716e6624db9eb25b" dependencies = [ "chrono", "dunce", @@ -1299,6 +1299,7 @@ dependencies = [ "image", "log", "plist", + "rustix 1.1.6", "semver", "serde", "serde_json", diff --git a/Cargo.toml b/Cargo.toml index d566a69d1f..d850df512f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,7 +30,7 @@ craft-print = { git = "https://github.com/storytold/craft-libs", version = "0.1. # Whether a path stored in a document may be opened (GoToR link targets). craft-docpath = { version = "0.1.0", git = "https://github.com/storytold/craft-libs", rev = "787a7a0f972fd1d7a7618fdbeca3a5267a0407ff" } # craft-update: app-specific pretend versions apply to startup and manual checks. -craft-update = { git = "https://github.com/storytold/craft-libs", version = "=0.1.0", rev = "1b34eba41e62138fc2438d2734523b302840b45d", features = ["egui"] } +craft-update = { git = "https://github.com/storytold/craft-libs", version = "=0.1.0", rev = "6e03f65bf45181db77ba6229716e6624db9eb25b", features = ["egui"] } # internal pdfcraft-geom = { path = "crates/geom", version = "0.18.1" } pdfcraft-filters = { path = "crates/filters", version = "0.18.1" } From 583ae295993ab0c944ebb87d50f7db6acb289d1c Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 14:46:34 -0500 Subject: [PATCH 03/22] fix(sign): a widget removed from a page's inline Annots is not a comment change (#6013) --- crates/sign/tests/it/pdf.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/crates/sign/tests/it/pdf.rs b/crates/sign/tests/it/pdf.rs index e765941c9a..fa45ab1ecb 100644 --- a/crates/sign/tests/it/pdf.rs +++ b/crates/sign/tests/it/pdf.rs @@ -325,6 +325,21 @@ fn an_indirect_annots_array_is_judged_by_the_annotations_it_gains() { assert_eq!((s.status, s.modification), (Status::Invalid, Modification::Disallowed(vec![kind::OTHER]))); } +/// A signature widget taken off the page's inline /Annots array is covered by no permission (the +/// indirect-array case is the test above), and stays a change whatever level the signature has. +#[test] +fn removing_a_widget_from_an_inline_annots_array_is_not_a_comment_change() { + let id = pkcs12::open(&data("rsa-aes.p12"), "test").unwrap(); + let trust = TrustStore { certs: vec![id.certificate.clone()] }; + let page = pdfcraft_cos::ObjRef { num: 3, generation: 0 }; + let signed = pdfcraft_sign::sign(&open(&fixture()), &id, &opts()).unwrap(); + let inline = open(&signed).get(page).as_dict().and_then(|d| d.get(b"Annots").cloned()).unwrap(); + assert!(matches!(inline, Object::Array(ref a) if !a.is_empty()), "the page keeps its widgets inline"); + let stripped = edit_after(&signed, |doc| doc.update_dict(page, |p| p.set(b"Annots".to_vec(), Object::Array(vec![]))).unwrap()); + let s = signatures(&open(&stripped), &stripped, &trust).into_iter().find(|s| s.signed).unwrap(); + assert_eq!((s.status, s.modification), (Status::Invalid, Modification::Disallowed(vec![kind::OTHER]))); +} + /// Issue #1328: a signature field whose /V is null, directly or through a reference, is unsigned: /// it is listed as unsigned and can be signed by name. #[test] From d46d9daea5b0b3bb316bb5889b1757df5e2512ab Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 14:46:47 -0500 Subject: [PATCH 04/22] fix(edit): editor font enumeration reads a face name without building font metrics (#6086) --- crates/edit/src/text/editor.rs | 4 ++-- crates/fonts/src/pdf.rs | 25 +++++++++++++++---------- crates/fonts/src/pdf/tests.rs | 10 ++++++++++ 3 files changed, 27 insertions(+), 12 deletions(-) diff --git a/crates/edit/src/text/editor.rs b/crates/edit/src/text/editor.rs index 4f4abf3a43..e6df1dbfee 100644 --- a/crates/edit/src/text/editor.rs +++ b/crates/edit/src/text/editor.rs @@ -2,7 +2,7 @@ use super::blocks::{BlockStyle, TextBlock}; use super::fallback::MAX_TYPE3_GLYPHS; use super::lines::{page_dict, text_lines}; use super::rewrite::rewrite_block_from; -use super::{DecodeBudget, Dict, Document, EditError, Metrics, Object, TextLine, TextSession}; +use super::{DecodeBudget, Dict, Document, EditError, Object, TextLine, TextSession}; /// An embedded source font or an approved bundled document-font choice. /// Source bytes stay in memory and are never installed or copied into another document. @@ -45,7 +45,7 @@ pub fn text_editor_fonts(doc: &Document, page: usize) -> Result Option { } } +/// The font's `/BaseFont` (a Type 3 font's descriptor `/FontName` when it has none), read without +/// building its metrics, so it decodes no CMap. +pub fn base_font_name(doc: &Document, font: &Dict) -> String { + font.name(b"BaseFont") + .map(|name| String::from_utf8_lossy(name).into_owned()) + .or_else(|| { + (font.name(b"Subtype") == Some(b"Type3")) + .then(|| dict(doc, font.get(b"FontDescriptor"))) + .flatten() + .and_then(|d| d.name(b"FontName").map(|name| String::from_utf8_lossy(name).into_owned())) + }) + .unwrap_or_default() +} + /// Codespace ranges and CID mappings from an embedded CMap stream, at most /// [`MAX_CMAP_RANGES`] of each (every glyph searches them). #[allow(clippy::type_complexity)] @@ -909,16 +923,7 @@ impl Metrics { pub fn from_dict_within(doc: &Document, font: &Dict, budget: &mut DecodeBudget) -> Self { let mut reads = Reads::new(doc, budget); let mut m = Self::read_metrics(doc, font, &mut reads); - let base = font - .name(b"BaseFont") - .map(|name| String::from_utf8_lossy(name).into_owned()) - .or_else(|| { - (font.name(b"Subtype") == Some(b"Type3")) - .then(|| dict(doc, font.get(b"FontDescriptor"))) - .flatten() - .and_then(|d| d.name(b"FontName").map(|name| String::from_utf8_lossy(name).into_owned())) - }) - .unwrap_or_default(); + let base = base_font_name(doc, font); m.subset = base.len() > 7 && base.as_bytes()[6] == b'+' && base[..6].bytes().all(|b| b.is_ascii_uppercase()); let (bold, italic) = style_from_name(&base); m.bold |= bold; diff --git a/crates/fonts/src/pdf/tests.rs b/crates/fonts/src/pdf/tests.rs index 4c5f90044f..74880c94a4 100644 --- a/crates/fonts/src/pdf/tests.rs +++ b/crates/fonts/src/pdf/tests.rs @@ -1823,6 +1823,16 @@ fn low_codes_resolve_to_the_covering_range() { assert_eq!((u.covering(0xFFFF).is_some(), u.covering(0x1_0001).is_some(), u.covering(0x1_0002).is_none()), (true, true, true)); } +#[test] +fn base_font_name_matches_the_metrics_name() { + let doc = Document::new_empty(); + let mut f = Dict::new(); + f.set(b"BaseFont".to_vec(), Object::name("ABCDEF+Body")); + assert_eq!(base_font_name(&doc, &f), "ABCDEF+Body"); + assert_eq!(base_font_name(&doc, &f), Metrics::from_dict(&doc, &f).base_font); + assert_eq!(base_font_name(&doc, &Dict::new()), ""); +} + #[test] fn refused_subset_reads_do_not_encode_unverified_glyphs() { let mut doc = Document::new_empty(); From 683ccc9fcca358568605e7226ae65a9447135fce Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 14:47:00 -0500 Subject: [PATCH 05/22] fix(optimize): walk shared resource dictionaries and name-tree kids once (#5956) * fix(optimize): walk shared resource dictionaries and name-tree kids once * test(optimize): restore the file around the appended tests * address review: import ObjRef in the optimize audit module --- crates/optimize/src/audit.rs | 31 ++++++++++----- crates/optimize/src/links.rs | 8 +++- crates/optimize/tests/optimize.rs | 64 +++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 12 deletions(-) diff --git a/crates/optimize/src/audit.rs b/crates/optimize/src/audit.rs index 789bf2e89d..b06d76d6c7 100644 --- a/crates/optimize/src/audit.rs +++ b/crates/optimize/src/audit.rs @@ -9,7 +9,7 @@ use std::collections::{HashMap, HashSet}; -use pdfcraft_cos::{Dict, Document, MAX_DECODED, Object, XrefEntry}; +use pdfcraft_cos::{Dict, Document, MAX_DECODED, ObjRef, Object, XrefEntry}; /// The categories, in Acrobat's order. #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] @@ -91,6 +91,9 @@ const MAX_FORM_DEPTH: usize = 64; struct Audit<'a> { doc: &'a Document, owner: HashMap, + /// Resource dictionaries (by reference) already walked: one shared by many pages, forms or + /// appearances is walked once, as claiming is the same each time. + walked: HashSet, } impl Audit<'_> { @@ -124,6 +127,18 @@ impl Audit<'_> { o.map(|x| self.doc.resolve(x)).and_then(|x| x.as_dict().cloned()).unwrap_or_default() } + /// The resource dictionary `holder` points at (if it does), by kind: [`Self::resources`], + /// once per dictionary given by reference. + fn resources_of(&mut self, holder: Option<&Object>, depth: usize) { + if let Some(Object::Ref(r)) = holder + && !self.walked.insert(*r) + { + return; + } + let res = self.resolve_dict(holder); + self.resources(&res, depth); + } + /// A resource dictionary, by kind; `depth` counts the form XObjects it is nested in. fn resources(&mut self, res: &Dict, depth: usize) { let xobjects = self.resolve_dict(res.get(b"XObject")); @@ -135,8 +150,7 @@ impl Audit<'_> { // A form: its own resources first, by kind, then the rest of it. Owning it // first stops a form that draws itself. self.owner.insert(r.num, SpaceCategory::XObjectForms); - let inner = self.resolve_dict(s.dict.get(b"Resources")); - self.resources(&inner, depth + 1); + self.resources_of(s.dict.get(b"Resources"), depth + 1); self.claim(&Object::Dict(s.dict.clone()), SpaceCategory::XObjectForms); } _ => self.claim(x, SpaceCategory::XObjectForms), @@ -172,8 +186,7 @@ impl Audit<'_> { _ => Vec::new(), }; for dict in streams { - let res = self.resolve_dict(dict.get(b"Resources")); - self.resources(&res, 1); + self.resources_of(dict.get(b"Resources"), 1); } } } @@ -216,19 +229,17 @@ fn object_stream(doc: &Document, num: u32) -> Option { /// Audit the space a document uses; `file_len` is the size of the file on disk (the rest is /// overhead). pub fn audit_space(doc: &Document, file_len: u64) -> Vec { - let mut a = Audit { doc, owner: HashMap::new() }; + let mut a = Audit { doc, owner: HashMap::new(), walked: HashSet::new() }; let cat = doc.root().and_then(|r| doc.get(r).as_dict().cloned()).unwrap_or_default(); let pages = pdfcraft_model::pages(doc); // Order matters: the most specific owners first. Resources by kind before anything that // uses them: a content stream's dictionary may point at what it draws (PdfCraft's /PCAdded // records an added image), and form fields and comments draw with fonts and images too. for p in &pages { - let res = a.resolve_dict(p.dict.get(b"Resources")); - a.resources(&res, 0); + a.resources_of(p.dict.get(b"Resources"), 0); } let form = a.resolve_dict(cat.get(b"AcroForm")); - let form_resources = a.resolve_dict(form.get(b"DR")); - a.resources(&form_resources, 0); + a.resources_of(form.get(b"DR"), 0); for p in &pages { for x in annotations(doc, &p.dict) { a.appearance_resources(&x); diff --git a/crates/optimize/src/links.rs b/crates/optimize/src/links.rs index 5b74ad7023..bef69c43b7 100644 --- a/crates/optimize/src/links.rs +++ b/crates/optimize/src/links.rs @@ -52,8 +52,12 @@ fn tree_entries(doc: &Document, node: &Object) -> Vec<(Vec, Object)> { } } } - if let Some(kids) = d.get(b"Kids").map(|x| doc.resolve(x)).and_then(|x| x.as_array().cloned()) { - queue.extend(kids.into_iter().map(|k| (k, depth + 1))); + // A kids array shared by reference is read once, and only kids not yet seen are queued: + // nodes sharing one array can't multiply the queue. + if let Some(kids) = + d.get(b"Kids").filter(|k| !matches!(k, Object::Ref(r) if !seen.insert(*r))).map(|x| doc.resolve(x)).and_then(|x| x.as_array().cloned()) + { + queue.extend(kids.into_iter().filter(|k| !matches!(k, Object::Ref(r) if seen.contains(r))).map(|k| (k, depth + 1))); } } out diff --git a/crates/optimize/tests/optimize.rs b/crates/optimize/tests/optimize.rs index 7da6151deb..eee2e9570c 100644 --- a/crates/optimize/tests/optimize.rs +++ b/crates/optimize/tests/optimize.rs @@ -5613,6 +5613,70 @@ fn a_picture_whose_mask_is_not_a_stencil_is_left_alone() { } } +/// A resource dictionary shared by reference between thousands of pages is walked once by the +/// space audit, not once per page. +#[test] +fn space_audit_walks_a_shared_resource_dictionary_once() { + let mut doc = Document::new_empty(); + let img = image(&mut doc, 4, 4, 3, false, None); + let mut x = Dict::new(); + for i in 0..5_000 { + x.set(format!("Im{i}").into_bytes(), Object::Ref(img)); + } + let mut res = Dict::new(); + res.set(b"XObject".to_vec(), Object::Dict(x)); + let shared = doc.add(Object::Dict(res)); + let pages = doc.root().and_then(|r| doc.get(r).as_dict().and_then(|d| d.reference(b"Pages"))).unwrap(); + let mut kids = Vec::new(); + for _ in 0..2_000 { + let mut p = Dict::new(); + p.set(b"Type".to_vec(), Object::name("Page")); + p.set(b"Parent".to_vec(), Object::Ref(pages)); + p.set(b"MediaBox".to_vec(), Object::Array(vec![0.into(), 0.into(), 612.into(), 792.into()])); + p.set(b"Resources".to_vec(), Object::Ref(shared)); + kids.push(Object::Ref(doc.add(Object::Dict(p)))); + } + doc.update_dict(pages, |d| { + d.set(b"Count".to_vec(), Object::Int(kids.len() as i64)); + d.set(b"Kids".to_vec(), Object::Array(kids)); + }) + .unwrap(); + let started = std::time::Instant::now(); + let audit = audit_space(&doc, 1 << 20); + assert!(started.elapsed() < std::time::Duration::from_secs(5), "{:?}", started.elapsed()); + assert!(audit.iter().any(|u| u.category == SpaceCategory::Images && u.bytes > 0), "{audit:?}"); +} + +/// A name tree whose nodes all share one kids array (by reference) is read once per node, not +/// once per node per kid: the queue can't grow to the nodes times the kids. +#[test] +fn a_name_tree_sharing_one_kids_array_is_walked_once() { + let mut doc = Document::new_empty(); + let p = page(&mut doc, &[], "BT ET"); + let s = |t: &str| Object::String(pdfcraft_cos::PdfString::literal(t.as_bytes().to_vec())); + let fit = Object::Array(vec![Object::Ref(p), Object::name("Fit")]); + let kids = doc.add(Object::Array(Vec::new())); + let mut nodes = Vec::new(); + for i in 0..3_000 { + let mut node = Dict::new(); + node.set(b"Kids".to_vec(), Object::Ref(kids)); + node.set(b"Names".to_vec(), Object::Array(vec![s(&format!("d{i}")), fit.clone()])); + nodes.push(Object::Ref(doc.add(Object::Dict(node)))); + } + doc.set(kids, Object::Array(nodes)); + let mut tree = Dict::new(); + tree.set(b"Kids".to_vec(), Object::Ref(kids)); + let tree = doc.add(Object::Dict(tree)); + let mut names = Dict::new(); + names.set(b"Dests".to_vec(), Object::Ref(tree)); + let root = doc.root().unwrap(); + doc.update_dict(root, |c| c.set(b"Names".to_vec(), Object::Dict(names))).unwrap(); + let started = std::time::Instant::now(); + let report = optimize(&mut doc, &Settings::default()).unwrap(); + assert!(started.elapsed() < std::time::Duration::from_secs(10), "{:?}", started.elapsed()); + assert_eq!(report.unreferenced_dests, 3_000, "every node's destination was found, once"); +} + /// Pictures run on several threads: what comes out doesn't depend on how many. #[test] fn the_same_file_comes_out_with_any_number_of_threads() { From fba22fc73840986b7e7307d4844680868b29d667 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 14:47:15 -0500 Subject: [PATCH 06/22] fix(optimize): the fragment search charges every operator of a group it reads (#6095) * fix(optimize): the fragment search charges every operator of a group it reads * address review: conflict markers removed; the fragment search charges every operator it reads to the production allowance * address review: group reads are charged before a parse failure and count all four operators --- crates/optimize/src/fragments.rs | 98 +++++++++++++++++++++++++------- 1 file changed, 78 insertions(+), 20 deletions(-) diff --git a/crates/optimize/src/fragments.rs b/crates/optimize/src/fragments.rs index aa52c20750..9ccca4791f 100644 --- a/crates/optimize/src/fragments.rs +++ b/crates/optimize/src/fragments.rs @@ -30,17 +30,21 @@ pub const MIN_FRAGMENTS: usize = 4; /// Most pixels a merged image may have (memory). const MAX_PIXELS: u64 = 80_000_000; -/// Fragment groups a page's search for runs may read per content operator, and at least. A run -/// that is not one picture is searched again from each of its groups, so without a limit images -/// drawn over one another take time quadratic in their number; past it, the rest of the page is -/// left as it is. -const SCAN_PER_OP: usize = 16; +/// Content operators a page's search for runs may read per content operator of the page, and at +/// least. A run that is not one picture is searched again from each of its groups, so without a +/// limit images drawn over one another take time quadratic in their number; past it, the rest of +/// the page is left as it is. Every operator of a group counts (its `cm` operators are composed +/// each time), not just the group; a group of four operators costs four, so the allowance is 16 +/// groups per operator, as before, and groups with many `cm` operators pay for them. +const SCAN_PER_OP: usize = 64; const MIN_SCAN: usize = 1 << 16; #[cfg(test)] thread_local! { /// Fragment groups read, for the test that keeps a page's search linear in its content. static GROUP_READS: std::cell::Cell = const { std::cell::Cell::new(0) }; + /// Operators group reads walked, for the test that keeps the search linear in operators. + static OP_READS: std::cell::Cell = const { std::cell::Cell::new(0) }; } /// One fragment: its image, its box in the coordinates the run is drawn in, and its size. @@ -53,21 +57,44 @@ struct Fragment { /// The ops `q`, one or more `cm`, `Do`, `Q` starting at `i`: the index after them, the matrix /// and the name drawn. -fn group_at(ops: &[Op], i: usize) -> Option<(usize, Matrix, Vec)> { +fn group_at(ops: &[Op], i: usize, reads: &std::cell::Cell) -> Option<(usize, Matrix, Vec)> { #[cfg(test)] GROUP_READS.with(|c| c.set(c.get().saturating_add(1))); - if !ops.get(i)?.is("q") { + let mut looked = 0usize; + let found = group_scan(ops, i, &mut looked); + // Every operator looked at is charged to the page's search allowance, whether or not the + // group turns out to be one. + reads.set(reads.get().saturating_add(looked)); + #[cfg(test)] + OP_READS.with(|c| c.set(c.get().saturating_add(looked))); + found +} + +/// [`group_at`]'s search, counting in `looked` each operator it examines. +fn group_scan(ops: &[Op], i: usize, looked: &mut usize) -> Option<(usize, Matrix, Vec)> { + let q = ops.get(i)?; + *looked += 1; + if !q.is("q") { return None; } let mut j = i + 1; let mut m = Matrix([1.0, 0.0, 0.0, 1.0, 0.0, 0.0]); while let Some(op) = ops.get(j).filter(|o| o.is("cm")) { + *looked += 1; m = Matrix::from_operands(&op.operands)?.then(&m); j += 1; } - let draw = ops.get(j).filter(|o| o.is("Do"))?; + let draw = ops.get(j)?; + *looked += 1; + if !draw.is("Do") { + return None; + } let name = draw.name(0)?.to_vec(); - ops.get(j + 1).filter(|o| o.is("Q"))?; + let end = ops.get(j + 1)?; + *looked += 1; + if !end.is("Q") { + return None; + } (j > i + 1).then_some((j + 2, m, name)) } @@ -152,7 +179,7 @@ fn stacked(run: &mut [Fragment]) -> Option>> { /// The fragment groups from `i` on (at most `limit`) while they form a picture drawn row by row /// or strip by strip in one direction, up to a gap or a jump. -fn drawn_in_sequence(ops: &[Op], i: usize, limit: usize, xobjects: &Dict, doc: &Document) -> Vec { +fn drawn_in_sequence(ops: &[Op], i: usize, limit: usize, xobjects: &Dict, doc: &Document, reads: &std::cell::Cell) -> Vec { let mut run: Vec = Vec::new(); let mut row_x = [0.0f64, 0.0]; let mut cur_x = [0.0f64, 0.0]; @@ -163,7 +190,7 @@ fn drawn_in_sequence(ops: &[Op], i: usize, limit: usize, xobjects: &Dict, doc: & let mut complete = 0usize; let mut j = i; while run.len() < limit { - let Some((next, m, name)) = group_at(ops, j) else { break }; + let Some((next, m, name)) = group_at(ops, j, reads) else { break }; let Some((image, width, height, ..)) = fragment_image(doc, xobjects, &name) else { break }; let [a, _, _, d, e, f] = m.0; let f2 = Fragment { image, rect: [e, f, e + a, f + d], width, height }; @@ -240,10 +267,10 @@ fn drawn_in_sequence(ops: &[Op], i: usize, limit: usize, xobjects: &Dict, doc: & } /// The op index after the first `count` fragment groups from `i`. -fn run_end(ops: &[Op], i: usize, count: usize) -> usize { +fn run_end(ops: &[Op], i: usize, count: usize, reads: &std::cell::Cell) -> usize { let mut j = i; for _ in 0..count { - match group_at(ops, j) { + match group_at(ops, j, reads) { Some((next, ..)) => j = next, None => break, } @@ -386,9 +413,9 @@ fn merge_page( let mut out: Vec = Vec::with_capacity(ops.len()); let (mut i, mut merged, mut k) = (0, 0, 0); let budget = ops.len().saturating_mul(SCAN_PER_OP).max(MIN_SCAN); - let mut scanned = 0usize; + let reads = std::cell::Cell::new(0usize); while i < ops.len() { - if scanned > budget { + if reads.get() > budget { out.extend(ops.get(i..).unwrap_or_default().iter().cloned()); break; } @@ -396,7 +423,7 @@ fn merge_page( let mut run = Vec::new(); let mut kind: Option<(Object, usize)> = None; let mut j = i; - while let Some((next, m, name)) = group_at(&ops, j) { + while let Some((next, m, name)) = group_at(&ops, j, &reads) { let Some((image, width, height, cs, n)) = fragment_image(doc, &xobjects, &name) else { break }; let [a, b, c, d, e, f] = m.0; if b.abs() > 1e-9 || c.abs() > 1e-9 || !(a > 0.0 && d > 0.0) { @@ -410,13 +437,11 @@ fn merge_page( run.push(Fragment { image, rect: [e, f, e + a, f + d], width, height }); j = next; } - scanned = scanned.saturating_add(run.len()).saturating_add(1); // Not one picture as a whole: a prefix drawn row by row or strip by strip may be. let mut rows = if run.len() >= MIN_FRAGMENTS { stacked(&mut run) } else { None }; if run.len() >= MIN_FRAGMENTS && rows.is_none() { - run = drawn_in_sequence(&ops, i, run.len(), &xobjects, doc); - j = run_end(&ops, i, run.len()); - scanned = scanned.saturating_add(run.len().saturating_mul(2)).saturating_add(2); + run = drawn_in_sequence(&ops, i, run.len(), &xobjects, doc, &reads); + j = run_end(&ops, i, run.len(), &reads); rows = stacked(&mut run); } let Some(((cs, n), rows)) = kind.zip(rows).filter(|_| run.len() >= MIN_FRAGMENTS) else { @@ -773,4 +798,37 @@ mod tests { assert_eq!((merged, report.merged_images), (0, 0)); } } + + /// The same with many `cm` operators in each group: the search reads a bounded number of operators + /// for each one of the page, however the matrices are spread over the groups. + #[test] + fn groups_with_many_matrices_are_charged_for_each_operator() { + let mut doc = Document::new_empty(); + let mut image = Dict::new(); + for (k, v) in + [("Subtype", Object::name("Image")), ("Width", Object::Int(1)), ("Height", Object::Int(1)), ("BitsPerComponent", Object::Int(8))] + { + image.set(k.as_bytes().to_vec(), v); + } + image.set(b"ColorSpace".to_vec(), Object::name("DeviceGray")); + let image = doc.add(Object::Stream(Stream::from_raw(image, vec![0]))); + let groups = 400; + let content = doc.add(Object::Stream(Stream::flate( + Dict::new(), + format!("q 10 0 0 10 0 0 cm {}/Im Do Q\n", "1 0 0 1 0 0 cm ".repeat(49)).repeat(groups).as_bytes(), + ))); + let mut xobjects = Dict::new(); + xobjects.set(b"Im".to_vec(), Object::Ref(image)); + let mut resources = Dict::new(); + resources.set(b"XObject".to_vec(), Object::Dict(xobjects)); + let mut page = Dict::new(); + page.set(b"Contents".to_vec(), Object::Ref(content)); + page.set(b"Resources".to_vec(), Object::Dict(resources)); + let page = doc.add(Object::Dict(page)); + OP_READS.with(|c| c.set(0)); + let merged = merge_page(&mut doc, page, &mut Report::default(), &mut |_| true, &mut ImageBudget::with_work(u64::MAX)); + assert_eq!(merged.ok(), Some(0)); + let reads = OP_READS.with(|c| c.get()); + assert!(reads <= 80 * 53 * groups, "{reads} operator reads"); + } } From 87aebd1b24618fa822d994f32209bff08e974056 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 14:47:29 -0500 Subject: [PATCH 07/22] 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 From 13d63673857d10138c0fa4302c3bcc75cf50171c Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 14:47:42 -0500 Subject: [PATCH 08/22] fix(redact): read each Type 3 glyph program once per cleanup (#5993) clean_type3_font decoded and parsed every surviving glyph program of a font, for every font. Many small font dictionaries sharing one compressed CharProc multiplied that work without multiplying the stored data, with a fresh allowance for each decode. The resource names each indirect glyph program uses are now cached by object across fonts (resource-name interpretation stays per font, since each font prunes its own /Resources), and all decoding spends one cleanup-wide DecodeBudget; past it the cleanup fails closed. --- .../redact/src/tests/surviving_references.rs | 29 ++++++++++++++ crates/redact/src/type3.rs | 39 +++++++++++++++++-- 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/crates/redact/src/tests/surviving_references.rs b/crates/redact/src/tests/surviving_references.rs index d02e76832e..bec850e302 100644 --- a/crates/redact/src/tests/surviving_references.rs +++ b/crates/redact/src/tests/surviving_references.rs @@ -390,6 +390,35 @@ fn type3_font_used_in_a_form_xobject_has_its_redacted_glyph_removed() { assert!(!full_save_holds(&doc, b"SECRET_FORM_GLYPH_PAYLOAD"), "redacted glyph program survived"); } +/// Type 3 cleanup decoded a glyph program again for every font listing it: 2 000 fonts sharing +/// one large program read it 2 000 times. It is read once now, and the cleanup still keeps the +/// glyph every font shows. +#[test] +fn a_glyph_program_many_type3_fonts_share_is_read_once() { + const FONTS: usize = 2_000; + let names: String = (0..FONTS).map(|i| format!("/T{i} {} 0 R ", 6 + i)).collect(); + let shows: String = (0..FONTS).map(|i| format!("/T{i} 10 Tf (A) Tj ")).collect(); + let mut objs = vec![ + b"<< /Type /Catalog /Pages 2 0 R >>".to_vec(), + b"<< /Type /Pages /Kids [3 0 R] /Count 1 >>".to_vec(), + format!("<< /Type /Page /Parent 2 0 R /MediaBox [0 0 300 300] /Contents 4 0 R /Resources << /Font << {names}>> >> >>").into_bytes(), + stream("", format!("BT 10 100 Td {shows}ET").as_bytes()), + stream("", [b"SHARED_GLYPH ".as_slice(), &b"0 0 m 1 1 l S ".repeat(50_000)].concat()), + ]; + objs.extend((0..FONTS).map(|_| { + b"<< /Type /Font /Subtype /Type3 /FontBBox [0 0 1000 1000] /FontMatrix [0.001 0 0 0.001 0 0] /FirstChar 65 /LastChar 65 /Widths [0] /Encoding << /Differences [65 /A] >> /CharProcs << /A 5 0 R >> >>".to_vec() + })); + let mut doc = pdf(objs); + // The glyphs are drawn at x 10 with no width, so a mark away from them leaves them all. + mark(&mut doc, 0, &[[200.0, 200.0, 250.0, 250.0]], ""); + let started = std::time::Instant::now(); + apply(&mut doc, None).unwrap(); + assert!(started.elapsed() < std::time::Duration::from_secs(60), "{:?}", started.elapsed()); + let font = doc.get(ObjRef::new(6, 0)); + let procs = doc.resolve(font.as_dict().unwrap().get(b"CharProcs").unwrap()); + assert!(procs.as_dict().unwrap().contains(b"A"), "the glyph every font shows was unlinked"); +} + /// A Type 3 font with glyphs A (its program object 7) and B (object 8), at object 6. const TYPE3: &str = "<< /Type /Font /Subtype /Type3 /FontBBox [0 -200 1000 800] /FontMatrix [0.001 0 0 0.001 0 0] /FirstChar 65 /LastChar 66 /Widths [500 500] /Encoding << /Differences [65 /A /B] >> /CharProcs << /A 7 0 R /B 8 0 R >> >>"; diff --git a/crates/redact/src/type3.rs b/crates/redact/src/type3.rs index 11d5a76f6a..1817fb3c39 100644 --- a/crates/redact/src/type3.rs +++ b/crates/redact/src/type3.rs @@ -3,7 +3,7 @@ use std::collections::{HashMap, HashSet}; use pdfcraft_content::Pieces; -use pdfcraft_cos::{CosError, Dict, Document, ObjRef, Object}; +use pdfcraft_cos::{CosError, DecodeBudget, Dict, Document, ObjRef, Object}; use crate::{RedactError, page_streams}; @@ -130,6 +130,25 @@ struct UsedInGlyphs { properties: HashSet>, } +impl UsedInGlyphs { + fn merge(&mut self, other: &UsedInGlyphs) { + self.xobjects.extend(other.xobjects.iter().cloned()); + self.colorspaces.extend(other.colorspaces.iter().cloned()); + self.patterns.extend(other.patterns.iter().cloned()); + self.shadings.extend(other.shadings.iter().cloned()); + self.extgstates.extend(other.extgstates.iter().cloned()); + self.fonts.extend(other.fonts.iter().cloned()); + self.properties.extend(other.properties.iter().cloned()); + } +} + +/// What one Type 3 cleanup reads of glyph programs: the resource names each indirect program +/// uses, read once however many fonts share it, and one decoding allowance for all of them. +struct GlyphReads { + used: HashMap, + budget: DecodeBudget, +} + fn collect_used_resources_in_stream(data: &[u8], used: &mut UsedInGlyphs) -> Result<(), CosError> { // One operator at a time, so a glyph program of millions of them is never held whole. A byte // that can't be read anywhere in it still refuses, once the loop has read it all. @@ -731,9 +750,10 @@ pub(crate) fn unlink_type3(doc: &mut Document) -> Result<(), RedactError> { } // For each Type 3 font: unlink unused glyphs from CharProcs and prune exclusive resources. + let mut reads = GlyphReads { used: HashMap::new(), budget: DecodeBudget::page() }; for (font_key, font_dict) in &type3_fonts { let surviving = used_glyphs.get(font_key).cloned().unwrap_or_default(); - clean_type3_font(doc, &holders, &form_holders, font_key, font_dict, &surviving)?; + clean_type3_font(doc, &holders, &form_holders, font_key, font_dict, &surviving, &mut reads)?; } Ok(()) @@ -808,6 +828,7 @@ fn clean_type3_font( font_key: &FontKey, font_dict: &Dict, surviving: &HashSet>, + reads: &mut GlyphReads, ) -> Result<(), RedactError> { let Some(char_procs_val) = font_dict.get(b"CharProcs") else { return Err(CosError::Syntax { offset: 0, detail: "redaction could not establish safe Type 3 font cleanup: missing CharProcs".into() }.into()); @@ -851,6 +872,10 @@ fn clean_type3_font( let mut used_in_glyphs = UsedInGlyphs::default(); for (_, proc_val) in char_procs.iter() { + if let Some(used) = proc_val.as_ref().and_then(|r| reads.used.get(&r)) { + used_in_glyphs.merge(used); + continue; + } let resolved = doc.resolve(proc_val); let Object::Stream(s) = &*resolved else { return Err(CosError::Syntax { @@ -859,8 +884,14 @@ fn clean_type3_font( } .into()); }; - let data = doc.with_direct_parms(s).decoded()?; - collect_used_resources_in_stream(&data, &mut used_in_glyphs)?; + // Past the allowance the cleanup can't be shown complete, and the redaction is refused. + let data = reads.budget.decode(&doc.with_direct_parms(s))?; + let mut used = UsedInGlyphs::default(); + collect_used_resources_in_stream(&data, &mut used)?; + used_in_glyphs.merge(&used); + if let Some(r) = proc_val.as_ref() { + reads.used.insert(r, used); + } } if prune_type3_font_resources(doc, &mut f_res, &used_in_glyphs) { From b52bc303281b58eb0bd28134cc7a1dc764177007 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 14:47:56 -0500 Subject: [PATCH 09/22] fix(annot): count ink points as strokes resolve and cap the eraser's samples (#6001) * fix(annot): count ink points as strokes resolve and cap the eraser's samples Resizing and erasing a drawing resolved every /InkList entry without a total, so one stroke array listed many times expanded freely; the eraser densified every segment into up to 2,000 samples with no overall limit. * review: refuse an eraser result that exceeds the stored ink point cap * review: say the erasure would make the drawing too large * address review: the eraser counts each stroke's final point against its sample total --- crates/annot/src/modify.rs | 69 ++++++++++++++++++++++++++------------ crates/annot/src/tests.rs | 67 ++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 22 deletions(-) diff --git a/crates/annot/src/modify.rs b/crates/annot/src/modify.rs index ae2222b387..6566337ca1 100644 --- a/crates/annot/src/modify.rs +++ b/crates/annot/src/modify.rs @@ -857,19 +857,38 @@ pub fn set_points(doc: &mut Document, page: usize, index: usize, points: &[[f64; /// Scale a drawing's strokes into `rect`. The margin between the strokes and the old `/Rect` (half /// the line width and a little more) stays the same on each side, so the line isn't clipped. +/// Most stroke points a stored drawing may have for resizing or erasing it. Counted as each +/// stroke is resolved, so one stroke array referenced again and again can't expand past it. +const MAX_STORED_INK_POINTS: usize = 1_000_000; +/// Most points the eraser densifies a drawing into before testing them against its path. +const MAX_ERASE_SAMPLES: usize = 4_000_000; + +/// The strokes of an ink annotation's `/InkList`, at most [`MAX_STORED_INK_POINTS`] points. +fn ink_strokes(doc: &Document, d: &Dict) -> Result>, AnnotError> { + let too_many = || AnnotError::Invalid(format!("the drawing has more than {MAX_STORED_INK_POINTS} points")); + let list = d.get(b"InkList").map(|l| doc.resolve(l)); + let mut points = 0usize; + let mut strokes = Vec::new(); + for s in list.as_deref().and_then(Object::as_array).map(Vec::as_slice).unwrap_or_default() { + let stroke = doc.resolve(s); + let Some(a) = stroke.as_array() else { + strokes.push(Vec::new()); + continue; + }; + // Charged before the coordinates are read: a shared array costs each time it is listed. + points = points.saturating_add(a.len() / 2); + if points > MAX_STORED_INK_POINTS { + return Err(too_many()); + } + let v: Vec = a.iter().filter_map(|x| doc.resolve(x).as_f64()).collect(); + strokes.push(v.as_chunks::<2>().0.iter().map(|p| [p[0], p[1]]).collect()); + } + Ok(strokes) +} + fn resize_ink(doc: &mut Document, r: ObjRef, rect: [f64; 4], meta: &Meta) -> Result<(), AnnotError> { let d = annot_dict(doc, r); - let strokes: Vec> = d - .get(b"InkList") - .map(|l| doc.resolve(l)) - .and_then(|l| l.as_array().cloned()) - .unwrap_or_default() - .iter() - .map(|s| { - let v: Vec = doc.resolve(s).as_array().map(|a| a.iter().filter_map(|x| doc.resolve(x).as_f64()).collect()).unwrap_or_default(); - v.as_chunks::<2>().0.iter().map(|p| [p[0], p[1]]).collect() - }) - .collect(); + let strokes = ink_strokes(doc, &d)?; let Some(inner) = bounds(strokes.iter().flatten().copied()) else { return Err(AnnotError::Invalid("the drawing has no strokes".into())); }; @@ -1475,17 +1494,7 @@ pub fn erase_ink(doc: &mut Document, page: usize, index: usize, path: &[[f64; 2] if path.is_empty() || !path.iter().all(|p| finite(p)) || !radius.is_finite() || radius <= 0.0 { return Err(AnnotError::Invalid("invalid eraser path".into())); } - let strokes: Vec> = d - .get(b"InkList") - .map(|l| doc.resolve(l)) - .and_then(|l| l.as_array().cloned()) - .unwrap_or_default() - .iter() - .map(|s| { - let v: Vec = doc.resolve(s).as_array().map(|a| a.iter().filter_map(|x| doc.resolve(x).as_f64()).collect()).unwrap_or_default(); - v.as_chunks::<2>().0.iter().map(|p| [p[0], p[1]]).collect() - }) - .collect(); + let strokes = ink_strokes(doc, &d)?; // Distance from a point to the eraser's path (segments, or a single point). let near = |p: [f64; 2]| -> bool { if path.len() == 1 { @@ -1501,17 +1510,27 @@ pub fn erase_ink(doc: &mut Document, page: usize, index: usize, path: &[[f64; 2] }; let mut kept: Vec> = Vec::new(); let mut erased = false; + let mut samples = 0usize; for s in &strokes { // Densify so long segments are cut where the eraser crosses them. let mut pts: Vec<[f64; 2]> = Vec::new(); for w in s.windows(2) { let n = ((w[1][0] - w[0][0]).hypot(w[1][1] - w[0][1]) / (radius / 2.0)).ceil().clamp(1.0, 2000.0) as usize; + samples = samples.saturating_add(n); + if samples > MAX_ERASE_SAMPLES { + return Err(AnnotError::Invalid("the drawing is too large to erase at this eraser size".into())); + } for k in 0..n { let t = k as f64 / n as f64; pts.push([w[0][0] + (w[1][0] - w[0][0]) * t, w[0][1] + (w[1][1] - w[0][1]) * t]); } } if let Some(last) = s.last() { + // The final point is a sample too. + samples = samples.saturating_add(1); + if samples > MAX_ERASE_SAMPLES { + return Err(AnnotError::Invalid("the drawing is too large to erase at this eraser size".into())); + } pts.push(*last); } let mut run: Vec<[f64; 2]> = Vec::new(); @@ -1537,6 +1556,12 @@ pub fn erase_ink(doc: &mut Document, page: usize, index: usize, path: &[[f64; 2] delete_annotation(doc, page, index)?; return Ok(true); } + // The retained runs are stored as the new `/InkList`: past the cap, writing them would leave a + // drawing that later `resize_ink` and `erase_ink` refuse to read. + let points: usize = kept.iter().map(Vec::len).sum(); + if points > MAX_STORED_INK_POINTS { + return Err(AnnotError::Invalid(format!("erasing would leave a drawing with more than {MAX_STORED_INK_POINTS} points"))); + } let width = d.get(b"BS").and_then(|b| b.as_dict()).and_then(|b| b.get(b"W")).and_then(|w| w.as_f64()).unwrap_or(1.0); let rect = grow(bounds(kept.iter().flatten().copied()).unwrap_or_default(), width / 2.0 + 1.0); doc.update_dict(r, |d| { diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index ce5a8c1604..980959d4d9 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -4797,6 +4797,73 @@ fn freehand_highlight_is_compatible_ink_with_marker_blending() { } } +/// A drawing whose /InkList lists one stroke array over and over is refused once its points +/// pass the cap, counted as each stroke is resolved, before they are collected or scaled. +#[test] +fn ink_points_are_counted_as_strokes_resolve() { + let numbers: String = (0..2000).map(|k| format!(" {}", k % 100)).collect(); + let refs = " 5 0 R".repeat(501); + let doc_with = |refs: &str| { + doc_of(&[ + "<< /Type /Catalog /Pages 2 0 R >>".into(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 612 792] >>".into(), + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>".into(), + format!("<< /Type /Annot /Subtype /Ink /Rect [0 0 100 100] /InkList [{refs}] >>"), + format!("[{numbers}]"), + ]) + }; + // 501 × 1,000 points is under the cap: the points are read (this bare drawing then has no + // appearance it can be redrawn with, which is another matter). + let mut doc = doc_with(&refs); + let under = set_rect(&mut doc, 0, 0, [0.0, 0.0, 50.0, 50.0], &meta("")); + assert!(!matches!(&under, Err(AnnotError::Invalid(m)) if m.contains("points")), "{under:?}"); + // 1,001 × 1,000 points is over it. + let mut doc = doc_with(&" 5 0 R".repeat(1001)); + match set_rect(&mut doc, 0, 0, [0.0, 0.0, 50.0, 50.0], &meta("")) { + Err(AnnotError::Invalid(m)) => assert!(m.contains("more than 1000000 points"), "{m}"), + other => panic!("refused: {other:?}"), + } + match erase_ink(&mut doc, 0, 0, &[[1.0, 1.0]], 1.0, &meta("")) { + Err(AnnotError::Invalid(m)) => assert!(m.contains("more than 1000000 points"), "{m}"), + other => panic!("refused: {other:?}"), + } +} + +/// The eraser's densified samples have a total: a tiny eraser over long strokes is refused +/// instead of interpolating millions of points. +#[test] +fn the_eraser_densifies_within_a_total() { + let zigzag: String = (0..2100).map(|k| format!(" {} {}", if k % 2 == 0 { 0 } else { 5000 }, k)).collect(); + let mut doc = doc_of(&[ + "<< /Type /Catalog /Pages 2 0 R >>".into(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 612 792] >>".into(), + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>".into(), + format!("<< /Type /Annot /Subtype /Ink /Rect [0 0 5000 2100] /InkList [[{zigzag}]] >>"), + ]); + match erase_ink(&mut doc, 0, 0, &[[1.0, 1.0]], 0.01, &meta("")) { + Err(AnnotError::Invalid(m)) => assert!(m.contains("too large to erase"), "{m}"), + other => panic!("refused: {other:?}"), + } +} + +/// Each stroke's final point is a sample too: 500,000 strokes of two points at 8 samples a segment +/// are 4,000,000 densified samples and as many final points again, past the total. +#[test] +fn the_eraser_counts_each_strokes_final_point() { + let refs = " 5 0 R".repeat(500_000); + let mut doc = doc_of(&[ + "<< /Type /Catalog /Pages 2 0 R >>".into(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 612 792] >>".into(), + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>".into(), + format!("<< /Type /Annot /Subtype /Ink /Rect [0 0 100 100] /InkList [{refs}] >>"), + "[0 0 8 0]".into(), + ]); + match erase_ink(&mut doc, 0, 0, &[[1.0, 1.0]], 2.0, &meta("")) { + Err(AnnotError::Invalid(m)) => assert!(m.contains("too large to erase"), "{m}"), + other => panic!("refused: {other:?}"), + } +} + /// A 3D annotation is PDF 1.6 and a PRC model 1.7 (ISO 32000-2 §13.6): an older document's /// catalog says so, and a newer one is left as it is. #[test] From ee367a4a1c6087c07003cdf9d3999bc4281a997a Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 14:48:10 -0500 Subject: [PATCH 10/22] fix(xfa): bound the name and path bytes data nodes and queued instances copy (#6032) * fix(xfa): bound the name and path bytes data nodes and queued instances copy * review: bound the name bytes of a whole data path and report each cap separately * address review: the queued-instance allowance counts path segment buffers and the dedup copy * address review: the queued path's spare capacity is counted --- crates/xfa/src/data.rs | 16 +++++++++++++--- crates/xfa/src/data/build.rs | 2 +- crates/xfa/src/data/write.rs | 24 +++++++++++++++++++++++- crates/xfa/src/tests.rs | 25 +++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 5 deletions(-) diff --git a/crates/xfa/src/data.rs b/crates/xfa/src/data.rs index 34be028d89..4e7de4af74 100644 --- a/crates/xfa/src/data.rs +++ b/crates/xfa/src/data.rs @@ -148,13 +148,21 @@ pub enum DataOp { impl DataNode { /// The node at `path`, made (with empty siblings before it) where missing; `None` past /// the index cap. - fn ensure(&mut self, path: &[(String, usize)]) -> Option<&mut DataNode> { + fn ensure(&mut self, path: &[(String, usize)], budget: &mut usize) -> Option<&mut DataNode> { let mut node = self; for (name, idx) in path { if *idx >= MAX_INDEX { return None; } let have = node.children.iter().filter(|c| &c.name == name).count(); + // Every skipped sibling owns a copy of the name: bounded as `Build::at` bounds a write. + // The budget is the whole path's, not each level's: a deep path that is affordable at + // every level taken alone would otherwise allocate its way past the bound. + let need = (*idx + 1).saturating_sub(have).saturating_mul(name.len()); + if need > *budget { + return None; + } + *budget -= need; for _ in have..=*idx { node.children.push(DataNode { name: name.clone(), ..Default::default() }); } @@ -166,9 +174,11 @@ impl DataNode { /// Apply `op` to this tree (the root is the `xfa:data` element), as [`write_data_ops`] /// will to the packet, so scripts that run before it is written see the change. pub fn apply(&mut self, op: &DataOp) { + // One budget for the op's whole path, as `Build::at` bounds a write. + let mut budget = build::MAX_NEW_NAME_BYTES; match op { DataOp::Value { path, text } => { - if let Some(n) = self.ensure(path) { + if let Some(n) = self.ensure(path, &mut budget) { // A multi-select list's picks, one `value` each (see `value_at`), told apart // as the packet writer does: `value` leaves only, or a group holding none. let picks = if n.children.is_empty() { n.group } else { n.children.iter().all(|c| c.name == "value" && c.children.is_empty()) }; @@ -189,7 +199,7 @@ impl DataNode { if let Some(&last) = count.checked_sub(1).as_ref() { let mut path = parent.clone(); path.push((name.clone(), last.min(MAX_INDEX.saturating_sub(1)))); - let _ = self.ensure(&path); + let _ = self.ensure(&path, &mut budget); } } DataOp::Remove(path) => { diff --git a/crates/xfa/src/data/build.rs b/crates/xfa/src/data/build.rs index 98e34f0f49..e90904d60c 100644 --- a/crates/xfa/src/data/build.rs +++ b/crates/xfa/src/data/build.rs @@ -38,7 +38,7 @@ pub(super) const MAX_INDEX: usize = 10_000; /// Most bytes of names the nodes one write creates hold together: every sibling an index /// skips over gets its own copy of the name. -const MAX_NEW_NAME_BYTES: usize = 4 << 20; +pub(super) const MAX_NEW_NAME_BYTES: usize = 4 << 20; /// What one write may still create: nodes and bytes of their names. pub(super) struct NewNodes { diff --git a/crates/xfa/src/data/write.rs b/crates/xfa/src/data/write.rs index 4667036fe5..554685e1ad 100644 --- a/crates/xfa/src/data/write.rs +++ b/crates/xfa/src/data/write.rs @@ -82,6 +82,11 @@ pub fn remove_data_instance(doc: &mut Document, path: &DataPath) -> Result) let mut run: Vec = Vec::new(); // Instances queued in this run, by parent and name: asking for more only queues the rest. let mut queued: HashMap<(&DataPath, &str), usize> = HashMap::new(); - let (mut ensured, mut capped) = (0usize, false); + let (mut ensured, mut ensured_bytes, mut capped, mut capped_bytes) = (0usize, 0usize, false, false); let flush = |run: &mut Vec, steps: &mut Vec>, removes: &mut Vec>| { if !run.is_empty() { let mut seen = std::collections::HashSet::new(); @@ -117,12 +122,24 @@ pub fn write_data_ops(doc: &mut Document, ops: &[DataOp], reuse: Option) } DataOp::Instances { parent, name, count } => { let done = queued.entry((parent, name.as_str())).or_insert(0); + // The path (its segments' buffers and their text) is held by the queued step, which a + // clone then `push` can leave with up to twice the room, and cloned again into + // `flush`'s dedup set, beside the label and the step itself. + let segment = std::mem::size_of::<(String, usize)>(); + let path_bytes = + parent.iter().fold(segment.saturating_add(name.len()), |n, (p, _)| n.saturating_add(segment).saturating_add(p.len())); + let per_instance = path_bytes.saturating_mul(3).saturating_add(name.len()).saturating_add(16 + std::mem::size_of::()); while *done < (*count).min(MAX_INDEX) { if ensured >= MAX_ENSURED { capped = true; break; } + if ensured_bytes.saturating_add(per_instance) > MAX_ENSURED_BYTES { + capped_bytes = true; + break; + } ensured += 1; + ensured_bytes += per_instance; #[cfg(test)] ENSURED.with(|n| n.set(n.get() + 1)); let k = *done; @@ -154,6 +171,11 @@ pub fn write_data_ops(doc: &mut Document, ops: &[DataOp], reuse: Option) if capped { written.warnings.push(format!("XFA data instances not written past {MAX_ENSURED}: the rows were added and removed too often")); } + if capped_bytes { + written + .warnings + .push(format!("XFA data instances not written past {MAX_ENSURED_BYTES} bytes of names: the rows were added and removed too often")); + } Ok(written) } diff --git a/crates/xfa/src/tests.rs b/crates/xfa/src/tests.rs index 682887e673..27e6522f26 100644 --- a/crates/xfa/src/tests.rs +++ b/crates/xfa/src/tests.rs @@ -7114,6 +7114,31 @@ fn a_container_is_as_wide_as_a_turned_draw_stands() { } } +/// Every sibling a SOM index skips owns a copy of the element name: the in-memory builder refuses +/// a path whose copies would pass the name-byte bound the writer holds itself to, and creates +/// nothing for it. +#[test] +fn the_in_memory_data_builder_bounds_copied_name_bytes() { + let mut root = DataNode::default(); + // 1 KiB x 9,999 skipped siblings is about 10 MB, over the 4 MiB bound. + let long = "n".repeat(1 << 10); + root.apply(&DataOp::Value { path: vec![(long.clone(), 9_999)], text: "x".into() }); + assert!(root.children.is_empty()); + // Within the bound, the same shape still builds (and keeps indexes). + root.apply(&DataOp::Value { path: vec![(long, 100)], text: "x".into() }); + assert_eq!(root.children.len(), 101); +} + +/// Each queued instance owns a copy of its parent path: a long ancestor stops the queue at the +/// byte allowance, reported as capped, instead of being multiplied by the instance count. +#[test] +fn queued_instances_are_bounded_by_the_path_bytes_they_copy() { + let mut doc = Document::new_empty(); + let parent: DataPath = vec![("a".repeat(16 << 20), 0), ("b".repeat(16 << 20), 0)]; + let written = crate::data::write_data_ops(&mut doc, &[DataOp::Instances { parent, name: "row".into(), count: 100 }], None).unwrap(); + assert!(written.warnings.iter().any(|w| w.contains("not written past")), "{:?}", written.warnings); +} + /// A page break splits an empty multi-line field that grows with its text (XFA 3.3 pp. 291–292: /// text splits between lines, and a field's `keep intact` is `none`): it starts where it is, its /// widget runs to the bottom of the content area, and the rest of its box, closed on every side, From 1e91e8a93ec8b8107f2c1f4f654c0a7629b07119 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 14:48:24 -0500 Subject: [PATCH 11/22] fix(forms): a read-only radio button is not picked or left through set_value (#6038) * fix(forms): a read-only radio button is not picked or left through set_value * address review: use /V as the selection in the read-only radio check, keep reset going * address review: radio read-only checks judge each button by the state it would get, and cover FieldValue::Widget --- crates/forms/src/tests.rs | 62 ++++++++++++++++++++++++++++++++++++ crates/forms/src/values.rs | 65 ++++++++++++++++++++++++++++++++------ 2 files changed, 117 insertions(+), 10 deletions(-) diff --git a/crates/forms/src/tests.rs b/crates/forms/src/tests.rs index 75dd075889..a2a87f3a73 100644 --- a/crates/forms/src/tests.rs +++ b/crates/forms/src/tests.rs @@ -6802,6 +6802,48 @@ fn locked_button_export_edits_leave_the_document_unchanged() { } } +/// A radio button that is read-only on its own (its own `/Ff`) can't be picked, and a selected one +/// can't be switched away from, though the group as a whole is writable. +#[test] +fn a_read_only_radio_button_is_neither_picked_nor_left() { + let lock = |doc: &mut Document, num: u32| doc.update_dict(pdfcraft_cos::ObjRef::new(num, 0), |d| d.set(b"Ff".to_vec(), Object::Int(1))).unwrap(); + let mut doc = radio_group_form(); + lock(&mut doc, 7); // Medium + assert!(matches!(set_value(&mut doc, "size", &FieldValue::Radio(Some("M".into()))), Err(FormError::ReadOnly(_)))); + set_value(&mut doc, "size", &FieldValue::Radio(Some("L".into()))).expect("an unlocked button is picked"); + // Small (selected) locked: the group stays on it. + let mut doc = radio_group_form(); + lock(&mut doc, 6); + assert!(matches!(set_value(&mut doc, "size", &FieldValue::Radio(Some("L".into()))), Err(FormError::ReadOnly(_)))); + assert_eq!(field(&fields(&doc), "size").value, ["S"]); +} + +/// The group's `/V` names its selection even when the button's `/AS` says nothing: a read-only +/// button that `/V` selects is not switched away from either. +#[test] +fn a_read_only_radio_button_selected_by_the_group_value_is_not_left() { + let mut doc = radio_group_form(); + doc.update_dict(pdfcraft_cos::ObjRef::new(6, 0), |d| { + d.set(b"Ff".to_vec(), Object::Int(1)); + d.set(b"AS".to_vec(), Object::name("Off")); + }) + .unwrap(); + assert!(matches!(set_value(&mut doc, "size", &FieldValue::Radio(Some("L".into()))), Err(FormError::ReadOnly(_)))); + assert_eq!(field(&fields(&doc), "size").value, ["S"]); +} + +/// Clearing the form skips a group held by a read-only button and still clears the rest. +#[test] +fn clearing_the_form_goes_on_past_a_group_held_by_a_read_only_button() { + let mut doc = radio_group_form(); + doc.update_dict(pdfcraft_cos::ObjRef::new(6, 0), |d| d.set(b"Ff".to_vec(), Object::Int(1))).unwrap(); + doc.update_dict(pdfcraft_cos::ObjRef::new(9, 0), |d| d.set(b"DV".to_vec(), pdfcraft_cos::PdfString::text("Bob"))).unwrap(); + assert_eq!(reset(&mut doc, None).unwrap(), 1); + let all = fields(&doc); + assert_eq!(field(&all, "size").value, ["S"]); + assert_eq!(field(&all, "name").value, ["Bob"]); +} + #[test] /// Cosmetic field updates skip calculations, while a value write still triggers the chain. fn script_cosmetic_mutations_do_not_run_the_calculation_chain() { @@ -6937,6 +6979,26 @@ fn radio_buttons_without_as_sharing_the_value_show_one_on() { assert_eq!(on_widgets(&doc, "r").0, [false, true, false]); } +/// Two buttons share the on state `Yes`; the first is on by its `/AS` and the second is read-only +/// and off. `/V` is `Yes` for both, but only the first is selected: picking another option switches +/// the first off, which is allowed, and the read-only one never changes. +#[test] +fn a_read_only_button_sharing_the_on_state_of_the_selected_one_does_not_block() { + let mut doc = radio_without_as(49152, "/Ff 1 /AS /Off"); + doc.update_dict(pdfcraft_cos::ObjRef::new(6, 0), |d| d.set(b"AS".to_vec(), Object::name("Yes"))).unwrap(); + set_value(&mut doc, "r", &FieldValue::Radio(Some("No".into()))).expect("the writable selected button may be switched off"); + assert_eq!(on_widgets(&doc, "r").0, [false, false, true]); +} + +/// Selecting another button through `FieldValue::Widget` can't switch a read-only selected one off. +#[test] +fn a_read_only_selected_radio_button_is_not_left_by_picking_a_widget() { + let mut doc = radio_group_form(); + doc.update_dict(pdfcraft_cos::ObjRef::new(6, 0), |d| d.set(b"Ff".to_vec(), Object::Int(1))).unwrap(); + assert!(matches!(set_value(&mut doc, "size", &FieldValue::Widget(2)), Err(FormError::ReadOnly(_)))); + assert_eq!(field(&fields(&doc), "size").value, ["S"]); +} + /// Table 227, ReadOnly: a processor "shall not allow a user to change the value". A button whose own /// dictionary sets it can't be turned on by number. #[test] diff --git a/crates/forms/src/values.rs b/crates/forms/src/values.rs index af2c5e1aca..b60d8aa140 100644 --- a/crates/forms/src/values.rs +++ b/crates/forms/src/values.rs @@ -356,6 +356,9 @@ fn write_value_impl(doc: &mut Document, f: &Field, value: &FieldValue, scripts: let Some(state) = f.widgets.get(*i).and_then(|w| w.on_state.clone()) else { return invalid(format!("{:?} has no button {} with an on state", f.name, i.saturating_add(1))); }; + if f.kind == FieldKind::Radio && radio_change_blocked(f, Some(&state), Some(*i)) { + return Err(FormError::ReadOnly(f.name.clone())); + } set_states(doc, f, Some(&state), Some(*i)) } (FieldKind::CheckBox, v) => { @@ -406,6 +409,9 @@ fn write_value_impl(doc: &mut Document, f: &Field, value: &FieldValue, scripts: if choice.is_none() && f.has(flags::NO_TOGGLE_TO_OFF) && !f.value.is_empty() { return invalid(format!("{:?} must keep one option selected", f.name)); } + if radio_change_blocked(f, choice.as_deref(), None) { + return Err(FormError::ReadOnly(f.name.clone())); + } set_states(doc, f, choice.as_deref(), None) } (FieldKind::Combo | FieldKind::List, FieldValue::Choice(_) | FieldValue::Text(_) | FieldValue::ChoiceIndices(_)) => { @@ -541,17 +547,49 @@ pub(crate) fn scroll_widgets(doc: &mut Document, f: &Field, picked: &[usize], co Ok(()) } +/// The one widget of an exclusive radio group that turns on when `on` is chosen (`pick` when it has +/// that state, else the one already on, else the first); `None` when `on` is `None` or the group +/// turns every widget with that state on together. +fn chosen_widget(f: &Field, on: Option<&str>, pick: Option) -> Option { + let exclusive = f.kind == FieldKind::Radio && !f.has(flags::RADIOS_IN_UNISON); + on.filter(|_| exclusive).and_then(|c| { + let has = |i: &usize| f.widgets.get(*i).is_some_and(|w| w.on_state.as_deref() == Some(c)); + let n = f.widgets.len(); + pick.filter(has).or_else(|| (0..n).filter(has).find(|&i| f.widget_on(i))).or_else(|| (0..n).find(has)) + }) +} + +/// Whether setting a radio group to `on` (through widget `pick`, when the caller chose a widget) +/// would turn a read-only button on or off: such a button can't be picked, and a selected one can't +/// be switched off by picking another. Judged per widget, by the state `set_states` would give it, +/// so buttons sharing an on-state name are told apart by their `/AS`; the group's `/V` stands for +/// the selection only when no button's `/AS` says which is on. +fn radio_change_blocked(f: &Field, on: Option<&str>, pick: Option) -> bool { + let chosen = chosen_widget(f, on, pick); + let before = |i: usize, w: &Widget| { + let Some(s) = w.on_state.as_deref() else { return false }; + if f.widget_on(i) { + return true; + } + // No button sharing this on state is on by its `/AS` and the group's `/V` names it: the + // first of them holds the selection, whatever its own `/AS` says (an `Off` among them). + let sharing = |x: &Widget| x.on_state.as_deref() == Some(s); + f.value.first().map(String::as_str) == Some(s) + && !f.widgets.iter().any(|x| sharing(x) && x.state.as_deref() == Some(s)) + && f.widgets.iter().position(sharing) == Some(i) + }; + f.widgets.iter().enumerate().any(|(i, w)| { + let after = on.is_some() && w.on_state.as_deref() == on && chosen.is_none_or(|k| k == i); + w.read_only && before(i, w) != after + }) +} + /// Check boxes and radio buttons: `/V` on the field, `/AS` on each widget. The widgets whose on /// state is `on` turn on together in a check box (ISO 32000-2 §12.7.5.2.3, EXAMPLE 2) and in a /// radio group with RadiosInUnison. Without it, at most one radio button is on (Table 229, /// §12.7.5.2.3): widget `pick` when it has that state, else the one already on, else the first. fn set_states(doc: &mut Document, f: &Field, on: Option<&str>, pick: Option) -> Result<(), FormError> { - let exclusive = f.kind == FieldKind::Radio && !f.has(flags::RADIOS_IN_UNISON); - let chosen = on.filter(|_| exclusive).and_then(|c| { - let has = |i: &usize| f.widgets.get(*i).is_some_and(|w| w.on_state.as_deref() == Some(c)); - let n = f.widgets.len(); - pick.filter(has).or_else(|| (0..n).filter(has).find(|&i| f.widget_on(i))).or_else(|| (0..n).find(has)) - }); + let chosen = chosen_widget(f, on, pick); // `/V` names the on state, the same for every widget that has it: which of them is on is // their `/AS` alone (§12.7.5.2.4). let v = on.unwrap_or("Off"); @@ -919,10 +957,17 @@ pub fn reset_with_formatting( // A reset is not blocked by NoToggleToOff: go through the writer directly. let mut tmp = f.clone(); tmp.flags &= !flags::NO_TOGGLE_TO_OFF; - write_value_impl(doc, &tmp, &v, scripts, true)?; - changed += 1; - if matches!(f.kind, FieldKind::Text | FieldKind::Combo | FieldKind::List) { - redraw_fields.insert(f.obj); + match write_value_impl(doc, &tmp, &v, scripts, true) { + Ok(()) => { + changed += 1; + if matches!(f.kind, FieldKind::Text | FieldKind::Combo | FieldKind::List) { + redraw_fields.insert(f.obj); + } + } + // a read-only button of the group holds the value: this field is left as it is, and the + // rest of the form is still cleared + Err(FormError::ReadOnly(_)) => {} + Err(e) => return Err(e), } } let calculation_runner: &mut dyn Scripts = if calculate_with_scripts { scripts } else { &mut NoScripts }; From 2b65ab000bfe0997dec5fbed2769d3da3799355b Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 15:20:18 -0500 Subject: [PATCH 12/22] ci: use StepSecurity's maintained actions (#7026) Each third-party action that StepSecurity publishes a secure drop-in replacement for now uses that replacement, pinned to a commit with its version as a comment. Versions follow what we used: the newest StepSecurity release in the same major, or the closest major they publish. dtolnay/rust-toolchain refs that named a toolchain (stable, nightly, 1.76) become the replacement's `toolchain` input. --- .github/workflows/fuzz.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml index fe0a0074d7..78dc1b614b 100644 --- a/.github/workflows/fuzz.yml +++ b/.github/workflows/fuzz.yml @@ -31,10 +31,10 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false - - uses: dtolnay/rust-toolchain@686976e191b89faba57d3206551f0f330d8cb249 # stable + - uses: step-security/dtolnay-rust-toolchain@8dd607977d06adf4ed49b190381e74c17a9dd25f # v1.0.0 with: toolchain: stable - - uses: Swatinem/rust-cache@d1527c8f67ef9af357290475d4af4b0fab8f12e0 # v2.9.3 + - uses: step-security/rust-cache@8871c8ba063416da7c32246932ea6ae08700453e # v2.9.2 with: key: fuzz - name: Fetch the pdf.js corpus (seeds) From c73cccfb33a9146470060bf692578fbabb543607 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 15:37:44 -0500 Subject: [PATCH 13/22] fix(redact): hidden-layer removal reads the default state as viewers do (#5867) * fix(redact): hidden-layer removal reads the default state as viewers do hidden_layers ignored an indirect /BaseState and, with /BaseState /OFF, let /ON override /OFF, so a group both lists named survived as visible. The OCMD and OCG classifier also read /Type directly, so a membership dictionary with an indirect /Type was never evaluated and its content kept. The default state is now /BaseState (resolved), then /ON, then /OFF, and /Type is resolved before classifying a group or membership dictionary. * address review: follow chained /Type and /BaseState references, hash-set ON/OFF lists * address review: a visibility expression naming a group through an alias reference fails closed --- crates/redact/src/interp/layers.rs | 22 +++++++-- crates/redact/src/lib.rs | 13 +++++ crates/redact/src/sanitize.rs | 13 ++--- crates/redact/src/tests.rs | 76 ++++++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 10 deletions(-) diff --git a/crates/redact/src/interp/layers.rs b/crates/redact/src/interp/layers.rs index 362cb36620..7b8ef82120 100644 --- a/crates/redact/src/interp/layers.rs +++ b/crates/redact/src/interp/layers.rs @@ -20,7 +20,7 @@ impl<'a> Scope<'a> { // groups. A membership dictionary's default policy is AnyOn. let d = doc.resolve(o); let Some(d) = d.as_dict() else { return false }; - if d.name(b"Type") != Some(b"OCMD") { + if !has_type(doc, d, b"OCMD") { return false; } let groups: Vec = match d.get(b"OCGs").map(|g| (*doc.resolve(g)).clone()) { @@ -102,7 +102,7 @@ impl<'a> Scope<'a> { } let resolved = doc.resolve(expression); let complete = match &*resolved { - Object::Dict(d) if d.name(b"Type") == Some(b"OCG") => { + Object::Dict(d) if has_type(doc, d, b"OCG") => { if let Some(r) = reference && hidden_layers.contains(&r) { @@ -144,6 +144,11 @@ impl<'a> Scope<'a> { } *budget -= 1; let reference = expression.as_ref(); + // A reference to a reference is no listed group: the renderer rejects the alias, so it + // is not taken as visible because the object it names isn't hidden. + if reference.is_some_and(|r| matches!(*doc.get(r), Object::Ref(_))) { + return None; + } if let Some(r) = reference && !path.insert(r) { @@ -151,7 +156,7 @@ impl<'a> Scope<'a> { } let resolved = doc.resolve(expression); let result = match &*resolved { - Object::Dict(d) if d.name(b"Type") == Some(b"OCG") => reference.map(|r| !self.hidden_layers.contains(&r)), + Object::Dict(d) if has_type(doc, d, b"OCG") => reference.map(|r| !self.hidden_layers.contains(&r)), Object::Array(items) if (2..=4097).contains(&items.len()) => { let operator = match items.first() { Some(Object::Name(n)) => n.as_slice(), @@ -195,6 +200,9 @@ impl<'a> Scope<'a> { if self.hidden_layers.contains(&r) { return true; } + if matches!(*doc.get(r), Object::Ref(_)) { + return true; + } if !path.insert(r) { return true; } @@ -202,7 +210,7 @@ impl<'a> Scope<'a> { let resolved = doc.resolve(expression); let found = match &*resolved { Object::Array(items) => items.iter().any(|item| self.expression_mentions_hidden(doc, item, depth + 1, path, budget)), - Object::Dict(d) if d.name(b"Type") == Some(b"OCG") => expression.as_ref().is_none_or(|r| self.hidden_layers.contains(&r)), + Object::Dict(d) if has_type(doc, d, b"OCG") => expression.as_ref().is_none_or(|r| self.hidden_layers.contains(&r)), Object::Dict(_) => true, _ => false, }; @@ -256,3 +264,9 @@ pub(super) fn strip_hidden_layers(doc: &Document, scope: &mut Scope<'_>, ops: Ve } (out, removed) } + +/// Is this dictionary's `/Type` the given name? An indirect `/Type` is followed, as a viewer +/// follows it: a group or membership dictionary must not escape classification through one. +fn has_type(doc: &Document, d: &Dict, name: &[u8]) -> bool { + d.get(b"Type").is_some_and(|t| crate::resolve_chain(doc, t).as_name() == Some(name)) +} diff --git a/crates/redact/src/lib.rs b/crates/redact/src/lib.rs index 7fd34dae12..8f54cefdc6 100644 --- a/crates/redact/src/lib.rs +++ b/crates/redact/src/lib.rs @@ -1137,6 +1137,19 @@ pub fn clear_marks(doc: &mut Document, pages: Option<&[usize]>) -> Result std::sync::Arc { + let mut o = doc.resolve(o); + for _ in 0..16 { + if !matches!(*o, pdfcraft_cos::Object::Ref(_)) { + break; + } + o = doc.resolve(&o); + } + o +} + /// The name to save a document under after [`apply`]: `_Redacted` before the extension, as /// Acrobat appends it ("contract.pdf" gives "contract_Redacted.pdf"). The `.pdf` extension is /// matched in any letter case. A name that already ends in `_Redacted` keeps it once. diff --git a/crates/redact/src/sanitize.rs b/crates/redact/src/sanitize.rs index cd38b9eb19..61c7cedda4 100644 --- a/crates/redact/src/sanitize.rs +++ b/crates/redact/src/sanitize.rs @@ -479,12 +479,13 @@ fn hidden_layers(doc: &Document) -> Vec { o.and_then(|g| doc.resolve(g).as_array().map(|a| a.iter().filter_map(Object::as_ref).collect())).unwrap_or_default() }; let all = refs(oc.get(b"OCGs")); - let (on, off) = (refs(d.get(b"ON")), refs(d.get(b"OFF"))); - let mut hidden: Vec = if d.name(b"BaseState") == Some(b"OFF") { - all.iter().copied().filter(|g| !on.contains(g)).collect() - } else { - off.into_iter().filter(|g| all.contains(g)).collect() - }; + // sets: the lists are the document's, and a filter over every group would scan them each time + let (on, off): (std::collections::HashSet, std::collections::HashSet) = + (refs(d.get(b"ON")).into_iter().collect(), refs(d.get(b"OFF")).into_iter().collect()); + // The default state is /BaseState, then /ON turns groups on and /OFF turns them off, so a + // group both lists name ends up off (§8.11.4.3, Table 101) — the viewer reads it the same way. + let base_off = d.get(b"BaseState").map(|b| crate::resolve_chain(doc, b)).is_some_and(|b| b.as_name() == Some(b"OFF".as_slice())); + let mut hidden: Vec = all.iter().copied().filter(|g| off.contains(g) || (base_off && !on.contains(g))).collect(); // Each rule, layer list and layer is read once, however often the rules name them: many rules // can share one long list. let known: std::collections::HashSet = all.iter().copied().collect(); diff --git a/crates/redact/src/tests.rs b/crates/redact/src/tests.rs index dc94a2c5f5..08125165b5 100644 --- a/crates/redact/src/tests.rs +++ b/crates/redact/src/tests.rs @@ -4151,6 +4151,8 @@ fn hidden_ocmd_policy_matches_the_default_configuration() { (Some("AllOn"), false, Some("not_off"), false), (None, false, Some("malformed_not"), true), (None, false, Some("malformed_without_ocgs"), true), + // `/VE` names the hidden group through an alias object (10 0 R is a reference to 8 0 R). + (None, false, Some("alias"), true), ]; for (policy, malformed_policy, visibility, should_remove) in cases { let mut doc = one_page( @@ -4188,9 +4190,11 @@ fn hidden_ocmd_policy_matches_the_default_configuration() { Some("malformed_without_ocgs") => { membership.set(b"VE".to_vec(), Object::Array(vec![Object::name("UnknownOperator"), Object::Ref(ObjRef::new(8, 0))])) } + Some("alias") => membership.set(b"VE".to_vec(), Object::Array(vec![Object::name("Or"), Object::Ref(ObjRef::new(10, 0))])), _ => {} } doc.set(ObjRef::new(9, 0), Object::Dict(membership)); + doc.set(ObjRef::new(10, 0), Object::Ref(ObjRef::new(8, 0))); let root = doc.root().unwrap(); doc.update_dict(root, |d| { d.set( @@ -6670,6 +6674,78 @@ fn a_reply_on_another_page_to_a_removed_popup_goes_too() { } } +/// One page drawing `SECRET` in the `/OC /L` block, with layer 7 0 R in the default +/// configuration `config` (objects 8 and up are `extra`). +fn layer_state_page(config: Vec<(&str, Object)>, resource: &str, extra: Vec>) -> Document { + let mut objects = vec![b"<< /Type /OCG /Name (Secret) >>".to_vec()]; + objects.extend(extra); + let mut doc = one_page(b"/OC /L BDC BT /F1 10 Tf 10 100 Td (SECRET) Tj ET EMC", &format!("/Properties << /L {resource} >>"), objects); + let config = Object::Dict(Dict::from_iter(config.into_iter().map(|(k, v)| (k.as_bytes().to_vec(), v)))); + let root = doc.root().unwrap(); + doc.update_dict(root, |d| { + d.set( + b"OCProperties".to_vec(), + Object::Dict(Dict::from_iter([(b"OCGs".to_vec(), Object::Array(vec![Object::Ref(ObjRef::new(7, 0))])), (b"D".to_vec(), config)])), + ); + }) + .unwrap(); + doc +} + +fn assert_layer_secret_removed(mut doc: Document) { + use crate::sanitize::{Hidden, remove_hidden, scan}; + let counts = scan(&doc); + assert!(counts.iter().any(|(h, n)| *h == Hidden::HiddenLayers && *n > 0), "{counts:?}"); + remove_hidden(&mut doc, &[Hidden::HiddenLayers]).unwrap(); + let saved = Document::open(Arc::new(write_full(&doc, &SaveOptions::default()).unwrap())).unwrap(); + assert!(!pdfcraft_edit::text_lines(&saved, 0).unwrap().iter().any(|line| line.text.contains("SECRET"))); +} + +#[test] +fn an_indirect_base_state_off_hides_the_layer() { + assert_layer_secret_removed(layer_state_page(vec![("BaseState", Object::Ref(ObjRef::new(8, 0)))], "7 0 R", vec![b"/OFF".to_vec()])); +} + +#[test] +fn a_chained_base_state_off_hides_the_layer() { + assert_layer_secret_removed(layer_state_page( + vec![("BaseState", Object::Ref(ObjRef::new(8, 0)))], + "7 0 R", + vec![b"9 0 R".to_vec(), b"/OFF".to_vec()], + )); +} + +#[test] +fn a_chained_membership_type_is_evaluated() { + assert_layer_secret_removed(layer_state_page( + vec![("BaseState", Object::name("ON"))], + "8 0 R", + vec![b"<< /Type 9 0 R /OCGs [7 0 R] /P /AllOff >>".to_vec(), b"10 0 R".to_vec(), b"/OCMD".to_vec()], + )); +} + +#[test] +fn off_wins_over_on_when_the_base_state_is_off() { + assert_layer_secret_removed(layer_state_page( + vec![ + ("BaseState", Object::name("OFF")), + ("ON", Object::Array(vec![Object::Ref(ObjRef::new(7, 0))])), + ("OFF", Object::Array(vec![Object::Ref(ObjRef::new(7, 0))])), + ], + "7 0 R", + Vec::new(), + )); +} + +#[test] +fn a_membership_dictionary_with_an_indirect_type_is_evaluated() { + assert_layer_secret_removed(layer_state_page( + vec![("BaseState", Object::name("ON"))], + "8 0 R", + vec![b"<< /Type 9 0 R /OCGs [7 0 R] /P /AllOff >>".to_vec(), b"/OCMD".to_vec()], + )); +} + /// An invisible OCR-style text layer drawn in a Type3 font (what the OCR output writes for /// characters beyond WinAnsi) is redacted like any other text: its glyphs are hit-tested by /// their real advances, removed, and the verifier passes. From daec898d7c56fc9f283777c99e2f1ca1f52d53c2 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 15:49:35 -0500 Subject: [PATCH 14/22] fix(sign): judge a shared signature value once per listing; dedupe DSS evidence by object number (#6041) * fix(sign): judge a shared signature value once per listing; dedupe DSS evidence by object number * address review: shared signature values are judged by output index without copying the dictionary; a dead evidence reference does not hide a live one * address review: repeated evidence references are skipped before they are resolved * address review: each full evidence reference is tried once --- crates/sign/src/pdf/listing.rs | 16 ++++++++++++- crates/sign/src/pdf/validate.rs | 20 +++++++++++++--- crates/sign/src/pdf_discovery_tests.rs | 13 +++++++++++ crates/sign/tests/it/pdf.rs | 32 ++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 4 deletions(-) diff --git a/crates/sign/src/pdf/listing.rs b/crates/sign/src/pdf/listing.rs index c2a7183db1..2d41477127 100644 --- a/crates/sign/src/pdf/listing.rs +++ b/crates/sign/src/pdf/listing.rs @@ -431,8 +431,12 @@ pub fn list(doc: &Document, bytes: &[u8], trust: &TrustStore) -> Vec Vec { let pages = annot_pages(doc); - let mut out = Vec::new(); + let mut out: Vec = Vec::new(); let mut field_values: HashSet = HashSet::new(); + // Fields that name one signature value are judged once: validation does not depend on the + // field, and repeating it per field would repeat the digest, chain and evidence work. + // The row of `out` that holds each value's verdict: a later field copies it, not a map of copies. + let mut judged: HashMap = HashMap::new(); let (fields, whole) = walk_sig_fields(doc); for f in fields { if let Some(v_ref) = f.dict.get(b"V").and_then(Object::as_ref) { @@ -445,6 +449,13 @@ pub fn list_cached(doc: &Document, bytes: &[u8], trust: &TrustStore, cache: &Dig .filter(|r| r.len() == 4) .map(|r| [r[0].min(r[2]), r[1].min(r[3]), r[0].max(r[2]), r[1].max(r[3])]); let visible = widget.is_some_and(|(_, w)| widget_shown(doc, w)); + // A repeat of a judged value reads nothing of it: the signature dictionary is not copied. + let shared = f.dict.get(b"V").and_then(Object::as_ref).filter(|_| f.dict.get(b"V").is_some_and(|v| doc.resolve(v).as_dict().is_some())); + if let Some(first) = shared.and_then(|r| judged.get(&r)).and_then(|&i| out.get(i)) { + let own = SignatureInfo { field: f.name.clone(), signed: true, page, rect, visible, ..first.clone() }; + out.push(own); + continue; + } let v = f.signature(doc); let mut info = SignatureInfo { field: f.name.clone(), @@ -477,6 +488,9 @@ pub fn list_cached(doc: &Document, bytes: &[u8], trust: &TrustStore, cache: &Dig if let Some(v) = v { validate_into(doc, bytes, trust, &v, &mut info, cache); } + if let Some(r) = shared { + judged.insert(r, out.len()); + } out.push(info); } // Standalone document timestamps (ISO 32000-2 §12.8.2.2) live outside AcroForm fields. diff --git a/crates/sign/src/pdf/validate.rs b/crates/sign/src/pdf/validate.rs index 52bf79351f..d3ad411a62 100644 --- a/crates/sign/src/pdf/validate.rs +++ b/crates/sign/src/pdf/validate.rs @@ -371,7 +371,11 @@ pub(crate) const MAX_DSS_BYTES: usize = 64 << 20; pub(crate) struct DssEvidence<'a> { doc: &'a Document, items: std::vec::IntoIter, - seen: HashSet, + /// Object numbers read: `Document::get` resolves a reference by number, so `5 0 R` and `5 1 R` + /// are one stream. + seen: HashSet, + /// Full references already tried, resolved or not. + tried: HashSet, taken: usize, left: usize, /// A cap stopped the array being read in full. @@ -381,7 +385,7 @@ pub(crate) struct DssEvidence<'a> { impl<'a> DssEvidence<'a> { pub(crate) fn new(doc: &'a Document, dss: &Dict, key: &[u8]) -> Self { let items = dss.get(key).map(|o| doc.resolve(o)).and_then(|o| o.as_array().cloned()).unwrap_or_default(); - DssEvidence { doc, items: items.into_iter(), seen: HashSet::new(), taken: 0, left: MAX_DSS_BYTES, cut: false } + DssEvidence { doc, items: items.into_iter(), seen: HashSet::new(), tried: HashSet::new(), taken: 0, left: MAX_DSS_BYTES, cut: false } } } @@ -395,11 +399,21 @@ impl Iterator for DssEvidence<'_> { // Repeats are skipped before the caps count anything, so repeating one stream cannot use // them up; the scan of the array itself is bounded by the parser's cap on array length. for item in self.items.by_ref() { - if item.as_ref().is_some_and(|r| !self.seen.insert(r)) { + // Resolved first: a reference to a generation the slot doesn't hold reads as null and + // must not make a later, valid reference to the same object number a repeat. + // A repeat is skipped before anything is resolved: a long chain of repeats costs no + // lookups. The number is recorded only once the reference gave a stream. + // Each full reference is tried once, so one that never reaches a stream (a cycle) costs + // its lookups once, however often it is listed; `seen` keeps object numbers apart so a + // dead generation can't hide a live one. + if item.as_ref().is_some_and(|r| self.seen.contains(&r.num) || !self.tried.insert(r)) { continue; } let o = self.doc.resolve(&item); let Object::Stream(s) = o.as_ref() else { continue }; + if let Some(r) = item.as_ref() { + self.seen.insert(r.num); + } if self.taken >= MAX_DSS_ENTRIES { self.cut = true; return None; diff --git a/crates/sign/src/pdf_discovery_tests.rs b/crates/sign/src/pdf_discovery_tests.rs index 29f115cb07..d3250d62ed 100644 --- a/crates/sign/src/pdf_discovery_tests.rs +++ b/crates/sign/src/pdf_discovery_tests.rs @@ -373,6 +373,19 @@ fn only_grew_compares_huge_arrays_without_quadratic_work() { assert!(start.elapsed() < std::time::Duration::from_secs(10), "{:?}", start.elapsed()); } +/// A reference to a generation the object doesn't have reads as null: it is no repeat of the valid +/// reference to the same object number that follows it. +#[test] +fn dss_evidence_skips_a_dead_reference_without_hiding_the_live_one() { + let mut doc = fixture(&[]); + let live = doc.add(Object::Stream(Stream::flate(Dict::new(), b"evidence"))); + let dead = ObjRef::new(live.num, live.generation + 1); + let mut dss = Dict::new(); + dss.set(b"CRLs".to_vec(), Object::Array(vec![Object::Ref(dead), Object::Ref(live)])); + let read: Vec> = DssEvidence::new(&doc, &dss, b"CRLs").collect(); + assert_eq!(read, vec![b"evidence".to_vec()]); +} + #[test] fn dss_evidence_decodes_each_stream_once_within_its_caps() { // One small stream listed 1000 times, then streams that inflate past the evidence cap. diff --git a/crates/sign/tests/it/pdf.rs b/crates/sign/tests/it/pdf.rs index fa45ab1ecb..5d24d05dce 100644 --- a/crates/sign/tests/it/pdf.rs +++ b/crates/sign/tests/it/pdf.rs @@ -3369,6 +3369,38 @@ fn signing_a_field_with_a_null_value_after_another_signature_keeps_the_first_val ); } +/// Fields that name one signature value are listed each, under their own name, with the verdict +/// the value earned once. +#[test] +fn fields_sharing_a_signature_value_are_listed_with_one_verdict() { + let id = pkcs12::open(&data("rsa-aes.p12"), "test").unwrap(); + let trust = TrustStore { certs: vec![id.certificate.clone()] }; + let signed = pdfcraft_sign::sign(&open(&fixture()), &id, &SignOptions { field: Some("Approval".into()), ..opts() }).unwrap(); + let v = open(&signed).get(pdfcraft_cos::ObjRef { num: 6, generation: 0 }).as_dict().and_then(|d| d.get(b"V").cloned()).unwrap(); + let shared = edit_after(&signed, |doc| { + let mut w = pdfcraft_cos::Dict::new(); + w.set(b"Type".to_vec(), Object::name("Annot")); + w.set(b"Subtype".to_vec(), Object::name("Widget")); + w.set(b"FT".to_vec(), Object::name("Sig")); + w.set(b"T".to_vec(), PdfString::text("Twin")); + w.set(b"V".to_vec(), v.clone()); + w.set(b"Rect".to_vec(), Object::Array(vec![Object::Int(0); 4])); + let w = doc.add(Object::Dict(w)); + let root = doc.root().unwrap(); + let af = doc.get(root).as_dict().and_then(|c| c.get(b"AcroForm").cloned()).unwrap(); + let af = doc.resolve(&af).as_dict().cloned().unwrap(); + let mut fields = af.get(b"Fields").map(|f| doc.resolve(f)).and_then(|f| f.as_array().cloned()).unwrap(); + fields.push(Object::Ref(w)); + let mut af = af; + af.set(b"Fields".to_vec(), Object::Array(fields)); + doc.update_dict(root, |c| c.set(b"AcroForm".to_vec(), Object::Dict(af))).unwrap(); + }); + let list = signatures(&open(&shared), &shared, &trust); + let (a, b) = (list.iter().find(|s| s.field == "Approval").unwrap(), list.iter().find(|s| s.field == "Twin").unwrap()); + assert!(a.signed && b.signed); + assert_eq!((a.status, &a.modification, &a.signer, a.signed_len), (b.status, &b.modification, &b.signer, b.signed_len)); +} + #[test] fn the_catalogs_perms_do_not_change_under_no_changes() { // /Perms names the certification signature (DocMDP). Under P=1 nothing may change, so a later From 77078864f41f07818624ea98c3d1ddee1201e547 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 15:50:02 -0500 Subject: [PATCH 15/22] fix(render): charge inspection text (fields, tooltips, layers, bookmarks, blank comments) against shared allowances (#6015) * fix(render): charge field, tooltip, layer, bookmark and blank-comment text against shared allowances * address review: charge comment text once, keep JS and field-action flags past the budgets * review: count an unlisted field's actions through its whole subtree * review: mark multi-select values the allowance dropped * review: count only real actions, walk each field once, keep name selections * address review: revisit shared field descendants from shallower paths, charge value separators, check every widget for actions * merge main, keep both --- crates/render/src/inspect/annotations.rs | 169 +++++++++++++++++++---- crates/render/src/inspect/core.rs | 68 ++++++++- crates/render/src/inspect/navigation.rs | 2 +- crates/render/src/inspect/resources.rs | 7 +- crates/render/src/inspect/tests.rs | 80 +++++++++++ 5 files changed, 289 insertions(+), 37 deletions(-) diff --git a/crates/render/src/inspect/annotations.rs b/crates/render/src/inspect/annotations.rs index 7313716a2e..673d930743 100644 --- a/crates/render/src/inspect/annotations.rs +++ b/crates/render/src/inspect/annotations.rs @@ -144,12 +144,7 @@ impl<'a> Inspector<'a> { info.attachments.push(self.attachment(fs, AttachmentSource::Annotation { page, index }, text_budget, fallback)); } if subtype == "Link" { - if let Some(link) = self.link(page, index, d, info.uri_base.as_deref(), text_budget, &mut unfit_quads) { - if matches!(&link.target, LinkTarget::Other(s) if s == "JavaScript") { - info.has_javascript = true; - } - info.links.push(link); - } + self.collect_link(info, (page, index), d, text_budget, &mut unfit_quads); continue; } // Not comments: form widgets, pop-ups and non-markup annotations such as the @@ -161,6 +156,20 @@ impl<'a> Inspector<'a> { if comments_full { continue; } + // Text that trims to nothing is not kept, so `retained` never sees it: charge what is + // read, so many comments sharing one huge blank string stop being decoded over and over. + let raw: usize = [&b"T"[..], b"Contents", b"M", b"NM", b"State", b"DA"] + .iter() + .filter_map(|k| match d.get(k).map(|o| self.resolve(o)) { + Ok(Object::String(b, _) | Object::Name(b)) => Some(b.len()), + _ => None, + }) + .fold(0, usize::saturating_add); + // (charged once: below, by the larger of these raw bytes and what the annotation keeps) + if kept.saturating_add(raw) > MAX_ANNOTATION_BYTES { + comments_full = true; + continue; + } let rect = rect4(self.resolve(d.get(b"Rect").unwrap_or(&Object::Null))); let c = match d.get(b"C").map(|o| self.resolve(o)) { Ok(Object::Array(c)) => Some(c), @@ -262,7 +271,7 @@ impl<'a> Inspector<'a> { text_box, popup, }; - kept = kept.saturating_add(retained(&annotation)); + kept = kept.saturating_add(retained(&annotation).max(raw)); if kept > MAX_ANNOTATION_BYTES { comments_full = true; continue; @@ -356,6 +365,23 @@ impl<'a> Inspector<'a> { Some(Link { page, index, rect, quads, target }) } + /// Lists the link annotation `d`, and notes the JavaScript it runs. JavaScript is read from the + /// action, not the listed link: once the text budget is spent the action's name is not kept. + fn collect_link(&self, info: &mut DocInfo, at: (usize, usize), d: &Dictionary, text_budget: &mut usize, unfit_quads: &mut HashSet) { + info.has_javascript |= self.runs_javascript(d); + if let Some(link) = self.link(at.0, at.1, d, info.uri_base.as_deref(), text_budget, unfit_quads) { + if matches!(&link.target, LinkTarget::Other(s) if s == "JavaScript") { + info.has_javascript = true; + } + info.links.push(link); + } + } + + /// Whether the link's own action is a JavaScript action. + fn runs_javascript(&self, d: &Dictionary) -> bool { + d.get(b"A").ok().and_then(|a| self.dict(a)).is_some_and(|a| self.name(a, b"S").as_deref() == Some("JavaScript")) + } + /// What an action does when a link or a bookmark runs it. Only a go-to's `/D` is a place in /// this document; a remote go-to's names a page or destination of another file (ISO 32000-2 /// §12.6.4.3). `None` for an action without a type, or a go-to whose destination is not here. @@ -376,7 +402,7 @@ impl<'a> Inspector<'a> { "SetOCGState" => self.layer_state(a), "GoToR" => self.remote_go_to(a).unwrap_or_else(|| LinkTarget::Other("GoToR".into())), "Launch" => self.launch_pdf(a).unwrap_or_else(|| LinkTarget::Other("Launch".into())), - other => LinkTarget::Other(other.to_string()), + other => LinkTarget::Other(spend(other.as_bytes(), text_budget, |b| String::from_utf8_lossy(b).into_owned()).unwrap_or_default()), }) } @@ -492,6 +518,53 @@ impl<'a> Inspector<'a> { // ── form fields ───────────────────────────────────────────────────────────────────────── + /// Whether `o` or one of its descendants under `/Kids` carries an action (`/A` or `/AA`). It + /// walks the same tree `field` does, bounded the same way: a field left out of the listing for + /// want of text allowance can still have its action on a widget several levels down. + pub(super) fn has_action(&self, o: &Object, depth: u32) -> bool { + self.has_action_seen(o, depth, &mut HashMap::new()) + } + + /// The walk behind [`Self::has_action`]. Each indirect descendant is read again only from a + /// shallower depth than before (the depth limit may have cut its subtree): a field tree whose + /// `/Kids` repeat the same objects would otherwise be walked exponentially, and this reads each + /// object at most once per depth. + fn has_action_seen(&self, o: &Object, depth: u32, seen: &mut HashMap) -> bool { + if depth > 32 { + return false; + } + if let Object::Reference(id) = o { + if seen.get(id).is_some_and(|&first| first <= depth) { + return false; + } + seen.insert(*id, depth); + } + let Some(d) = self.dict(o) else { return false }; + if self.carries_action(d) { + return true; + } + let kids = match d.get(b"Kids").map(|k| self.resolve(k)) { + Ok(Object::Array(k)) => k, + _ => return false, + }; + kids.iter().any(|k| self.has_action_seen(k, depth + 1, seen)) + } + + /// Whether `/A` or `/AA` here holds an action. An editor can leave an empty dictionary (or + /// array) behind, and that is no action: a document with none must not report JavaScript. An + /// `/A` is an action when it carries `/S`; an `/AA` is one when an event maps to such an action. + fn carries_action(&self, d: &Dictionary) -> bool { + let action = |o: &Object| matches!(self.resolve(o), Object::Dictionary(a) if a.get(b"S").is_ok()); + [&b"A"[..], &b"AA"[..]].iter().any(|k| match d.get(k) { + Ok(o) => match self.resolve(o) { + Object::Dictionary(x) => x.get(b"S").is_ok() || x.iter().any(|(_, v)| action(v)), + Object::Array(a) => a.iter().any(action), + _ => false, + }, + Err(_) => false, + }) + } + pub(super) fn field( &self, o: &'a Object, @@ -510,19 +583,35 @@ impl<'a> Inspector<'a> { } let Some(d) = self.dict(o) else { return }; let (parent_name, parent_ft, parent_ff) = parent.unwrap_or(("", None, None)); - let partial = self.text(d, b"T"); - let full = match (&partial, parent_name.is_empty()) { - (Some(t), true) => t.clone(), - (Some(t), false) => format!("{parent_name}.{t}"), - (None, _) => parent_name.to_string(), - }; - let ft = self.name(d, b"FT").or(parent_ft); - let ff = d.get(b"Ff").ok().and_then(|x| x.as_i64().ok()).or(parent_ff); // Non-terminal: has kids that are fields (they carry /T). Widgets-only kids are terminal. let mut kids: Vec = match d.get(b"Kids").map(|k| self.resolve(k)) { Ok(Object::Array(k)) => k.clone(), _ => Vec::new(), }; + // A field left unlisted for want of text allowance still has its actions counted, through + // its whole subtree: an action can sit on a descendant widget, not only on the field. + let unlisted = || { + if self.has_action(o, 0) { + self.field_actions.set(true); + } + }; + // A /T that does not fit the allowance lists no field (not one under its parent's name). + let Ok(partial) = self.field_text_within(d, b"T") else { + unlisted(); + return; + }; + let full = match (&partial, parent_name.is_empty()) { + (Some(t), true) => t.clone(), + (Some(t), false) => format!("{parent_name}.{t}"), + (None, _) => parent_name.to_string(), + }; + // Every field copies its parent's full name: charge it, and list no more once it is spent. + if !self.field_charge(full.len()) { + unlisted(); + return; + } + let ft = self.name(d, b"FT").or(parent_ft); + let ff = d.get(b"Ff").ok().and_then(|x| x.as_i64().ok()).or(parent_ff); let (field_kids, widget_kids): (Vec, Vec) = kids.iter().cloned().partition(|k| self.dict(k).is_some_and(|kd| kd.get(b"T").is_ok())); let field_kids: Vec<_> = field_kids.iter().filter(|k| !self.not_a_child(walk, o, k)).collect(); @@ -570,17 +659,37 @@ impl<'a> Inspector<'a> { _ => FieldKind::Unknown, }; let value = match d.get(b"V").map(|v| self.resolve(v)) { - Ok(Object::Name(n)) => Some(String::from_utf8_lossy(n).into_owned()), - Ok(Object::String(bytes, _)) => Some(text_string(bytes)), - Ok(Object::Array(a)) => Some( - a.iter() - .filter_map(|x| match self.resolve(x) { - Object::String(bytes, _) => Some(text_string(bytes)), - _ => None, - }) - .collect::>() - .join(", "), - ), + Ok(Object::Name(n)) => self.field_spend(n, |n| String::from_utf8_lossy(n).into_owned()), + Ok(Object::String(bytes, _)) => self.field_spend(bytes, text_string), + Ok(Object::Array(a)) => { + let mut shown = Vec::new(); + let mut dropped = false; + for x in a { + let text = match self.resolve(x) { + Object::String(bytes, _) => self.field_spend(bytes, text_string), + // A name entry (`/V [/Opt1 (b)]`) is a selection too, as the forms reader + // takes it; leaving it out reported a partial selection as complete. + Object::Name(n) => self.field_spend(n, |n| String::from_utf8_lossy(n).into_owned()), + _ => continue, + }; + // The ", " between selections is charged to the allowance like the text. + match text { + Some(text) if shown.is_empty() || self.field_charge(2) => shown.push(text), + _ => dropped = true, + } + } + // A selected value the allowance could not hold shows as an omission rather than + // as part of a complete selection (its few bytes are charged, fit or not). + let mut value = shown.join(", "); + if dropped { + self.field_charge(5); + if !value.is_empty() { + value.push_str(", "); + } + value.push('…'); + } + Some(value) + } Ok(Object::Dictionary(_)) if kind == FieldKind::Signature => Some("signed".into()), _ => None, }; @@ -602,9 +711,11 @@ impl<'a> Inspector<'a> { .and_then(|p| self.page_of(p)) .or_else(|| self.find_widget_page(o, &kids)) .or_else(|| named.and_then(|id| self.annotation_pages.get_or_init(|| self.annotation_pages()).get(&id).copied())); - let has_actions = d.get(b"AA").is_ok() || widget.is_some_and(|w| w.get(b"AA").is_ok() || w.get(b"A").is_ok()); + let has_actions = self.carries_action(d) + || widget.is_some_and(|w| self.carries_action(w)) + || kids.iter().any(|k| self.dict(k).is_some_and(|w| self.carries_action(w))); let rect = widget.and_then(|w| w.get(b"Rect").ok()).map(|r| rect4(self.resolve(r))); - out.push(Field { name: full, kind, value, page, tooltip: self.text(d, b"TU"), has_actions, rect }); + out.push(Field { name: full, kind, value, page, tooltip: self.field_text(d, b"TU"), has_actions, rect }); } /// Whether `k`, listed in the `/Kids` of field `o`, is not a child of `o` after all: `o` itself diff --git a/crates/render/src/inspect/core.rs b/crates/render/src/inspect/core.rs index 88dc13c34a..adf3edffbd 100644 --- a/crates/render/src/inspect/core.rs +++ b/crates/render/src/inspect/core.rs @@ -155,16 +155,34 @@ pub(super) struct Inspector<'a> { /// has no `/P`. Searching every page's `/Annots` for each such field was O(fields × /// annotations): a second on a form of 8,000 fields. pub(super) annotation_pages: OnceCell>, + /// What is left of [`MAX_FIELD_TEXT_BYTES`]: names, values and tooltips of form fields and + /// layer names, which many entries can share one string of. + pub(super) field_text: std::cell::Cell, + /// Set when a field with an action went unlisted because the text allowance was spent, so the + /// document still reports JavaScript. + pub(super) field_actions: std::cell::Cell, /// The first page widget naming each field as its `/Parent`, built when a field has no widget /// among its `/Kids`. pub(super) parent_widgets: OnceCell>, } +/// Bytes of field and layer text one inspection reads and keeps together. +pub(super) const MAX_FIELD_TEXT_BYTES: usize = 64 << 20; + impl<'a> Inspector<'a> { pub(super) fn new(doc: &'a dyn Structure) -> Self { let page_ids = doc.page_ids(); let page_index = page_ids.iter().copied().enumerate().map(|(index, id)| (id, index)).collect(); - let mut me = Self { doc, page_ids, page_index, named: HashMap::new(), annotation_pages: OnceCell::new(), parent_widgets: OnceCell::new() }; + let mut me = Self { + doc, + page_ids, + page_index, + named: HashMap::new(), + annotation_pages: OnceCell::new(), + field_text: MAX_FIELD_TEXT_BYTES.into(), + field_actions: false.into(), + parent_widgets: OnceCell::new(), + }; let mut named = HashMap::new(); if let Some(tree) = doc.catalog().ok().and_then(|c| c.get(b"Names").ok()).and_then(|o| me.dict(o)).and_then(|n| n.get(b"Dests").ok()).and_then(|o| me.dict(o)) @@ -260,7 +278,7 @@ impl<'a> Inspector<'a> { } } } - info.has_javascript |= info.fields.iter().any(|f| f.has_actions); + info.has_javascript |= info.fields.iter().any(|f| f.has_actions) || self.field_actions.get(); let needs_rendering = catalog.get(b"NeedsRendering").ok().and_then(|o| self.resolve(o).as_bool().ok()).unwrap_or(false); if let Some(xfa) = catalog.get(b"AcroForm").ok().and_then(|o| self.dict(o)).and_then(|form| form.get(b"XFA").ok()) { // A form with no fields is taken to be laid out from its template, unless its @@ -351,11 +369,49 @@ impl<'a> Inspector<'a> { /// Whitespace-trimmed metadata, spent from `budget` as [`spend`] does, so text that trims /// short or to nothing still spends its raw length. + /// [`Self::budgeted_text`] against the shared field and layer text allowance. + pub(super) fn field_text(&self, d: &Dictionary, key: &[u8]) -> Option { + let mut left = self.field_text.get(); + let text = self.budgeted_text(d, key, &mut left); + self.field_text.set(left); + text + } + + /// Converts `bytes` against the field and layer text allowance, as [`spend`] does. + pub(super) fn field_spend(&self, bytes: &[u8], convert: impl FnOnce(&[u8]) -> String) -> Option { + let mut left = self.field_text.get(); + let text = spend(bytes, &mut left, convert); + self.field_text.set(left); + text + } + + /// Spends `n` bytes of the field and layer text allowance; `false` when too few are left. + pub(super) fn field_charge(&self, n: usize) -> bool { + let left = self.field_text.get(); + self.field_text.set(left.saturating_sub(n)); + n <= left + } + pub(super) fn budgeted_text(&self, d: &Dictionary, key: &[u8], budget: &mut usize) -> Option { - let o = self.resolve(d.get(key).ok()?); - let (Object::String(bytes, _) | Object::Name(bytes)) = o else { return None }; - let text = spend(bytes, budget, |_| trimmed_text(o).unwrap_or_default())?; - (!text.is_empty()).then_some(text) + self.budgeted_text_within(d, key, budget).ok().flatten() + } + + /// [`Self::budgeted_text`], but `Err` when the text is there and does not fit `budget` + /// (against `Ok(None)`: no text, or none that trims to anything). + pub(super) fn budgeted_text_within(&self, d: &Dictionary, key: &[u8], budget: &mut usize) -> Result, ()> { + let Ok(o) = d.get(key) else { return Ok(None) }; + let o = self.resolve(o); + let (Object::String(bytes, _) | Object::Name(bytes)) = o else { return Ok(None) }; + let text = spend(bytes, budget, |_| trimmed_text(o).unwrap_or_default()).ok_or(())?; + Ok((!text.is_empty()).then_some(text)) + } + + /// [`Self::field_text`], but `Err` when the text does not fit the allowance. + pub(super) fn field_text_within(&self, d: &Dictionary, key: &[u8]) -> Result, ()> { + let mut left = self.field_text.get(); + let text = self.budgeted_text_within(d, key, &mut left); + self.field_text.set(left); + text } pub(super) fn name(&self, d: &Dictionary, key: &[u8]) -> Option { diff --git a/crates/render/src/inspect/navigation.rs b/crates/render/src/inspect/navigation.rs index c5f2a1be86..f680b46246 100644 --- a/crates/render/src/inspect/navigation.rs +++ b/crates/render/src/inspect/navigation.rs @@ -250,7 +250,7 @@ impl<'a> Inspector<'a> { None => {} } let open = d.get(b"Count").ok().and_then(|c| c.as_i64().ok()).is_some_and(|c| c > 0); - let title = self.text(d, b"Title").unwrap_or_default(); + let title = self.field_text(d, b"Title").unwrap_or_default(); let color = match d.get(b"C").map(|o| self.resolve(o)) { Ok(Object::Array(c)) => outline_color(c.iter().map(|v| self.resolve(v).as_float().ok())), _ => None, diff --git a/crates/render/src/inspect/resources.rs b/crates/render/src/inspect/resources.rs index 3dbc1c665f..6d90f715e1 100644 --- a/crates/render/src/inspect/resources.rs +++ b/crates/render/src/inspect/resources.rs @@ -121,9 +121,14 @@ impl<'a> Inspector<'a> { .unwrap_or_default(); let auto = config.map(|c| self.view_auto_state(c)).unwrap_or_default(); let Ok(Object::Array(ocgs)) = props.get(b"OCGs").map(|o| self.resolve(o)) else { return }; + let mut seen = HashSet::new(); for g in ocgs { let Some(d) = self.dict(g) else { continue }; let id = g.as_reference().ok(); + // A group listed twice is one layer (and its name is read once). + if id.is_some_and(|id| !seen.insert(id)) { + continue; + } // The View auto state, when it manages the group, then the configuration. let visible = match id { Some(id) if let Some(&state) = auto.get(&id) => state, @@ -132,7 +137,7 @@ impl<'a> Inspector<'a> { _ => !base_off, }; let Some(id) = id else { continue }; - out.push(Layer { id, name: self.text(d, b"Name").unwrap_or_else(|| "Layer".into()), visible, initial: visible }); + out.push(Layer { id, name: self.field_text(d, b"Name").unwrap_or_else(|| "Layer".into()), visible, initial: visible }); } } diff --git a/crates/render/src/inspect/tests.rs b/crates/render/src/inspect/tests.rs index 68486d8097..c908b241cf 100644 --- a/crates/render/src/inspect/tests.rs +++ b/crates/render/src/inspect/tests.rs @@ -1161,6 +1161,50 @@ fn repeated_annotations_are_listed_once_within_a_byte_budget() { assert!(notes.iter().map(retained).sum::() <= MAX_ANNOTATION_BYTES); } +/// Blank text is not kept, so only reading it is charged: many comments sharing one large blank +/// string stop being built at the budget instead of decoding it once each. +#[test] +fn comments_sharing_a_blank_string_stop_at_the_read_budget() { + let small = "<< /Subtype /Text /Rect [0 0 1 1] /Contents 7 0 R >> ".repeat(1200); + let pdf = format!( + "%PDF-1.7\n1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj\n\ + 2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj\n\ + 3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 200 200] /Annots [{small}] >> endobj\n\ + 7 0 obj ({}) endobj\ntrailer << /Root 1 0 R >>\n%%EOF", + " ".repeat(64 << 10), + ); + let doc = Document::load_mem(pdf.as_bytes()).expect("loads"); + let page = PageInfo { width: 200.0, height: 200.0, crop: [0.0, 0.0, 200.0, 200.0], rotation: 0, label: String::new() }; + let mut info = DocInfo { pages: vec![page], ..Default::default() }; + Inspector::new(&doc).fill(&mut info); + assert!(!info.annotations.is_empty() && info.annotations.len() < 1200, "{}", info.annotations.len()); +} + +/// Many fields sharing one large tooltip, and one layer listed many times, are read within a byte +/// allowance rather than once each. +#[test] +fn fields_sharing_a_tooltip_and_repeated_layers_stop_at_the_text_budget() { + let fields = (0..1300).map(|i| format!("<< /FT /Tx /T (f{i}) /TU 7 0 R /V 7 0 R >> ")).collect::(); + let pdf = format!( + "%PDF-1.7\n1 0 obj << /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [{fields}] >> \ + /OCProperties << /OCGs [{}] >> >> endobj\n\ + 2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj\n\ + 3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 200 200] >> endobj\n\ + 6 0 obj << /Type /OCG /Name (Ink) >> endobj\n\ + 7 0 obj ({}) endobj\ntrailer << /Root 1 0 R >>\n%%EOF", + "6 0 R ".repeat(1000), + "x".repeat(32 << 10), + ); + let doc = Document::load_mem(pdf.as_bytes()).expect("loads"); + let page = PageInfo { width: 200.0, height: 200.0, crop: [0.0, 0.0, 200.0, 200.0], rotation: 0, label: String::new() }; + let mut info = DocInfo { pages: vec![page], ..Default::default() }; + Inspector::new(&doc).fill(&mut info); + let kept: usize = info.fields.iter().map(|f| f.tooltip.as_ref().map_or(0, String::len) + f.value.as_ref().map_or(0, String::len)).sum(); + assert!(kept > 0 && kept <= super::core::MAX_FIELD_TEXT_BYTES, "{kept}"); + assert!(info.fields.iter().any(|f| f.tooltip.is_none()), "text past the allowance is left out"); + assert_eq!(info.layers.len(), 1); +} + /// A link reached through an indirect object that holds only a reference to it is the same /// annotation as the link itself, and so is a page array reached the same way: each is listed once. #[test] @@ -2686,6 +2730,42 @@ fn inspection_keeps_no_private_page_data() { assert_eq!(report(with), report(without), "what inspection reports does not change"); } +/// A field with several widgets reports an action on any of them, not only the first, and so does +/// the document's JavaScript flag. +#[test] +fn a_field_reports_an_action_on_a_later_widget() { + let pdf = "%PDF-1.7\n1 0 obj << /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R] >> >> endobj\n\ + 2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj\n\ + 3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 200 200] >> endobj\n\ + 4 0 obj << /FT /Tx /T (f) /Kids [5 0 R 6 0 R] >> endobj\n\ + 5 0 obj << /Subtype /Widget /Rect [0 0 1 1] >> endobj\n\ + 6 0 obj << /Subtype /Widget /Rect [0 0 1 1] /A << /S /JavaScript /JS (app.alert(1)) >> >> endobj\n\ + trailer << /Root 1 0 R >>\n%%EOF"; + let doc = Document::load_mem(pdf.as_bytes()).expect("loads"); + let page = PageInfo { width: 200.0, height: 200.0, crop: [0.0, 0.0, 200.0, 200.0], rotation: 0, label: String::new() }; + let mut info = DocInfo { pages: vec![page], ..Default::default() }; + Inspector::new(&doc).fill(&mut info); + assert!(info.fields.iter().any(|f| f.name == "f" && f.has_actions), "{:?}", info.fields); + assert!(info.has_javascript); +} + +/// A shared descendant first reached at the depth limit (its action-bearing child cut) is read +/// again from a shallower path, so the action is still found. +#[test] +fn an_action_below_a_shared_field_first_met_deep_is_found_from_a_shallower_path() { + // 1 is the root, 2..=32 a chain of 31 fields, 33 the shared field, 34 holds the action, + // 35 a short path to 33. + let mut bodies = vec!["<< /Kids [2 0 R 35 0 R] >>".to_string()]; + for i in 2..=32 { + bodies.push(format!("<< /Kids [{} 0 R] >>", i + 1)); + } + bodies.push("<< /Kids [34 0 R] >>".into()); + bodies.push("<< /A << /S /JavaScript /JS (x) >> >>".into()); + bodies.push("<< /Kids [33 0 R] >>".into()); + let doc = Document::load_mem(&pdf_of(&bodies)).expect("loads"); + assert!(Inspector::new(&doc).has_action(&Object::Reference((1, 0)), 0)); +} + /// A `/Kids` entry that loops back, or names a root field that doesn't name this one as its /// `/Parent`, is not a child (ISO 32000-2 §12.7.3, Table 224; §12.7.4.1, Table 226): the field /// keeps its other kids and is listed with them. From abec4e3feea96d3566c7c026d5d137172fdf2596 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 15:50:29 -0500 Subject: [PATCH 16/22] fix(js): bound array text, month parsing, name registration and list work (#5883) * fix(js): bound array text, month parsing, name registration and list work - strings() (field value and colour setters, resetForm) copied every array entry into its own String with no total: one long string listed a million times was copied a million times. The copies together are now held to the run's MAX_PAYLOAD byte budget. - FormCalc parse_date copied and lowercased the rest of the text for every month token of the picture, quadratic in both; it reads at most the longest month name now. - XFA global registration checked each child name against a Vec of every name defined so far, quadratic in a wide form; it is a HashSet. - FormCalc gather charged one step for a whole collection and list_push charged nothing for a duplicate it dropped, so .all on same-named siblings did quadratic work for one step a pair; every node visited costs a step. - add_instance reserved only the new root's SOM before copying a subtree; the copy's nodes, names and SOMs are charged to the byte budget first. * address review: gather charges each call; check an array entry's length before copying it * Hold an array entry to its widest escaped size before copying it The pre-copy check compared the payload budget against a JS string's UTF-16 length, which is shorter than the escaped UTF-8 form it is copied to: a string of quotes or non-ASCII text could allocate up to six times the budget before the post-check rejected it. The check now uses the widest escaped width, so the copy cannot exceed the budget it is checked against. * review: measure an array entry's escaped width before refusing it * address review: count an array entry's escaped width without copying it * address review: charge retained field text and colours to a run-wide budget * fmt * fmt * merge main, keep both --- crates/js/src/document.rs | 11 +++++--- crates/js/src/formcalc/access.rs | 13 ++++++--- crates/js/src/formcalc/dates.rs | 8 ++++-- crates/js/src/host.rs | 40 ++++++++++++++++++++++++--- crates/js/src/run.rs | 1 + crates/js/src/tests.rs | 46 ++++++++++++++++++++++++++++++++ crates/js/src/xfa/globals.rs | 6 +++-- crates/js/src/xfa/instances.rs | 16 ++++++++++- 8 files changed, 126 insertions(+), 15 deletions(-) diff --git a/crates/js/src/document.rs b/crates/js/src/document.rs index f13c9434e9..cccf89a7a2 100644 --- a/crates/js/src/document.rs +++ b/crates/js/src/document.rs @@ -1,4 +1,6 @@ -use super::host::{FieldKey, arg, colour, error, function, host, read, s, set_value, strings, text, text_line, value_of, write}; +use super::host::{ + FieldKey, arg, charge, colour, error, function, host, kind_of, read, s, strings, text, text_bytes, text_line, value_for, value_of, write, +}; use super::model::{DISPLAY_HIDDEN, DISPLAY_VISIBLE, FieldType, Request}; use super::{Attribute, Context, JsArray, JsObject, JsResult, JsString, JsValue, NativeFunction, ObjectInitializer, js_string}; @@ -41,8 +43,9 @@ fn field_object_new(ctx: &mut Context, name: &str, id: Option, token: u64) let value_get = getter!(ctx, n, |f, c| value_of(&f, c)); let value_set = { fn set(_: &JsValue, args: &[JsValue], name: &FieldKey, c: &mut Context) -> JsResult { - let vals = strings(&arg(args, 0), c)?; - write(c, name, |f| set_value(f, vals))?; + let vals = value_for(kind_of(c, name)?, strings(&arg(args, 0), c)?); + charge(c, name, |f| text_bytes(&f.value), text_bytes(&vals))?; + write(c, name, |f| f.value = vals)?; Ok(JsValue::undefined()) } function(ctx, NativeFunction::from_copy_closure_with_captures(set, n.clone())) @@ -104,6 +107,7 @@ fn field_object_new(ctx: &mut Context, name: &str, id: Option, token: u64) let text_color_set = { fn set(_: &JsValue, args: &[JsValue], name: &FieldKey, c: &mut Context) -> JsResult { let v = strings(&arg(args, 0), c)?; + charge(c, name, |f| f.text_color.as_deref().map_or(0, text_bytes), text_bytes(&v))?; write(c, name, |f| f.text_color = Some(v.into()))?; Ok(JsValue::undefined()) } @@ -113,6 +117,7 @@ fn field_object_new(ctx: &mut Context, name: &str, id: Option, token: u64) let fill_color_set = { fn set(_: &JsValue, args: &[JsValue], name: &FieldKey, c: &mut Context) -> JsResult { let v = strings(&arg(args, 0), c)?; + charge(c, name, |f| f.fill_color.as_deref().map_or(0, text_bytes), text_bytes(&v))?; write(c, name, |f| f.fill_color = Some(v.into()))?; Ok(JsValue::undefined()) } diff --git a/crates/js/src/formcalc/access.rs b/crates/js/src/formcalc/access.rs index 26de7ba9eb..3c323ca088 100644 --- a/crates/js/src/formcalc/access.rs +++ b/crates/js/src/formcalc/access.rs @@ -33,9 +33,12 @@ impl Interp<'_> { Ok(()) } - /// `more` nodes into `out`, each once, never past [`MAX_LIST`]. + /// `more` nodes into `out`, each once, never past [`MAX_LIST`]. Every node visited costs a + /// step, kept or not, and the call itself one more: a search that matches nothing still + /// costs work when it is repeated over a long list. pub(super) fn gather(&mut self, out: &mut Vec, seen: &mut HashSet, more: impl IntoIterator) -> R<()> { for i in more { + self.step()?; if seen.insert(i) { out.push(i); if out.len() > MAX_LIST { @@ -43,7 +46,7 @@ impl Interp<'_> { } } } - self.charge(1) + self.step() } /// The nodes of `v`, each once. @@ -294,8 +297,10 @@ impl Interp<'_> { } } - /// `v` onto `out`: a node only once, never past [`MAX_LIST`]. + /// `v` onto `out`: a node only once, never past [`MAX_LIST`]. A duplicate costs its step + /// too: `all` on a list of same-named siblings makes one for every pair. fn list_push(&mut self, out: &mut Vec, seen: &mut HashSet, v: Val) -> R<()> { + self.step()?; if let Val::Node(i) = v && !seen.insert(i) { @@ -305,7 +310,7 @@ impl Interp<'_> { if out.len() > MAX_LIST { return Err(LIST_TOO_LONG.into()); } - self.charge(1) + Ok(()) } /// Property `name` of form object `i` as FormCalc reads it: its value, attributes, diff --git a/crates/js/src/formcalc/dates.rs b/crates/js/src/formcalc/dates.rs index 4d9befeab2..f201a23525 100644 --- a/crates/js/src/formcalc/dates.rs +++ b/crates/js/src/formcalc/dates.rs @@ -56,6 +56,9 @@ const CENTURY_SPLIT: i64 = 30; const MONTHS: [&str; 12] = ["January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"]; +/// Characters of the longest month name: all a month token needs to read of the text. +const LONGEST_MONTH: usize = 9; + /// Parse `text` as a date by `picture` (`YYYY-MM-DD`, `MM/DD/YYYY`, `MMM D, YYYY`, …). pub(super) fn parse_date(text: &str, picture: &str) -> Option<(i64, u32, u32)> { let pic: Vec = picture.chars().collect(); @@ -71,8 +74,9 @@ pub(super) fn parse_date(text: &str, picture: &str) -> Option<(i64, u32, u32)> { } i += run; if c == 'M' && run >= 3 { - // Month name. - let rest: String = txt.get(j..).unwrap_or(&[]).iter().collect(); + // Month name: only as much of the text as a name can be, so a picture of many + // month tokens does not copy the rest of a long text for each of them. + let rest: String = txt.get(j..).unwrap_or(&[]).iter().take(LONGEST_MONTH).collect(); let lower = rest.to_ascii_lowercase(); let found = MONTHS .iter() diff --git a/crates/js/src/host.rs b/crates/js/src/host.rs index 5966e63545..d829041ce6 100644 --- a/crates/js/src/host.rs +++ b/crates/js/src/host.rs @@ -27,6 +27,10 @@ pub(crate) struct Host { pub(crate) reset_limit_exceeded: bool, /// Bytes of text kept in alerts, console lines and requests, against [`xfa::MAX_PAYLOAD`]. pub(crate) kept: usize, + /// Bytes of text the script has added to the fields' values and colours, net of what it + /// replaced, against [`xfa::MAX_PAYLOAD`]: each assignment alone is capped, so without this + /// many fields could each hold a maximal array. + pub(crate) retained: usize, /// Alerts, console lines and requests the limits left out. pub(crate) dropped: usize, } @@ -262,12 +266,42 @@ pub(crate) fn value_of(f: &FieldState, ctx: &mut Context) -> JsValue { } } -pub(crate) fn set_value(f: &mut FieldState, vals: Vec) { - f.value = match f.kind { +/// Bytes of text in `v`, counting each string's heap size and its handle. +pub(crate) fn text_bytes(v: &[String]) -> usize { + v.iter().fold(0usize, |n, t| n.saturating_add(t.len()).saturating_add(std::mem::size_of::())) +} + +/// Charge the replacement of the text `old` selects from field `name` with `new_bytes` of text to +/// the run's retained-text budget; an assignment the budget has no room for is an error and +/// charges nothing. +pub(crate) fn charge(ctx: &Context, name: &FieldKey, old: impl FnOnce(&FieldState) -> usize, new_bytes: usize) -> JsResult<()> { + let i = field_index(ctx, name)?; + let mut h = host(ctx)?.borrow_mut(); + let freed = old(&h.fields[i]); + let total = h.retained.saturating_sub(freed).saturating_add(new_bytes); + if total > xfa::MAX_PAYLOAD { + return Err(error(format!("the fields' values and colours hold more than {} bytes of text", xfa::MAX_PAYLOAD))); + } + h.retained = total; + Ok(()) +} + +pub(crate) fn kind_of(ctx: &Context, name: &FieldKey) -> JsResult { + let i = field_index(ctx, name)?; + Ok(host(ctx)?.borrow().fields[i].kind) +} + +/// What a field of `kind` keeps of the values assigned to it. +pub(crate) fn value_for(kind: FieldType, vals: Vec) -> Vec { + match kind { FieldType::CheckBox | FieldType::RadioButton => vals.into_iter().filter(|v| v != "Off" && !v.is_empty()).take(1).collect(), FieldType::ListBox => vals, _ => vals.into_iter().take(1).collect(), - }; + } +} + +pub(crate) fn set_value(f: &mut FieldState, vals: Vec) { + f.value = value_for(f.kind, vals); } #[cfg(test)] diff --git a/crates/js/src/run.rs b/crates/js/src/run.rs index ececa107a3..7019bd768c 100644 --- a/crates/js/src/run.rs +++ b/crates/js/src/run.rs @@ -270,6 +270,7 @@ pub(crate) fn run_in_context( reset_calls: setup.reset_calls, reset_limit_exceeded: false, kept: 0, + retained: 0, dropped: 0, })); let mut out = Outcome { rc: true, value: event.value.clone(), change: event.change.clone(), ..Default::default() }; diff --git a/crates/js/src/tests.rs b/crates/js/src/tests.rs index 1a41afeea8..ece4443b7d 100644 --- a/crates/js/src/tests.rs +++ b/crates/js/src/tests.rs @@ -87,6 +87,17 @@ fn calculate_scripts_read_fields_and_set_the_value() { assert!(o.rc); } +/// An array of field values copies each entry: one long string listed many times is held to +/// the run's byte budget, not copied once per listing. +#[test] +fn an_array_of_values_stops_at_the_byte_budget() { + let fill = "var s = 'x'; while (s.length < 65536) s += s; var a = []; for (var i = 0; i < N; i++) a.push(s); getField('qty').value = a;"; + let o = go(&fill.replace('N', "600"), &Event::field("Calculate", "total", "")); + assert!(o.error.as_deref().is_some_and(|e| e.contains("byte limit exceeded")), "{:?}", o.error); + let o = go(&fill.replace('N', "400"), &Event::field("Calculate", "total", "")); + assert_eq!(o.error, None); +} + /// `event.value` reads and writes as a plain value, numbers included, and the outcome says whether /// the script assigned it, even to the value it already had. #[test] @@ -528,6 +539,7 @@ fn app_alert_titles_are_clipped_and_charged() { reset_calls: 0, reset_limit_exceeded: false, kept: 0, + retained: 0, dropped: 0, }; h.alert(Message { title: Some("Order form".into()), ..Message::plain("Saved") }); @@ -1113,6 +1125,24 @@ mod xfa_model { assert_eq!(o.result.as_deref(), Some("7")); } + /// A copy of a subform costs its nodes, names and SOMs: instances of one with a long name + /// stop at the run's byte budget, well before the instance cap. + #[test] + fn instances_of_a_subform_with_long_names_stop_at_the_byte_budget() { + let mut f = form(); + let table = &mut f.children[0].children[4]; + assert_eq!(table.name, "table"); + for row in &mut table.children { + row.children.push(field(&"n".repeat(1 << 20), "", "", false)); + } + let ev = XfaEvent { activity: "click".into(), target: "form1[0].page1[0].go[0]".into(), ..Default::default() }; + let o = run_xfa("for (var i = 0; i < 100; i++) table._row.addInstance(1); table._row.count", &ev, &XfaDoc::default(), &f, Limits::default()); + assert_eq!(o.error, None, "{:?}", o.error); + let count: usize = o.result.as_deref().unwrap_or_default().parse().expect("a count"); + assert!((3..50).contains(&count), "{count} instances"); + assert_eq!(o.effects.iter().filter(|e| matches!(e, XfaEffect::AddInstance { .. })).count(), count - 2); + } + /// On screen every page is a sheet of its own: `xfa.layout.sheet` counts it from 0, as /// `absPage` does (forms show `sheet(this) + 1`), and `sheetCount` is the page count. #[test] @@ -1625,6 +1655,10 @@ mod formcalc { #[test] fn date2num_rejects_days_past_the_end_of_the_month() { + // A month name is read from the text's next few characters only, whatever follows. + let serial = calc(r#"Date2Num("2024-09-01")"#); + assert_eq!(calc(r#"Date2Num("September 1, 2024, and a note that runs on", "MMMM D, YYYY")"#), serial); + assert_eq!(calc(r#"Date2Num("Sep 1, 2024", "MMM D, YYYY")"#), serial); assert_eq!(calc(r#"Date2Num("2023-02-30")"#), "0"); assert_eq!(calc(r#"Date2Num("31/04/2024", "DD/MM/YYYY")"#), "0"); assert_eq!(calc(r#"Date2Num("2023-02-29")"#), "0"); @@ -2641,6 +2675,18 @@ fn a_plain_form_sees_the_xfa_version_but_not_installed() { assert_eq!(o.alerts, [Message::plain("old viewer")]); } +/// Arrays assigned to several fields are held to one run-wide budget, and replacing a field's +/// array frees its charge. +#[test] +fn retained_array_text_is_charged_across_fields() { + let fill = "var s = 'x'; while (s.length < 65536) s += s; var a = []; for (var i = 0; i < 320; i++) a.push(s);"; + let one = |tail: &str| go(&format!("{fill} {tail}"), &Event::field("Calculate", "total", "")); + let o = one("getField('qty').textColor = a; getField('qty').textColor = a; getField('zip').textColor = ['RGB'];"); + assert_eq!(o.error, None, "replacing an array frees its charge: {:?}", o.error); + let o = one("getField('qty').textColor = a; getField('zip').fillColor = a;"); + assert!(o.error.as_deref().is_some_and(|e| e.contains("hold more than")), "{:?}", o.error); +} + #[test] fn string_arrays_refuse_aggregate_text_before_request_clipping() { let o = go("resetForm(new Array(65).fill('x'.repeat(1 << 19)));", &Event::doc("Console")); diff --git a/crates/js/src/xfa/globals.rs b/crates/js/src/xfa/globals.rs index de57d7fd49..46dc8a713b 100644 --- a/crates/js/src/xfa/globals.rs +++ b/crates/js/src/xfa/globals.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + use super::host::host; use super::instances::instance_manager; use super::model::{MAX_DEPTH, XfaDoc, XfaEffect, XfaEvent, clip, payload, reset_names}; @@ -379,7 +381,7 @@ pub(super) fn install(ctx: &mut Context, event: &XfaEvent, doc: &XfaDoc, current "DataView", "Intl", ]; - let mut defined: Vec = RESERVED.iter().map(ToString::to_string).collect(); + let mut defined: HashSet = RESERVED.iter().map(ToString::to_string).collect(); for scope in scopes { let kids: Vec<(usize, String, bool)> = { let h = host(ctx)?.borrow(); @@ -398,7 +400,7 @@ pub(super) fn install(ctx: &mut Context, event: &XfaEvent, doc: &XfaDoc, current let im = instance_manager(ctx, scope, &name)?; ctx.register_global_property(JsString::from(format!("_{name}").as_str()), im, rw)?; } - defined.push(name); + defined.insert(name); } } Ok(()) diff --git a/crates/js/src/xfa/instances.rs b/crates/js/src/xfa/instances.rs index 2985794dc3..c737ea9db9 100644 --- a/crates/js/src/xfa/instances.rs +++ b/crates/js/src/xfa/instances.rs @@ -137,6 +137,17 @@ fn child_som(parent_som: String, name: &str, index: usize) -> String { } } +/// Bytes a copy of `src`'s subtree keeps: a node, its name and its SOM (the copy's SOMs differ +/// in their indices only) for every node [`clone_subtree`] copies. +fn subtree_bytes(h: &XHost, src: usize, depth: usize) -> usize { + let Some(n) = h.nodes.get(src).filter(|_| depth <= MAX_DEPTH) else { return 0 }; + let own = std::mem::size_of::().saturating_add(n.name.len()).saturating_add(n.som.len()); + n.children + .iter() + .filter(|&&k| !h.nodes.get(k).is_some_and(|c| c.gone || c.repeatable && c.index > 0)) + .fold(own, |sum, &k| sum.saturating_add(subtree_bytes(h, k, depth + 1))) +} + /// A copy of `src`'s subtree (values cleared) under `parent`, returning the copy's index. fn clone_subtree(h: &mut XHost, src: usize, parent: Option, depth: usize) -> Option { if depth > MAX_DEPTH || h.nodes.len() >= MAX_NODES { @@ -214,7 +225,10 @@ impl XHost { let name = h.nodes.get(first).map(|n| n.name.to_string()).unwrap_or_default(); let parent_som = h.nodes.get(parent).map(|p| p.som.to_string()).unwrap_or_default(); let som = child_som(parent_som, &name, live.len()); - if !h.reserve(&XfaEffect::AddInstance { som: String::new() }, som.len()) { + // The copy's nodes, their names and SOMs, are charged to the run's byte budget before + // they are made: a subtree with long names must not be copied past it. + let copied = subtree_bytes(h, first, 0); + if !h.reserve(&XfaEffect::AddInstance { som: String::new() }, som.len().saturating_add(copied)) { return None; } let new = clone_subtree(h, first, Some(parent), 0)?; From 69936f1d1f2f589a907941fbc658ed3a75eea56b Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 15:50:56 -0500 Subject: [PATCH 17/22] fix(engine): run the renderer redaction check on redactions applied inside a batch (#5977) * fix(engine): run the renderer redaction check on redactions applied inside a batch * Resolve redaction marks by page object and measure the edited document The renderer check looked marks up by the page index they had when they were drawn, so a MovePages later in the same batch pointed them at another page's text, and it measured glyphs with page geometry from before the edit. Each mark now carries the page object it was placed on, and the geometry is read back from the document as the batch wrote it. * review: skip marks whose page the batch deleted * review: group marks by page once and refuse an ambiguous page tree * address review: refuse a batch that deletes a marked page while adding pages * merge main, keep both * address review: a batch that applies redactions and then copies a page is refused --- crates/engine/src/document.rs | 47 ++++++++++++++------ crates/engine/src/edit/run.rs | 27 +++++++++++- crates/engine/src/session/editing.rs | 20 ++++++--- crates/engine/src/tests.rs | 65 +++++++++++++++++++++++++++- crates/redact/src/lib.rs | 6 +++ 5 files changed, 142 insertions(+), 23 deletions(-) diff --git a/crates/engine/src/document.rs b/crates/engine/src/document.rs index 602410922f..805e089dd7 100644 --- a/crates/engine/src/document.rs +++ b/crates/engine/src/document.rs @@ -958,41 +958,60 @@ pub(crate) fn recovered_render_password(cos: &pdfcraft_cos::Document) -> Option< /// the document as it was. pub(crate) fn shown_under_redactions( after: &pdfcraft_cos::Document, - marks: &[pdfcraft_redact::Mark], + marks: &[(pdfcraft_cos::ObjRef, pdfcraft_redact::Mark)], overlays: &[pdfcraft_cos::ObjRef], - pages: &[pdfcraft_render::PageInfo], password: Option<&str>, ) -> Result<(), EditError> { use pdfcraft_cos::Object; use pdfcraft_redact::RedactError; - let mut marked: Vec = marks.iter().map(|m| m.page).collect(); + let mut shown = after.clone(); + let model = pdfcraft_model::pages(&shown); + // A batch can move pages after the marks were collected: a mark is found by the page object it + // was placed on, not by the index that page had when it was drawn. A page the batch deleted + // takes its marks with it, so they are not checked against whatever page now holds the index. + // One index, built once: each mark is resolved against it instead of scanning every page, + // and the marks are grouped by resolved page in the same pass. + let mut index: std::collections::HashMap = std::collections::HashMap::new(); + for (i, p) in model.iter().enumerate() { + index.entry(p.obj).or_insert(i); + } + let mut marked: Vec = Vec::new(); + let mut rects_of: std::collections::HashMap> = std::collections::HashMap::new(); + for (obj, m) in marks { + let Some(&pi) = index.get(obj) else { continue }; + if !rects_of.contains_key(&pi) { + marked.push(pi); + } + rects_of.entry(pi).or_default().extend(m.rects.iter().map(|r| [r[0].min(r[2]), r[1].min(r[3]), r[0].max(r[2]), r[1].max(r[3])])); + } marked.sort_unstable(); - marked.dedup(); if marked.is_empty() { return Ok(()); } - let mut shown = after.clone(); - let model = pdfcraft_model::pages(&shown); for &pi in &marked { let Some(page) = model.get(pi) else { continue }; let Some(Object::Array(list)) = page.dict.get(b"Contents").map(|c| (*shown.resolve(c)).clone()) else { continue }; let kept: Vec = list.into_iter().filter(|o| !o.as_ref().is_some_and(|r| overlays.contains(&r))).collect(); shown.update_dict(page.obj, |d| d.set(b"Contents".to_vec(), Object::Array(kept))).map_err(|e| EditError::Write(e.to_string()))?; } - let bytes = write_incremental(&shown, &SaveOptions::default()).map_err(|e| EditError::Write(e.to_string()))?; + let bytes = Arc::new(write_incremental(&shown, &SaveOptions::default()).map_err(|e| EditError::Write(e.to_string()))?); + // Page geometry comes from the document as it is now written: a page moved earlier in the batch + // brings its own size and rotation with it. + let pages = pdfcraft_render::inspect(bytes.clone(), password).map_err(|e| EditError::Write(e.to_string()))?.pages; + // The model counts one entry per `/Kids` entry; the renderer one per page it draws. If they + // disagree, a mark's index could land on another page's text, and the check cannot confirm + // the redaction. + if pages.len() != model.len() { + return Err(RedactError::AmbiguousPages.into()); + } let config = RenderConfig { password: password.map(Arc::from), ..Default::default() }; - let mut renderer = pdfcraft_render::PageRenderer::new(Arc::new(bytes), config); + let mut renderer = pdfcraft_render::PageRenderer::new(bytes, config); let mut residue = 0; for &pi in &marked { let request = pdfcraft_render::RenderRequest { page: pi, kind: pdfcraft_render::RequestKind::Text, scale: 1.0, ..Default::default() }; // A page that cannot be read back cannot be confirmed clean. let (Some(info), Some(text)) = (pages.get(pi), renderer.render(request).text) else { return Err(RedactError::Unreadable(pi + 1).into()) }; - let rects: Vec<[f64; 4]> = marks - .iter() - .filter(|m| m.page == pi) - .flat_map(|m| m.rects.iter()) - .map(|r| [r[0].min(r[2]), r[1].min(r[3]), r[0].max(r[2]), r[1].max(r[3])]) - .collect(); + let rects = rects_of.get(&pi).cloned().unwrap_or_default(); for glyph in text.glyphs.iter().filter(|g| !g.text.trim().is_empty()) { let [x0, y0, x1, y1] = info.view_rect_to_user(glyph.rect).map(f64::from); let area = (x1 - x0) * (y1 - y0); diff --git a/crates/engine/src/edit/run.rs b/crates/engine/src/edit/run.rs index 57e5729fc8..0a7a348fca 100644 --- a/crates/engine/src/edit/run.rs +++ b/crates/engine/src/edit/run.rs @@ -253,6 +253,11 @@ pub(crate) struct EditCtx { /// The overlay streams the edit's applied redactions added, which alone the renderer's check /// leaves out. pub(crate) redaction_overlays: Vec, + /// The marks the edit's applied redactions took out (a batch's included), as they were when + /// each apply ran, for the renderer's check. + /// Each applied mark with the page object it was drawn on, which a later page move changes the + /// index of but not the identity. + pub(crate) redaction_marks: Vec<(pdfcraft_cos::ObjRef, pdfcraft_redact::Mark)>, /// What the edit's applied redactions left as it was that still holds the removed text. pub(crate) redaction_elsewhere: Option, /// What became of the PDF/A and PDF/UA identification when the edit's applied redactions @@ -310,6 +315,7 @@ impl EditCtx { rng: getrandom::fill, renamed_fields: Vec::new(), redaction_overlays: Vec::new(), + redaction_marks: Vec::new(), redaction_elsewhere: None, redaction_identification: pdfcraft_redact::Identification::default(), prepared: None, @@ -529,7 +535,17 @@ pub(crate) fn run_edit(doc: &mut pdfcraft_cos::Document, edit: &Edit, cx: &mut E pdfcraft_organize::insert_blank_page(doc, *at, *width, *height)?; } Edit::SetInfo { key, value } => pdfcraft_organize::set_info(doc, key, value)?, - Edit::DuplicatePages { pages, at } => pdfcraft_organize::duplicate_pages(doc, pages, *at)?, + Edit::DuplicatePages { pages, at } => { + // A copy of a page cannot be tied back to the marks an earlier redaction of this edit + // applied: text put back under a mark would sit on the copy, unchecked. Fail closed. + if !cx.redaction_marks.is_empty() { + return Err(EditError::Invalid( + "a batch that applies redactions can't also copy pages: the copies can't be checked against the marks; the document was not changed" + .into(), + )); + } + pdfcraft_organize::duplicate_pages(doc, pages, *at)? + } Edit::ReplacePages { pages, name, bytes, src_pages } => { let src = crate::sources::open_source_into(name, bytes, doc)?; pdfcraft_organize::replace_pages(doc, pages, &src, src_pages)?; @@ -991,8 +1007,15 @@ pub(crate) fn run_edit(doc: &mut pdfcraft_cos::Document, edit: &Edit, cx: &mut E pdfcraft_edit::add_content(doc, *page, &content)?; } Edit::ApplyRedactions { pages } => { + let model = pdfcraft_model::pages(doc); + cx.redaction_marks.extend( + pdfcraft_redact::marks(doc) + .into_iter() + .filter(|m| pages.as_ref().is_none_or(|p| p.contains(&m.page))) + .filter_map(|m| model.get(m.page).map(|p| (p.obj, m))), + ); let report = pdfcraft_redact::apply(doc, pages.as_deref())?; - cx.redaction_overlays = report.overlays; + cx.redaction_overlays.extend(report.overlays); cx.redaction_elsewhere = Some(report.still_holding); cx.redaction_identification = report.identification; } diff --git a/crates/engine/src/session/editing.rs b/crates/engine/src/session/editing.rs index 4a6f372fb4..a039f56798 100644 --- a/crates/engine/src/session/editing.rs +++ b/crates/engine/src/session/editing.rs @@ -185,6 +185,7 @@ impl Session { } cx.renamed_fields.clear(); cx.redaction_overlays.clear(); + cx.redaction_marks.clear(); // `next` is a copy: if the edit fails or crashes, the document is unchanged. run_checked_edit(&mut next, &edit, &mut cx)?; // A field's enter or exit scripts that changed nothing leave no edit behind (no undo step, @@ -195,11 +196,20 @@ impl Session { return Ok(None); } // Redaction removed what its own interpreter places under the marks. The marks were drawn - // over the page as the renderer shows it, so the renderer has the last word. - if let Edit::ApplyRedactions { pages } = &edit { - let marks: Vec = - pdfcraft_redact::marks(&editor.cos).into_iter().filter(|m| pages.as_ref().is_none_or(|p| p.contains(&m.page))).collect(); - guard(|| shown_under_redactions(&next, &marks, &cx.redaction_overlays, &doc.info.pages, doc.password.as_deref())) + // over the page as the renderer shows it, so the renderer has the last word: on every + // apply the edit ran, a batch's included. + if !cx.redaction_marks.is_empty() { + // A batch that deletes a marked page and adds pages cannot be tied back to the marks: + // a copy of a page may carry the text the redaction removed from the original only. + // Fail closed. + let before: std::collections::HashSet<_> = pdfcraft_model::pages(&editor.cos).iter().map(|p| p.obj).collect(); + let after: std::collections::HashSet<_> = pdfcraft_model::pages(&next).iter().map(|p| p.obj).collect(); + if cx.redaction_marks.iter().any(|(o, _)| !after.contains(o)) && after.iter().any(|o| !before.contains(o)) { + return Err(EditError::Invalid( + "a batch that deletes a page with a redaction mark and adds pages can't be verified; the document was not changed".into(), + )); + } + guard(|| shown_under_redactions(&next, &cx.redaction_marks, &cx.redaction_overlays, doc.password.as_deref())) .unwrap_or_else(|m| Err(EditError::Invalid(format!("{} failed unexpectedly ({m}); the document was not changed", edit.label()))))?; } // Renamed fields are said, not slipped in: a repair-log note (with the caveat that diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 4e24a4f608..91b00df4cc 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3542,9 +3542,10 @@ fn the_renderer_check_refuses_text_still_shown_under_a_mark() { mark_shown_text(&mut s, id, "SECRET42"); let doc = s.get(id).unwrap(); let mut cos = doc.editor.as_ref().unwrap().cos.clone(); - let marks = pdfcraft_redact::marks(&cos); + let model = pdfcraft_model::pages(&cos); + let marks: Vec<_> = pdfcraft_redact::marks(&cos).into_iter().filter_map(|m| model.get(m.page).map(|p| (p.obj, m))).collect(); let refused = |cos: &pdfcraft_cos::Document| { - let shown = shown_under_redactions(cos, &marks, &[], &doc.info.pages, None); + let shown = shown_under_redactions(cos, &marks, &[], None); matches!(shown, Err(EditError::Redact(pdfcraft_redact::RedactError::Residue(n))) if n > 0) }; assert!(refused(&cos)); @@ -13778,6 +13779,66 @@ fn image_transform_engine_refuses_singular_matrices_without_mutation() { } } +/// The renderer's check covers a redaction applied inside a batch too, with the marks as they +/// were when it ran: one the batch itself added is checked, and text a later edit of the batch +/// puts back under a mark refuses the whole batch. +#[test] +fn redactions_applied_in_a_batch_get_the_renderer_check() { + let mut s = Session::new().with_clock(|| 1_700_000_000); + let page = one_line_page_with("/Font << /F1 4 0 R >>", "", "BT /F1 16 Tf 20 150 Td (Public words then SECRET42) Tj ET"); + let id = s.open("batch.pdf", None, Arc::new(page), None).unwrap(); + mark_shown_text(&mut s, id, "SECRET42"); + let apply = Edit::ApplyRedactions { pages: None }; + // Text added where the mark was shows under it again: the renderer sees it and refuses. + let under = AddedText { rect: [150.0, 130.0, 300.0, 160.0], text: "SECRET42 SECRET42".into(), ..Default::default() }; + let refused = s.apply(id, Edit::Batch { label: "Redact then write".into(), edits: vec![apply.clone(), Edit::AddText { page: 0, text: under }] }); + assert!(matches!(refused, Err(EditError::Redact(pdfcraft_redact::RedactError::Residue(n))) if n > 0), "{refused:?}"); + assert!(page_texts(&s, id)[0].contains("SECRET42"), "nothing changed"); + assert_eq!(s.get(id).unwrap().redaction_marks(), 1); + // Applied alone in a batch, the redaction goes through, and the text is gone. + s.apply(id, Edit::Batch { label: "Redact".into(), edits: vec![apply] }).unwrap(); + assert!(!page_texts(&s, id)[0].contains("SECRET42"), "{:?}", page_texts(&s, id)); + assert_eq!(s.get(id).unwrap().redaction_marks(), 0); +} + +/// A batch that deletes a page holding a redaction mark and adds pages is refused whole: the +/// marks cannot be tied to copies, so redaction fails closed. Without added pages it goes through. +#[test] +fn a_batch_that_deletes_a_marked_page_and_adds_pages_is_refused() { + let mut s = Session::new().with_clock(|| 1_700_000_000); + let page = one_line_page_with("/Font << /F1 4 0 R >>", "", "BT /F1 16 Tf 20 150 Td (Public words then SECRET42) Tj ET"); + let id = s.open("batch.pdf", None, Arc::new(page), None).unwrap(); + mark_shown_text(&mut s, id, "SECRET42"); + let edits = vec![ + Edit::ApplyRedactions { pages: None }, + Edit::InsertBlankPage { at: 1, width: 612.0, height: 792.0 }, + Edit::DeletePages { pages: vec![0] }, + ]; + let refused = s.apply(id, Edit::Batch { label: "x".into(), edits }); + assert!(matches!(refused, Err(EditError::Invalid(_))), "{refused:?}"); + assert!(page_texts(&s, id)[0].contains("SECRET42"), "nothing changed"); + assert_eq!(s.get(id).unwrap().redaction_marks(), 1); +} + +/// A batch that applies redactions and then copies a page is refused whole: the copy can't be tied +/// to the marks, so text restored under one would be unchecked. Copying first, or alone, is fine. +#[test] +fn a_batch_that_applies_redactions_and_copies_a_page_is_refused() { + let mut s = Session::new().with_clock(|| 1_700_000_000); + let page = one_line_page_with("/Font << /F1 4 0 R >>", "", "BT /F1 16 Tf 20 150 Td (Public words then SECRET42) Tj ET"); + let id = s.open("batch.pdf", None, Arc::new(page), None).unwrap(); + mark_shown_text(&mut s, id, "SECRET42"); + let apply = Edit::ApplyRedactions { pages: None }; + let copy = Edit::DuplicatePages { pages: vec![0], at: None }; + let refused = s.apply(id, Edit::Batch { label: "x".into(), edits: vec![apply.clone(), copy.clone()] }); + assert!(matches!(&refused, Err(EditError::Invalid(m)) if m.contains("copy pages")), "{refused:?}"); + assert!(page_texts(&s, id)[0].contains("SECRET42"), "nothing changed"); + assert_eq!(s.get(id).unwrap().redaction_marks(), 1); + // Copying before the redaction is applied puts the mark on the copy too, and both are checked. + s.apply(id, Edit::Batch { label: "y".into(), edits: vec![copy, apply] }).unwrap(); + assert!(page_texts(&s, id).iter().all(|t| !t.contains("SECRET42")), "{:?}", page_texts(&s, id)); +} + /// A reset in an opening script reaches a field of a row an earlier opening script added, before /// the form is laid out again with the row's widgets: its data goes back to its default and stays /// so when the form is saved. diff --git a/crates/redact/src/lib.rs b/crates/redact/src/lib.rs index 8f54cefdc6..819cae99cd 100644 --- a/crates/redact/src/lib.rs +++ b/crates/redact/src/lib.rs @@ -58,6 +58,12 @@ pub enum RedactError { NothingToApply, #[error("page {0} has content that can't be read, so it can't be redacted safely")] Unreadable(usize), + /// The page tree and the renderer count a different number of pages (a page repeated in + /// `/Kids`, say), so a mark's page can't be matched to the page the renderer drew. + #[error( + "the document's page tree and its rendered pages disagree on how many pages there are, so the redaction could not be verified; nothing was changed" + )] + AmbiguousPages, /// Redact annotations with no readable area (neither usable `/QuadPoints` nor a usable `/Rect`). /// Skipping them would leave the text under them in the file while the others report success. #[error( From 1d7a9f9ffe45247dbc20a2047a4d2b151cb8385c Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 15:51:22 -0500 Subject: [PATCH 18/22] fix(edit,a11y): font aliases share the metrics built for their font (#5998) * fix(edit,a11y): font aliases share the metrics built for their font Text extraction and heading extraction built and kept font metrics per resource name, so many names for one font object each held a copy of its Unicode and width tables. * fmt * address review: font metrics are cached by the object at the end of the reference chain --- crates/a11y/src/headings.rs | 29 ++++++++++++++++++++++++++--- crates/edit/src/tests.rs | 33 +++++++++++++++++++++++++++++++++ crates/edit/src/text.rs | 2 ++ crates/edit/src/text/interp.rs | 22 +++++++++++++++++++++- 4 files changed, 82 insertions(+), 4 deletions(-) diff --git a/crates/a11y/src/headings.rs b/crates/a11y/src/headings.rs index 235ba4b452..a9de970b65 100644 --- a/crates/a11y/src/headings.rs +++ b/crates/a11y/src/headings.rs @@ -206,7 +206,9 @@ fn sub(doc: &Document, d: &Dict, key: &[u8]) -> Dict { pub(crate) fn mcid_texts(doc: &Document, page: &Dict) -> HashMap { let res = sub(doc, page, b"Resources"); let (fonts, props) = (sub(doc, &res, b"Font"), sub(doc, &res, b"Properties")); - let mut metrics: HashMap, Metrics> = HashMap::new(); + let mut metrics: HashMap, std::rc::Rc> = HashMap::new(); + // Built once per font object, however many resource names alias it. + let mut by_font: HashMap> = HashMap::new(); // The id each open marked-content sequence is in: its own, else the enclosing one's. let mut stack: Vec> = Vec::new(); let mut out: HashMap = HashMap::new(); @@ -255,9 +257,30 @@ pub(crate) fn mcid_texts(doc: &Document, page: &Dict) -> HashMap { gs.font = op.name(0).map(<[u8]>::to_vec); if let Some(name) = op.name(0) && !metrics.contains_key(name) - && let Some(f) = fonts.get(name).map(|f| doc.resolve(f)).and_then(|f| f.as_dict().cloned()) { - metrics.insert(name.to_vec(), Metrics::from_dict(doc, &f)); + // Keyed by the object at the end of the reference chain, so names that reach one + // font through different chains share its metrics (the hop limit is resolve's). + let font = fonts.get(name).and_then(|o| { + let (mut cur, mut last) = (o.clone(), None); + for _ in 0..16 { + let Object::Ref(r) = cur else { break }; + last = Some(r); + cur = (*doc.get(r)).clone(); + } + last + }); + let built = match font.and_then(|r| by_font.get(&r)) { + Some(m) => Some(m.clone()), + None => { + fonts.get(name).map(|f| doc.resolve(f)).and_then(|f| f.as_dict().map(|d| std::rc::Rc::new(Metrics::from_dict(doc, d)))) + } + }; + if let Some(m) = built { + if let Some(r) = font { + by_font.insert(r, m.clone()); + } + metrics.insert(name.to_vec(), m); + } } } b"Tc" => gs.char_spacing = op.num(0).unwrap_or(0.0), diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index 1f2b925b46..b956cb7079 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -14042,6 +14042,39 @@ fn review_inside_scope_group_move_matches_direct_transform_under_outer_scale() { } } +/// A page that names one font under 300 resource names builds its metrics once: each alias +/// used to build (and keep) a set of its own. +#[test] +fn font_aliases_share_one_set_of_metrics() { + let fonts: String = (0..300).map(|i| format!(" /A{i} 5 0 R")).collect(); + let content: String = format!("BT 72 700 Td {} ET", (0..300).map(|i| format!("/A{i} 12 Tf (x) Tj ")).collect::()); + let objs: Vec = vec![ + "<< /Type /Catalog /Pages 2 0 R >>".into(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 >>".into(), + format!("<< /Type /Page /Parent 2 0 R /MediaBox [0 0 600 800] /Contents 4 0 R /Resources << /Font <<{fonts} >> >> >>"), + format!("<< /Length {} >>\nstream\n{content}\nendstream", content.len()), + "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>".into(), + ]; + let mut out = b"%PDF-1.7\n".to_vec(); + let mut offs = Vec::new(); + for (i, o) in objs.iter().enumerate() { + offs.push(out.len()); + out.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes()); + } + let x = out.len(); + out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for o in offs { + out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{x}\n%%EOF\n", objs.len() + 1).as_bytes()); + let doc = Document::open(Arc::new(out)).unwrap(); + text::METRICS_BUILT.with(|n| n.set(0)); + let lines = text::text_lines(&doc, 0).unwrap(); + assert!(lines.iter().map(|l| l.text.matches('x').count()).sum::() >= 300, "{lines:?}"); + let built = text::METRICS_BUILT.with(std::cell::Cell::get); + assert!(built < 10, "metrics built {built} times for one font"); +} + /// Each `Tj`/`TJ` moves the text matrix by the width of its text. A run shown later in the same /// text object without a `Td` or `Tm` of its own (a raised run in another colour, a column after /// a `TJ` gap) is placed by those advances. Rewriting the paragraph before it must keep them, as diff --git a/crates/edit/src/text.rs b/crates/edit/src/text.rs index b58c41303f..eba8834c2b 100644 --- a/crates/edit/src/text.rs +++ b/crates/edit/src/text.rs @@ -54,6 +54,8 @@ pub(crate) use fallback::{SOURCE_SPACE_CALLS, take_source_character_measurements #[cfg(test)] use fallback::{add_type3_font, is_type3_fallback}; pub(crate) use fallback::{glyph_path, type3_font_dict, unique_font_name}; +#[cfg(test)] +pub(crate) use interp::METRICS_BUILT; use interp::{Origin, Paint, union}; #[cfg(test)] pub(crate) use lines::dropped_advances_within; diff --git a/crates/edit/src/text/interp.rs b/crates/edit/src/text/interp.rs index 0bc9373899..e628e017c4 100644 --- a/crates/edit/src/text/interp.rs +++ b/crates/edit/src/text/interp.rs @@ -664,6 +664,13 @@ impl Carry { /// The advance is an error message when the font's writing mode can't be read. type CachedFont = (Rc, Option, Result, String>); +#[cfg(test)] +thread_local! { + /// Font metrics built by [`interpret`] on this thread, for the test that keeps font aliases + /// sharing one set. + pub(crate) static METRICS_BUILT: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + /// A `Do` met while interpreting: its operator index, XObject name and the state there. type Invocation = (usize, Vec, Ts); @@ -976,6 +983,7 @@ pub(super) fn interpret( let fonts_res = super::resource_dict(doc, resources, b"Font", &mut held_fonts).unwrap_or(&none); let states = super::resource_dict(doc, resources, b"ExtGState", &mut held_states).unwrap_or(&none); let mut out: Vec = Vec::new(); + let mut by_font: HashMap = HashMap::new(); let mut ts = carry.ts.clone(); let mut at_bt = (0usize, ts.clone()); let mut stack: Vec<(Ts, Matrix, Matrix)> = std::mem::take(&mut carry.stack); @@ -1105,11 +1113,23 @@ pub(super) fn interpret( ts.size = op.num(1).unwrap_or(ts.size); if let Some(name) = op.name(0) { let (m, face, vertical) = cache.entry(name.to_vec()).or_insert_with(|| { + // Names that refer to one font object share what was built for it: a + // thousand aliases of one font are one set of metrics, not a thousand. + let font = fonts_res.get(name).and_then(Object::as_ref); + if let Some(built) = font.and_then(|r| by_font.get(&r)) { + return built.clone(); + } + #[cfg(test)] + METRICS_BUILT.with(|n| n.set(n.get() + 1)); let dict = fonts_res.get(name).and_then(|f| doc.resolve(f).as_dict().cloned()); let metrics = dict.as_ref().map_or_else(Metrics::fallback, |d| Metrics::from_dict(doc, d)); let vertical = dict.as_ref().map_or(Ok(None), |d| vertical_advance(doc, d).map_err(|e| e.to_string())); let face = dict.filter(is_type3_fallback).map(|_| metrics.base_font.clone()); - (Rc::new(metrics), face, vertical) + let built: CachedFont = (Rc::new(metrics), face, vertical); + if let Some(r) = font { + by_font.insert(r, built.clone()); + } + built }); ts.font = Some((name.to_vec(), m.clone())); ts.face = face.clone(); From 8c99ec4e084a02af89ab9b20f5502be15ca04a47 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Wed, 11 Nov 2026 23:04:31 +0100 Subject: [PATCH 19/22] CI: nightly release dry run (#7040) release.yml also runs nightly on main with craft-common-actions' dry-run: every release build except macOS, with nothing signed, pushed or drafted. Breakage that only the release build hits (riscv64, Flatpak, FreeBSD, packaging) then shows up the next morning instead of on release day, as it did on 11 Nov 2026. --- .github/workflows/release.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 4b44d79b68..d214d6738b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -1,6 +1,10 @@ name: Release on: + # A nightly dry run on main: every release build except macOS, nothing signed or released + # (craft-common-actions release.yml `dry-run`). Finds release-only breakage before release day. + schedule: + - cron: "20 2 * * *" push: branches: [release] workflow_dispatch: @@ -30,6 +34,7 @@ jobs: # Intentionally tracked at @main (not SHA-pinned) so org-wide CI improvements, security fixes, and build optimizations propagate without PR churn across all craftapp repos. uses: storytold/craft-common-actions/.github/workflows/release.yml@main with: + dry-run: ${{ github.event_name == 'schedule' }} app-name: pdfcraft app-title: PdfCraft craft-fonts-ref: b076417b5880bd4154bd22f00695b1d28c6bea83 From 9f763636fd46ce487173925c63799cc87774fc24 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 18:16:31 -0500 Subject: [PATCH 20/22] fix(forms,xfa): find every password field before a save skips scrubbing it (#5864) * fix(forms,xfa): find every password field before a save skips scrubbing it The quick Password-flag check in pdfcraft-forms stopped silently at its depth limit, and visited the tree in an order where a field the full field walk reaches could be cut off on a long branch first. It now answers true (read every field) whenever it cannot look as far as the field walk, and also walks below fields adopted from page widgets. XFA password classification walked AcroForm fields only 32 levels deep, shallower than editing and dataset synchronization, and read the datasets packet of an XFA array without the prefixes its preamble declares, so a typed password could stay in the saved datasets. It now walks as deep as the field walk, refuses to clear (and so to save) when the tree is deeper than it can check, and reads the packet the way the datasets writer does. * address review: check every page widget's /Parent chain for the password flag * address review: password scan follows the parents of nameless /Fields entries; field-walk completeness checked before the datasets packet is read * address review: the field walk's completeness is decided after every root; the subtree scan starts at the last dictionary of the parent chain * address review: a skipped visit does not replace the reach a field was classified with --- crates/forms/src/tests.rs | 85 ++++++++++++++++++++++++++++++++ crates/forms/src/values.rs | 74 +++++++++++++++++++++------ crates/xfa/src/data/passwords.rs | 80 +++++++++++++++++++++--------- crates/xfa/src/data/path.rs | 27 ++++++++++ crates/xfa/src/tests.rs | 68 +++++++++++++++++++++++++ 5 files changed, 298 insertions(+), 36 deletions(-) diff --git a/crates/forms/src/tests.rs b/crates/forms/src/tests.rs index a2a87f3a73..2ce02626c9 100644 --- a/crates/forms/src/tests.rs +++ b/crates/forms/src/tests.rs @@ -5601,6 +5601,75 @@ fn a_page_widget_inheriting_password_flag_beyond_depth_32_is_recognized() { assert!(!has_password_values(&doc)); } +/// A field reachable both near the top of the tree and at the end of a long branch: the field +/// walk reaches it near the top and reads the password field below it, so the quick check must +/// not stop at the long branch's depth limit and conclude there is none. The password field's +/// widget is on the page but names no `/Parent`, so the page widgets alone don't find it either. +#[test] +fn a_password_field_past_the_depth_limit_on_one_branch_is_still_found() { + let chain = 64; + let x = 6 + chain; // the shared field + let y = x + 1; // the password field + let w = y + 1; // its widget + let mut objs: Vec = vec![ + "<< /Type /Catalog /Pages 2 0 R /AcroForm 4 0 R >>".into(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 600 800] >>".into(), + format!("<< /Type /Page /Parent 2 0 R /Annots [{w} 0 R] >>"), + "<< /Fields [5 0 R 6 0 R] /DA (/Helv 0 Tf 0 g) >>".into(), + format!("<< /T (short) /Kids [{x} 0 R] >>"), + ]; + for d in 0..chain { + let next = if d + 1 == chain { x } else { 7 + d }; + objs.push(format!("<< /T (n{d}) /Kids [{next} 0 R] >>")); + } + objs.push(format!("<< /T (shared) /Kids [{y} 0 R] >>")); + objs.push(format!("<< /FT /Tx /T (pin) /Ff 8192 /V (deepsecret) /Kids [{w} 0 R] >>")); + objs.push("<< /Type /Annot /Subtype /Widget /Rect [50 600 150 620] /P 3 0 R >>".into()); + let mut doc = document(&objs); + assert!(fields(&doc).iter().any(|f| f.has(flags::PASSWORD)), "the field walk reads the password field"); + assert!(has_password_flag(&doc)); + assert!(has_password_values(&doc)); + assert_eq!(clear_password_values(&mut doc).unwrap(), 1); + assert!(!has_password_values(&doc)); +} + +/// A password field below an unlisted field whose other widget is on the page: the field walk +/// adopts the whole unlisted field from that widget, so the quick check looks below it too. +#[test] +fn a_password_field_below_an_adopted_page_field_is_found() { + let mut doc = document(&[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm 4 0 R >>".into(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 600 800] >>".into(), + "<< /Type /Page /Parent 2 0 R /Annots [6 0 R] >>".into(), + "<< /Fields [] /DA (/Helv 0 Tf 0 g) >>".into(), + "<< /T (grp) /Kids [6 0 R 7 0 R] >>".into(), + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (name) /Parent 5 0 R /Rect [50 700 150 720] /P 3 0 R >>".into(), + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (pin) /Ff 8192 /V (secret8) /Parent 5 0 R /Rect [0 0 0 0] >>".into(), + ]); + assert!(fields(&doc).iter().any(|f| f.has(flags::PASSWORD)), "the field walk adopts the password field"); + assert!(has_password_flag(&doc)); + assert!(has_password_values(&doc)); + assert_eq!(clear_password_values(&mut doc).unwrap(), 1); + assert!(!has_password_values(&doc)); +} + +/// A nameless widget listed in `/Fields` and on no page, whose `/Parent` is an unlisted password +/// field: the field walk adopts the parent, so the quick check follows the entry's `/Parent` too. +#[test] +fn a_password_parent_of_a_nameless_fields_entry_is_found() { + let mut doc = document(&[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm 4 0 R >>".into(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 600 800] >>".into(), + "<< /Type /Page /Parent 2 0 R >>".into(), + "<< /Fields [6 0 R] /DA (/Helv 0 Tf 0 g) >>".into(), + "<< /T (pin) /FT /Tx /Ff 8192 /V (secret8) /Kids [6 0 R] >>".into(), + "<< /Type /Annot /Subtype /Widget /Parent 5 0 R /Rect [0 0 0 0] >>".into(), + ]); + assert!(fields(&doc).iter().any(|f| f.has(flags::PASSWORD) && f.value.iter().any(|v| v == "secret8")), "the field walk reads the parent"); + assert!(has_password_flag(&doc)); + assert_eq!(clear_password_values(&mut doc).unwrap(), 1); +} + /// #4018: a value that doesn't fit its box at the field's font size is reported (it is drawn cut /// at the box). An auto-sized field shrinks to fit, and a short or empty value fits. #[test] @@ -6802,6 +6871,22 @@ fn locked_button_export_edits_leave_the_document_unchanged() { } } +/// A widget reached from `/Fields` as an unnamed kid of a plain field, whose own `/Parent` is an +/// unlisted password field: the quick check must still read that `/Parent` chain. +#[test] +fn a_page_widget_seen_under_fields_still_has_its_parent_chain_checked() { + let doc = document(&[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm 4 0 R >>".into(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 600 800] >>".into(), + "<< /Type /Page /Parent 2 0 R /Annots [6 0 R] >>".into(), + "<< /Fields [5 0 R] /DA (/Helv 0 Tf 0 g) >>".into(), + "<< /T (grp) /Kids [6 0 R] >>".into(), + "<< /Type /Annot /Subtype /Widget /Parent 7 0 R /Rect [50 700 150 720] /P 3 0 R >>".into(), + "<< /FT /Tx /T (pin) /Ff 8192 /V (secret9) /Kids [6 0 R] >>".into(), + ]); + assert!(has_password_flag(&doc)); +} + /// A radio button that is read-only on its own (its own `/Ff`) can't be picked, and a selected one /// can't be switched away from, though the group as a whole is writable. #[test] diff --git a/crates/forms/src/values.rs b/crates/forms/src/values.rs index b60d8aa140..3600695c03 100644 --- a/crates/forms/src/values.rs +++ b/crates/forms/src/values.rs @@ -758,29 +758,69 @@ fn password_fields(doc: &Document) -> Vec { /// again only makes the answer cautious. It reads only `/Ff`, `/Kids` and `/Parent`, so a save /// or recovery snapshot of a form without such a field doesn't build every [`Field`]. Like /// [`fields`], it looks at the form's field tree and at the widgets on the pages, which may be -/// missing from `/Fields`. Each dictionary is visited once and at most 64 levels deep. +/// missing from `/Fields`, together with the whole tree above and below them that [`fields`] +/// adopts. Each dictionary is visited once. +/// +/// It must never answer `false` for a form whose [`fields`] include a password field, or a typed +/// password would be saved. So where it can't look as far as [`fields`] may (a branch deeper than +/// the field walk's limit, which depends on the order the tree is visited in, or a `/Parent` +/// chain too long to follow), it answers `true` and the caller reads every field. pub(crate) fn has_password_flag(doc: &Document) -> bool { + const MAX_FIELD_DEPTH: usize = 64; let sets_it = |r: ObjRef| doc.get(r).as_dict().and_then(|d| field_flags(doc, d)).is_some_and(|ff| ff & flags::PASSWORD != 0); - let top = acroform(doc).and_then(|af| af.get(b"Fields").map(|o| doc.resolve(o)).and_then(|o| o.as_array().cloned())).unwrap_or_default(); - let mut stack: Vec<(Object, usize)> = top.into_iter().map(|o| (o, 0)).collect(); + let kids_of = + |r: ObjRef| doc.get(r).as_dict().and_then(|d| d.get(b"Kids").map(|k| doc.resolve(k)).and_then(|k| k.as_array().cloned())).unwrap_or_default(); let mut seen = std::collections::HashSet::new(); - while let Some((o, depth)) = stack.pop() { - let Object::Ref(r) = o else { continue }; - if depth > 64 || !seen.insert(r) { - continue; + // Whether a dictionary below `start` sets the flag, or the walk was cut short. + let subtree = |start: Vec, seen: &mut std::collections::HashSet| { + let mut stack: Vec<(Object, usize)> = start.into_iter().map(|o| (o, 0)).collect(); + while let Some((o, depth)) = stack.pop() { + let Object::Ref(r) = o else { continue }; + if !seen.insert(r) { + continue; + } + if depth > MAX_FIELD_DEPTH || sets_it(r) { + return true; + } + stack.extend(kids_of(r).into_iter().map(|k| (k, depth + 1))); } - if sets_it(r) { + false + }; + let top = acroform(doc).and_then(|af| af.get(b"Fields").map(|o| doc.resolve(o)).and_then(|o| o.as_array().cloned())).unwrap_or_default(); + let top_refs: Vec = top.iter().filter_map(Object::as_ref).collect(); + if subtree(top, &mut seen) { + return true; + } + // A `/Fields` entry may itself name a field above it through `/Parent` (one `fields` adopts when + // the entry has no name of its own), whether or not a page lists it. + for r in top_refs { + let chain = field_chain(doc, r); + if chain.len() >= MAX_FIELD_DEPTH || chain.iter().copied().any(sets_it) { + return true; + } + if subtree(vec![Object::Ref(field_root(doc, &chain, r))], &mut seen) { return true; } - let kids = doc.get(r).as_dict().and_then(|d| d.get(b"Kids").map(|k| doc.resolve(k)).and_then(|k| k.as_array().cloned())).unwrap_or_default(); - stack.extend(kids.into_iter().map(|k| (k, depth + 1))); } - // Widgets on the pages, and the fields above them. - page_refs(doc).into_iter().any(|p| { + // Widgets on the pages, the fields above them, and the fields below the topmost one. + for p in page_refs(doc) { let annots = doc.get(p).as_dict().and_then(|d| d.get(b"Annots").map(|a| doc.resolve(a)).and_then(|a| a.as_array().cloned())).unwrap_or_default(); - annots.iter().filter_map(Object::as_ref).filter(|r| !seen.contains(r)).any(|r| field_chain(doc, r).into_iter().any(sets_it)) - }) + // Every widget's `/Parent` chain is read, even one already reached from `/Fields`: the + // field walk can adopt it under a different parent there. Each chain is at most + // MAX_FIELD_DEPTH long, and `subtree` returns at once for a root it has seen. + for r in annots.iter().filter_map(Object::as_ref) { + let chain = field_chain(doc, r); + if chain.len() >= MAX_FIELD_DEPTH || chain.iter().copied().any(sets_it) { + return true; + } + let root = field_root(doc, &chain, r); + if subtree(vec![Object::Ref(root)], &mut seen) { + return true; + } + } + } + false } /// Whether any text field with the Password flag holds a value in `doc`. @@ -806,6 +846,12 @@ pub fn restore_password_values(doc: &mut Document, values: &[(String, String)]) Ok(()) } +/// The topmost dictionary of `chain` (from [`field_chain`]): a `/Parent` that is not a dictionary +/// ends the field tree, so the walk below it starts at the last one that is. +fn field_root(doc: &Document, chain: &[ObjRef], r: ObjRef) -> ObjRef { + chain.iter().rev().copied().find(|c| doc.get(*c).as_dict().is_some()).unwrap_or(r) +} + /// `r` and its field ancestors (`/Parent`), nearest first, at most 64 deep and without repeats. fn field_chain(doc: &Document, r: ObjRef) -> Vec { let mut chain = vec![r]; diff --git a/crates/xfa/src/data/passwords.rs b/crates/xfa/src/data/passwords.rs index 0f98a3f2b4..896d513f2c 100644 --- a/crates/xfa/src/data/passwords.rs +++ b/crates/xfa/src/data/passwords.rs @@ -1,26 +1,38 @@ use super::import::datasets_packet; use super::merge::Op; -use super::path::{data_path, parse_datasets}; +use super::path::{data_path, parse_datasets_packet}; use super::values::{Want, path_of, text_of, with_template}; use super::write::{datasets_stream_ref, rewrite}; use super::{DataNode, DataPath, Document, MAX_DEPTH, Node, ObjRef, Object, SomIndices, Template, Ui, XfaError}; +/// How deep the AcroForm field tree is walked for password fields: as deep as the form's own +/// field walk (`pdfcraft_forms::fields`), which is what editing and dataset synchronization see. +const MAX_ACRO_DEPTH: usize = 64; + /// The SOM path and default value of every AcroForm text field with the Password flag (bit 14, /// `1 << 13`) in `doc`, for static XFA forms whose password fields set the flag in the AcroForm -/// dictionary. -fn acro_password_fields(doc: &Document) -> Vec<(String, String)> { +/// dictionary. `None` when the walk was cut short at [`MAX_ACRO_DEPTH`]: which branch is cut +/// depends on the order the tree is visited in, so a field the form's own walk reaches may have +/// been missed, and the caller can't tell that no password field is left. +fn acro_password_fields(doc: &Document) -> Option> { const PASSWORD_FLAG: i64 = 1 << 13; - let Some(root) = doc.root() else { return Vec::new() }; + let Some(root) = doc.root() else { return Some(Vec::new()) }; let catalog = doc.get(root); - let Some(acro) = catalog.as_dict().and_then(|c| c.get(b"AcroForm")).map(|a| doc.resolve(a)) else { return Vec::new() }; + let Some(acro) = catalog.as_dict().and_then(|c| c.get(b"AcroForm")).map(|a| doc.resolve(a)) else { return Some(Vec::new()) }; let Some(top) = acro.as_dict().and_then(|a| a.get(b"Fields")).map(|f| doc.resolve(f)).and_then(|f| f.as_array().cloned()) else { - return Vec::new(); + return Some(Vec::new()); }; struct AcroWalk<'a> { doc: &'a Document, - seen: std::collections::HashSet, + /// Each field reached, with what it was reached with: its parent's name, type, flags and + /// default, which decide what it classifies as. + seen: std::collections::HashMap, + /// Fields past the depth limit, with what they were reached with. One reached again from + /// another root in the same way was classified there; any other is not fully read. + cut: Vec<(ObjRef, Reach)>, out: Vec<(String, String)>, } + type Reach = (String, Option>, i64, Option); impl AcroWalk<'_> { fn walk( &mut self, @@ -31,12 +43,19 @@ fn acro_password_fields(doc: &Document) -> Vec<(String, String)> { inherited_dv: Option<&str>, depth: usize, ) { - if depth > 32 { + if depth > MAX_ACRO_DEPTH { + let reach: Reach = (parent_name.to_string(), inherited_ft.map(<[u8]>::to_vec), inherited_ff, inherited_dv.map(str::to_string)); + self.cut.extend(items.iter().filter_map(Object::as_ref).map(|r| (r, reach.clone()))); return; } for item in items { let Some(r) = item.as_ref() else { continue }; - if !self.seen.insert(r) { + let reach: Reach = (parent_name.to_string(), inherited_ft.map(<[u8]>::to_vec), inherited_ff, inherited_dv.map(str::to_string)); + // The reach a field was classified with stays: a later visit that skips it classified + // nothing, so it must not stand in for the cut-off one. + if let std::collections::hash_map::Entry::Vacant(slot) = self.seen.entry(r) { + slot.insert(reach); + } else { continue; } let obj = self.doc.get(r); @@ -65,20 +84,35 @@ fn acro_password_fields(doc: &Document) -> Vec<(String, String)> { } } } - let mut w = AcroWalk { doc, seen: std::collections::HashSet::new(), out: Vec::new() }; + let mut w = AcroWalk { doc, seen: std::collections::HashMap::new(), cut: Vec::new(), out: Vec::new() }; w.walk(&top, "", None, 0, None, 0); - w.out + // Decided once every root has been visited: a branch cut deep down may be reached shallower + // from a later root, and is then read in full. + let incomplete = w.cut.iter().any(|(r, reach)| w.seen.get(r) != Some(reach)); + (!incomplete).then_some(w.out) } /// The `Want` entries needed to restore password fields in `doc`'s datasets packet to their /// defaults (or empty when they have no default). -fn password_dataset_wants(doc: &Document) -> Vec { +/// +/// The packet is read as the datasets writer reads it ([`parse_datasets_packet`]): a packet the +/// writer can't read holds no value it wrote, so there is nothing typed in it to clear. An error +/// when the password fields can't all be found (a field tree deeper than the field walk reaches): +/// the save is refused rather than written with a typed password left in the data. +fn password_dataset_wants(doc: &Document) -> Result, XfaError> { if datasets_stream_ref(doc).is_none() { - return Vec::new(); + return Ok(Vec::new()); } - let Ok(Some(packet)) = datasets_packet(doc) else { return Vec::new() }; - let Some(data) = parse_datasets(&packet) else { return Vec::new() }; - with_template(doc, None, |tpl| { + // Checked before the packet is read: a packet that can't be parsed leaves nothing to clear, but + // a form whose fields can't all be found is refused whatever the packet holds. + let Some(acro_fields) = acro_password_fields(doc) else { + return Err(XfaError::Malformed( + "the form's fields nest too deeply to find every password field, so typed passwords could not be removed from its XFA data".into(), + )); + }; + let Ok(Some(packet)) = datasets_packet(doc) else { return Ok(Vec::new()) }; + let Some(data) = parse_datasets_packet(&packet) else { return Ok(Vec::new()) }; + Ok(with_template(doc, None, |tpl| { let mut candidates: Vec<(String, DataPath, String)> = Vec::new(); let mut seen = std::collections::HashSet::new(); if let Some(t) = tpl { @@ -153,7 +187,7 @@ fn password_dataset_wants(doc: &Document) -> Vec { } } } - for (som, default) in acro_password_fields(doc) { + for (som, default) in acro_fields { let path = path_of(tpl, &som); if !path.is_empty() && seen.insert(path.clone()) { candidates.push((som, path, default)); @@ -166,27 +200,29 @@ fn password_dataset_wants(doc: &Document) -> Vec { (!cur.is_empty() && cur != default).then_some(Want { som, path, text: default, ensure: false }) }) .collect() - }) + })) } /// Whether any XFA password field (`` or an AcroForm text field with the Password -/// flag) holds a value in `doc`'s datasets packet beyond its default (#3944). +/// flag) holds a value in `doc`'s datasets packet beyond its default (#3944). A form whose +/// password fields can't all be found counts as holding some, so that clearing them +/// ([`clear_password_values`]) reports why the document can't be written without them. pub fn has_password_values(doc: &Document) -> bool { - !password_dataset_wants(doc).is_empty() + !password_dataset_wants(doc).is_ok_and(|w| w.is_empty()) } /// Clear typed password values from `doc`'s XFA datasets packet (restoring a template default /// where the field has one, else emptying the element), replacing the datasets stream in place /// (#3944). Returns how many password fields were cleared in the datasets. pub fn clear_password_values(doc: &mut Document) -> Result { - let wants = password_dataset_wants(doc); + let wants = password_dataset_wants(doc)?; if wants.is_empty() { return Ok(0); } let count = wants.len(); let reuse = datasets_stream_ref(doc); let report = rewrite(doc, &[Op::Values(&wants)], reuse)?; - if !report.written || !password_dataset_wants(doc).is_empty() { + if !report.written || !password_dataset_wants(doc)?.is_empty() { let reason = report.warnings.first().map_or("the XFA datasets password values could not be cleared", String::as_str); return Err(XfaError::Malformed(reason.to_string())); } diff --git a/crates/xfa/src/data/path.rs b/crates/xfa/src/data/path.rs index 23c5e5ba5d..7fdba77071 100644 --- a/crates/xfa/src/data/path.rs +++ b/crates/xfa/src/data/path.rs @@ -24,6 +24,33 @@ pub fn parse_datasets(xdp: &str) -> Option { Some(to_node(data, 0)) } +/// The `xfa:data` element of a datasets packet as the datasets writer reads it before changing it: +/// a packet of an array may use the `xfa` and `xdp` prefixes its XDP envelope declares, so it is +/// read inside an element declaring them when it doesn't parse alone, and the datasets element in +/// the XFA data namespace is preferred over another with the same local name. Reading the packet +/// any other way could miss the values the writer put in it. +pub(super) fn parse_datasets_packet(text: &str) -> Option { + if crate::xml_nesting_depth(text) > crate::MAX_XML_DEPTH { + return None; + } + let opts = || roxmltree::ParsingOptions { allow_dtd: false, nodes_limit: MAX_NODES, ..Default::default() }; + let wrapped; + let doc = match roxmltree::Document::parse_with_options(text, opts()) { + Ok(d) => d, + Err(_) => { + wrapped = format!("{}{text}{}", super::merge::WRAP_OPEN, super::merge::WRAP_CLOSE); + roxmltree::Document::parse_with_options(&wrapped, opts()).ok()? + } + }; + let is = |n: &roxmltree::Node, name: &str| n.is_element() && n.tag_name().name() == name; + let datasets = doc + .descendants() + .find(|n| is(n, "datasets") && n.tag_name().namespace() == Some(super::XFA_DATA_NS)) + .or_else(|| doc.descendants().find(|n| is(n, "datasets")))?; + let data = datasets.children().find(|n| is(n, "data"))?; + Some(to_node(data, 0)) +} + /// The data path a SOM expression names: `form[0].#subform[0].name[1]` → `[(form, 0), (name, 1)]`. /// Unnamed containers (`#subform`) have no data node. pub fn som_to_path(som: &str) -> DataPath { diff --git a/crates/xfa/src/tests.rs b/crates/xfa/src/tests.rs index 27e6522f26..c1abcce190 100644 --- a/crates/xfa/src/tests.rs +++ b/crates/xfa/src/tests.rs @@ -7114,6 +7114,74 @@ fn a_container_is_as_wide_as_a_turned_draw_stands() { } } +/// The static fixture with its text field (`form1[0].page1[0].name[0]`) flagged Password and +/// `levels` unnamed fields between `form1[0]` and `page1[0]`, which add no SOM segment. +fn static_password_doc(levels: usize, data: &str) -> Document { + let mut doc = Document::open(Arc::new(static_shell(data))).unwrap(); + doc.update_dict(pdfcraft_cos::ObjRef::new(8, 0), |d| d.set(b"Ff".to_vec(), Object::Int(1 << 13))).unwrap(); + let mut next = pdfcraft_cos::ObjRef::new(7, 0); + for _ in 0..levels { + let mut d = Dict::new(); + d.set(b"Kids".to_vec(), Object::Array(vec![Object::Ref(next)])); + next = doc.add(Object::Dict(d)); + } + doc.update_dict(pdfcraft_cos::ObjRef::new(6, 0), |d| d.set(b"Kids".to_vec(), Object::Array(vec![Object::Ref(next)]))).unwrap(); + doc +} + +/// #3944: a Password-flagged AcroForm field of a static XFA form nested deeper than 32 levels (as +/// deep as the form's own field walk reaches) has its typed value cleared from the datasets. +#[test] +fn a_deeply_nested_acroform_password_field_is_cleared_from_the_datasets() { + let data = "hunter2"; + let mut doc = static_password_doc(40, data); + assert!(has_password_values(&doc)); + assert_eq!(clear_password_values(&mut doc).unwrap(), 1); + assert!(!has_password_values(&doc)); + assert!(!String::from_utf8_lossy(&xfa_bytes(&doc)).contains("hunter2")); +} + +/// A field tree deeper than the walk reaches can't be shown to hold no password field: clearing +/// refuses (so the save does) rather than leave a typed password in the datasets. +#[test] +fn a_field_tree_too_deep_to_check_for_passwords_refuses_to_clear() { + let mut doc = static_password_doc(80, "hunter2"); + assert!(has_password_values(&doc)); + assert!(clear_password_values(&mut doc).is_err()); + // A shallow form without a typed value has nothing to clear. + let mut doc = static_password_doc(0, ""); + assert!(!has_password_values(&doc)); + assert_eq!(clear_password_values(&mut doc).unwrap(), 0); +} + +/// A datasets packet of an XFA array may use the `xfa` prefix its preamble declares: the typed +/// password in it is still found and cleared, as the datasets writer reads that packet too. +#[test] +fn a_password_in_a_datasets_packet_using_the_preamble_prefix_is_cleared() { + let mut doc = static_password_doc(0, ""); + let pre = r#""#; + let tpl = r#""#; + let ds = "hunter2"; + let parts: Vec = [("preamble", pre), ("template", tpl), ("datasets", ds), ("postamble", "")] + .into_iter() + .flat_map(|(name, text)| { + let r = doc.add(Object::Stream(pdfcraft_cos::Stream::flate(Dict::new(), text.as_bytes()))); + [Object::String(pdfcraft_cos::PdfString::literal(name.as_bytes().to_vec())), Object::Ref(r)] + }) + .collect(); + let root = doc.root().unwrap(); + doc.update_dict(root, |c| { + let mut acro = c.get(b"AcroForm").and_then(Object::as_dict).cloned().unwrap(); + acro.set(b"XFA".to_vec(), Object::Array(parts)); + c.set(b"AcroForm".to_vec(), Object::Dict(acro)); + }) + .unwrap(); + assert!(crate::data::parse_datasets(ds).is_none(), "the packet doesn't parse alone"); + assert!(has_password_values(&doc)); + assert_eq!(clear_password_values(&mut doc).unwrap(), 1); + assert!(!has_password_values(&doc)); +} + /// Every sibling a SOM index skips owns a copy of the element name: the in-memory builder refuses /// a path whose copies would pass the name-byte bound the writer holds itself to, and creates /// nothing for it. From 80e26e674d1aa33078e83c8139875047f3289eb6 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 18:16:48 -0500 Subject: [PATCH 21/22] fix(redact): remove a stroked path by the box its stroke paints (#5876) * fix(redact): remove a stroked path by the box its stroke paints Path redaction tested a path's construction box against the marks. A horizontal or vertical stroke has a zero-area box, which overlaps nothing, so its drawing commands stayed beneath the overlay; a wide stroke whose points lie beside a mark but whose ink reaches into it stayed too. The interpreter now tracks the line width, miter limit and join style (`w`, `M`, `j`, and `LW`, `ML`, `LJ` through `gs`) and, for a stroking operator, grows the box by half the width (at least one unit), times the miter limit when the path has mitred joins, scaled by the CTM's largest stretch, before the hit test. The bound is conservative: a stroke near a mark may now go where before it stayed. * address review: pad hairlines by half a unit, rectangles for their corners, square caps by sqrt 2 * review: test a kept clip against the path as constructed * address review: a line state set between a path and its stroke no longer lets the stroke skip the redaction test * address review: line-state operators read the operand before the operator --- crates/redact/src/interp.rs | 69 +++++++++++++++++++++++++++++++++++-- crates/redact/src/tests.rs | 69 +++++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 3 deletions(-) diff --git a/crates/redact/src/interp.rs b/crates/redact/src/interp.rs index 80117f451d..12636dc88f 100644 --- a/crates/redact/src/interp.rs +++ b/crates/redact/src/interp.rs @@ -93,6 +93,12 @@ struct Gs { scale: f64, leading: f64, rise: f64, + /// The stroke parameters that widen a painted path beyond its construction points. + line_width: f64, + miter_limit: f64, + line_join: i64, + /// `J` / ExtGState `/LC`: a projecting square cap (2) reaches past the endpoint's corner. + line_cap: i64, /// The tiling or shading pattern selected as the fill colour (`/Name scn`), and as the stroke /// colour (`/Name SCN`). Part of the graphics state, so `q`/`Q` save and restore them with /// everything else. A pattern's cell is a content stream of its own and this pass does not @@ -394,6 +400,10 @@ pub(crate) fn process(doc: &mut Document, scope: &mut Scope<'_>, streams: &[Vec< scale: 1.0, leading: 0.0, rise: 0.0, + line_width: 1.0, + miter_limit: 10.0, + line_join: 0, + line_cap: 0, fill_pattern: None, stroke_pattern: None, type3_inherits: false, @@ -438,6 +448,7 @@ fn process_from(doc: &mut Document, scope: &mut Scope<'_>, streams: &[Vec], } if !walk.path.is_empty() { done.ops.append(&mut walk.path); + done.ops.append(&mut walk.pending); } processed.push((done.ops, done.changed)); } @@ -505,6 +516,10 @@ struct Walk { path: Vec, path_box: Option<[f64; 4]>, clip: bool, + /// Line-state operators (`w`, `M`, `j`, `J`, `d`, `gs`) met while a path is open: they apply to + /// the stroke that ends it, so the path stays whole for the hit test, and they are written + /// after it, kept even when the path goes. + pending: Vec, /// Painting operators written again under a clip that cuts the regions out: unchanged as /// operators, but what they paint is no longer all there. clipped: HashSet, @@ -526,6 +541,7 @@ impl Walk { path: Vec::new(), path_box: None, clip: false, + pending: Vec::new(), clipped: HashSet::new(), out: Output::default(), } @@ -542,14 +558,20 @@ impl Walk { self.paint_path(scope, op, piece); return; } + if !self.path.is_empty() && matches!(o, b"w" | b"M" | b"j" | b"J" | b"d" | b"gs") { + self.graphics_state(doc, scope, &op); + self.pending.push(op); + return; + } if !self.path.is_empty() { // A path not ended by a painting operator: pass it through. piece.ops.append(&mut self.path); + piece.ops.append(&mut self.pending); self.path_box = None; self.clip = false; } let kept = match o { - b"q" | b"Q" | b"cm" | b"Tf" | b"gs" => { + b"q" | b"Q" | b"cm" | b"Tf" | b"gs" | b"w" | b"M" | b"j" | b"J" => { self.graphics_state(doc, scope, &op); Some(op) } @@ -627,11 +649,35 @@ impl Walk { self.path.push(op); } + /// The box a stroke of the current path can paint, grown from its construction box `b` by + /// what the pen adds: half the line width (a width of 0 is one device pixel, taken as one + /// unit) scaled by the CTM's largest stretch, and never less than half a unit of page space + /// (a renderer paints a hairline, or a line a CTM shrinks, about a device pixel wide); times + /// the miter limit when the path has joins and they are mitred (a rectangle has four), or + /// √2 for a projecting square cap (its outer corner). Caps and joins never reach further, so + /// a hit test on it is conservative, and a horizontal or vertical line no longer has a + /// zero-area box. + fn stroke_box(&self, b: [f64; 4]) -> [f64; 4] { + let m = self.gs.ctm.0; + let scale = (m[0] * m[0] + m[1] * m[1] + m[2] * m[2] + m[3] * m[3]).sqrt(); + let joins = self.path.iter().filter(|op| matches!(op.op.as_slice(), b"l" | b"c" | b"v" | b"y" | b"h" | b"re")).count() >= 2 + || self.path.iter().any(|op| op.op.as_slice() == b"re"); + let miter = if joins && self.gs.line_join == 0 { self.gs.miter_limit.max(1.0) } else { 1.0 }; + let reach = if self.gs.line_cap == 2 { miter.max(std::f64::consts::SQRT_2) } else { miter }; + let pad = (self.gs.line_width.max(1.0) / 2.0 * scale).max(0.5) * reach; + [b[0] - pad, b[1] - pad, b[2] + pad, b[3] + pad] + } + /// A painting operator ends the path; one under a region is removed (or kept as a clip). fn paint_path(&mut self, scope: &mut Scope<'_>, op: Op, piece: &mut Piece) { + let strokes = matches!(op.op.as_slice(), b"S" | b"s" | b"B" | b"B*" | b"b" | b"b*"); + // A `W`/`W*` makes the path as constructed the clip; the stroke expansion only describes + // what the painting itself covers. + let path_box = self.path_box.take(); + let bbox = path_box.map(|b| if strokes { self.stroke_box(b) } else { b }); let body = std::mem::take(&mut self.path); - let bbox = self.path_box.take(); let clips = std::mem::replace(&mut self.clip, false); + let pending = std::mem::take(&mut self.pending); let hit = scope.mode == Mode::Apply && op.op.as_slice() != b"n" && bbox.is_some_and(|b| scope.hits(b)); if !hit { // The paint survives, so the pattern it draws with is still in use. `n` paints @@ -641,25 +687,30 @@ impl Walk { let fills = matches!(o, b"f" | b"F" | b"f*" | b"B" | b"B*" | b"b" | b"b*"); uses_patterns(&mut self.out, &self.gs, fills, strokes); piece.ops.extend(body); + piece.ops.extend(pending); piece.ops.push(op); return; } let b = bbox.unwrap_or_default(); piece.changed = true; scope.report.paths_removed += 1; - if scope.covered(b) && clips { + if scope.covered(path_box.unwrap_or(b)) && clips { // Everything this clip lets through is under a mark, so an empty clip hides the same // content. The original outline is confidential and must not stay in the stream. + piece.ops.extend(pending); for o in ["re", "W", "n"] { let operands = if o == "re" { vec![num(0.0); 4] } else { vec![] }; piece.ops.push(Op::new(o, operands)); } + } else { + piece.ops.extend(pending); } // Partial vector redaction must not retain underlying drawing commands // behind clipping (F-008). Refuse partial clip retention and remove the path. } fn graphics_state(&mut self, doc: &Document, scope: &mut Scope<'_>, op: &Op) { + let last = op.nums::<1>().map(|[v]| v); match op.op.as_slice() { b"q" => self.stack.push((self.gs.clone(), self.tm, self.tlm)), b"Q" => { @@ -673,12 +724,24 @@ impl Walk { } } b"Tf" => self.select_font(doc, scope, op), + // The operand before the operator, as the renderer reads it: `1 12 w` is a width of 12. + b"w" => self.gs.line_width = last.unwrap_or(self.gs.line_width), + b"M" => self.gs.miter_limit = last.unwrap_or(self.gs.miter_limit), + b"j" => self.gs.line_join = last.map_or(self.gs.line_join, |j| j as i64), + b"J" => self.gs.line_cap = last.map_or(self.gs.line_cap, |j| j as i64), _ => { if let Some((font, vertical, size)) = op.last_name().and_then(|n| extgstate_font(doc, scope, &self.ext_gstates, n, &self.fallback)) { (self.gs.font, self.gs.vertical, self.gs.size) = (font, vertical, size); // As for `Tf`: a Type 3 font without /Resources paints through this scope's. self.gs.type3_inherits = op.last_name().is_some_and(|n| extgstate_type3_inherits(doc, &self.ext_gstates, n)); } + if let Some(state) = op.name(0).and_then(|n| self.ext_gstates.get(n)).and_then(|s| doc.resolve(s).as_dict().cloned()) { + let num = |key: &[u8]| state.get(key).and_then(|v| doc.resolve(v).as_f64()); + self.gs.line_width = num(b"LW").unwrap_or(self.gs.line_width); + self.gs.miter_limit = num(b"ML").unwrap_or(self.gs.miter_limit); + self.gs.line_join = num(b"LJ").map_or(self.gs.line_join, |j| j as i64); + self.gs.line_cap = num(b"LC").map_or(self.gs.line_cap, |j| j as i64); + } } } } diff --git a/crates/redact/src/tests.rs b/crates/redact/src/tests.rs index 08125165b5..43b6caaadc 100644 --- a/crates/redact/src/tests.rs +++ b/crates/redact/src/tests.rs @@ -837,6 +837,22 @@ fn vectors_are_removed_or_clipped_and_inline_images_go() { assert!(c.contains("150 150 m"), "the line elsewhere stays"); } +/// A horizontal stroke's construction box has no area, so it overlapped nothing and stayed under +/// the mark. The painted stroke is as wide as the line: a 12 pt line centred 5 pt above the mark +/// reaches into it and goes; a 1 pt line there does not and stays. +#[test] +fn a_stroke_is_removed_by_its_painted_width() { + let src = b"12 w 10 60 m 100 60 l S 1 w 10 60 m 100 60 l S 4 w 10 50 m 100 50 l S"; + let mut doc = one_page(src, "", vec![]); + mark(&mut doc, 0, &[[40.0, 45.0, 60.0, 55.0]], ""); + let r = apply(&mut doc, None).unwrap(); + assert_eq!(r.paths_removed, 2, "{r:?}"); + let c = content(&doc, 0); + assert!(!c.contains("12 w\n10 60 m"), "the wide line reaches the mark: {c}"); + assert!(c.contains("1 w\n10 60 m\n100 60 l\nS"), "the thin line beside it stays: {c}"); + assert!(!c.contains("10 50 m"), "the line through the mark goes: {c}"); +} + /// A tiling pattern's cell is a content stream of its own and can draw text. Redacting a shape /// filled with such a pattern removed the fill from the page but left the pattern in the page's /// resources, so its cell -- with the text that was under the mark -- stayed in the saved file @@ -6674,6 +6690,59 @@ fn a_reply_on_another_page_to_a_removed_popup_goes_too() { } } +/// A hairline under a shrinking CTM still paints about a device pixel: its pad must not shrink +/// with the CTM, or a mark on that ink misses it. +#[test] +fn a_hairline_under_a_shrinking_ctm_is_padded_by_half_a_unit() { + // Device line from (10, 10) to (50, 10); the mark lies on its painted pixel, above its axis. + let mut doc = one_page(b"0.01 0 0 0.01 0 0 cm 0 w 1000 1000 m 5000 1000 l S", "", vec![]); + mark(&mut doc, 0, &[[20.0, 10.2, 30.0, 10.4]], ""); + assert_eq!(apply(&mut doc, None).unwrap().paths_removed, 1); +} + +/// One `re` has four mitred corners: it is padded by the miter limit like any joined path. +#[test] +fn a_rectangle_stroke_is_padded_for_its_mitred_corners() { + let mut doc = one_page(b"2 w 100 100 50 50 re S", "", vec![]); + // Past half the line width from the rectangle, inside the miter reach. + mark(&mut doc, 0, &[[155.0, 120.0, 160.0, 130.0]], ""); + assert_eq!(apply(&mut doc, None).unwrap().paths_removed, 1); +} + +/// A projecting square cap's outer corner reaches lw/2 x sqrt(2) from the endpoint. Under a shear +/// it maps past a pad of lw/2 times the CTM's stretch (the review's example: corner at x = 12). +#[test] +fn a_square_cap_corner_under_a_shear_is_inside_the_pad() { + let mut doc = one_page(b"1 0 1 1 0 0 cm 2 J 2 w 0 0 m 10 0 l S", "", vec![]); + mark(&mut doc, 0, &[[11.8, 0.8, 11.9, 0.95]], ""); + assert_eq!(apply(&mut doc, None).unwrap().paths_removed, 1); +} + +/// A line width set between the path and its `S` still applies to that stroke, so the stroke is +/// tested at the width it is painted with; the width itself stays, the path goes. +#[test] +fn a_width_set_between_the_path_and_its_stroke_does_not_hide_it_from_the_test() { + let mut doc = one_page(b"10 60 m 100 60 l 12 w S 5 5 m 9 9 l S", "", vec![]); + mark(&mut doc, 0, &[[40.0, 45.0, 60.0, 55.0]], ""); + let r = apply(&mut doc, None).unwrap(); + assert_eq!(r.paths_removed, 1, "{r:?}"); + let c = content(&doc, 0); + assert!(!c.contains("10 60 m") && !c.contains("100 60 l"), "the wide line reaches the mark: {c}"); + assert!(c.contains("12 w"), "the width stays: {c}"); + assert!(c.contains("5 5 m"), "the line elsewhere stays: {c}"); +} + +/// The renderer takes a line-state operator's operand from the end: `1 12 w` strokes 12 points wide, +/// so the stroke is tested at that width. +#[test] +fn a_surplus_operand_before_the_line_width_does_not_hide_the_stroke() { + let mut doc = one_page(b"1 12 w 10 60 m 100 60 l S", "", vec![]); + mark(&mut doc, 0, &[[40.0, 45.0, 60.0, 55.0]], ""); + let r = apply(&mut doc, None).unwrap(); + assert_eq!(r.paths_removed, 1, "{r:?}"); + assert!(!content(&doc, 0).contains("100 60 l")); +} + /// One page drawing `SECRET` in the `/OC /L` block, with layer 7 0 R in the default /// configuration `config` (objects 8 and up are `extra`). fn layer_state_page(config: Vec<(&str, Object)>, resource: &str, extra: Vec>) -> Document { From 1d2af9907ba772b368f6b54811359805fa5428e9 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 18:38:09 -0500 Subject: [PATCH 22/22] Fix Flatpak updates and bump to 0.21.1 (#7020) * fix(flatpak): allow release checks and self-update helpers * test: keep Flatpak permission parsing within strict Clippy rules * chore: bump version to 0.21.1 for the Flatpak update fix --- Cargo.lock | 78 +++++++++---------- Cargo.toml | 2 +- crates/ui-egui/tests/flatpak_updates.rs | 27 +++++++ .../ai.storyteller.pdfcraft.bundle.yml | 2 + .../linux/flatpak/ai.storyteller.pdfcraft.yml | 2 + 5 files changed, 71 insertions(+), 40 deletions(-) create mode 100644 crates/ui-egui/tests/flatpak_updates.rs diff --git a/Cargo.lock b/Cargo.lock index cecedc12c9..0e4c910427 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4264,7 +4264,7 @@ dependencies = [ [[package]] name = "pdfcraft" -version = "0.21.0" +version = "0.21.1" dependencies = [ "chrono", "craft-update", @@ -4289,7 +4289,7 @@ dependencies = [ [[package]] name = "pdfcraft-a11y" -version = "0.21.0" +version = "0.21.1" dependencies = [ "pdfcraft-content", "pdfcraft-cos", @@ -4300,7 +4300,7 @@ dependencies = [ [[package]] name = "pdfcraft-annot" -version = "0.21.0" +version = "0.21.1" dependencies = [ "hayro-syntax", "pdfcraft-cos", @@ -4312,7 +4312,7 @@ dependencies = [ [[package]] name = "pdfcraft-audit" -version = "0.21.0" +version = "0.21.1" dependencies = [ "pdfcraft-content", "pdfcraft-cos", @@ -4323,7 +4323,7 @@ dependencies = [ [[package]] name = "pdfcraft-automation" -version = "0.21.0" +version = "0.21.1" dependencies = [ "base64 0.23.1", "flate2", @@ -4347,7 +4347,7 @@ dependencies = [ [[package]] name = "pdfcraft-cli" -version = "0.21.0" +version = "0.21.1" dependencies = [ "base64 0.23.1", "pdfcraft-audit", @@ -4365,11 +4365,11 @@ dependencies = [ [[package]] name = "pdfcraft-compare" -version = "0.21.0" +version = "0.21.1" [[package]] name = "pdfcraft-content" -version = "0.21.0" +version = "0.21.1" dependencies = [ "hayro-syntax", "pdfcraft-cos", @@ -4377,7 +4377,7 @@ dependencies = [ [[package]] name = "pdfcraft-cos" -version = "0.21.0" +version = "0.21.1" dependencies = [ "chrono", "hayro-syntax", @@ -4394,7 +4394,7 @@ dependencies = [ [[package]] name = "pdfcraft-create" -version = "0.21.0" +version = "0.21.1" dependencies = [ "encoding_rs", "hayro-ccitt", @@ -4414,7 +4414,7 @@ dependencies = [ [[package]] name = "pdfcraft-crypt" -version = "0.21.0" +version = "0.21.1" dependencies = [ "aes", "md-5", @@ -4427,7 +4427,7 @@ dependencies = [ [[package]] name = "pdfcraft-edit" -version = "0.21.0" +version = "0.21.1" dependencies = [ "hayro-syntax", "icu_segmenter", @@ -4446,7 +4446,7 @@ dependencies = [ [[package]] name = "pdfcraft-engine" -version = "0.21.0" +version = "0.21.1" dependencies = [ "chrono", "craft-docpath", @@ -4490,14 +4490,14 @@ dependencies = [ [[package]] name = "pdfcraft-export" -version = "0.21.0" +version = "0.21.1" dependencies = [ "flate2", ] [[package]] name = "pdfcraft-filters" -version = "0.21.0" +version = "0.21.1" dependencies = [ "fax", "flate2", @@ -4510,7 +4510,7 @@ dependencies = [ [[package]] name = "pdfcraft-fonts" -version = "0.21.0" +version = "0.21.1" dependencies = [ "harfrust", "pdfcraft-cos", @@ -4520,7 +4520,7 @@ dependencies = [ [[package]] name = "pdfcraft-forms" -version = "0.21.0" +version = "0.21.1" dependencies = [ "hayro-syntax", "pdfcraft-content", @@ -4533,11 +4533,11 @@ dependencies = [ [[package]] name = "pdfcraft-geom" -version = "0.21.0" +version = "0.21.1" [[package]] name = "pdfcraft-js" -version = "0.21.0" +version = "0.21.1" dependencies = [ "boa_engine", "chrono", @@ -4545,7 +4545,7 @@ dependencies = [ [[package]] name = "pdfcraft-measure" -version = "0.21.0" +version = "0.21.1" dependencies = [ "pdfcraft-annot", "pdfcraft-content", @@ -4558,14 +4558,14 @@ dependencies = [ [[package]] name = "pdfcraft-model" -version = "0.21.0" +version = "0.21.1" dependencies = [ "pdfcraft-cos", ] [[package]] name = "pdfcraft-ocr" -version = "0.21.0" +version = "0.21.1" dependencies = [ "ocrs", "pdfcraft-fonts", @@ -4580,7 +4580,7 @@ dependencies = [ [[package]] name = "pdfcraft-ocr-worker" -version = "0.21.0" +version = "0.21.1" dependencies = [ "js-sys", "pdfcraft-ocr", @@ -4590,7 +4590,7 @@ dependencies = [ [[package]] name = "pdfcraft-optimize" -version = "0.21.0" +version = "0.21.1" dependencies = [ "image", "pdfcraft-annot", @@ -4607,11 +4607,11 @@ dependencies = [ [[package]] name = "pdfcraft-oracle" -version = "0.21.0" +version = "0.21.1" [[package]] name = "pdfcraft-organize" -version = "0.21.0" +version = "0.21.1" dependencies = [ "hayro-syntax", "pdfcraft-content", @@ -4622,7 +4622,7 @@ dependencies = [ [[package]] name = "pdfcraft-platform" -version = "0.21.0" +version = "0.21.1" dependencies = [ "async-channel", "futures-lite", @@ -4633,7 +4633,7 @@ dependencies = [ [[package]] name = "pdfcraft-prc" -version = "0.21.0" +version = "0.21.1" dependencies = [ "flate2", "proptest", @@ -4642,7 +4642,7 @@ dependencies = [ [[package]] name = "pdfcraft-prc-render" -version = "0.21.0" +version = "0.21.1" dependencies = [ "flate2", "pdfcraft-prc", @@ -4651,7 +4651,7 @@ dependencies = [ [[package]] name = "pdfcraft-preflight" -version = "0.21.0" +version = "0.21.1" dependencies = [ "flate2", "pdfcraft-content", @@ -4662,7 +4662,7 @@ dependencies = [ [[package]] name = "pdfcraft-print" -version = "0.21.0" +version = "0.21.1" dependencies = [ "craft-print", "log", @@ -4677,7 +4677,7 @@ dependencies = [ [[package]] name = "pdfcraft-redact" -version = "0.21.0" +version = "0.21.1" dependencies = [ "aho-corasick", "pdfcraft-annot", @@ -4701,7 +4701,7 @@ dependencies = [ [[package]] name = "pdfcraft-render" -version = "0.21.0" +version = "0.21.1" dependencies = [ "bytemuck", "flate2", @@ -4725,7 +4725,7 @@ dependencies = [ [[package]] name = "pdfcraft-sign" -version = "0.21.0" +version = "0.21.1" dependencies = [ "aes", "aws-lc-rs", @@ -4764,11 +4764,11 @@ dependencies = [ [[package]] name = "pdfcraft-testkit" -version = "0.21.0" +version = "0.21.1" [[package]] name = "pdfcraft-ui-egui" -version = "0.21.0" +version = "0.21.1" dependencies = [ "arboard", "base64 0.23.1", @@ -4827,7 +4827,7 @@ dependencies = [ [[package]] name = "pdfcraft-web" -version = "0.21.0" +version = "0.21.1" dependencies = [ "craft-web-api", "craft-web-host", @@ -4842,7 +4842,7 @@ dependencies = [ [[package]] name = "pdfcraft-xfa" -version = "0.21.0" +version = "0.21.1" dependencies = [ "base64 0.23.1", "image", @@ -4856,7 +4856,7 @@ dependencies = [ [[package]] name = "pdfcraft-xfdf" -version = "0.21.0" +version = "0.21.1" dependencies = [ "base64 0.23.1", "pdfcraft-annot", diff --git a/Cargo.toml b/Cargo.toml index 9cd5b3b7c3..752f580475 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,7 +5,7 @@ exclude = ["vendor"] default-members = ["crates/*", "apps/pdfcraft", "apps/pdfcraft-cli", "xtask"] [workspace.package] -version = "0.21.0" +version = "0.21.1" edition = "2024" license = "MIT OR Apache-2.0" rust-version = "1.95" diff --git a/crates/ui-egui/tests/flatpak_updates.rs b/crates/ui-egui/tests/flatpak_updates.rs new file mode 100644 index 0000000000..536613f08a --- /dev/null +++ b/crates/ui-egui/tests/flatpak_updates.rs @@ -0,0 +1,27 @@ +//! Release and source Flatpaks must allow release checks and host update helpers. +fn finish_args(manifest: &str) -> Vec<&str> { + manifest + .split("finish-args:") + .nth(1) + .unwrap_or_default() + .lines() + .take_while(|line| line.is_empty() || line.starts_with(' ') || line.starts_with('#')) + .filter_map(|line| line.trim().strip_prefix("- ")) + .map(|arg| arg.trim_matches(['\'', '"'])) + .collect() +} + +#[test] +fn flatpak_update_permissions() { + let manifests = [ + include_str!("../../../packaging/linux/flatpak/ai.storyteller.pdfcraft.bundle.yml"), + include_str!("../../../packaging/linux/flatpak/ai.storyteller.pdfcraft.yml"), + ]; + let args = manifests.map(finish_args); + assert_eq!(args[0], args[1], "source and release permissions must agree"); + for permissions in args { + for required in ["--share=network", "--talk-name=org.freedesktop.Flatpak"] { + assert!(permissions.contains(&required), "missing update permission: {required}"); + } + } +} diff --git a/packaging/linux/flatpak/ai.storyteller.pdfcraft.bundle.yml b/packaging/linux/flatpak/ai.storyteller.pdfcraft.bundle.yml index d781ad6c7c..8d2fb49192 100644 --- a/packaging/linux/flatpak/ai.storyteller.pdfcraft.bundle.yml +++ b/packaging/linux/flatpak/ai.storyteller.pdfcraft.bundle.yml @@ -17,6 +17,8 @@ runtime-version: '25.08' sdk: org.freedesktop.Sdk command: pdfcraft finish-args: + # The shared updater inspects and updates this installation via flatpak-spawn --host. + - --talk-name=org.freedesktop.Flatpak - --filesystem=xdg-data/ArtCraft/versions:ro # Display: Wayland first; X11 only when no Wayland compositor is running. - --socket=wayland diff --git a/packaging/linux/flatpak/ai.storyteller.pdfcraft.yml b/packaging/linux/flatpak/ai.storyteller.pdfcraft.yml index fba62fe382..2c5d6d88c6 100644 --- a/packaging/linux/flatpak/ai.storyteller.pdfcraft.yml +++ b/packaging/linux/flatpak/ai.storyteller.pdfcraft.yml @@ -18,6 +18,8 @@ sdk-extensions: - org.freedesktop.Sdk.Extension.rust-stable command: pdfcraft finish-args: + # The shared updater inspects and updates this installation via flatpak-spawn --host. + - --talk-name=org.freedesktop.Flatpak - --filesystem=xdg-data/ArtCraft/versions:ro # Display: Wayland first; X11 only when no Wayland compositor is running. - --socket=wayland