From 940fc1364ab495e5001148d722b17b430b2f6a0e Mon Sep 17 00:00:00 2001
From: Jonathan Lebon <jonathan@jlebon.com>
Date: Fri, 26 Oct 2018 13:58:46 -0400
Subject: [PATCH] compose: Check that add-files are compatible after parsing

While serde gives us type checking, it of course doesn't understand
semantics beyond that. One example is checking the compatibility of
`add-files` entries with the OSTree model. This is something we can do
upfront early on to avoid surprises for users.

Also tweak the docs to reflect this new check.

Related: #1642

Closes: #1643
Approved by: cgwalters
---
 rust/src/treefile.rs                    | 29 +++++++++++++++++++++++++
 tests/compose-tests/test-misc-tweaks.sh | 13 +++++++++++
 2 files changed, 42 insertions(+)

diff --git a/rust/src/treefile.rs b/rust/src/treefile.rs
index aeccbb8b..71937b4f 100644
--- a/rust/src/treefile.rs
+++ b/rust/src/treefile.rs
@@ -302,6 +302,18 @@ fn treefile_parse_recurse<P: AsRef<Path>>(
     Ok(parsed)
 }
 
+// Similar to the importer check but just checks for prefixes since
+// they're files, and also allows /etc since it's before conversion
+fn add_files_path_is_valid(path: &str) -> bool {
+    let path = path.trim_left_matches("/");
+    (path.starts_with("usr/") && !path.starts_with("usr/local/"))
+        || path.starts_with("etc/")
+        || path.starts_with("bin/")
+        || path.starts_with("sbin/")
+        || path.starts_with("lib/")
+        || path.starts_with("lib64/")
+}
+
 impl Treefile {
     /// The main treefile creation entrypoint.
     pub fn new_boxed(
@@ -310,6 +322,7 @@ impl Treefile {
         workdir: openat::Dir,
     ) -> io::Result<Box<Treefile>> {
         let parsed = treefile_parse_recurse(filename, arch, 0)?;
+        Treefile::validate_config(&parsed.config)?;
         let dfd = openat::Dir::open(filename.parent().unwrap())?;
         let rojig_spec = if let &Some(ref rojig) = &parsed.config.rojig {
             Some(Treefile::write_rojig_spec(&workdir, rojig)?)
@@ -327,6 +340,22 @@ impl Treefile {
         }))
     }
 
+    /// Do some upfront semantic checks we can do beyond just the type safety serde provides.
+    fn validate_config(config: &TreeComposeConfig) -> io::Result<()> {
+        // check add-files
+        if let Some(files) = &config.add_files {
+            for (_, dest) in files.iter() {
+                if !add_files_path_is_valid(&dest) {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidInput,
+                        format!("Unsupported path in add-files: {}", dest),
+                    ));
+                }
+            }
+        }
+        Ok(())
+    }
+
     fn serialize_json_string(config: &TreeComposeConfig) -> io::Result<CUtf8Buf> {
         let mut output = vec![];
         serde_json::to_writer_pretty(&mut output, config)?;
diff --git a/tests/compose-tests/test-misc-tweaks.sh b/tests/compose-tests/test-misc-tweaks.sh
index 3c2e6ff5..273c238f 100755
--- a/tests/compose-tests/test-misc-tweaks.sh
+++ b/tests/compose-tests/test-misc-tweaks.sh
@@ -112,3 +112,16 @@ ostree --repo=${repobuild} show ${treeref} \
 assert_file_has_content_literal pkglist.txt 'systemd-'
 assert_not_file_has_content pkglist.txt 'systemd-bootchart'
 echo "ok recommends"
+
+# Check that add-files with bad paths are rejected
+prepare_compose_test "add-files-failure"
+pysetjsonmember "add-files" '[["foo.txt", "/var/lib/foo.txt"]]'
+
+# Do the compose ourselves since set -e doesn't work in function calls in if
+rm ${compose_workdir} -rf
+mkdir ${test_tmpdir}/workdir
+if rpm-ostree compose tree ${compose_base_argv} ${treefile} |& tee err.txt; then
+    assert_not_reached err.txt "Successfully composed with add-files for /var/lib?"
+fi
+assert_file_has_content_literal err.txt "Unsupported path in add-files: /var"
+echo "ok bad add-files"