From 15c77391f0416e044609d4f257878e3b15d11267 Mon Sep 17 00:00:00 2001
From: Ron McCorkle <31546545+mack42@users.noreply.github.com>
Date: Sun, 19 Jul 2026 19:57:25 +0000
Subject: [PATCH] fix: preflight and fail closed on restore apply errors

---
 aifw-api/src/backup.rs               | 109 ++++++++++++++++++++++++++-
 aifw-api/src/opnsense/importer.rs    |   6 +-
 aifw-api/src/routes/static_routes.rs |  26 +++++--
 aifw-api/src/tests.rs                |  32 ++++++++
 4 files changed, 163 insertions(+), 10 deletions(-)

diff --git a/aifw-api/src/backup.rs b/aifw-api/src/backup.rs
index 17e136e2..dc941fc8 100644
--- a/aifw-api/src/backup.rs
+++ b/aifw-api/src/backup.rs
@@ -1511,6 +1511,109 @@ fn apply_fail(step: &str, e: impl std::fmt::Display) -> StatusCode {
     StatusCode::INTERNAL_SERVER_ERROR
 }
 
+/// Render and validate every required target resource before the first
+/// destructive DELETE. Older restore code discovered malformed rows halfway
+/// through apply and silently skipped them, making rollback the normal
+/// validation path and allowing partial-success responses.
+fn validate_restore_target(
+    config: &FirewallConfig,
+    iface_map: &InterfaceMap,
+) -> Result<(), StatusCode> {
+    use aifw_common::{Address, AliasType, CountryCode};
+
+    for rule in &config.rules {
+        if rule
+            .interface
+            .as_deref()
+            .is_some_and(|name| map_iface(name, iface_map).is_none())
+        {
+            continue;
+        }
+        rule_from_config(rule)
+            .ok_or_else(|| apply_fail(&format!("validating rule {}", rule.id), "invalid rule"))?;
+    }
+    for nat in &config.nat {
+        if map_iface(&nat.interface, iface_map).is_none() {
+            continue;
+        }
+        nat_from_config(nat).ok_or_else(|| {
+            apply_fail(
+                &format!("validating nat rule {}", nat.id),
+                "invalid NAT rule",
+            )
+        })?;
+    }
+    for alias in &config.aliases {
+        AliasType::parse(&alias.alias_type).ok_or_else(|| {
+            apply_fail(
+                &format!("validating alias {}", alias.name),
+                "invalid alias type",
+            )
+        })?;
+    }
+    for route in &config.static_routes {
+        crate::routes::validate_route_target(&route.destination).map_err(|_| {
+            apply_fail(
+                &format!("validating route {}", route.destination),
+                "invalid destination",
+            )
+        })?;
+        crate::routes::validate_route_target(&route.gateway).map_err(|_| {
+            apply_fail(
+                &format!("validating route {}", route.destination),
+                "invalid gateway",
+            )
+        })?;
+    }
+    for geo in &config.geoip {
+        CountryCode::new(&geo.country)
+            .map_err(|e| apply_fail(&format!("validating geo-ip {}", geo.id), e))?;
+    }
+    for tunnel in &config.vpn.wireguard {
+        if map_iface(&tunnel.interface, iface_map).is_none() {
+            continue;
+        }
+        Address::parse(&tunnel.address)
+            .map_err(|e| apply_fail(&format!("validating wireguard {}", tunnel.name), e))?;
+        for peer in &tunnel.peers {
+            for address in &peer.allowed_ips {
+                Address::parse(address).map_err(|e| {
+                    apply_fail(&format!("validating wireguard peer {}", peer.name), e)
+                })?;
+            }
+        }
+    }
+    for sa in &config.vpn.ipsec {
+        Address::parse(&sa.src_addr)
+            .map_err(|e| apply_fail(&format!("validating ipsec {}", sa.name), e))?;
+        Address::parse(&sa.dst_addr)
+            .map_err(|e| apply_fail(&format!("validating ipsec {}", sa.name), e))?;
+    }
+    for queue in &config.queues {
+        if map_iface(&queue.interface, iface_map).is_none() {
+            continue;
+        }
+        queue_from_config(queue).ok_or_else(|| {
+            apply_fail(
+                &format!("validating shaping queue {}", queue.name),
+                "invalid queue",
+            )
+        })?;
+    }
+    for limit in &config.rate_limits {
+        if limit
+            .interface
+            .as_deref()
+            .is_some_and(|name| map_iface(name, iface_map).is_none())
+        {
+            continue;
+        }
+        rate_limit_from_config(limit)
+            .ok_or_else(|| apply_fail("validating rate limit", "invalid rate limit"))?;
+    }
+    Ok(())
+}
+
 /// Restore-with-rollback wrapper around [`apply_firewall_config`] (#535).
 ///
 /// Captures the current running config first, applies the target strictly,
@@ -1529,6 +1632,7 @@ pub(crate) async fn apply_firewall_config_or_rollback(
     config: &FirewallConfig,
     iface_map: &InterfaceMap,
 ) -> Result<(), StatusCode> {
+    validate_restore_target(config, iface_map)?;
     let snapshot = build_current_config(state).await?;
     let Err(apply_err) = apply_firewall_config(state, config, iface_map).await else {
         return Ok(());
@@ -1580,6 +1684,8 @@ pub(crate) async fn apply_firewall_config(
         Address, CountryCode, GeoIpRule, Interface, IpsecSa, VpnStatus, WgPeer, WgTunnel,
     };
 
+    validate_restore_target(config, iface_map)?;
+
     // Restore preludes — clear existing rows before re-populating from the
     // imported config. A failed DELETE (locked DB, FK violation, schema
     // drift) used to warn and continue, leaving stale rows in a half-imported
@@ -1707,7 +1813,8 @@ pub(crate) async fn apply_firewall_config(
                 iface_after.as_deref(),
                 rc.fib,
             )
-            .await;
+            .await
+            .map_err(|e| apply_fail(&format!("static route {} apply", rc.destination), e))?;
         }
     }
 
diff --git a/aifw-api/src/opnsense/importer.rs b/aifw-api/src/opnsense/importer.rs
index 08aceac4..475a64dc 100644
--- a/aifw-api/src/opnsense/importer.rs
+++ b/aifw-api/src/opnsense/importer.rs
@@ -1504,7 +1504,11 @@ async fn apply_routes(
                 // Attempt to program the kernel route. Mirrors what
                 // `routes::create_static_route` does for a single-route
                 // creation request.
-                crate::routes::apply_route_to_system(&r.network, gateway, None, 0).await;
+                if let Err(e) =
+                    crate::routes::apply_route_to_system(&r.network, gateway, None, 0).await
+                {
+                    tracing::warn!(error = %e, "opnsense import: route apply failed");
+                }
                 tracker.static_route_ids.push(id);
                 summary.static_routes += 1;
             }
diff --git a/aifw-api/src/routes/static_routes.rs b/aifw-api/src/routes/static_routes.rs
index d0d947f4..de368593 100644
--- a/aifw-api/src/routes/static_routes.rs
+++ b/aifw-api/src/routes/static_routes.rs
@@ -110,14 +110,16 @@ pub async fn create_static_route(
     .map_err(|_| bad_request())?;
 
     // Apply to system if enabled
-    if enabled {
-        apply_route_to_system(
+    if enabled
+        && let Err(e) = apply_route_to_system(
             &req.destination,
             &req.gateway,
             req.interface.as_deref(),
             fib,
         )
-        .await;
+        .await
+    {
+        tracing::warn!(error = %e, "route create: system apply failed; route remains disabled until startup retry");
     }
 
     let route = StaticRoute {
@@ -169,14 +171,16 @@ pub async fn update_static_route(
     .await
     .map_err(|_| internal())?;
 
-    if enabled {
-        apply_route_to_system(
+    if enabled
+        && let Err(e) = apply_route_to_system(
             &req.destination,
             &req.gateway,
             req.interface.as_deref(),
             fib,
         )
-        .await;
+        .await
+    {
+        tracing::warn!(error = %e, "route update: system apply failed");
     }
 
     let now = chrono::Utc::now().to_rfc3339();
@@ -227,7 +231,7 @@ pub(crate) async fn apply_route_to_system(
     gateway: &str,
     interface: Option<&str>,
     fib: u32,
-) {
+) -> Result<(), String> {
     let fib_s = fib.to_string();
     let mut args: Vec<&str> = Vec::new();
     args.push("/sbin/route");
@@ -249,6 +253,7 @@ pub(crate) async fn apply_route_to_system(
     match output {
         Ok(o) if o.status.success() => {
             tracing::info!(destination, gateway, fib, "route added");
+            Ok(())
         }
         Ok(o) => {
             let err = String::from_utf8_lossy(&o.stderr);
@@ -257,12 +262,15 @@ pub(crate) async fn apply_route_to_system(
             // "route: writing to routing socket: File exists" on FreeBSD.
             if err.contains("File exists") {
                 tracing::debug!(destination, gateway, fib, "route already present, skipping");
+                Ok(())
             } else {
                 tracing::warn!(destination, gateway, fib, error = %err, "route add failed");
+                Err(format!("route add failed: {err}"))
             }
         }
         Err(e) => {
             tracing::warn!(destination, gateway, fib, error = %e, "route command failed");
+            Err(format!("route command failed: {e}"))
         }
     }
 }
@@ -317,6 +325,8 @@ pub async fn apply_all_routes(pool: &sqlx::SqlitePool) {
 
     tracing::info!(count = routes.len(), "applying static routes on startup");
     for (dest, gw, iface, fib) in &routes {
-        apply_route_to_system(dest, gw, iface.as_deref(), *fib as u32).await;
+        if let Err(e) = apply_route_to_system(dest, gw, iface.as_deref(), *fib as u32).await {
+            tracing::warn!(destination = %dest, error = %e, "route startup apply failed");
+        }
     }
 }
diff --git a/aifw-api/src/tests.rs b/aifw-api/src/tests.rs
index 61d88fc4..08d2093c 100644
--- a/aifw-api/src/tests.rs
+++ b/aifw-api/src/tests.rs
@@ -2296,6 +2296,38 @@ mod tests {
         assert!(res.is_err(), "partial apply must not report success");
     }
 
+    #[tokio::test]
+    async fn test_restore_preflight_rejects_invalid_target_without_mutation() {
+        let state = crate::create_app_state_in_memory(plain_auth_settings())
+            .await
+            .unwrap();
+        let mut existing = aifw_common::Rule::new(
+            aifw_common::Action::Block,
+            aifw_common::Direction::In,
+            aifw_common::Protocol::Tcp,
+            aifw_common::RuleMatch {
+                src_addr: aifw_common::Address::Any,
+                src_port: None,
+                dst_addr: aifw_common::Address::Any,
+                dst_port: None,
+            },
+        );
+        existing.label = Some("must-survive-preflight".into());
+        let existing_id = existing.id;
+        state.rule_engine.add_rule(existing).await.unwrap();
+
+        let mut config = crate::backup::build_current_config(&state).await.unwrap();
+        config.rules[0].src_addr = Some("definitely-not-an-address".into());
+        let res =
+            crate::backup::apply_firewall_config_or_rollback(&state, &config, &Default::default())
+                .await;
+        assert!(res.is_err());
+        assert!(
+            state.rule_engine.get_rule(existing_id).await.is_ok(),
+            "preflight failure must happen before destructive table clears"
+        );
+    }
+
     // --- IPsec tunnels (#530) ---
 
     fn ipsec_tunnel_body() -> Value {