Merge pull request #3224 from spinframework/validate-allowed-hosts-default-vars

Validate allowed_outbound_hosts template with defaults

Author: lann

Cargo.lock

@@ -150,7 +150,7 @@ impl Resolver {
     }
 
     /// Resolves the given template.
-    fn resolve_template(&self, template: &Template) -> Result<String> {
+    pub fn resolve_template(&self, template: &Template) -> Result<String> {
         let mut resolved_parts: Vec<Cow<str>> = Vec::with_capacity(template.parts().len());
         for part in template.parts() {
             resolved_parts.push(match part {

crates/expressions/src/lib.rs

@@ -16,6 +16,7 @@ serde = { workspace = true }
 serde_json = { workspace = true }
 sha2 = { workspace = true }
 spin-common = { path = "../common" }
+spin-expressions = { path = "../expressions" }
 spin-locked-app = { path = "../locked-app" }
 spin-manifest = { path = "../manifest" }
 spin-outbound-networking-config = { path = "../outbound-networking-config" }

crates/loader/Cargo.toml

@@ -4,6 +4,7 @@ use anyhow::{anyhow, bail, ensure, Context, Result};
 use futures::{future::try_join_all, StreamExt};
 use reqwest::Url;
 use spin_common::{paths::parent_dir, sloth, ui::quoted_path};
+use spin_expressions::Resolver;
 use spin_locked_app::{
     locked::{
         self, ContentPath, ContentRef, LockedApp, LockedComponent, LockedComponentDependency,
@@ -87,7 +88,8 @@ impl LocalLoader {
         let variables = variables
             .into_iter()
             .map(|(name, v)| Ok((name.to_string(), locked_variable(v)?)))
-            .collect::<Result<_>>()?;
+            .collect::<Result<BTreeMap<_, _>>>()?;
+        let resolver = Resolver::new(variables.clone())?;
 
         let triggers = triggers
             .into_iter()
@@ -102,10 +104,13 @@ impl LocalLoader {
         let sloth_guard = warn_if_component_load_slothful();
 
         // Load all components concurrently
-        let components = try_join_all(components.into_iter().map(|(id, c)| async move {
-            self.load_component(&id, c)
-                .await
-                .with_context(|| format!("Failed to load component `{id}`"))
+        let components = try_join_all(components.into_iter().map(|(id, c)| {
+            let resolver = &resolver;
+            async move {
+                self.load_component(&id, c, resolver)
+                    .await
+                    .with_context(|| format!("Failed to load component `{id}`"))
+            }
         }))
         .await?;
 
@@ -138,11 +143,12 @@ impl LocalLoader {
         &self,
         id: &KebabId,
         component: v2::Component,
+        resolver: &Resolver,
     ) -> Result<LockedComponent> {
         let allowed_outbound_hosts = component
             .normalized_allowed_outbound_hosts()
             .context("`allowed_http_hosts` is malformed")?;
-        AllowedHostsConfig::validate(&allowed_outbound_hosts)
+        AllowedHostsConfig::validate(&allowed_outbound_hosts, resolver)
             .context("`allowed_outbound_hosts` is malformed")?;
 
         let component_requires_service_chaining = requires_service_chaining(&component);
@@ -872,8 +878,7 @@ mod test {
         let err_ctx = format!("{err:#}");
         assert!(
             err_ctx.contains(r#""/" is not a valid destination file name"#),
-            "expected error to show destination file name but got {}",
-            err_ctx
+            "expected error to show destination file name but got {err_ctx}",
         );
         Ok(())
     }

crates/loader/src/local.rs

@@ -0,0 +1,8 @@
+Failed to load Spin app from "<test-dir>/invalid-variable-default-in-allowed-hosts.toml"
+
+Caused by:
+    0: Failed to load component `test`
+    1: `allowed_outbound_hosts` is malformed
+    2: using default variable value(s) with template "https://{{ invalid_default_host }}" results in invalid config "https://invalid host"
+    3: Invalid allowed host "invalid host"
+    4: invalid international domain name

crates/loader/tests/ui/invalid-variable-default-in-allowed-hosts.err

@@ -0,0 +1,14 @@
+spin_manifest_version = 2
+
+[application]
+name = "invalid-app"
+
+[variables]
+invalid_default_host = { default = "invalid host" }
+
+[[trigger.fake]]
+component = "test"
+
+[component.test]
+source = "dummy.wasm"
+allowed_outbound_hosts = ["https://{{ invalid_default_host }}"]

crates/loader/tests/ui/invalid-variable-default-in-allowed-hosts.toml

@@ -3,6 +3,7 @@ use std::sync::Arc;
 
 use anyhow::{bail, ensure, Context as _};
 use futures_util::future::{BoxFuture, Shared};
+use spin_expressions::Resolver;
 use url::Host;
 
 /// The domain used for service chaining.
@@ -433,6 +434,22 @@ impl PartialAllowedHostConfig {
             Self::Unresolved(t) => AllowedHostConfig::parse(resolver.resolve_template(&t)?),
         }
     }
+
+    /// Validates this config. Only templates that can be resolved with default
+    /// values from the given resolver will be fully validated.
+    fn validate(&self, resolver: &Resolver) -> anyhow::Result<()> {
+        if let Self::Unresolved(template) = self {
+            let Ok(resolved) = resolver.resolve_template(template) else {
+                // We're missing a default value so we can't validate further
+                return Ok(());
+            };
+            AllowedHostConfig::parse(&resolved).with_context(|| {
+                let template_str = template.to_string();
+                format!("using default variable value(s) with template {template_str:?} results in invalid config {resolved:?}")
+            })?;
+        }
+        Ok(())
+    }
 }
 
 /// Represents an allowed_outbound_hosts config.
@@ -457,10 +474,11 @@ impl AllowedHostsConfig {
         Ok(Self::SpecificHosts(allowed))
     }
 
-    /// Validate the given allowed_outbound_hosts values. Templated values are
-    /// only validated against template syntax.
-    pub fn validate<S: AsRef<str>>(hosts: &[S]) -> anyhow::Result<()> {
-        _ = Self::parse_partial(hosts)?;
+    /// Validates the given allowed_outbound_hosts values with the given resolver.
+    pub fn validate<S: AsRef<str>>(hosts: &[S], resolver: &Resolver) -> anyhow::Result<()> {
+        for partial in Self::parse_partial(hosts)? {
+            partial.validate(resolver)?;
+        }
         Ok(())
     }