WIP - Abort reference resolution when a reference loop is detected

src/node/mod.rs

@@ -1,5 +1,6 @@
 use anyhow::{anyhow, Context, Result};
 use serde::Deserialize;
+use std::collections::HashSet;
 use std::path::PathBuf;
 // TODO(sg): Switch to serde_yaml's `apply_merge()` once it supports recursive merges, cf.
 // https://github.com/dtolnay/serde-yaml/issues/362
@@ -216,7 +217,10 @@ impl Node {
                 if let Some(clstoken) = clstoken {
                     // If we got a token, render it, and convert it into a string with
                     // `raw_string()` to ensure no spurious quotes are injected.
-                    clstoken.render(&root.parameters)?.raw_string()?
+                    let mut seen_paths = HashSet::new();
+                    clstoken
+                        .render(&root.parameters, &mut seen_paths)?
+                        .raw_string()?
                 } else {
                     // If Token::parse() returns None, the class name can't contain any references,
                     // just convert cls into an owned String.

src/refs/mod.rs

@@ -3,6 +3,7 @@ mod parser;
 use crate::types::{Mapping, Value};
 use anyhow::{anyhow, Result};
 use nom::error::{convert_error, VerboseError};
+use std::collections::HashSet;
 
 #[derive(Debug, PartialEq, Eq)]
 /// Represents a parsed Reclass reference
@@ -48,25 +49,28 @@ impl Token {
     /// the Mapping provided through parameter `params`.
     ///
     /// The heavy lifting is done by `Token::resolve()`.
-    pub fn render(&self, params: &Mapping) -> Result<Value> {
+    pub fn render(&self, params: &Mapping, seen_paths: &mut HashSet<String>) -> Result<Value> {
         if self.is_ref() {
             // handle value refs (i.e. refs where the full value of the key is replaced)
             // We call `interpolate()` after `resolve()` to ensure that we fully interpolate all
             // references if the result of `resolve()` is a complex Value (Mapping or Sequence).
-            self.resolve(params)?.interpolate(params)
+            self.resolve(params, seen_paths)?
+                .interpolate(params, seen_paths)
         } else {
-            Ok(Value::Literal(self.resolve(params)?.raw_string()?))
+            Ok(Value::Literal(
+                self.resolve(params, seen_paths)?.raw_string()?,
+            ))
         }
     }
 
     /// Resolves the Token into a [`Value`]. References are looked up in the provided `params`
     /// Mapping.
-    fn resolve(&self, params: &Mapping) -> Result<Value> {
+    fn resolve(&self, params: &Mapping, seen_paths: &mut HashSet<String>) -> Result<Value> {
         match self {
             // Literal tokens can be directly turned into `Value::Literal`
             Self::Literal(s) => Ok(Value::Literal(s.to_string())),
             Self::Combined(tokens) => {
-                let res = interpolate_token_slice(tokens, params)?;
+                let res = interpolate_token_slice(tokens, params, seen_paths)?;
                 // The result of `interpolate_token_slice()` for a `Token::Combined()` can't result
                 // in more unresolved refs since we iterate over each segment until there's no
                 // Value::String() left, so we return a Value::Literal().
@@ -78,7 +82,21 @@ impl Token {
             Self::Ref(parts) => {
                 // Construct flattened ref path by resolving any potential nested references in the
                 // Ref's Vec<Token>.
-                let path = interpolate_token_slice(parts, params)?;
+                let path = interpolate_token_slice(parts, params, seen_paths)?;
+
+                if seen_paths.contains(&path) {
+                    // we've already seen this exact reference -> abort resolution because there's
+                    // a loop. Generate a nice error message showing all the ref paths that form
+                    // the loop.
+                    let mut paths = seen_paths
+                        .iter()
+                        .map(|p| format!("\"{p}\""))
+                        .collect::<Vec<String>>();
+                    paths.sort();
+                    let paths = paths.join(", ");
+                    return Err(anyhow!("Reference loop with reference paths [{paths}]"));
+                }
+                seen_paths.insert(path.clone());
 
                 // generate iterator containing flattened reference path segments
                 let mut refpath_iter = path.split(':');
@@ -122,16 +140,17 @@ impl Token {
                         // individual references are resolved, and always do value lookups on
                         // resolved references.
                         Value::String(_) => {
-                            newv = v.interpolate(params)?;
+                            newv = v.interpolate(params, seen_paths)?;
                             v = newv.get(&key.into()).ok_or_else(|| {
                                 anyhow!("unable to lookup key '{key}' for '{path}'")
                             })?;
                         }
                         Value::ValueList(l) => {
                             let mut i = vec![];
                             for v in l {
+                                let mut seen = seen_paths.clone();
                                 let v = if v.is_string() {
-                                    v.interpolate(params)?
+                                    v.interpolate(params, &mut seen)?
                                 } else {
                                     v.clone()
                                 };
@@ -161,7 +180,8 @@ impl Token {
                 // `Value::ValueList`. This ensures that the returned Value will never contain
                 // further references.
                 while v.is_string() || v.is_value_list() {
-                    v = v.interpolate(params)?;
+                    let mut seen = seen_paths.clone();
+                    v = v.interpolate(params, &mut seen)?;
                 }
                 Ok(v)
             }
@@ -195,15 +215,23 @@ impl std::fmt::Display for Token {
 
 /// Interpolate a `Vec<Token>`. Called from `Token::resolve()` for `Token::Combined` and
 /// `Token::Ref` Vecs.
-fn interpolate_token_slice(tokens: &[Token], params: &Mapping) -> Result<String> {
+fn interpolate_token_slice(
+    tokens: &[Token],
+    params: &Mapping,
+    seen_paths: &mut HashSet<String>,
+) -> Result<String> {
     // Iterate through each element of the Vec, and call Token::resolve() on each element.
     // Additionally, we repeatedly call `Value::interpolate()` on the resolved value for each
     // element, as long as that Value is a `Value::String`.
     let mut res = String::new();
     for t in tokens {
-        let mut v = t.resolve(params)?;
+        // Multiple separate refs in a combined or ref token can't form loops between each other.
+        // Each individual ref can still be part of a loop, so we copy the input `seen_paths` while
+        // resolving each element.
+        let mut seen = seen_paths.clone();
+        let mut v = t.resolve(params, &mut seen)?;
         while v.is_string() {
-            v = v.interpolate(params)?;
+            v = v.interpolate(params, &mut seen)?;
         }
         res.push_str(&v.raw_string()?);
     }

src/refs/token_resolve_parse_tests.rs

@@ -6,7 +6,8 @@ fn test_resolve_ref_str() {
     let token = Token::Ref(vec![Token::literal_from_str("foo")]);
     let params = Mapping::from_str("foo: bar").unwrap();
 
-    let v = token.resolve(&params).unwrap();
+    let mut seen = HashSet::new();
+    let v = token.resolve(&params, &mut seen).unwrap();
     assert_eq!(v, Value::Literal("bar".into()));
 }
 
@@ -15,7 +16,8 @@ fn test_resolve_ref_val() {
     let token = Token::Ref(vec![Token::literal_from_str("foo")]);
     let params = Mapping::from_str("foo: True").unwrap();
 
-    let v = token.resolve(&params).unwrap();
+    let mut seen = HashSet::new();
+    let v = token.resolve(&params, &mut seen).unwrap();
     assert_eq!(v, Value::Bool(true));
 }
 
@@ -24,7 +26,8 @@ fn test_resolve_literal() {
     let token = Token::literal_from_str("foo");
     let params = Mapping::new();
 
-    let v = token.resolve(&params).unwrap();
+    let mut seen = HashSet::new();
+    let v = token.resolve(&params, &mut seen).unwrap();
     assert_eq!(v, Value::Literal("foo".into()));
 }
 
@@ -36,7 +39,8 @@ fn test_resolve_combined() {
     ]);
     let params = Mapping::from_str("{foo: bar, bar: baz}").unwrap();
 
-    let v = token.resolve(&params).unwrap();
+    let mut seen = HashSet::new();
+    let v = token.resolve(&params, &mut seen).unwrap();
     assert_eq!(v, Value::Literal("foobar".into()));
 }
 #[test]
@@ -48,7 +52,8 @@ fn test_resolve_combined_2() {
     ]);
     let params = Mapping::from_str(r#"{foo: "${bar}", bar: baz}"#).unwrap();
 
-    let v = token.resolve(&params).unwrap();
+    let mut seen = HashSet::new();
+    let v = token.resolve(&params, &mut seen).unwrap();
     assert_eq!(v, Value::Literal("foobaz".into()));
 }
 
@@ -64,7 +69,8 @@ fn test_resolve_combined_3() {
     "#;
     let params = Mapping::from_str(params).unwrap();
 
-    let v = token.resolve(&params).unwrap();
+    let mut seen = HashSet::new();
+    let v = token.resolve(&params, &mut seen).unwrap();
     assert_eq!(v, Value::Literal("foo${bar}".into()));
 }
 
@@ -100,28 +106,24 @@ fn test_token_parse_embedded_ref() {
     );
 }
 
-#[test]
-fn test_resolve() {
-    let p = Mapping::from_str("foo: foo").unwrap();
-    let reftoken = parse_ref(&"${foo}").unwrap();
-
-    assert_eq!(reftoken.resolve(&p).unwrap(), Value::Literal("foo".into()));
-}
-
 #[test]
 fn test_resolve_subkey() {
     let p = Mapping::from_str("foo: {foo: foo}").unwrap();
     let reftoken = parse_ref(&"${foo:foo}").unwrap();
 
-    assert_eq!(reftoken.resolve(&p).unwrap(), Value::Literal("foo".into()));
+    let mut seen = HashSet::new();
+    let v = reftoken.resolve(&p, &mut seen).unwrap();
+    assert_eq!(v, Value::Literal("foo".into()));
 }
 
 #[test]
 fn test_resolve_nested() {
     let p = Mapping::from_str("{foo: foo, bar: {foo: foo}}").unwrap();
     let reftoken = parse_ref(&"${bar:${foo}}").unwrap();
 
-    assert_eq!(reftoken.resolve(&p).unwrap(), Value::Literal("foo".into()));
+    let mut seen = HashSet::new();
+    let v = reftoken.resolve(&p, &mut seen).unwrap();
+    assert_eq!(v, Value::Literal("foo".into()));
 }
 
 #[test]
@@ -135,10 +137,9 @@ fn test_resolve_nested_subkey() {
 
     // ${bar:${foo:bar}} == ${bar:foo} == foo
     let reftoken = parse_ref(&"${bar:${foo:bar}}").unwrap();
-    assert_eq!(
-        reftoken.resolve(&p).unwrap(),
-        Value::Literal("foo".to_string())
-    );
+    let mut seen = HashSet::new();
+    let v = reftoken.resolve(&p, &mut seen).unwrap();
+    assert_eq!(v, Value::Literal("foo".to_string()));
 }
 
 #[test]
@@ -152,10 +153,9 @@ fn test_resolve_kapitan_secret_ref() {
 
     let reftoken = parse_ref(&"?{vaultkv:foo/bar/${baz:baz}/qux}").unwrap();
     dbg!(&reftoken);
-    assert_eq!(
-        reftoken.resolve(&p).unwrap(),
-        Value::Literal("?{vaultkv:foo/bar/baz/qux}".to_string())
-    );
+    let mut seen = HashSet::new();
+    let v = reftoken.resolve(&p, &mut seen).unwrap();
+    assert_eq!(v, Value::Literal("?{vaultkv:foo/bar/baz/qux}".to_string()));
 }
 
 #[test]
@@ -168,10 +168,9 @@ fn test_resolve_escaped_ref() {
     let p = Mapping::from_str(params).unwrap();
 
     let reftoken = parse_ref("\\${PROJECT_LABEL}").unwrap();
-    assert_eq!(
-        reftoken.resolve(&p).unwrap(),
-        Value::Literal("${PROJECT_LABEL}".to_string())
-    );
+    let mut seen = HashSet::new();
+    let v = reftoken.resolve(&p, &mut seen).unwrap();
+    assert_eq!(v, Value::Literal("${PROJECT_LABEL}".to_string()));
 }
 
 #[test]
@@ -183,8 +182,10 @@ fn test_resolve_mapping_value() {
     "#;
     let p = Mapping::from_str(p).unwrap();
     let reftoken = parse_ref("${foo}").unwrap();
+    let mut seen = HashSet::new();
+    let v = reftoken.resolve(&p, &mut seen).unwrap();
     assert_eq!(
-        reftoken.resolve(&p).unwrap(),
+        v,
         Value::Mapping(Mapping::from_str("{bar: bar, baz: baz}").unwrap())
     );
 }
@@ -198,10 +199,54 @@ fn test_resolve_mapping_embedded() {
     "#;
     let p = Mapping::from_str(p).unwrap();
     let reftoken = parse_ref("foo: ${foo}").unwrap();
+    let mut seen = HashSet::new();
+    let v = reftoken.resolve(&p, &mut seen).unwrap();
     assert_eq!(
-        reftoken.resolve(&p).unwrap(),
+        v,
         // Mapping is serialized as JSON when embedded in a string. serde_json emits JSON maps
         // with lexically sorted keys and minimal whitespace.
         Value::Literal(r#"foo: {"bar":"bar","baz":"baz"}"#.to_string())
     );
 }
+
+#[test]
+#[should_panic(expected = "Reference loop with reference paths [\"foo\"]")]
+fn test_resolve_recursive_error() {
+    let p = r#"
+    foo: ${foo}
+    "#;
+    let p = Mapping::from_str(p).unwrap();
+    let reftoken = parse_ref("${foo}").unwrap();
+
+    let mut seen = HashSet::new();
+    let _v = reftoken.resolve(&p, &mut seen).unwrap();
+}
+
+#[test]
+#[should_panic(expected = "Reference loop with reference paths [\"bar\", \"foo\"]")]
+fn test_resolve_recursive_error_2() {
+    let p = r#"
+    foo: ${bar}
+    bar: ${foo}
+    "#;
+    let p = Mapping::from_str(p).unwrap();
+    let reftoken = parse_ref("${foo}").unwrap();
+
+    let mut seen = HashSet::new();
+    let _v = reftoken.resolve(&p, &mut seen).unwrap();
+}
+
+#[test]
+#[should_panic(expected = "Reference loop with reference paths [\"baz\", \"foo\"]")]
+fn test_resolve_nested_recursive_error() {
+    let p = r#"
+    foo: ${baz}
+    baz:
+      qux: ${foo}
+    "#;
+    let p = Mapping::from_str(p).unwrap();
+    let reftoken = parse_ref("${foo}").unwrap();
+
+    let mut seen = HashSet::new();
+    let _v = reftoken.resolve(&p, &mut seen).unwrap();
+}

src/types/mapping.rs

@@ -356,10 +356,17 @@ impl Mapping {
     /// The method looks up reference values in parameter `root`. After interpolation of each
     /// Mapping key-value pair, the resulting value is flattened before it's inserted in the new
     /// Mapping. Mapping keys are inserted into the new mapping unchanged.
-    pub(super) fn interpolate(&self, root: &Self) -> Result<Self> {
+    pub(super) fn interpolate(
+        &self,
+        root: &Self,
+        seen_paths: &mut HashSet<String>,
+    ) -> Result<Self> {
         let mut res = Self::new();
         for (k, v) in self {
-            let mut v = v.interpolate(root)?;
+            // Resolve each mapping value with a clone of the already seen paths. Refs in separate
+            // mapping values can't form new loops with each other.
+            let mut seen = seen_paths.clone();
+            let mut v = v.interpolate(root, &mut seen)?;
             v.flatten()?;
             res.insert(k.clone(), v)?;
         }