From 90982ba9a86233db45e674baea493fc26196151f Mon Sep 17 00:00:00 2001
From: aiongg <aiongtaigi@gmail.com>
Date: Thu, 22 Oct 2026 16:03:50 +0000
Subject: [PATCH] fix(preflight): check the actions an allowed action chains to

Conversion and verification looked only at an action's own /S. An allowed
action (a GoTo, say) whose /Next held a JavaScript or other forbidden
action written directly inside it was kept, and verification never looked
at actions written directly in a catalog /OpenAction or an annotation /A.
Both now follow /Next through the actions written inside an action (an
indirect /Next is an object of its own, checked as one), within a bound,
and conversion removes the whole chain.
---
 crates/preflight/src/lib.rs   | 63 +++++++++++++++++++++++++++--------
 crates/preflight/src/tests.rs | 26 +++++++++++++++
 2 files changed, 76 insertions(+), 13 deletions(-)

diff --git a/crates/preflight/src/lib.rs b/crates/preflight/src/lib.rs
index 731234aa6b..905ba8cbf3 100644
--- a/crates/preflight/src/lib.rs
+++ b/crates/preflight/src/lib.rs
@@ -515,12 +515,14 @@ pub fn verify(doc: &Document, level: Level) -> Vec<Issue> {
         let Some(d) = dict_of(o) else { continue };
         let ty = d.name(b"Type");
         let sub = d.name(b"Subtype");
-        // Actions anywhere (structure elements also have /S: their role).
-        if let Some(s) = d.name(b"S")
-            && FORBIDDEN_ACTIONS.contains(&s)
-            && is_action(d)
-        {
-            issue("6.5.1", "A {} action isn't allowed", Some(String::from_utf8_lossy(s).into_owned()), None, true);
+        // Actions anywhere (structure elements also have /S: their role), with the actions they
+        // chain to and those written directly in an /A or /OpenAction.
+        let own = (d.get(b"S").is_some() && is_action(d)).then_some(d);
+        let direct = [b"A".as_slice(), b"OpenAction"].map(|k| d.get(k).and_then(|a| a.as_dict()));
+        for a in [own].into_iter().chain(direct).flatten() {
+            if let Some(s) = chain_forbidden(a) {
+                issue("6.5.1", "A {} action isn't allowed", Some(s), None, true);
+            }
         }
         // Annotations.
         if ty == Some(b"Annot") || (sub.is_some() && d.get(b"Rect").is_some() && d.get(b"P").is_some()) {
@@ -652,10 +654,39 @@ fn is_action(d: &Dict) -> bool {
     }
 }
 
-/// Whether `o` is (or refers to) a forbidden action, or to nothing.
+/// How many actions one action and the `/Next` actions written inside it may hold before the
+/// whole chain counts as forbidden, since it can't be checked.
+const MAX_ACTION_CHAIN: usize = 1024;
+
+/// The first forbidden action type in the action `a` or in the actions written directly in its
+/// `/Next` (ISO 32000-2 12.6.2: a dictionary or an array of them). An action `/Next` refers to
+/// is an object of its own, checked and removed as one.
+fn chain_forbidden(a: &Dict) -> Option<String> {
+    let mut stack = vec![a];
+    let mut read = 0;
+    while let Some(d) = stack.pop() {
+        read += 1;
+        if read > MAX_ACTION_CHAIN {
+            return Some("too deeply chained".into());
+        }
+        if let Some(s) = d.name(b"S")
+            && FORBIDDEN_ACTIONS.contains(&s)
+        {
+            return Some(String::from_utf8_lossy(s).into_owned());
+        }
+        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)),
+            _ => {}
+        }
+    }
+    None
+}
+
+/// Whether `o` is (or refers to) a forbidden action or one chaining to it, or to nothing.
 fn forbidden(doc: &Document, o: &Object) -> bool {
     match &*doc.resolve(o) {
-        Object::Dict(a) => a.name(b"S").is_some_and(|s| FORBIDDEN_ACTIONS.contains(&s)),
+        Object::Dict(a) => chain_forbidden(a).is_some(),
         Object::Null => true,
         _ => false,
     }
@@ -801,14 +832,20 @@ pub fn convert(doc: &mut Document, level: Level) -> Result<Report, pdfcraft_cos:
             files += 1;
         }
     }
-    // Forbidden actions left as objects of their own (nothing refers to them any more).
+    // Forbidden actions left as objects of their own, or chained to by one, and those written
+    // directly in an /A elsewhere (an outline item's, say).
     for (r, o) in objects(doc) {
-        if let Object::Dict(d) = &*o
-            && d.name(b"S").is_some_and(|s| FORBIDDEN_ACTIONS.contains(&s))
-            && is_action(d)
-        {
+        let Object::Dict(d) = &*o else { continue };
+        if d.get(b"S").is_some() && is_action(d) && chain_forbidden(d).is_some() {
             doc.set(r, Object::Null);
             actions += 1;
+        } else if let Some(Object::Dict(a)) = d.get(b"A")
+            && chain_forbidden(a).is_some()
+        {
+            let mut nd = d.clone();
+            nd.remove(b"A");
+            doc.set(r, Object::Dict(nd));
+            actions += 1;
         }
     }
     for (n, what) in [
diff --git a/crates/preflight/src/tests.rs b/crates/preflight/src/tests.rs
index a44d47de15..d4a2c8157b 100644
--- a/crates/preflight/src/tests.rs
+++ b/crates/preflight/src/tests.rs
@@ -430,6 +430,32 @@ fn content_colour_reads_operators_one_at_a_time_and_stops_when_all_are_found() {
     assert_eq!(read, 3);
 }
 
+/// A forbidden action chained by `/Next` behind an allowed one, written directly inside its
+/// parent, is reported and removed like any other.
+#[test]
+fn forbidden_actions_chained_behind_allowed_ones_are_found_and_removed() {
+    let js = "<< /S /JavaScript /JS (app.alert(1)) >>";
+    let page = "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>";
+    let pages = "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 200] >>";
+    let link = |a: &str| format!("<< /Type /Annot /Subtype /Link /Rect [0 0 0 0] /P 3 0 R /F 4 /A {a} >>");
+    let open_action = format!("<< /Type /Catalog /Pages 2 0 R /OpenAction << /S /GoTo /D [3 0 R /Fit] /Next {js} >> >>");
+    let goto = format!("<< /Type /Action /S /GoTo /D [3 0 R /Fit] /Next [{js}] >>");
+    let cases = [
+        build(&[&open_action, pages, page, &link("<< /S /GoTo /D [3 0 R /Fit] >>")], ""),
+        build(&["<< /Type /Catalog /Pages 2 0 R >>", pages, page, &link("5 0 R"), &goto], ""),
+        build(&["<< /Type /Catalog /Pages 2 0 R >>", pages, page, &link(&format!("<< /S /GoTo /D [3 0 R /Fit] /Next {js} >>"))], ""),
+    ];
+    for (i, mut doc) in cases.into_iter().enumerate() {
+        let found = messages(&verify(&doc, Level::A2b)).contains(&"A JavaScript action isn't allowed");
+        assert!(found, "case {i}: {:#?}", verify(&doc, Level::A2b));
+        let r = convert(&mut doc, Level::A2b).unwrap();
+        assert!(!r.remaining.iter().any(|x| x.clause == "6.5.1"), "case {i}: {:#?}", r.remaining);
+        let plain = pdfcraft_cos::SaveOptions { object_streams: false, ..Default::default() };
+        let bytes = pdfcraft_cos::write_full(&doc, &plain).unwrap();
+        assert!(!bytes.windows(11).any(|w| w == b"/JavaScript"), "case {i}: {}", String::from_utf8_lossy(&bytes));
+    }
+}
+
 /// Each annotation's page is found from one pass over the pages' `/Annots`, not by searching
 /// every page's list again for each annotation.
 #[test]