From 71c5b1712ad3ac8bb40b876d59bd1effab59173a Mon Sep 17 00:00:00 2001 From: Nikita Date: Tue, 13 Oct 2026 10:53:08 +0300 Subject: [PATCH] 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 --- crates/pptx/src/opc.rs | 13 ++++++++ crates/pptx/src/read/chart.rs | 51 ++++++++++++++++++++++++-------- crates/pptx/src/write/chart.rs | 10 ++----- crates/pptx/tests/chart_embed.rs | 10 ++++++- 4 files changed, 63 insertions(+), 21 deletions(-) diff --git a/crates/pptx/src/opc.rs b/crates/pptx/src/opc.rs index f6d3d3cc..2a44722f 100644 --- a/crates/pptx/src/opc.rs +++ b/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")); + } } diff --git a/crates/pptx/src/read/chart.rs b/crates/pptx/src/read/chart.rs index 2e7ceaa2..9cf694f0 100644 --- a/crates/pptx/src/read/chart.rs +++ b/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, - seen: &mut std::collections::HashSet, + seen: &mut std::collections::HashMap, 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 = 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 } diff --git a/crates/pptx/src/write/chart.rs b/crates/pptx/src/write/chart.rs index 5a075b01..752f712f 100644 --- a/crates/pptx/src/write/chart.rs +++ b/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()); } } diff --git a/crates/pptx/tests/chart_embed.rs b/crates/pptx/tests/chart_embed.rs index 47b2f709..ed5a8def 100644 --- a/crates/pptx/tests/chart_embed.rs +++ b/crates/pptx/tests/chart_embed.rs @@ -56,7 +56,7 @@ fn with_workbook(bytes: &[u8]) -> Vec { 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#" -"#; +"#; 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"))