Charts: ignore relationships that point outside a chart's own parts (#335)

A chart relationship must not be able to replace a slide or another package part when the deck is saved.

Co-authored-by: Cursor <cursoragent@cursor.com>

crates/pptx/src/opc.rs

@@ -402,4 +402,17 @@ mod tests {
         assert_eq!(relative("ppt/presentation.xml", "ppt/slides/slide1.xml"), "slides/slide1.xml");
         assert_eq!(relative("ppt/slides/slide1.xml", "ppt/slides/slide2.xml"), "slide2.xml");
     }
+
+    #[test]
+    fn sidecar_parts_stay_inside_chart_folders() {
+        assert!(sidecar_part("ppt/embeddings/Microsoft_Excel_Worksheet1.xlsx"));
+        assert!(sidecar_part("ppt/charts/style1.xml"));
+        assert!(sidecar_part("ppt/drawings/chartDrawing1.xml"));
+        assert!(sidecar_part("ppt/media/image1.png"));
+        assert!(!sidecar_part("ppt/charts/chart1.xml"));
+        assert!(!sidecar_part("ppt/charts/_rels/chart1.xml.rels"));
+        assert!(!sidecar_part("ppt/slides/slide1.xml"));
+        assert!(!sidecar_part("ppt/embeddings/../../ppt/slides/slide1.xml"));
+        assert!(!sidecar_part("/etc/passwd"));
+    }
 }

crates/pptx/src/read/chart.rs

@@ -234,45 +234,71 @@ pub(crate) fn capture_kept(pkg: &crate::opc::Package, chart_part: &str) -> deckc
     use deckcraft_model::chart::{ChartKept, ChartRel};
 
     let mut kept = ChartKept::default();
-    let mut seen = std::collections::HashSet::new();
-    seen.insert(crate::opc::norm(chart_part));
+    // The chart document itself is rewritten; don't store a second copy of it.
+    let mut seen = std::collections::HashMap::from([(crate::opc::norm(chart_part), true)]);
     for r in pkg.rels(chart_part).items {
-        kept.rels.push(ChartRel { id: r.id, rel_type: r.ty, target: r.target_raw, external: r.external });
-        if !r.external {
-            copy_part(pkg, &r.target, &mut kept.parts, &mut seen, 0);
+        if !safe_rel(&r.id, &r.ty, &r.target_raw) {
+            log::warn!("pptx: skipping chart relationship {}", r.id);
+            continue;
+        }
+        if r.external {
+            kept.rels.push(ChartRel { id: r.id, rel_type: r.ty, target: r.target_raw, external: true });
+            continue;
+        }
+        if copy_part(pkg, &r.target, &mut kept.parts, &mut seen, 0) {
+            kept.rels.push(ChartRel { id: r.id, rel_type: r.ty, target: r.target_raw, external: false });
         }
     }
     kept
 }
 
+fn safe_rel(id: &str, ty: &str, target: &str) -> bool {
+    [id, ty, target].iter().all(|s| !s.is_empty() && s.len() <= 4096 && !s.chars().any(|c| c.is_control() || matches!(c, '<' | '>' | '"' | '\'')))
+}
+
+/// `true` when `part` is stored (now or from an earlier relationship).
 fn copy_part(
     pkg: &crate::opc::Package,
     part: &str,
     out: &mut Vec<deckcraft_model::chart::ChartPart>,
-    seen: &mut std::collections::HashSet<String>,
+    seen: &mut std::collections::HashMap<String, bool>,
     depth: usize,
-) {
+) -> bool {
     use deckcraft_model::chart::ChartPart;
 
     if depth > MAX_KEPT_DEPTH || out.len() >= MAX_KEPT_PARTS {
         log::warn!("pptx: chart part closure stopped at {part}");
-        return;
+        return false;
     }
     let key = crate::opc::norm(part);
-    if !seen.insert(key) {
-        return;
+    if let Some(ok) = seen.get(&key) {
+        return *ok;
+    }
+    if !crate::opc::sidecar_part(part) {
+        log::warn!("pptx: chart relationship target {part} is not kept");
+        seen.insert(key, false);
+        return false;
     }
     let Some(bytes) = pkg.get(part) else {
         log::warn!("pptx: chart relationship target {part} is missing");
-        return;
+        seen.insert(key, false);
+        return false;
     };
     let name = pkg.original_name(part).unwrap_or(part).trim_start_matches('/').to_string();
+    if !crate::opc::sidecar_part(&name) {
+        log::warn!("pptx: chart part name {name} is not kept");
+        seen.insert(key, false);
+        return false;
+    }
     let content_type = pkg.content_type(part).unwrap_or_default();
     out.push(ChartPart { name, content_type, bytes: bytes.to_vec() });
+    seen.insert(key, true);
     let rels_name = crate::opc::rels_path(part);
-    if seen.insert(crate::opc::norm(&rels_name))
+    if crate::opc::sidecar_part(&rels_name)
+        && seen.get(&crate::opc::norm(&rels_name)).is_none()
         && let Some(rels_bytes) = pkg.get(&rels_name)
     {
+        seen.insert(crate::opc::norm(&rels_name), true);
         let name = pkg.original_name(&rels_name).unwrap_or(&rels_name).trim_start_matches('/').to_string();
         out.push(ChartPart { name, content_type: "application/vnd.openxmlformats-package.relationships+xml".into(), bytes: rels_bytes.to_vec() });
         let targets: Vec<String> = pkg.rels(part).items.into_iter().filter(|r| !r.external).map(|r| r.target).collect();
@@ -280,4 +306,5 @@ fn copy_part(
             copy_part(pkg, &target, out, seen, depth + 1);
         }
     }
+    true
 }

crates/pptx/src/write/chart.rs

@@ -509,13 +509,7 @@ pub fn write_kept(pkg: &mut crate::opc::PackageOut, chart_name: &str, c: &Chart)
             log::warn!("pptx: not writing chart part {}", p.name);
             continue;
         }
-        let owned;
-        let ct = if !p.content_type.is_empty() {
-            p.content_type.as_str()
-        } else {
-            owned = crate::mime_for(&p.name).to_string();
-            owned.as_str()
-        };
-        pkg.binary(&p.name, ct, p.bytes.clone());
+        let ct = if !p.content_type.is_empty() { p.content_type.clone() } else { crate::mime_for(&p.name).to_string() };
+        pkg.binary(&p.name, &ct, p.bytes.clone());
     }
 }

crates/pptx/tests/chart_embed.rs

@@ -56,7 +56,7 @@ fn with_workbook(bytes: &[u8]) -> Vec<u8> {
     let chart_part = chart_part.expect("chart part");
     let rels = format!("ppt/charts/_rels/{}.rels", chart_part.rsplit('/').next().unwrap_or("chart1.xml"));
     let rels_xml = br#"<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
-<Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships"><Relationship Id="rId1" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/package" Target="../embeddings/Microsoft_Excel_Worksheet1.xlsx"/></Relationships>"#;
+<Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships"><Relationship Id="rId1" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/package" Target="../embeddings/Microsoft_Excel_Worksheet1.xlsx"/><Relationship Id="rIdBad" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/slide" Target="../slides/slide1.xml"/></Relationships>"#;
     out.start_file(rels, opts).expect("rels");
     out.write_all(rels_xml).expect("rels bytes");
     out.start_file(EMBED, opts).expect("embed");
@@ -106,6 +106,14 @@ fn an_unchanged_chart_keeps_its_embedded_workbook() {
         .map(|(_, d)| String::from_utf8_lossy(d).into_owned())
         .expect("chart relationships");
     assert!(rels.contains("../embeddings/Microsoft_Excel_Worksheet1.xlsx"), "{rels}");
+    assert!(!rels.contains("slide1.xml"), "a chart relationship must not replace the slide: {rels}");
+    let slide = saved_parts
+        .iter()
+        .find(|(n, _)| n == "ppt/slides/slide1.xml")
+        .map(|(_, d)| String::from_utf8_lossy(d).into_owned())
+        .expect("slide");
+    assert!(slide.contains("sld"), "{slide}");
+    assert!(!slide.as_bytes().windows(WORKBOOK.len()).any(|w| w == WORKBOOK));
     let xml = saved_parts
         .iter()
         .find(|(n, _)| n.starts_with("ppt/charts/chart") && n.ends_with(".xml"))