craft-odf: Manifest::parse_strict and Package::manifest_strict (#972)

A strict read of the manifest for a reader that refuses a damaged package
instead of opening what it can, as GridCraft's ODS reader does today
(crates/xlsx/src/ods.rs check_manifest): malformed or unbalanced XML, any
document type declaration, undefined entities, content outside the root,
a root that is not manifest:manifest by namespace, a file entry without a
full-path, or more entries than the reader keeps are errors
(Error::Manifest).

crates/odf/README.md

@@ -7,7 +7,7 @@ this crate reads and writes that container and nothing inside it.
 | Module | What it does |
 | --- | --- |
 | `Package`, `PackageWriter` | The ZIP container: the `mimetype` file, the manifest, the files, within `Limits`. |
-| `Manifest` | `META-INF/manifest.xml`: each file's media type, and how an encrypted file is encrypted. Read by namespace, whatever prefix the file uses. |
+| `Manifest` | `META-INF/manifest.xml`: each file's media type, and how an encrypted file is encrypted. Read by namespace, whatever prefix the file uses; `parse_strict` / `Package::manifest_strict` refuse a damaged one instead of reading what they can. |
 | `crypt` | Password-protected packages: AES-256-CBC, SHA-256 start key, PBKDF2 (HMAC-SHA-1), SHA-256 or SHA-1 checksums. A package in, a package out. |
 | `flat` | Flat single-file documents (`.fodt`, `.fods`, `.fodp`, `.fodg`): one `office:document` taken apart into a package's files, and a package put together into one. |
 | `zip_guard` | Refuses a ZIP whose end record declares too many entries, before `zip` reserves room for them. |

crates/odf/src/lib.rs

@@ -76,6 +76,10 @@ pub enum Error {
     /// A flat document that is no `office:document`, or not UTF-8.
     #[error("not a flat OpenDocument file ({0})")]
     Flat(&'static str),
+    /// A manifest that [`Manifest::parse_strict`] refuses: malformed XML, a document type
+    /// declaration, a root that is no `manifest:manifest`, or more entries than it reads.
+    #[error("the package's manifest is not valid ({0})")]
+    Manifest(String),
 }
 
 impl Error {

crates/odf/src/manifest.rs

@@ -5,7 +5,9 @@ use std::fmt::Write as _;
 
 use base64::Engine as _;
 use base64::engine::general_purpose::STANDARD;
-use craft_xml::{Canonical, El, Options, esc_attr};
+use craft_xml::{Canonical, Doctype, El, Options, esc_attr};
+
+use crate::Error;
 
 /// The manifest's path in a package.
 pub const MANIFEST_PATH: &str = "META-INF/manifest.xml";
@@ -76,6 +78,29 @@ impl Manifest {
         Manifest { version: attr(&root, "version").map(str::to_string), entries }
     }
 
+    /// Reads a manifest, refusing one a lenient read would repair: malformed or unbalanced
+    /// XML, a document type declaration (of any kind), an undefined entity, text or a second
+    /// element outside the root, a root that is not `manifest` in the manifest namespace, a
+    /// `file-entry` without a `full-path`, or more than [`MAX_ENTRIES`] entries. For a reader
+    /// that refuses a damaged package rather than open what it can.
+    pub fn parse_strict(xml: &[u8]) -> Result<Manifest, Error> {
+        let bad = |why: String| Error::Manifest(why);
+        let canonical = Canonical::new(&[("manifest", MANIFEST_NS)]);
+        let opts = Options { doctype: Doctype::Reject, ..Options::canonical(&canonical).strict() };
+        let root = opts.parse(xml).map_err(|e| bad(e.to_string()))?;
+        if root.name != "manifest:manifest" {
+            return Err(bad(format!("the root element is {}, not manifest:manifest", root.name)));
+        }
+        let mut entries = Vec::new();
+        for e in root.elements().filter(|e| e.name == "manifest:file-entry") {
+            if entries.len() == MAX_ENTRIES {
+                return Err(bad(format!("more than {MAX_ENTRIES} file entries")));
+            }
+            entries.push(file_entry(e).ok_or_else(|| bad("a file entry has no full-path".into()))?);
+        }
+        Ok(Manifest { version: attr(&root, "version").map(str::to_string), entries })
+    }
+
     /// The entry for `path`.
     pub fn entry(&self, path: &str) -> Option<&FileEntry> {
         self.entries.iter().find(|e| e.path == path)

crates/odf/src/package.rs

@@ -135,6 +135,13 @@ impl<'a> Package<'a> {
         self.read(MANIFEST_PATH).ok().flatten().map(|xml| Manifest::parse(&xml)).unwrap_or_default()
     }
 
+    /// The package's manifest read with [`Manifest::parse_strict`]: an error when the package
+    /// has none, or one a strict read refuses, or it is over a limit.
+    pub fn manifest_strict(&mut self) -> Result<Manifest, Error> {
+        let xml = self.read(MANIFEST_PATH)?.ok_or_else(|| Error::Manifest("the package has no manifest".into()))?;
+        Manifest::parse_strict(&xml)
+    }
+
     /// Does the manifest say any file is encrypted ([`crate::crypt`])?
     pub fn is_encrypted(&mut self) -> bool {
         self.manifest().is_encrypted()

crates/odf/src/tests.rs

@@ -68,6 +68,48 @@ fn a_manifest_is_read_by_namespace_and_survives_damage() {
     assert_eq!(Manifest::parse(many.as_bytes()).entries.len(), crate::manifest::MAX_ENTRIES);
 }
 
+#[test]
+fn a_strict_manifest_refuses_what_a_lenient_one_repairs() {
+    const NS: &str = r#"xmlns:manifest="urn:oasis:names:tc:opendocument:xmlns:manifest:1.0""#;
+    let good = format!(
+        r#"<?xml version="1.0"?><manifest:manifest {NS} manifest:version="1.3"><manifest:file-entry manifest:full-path="/" manifest:media-type="a/b"/></manifest:manifest>"#
+    );
+    let m = Manifest::parse_strict(good.as_bytes()).unwrap();
+    assert_eq!((m.version.as_deref(), m.media_type("/")), (Some("1.3"), Some("a/b")));
+    // Another prefix for the same namespace is the same manifest.
+    let other = r#"<m:manifest xmlns:m="urn:oasis:names:tc:opendocument:xmlns:manifest:1.0" m:version="1.3"><m:file-entry m:full-path="/" m:media-type="a/b"/></m:manifest>"#;
+    assert_eq!(Manifest::parse_strict(other.as_bytes()), Ok(m.clone()));
+    for bad in [
+        format!(r#"<manifest:manifest {NS}><manifest:file-entry manifest:full-path="/">"#),
+        format!(r#"<!DOCTYPE manifest:manifest><manifest:manifest {NS}/>"#),
+        format!(r#"<!DOCTYPE m [<!ENTITY a "b">]><manifest:manifest {NS}/>"#),
+        format!(r#"<manifest:manifest {NS}/><manifest:manifest {NS}/>"#),
+        format!(r#"<manifest:manifest {NS}/>trailing"#),
+        format!(r#"<manifest:manifest {NS}><manifest:file-entry manifest:full-path="&bogus;"/></manifest:manifest>"#),
+        format!(r#"<manifest:manifest {NS}><manifest:file-entry manifest:media-type="a/b"/></manifest:manifest>"#),
+        format!(r#"<manifest:manifest {NS}></manifest:file-entry></manifest:manifest>"#),
+        r#"<manifest:manifest><manifest:file-entry manifest:full-path="/"/></manifest:manifest>"#.to_string(),
+        r#"<html/>"#.to_string(),
+        String::new(),
+    ] {
+        assert!(matches!(Manifest::parse_strict(bad.as_bytes()), Err(Error::Manifest(_))), "{bad}");
+        let _ = Manifest::parse(bad.as_bytes());
+    }
+    // The lenient read takes what it can of the first.
+    assert_eq!(Manifest::parse(format!(r#"<manifest:manifest {NS}><manifest:file-entry manifest:full-path="/">"#).as_bytes()).entries.len(), 1);
+    // Entries: up to the cap, not past it.
+    let many = |n: usize| format!(r#"<manifest:manifest {NS}>{}</manifest:manifest>"#, r#"<manifest:file-entry manifest:full-path="a"/>"#.repeat(n));
+    assert_eq!(Manifest::parse_strict(many(crate::manifest::MAX_ENTRIES).as_bytes()).map(|m| m.entries.len()), Ok(crate::manifest::MAX_ENTRIES));
+    assert!(Manifest::parse_strict(many(crate::manifest::MAX_ENTRIES + 1).as_bytes()).is_err());
+    // Through a package: missing and present.
+    let mut p_bytes = zip_of(&[("mimetype", mime::SPREADSHEET.as_bytes())]);
+    assert!(matches!(Package::open(&p_bytes, &Limits::DEFAULT).unwrap().manifest_strict(), Err(Error::Manifest(_))));
+    p_bytes = zip_of(&[("mimetype", mime::SPREADSHEET.as_bytes()), ("META-INF/manifest.xml", good.as_bytes())]);
+    assert_eq!(Package::open(&p_bytes, &Limits::DEFAULT).unwrap().manifest_strict(), Ok(m));
+    // A written package passes.
+    assert!(Package::open(&secret(), &Limits::DEFAULT).unwrap().manifest_strict().is_ok_and(|m| m.entries.len() == 4));
+}
+
 #[test]
 fn a_package_is_held_to_its_limits() {
     let bytes = zip_of(&[("mimetype", mime::SPREADSHEET.as_bytes()), ("a.xml", &[b'a'; 100]), ("b.xml", &[b'b'; 100])]);