From 3be126f9bbe9689b81387f555ab6f884863c8cb5 Mon Sep 17 00:00:00 2001 From: Michael Higgins <55243596+Higgs32584@users.noreply.github.com> Date: Wed, 11 Nov 2026 01:32:14 -0500 Subject: [PATCH] fix(engine, automation): Action Wizard and doc_batch honour JavaScript being off (#5080) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What this changes Batch processing now keeps the caller's "Enable JavaScript" setting. Before, each file was processed in a fresh `Session::new()`, where JavaScript is on, so open-time XFA scripts and "Run a JavaScript" steps ran and their results were saved. - `actions::run_on` and `ocr::recognize_file` take a `javascript: bool` and call `set_javascript` before opening the file. In `recognize_file` the open now goes through a small private helper, `ocr::open_file`, so the open path can be tested without the recognition models. - `doc_batch`'s `Recipe` carries the setting into its per-file session and its OCR step. - Callers pass their own `session.javascript()`: the desktop Action Wizard, desktop multi-file OCR, `action_run`, `ocr_recognize_files` and `doc_batch`. Also relevant to #5054: an administrator's `DisableJavaScript` policy now reaches these sessions. Fixes #5080 ## How you verified it Regression tests, adopted from the reviewers' patches, plus one for the OCR open path: - `crates/engine/src/tests.rs::an_action_runs_a_javascript_step_only_when_javascript_is_on` (croquete1): a "Run a JavaScript" step writes its field with JavaScript on, and `run_on(.., false, ..)` refuses it instead of saving a scripted value. croquete1 reported it failing with the `set_javascript` call in `run_on` removed. - `crates/automation/tests/it/automation.rs::file_processing_keeps_javascript_off_for_xfa_open_scripts` (Harrison-Booth): with `js_enabled` off, an XFA initialize script that sets `qty` to 42 doesn't reach the output of `action_run`, `doc_batch` or (only when its models are installed) `ocr_recognize_files`. The contributor reported that each propagation removed on its own makes it fail. - `crates/engine/src/ocr.rs::tests::files_open_for_recognition_with_the_callers_javascript_preference`: opens the scripted XFA fixture through `recognize_file`'s open path with JavaScript off (the initialize script leaves `qty` empty) and on (it sets `qty` to 2). This covers the OCR propagation in runs without the OCR models. What was run locally, and what wasn't: - `cargo fmt --check` and `cargo clippy --all-targets -- -D warnings` were clean on `pdfcraft-engine`, `pdfcraft-automation` and `pdfcraft-ui-egui` at the earlier heads. On the latest commit only `cargo fmt --all` was run; clippy was not re-run. - No tests were run locally, including the new `ocr::tests` one: test builds time out on this machine, and the run was stopped before it compiled. The new test has not been seen failing either. If `set_javascript` is removed, the session keeps its JavaScript-on default, so the off case would read `qty` = 2 like the on case, and the first assertion would fail. Seeing that happen is still owed. - The tests are left to the maintainers' CI. No CI result is claimed for this PR. - [x] `cargo fmt --check` - [ ] clippy with `-D warnings` on the crates I changed (clean at the earlier heads; not re-run on the latest commit) - [ ] tests pass on the crates I changed (not run locally) - [x] `cargo xtask layers` and `cargo xtask assets`: N/A. No crate boundaries, dependencies or assets change. ## Never-crash - [x] No new `unwrap`/`expect`/`panic!`/`unreachable!`/`todo!` outside tests. The new non-test code only passes a `bool` and propagates errors with `?`. - [x] No indexing or slicing with input-derived positions, no new arithmetic, allocations or recursion. - [x] No `unsafe` - [x] Any crash this fixes has a regression test: N/A, this fixes no crash. ## Provenance - [x] No asset from an Adobe product, and no Adobe icon recreated, traced or imitated (no assets change) - [x] No font files committed here - [x] No GPL/AGPL code copied in - [x] Behaviour comes from the project's own JavaScript preference (`Session::set_javascript`) and issue #5080, not from another implementation's source - [x] Every new asset has an `ATTRIBUTION.toml` entry: N/A, no new assets ## Security - [x] `--root` confinement still holds: `action_run` and `ocr_recognize_files` still resolve every input and output through `resolve`, and `doc_batch`'s path handling is unchanged. No new paths are read or written. - [x] MCP and the UI control channel are untouched. - [x] No new outbound network access. - This narrows script execution: with JavaScript off, the per-file sessions of the Action Wizard, multi-file OCR, `action_run`, `ocr_recognize_files` and `doc_batch` no longer run document or step scripts. ## Parity and reach - [ ] `parity/acrobat-features.toml` not updated: this is a bug fix, and the `misc.batch-processing` entry's status (`partial`) and notes stay true. - [x] Reachable headlessly: `action_run`, `ocr_recognize_files` and `doc_batch` take the setting from the session, which `js_enabled` sets. - [x] Docs: N/A. No documented behaviour changes (`docs/cli-batch.md` doesn't describe JavaScript handling). ## Anything a reviewer should look at first - The existing `run_on` tests pass `true` to keep their behaviour; the tests above cover the JavaScript-off path. - `recognize_file` now has 7 parameters, the `too_many_arguments` limit. - The OCR branch of the end-to-end test runs only where the OCR models are installed. The engine unit test covers the OCR open path without them. Provisioning the models in CI is left as a separate decision. ## Summary by CodeRabbit * **Bug Fixes** * File actions, batch processing, and OCR now honor the JavaScript setting from your current session for each file. * When JavaScript is disabled, document scripts—including scripts that run when a document opens—won’t execute during processing. When enabled, JavaScript steps can run as expected. * This behavior applies across automated and in-app workflows. Per-file results and OCR output handling remain unchanged. --- crates/automation/src/a11y/documents.rs | 7 +- crates/automation/src/a11y/ocr.rs | 12 +++- crates/automation/src/batch.rs | 13 +++- crates/automation/tests/it/action_failures.rs | 6 +- crates/automation/tests/it/automation.rs | 67 +++++++++++++++++++ crates/engine/src/actions.rs | 16 ++++- crates/engine/src/ocr.rs | 65 ++++++++++++------ crates/engine/src/tests.rs | 36 ++++++++-- crates/ui-egui/src/workflows/actions_ui.rs | 4 +- crates/ui-egui/src/workflows/ocr_ui.rs | 24 ++++--- crates/ui-egui/src/workflows/ocr_web.rs | 7 +- 11 files changed, 212 insertions(+), 45 deletions(-) diff --git a/crates/automation/src/a11y/documents.rs b/crates/automation/src/a11y/documents.rs index 137c98c5d4..bfc8b3379a 100644 --- a/crates/automation/src/a11y/documents.rs +++ b/crates/automation/src/a11y/documents.rs @@ -118,6 +118,9 @@ impl Automation { Ok(json!({ "actions": actions, "steps": steps })) } + /// `action_run`: run a named action or a list of steps on each file in `paths`, writing the + /// results into `folder`; each file is processed in its own session that keeps this + /// session's JavaScript preference. pub(crate) fn action_run(&mut self, a: &Args) -> Result { use pdfcraft_engine::actions::{Action, Step, builtin, run_on}; let action = if let Some(name) = a.opt_str("action")? { @@ -138,6 +141,8 @@ impl Automation { }; let folder = self.resolve(a.str("folder")?, true)?; std::fs::create_dir_all(&folder).map_err(|e| failed(e.to_string()))?; + // Each file runs in its own session; it keeps this one's JavaScript preference. + let javascript = self.session.javascript(); let mut out = Vec::new(); for p in a.strs("paths")? { let name = std::path::Path::new(p).file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_else(|| "document.pdf".into()); @@ -145,7 +150,7 @@ impl Automation { .resolve(p, false) .map_err(|e| e.to_string()) .and_then(|src| crate::read_input_bytes(&src).map_err(|e| e.to_string())) - .and_then(|bytes| run_on(&action, &name, std::sync::Arc::new(bytes), |_, _| {})); + .and_then(|bytes| run_on(&action, &name, std::sync::Arc::new(bytes), javascript, |_, _| {})); out.push(match result { Ok(r) => { let target = crate::batch::publish(&child(&folder, &name), &r.bytes)?; diff --git a/crates/automation/src/a11y/ocr.rs b/crates/automation/src/a11y/ocr.rs index 6d1eaaa389..9f21fc285e 100644 --- a/crates/automation/src/a11y/ocr.rs +++ b/crates/automation/src/a11y/ocr.rs @@ -164,6 +164,8 @@ impl Automation { } // The engines are built (and named if they cannot be) before the first file is read. let recognizers = pdfcraft_engine::ocr::recognizers(&settings).map_err(failed)?; + // Each file opens in its own session; it keeps this one's JavaScript preference. + let javascript = self.session.javascript(); let mut out = Vec::new(); let (mut ok, mut skipped_files, mut words) = (0usize, 0usize, 0usize); for p in paths { @@ -171,7 +173,15 @@ impl Automation { let result = self.resolve(p, false).map_err(|e| e.to_string()).and_then(|src| crate::read_input_bytes(&src).map_err(|e| e.to_string())).and_then( |bytes| { - pdfcraft_engine::ocr::recognize_file(&name, std::sync::Arc::new(bytes), None, settings.clone(), &recognizers, |_, _| true) + pdfcraft_engine::ocr::recognize_file( + &name, + std::sync::Arc::new(bytes), + None, + settings.clone(), + &recognizers, + javascript, + |_, _| true, + ) }, ); match result { diff --git a/crates/automation/src/batch.rs b/crates/automation/src/batch.rs index 14a98f23e2..9359cb50b8 100644 --- a/crates/automation/src/batch.rs +++ b/crates/automation/src/batch.rs @@ -19,9 +19,13 @@ struct Recipe<'a> { images: optimize::ImageSettings, mono: optimize::MonoSettings, ocr: ocr::OcrSettings, + /// The calling session's JavaScript preference, carried into each file's own session. + javascript: bool, } impl<'a> Recipe<'a> { + /// Read and validate `doc_batch`'s steps and settings, taking the JavaScript preference from + /// `auto`'s session. fn parse(auto: &Automation, args: &'a Args) -> Result { let steps = args.strs("steps")?; if steps.is_empty() || steps.len() > 3 { @@ -60,7 +64,7 @@ impl<'a> Recipe<'a> { if args.opt_num("dpi")?.is_some_and(|dpi| !(72.0..=600.0).contains(&dpi)) { return Err(Args::wrong("dpi", "72 to 600")); } - Ok(Self { steps, images, mono, ocr: auto.ocr_settings(args)? }) + Ok(Self { steps, images, mono, ocr: auto.ocr_settings(args)?, javascript: auto.session.javascript() }) } fn accepts(&self, path: &Path) -> bool { @@ -68,9 +72,13 @@ impl<'a> Recipe<'a> { extension == "pdf" || (self.steps.contains(&"convert") && pdfcraft_engine::CONVERTIBLE.contains(&extension.as_str())) } + /// Run the recipe's steps on the file at `path` in a fresh session; returns the output bytes + /// and a report entry per step. fn process(&self, path: &Path) -> Result<(Arc>, Vec)> { // A fresh session per file bounds retained document state and never touches caller tabs. + // It keeps the caller's JavaScript preference, set before any open runs open-time scripts. let mut session = Session::new(); + session.set_javascript(self.javascript); let mut input = Vec::new(); std::fs::File::open(path).map_err(failed)?.take(MAX_INPUT + 1).read_to_end(&mut input).map_err(failed)?; if input.len() as u64 > MAX_INPUT { @@ -85,7 +93,8 @@ impl<'a> Recipe<'a> { "convert" => bytes = session.convert_to_pdf(name, &bytes)?.1, "ocr" => { let recognizers = ocr::recognizers(&self.ocr).map_err(failed)?; - let result = ocr::recognize_file(name, bytes, None, self.ocr.clone(), &recognizers, |_, _| true).map_err(failed)?; + let result = + ocr::recognize_file(name, bytes, None, self.ocr.clone(), &recognizers, self.javascript, |_, _| true).map_err(failed)?; log.push( json!({"step":"ocr","words":result.words(),"skipped_pages":result.pages.iter().filter(|p| p.skipped.is_some()).count()}), ); diff --git a/crates/automation/tests/it/action_failures.rs b/crates/automation/tests/it/action_failures.rs index b487103a21..cae72832ab 100644 --- a/crates/automation/tests/it/action_failures.rs +++ b/crates/automation/tests/it/action_failures.rs @@ -19,14 +19,14 @@ fn one_step(step: Step) -> Action { /// The standard fonts can't draw the character, so the watermark step fails and no file is produced. #[test] fn failing_watermark_step_fails_the_file_and_publishes_nothing() { - let r = run_on(&one_step(Step::AddWatermark("中".into())), "one.pdf", Arc::new(pdf()), |_, _| {}); + let r = run_on(&one_step(Step::AddWatermark("中".into())), "one.pdf", Arc::new(pdf()), true, |_, _| {}); assert!(r.is_err(), "a failed step must fail its file: {r:?}"); } /// A comment step on a document with no comments has nothing to do, which is not a failure. #[test] fn nothing_to_do_step_still_succeeds() { - let r = run_on(&one_step(Step::FlattenComments), "one.pdf", Arc::new(pdf()), |_, _| {}); + let r = run_on(&one_step(Step::FlattenComments), "one.pdf", Arc::new(pdf()), true, |_, _| {}); assert!(r.is_ok(), "a step with nothing to do is not a failure: {r:?}"); } @@ -61,6 +61,6 @@ fn action_run_marks_the_failing_file_as_error_without_output() { /// A removal step with nothing to remove keeps its existing note and succeeds. #[test] fn removal_step_with_nothing_to_remove_still_succeeds() { - let r = run_on(&one_step(Step::RemoveHiddenInformation), "one.pdf", Arc::new(pdf()), |_, _| {}); + let r = run_on(&one_step(Step::RemoveHiddenInformation), "one.pdf", Arc::new(pdf()), true, |_, _| {}); assert!(r.is_ok(), "nothing to remove is not a failure: {r:?}"); } diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index ea883f499e..be0510c5d7 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -9694,6 +9694,8 @@ fn comparing_documents_through_tools() { assert!(a.call("doc_compare", &json!({ "doc": v2, "other": 999 })).is_err()); } +/// `action_list` and `action_run`: built-in actions by name and ad-hoc steps run on files, +/// missing inputs are reported per file, and an unknown step is refused. #[test] fn actions_through_tools() { let dir = workdir("actions"); @@ -9711,6 +9713,71 @@ fn actions_through_tools() { assert!(a.call("action_run", &json!({ "steps": [{ "step": "fly" }], "paths": ["a.pdf"], "folder": "x" })).is_err()); } +/// JavaScript stays off in the short-lived sessions used by the three file-processing entry +/// points (#5080). The input's initialize script changes `qty` to 42, which is visible in the +/// saved XFA data if any entry point opens it with JavaScript enabled. +#[test] +fn file_processing_keeps_javascript_off_for_xfa_open_scripts() { + let dir = workdir("batch-javascript-off-xfa"); + let mut seed = 0x1234_5678_u32; + let padding: String = (0..32_000) + .map(|_| { + seed ^= seed << 13; + seed ^= seed >> 17; + seed ^= seed << 5; + char::from(b'a' + (seed % 26) as u8) + }) + .collect(); + let template = pdfcraft_xfa::fixtures::scripted_template() + .replace("if (qty.rawValue === null) qty.rawValue = 2;", "qty.rawValue = 42; pad.rawValue = '';") + .replace( + "{padding}\n ().unwrap(); + let orphan = "x".repeat(64_000); + let orphan_offset = source.len(); + source.extend_from_slice(format!("999 0 obj\n<< /Length {} >>\nstream\n{orphan}\nendstream\nendobj\n", orphan.len()).as_bytes()); + let xref = source.len(); + source.extend_from_slice( + format!("xref\n999 1\n{orphan_offset:010} 00000 n \ntrailer\n<< /Size 1000 /Root 1 0 R /Prev {previous_xref} >>\nstartxref\n{xref}\n%%EOF\n") + .as_bytes(), + ); + std::fs::write(dir.join("scripted.pdf"), &source).unwrap(); + + let mut a = auto(&dir); + ok(&mut a, "js_enabled", json!({ "enabled": false })); + + ok(&mut a, "action_run", json!({ "steps": [{ "step": "set_title", "arg": "Processed" }], "paths": ["scripted.pdf"], "folder": "actions" })); + ok(&mut a, "doc_batch", json!({ "paths": ["scripted.pdf"], "steps": ["optimize"], "out_dir": "batch" })); + + // Open each result through the same JavaScript-off session and inspect the field data. + for path in ["actions/scripted.pdf", "batch/scripted.pdf"] { + let doc = ok(&mut a, "doc_open", json!({ "path": path }))["doc"].as_u64().unwrap(); + let fields = ok(&mut a, "form_fields", json!({ "doc": doc })); + let qty = fields["fields"].as_array().unwrap().iter().find(|field| field["name"] == "qty").unwrap(); + assert_ne!(qty["value"], "42", "{path} ran the XFA initialize script"); + } + + // OCR needs its language models. When present, it uses the same per-file open path and must + // also keep the script from changing the saved field value. Without them, the engine's + // `ocr::tests::files_open_for_recognition_with_the_callers_javascript_preference` still + // checks the preference `recognize_file` opens each file with. + if ok(&mut a, "ocr_status", json!({}))["available"] == true { + ok(&mut a, "ocr_recognize_files", json!({ "paths": ["scripted.pdf"], "folder": "ocr", "skip_text_pages": false })); + let doc = ok(&mut a, "doc_open", json!({ "path": "ocr/scripted.pdf" }))["doc"].as_u64().unwrap(); + let fields = ok(&mut a, "form_fields", json!({ "doc": doc })); + let qty = fields["fields"].as_array().unwrap().iter().find(|field| field["name"] == "qty").unwrap(); + assert_ne!(qty["value"], "42", "OCR ran the XFA initialize script"); + } +} + #[test] fn action_run_keeps_outputs_with_duplicate_input_basenames() { let dir = workdir("action-duplicate-basenames"); diff --git a/crates/engine/src/actions.rs b/crates/engine/src/actions.rs index 249279e09d..6608cb3297 100644 --- a/crates/engine/src/actions.rs +++ b/crates/engine/src/actions.rs @@ -151,6 +151,8 @@ fn images_refused_note(images: usize) -> Option { (images > 0).then(|| format!("Reduce file size: {images} image(s) stayed as they were, past what one run may decode")) } +/// Apply one action step to document `id`, adding what it did to `log`; returns the new +/// document when the step replaced it. fn apply_step(s: &mut Session, id: DocId, step: &Step, log: &mut Vec) -> Result, EditError> { let pages = s.get(id).map_or(0, |d| d.info.pages.len()); let all: Vec = (0..pages).collect(); @@ -209,9 +211,19 @@ fn apply_step(s: &mut Session, id: DocId, step: &Step, log: &mut Vec) -> Ok(None) } -/// Run `action` on one file. `progress(step, steps)` is called before each step. -pub fn run_on(action: &Action, name: &str, bytes: Arc>, mut progress: impl FnMut(usize, usize)) -> Result { +/// Run `action` on one file. `javascript` is the caller's [`Session::javascript`] preference: +/// when off, neither open-time scripts nor "Run a JavaScript" steps run. `progress(step, steps)` +/// is called before each step. +pub fn run_on( + action: &Action, + name: &str, + bytes: Arc>, + javascript: bool, + mut progress: impl FnMut(usize, usize), +) -> Result { let mut s = Session::new(); + // Before the open: open-time (XFA) scripts read it there. + s.set_javascript(javascript); let mut id = s.open(name, None, bytes, None).map_err(|e| e.to_string())?; if let Some(why) = s.get(id).and_then(|d| d.read_only_reason.clone()) { return Err(why); diff --git a/crates/engine/src/ocr.rs b/crates/engine/src/ocr.rs index 539a03a6aa..b2a4717e46 100644 --- a/crates/engine/src/ocr.rs +++ b/crates/engine/src/ocr.rs @@ -1156,6 +1156,7 @@ pub struct FileResult { } impl FileResult { + /// How many words were recognized on all of the file's pages. pub fn words(&self) -> usize { self.pages.iter().map(|p| p.words.len()).sum() } @@ -1202,19 +1203,21 @@ pub fn low_confidence_pages(pages: &[OcrPage], threshold: f32) -> Option<(usize, (count > 0).then_some((count, page_numbers)) } -/// Recognize text in multiple files: one PDF's bytes in, the searchable PDF out. `progress` -/// works as for [`OcrJob::run`]. A file that the skip-files option passes over comes back with -/// [`FileResult::file_skipped`] set and its original bytes; the caller decides what to do with -/// it, but it is never an OCR success. +/// Recognize text in multiple files: one PDF's bytes in, the searchable PDF out. `javascript` is +/// the caller's [`Session::javascript`] preference (when off, no open-time script runs). +/// `progress` works as for [`OcrJob::run`]. A file that the skip-files option passes over comes +/// back with [`FileResult::file_skipped`] set and its original bytes; the caller decides what to +/// do with it, but it is never an OCR success. pub fn recognize_file( name: &str, bytes: Arc>, password: Option<&str>, settings: OcrSettings, recognizers: &Recognizers, + javascript: bool, progress: impl FnMut(usize, usize) -> bool, ) -> Result { - match FileJob::open(name, bytes, password, settings)? { + match FileJob::open(name, bytes, password, settings, javascript)? { Opened::Skipped(result) => Ok(result), Opened::Read(mut file) => { let job = file.job.take().ok_or("the document could not be read")?; @@ -1242,12 +1245,9 @@ pub enum Opened { } impl FileJob { - pub fn open(name: &str, bytes: Arc>, password: Option<&str>, settings: OcrSettings) -> Result { - let mut session = Session::new(); - let id = session.open(name, None, bytes.clone(), password).map_err(|e| e.to_string())?; - if let Some(why) = session.get(id).and_then(|d| d.read_only_reason.clone()) { - return Err(why); - } + /// Open the file; `javascript` is as for [`recognize_file`]. + pub fn open(name: &str, bytes: Arc>, password: Option<&str>, settings: OcrSettings, javascript: bool) -> Result { + let (session, id) = open_file(name, bytes.clone(), password, javascript)?; let job = session.ocr_job(id, &[], settings.clone()).ok_or("the document could not be read")?; // The skip-files probe runs before any engine work: a file with text needs no engine at // all, and the original bytes come back unchanged. @@ -1280,11 +1280,38 @@ impl FileJob { } } +/// Open one of [`recognize_file`]'s files in a session of its own that keeps the caller's +/// JavaScript preference; a file that cannot be changed is refused with the reason. +fn open_file(name: &str, bytes: Arc>, password: Option<&str>, javascript: bool) -> Result<(Session, DocId), String> { + let mut s = Session::new(); + // Before the open: open-time (XFA) scripts read it there. + s.set_javascript(javascript); + let id = s.open(name, None, bytes, password).map_err(|e| e.to_string())?; + if let Some(why) = s.get(id).and_then(|d| d.read_only_reason.clone()) { + return Err(why); + } + Ok((s, id)) +} + #[cfg(test)] mod tests { use super::*; use crate::export; + /// #5080: [`recognize_file`] opens each file with the caller's JavaScript preference, so an + /// XFA initialize script (it sets an empty `qty` to 2) runs only when that is on. This checks + /// the open path without the recognition models, which the end-to-end test needs. + #[test] + fn files_open_for_recognition_with_the_callers_javascript_preference() { + let bytes = Arc::new(pdfcraft_xfa::fixtures::shell(&pdfcraft_xfa::fixtures::scripted_template())); + let qty = |javascript| { + let (s, id) = open_file("scripted.pdf", bytes.clone(), None, javascript).unwrap(); + s.get(id).unwrap().form.iter().find(|f| f.name == "qty").unwrap().value.clone() + }; + assert!(qty(false).is_empty(), "the initialize script ran with JavaScript off"); + assert_eq!(qty(true), ["2"], "the initialize script did not run with JavaScript on"); + } + /// One 300 × 300 page drawing `content`, with Helvetica as `/F1`. fn page(content: &str) -> Arc> { let pdf = format!( @@ -1741,7 +1768,7 @@ mod tests { let bytes = s.create_from_text("t", "Already typed here").unwrap(); let (r, fake) = fake_recognizers(); let settings = OcrSettings { skip_text_files: true, ..Default::default() }; - let out = recognize_file("t.pdf", bytes.clone(), None, settings, &r, |_, _| true).unwrap(); + let out = recognize_file("t.pdf", bytes.clone(), None, settings, &r, true, |_, _| true).unwrap(); assert_eq!(fake.calls(), 0, "the engine never ran"); assert_eq!(out.bytes.as_ptr(), bytes.as_ptr(), "the original bytes came back"); assert_eq!(out.file_skipped.as_deref(), Some("the file already contains text")); @@ -1756,7 +1783,7 @@ mod tests { let bytes = s.create_from_text("t", "Already typed here").unwrap(); let (r, fake) = fake_recognizers(); let settings = OcrSettings { skip_text_files: true, force_ocr: true, ..Default::default() }; - let out = recognize_file("t.pdf", bytes, None, settings, &r, |_, _| true).unwrap(); + let out = recognize_file("t.pdf", bytes, None, settings, &r, true, |_, _| true).unwrap(); assert_eq!(fake.calls(), 1); assert_eq!(out.file_skipped, None); assert_eq!(out.words(), 1); @@ -1770,7 +1797,7 @@ mod tests { let bytes = s.create_from_text("t", "A page with text").unwrap(); let (r, fake) = fake_recognizers(); let settings = OcrSettings { force_ocr: true, ..Default::default() }; - let out = recognize_file("t.pdf", bytes, None, settings, &r, |_, _| true).unwrap(); + let out = recognize_file("t.pdf", bytes, None, settings, &r, true, |_, _| true).unwrap(); assert_eq!(fake.calls(), 1, "the page's own text did not stop the run"); assert_eq!(out.pages[0].skipped, None); } @@ -1782,12 +1809,12 @@ mod tests { let s = Session::new(); let bytes = s.create_from_text("t", "A page with text").unwrap(); let (r, fake) = fake_recognizers(); - let out = recognize_file("t.pdf", bytes, None, OcrSettings::default(), &r, |_, _| true).unwrap(); + let out = recognize_file("t.pdf", bytes, None, OcrSettings::default(), &r, true, |_, _| true).unwrap(); assert_eq!(fake.calls(), 0); assert_eq!(out.pages[0].skipped.as_deref(), Some("the page already contains text")); assert_eq!(out.words(), 0); // Second run over the same (already text-bearing) file: skipped pages again, no calls. - let again = recognize_file("t.pdf", out.bytes, None, OcrSettings::default(), &r, |_, _| true).unwrap(); + let again = recognize_file("t.pdf", out.bytes, None, OcrSettings::default(), &r, true, |_, _| true).unwrap(); assert_eq!(fake.calls(), 0); assert_eq!(again.pages[0].skipped.as_deref(), Some("the page already contains text")); } @@ -1863,14 +1890,14 @@ mod tests { let bytes = s.create_from_text("t", "Already typed here").unwrap(); let (r, fake) = fake_recognizers(); let skip = OcrSettings { skip_text_files: true, ..Default::default() }; - let Ok(Opened::Skipped(out)) = FileJob::open("t.pdf", bytes.clone(), None, skip) else { panic!("the file should be skipped") }; + let Ok(Opened::Skipped(out)) = FileJob::open("t.pdf", bytes.clone(), None, skip, true) else { panic!("the file should be skipped") }; assert_eq!(out.file_skipped.as_deref(), Some("the file already contains text")); let force = OcrSettings { force_ocr: true, ..Default::default() }; - let Ok(Opened::Read(mut file)) = FileJob::open("t.pdf", bytes.clone(), None, force.clone()) else { panic!("the file should be read") }; + let Ok(Opened::Read(mut file)) = FileJob::open("t.pdf", bytes.clone(), None, force.clone(), true) else { panic!("the file should be read") }; let pages = file.job.take().unwrap().run(&r, |_, _| true); let out = file.finish(pages).unwrap(); assert_eq!((fake.calls(), out.words(), out.file_skipped.clone()), (1, 1, None)); - let whole = recognize_file("t.pdf", bytes, None, force, &r, |_, _| true).unwrap(); + let whole = recognize_file("t.pdf", bytes, None, force, &r, true, |_, _| true).unwrap(); // The saved bytes carry a modification time, so two saves a second apart differ there alone. assert_eq!((out.pages, out.file_skipped), (whole.pages, whole.file_skipped), "the same as reading it in one call"); } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 5952efab88..81c98d05ef 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -7491,13 +7491,15 @@ fn comparing_two_versions_of_a_document() { assert!(marks[0].contents.as_deref().unwrap().starts_with("Replaced: \"Monday.\"")); } +/// Built-in and custom actions run their steps on a file (with JavaScript on), report each +/// step's progress, and fail cleanly on a file that is not a PDF. #[test] fn actions_run_their_steps_on_files() { let s = Session::new(); let src = s.create_from_text("t", "Quarterly numbers").unwrap(); let built = actions::builtin(); let numbers = built.iter().find(|a| a.name == "Add Page Numbers").unwrap(); - let r = actions::run_on(numbers, "q.pdf", src.clone(), |_, _| {}).unwrap(); + let r = actions::run_on(numbers, "q.pdf", src.clone(), true, |_, _| {}).unwrap(); let mut s2 = Session::new(); let id = s2.open("out.pdf", None, r.bytes, None).unwrap(); assert!(page_texts(&s2, id)[0].contains("Page 1 of 1"), "{:?}", page_texts(&s2, id)); @@ -7515,14 +7517,38 @@ fn actions_run_their_steps_on_files() { builtin: false, }; let mut steps = Vec::new(); - let r = actions::run_on(&custom, "q.pdf", src, |i, n| steps.push((i, n))).unwrap(); + let r = actions::run_on(&custom, "q.pdf", src, true, |i, n| steps.push((i, n))).unwrap(); assert_eq!(steps, [(0, 4), (1, 4), (2, 4), (3, 4)]); assert_eq!(r.log.len(), 4, "{:?}", r.log); let id = s2.open("out2.pdf", None, r.bytes, None).unwrap(); let d = s2.get(id).unwrap(); assert_eq!(d.info.title.as_deref(), Some("Q3")); assert!(page_texts(&s2, id)[0].contains("DRAFT")); - assert!(actions::run_on(&custom, "bad.pdf", Arc::new(b"nope".to_vec()), |_, _| {}).is_err()); + assert!(actions::run_on(&custom, "bad.pdf", Arc::new(b"nope".to_vec()), true, |_, _| {}).is_err()); +} + +/// #5080: a "Run a JavaScript" step honours the caller's JavaScript preference. With it on, the +/// step runs and writes the field; with it off, the step does not run, so `run_on` reports the +/// refused script's error instead of saving a script-produced value. Before the fix each file +/// opened in a fresh JavaScript-on session, so the step ran whatever the caller chose. +#[test] +fn an_action_runs_a_javascript_step_only_when_javascript_is_on() { + let action = actions::Action { + name: "Set price".into(), + description: String::new(), + steps: vec![actions::Step::RunJavaScript("getField('price').value = 3;".into())], + builtin: false, + }; + let src = Arc::new(scripted_form()); + // JavaScript on: the step runs, so the saved file has price = 3. + let on = actions::run_on(&action, "f.pdf", src.clone(), true, |_, _| {}).unwrap(); + let mut s = Session::new(); + s.set_javascript(false); // read the saved state without re-running any script + let id = s.open("on.pdf", None, on.bytes, None).unwrap(); + let price = s.get(id).unwrap().form.iter().find(|f| f.name == "price").unwrap().value.clone(); + assert_eq!(price, ["3"], "the script ran with JavaScript on"); + // JavaScript off: the step does not run, so run_on fails rather than writing a scripted value. + assert!(actions::run_on(&action, "f.pdf", src, false, |_, _| {}).is_err()); } /// A removal step that fails (here: a signed file can't be rewritten without its hidden data) @@ -7539,7 +7565,7 @@ fn actions_fail_when_hidden_information_could_not_be_removed() { let signed = s.sign(id, &digital_id, SignOptions { page: 1, ..SignOptions::default() }).unwrap(); let built = actions::builtin(); let sanitize = built.iter().find(|a| a.name == "Sanitize Documents").unwrap(); - let e = actions::run_on(sanitize, "signed.pdf", signed, |_, _| {}).map(|r| r.log).unwrap_err(); + let e = actions::run_on(sanitize, "signed.pdf", signed, true, |_, _| {}).map(|r| r.log).unwrap_err(); assert!(e.starts_with("Sanitize document:"), "{e}"); } @@ -10967,7 +10993,7 @@ fn an_action_appends_to_a_file_whose_field_walk_passes_a_signature_over() { let bytes = Arc::new(signed_field_named(64 << 10)); let steps = vec![actions::Step::from_id("set_title", "Checked").unwrap()]; let title = actions::Action { name: "Title".into(), description: String::new(), steps, builtin: false }; - let r = actions::run_on(&title, "form.pdf", bytes.clone(), |_, _| {}).unwrap(); + let r = actions::run_on(&title, "form.pdf", bytes.clone(), true, |_, _| {}).unwrap(); assert!(r.bytes.starts_with(&bytes[..]), "the action rewrote the file instead of appending to it"); let mut s = Session::new(); let id = s.open("out.pdf", None, r.bytes, None).unwrap(); diff --git a/crates/ui-egui/src/workflows/actions_ui.rs b/crates/ui-egui/src/workflows/actions_ui.rs index 313d55c5f2..55b94039c0 100644 --- a/crates/ui-egui/src/workflows/actions_ui.rs +++ b/crates/ui-egui/src/workflows/actions_ui.rs @@ -98,6 +98,8 @@ impl PdfCraftApp { let p = progress.clone(); // The summary is written on the worker thread: draw it in the UI's language. let lang = crate::i18n::current(); + // Each file runs in its own session; it keeps this one's JavaScript preference. + let javascript = self.session.javascript(); let work = move || { crate::i18n::set_current(lang); let (mut ok, mut failed) = (0, Vec::new()); @@ -105,7 +107,7 @@ impl PdfCraftApp { if let Ok(mut s) = p.lock() { s.done = i; } - let r = pdfcraft_engine::actions::run_on(&action, &name, Arc::new(bytes), |_, _| {}).and_then(|r| { + let r = pdfcraft_engine::actions::run_on(&action, &name, Arc::new(bytes), javascript, |_, _| {}).and_then(|r| { #[cfg(not(target_arch = "wasm32"))] write_result(&dir, &name, &r.bytes).map_err(|e| e.to_string())?; #[cfg(target_arch = "wasm32")] diff --git a/crates/ui-egui/src/workflows/ocr_ui.rs b/crates/ui-egui/src/workflows/ocr_ui.rs index 756da56177..91378d9172 100644 --- a/crates/ui-egui/src/workflows/ocr_ui.rs +++ b/crates/ui-egui/src/workflows/ocr_ui.rs @@ -691,7 +691,8 @@ impl PdfCraftApp { #[cfg(target_arch = "wasm32")] fn start_web_batch(&mut self, files: Vec<(String, Vec)>, settings: OcrSettings) { let progress = Arc::new(Mutex::new(BatchProgress { total: files.len(), ..Default::default() })); - self.ocr_web_batch = Some(crate::ocr_web::Batch::new(files, settings)); + // Each file opens in its own session; it keeps this one's JavaScript preference. + self.ocr_web_batch = Some(crate::ocr_web::Batch::new(files, settings, self.session.javascript())); self.ocr_batch = Some(progress); } @@ -736,13 +737,16 @@ impl PdfCraftApp { let p = progress.clone(); // The summary is written on the worker thread: draw it in the UI's language. let lang = crate::i18n::current(); + // Each file opens in its own session; it keeps this one's JavaScript preference. + let javascript = self.session.javascript(); let work = move || { crate::i18n::set_current(lang); let write = move |name: &str, r: &pdfcraft_engine::ocr::FileResult| { crate::editing::write_atomically(&dir.join(name).to_string_lossy(), &r.bytes).map_err(|e| e.to_string()) }; // A panic in the recognizer still ends the batch with a summary (#3919). - let msg = pdfcraft_engine::guard(|| run_batch_files(files, settings, &recognizers, &p, write)).unwrap_or_else(|panic| failed(&panic)); + let msg = pdfcraft_engine::guard(|| run_batch_files(files, settings, &recognizers, javascript, &p, write)) + .unwrap_or_else(|panic| failed(&panic)); if let Ok(mut s) = p.lock() { s.done = s.total; s.message = Some(msg); @@ -981,12 +985,14 @@ impl PdfCraftApp { /// The batch's worker: read each file, hand the searchable copy to `write` (the output folder, /// or the web's download), count the buckets, and compose the summary. A file the skip-files /// option passes over is its own bucket and `write` is never called for it. A cancel is asked -/// for between files: the file in flight finishes, the rest are counted as not run. +/// for between files: the file in flight finishes, the rest are counted as not run. `javascript` +/// is the app session's preference, for each file's own session. #[cfg(not(target_arch = "wasm32"))] pub(crate) fn run_batch_files( files: Vec<(String, Vec)>, settings: OcrSettings, recognizers: &pdfcraft_engine::ocr::Recognizers, + javascript: bool, progress: &Mutex, mut write: impl FnMut(&str, &pdfcraft_engine::ocr::FileResult) -> Result<(), String>, ) -> String { @@ -1001,7 +1007,7 @@ pub(crate) fn run_batch_files( if let Ok(mut s) = progress.lock() { s.done = i; } - let r = pdfcraft_engine::ocr::recognize_file(&name, Arc::new(bytes), None, settings.clone(), recognizers, |_, _| true); + let r = pdfcraft_engine::ocr::recognize_file(&name, Arc::new(bytes), None, settings.clone(), recognizers, javascript, |_, _| true); match r { Ok(res) if res.skipped() => skipped += 1, Ok(res) => match write(&name, &res) { @@ -1328,7 +1334,7 @@ mod tests { let recognizers = Recognizers { primary: Arc::new(Fake { confidence: 95.0 }), secondary: None }; let writes = Arc::new(std::sync::atomic::AtomicUsize::new(0)); let w = writes.clone(); - let msg = run_batch_files(files, OcrSettings { skip_text_files: true, ..Default::default() }, &recognizers, &progress, move |_, _| { + let msg = run_batch_files(files, OcrSettings { skip_text_files: true, ..Default::default() }, &recognizers, true, &progress, move |_, _| { w.fetch_add(1, std::sync::atomic::Ordering::SeqCst); Ok(()) }); @@ -1338,7 +1344,7 @@ mod tests { let files = vec![("a.pdf".into(), pdf.to_vec()), ("b.pdf".into(), pdf.to_vec())]; let progress = Mutex::new(BatchProgress { total: files.len(), ..Default::default() }); let w = writes.clone(); - let msg = run_batch_files(files, OcrSettings { skip_text_files: true, ..Default::default() }, &recognizers, &progress, move |_, _| { + let msg = run_batch_files(files, OcrSettings { skip_text_files: true, ..Default::default() }, &recognizers, true, &progress, move |_, _| { w.fetch_add(1, std::sync::atomic::Ordering::SeqCst); Ok(()) }); @@ -1359,13 +1365,13 @@ mod tests { // The fixtures are text PDFs standing in for scans; the default skip-text-pages option // would (correctly) leave them alone, so the batch here reads everything. let settings = OcrSettings { skip_text_pages: false, ..Default::default() }; - let msg = run_batch_files(files, settings.clone(), &recognizers, &progress, |_, _| Ok(())); + let msg = run_batch_files(files, settings.clone(), &recognizers, true, &progress, |_, _| Ok(())); assert!(msg.contains("Recognized 2 words in 2 files"), "{msg}"); assert!(msg.contains("scan.pdf: 1 low-confidence word(s) on pages 1"), "{msg}"); // The write failing is a per-file failure, named in the summary. let files = vec![("scan.pdf".into(), pdf.to_vec())]; let progress = Mutex::new(BatchProgress { total: files.len(), ..Default::default() }); - let msg = run_batch_files(files, settings, &recognizers, &progress, |_, _| Err("disk full".into())); + let msg = run_batch_files(files, settings, &recognizers, true, &progress, |_, _| Err("disk full".into())); assert!(msg.contains("; failed: scan.pdf: disk full"), "{msg}"); } @@ -1380,7 +1386,7 @@ mod tests { let files = vec![("scan.pdf".into(), pdf.to_vec())]; let progress = Mutex::new(BatchProgress { total: files.len(), ..Default::default() }); let settings = OcrSettings { skip_text_pages: false, low_confidence_threshold: threshold, ..Default::default() }; - run_batch_files(files, settings, &recognizers, &progress, |_, _| Ok(())) + run_batch_files(files, settings, &recognizers, true, &progress, |_, _| Ok(())) }; assert!(run(OcrSettings::default().low_confidence_threshold).contains("1 low-confidence word(s)"), "the default flags a 40% word"); assert!(run(40.0).contains("1 low-confidence word(s)"), "at the threshold counts"); diff --git a/crates/ui-egui/src/workflows/ocr_web.rs b/crates/ui-egui/src/workflows/ocr_web.rs index 57f3a8348a..05f587ba63 100644 --- a/crates/ui-egui/src/workflows/ocr_web.rs +++ b/crates/ui-egui/src/workflows/ocr_web.rs @@ -446,6 +446,8 @@ fn place(shot: &OcrShot, lines: Result, String>) -> OcrPage { pub struct Batch { files: VecDeque<(String, Vec)>, settings: OcrSettings, + /// The app session's JavaScript preference, for each file's own session. + javascript: bool, current: Option<(String, Box, Pages)>, total: usize, pub ok: usize, @@ -459,11 +461,12 @@ pub struct Batch { } impl Batch { - pub fn new(files: Vec<(String, Vec)>, settings: OcrSettings) -> Batch { + pub fn new(files: Vec<(String, Vec)>, settings: OcrSettings, javascript: bool) -> Batch { let total = files.len(); Batch { files: files.into(), settings, + javascript, current: None, total, ok: 0, @@ -489,7 +492,7 @@ impl Batch { self.files.clear(); } let Some((name, bytes)) = self.files.pop_front() else { return true }; - match FileJob::open(&name, Arc::new(bytes), None, self.settings.clone()) { + match FileJob::open(&name, Arc::new(bytes), None, self.settings.clone(), self.javascript) { Ok(Opened::Skipped(_)) => self.skipped += 1, Ok(Opened::Read(mut file)) => match file.job.take() { Some(job) => self.current = Some((name, file, Pages::new(job.steps()))),