From bb68dabff41efea17ee6cdd85eb2465b02c1b7cc Mon Sep 17 00:00:00 2001 From: Lann Martin Date: Tue, 3 Jun 2025 16:11:31 -0400 Subject: [PATCH 1/5] Appease clippy Signed-off-by: Lann Martin --- crates/variables/src/env.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/variables/src/env.rs b/crates/variables/src/env.rs index 384e982008..6976b00ca3 100644 --- a/crates/variables/src/env.rs +++ b/crates/variables/src/env.rs @@ -52,10 +52,10 @@ impl EnvVariablesProvider { /// Creates a new EnvProvider. /// /// * `prefix` - The string prefix to use to distinguish an environment variable that should be used. - /// If not set, the default prefix is used. + /// If not set, the default prefix is used. /// * `env_fetcher` - The function to use to fetch an environment variable. /// * `dotenv_path` - The path to the .env file to load environment variables from. If not set, - /// no .env file is loaded. + /// no .env file is loaded. pub fn new( prefix: Option>, env_fetcher: impl Fn(&str) -> Result + Send + Sync + 'static, From caafc19f372e120a5791aecb7e1212cd95ee4059 Mon Sep 17 00:00:00 2001 From: Lann Martin Date: Wed, 18 Jun 2025 09:39:28 -0400 Subject: [PATCH 2/5] Fix a couple of random comments Signed-off-by: Lann Martin --- crates/factor-outbound-networking/src/allowed_hosts.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/factor-outbound-networking/src/allowed_hosts.rs b/crates/factor-outbound-networking/src/allowed_hosts.rs index 9dbbbce31a..7b8241cc17 100644 --- a/crates/factor-outbound-networking/src/allowed_hosts.rs +++ b/crates/factor-outbound-networking/src/allowed_hosts.rs @@ -84,10 +84,7 @@ pub struct AllowedHostConfig { } impl AllowedHostConfig { - /// Try to parse the address. - /// - /// If the parsing fails, the address is prepended with the scheme and parsing - /// is tried again. + /// Parses the given string as an `allowed_hosts_config` item. pub fn parse(url: impl Into) -> anyhow::Result { let original = url.into(); let url = original.trim(); @@ -435,7 +432,7 @@ pub struct OutboundUrl { } impl OutboundUrl { - /// Parse a URL. + /// Parses a URL. /// /// If parsing `url` fails, `{scheme}://` is prepended to `url` and parsing is tried again. pub fn parse(url: impl Into, scheme: &str) -> anyhow::Result { From 6484fe90192b4ed527cd4f7f56281cc40b187b27 Mon Sep 17 00:00:00 2001 From: Lann Martin Date: Wed, 18 Jun 2025 12:15:01 -0400 Subject: [PATCH 3/5] expressions: Fix Template Display impl Signed-off-by: Lann Martin --- crates/expressions/src/template.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/expressions/src/template.rs b/crates/expressions/src/template.rs index 694ee01f8a..b533015507 100644 --- a/crates/expressions/src/template.rs +++ b/crates/expressions/src/template.rs @@ -55,7 +55,7 @@ impl Display for Template { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { self.parts().try_for_each(|part| match part { Part::Lit(lit) => f.write_str(lit), - Part::Expr(expr) => write!(f, "{{ {} }}", expr), + Part::Expr(expr) => write!(f, "{{{{ {} }}}}", expr), }) } } From 3dbb9ce0b0b6a3e769bc371ac72fd083e02d26eb Mon Sep 17 00:00:00 2001 From: Lann Martin Date: Wed, 18 Jun 2025 12:22:07 -0400 Subject: [PATCH 4/5] outbound-networking: Add doc comments Signed-off-by: Lann Martin --- .../src/allowed_hosts.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/crates/factor-outbound-networking/src/allowed_hosts.rs b/crates/factor-outbound-networking/src/allowed_hosts.rs index 7b8241cc17..c7080731cb 100644 --- a/crates/factor-outbound-networking/src/allowed_hosts.rs +++ b/crates/factor-outbound-networking/src/allowed_hosts.rs @@ -74,7 +74,7 @@ pub fn validate_service_chaining_for_components( Ok(()) } -/// An address is a url-like string that contains a host, a port, and an optional scheme +/// Represents a single `allowed_outbound_hosts` item. #[derive(Eq, Debug, Clone)] pub struct AllowedHostConfig { original: String, @@ -113,24 +113,30 @@ impl AllowedHostConfig { }) } + /// Returns the scheme part of this config. pub fn scheme(&self) -> &SchemeConfig { &self.scheme } + /// Returns the host part of this config. pub fn host(&self) -> &HostConfig { &self.host } + /// Returns the port part of this config. pub fn port(&self) -> &PortConfig { &self.port } + /// Returns true if the given url is allowed by this config. fn allows(&self, url: &OutboundUrl) -> bool { self.scheme.allows(&url.scheme) && self.host.allows(&url.host) && self.port.allows(url.port, &url.scheme) } + /// Returns true if this config allows relative ("self") requests to any of + /// the given schemes. fn allows_relative(&self, schemes: &[&str]) -> bool { schemes.iter().any(|s| self.scheme.allows(s)) && self.host.allows_relative() } @@ -148,13 +154,17 @@ impl std::fmt::Display for AllowedHostConfig { } } +/// Represents the scheme part of an allowed_outbound_hosts item. #[derive(PartialEq, Eq, Debug, Clone)] pub enum SchemeConfig { + /// Any scheme is allowed: `*://` Any, + /// Any of the given schemes are allowed List(Vec), } impl SchemeConfig { + /// Parses the scheme part of an allowed_outbound_hosts item. fn parse(scheme: &str) -> anyhow::Result { if scheme == "*" { return Ok(Self::Any); @@ -172,10 +182,12 @@ impl SchemeConfig { Ok(Self::List(vec![scheme.into()])) } + /// Returns true if any scheme is allowed (i.e. `*://`). pub fn allows_any(&self) -> bool { matches!(self, Self::Any) } + /// Returns true if the given scheme is allowed. fn allows(&self, scheme: &str) -> bool { match self { SchemeConfig::Any => true, @@ -184,6 +196,7 @@ impl SchemeConfig { } } +/// Represents the host part of an allowed_outbound_hosts item. #[derive(Debug, PartialEq, Eq, Clone)] pub enum HostConfig { Any, @@ -194,6 +207,7 @@ pub enum HostConfig { } impl HostConfig { + /// Parses the host part of an allowed_outbound_hosts item. fn parse(mut host: &str) -> anyhow::Result { host = host.trim(); if host == "*" { @@ -234,6 +248,7 @@ impl HostConfig { Ok(Self::List(vec![host.into()])) } + /// Returns true if the given host is allowed by this config. fn allows(&self, host: &str) -> bool { match self { HostConfig::Any => true, @@ -249,6 +264,7 @@ impl HostConfig { } } + /// Returns true if this config allows relative ("self") requests. fn allows_relative(&self) -> bool { matches!(self, Self::Any | Self::ToSelf) } @@ -261,6 +277,7 @@ pub enum PortConfig { } impl PortConfig { + /// Parses the port part of an allowed_outbound_hosts item. fn parse(port: &str, scheme: &str) -> anyhow::Result { if port.is_empty() { return well_known_port(scheme) From 7e119db4937dbb2681c43c8be15a0eba001e48fe Mon Sep 17 00:00:00 2001 From: Lann Martin Date: Wed, 18 Jun 2025 12:28:31 -0400 Subject: [PATCH 5/5] outbound-networking: Refactor allowed_hosts Signed-off-by: Lann Martin --- .../src/allowed_hosts.rs | 120 ++++++++++++------ crates/loader/src/local.rs | 16 +-- 2 files changed, 84 insertions(+), 52 deletions(-) diff --git a/crates/factor-outbound-networking/src/allowed_hosts.rs b/crates/factor-outbound-networking/src/allowed_hosts.rs index c7080731cb..56dc0816c9 100644 --- a/crates/factor-outbound-networking/src/allowed_hosts.rs +++ b/crates/factor-outbound-networking/src/allowed_hosts.rs @@ -3,6 +3,7 @@ use std::ops::Range; use anyhow::{bail, ensure, Context}; use spin_factors::{App, AppComponent}; use spin_locked_app::MetadataKey; +use url::Host; const ALLOWED_HOSTS_KEY: MetadataKey> = MetadataKey::new("allowed_outbound_hosts"); const ALLOWED_HTTP_KEY: MetadataKey> = MetadataKey::new("allowed_http_hosts"); @@ -105,10 +106,17 @@ impl AllowedHostConfig { None => rest, }; + let port = PortConfig::parse(port, scheme) + .with_context(|| format!("Invalid allowed host port {port:?}"))?; + let scheme = SchemeConfig::parse(scheme) + .with_context(|| format!("Invalid allowed host scheme {scheme:?}"))?; + let host = + HostConfig::parse(host).with_context(|| format!("Invalid allowed host {host:?}"))?; + Ok(Self { - scheme: SchemeConfig::parse(scheme)?, - host: HostConfig::parse(host)?, - port: PortConfig::parse(port, scheme)?, + scheme, + host, + port, original, }) } @@ -128,6 +136,11 @@ impl AllowedHostConfig { &self.port } + /// Returns true if this config is for service chaining requests. + pub fn is_for_service_chaining(&self) -> bool { + self.host.is_for_service_chaining() + } + /// Returns true if the given url is allowed by this config. fn allows(&self, url: &OutboundUrl) -> bool { self.scheme.allows(&url.scheme) @@ -172,11 +185,11 @@ impl SchemeConfig { if scheme.starts_with('{') { // TODO: - bail!("scheme lists are not yet supported") + anyhow::bail!("scheme lists are not yet supported") } if scheme.chars().any(|c| !c.is_alphabetic()) { - anyhow::bail!(" scheme {scheme:?} contains non alphabetic character"); + anyhow::bail!("only alphabetic character(s) are allowed"); } Ok(Self::List(vec![scheme.into()])) @@ -202,7 +215,7 @@ pub enum HostConfig { Any, AnySubdomain(String), ToSelf, - List(Vec), + Literal(Host), Cidr(ip_network::IpNetwork), } @@ -227,40 +240,47 @@ impl HostConfig { return Ok(Self::Cidr(net)); } - if matches!(host.split('/').nth(1), Some(path) if !path.is_empty()) { - bail!("hosts must not contain paths"); + host = host.trim_end_matches('/'); + if host.contains('/') { + bail!("must not include a path"); } if let Some(domain) = host.strip_prefix("*.") { if domain.contains('*') { - bail!("Invalid allowed host {host}: wildcards are allowed only as prefixes"); + bail!("wildcards are allowed only as prefixes"); } return Ok(Self::AnySubdomain(format!(".{domain}"))); } if host.contains('*') { - bail!("Invalid allowed host {host}: wildcards are allowed only as subdomains"); + bail!("wildcards are allowed only as subdomains"); } - // Remove trailing slashes - host = host.trim_end_matches('/'); + Self::literal(host) + } - Ok(Self::List(vec![host.into()])) + /// Returns a HostConfig from the given literal host name. + fn literal(host: &str) -> anyhow::Result { + Ok(Self::Literal(Host::parse(host)?)) } /// Returns true if the given host is allowed by this config. fn allows(&self, host: &str) -> bool { - match self { - HostConfig::Any => true, - HostConfig::AnySubdomain(suffix) => host.ends_with(suffix), - HostConfig::List(l) => l.iter().any(|h| h.as_str() == host), - HostConfig::ToSelf => false, - HostConfig::Cidr(c) => { - let Ok(ip) = host.parse::() else { - return false; - }; - c.contains(ip) + let host: Host = match Host::parse(host) { + Ok(host) => host, + Err(err) => { + tracing::warn!(?err, "invalid host in HostConfig::allows"); + return false; } + }; + match (self, host) { + (HostConfig::Any, _) => true, + (HostConfig::AnySubdomain(suffix), Host::Domain(domain)) => domain.ends_with(suffix), + (HostConfig::Literal(literal), host) => host == *literal, + (HostConfig::Cidr(c), Host::Ipv4(ip)) => c.contains(ip), + (HostConfig::Cidr(c), Host::Ipv6(ip)) => c.contains(ip), + // Note: HostConfig::ToSelf is checked with ::allow_relative + _ => false, } } @@ -268,8 +288,18 @@ impl HostConfig { fn allows_relative(&self) -> bool { matches!(self, Self::Any | Self::ToSelf) } + + /// Returns true if this config is for service chaining requests. + fn is_for_service_chaining(&self) -> bool { + match self { + Self::Literal(Host::Domain(domain)) => domain.ends_with(SERVICE_CHAINING_DOMAIN_SUFFIX), + Self::AnySubdomain(suffix) => suffix == SERVICE_CHAINING_DOMAIN_SUFFIX, + _ => false, + } + } } +/// Represents the port part of an allowed_outbound_hosts item. #[derive(Debug, PartialEq, Eq, Clone)] pub enum PortConfig { Any, @@ -561,9 +591,10 @@ mod test { } impl HostConfig { - fn new(host: &str) -> Self { - Self::List(vec![host.into()]) + fn unwrap_literal(host: &str) -> Self { + Self::literal(host).unwrap_or_else(|_| panic!("invalid host {host:?}")) } + fn subdomain(domain: &str) -> Self { Self::AnySubdomain(format!(".{domain}")) } @@ -612,7 +643,7 @@ mod test { assert_eq!( AllowedHostConfig::new( SchemeConfig::new("http"), - HostConfig::new("spin.fermyon.dev"), + HostConfig::unwrap_literal("spin.fermyon.dev"), PortConfig::new(80) ), AllowedHostConfig::parse("http://spin.fermyon.dev").unwrap() @@ -622,7 +653,7 @@ mod test { AllowedHostConfig::new( SchemeConfig::new("http"), // Trailing slash is removed - HostConfig::new("spin.fermyon.dev"), + HostConfig::unwrap_literal("spin.fermyon.dev"), PortConfig::new(80) ), AllowedHostConfig::parse("http://spin.fermyon.dev/").unwrap() @@ -631,7 +662,7 @@ mod test { assert_eq!( AllowedHostConfig::new( SchemeConfig::new("https"), - HostConfig::new("spin.fermyon.dev"), + HostConfig::unwrap_literal("spin.fermyon.dev"), PortConfig::new(443) ), AllowedHostConfig::parse("https://spin.fermyon.dev").unwrap() @@ -643,7 +674,7 @@ mod test { assert_eq!( AllowedHostConfig::new( SchemeConfig::new("http"), - HostConfig::new("spin.fermyon.dev"), + HostConfig::unwrap_literal("spin.fermyon.dev"), PortConfig::new(4444) ), AllowedHostConfig::parse("http://spin.fermyon.dev:4444").unwrap() @@ -651,7 +682,7 @@ mod test { assert_eq!( AllowedHostConfig::new( SchemeConfig::new("http"), - HostConfig::new("spin.fermyon.dev"), + HostConfig::unwrap_literal("spin.fermyon.dev"), PortConfig::new(4444) ), AllowedHostConfig::parse("http://spin.fermyon.dev:4444/").unwrap() @@ -659,7 +690,7 @@ mod test { assert_eq!( AllowedHostConfig::new( SchemeConfig::new("https"), - HostConfig::new("spin.fermyon.dev"), + HostConfig::unwrap_literal("spin.fermyon.dev"), PortConfig::new(5555) ), AllowedHostConfig::parse("https://spin.fermyon.dev:5555").unwrap() @@ -671,7 +702,7 @@ mod test { assert_eq!( AllowedHostConfig::new( SchemeConfig::new("http"), - HostConfig::new("spin.fermyon.dev"), + HostConfig::unwrap_literal("spin.fermyon.dev"), PortConfig::range(4444..5555) ), AllowedHostConfig::parse("http://spin.fermyon.dev:4444..5555").unwrap() @@ -693,7 +724,7 @@ mod test { assert_eq!( AllowedHostConfig::new( SchemeConfig::Any, - HostConfig::new("spin.fermyon.dev"), + HostConfig::unwrap_literal("spin.fermyon.dev"), PortConfig::new(7777) ), AllowedHostConfig::parse("*://spin.fermyon.dev:7777").unwrap() @@ -718,7 +749,7 @@ mod test { assert_eq!( AllowedHostConfig::new( SchemeConfig::new("http"), - HostConfig::new("localhost"), + HostConfig::unwrap_literal("localhost"), PortConfig::new(80) ), AllowedHostConfig::parse("http://localhost").unwrap() @@ -727,7 +758,7 @@ mod test { assert_eq!( AllowedHostConfig::new( SchemeConfig::new("http"), - HostConfig::new("localhost"), + HostConfig::unwrap_literal("localhost"), PortConfig::new(3001) ), AllowedHostConfig::parse("http://localhost:3001").unwrap() @@ -751,7 +782,7 @@ mod test { assert_eq!( AllowedHostConfig::new( SchemeConfig::new("http"), - HostConfig::new("192.168.1.1"), + HostConfig::unwrap_literal("192.168.1.1"), PortConfig::new(80) ), AllowedHostConfig::parse("http://192.168.1.1").unwrap() @@ -759,7 +790,7 @@ mod test { assert_eq!( AllowedHostConfig::new( SchemeConfig::new("http"), - HostConfig::new("192.168.1.1"), + HostConfig::unwrap_literal("192.168.1.1"), PortConfig::new(3002) ), AllowedHostConfig::parse("http://192.168.1.1:3002").unwrap() @@ -767,7 +798,7 @@ mod test { assert_eq!( AllowedHostConfig::new( SchemeConfig::new("http"), - HostConfig::new("[::1]"), + HostConfig::unwrap_literal("[::1]"), PortConfig::new(8001) ), AllowedHostConfig::parse("http://[::1]:8001").unwrap() @@ -811,6 +842,19 @@ mod test { assert!(AllowedHostConfig::parse("http://*.fermyon.dev/a").is_err()); } + #[test] + fn test_rejects_invalid_parts() { + for invalid in [ + "h@x://localhost", + "http://invalid host", + "http://inv@lid-host", + "http://", + "http://:80", + ] { + AllowedHostConfig::parse(invalid).expect_err(invalid); + } + } + #[test] fn test_allowed_hosts_respects_allow_all() { assert!(AllowedHostsConfig::parse(&["insecure:allow-all"], &dummy_resolver()).is_err()); diff --git a/crates/loader/src/local.rs b/crates/loader/src/local.rs index a352872ea4..770a470b30 100644 --- a/crates/loader/src/local.rs +++ b/crates/loader/src/local.rs @@ -4,7 +4,6 @@ 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_factor_outbound_networking::SERVICE_CHAINING_DOMAIN_SUFFIX; use spin_locked_app::{ locked::{ self, ContentPath, ContentRef, LockedApp, LockedComponent, LockedComponentDependency, @@ -838,19 +837,8 @@ fn requires_service_chaining(component: &spin_manifest::schema::v2::Component) - } fn is_chaining_host(pattern: &str) -> bool { - use spin_factor_outbound_networking::{AllowedHostConfig, HostConfig}; - - let Ok(allowed) = AllowedHostConfig::parse(pattern) else { - return false; - }; - - match allowed.host() { - HostConfig::List(hosts) => hosts - .iter() - .any(|h| h.ends_with(SERVICE_CHAINING_DOMAIN_SUFFIX)), - HostConfig::AnySubdomain(domain) => domain == SERVICE_CHAINING_DOMAIN_SUFFIX, - _ => false, - } + spin_factor_outbound_networking::AllowedHostConfig::parse(pattern) + .is_ok_and(|config| config.is_for_service_chaining()) } const SLOTH_WARNING_DELAY_MILLIS: u64 = 1250;