Address review feedback

Signed-off-by: Lann Martin <[email protected]>

Author: lann

crates/factor-outbound-http/src/spin.rs

@@ -1,4 +1,3 @@
-use spin_factor_outbound_networking::OutboundUrl;
 use spin_world::{
     async_trait,
     v1::http,
@@ -9,8 +8,7 @@ use spin_world::{
 impl http::Host for crate::InstanceState {
     async fn send_request(&mut self, req: Request) -> Result<Response, HttpError> {
         // FIXME(lann): This is all just a stub to test allowed_outbound_hosts
-        let outbound_url = OutboundUrl::parse(&req.uri, "https").or(Err(HttpError::InvalidUrl))?;
-        match self.allowed_hosts.allows(&outbound_url).await {
+        match self.allowed_hosts.check_url(&req.uri, "https").await {
             Ok(true) => (),
             _ => {
                 return Err(HttpError::DestinationNotAllowed);

crates/factor-outbound-http/src/wasi.rs

@@ -127,20 +127,24 @@ async fn send_request_impl(
         let is_allowed = outbound_allowed_hosts
             .check_url(&request.uri().to_string(), "https")
             .await
-            .map_err(|_| ErrorCode::HttpRequestUriInvalid)?;
+            .unwrap_or(false);
         if !is_allowed {
             return Ok(Err(ErrorCode::HttpRequestDenied));
         }
     } else {
         // Relative URI ("self" request)
-        let allowed_hosts = outbound_allowed_hosts.resolve().await?;
-        if !allowed_hosts.allows_relative_url(&["http", "https"]) {
-            outbound_allowed_hosts.report_disallowed_host("http", "self");
+        let is_allowed = outbound_allowed_hosts
+            .check_relative_url(&["http", "https"])
+            .await
+            .unwrap_or(false);
+        if !is_allowed {
             return Ok(Err(ErrorCode::HttpRequestDenied));
         }
 
-        let origin = self_request_origin
-            .context("cannot send relative outbound request; no 'origin' set by host")?;
+        let Some(origin) = self_request_origin else {
+            tracing::error!("Couldn't handle outbound HTTP request to relative URI; no origin set");
+            return Ok(Err(ErrorCode::HttpRequestUriInvalid));
+        };
 
         config.use_tls = origin.use_tls();
 
@@ -150,12 +154,11 @@ async fn send_request_impl(
         *request.uri_mut() = origin.into_uri(path_and_query);
     }
 
-    if let Some(authority) = request.uri().authority() {
-        let current_span = tracing::Span::current();
-        current_span.record("server.address", authority.host());
-        if let Some(port) = authority.port() {
-            current_span.record("server.port", port.as_u16());
-        }
+    let authority = request.uri().authority().context("authority not set")?;
+    let current_span = tracing::Span::current();
+    current_span.record("server.address", authority.host());
+    if let Some(port) = authority.port() {
+        current_span.record("server.port", port.as_u16());
     }
 
     Ok(send_request_handler(request, config, tls_client_config).await)

crates/factor-outbound-networking/src/lib.rs

@@ -24,20 +24,18 @@ pub type SharedFutureResult<T> = Shared<BoxFuture<'static, Result<Arc<T>, Arc<an
 
 #[derive(Default)]
 pub struct OutboundNetworkingFactor {
-    disallowed_host_callback: Option<DisallowedHostCallback>,
+    disallowed_host_handler: Option<Arc<dyn DisallowedHostHandler>>,
 }
 
-pub type DisallowedHostCallback = fn(scheme: &str, authority: &str);
-
 impl OutboundNetworkingFactor {
     pub fn new() -> Self {
         Self::default()
     }
 
-    /// Sets a function to be called when a request is disallowed by an
+    /// Sets a handler to be called when a request is disallowed by an
     /// instance's configured `allowed_outbound_hosts`.
-    pub fn set_disallowed_host_callback(&mut self, callback: DisallowedHostCallback) {
-        self.disallowed_host_callback = Some(callback);
+    pub fn set_disallowed_host_handler(&mut self, handler: impl DisallowedHostHandler + 'static) {
+        self.disallowed_host_handler = Some(Arc::new(handler));
     }
 }
 
@@ -106,7 +104,7 @@ impl Factor for OutboundNetworkingFactor {
                 // Update Wasi socket allowed ports
                 let allowed_hosts = OutboundAllowedHosts {
                     allowed_hosts_future: allowed_hosts_future.clone(),
-                    disallowed_host_callback: self.disallowed_host_callback,
+                    disallowed_host_handler: self.disallowed_host_handler.clone(),
                 };
                 wasi_builder.outbound_socket_addr_check(move |addr, addr_use| {
                     let allowed_hosts = allowed_hosts.clone();
@@ -137,7 +135,7 @@ impl Factor for OutboundNetworkingFactor {
         Ok(InstanceBuilder {
             allowed_hosts_future,
             component_tls_configs,
-            disallowed_host_callback: self.disallowed_host_callback,
+            disallowed_host_handler: self.disallowed_host_handler.clone(),
         })
     }
 }
@@ -150,14 +148,14 @@ pub struct AppState {
 pub struct InstanceBuilder {
     allowed_hosts_future: SharedFutureResult<AllowedHostsConfig>,
     component_tls_configs: ComponentTlsConfigs,
-    disallowed_host_callback: Option<DisallowedHostCallback>,
+    disallowed_host_handler: Option<Arc<dyn DisallowedHostHandler>>,
 }
 
 impl InstanceBuilder {
     pub fn allowed_hosts(&self) -> OutboundAllowedHosts {
         OutboundAllowedHosts {
             allowed_hosts_future: self.allowed_hosts_future.clone(),
-            disallowed_host_callback: self.disallowed_host_callback,
+            disallowed_host_handler: self.disallowed_host_handler.clone(),
         }
     }
 
@@ -178,33 +176,10 @@ impl FactorInstanceBuilder for InstanceBuilder {
 #[derive(Clone)]
 pub struct OutboundAllowedHosts {
     allowed_hosts_future: SharedFutureResult<AllowedHostsConfig>,
-    disallowed_host_callback: Option<DisallowedHostCallback>,
+    disallowed_host_handler: Option<Arc<dyn DisallowedHostHandler>>,
 }
 
 impl OutboundAllowedHosts {
-    pub async fn resolve(&self) -> anyhow::Result<Arc<AllowedHostsConfig>> {
-        self.allowed_hosts_future.clone().await.map_err(|err| {
-            // TODO: better way to handle this?
-            anyhow::Error::msg(err)
-        })
-    }
-
-    /// Checks if the given URL is allowed by this component's
-    /// `allowed_outbound_hosts`.
-    pub async fn allows(&self, url: &OutboundUrl) -> anyhow::Result<bool> {
-        Ok(self.resolve().await?.allows(url))
-    }
-
-    /// Report that an outbound connection has been disallowed by e.g.
-    /// [`OutboundAllowedHosts::allows`] returning `false`.
-    ///
-    /// Calls the [`DisallowedHostCallback`] if set.
-    pub fn report_disallowed_host(&self, scheme: &str, authority: &str) {
-        if let Some(disallowed_host_callback) = self.disallowed_host_callback {
-            disallowed_host_callback(scheme, authority);
-        }
-    }
-
     /// Checks address against allowed hosts
     ///
     /// Calls the [`DisallowedHostCallback`] if set and URL is disallowed.
@@ -217,11 +192,47 @@ impl OutboundAllowedHosts {
         };
 
         let allowed_hosts = self.resolve().await?;
-
         let is_allowed = allowed_hosts.allows(&url);
         if !is_allowed {
             self.report_disallowed_host(url.scheme(), &url.authority());
         }
         Ok(is_allowed)
     }
+
+    /// Checks if allowed hosts permit relative requests
+    ///
+    /// Calls the [`DisallowedHostCallback`] if set and relative requests are
+    /// disallowed.
+    pub async fn check_relative_url(&self, schemes: &[&str]) -> anyhow::Result<bool> {
+        let allowed_hosts = self.resolve().await?;
+        let is_allowed = allowed_hosts.allows_relative_url(schemes);
+        if !is_allowed {
+            let scheme = schemes.first().unwrap_or(&"");
+            self.report_disallowed_host(scheme, "self");
+        }
+        Ok(is_allowed)
+    }
+
+    async fn resolve(&self) -> anyhow::Result<Arc<AllowedHostsConfig>> {
+        self.allowed_hosts_future.clone().await.map_err(|err| {
+            tracing::error!("Error resolving allowed_outbound_hosts variables: {err}");
+            anyhow::Error::msg(err)
+        })
+    }
+
+    fn report_disallowed_host(&self, scheme: &str, authority: &str) {
+        if let Some(handler) = &self.disallowed_host_handler {
+            handler.handle_disallowed_host(scheme, authority);
+        }
+    }
+}
+
+pub trait DisallowedHostHandler: Send + Sync {
+    fn handle_disallowed_host(&self, scheme: &str, authority: &str);
+}
+
+impl<F: Fn(&str, &str) + Send + Sync> DisallowedHostHandler for F {
+    fn handle_disallowed_host(&self, scheme: &str, authority: &str) {
+        self(scheme, authority);
+    }
 }

crates/trigger2/src/factors.rs

@@ -45,7 +45,7 @@ fn wasi_factor(working_dir: impl Into<PathBuf>, allow_transient_writes: bool) ->
 }
 
 fn outbound_networking_factor() -> OutboundNetworkingFactor {
-    fn disallowed_host_callback(scheme: &str, authority: &str) {
+    fn disallowed_host_handler(scheme: &str, authority: &str) {
         let host_pattern = format!("{scheme}://{authority}");
         tracing::error!("Outbound network destination not allowed: {host_pattern}");
         if scheme.starts_with("http") && authority == "self" {
@@ -59,7 +59,7 @@ fn outbound_networking_factor() -> OutboundNetworkingFactor {
     }
 
     let mut factor = OutboundNetworkingFactor::new();
-    factor.set_disallowed_host_callback(disallowed_host_callback);
+    factor.set_disallowed_host_handler(disallowed_host_handler);
     factor
 }