XLSX: a damaged Python part is a warning, and the workbook opens with its values

A Python part whose zip entry failed to read (a bad checksum or compressed
stream) made the whole workbook fail to open: the error was passed up, and the
failed read had also ended the package's read budget, so even a warning would
have left the sheets unread. The part is now read with
Package::read_optional_limited, which charges a failed read only that part's
allowance, and every error other than TooLarge is a warning. The Python cells
then keep only their cached values, as when the part is missing. Malformed XML
in the part was already a warning; the test covers it too.

crates/engine/src/tests/python_files.rs

@@ -243,6 +243,55 @@ fn a_py_formula_naming_a_missing_script_keeps_only_its_value() {
     assert_eq!(wb.sheet(0).unwrap().cell(CellRef::parse("A1").unwrap()).unwrap().py.as_ref().map(|p| p.code.as_str()), Some("5"));
 }
 
+/// `bytes` with the part `name` stored uncompressed and one byte of its data changed, so the
+/// archive is intact but reading the part fails its checksum.
+fn with_damaged_part(bytes: &[u8], name: &str, marker: &str) -> Vec<u8> {
+    let mut z = zip::ZipArchive::new(std::io::Cursor::new(bytes)).unwrap();
+    let mut out = zip::ZipWriter::new(std::io::Cursor::new(Vec::new()));
+    for i in 0..z.len() {
+        let mut f = z.by_index(i).unwrap();
+        let entry = f.name().to_string();
+        let mut data = Vec::new();
+        f.read_to_end(&mut data).unwrap();
+        let options = zip::write::SimpleFileOptions::default();
+        let options = if entry == name { options.compression_method(zip::CompressionMethod::Stored) } else { options };
+        out.start_file(entry, options).unwrap();
+        out.write_all(&data).unwrap();
+    }
+    let mut zipped = out.finish().unwrap().into_inner();
+    let at = zipped.windows(marker.len()).position(|w| w == marker.as_bytes()).expect("the marker is stored as it is");
+    zipped[at + 1] ^= 0x20;
+    zipped
+}
+
+#[test]
+fn a_damaged_python_part_is_a_warning_and_the_workbook_opens_with_its_values() {
+    let data = "<sheetData><row r=\"1\"><c r=\"A1\"><f>_xlfn._xlws.PY(0,0)</f><v>5</v></c><c r=\"B1\"><v>9</v></c></row></sheetData>";
+    let good = excel_shaped(data, Some(&["5"]));
+    // The part's data fails its checksum (a damaged archive entry).
+    let damaged = with_damaged_part(&good, "xl/python/python1.xml", "<pythonScripts>");
+    // The part is not well-formed XML, before its first script.
+    let broken = |at: &'static str, with: &'static str| {
+        with_parts(&good, move |name, text| if name == "xl/python/python1.xml" { text.replace(at, with) } else { text.to_string() }, &[])
+    };
+    let malformed = broken("<pythonScripts>", "<pythonScripts><!-- never closed");
+    for (name, bytes) in [("damaged.xlsx", damaged), ("malformed.xlsx", malformed)] {
+        let (wb, warnings) = crate::io::open_bytes(name, &bytes).unwrap_or_else(|e| panic!("{name}: {e}"));
+        let sh = wb.sheet(0).unwrap();
+        let a1 = sh.cell(CellRef::parse("A1").unwrap()).unwrap();
+        assert_eq!(a1.value, Value::Number(5.0), "{name}: the cached value is kept");
+        assert!(a1.py.is_none() && a1.formula.is_none(), "{name}: as for a missing script, only the value");
+        assert_eq!(sh.value(CellRef::parse("B1").unwrap()), Value::Number(9.0), "{name}: the rest of the sheet is read");
+        let part = warnings.iter().filter(|w| w.contains("Python part is damaged")).count();
+        assert_eq!(part, 1, "{name}: {warnings:?}");
+    }
+    // Damaged after its first script, the part keeps what it read before the damage.
+    let (wb, warnings) = crate::io::open_bytes("late.xlsx", &broken("</pythonScripts>", "<!-- never closed")).unwrap();
+    let a1 = wb.sheet(0).unwrap().cell(CellRef::parse("A1").unwrap()).unwrap().clone();
+    assert_eq!(a1.py.as_ref().map(|p| p.code.as_str()), Some("5"));
+    assert_eq!(warnings.iter().filter(|w| w.contains("Python part is damaged")).count(), 1, "{warnings:?}");
+}
+
 #[test]
 fn many_cells_naming_one_long_script_take_from_one_budget() {
     // 20,000 cells name one script of 8,000 characters: 160 MB of code if each took a copy.

crates/xlsx/src/package.rs

@@ -61,6 +61,19 @@ impl<'a> Package<'a> {
         self.read_limited(part, MAX_PART)
     }
 
+    /// Reads a part the reader can do without (the caller warns and goes on when it fails). A read
+    /// that fails partway is charged this part's whole allowance (`maximum`, and the byte that
+    /// detects an overrun) instead of ending the package's: the decompression work stays bounded,
+    /// and the parts read after it still open.
+    pub fn read_optional_limited(&mut self, part: &str, maximum: u64) -> Result<Option<Vec<u8>>, IoError> {
+        let before = self.total;
+        let read = self.read_limited(part, maximum);
+        if let Err(IoError::Zip(_)) = &read {
+            self.total = before.saturating_add(maximum.min(MAX_PART).saturating_add(1)).min(MAX_TOTAL);
+        }
+        read
+    }
+
     /// Reads a small metadata part without allowing it to expand to the general part limit.
     pub fn read_limited(&mut self, part: &str, maximum: u64) -> Result<Option<Vec<u8>>, IoError> {
         let maximum = maximum.min(MAX_PART);
@@ -227,4 +240,27 @@ mod tests {
         // Oversized repeated attempts stay cheap errors now: no allowance is left to pay for.
         assert!(matches!(pkg.read("b.xml"), Err(IoError::TooLarge(_))));
     }
+
+    /// A part the reader can do without, damaged inside an intact archive, costs its own
+    /// allowance: the parts after it are still read.
+    #[test]
+    fn a_damaged_optional_part_costs_only_its_allowance() {
+        use std::io::Write as _;
+        let mut w = zip::ZipWriter::new(std::io::Cursor::new(Vec::new()));
+        let stored = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored);
+        w.start_file("a.xml", stored).unwrap();
+        w.write_all(b"<a>damaged</a>").unwrap();
+        w.start_file("b.xml", zip::write::SimpleFileOptions::default()).unwrap();
+        w.write_all(b"hello").unwrap();
+        let mut z = w.finish().unwrap().into_inner();
+        let at = z.windows(7).position(|x| x == b"damaged").unwrap();
+        z[at] ^= 0x20;
+        let mut pkg = Package::open(&z).unwrap();
+        assert!(matches!(pkg.read_optional_limited("a.xml", 1024), Err(IoError::Zip(_))));
+        assert_eq!(pkg.total, 1025, "the failed read is charged its allowance");
+        assert_eq!(pkg.read("b.xml").unwrap().as_deref(), Some(&b"hello"[..]));
+        // An ordinary read of it still ends the package's budget.
+        assert!(pkg.read("a.xml").is_err());
+        assert_eq!(pkg.total, MAX_TOTAL);
+    }
 }

crates/xlsx/src/python_part.rs

@@ -71,7 +71,7 @@ impl ScriptsIn {
 pub fn read(cx: &mut Ctx<'_>, rels: &[crate::package::Rel]) -> Result<ScriptsIn, IoError> {
     let mut out = ScriptsIn { budget: MAX_CODE_BYTES, ..ScriptsIn::default() };
     let Some(rel) = rels.iter().find(|r| !r.external && r.kind.eq_ignore_ascii_case(REL_KIND)) else { return Ok(out) };
-    let bytes = match cx.pkg.read_limited(&rel.target, MAX_PART_BYTES) {
+    let bytes = match cx.pkg.read_optional_limited(&rel.target, MAX_PART_BYTES) {
         Ok(Some(b)) => b,
         Ok(None) => {
             cx.warn(format!("missing part {}", rel.target));
@@ -81,7 +81,12 @@ pub fn read(cx: &mut Ctx<'_>, rels: &[crate::package::Rel]) -> Result<ScriptsIn,
             cx.warn(format!("the Python part is too large ({m}); Python cells kept only their values"));
             return Ok(out);
         }
-        Err(e) => return Err(e),
+        // A damaged entry (a bad checksum or compressed stream) is a damaged part: its cells keep
+        // their values, as when the part is missing.
+        Err(e) => {
+            cx.warn(format!("the Python part is damaged ({e}); Python cells kept only their values"));
+            return Ok(out);
+        }
     };
     let mut over = 0usize;
     let parsed = xml::parse_streaming(&bytes, &["pythonScript"], &mut |el: El| {