Repository: fermyon/spin
Pull request #908 (2022-11-17 01:08:02)

Fail if template references unknown filter
The template system is intentionally forgiving of parse errors, because a template might contain text that confuses the Liquid parser but which an author wants to be included anyway.  However, this can disguise genuine authoring errors.  This particular one bit me when accidentally using the redirect template with a previous version of Spin - it appeared to succeed but actually failed on a new filter and dumped the unsubstituted template into my project.  With this fix, I need never be confused again.

Longer term, we perhaps need more robust mechanisms for template authors to exclude files from parsing, after which we can banhammer actual parse failures.

Signed-off-by: itowlson <ivan.towlson@fermyon.com>


Patch:
diff --git a/crates/templates/src/manager.rs b/crates/templates/src/manager.rs
index 0308cce3..2de8c3f0 100644
--- a/crates/templates/src/manager.rs
+++ b/crates/templates/src/manager.rs
@@ -928,4 +928,50 @@ mod tests {
         assert!(http_empty.supports_variant(&TemplateVariantInfo::NewApplication));
         assert!(!http_empty.supports_variant(&add_component));
     }
+
+    #[tokio::test]
+    async fn fails_on_unknown_filter() {
+        let temp_dir = tempdir().unwrap();
+        let store = TemplateStore::new(temp_dir.path());
+        let manager = TemplateManager { store };
+        let source = TemplateSource::File(test_data_root());
+
+        manager
+            .install(&source, &InstallOptions::default(), &DiscardingReporter)
+            .await
+            .unwrap();
+
+        let template = manager.get("bad-non-existent-filter").unwrap().unwrap();
+
+        let dest_temp_dir = tempdir().unwrap();
+        let output_dir = dest_temp_dir.path().join("myproj");
+        let values = [("p1".to_owned(), "biscuits".to_owned())]
+            .into_iter()
+            .collect();
+        let options = RunOptions {
+            variant: crate::template::TemplateVariantInfo::NewApplication,
+            output_path: output_dir.clone(),
+            name: "bad-filter-should-fail ".to_owned(),
+            values,
+            accept_defaults: false,
+        };
+
+        let err = template
+            .run(options)
+            .silent()
+            .await
+            .expect_err("Expected template to fail but it passed");
+
+        let err_str = err.to_string();
+
+        assert_contains(&err_str, "internal error");
+        assert_contains(&err_str, "unknown filter 'lol_snort'");
+    }
+
+    fn assert_contains(actual: &str, expected: &str) {
+        assert!(
+            actual.contains(expected),
+            "expected string containing '{expected}' but got '{actual}'"
+        );
+    }
 }
diff --git a/crates/templates/src/renderer.rs b/crates/templates/src/renderer.rs
index d87ef473..45f901d3 100644
--- a/crates/templates/src/renderer.rs
+++ b/crates/templates/src/renderer.rs
@@ -1,4 +1,5 @@
 use anyhow::anyhow;
+use lazy_static::lazy_static;
 use std::{collections::HashMap, path::PathBuf};
 
 use crate::writer::{TemplateOutput, TemplateOutputs};
@@ -68,13 +69,24 @@ impl RenderOperation {
 }
 
 impl TemplateContent {
-    pub(crate) fn infer_from_bytes(raw: Vec<u8>, parser: &liquid::Parser) -> TemplateContent {
+    pub(crate) fn infer_from_bytes(
+        raw: Vec<u8>,
+        parser: &liquid::Parser,
+    ) -> anyhow::Result<TemplateContent> {
         match string_from_bytes(&raw) {
-            None => TemplateContent::Binary(raw),
+            None => Ok(TemplateContent::Binary(raw)),
             Some(s) => {
                 match parser.parse(&s) {
-                    Ok(t) => TemplateContent::Template(t),
-                    Err(_) => TemplateContent::Binary(raw), // TODO: detect legit broken templates and error on them
+                    Ok(t) => Ok(TemplateContent::Template(t)),
+                    Err(e) => match understand_liquid_error(e) {
+                        TemplateParseFailure::Other(_e) => {
+                            // TODO: emit a warning?
+                            Ok(TemplateContent::Binary(raw))
+                        }
+                        TemplateParseFailure::UnknownFilter(id) => {
+                            Err(anyhow!("internal error in template: unknown filter '{id}'"))
+                        }
+                    },
                 }
             }
         }
@@ -99,3 +111,30 @@ fn string_from_bytes(bytes: &[u8]) -> Option<String> {
         Err(_) => None, // TODO: try other encodings!
     }
 }
+
+enum TemplateParseFailure {
+    UnknownFilter(String),
+    Other(liquid::Error),
+}
+
+lazy_static! {
+    static ref UNKNOWN_FILTER: regex::Regex =
+        regex::Regex::new("requested filter=(\\S+)").expect("Invalid unknown filter regex");
+}
+
+fn understand_liquid_error(e: liquid::Error) -> TemplateParseFailure {
+    let err_str = e.to_string();
+
+    // They should use typed errors like we, er, don't
+    match err_str.lines().next() {
+        None => TemplateParseFailure::Other(e),
+        Some("liquid: Unknown filter") => match UNKNOWN_FILTER.captures(&err_str) {
+            None => TemplateParseFailure::Other(e),
+            Some(captures) => match captures.get(1) {
+                None => TemplateParseFailure::Other(e),
+                Some(id) => TemplateParseFailure::UnknownFilter(id.as_str().to_owned()),
+            },
+        },
+        _ => TemplateParseFailure::Other(e),
+    }
+}
diff --git a/crates/templates/src/run.rs b/crates/templates/src/run.rs
index 28d53b37..3c53eb8c 100644
--- a/crates/templates/src/run.rs
+++ b/crates/templates/src/run.rs
@@ -233,7 +233,8 @@ impl Run {
         let abs_snippet_file = snippets_dir.join(snippet_file);
         let file_content = std::fs::read(abs_snippet_file)
             .with_context(|| format!("Error reading snippet file {}", snippet_file))?;
-        let content = TemplateContent::infer_from_bytes(file_content, &self.template_parser());
+        let content = TemplateContent::infer_from_bytes(file_content, &self.template_parser())
+            .with_context(|| format!("Error parsing snippet file {}", snippet_file))?;
 
         match id {
             "component" => {
@@ -278,7 +279,10 @@ impl Run {
         let contents = paths
             .iter()
             .map(std::fs::read)
-            .map(|c| c.map(|cc| TemplateContent::infer_from_bytes(cc, &template_parser)))
+            .map(|c| {
+                c.map_err(|e| e.into())
+                    .and_then(|cc| TemplateContent::infer_from_bytes(cc, &template_parser))
+            })
             .collect::<Result<Vec<_>, _>>()?;
         let pairs = paths.into_iter().zip(contents).collect();
         Ok(pairs)


Test patch:
diff --git a/crates/templates/tests/templates/bad-non-existent-filter/content/test.txt b/crates/templates/tests/templates/bad-non-existent-filter/content/test.txt
new file mode 100644
index 00000000..af069829
--- /dev/null
+++ b/crates/templates/tests/templates/bad-non-existent-filter/content/test.txt
@@ -0,0 +1,1 @@
+p1/lolsnort = {{p1 | lol_snort }}
diff --git a/crates/templates/tests/templates/bad-non-existent-filter/metadata/spin-template.toml b/crates/templates/tests/templates/bad-non-existent-filter/metadata/spin-template.toml
new file mode 100644
index 00000000..e4cc18f0
--- /dev/null
+++ b/crates/templates/tests/templates/bad-non-existent-filter/metadata/spin-template.toml
@@ -0,0 +1,6 @@
+manifest_version = "1"
+id = "bad-non-existent-filter"
+description = "TEST - INTENTIONALLY FAILS - do not use"
+
+[parameters]
+p1 = { type = "string", prompt = "P1" }