From 78ab12bed7baa8251823426adc473cc7309ab8d5 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL HE/HIM) (from Dev Box)" Date: Tue, 11 Aug 2026 16:18:36 -0700 Subject: [PATCH 1/7] Add scoped unspecified firewall rules Replace unspecifiedRulesAction with the scoped unspecifiedRules object and allow empty rule lists for authoritative reconciliation. Add Rust and Pester coverage for direction and profile filtering. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- Cargo.lock | 2 +- resources/windows_firewall/Cargo.toml | 2 +- resources/windows_firewall/locales/en-us.toml | 2 - resources/windows_firewall/src/firewall.rs | 457 ++++++++++++++---- resources/windows_firewall/src/types.rs | 34 +- .../tests/windows_firewall_get.tests.ps1 | 7 +- .../tests/windows_firewall_set.tests.ps1 | 170 ++++++- .../windows_firewall.dsc.resource.json | 55 ++- 8 files changed, 599 insertions(+), 130 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index adb4bb00c..af02a4ba9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4303,7 +4303,7 @@ checksum = "09ec2a7bb152e2252b53fa7803150007879548bc709c039df7627cabbd05d469" [[package]] name = "windows_firewall" -version = "0.2.0" +version = "0.3.0" dependencies = [ "rust-i18n", "serde", diff --git a/resources/windows_firewall/Cargo.toml b/resources/windows_firewall/Cargo.toml index 1230f4fe7..225b247ac 100644 --- a/resources/windows_firewall/Cargo.toml +++ b/resources/windows_firewall/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "windows_firewall" -version = "0.2.0" +version = "0.3.0" edition = "2024" [package.metadata.i18n] diff --git a/resources/windows_firewall/locales/en-us.toml b/resources/windows_firewall/locales/en-us.toml index d26b594be..36ab2a559 100644 --- a/resources/windows_firewall/locales/en-us.toml +++ b/resources/windows_firewall/locales/en-us.toml @@ -9,11 +9,9 @@ invalidJson = "Invalid JSON input: %{error}" windowsOnly = "This resource is only supported on Windows" [get] -rulesArrayEmpty = "The rules array cannot be empty for get operations" selectorRequired = "Each firewall rule in a get request must include a name" [set] -rulesArrayEmpty = "The rules array cannot be empty for set operations" selectorRequired = "Each firewall rule in a set request must include a name" [firewall] diff --git a/resources/windows_firewall/src/firewall.rs b/resources/windows_firewall/src/firewall.rs index f5c6714a6..26a7c0e7b 100644 --- a/resources/windows_firewall/src/firewall.rs +++ b/resources/windows_firewall/src/firewall.rs @@ -2,15 +2,21 @@ // Licensed under the MIT License. use rust_i18n::t; -use windows::core::{BSTR, Interface}; -use windows::core::HRESULT; use windows::Win32::Foundation::{S_FALSE, VARIANT_BOOL}; use windows::Win32::NetworkManagement::WindowsFirewall::*; -use windows::Win32::System::Com::{CLSCTX_INPROC_SERVER, CoCreateInstance, CoInitializeEx, CoUninitialize, IDispatch, COINIT_APARTMENTTHREADED}; +use windows::Win32::System::Com::{ + CLSCTX_INPROC_SERVER, COINIT_APARTMENTTHREADED, CoCreateInstance, CoInitializeEx, + CoUninitialize, IDispatch, +}; use windows::Win32::System::Ole::IEnumVARIANT; use windows::Win32::System::Variant::{VARIANT, VariantClear}; +use windows::core::HRESULT; +use windows::core::{BSTR, Interface}; -use crate::types::{FirewallError, FirewallRule, FirewallRuleList, Metadata, RuleAction, RuleDirection, UnspecifiedRulesAction}; +use crate::types::{ + FirewallError, FirewallRule, FirewallRuleList, Metadata, RuleAction, RuleDirection, + UnspecifiedRuleAction, UnspecifiedRules, +}; /// RAII wrapper for VARIANT that automatically calls VariantClear on drop struct SafeVariant(VARIANT); @@ -32,7 +38,10 @@ impl SafeVariant { impl Drop for SafeVariant { fn drop(&mut self) { if let Err(e) = unsafe { VariantClear(&mut self.0) } { - crate::write_error(&format!("Warning: VariantClear failed with HRESULT: {:#010x}", e.code().0 as u32)); + crate::write_error(&format!( + "Warning: VariantClear failed with HRESULT: {:#010x}", + e.code().0 as u32 + )); } } } @@ -62,36 +71,46 @@ struct FirewallStore { impl FirewallStore { fn open() -> Result { let com = ComGuard::new()?; - let policy: INetFwPolicy2 = unsafe { CoCreateInstance(&NetFwPolicy2, None, CLSCTX_INPROC_SERVER) } - .map_err(|error| t!("firewall.policyOpenFailed", error = error.to_string()).to_string())?; - let rules = unsafe { policy.Rules() } - .map_err(|error| t!("firewall.policyOpenFailed", error = error.to_string()).to_string())?; + let policy: INetFwPolicy2 = + unsafe { CoCreateInstance(&NetFwPolicy2, None, CLSCTX_INPROC_SERVER) }.map_err( + |error| t!("firewall.policyOpenFailed", error = error.to_string()).to_string(), + )?; + let rules = unsafe { policy.Rules() }.map_err(|error| { + t!("firewall.policyOpenFailed", error = error.to_string()).to_string() + })?; Ok(Self { rules, _com: com }) } fn enumerate_rules(&self) -> Result, FirewallError> { - let enumerator = unsafe { self.rules._NewEnum() } - .map_err(|error| t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string())?; - let enum_variant: IEnumVARIANT = enumerator - .cast() - .map_err(|error| t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string())?; + let enumerator = unsafe { self.rules._NewEnum() }.map_err(|error| { + t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string() + })?; + let enum_variant: IEnumVARIANT = enumerator.cast().map_err(|error| { + t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string() + })?; let mut results = Vec::new(); loop { let mut fetched = 0u32; let mut safe_variant = SafeVariant::new(); - let hr = unsafe { enum_variant.Next(std::slice::from_mut(safe_variant.as_mut()), &mut fetched) }; + let hr = unsafe { + enum_variant.Next(std::slice::from_mut(safe_variant.as_mut()), &mut fetched) + }; if hr == S_FALSE || fetched == 0 { break; } - hr.ok() - .map_err(|error| t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string())?; - - let dispatch = IDispatch::try_from(safe_variant.as_ref()) - .map_err(|error: windows::core::Error| t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string())?; - let rule: INetFwRule = dispatch - .cast() - .map_err(|error| t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string())?; + hr.ok().map_err(|error| { + t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string() + })?; + + let dispatch = IDispatch::try_from(safe_variant.as_ref()).map_err( + |error: windows::core::Error| { + t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string() + }, + )?; + let rule: INetFwRule = dispatch.cast().map_err(|error| { + t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string() + })?; results.push(rule); // SafeVariant will automatically call VariantClear when it goes out of scope @@ -100,7 +119,10 @@ impl FirewallStore { Ok(results) } - fn find_by_selector(&self, selector: &FirewallRule) -> Result, FirewallError> { + fn find_by_selector( + &self, + selector: &FirewallRule, + ) -> Result, FirewallError> { // HRESULT 0x80070002 is HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND), returned when the // rule name does not match any existing rule. const HRESULT_FILE_NOT_FOUND: HRESULT = HRESULT(0x80070002_u32 as i32); @@ -112,19 +134,34 @@ impl FirewallStore { match unsafe { self.rules.Item(&BSTR::from(lookup_name)) } { Ok(rule) => Ok(Some(rule)), Err(e) if e.code() == HRESULT_FILE_NOT_FOUND => Ok(None), - Err(e) => Err(t!("firewall.ruleLookupFailed", name = lookup_name, error = e.to_string()).to_string().into()), + Err(e) => Err(t!( + "firewall.ruleLookupFailed", + name = lookup_name, + error = e.to_string() + ) + .to_string() + .into()), } } fn remove_rule(&self, rule_name: &str) -> Result<(), FirewallError> { - unsafe { self.rules.Remove(&BSTR::from(rule_name)) } - .map_err(|error| t!("firewall.ruleRemoveFailed", name = rule_name, error = error.to_string()).to_string())?; + unsafe { self.rules.Remove(&BSTR::from(rule_name)) }.map_err(|error| { + t!( + "firewall.ruleRemoveFailed", + name = rule_name, + error = error.to_string() + ) + .to_string() + })?; Ok(()) } fn create_rule_object(&self) -> Result { - unsafe { CoCreateInstance(&NetFwRule, None, CLSCTX_INPROC_SERVER) } - .map_err(|error| t!("firewall.ruleCreateFailed", error = error.to_string()).to_string().into()) + unsafe { CoCreateInstance(&NetFwRule, None, CLSCTX_INPROC_SERVER) }.map_err(|error| { + t!("firewall.ruleCreateFailed", error = error.to_string()) + .to_string() + .into() + }) } } @@ -206,20 +243,47 @@ fn profiles_to_mask(values: &[String]) -> Result { "domain" => mask |= NET_FW_PROFILE2_DOMAIN.0, "private" => mask |= NET_FW_PROFILE2_PRIVATE.0, "public" => mask |= NET_FW_PROFILE2_PUBLIC.0, - _ => return Err(t!("firewall.invalidProfiles", value = value).to_string().into()), + _ => { + return Err(t!("firewall.invalidProfiles", value = value) + .to_string() + .into()); + } } } Ok(mask) } +fn rule_matches_unspecified_scope( + rule: &FirewallRule, + unspecified_rules: &UnspecifiedRules, +) -> Result { + if let Some(direction) = unspecified_rules.direction.as_ref() + && rule.direction.as_ref() != Some(direction) + { + return Ok(false); + } + + if let Some(profiles) = unspecified_rules.profiles.as_ref() { + let requested_mask = profiles_to_mask(profiles)?; + let rule_mask = profiles_to_mask(rule.profiles.as_deref().unwrap_or_default())?; + if requested_mask & rule_mask == 0 { + return Ok(false); + } + } + + Ok(true) +} + fn split_csv(value: Option) -> Option> { - value.map(|raw| { - raw.split(',') - .map(str::trim) - .filter(|entry| !entry.is_empty()) - .map(ToOwned::to_owned) - .collect::>() - }).filter(|items| !items.is_empty()) + value + .map(|raw| { + raw.split(',') + .map(str::trim) + .filter(|entry| !entry.is_empty()) + .map(ToOwned::to_owned) + .collect::>() + }) + .filter(|items| !items.is_empty()) } fn join_csv(value: &[String]) -> String { @@ -238,7 +302,11 @@ fn interface_types_to_string(values: &[String]) -> Result "remoteaccess" => normalized.push("RemoteAccess".to_string()), "wireless" => normalized.push("Wireless".to_string()), "lan" => normalized.push("Lan".to_string()), - _ => return Err(t!("firewall.invalidInterfaceType", value = value).to_string().into()), + _ => { + return Err(t!("firewall.invalidInterfaceType", value = value) + .to_string() + .into()); + } } } Ok(join_csv(&normalized)) @@ -251,21 +319,46 @@ fn protocol_supports_ports(protocol: i32) -> bool { fn validate_protocol(protocol: i32) -> Result<(), FirewallError> { // IANA protocol numbers 0-255 plus the Windows-specific 256 (Any) if !(0..=256).contains(&protocol) { - return Err(t!("firewall.invalidProtocol", value = protocol).to_string().into()); + return Err(t!("firewall.invalidProtocol", value = protocol) + .to_string() + .into()); } Ok(()) } fn map_update_err(name: &str) -> impl Fn(windows::core::Error) -> FirewallError + '_ { - move |error| t!("firewall.ruleUpdateFailed", name = name, error = error.to_string()).to_string().into() + move |error| { + t!( + "firewall.ruleUpdateFailed", + name = name, + error = error.to_string() + ) + .to_string() + .into() + } } fn map_read_err(name: &str) -> impl Fn(windows::core::Error) -> FirewallError + '_ { - move |error| t!("firewall.ruleReadFailed", name = name, error = error.to_string()).to_string().into() + move |error| { + t!( + "firewall.ruleReadFailed", + name = name, + error = error.to_string() + ) + .to_string() + .into() + } } fn rule_to_model(rule: &INetFwRule) -> Result { - let name = unsafe { rule.Name() }.map_err(|error| t!("firewall.ruleReadFailed", name = "", error = error.to_string()).to_string())?; + let name = unsafe { rule.Name() }.map_err(|error| { + t!( + "firewall.ruleReadFailed", + name = "", + error = error.to_string() + ) + .to_string() + })?; let name = name.to_string(); let err = map_read_err(&name); let profiles = profiles_from_mask(unsafe { rule.Profiles() }.map_err(&err)?); @@ -287,12 +380,18 @@ fn rule_to_model(rule: &INetFwRule) -> Result { enabled: Some(unsafe { rule.Enabled() }.map_err(&err)?.as_bool()), profiles: Some(profiles), grouping: bstr_to_option(unsafe { rule.Grouping() }.map_err(&err)?)?, - interface_types: split_csv(bstr_to_option(unsafe { rule.InterfaceTypes() }.map_err(&err)?)?), + interface_types: split_csv(bstr_to_option( + unsafe { rule.InterfaceTypes() }.map_err(&err)?, + )?), edge_traversal: Some(unsafe { rule.EdgeTraversal() }.map_err(&err)?.as_bool()), }) } -fn apply_rule_properties(rule: &INetFwRule, desired: &FirewallRule, existing_protocol: Option) -> Result<(), FirewallError> { +fn apply_rule_properties( + rule: &INetFwRule, + desired: &FirewallRule, + existing_protocol: Option, +) -> Result<(), FirewallError> { let name = desired.selector_name().unwrap_or(""); let err = map_update_err(name); @@ -315,20 +414,27 @@ fn apply_rule_properties(rule: &INetFwRule, desired: &FirewallRule, existing_pro // because the caller may only be setting local_ports or remote_ports. if let Some(protocol) = effective_protocol && !protocol_supports_ports(protocol) - && (desired.local_ports.is_some() || desired.remote_ports.is_some()) { - return Err(t!("firewall.portsNotAllowed", name = name, protocol = protocol).to_string().into()); - } + && (desired.local_ports.is_some() || desired.remote_ports.is_some()) + { + return Err( + t!("firewall.portsNotAllowed", name = name, protocol = protocol) + .to_string() + .into(), + ); + } if let Some(protocol) = desired.protocol { if let Some(current_protocol) = existing_protocol - && current_protocol != protocol && !protocol_supports_ports(protocol) { - if desired.local_ports.is_none() { - unsafe { rule.SetLocalPorts(&BSTR::from("")) }.map_err(&err)?; - } - if desired.remote_ports.is_none() { - unsafe { rule.SetRemotePorts(&BSTR::from("")) }.map_err(&err)?; - } + && current_protocol != protocol + && !protocol_supports_ports(protocol) + { + if desired.local_ports.is_none() { + unsafe { rule.SetLocalPorts(&BSTR::from("")) }.map_err(&err)?; + } + if desired.remote_ports.is_none() { + unsafe { rule.SetRemotePorts(&BSTR::from("")) }.map_err(&err)?; } + } unsafe { rule.SetProtocol(protocol) }.map_err(&err)?; } @@ -381,10 +487,6 @@ fn apply_rule_properties(rule: &INetFwRule, desired: &FirewallRule, existing_pro } pub fn get_rules(input: &FirewallRuleList) -> Result { - if input.rules.is_empty() { - return Err(t!("get.rulesArrayEmpty").to_string().into()); - } - let store = FirewallStore::open()?; let mut results = Vec::new(); @@ -399,7 +501,10 @@ pub fn get_rules(input: &FirewallRuleList) -> Result FirewallRule { @@ -407,29 +512,61 @@ fn project_rule(current: &FirewallRule, desired: &FirewallRule) -> FirewallRule name: current.name.clone(), exist: None, metadata: None, - description: desired.description.clone().or_else(|| current.description.clone()), - application_name: desired.application_name.clone().or_else(|| current.application_name.clone()), - service_name: desired.service_name.clone().or_else(|| current.service_name.clone()), + description: desired + .description + .clone() + .or_else(|| current.description.clone()), + application_name: desired + .application_name + .clone() + .or_else(|| current.application_name.clone()), + service_name: desired + .service_name + .clone() + .or_else(|| current.service_name.clone()), protocol: desired.protocol.or(current.protocol), - local_ports: desired.local_ports.clone().or_else(|| current.local_ports.clone()), - remote_ports: desired.remote_ports.clone().or_else(|| current.remote_ports.clone()), - local_addresses: desired.local_addresses.clone().or_else(|| current.local_addresses.clone()), - remote_addresses: desired.remote_addresses.clone().or_else(|| current.remote_addresses.clone()), - direction: desired.direction.clone().or_else(|| current.direction.clone()), + local_ports: desired + .local_ports + .clone() + .or_else(|| current.local_ports.clone()), + remote_ports: desired + .remote_ports + .clone() + .or_else(|| current.remote_ports.clone()), + local_addresses: desired + .local_addresses + .clone() + .or_else(|| current.local_addresses.clone()), + remote_addresses: desired + .remote_addresses + .clone() + .or_else(|| current.remote_addresses.clone()), + direction: desired + .direction + .clone() + .or_else(|| current.direction.clone()), action: desired.action.clone().or_else(|| current.action.clone()), enabled: desired.enabled.or(current.enabled), - profiles: desired.profiles.clone().or_else(|| current.profiles.clone()), - grouping: desired.grouping.clone().or_else(|| current.grouping.clone()), - interface_types: desired.interface_types.clone().or_else(|| current.interface_types.clone()), + profiles: desired + .profiles + .clone() + .or_else(|| current.profiles.clone()), + grouping: desired + .grouping + .clone() + .or_else(|| current.grouping.clone()), + interface_types: desired + .interface_types + .clone() + .or_else(|| current.interface_types.clone()), edge_traversal: desired.edge_traversal.or(current.edge_traversal), } } -pub fn set_rules(input: &FirewallRuleList, what_if: bool) -> Result { - if input.rules.is_empty() { - return Err(t!("set.rulesArrayEmpty").to_string().into()); - } - +pub fn set_rules( + input: &FirewallRuleList, + what_if: bool, +) -> Result { let store = FirewallStore::open()?; let mut results = Vec::new(); @@ -441,12 +578,20 @@ pub fn set_rules(input: &FirewallRuleList, what_if: bool) -> Result { let current = rule_to_model(&rule)?; - let rule_name = current.name.clone().unwrap_or_else(|| desired.selector_name().unwrap_or_default().to_string()); + let rule_name = current + .name + .clone() + .unwrap_or_else(|| desired.selector_name().unwrap_or_default().to_string()); if desired.exist == Some(false) { if what_if { let mut projected = desired.missing_from_input(); - projected.metadata = Some(Metadata { what_if: Some(vec![t!("firewall_helper.whatIfRemoveRule", name = rule_name).to_string()]) }); + projected.metadata = Some(Metadata { + what_if: Some(vec![ + t!("firewall_helper.whatIfRemoveRule", name = rule_name) + .to_string(), + ]), + }); results.push(projected); } else { store.remove_rule(&rule_name)?; @@ -468,39 +613,71 @@ pub fn set_rules(input: &FirewallRuleList, what_if: bool) -> Result"), error = "created rule not found").to_string())?; + .ok_or_else(|| { + t!( + "firewall.ruleLookupFailed", + name = desired.selector_name().unwrap_or(""), + error = "created rule not found" + ) + .to_string() + })?; results.push(rule_to_model(&created)?); } } } } - // Handle unspecified_rules_action: Disable or Remove rules not explicitly listed - match &input.unspecified_rules_action { - Some(UnspecifiedRulesAction::Disable) | Some(UnspecifiedRulesAction::Remove) => { - let is_remove = matches!(&input.unspecified_rules_action, Some(UnspecifiedRulesAction::Remove)); - let specified_names: std::collections::HashSet = input.rules.iter() + // Disable or remove rules that aren't explicitly listed and match the requested scope. + match &input.unspecified_rules { + Some(unspecified_rules) + if matches!( + unspecified_rules.action, + UnspecifiedRuleAction::Disable | UnspecifiedRuleAction::Remove + ) => + { + let is_remove = unspecified_rules.action == UnspecifiedRuleAction::Remove; + let specified_names: std::collections::HashSet = input + .rules + .iter() .filter_map(|r| r.selector_name().map(|n| n.to_ascii_lowercase())) .collect(); @@ -516,19 +693,34 @@ pub fn set_rules(input: &FirewallRuleList, what_if: bool) -> Result Result Result {} // None or Ignore — no additional action + _ => {} // None or Ignore: no additional action. } - Ok(FirewallRuleList { rules: results, unspecified_rules_action: input.unspecified_rules_action.clone() }) + Ok(FirewallRuleList { + rules: results, + unspecified_rules: input.unspecified_rules.clone(), + }) } pub fn export_rules() -> Result { @@ -566,5 +769,67 @@ pub fn export_rules() -> Result { results.push(rule_to_model(&rule)?); } - Ok(FirewallRuleList { rules: results, unspecified_rules_action: None }) + Ok(FirewallRuleList { + rules: results, + unspecified_rules: None, + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn rule(direction: RuleDirection, profiles: &[&str]) -> FirewallRule { + FirewallRule { + direction: Some(direction), + profiles: Some( + profiles + .iter() + .map(|profile| (*profile).to_string()) + .collect(), + ), + ..FirewallRule::default() + } + } + + fn scope(direction: Option, profiles: Option<&[&str]>) -> UnspecifiedRules { + UnspecifiedRules { + action: UnspecifiedRuleAction::Disable, + direction, + profiles: profiles.map(|values| { + values + .iter() + .map(|profile| (*profile).to_string()) + .collect() + }), + } + } + + #[test] + fn unspecified_rule_scope_combines_direction_and_profiles() { + let filter = scope(Some(RuleDirection::Inbound), Some(&["Domain"])); + + assert!( + rule_matches_unspecified_scope(&rule(RuleDirection::Inbound, &["Domain"]), &filter) + .unwrap() + ); + assert!( + !rule_matches_unspecified_scope(&rule(RuleDirection::Outbound, &["Domain"]), &filter) + .unwrap() + ); + assert!( + !rule_matches_unspecified_scope(&rule(RuleDirection::Inbound, &["Private"]), &filter) + .unwrap() + ); + } + + #[test] + fn unspecified_rule_profile_scope_intersects_all_profiles() { + let filter = scope(None, Some(&["Domain"])); + + assert!( + rule_matches_unspecified_scope(&rule(RuleDirection::Inbound, &["All"]), &filter) + .unwrap() + ); + } } diff --git a/resources/windows_firewall/src/types.rs b/resources/windows_firewall/src/types.rs index bd0a3dd4d..1567c4650 100644 --- a/resources/windows_firewall/src/types.rs +++ b/resources/windows_firewall/src/types.rs @@ -23,17 +23,29 @@ pub enum RuleAction { #[derive(Debug, Serialize, Deserialize, PartialEq, Clone)] #[serde(rename_all = "camelCase")] -pub enum UnspecifiedRulesAction { +pub enum UnspecifiedRuleAction { Ignore, Disable, Remove, } +#[derive(Debug, Serialize, Deserialize, Clone)] +#[serde(rename_all = "camelCase")] +pub struct UnspecifiedRules { + pub action: UnspecifiedRuleAction, + + #[serde(skip_serializing_if = "Option::is_none")] + pub direction: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + pub profiles: Option>, +} + #[derive(Debug, Default, Serialize, Deserialize, Clone)] #[serde(rename_all = "camelCase")] pub struct FirewallRuleList { #[serde(skip_serializing_if = "Option::is_none")] - pub unspecified_rules_action: Option, + pub unspecified_rules: Option, pub rules: Vec, } @@ -135,6 +147,22 @@ impl From for FirewallError { #[cfg(windows)] impl From for FirewallError { fn from(error: windows::core::Error) -> Self { - Self { message: error.to_string() } + Self { + message: error.to_string(), + } + } +} + +#[cfg(test)] +mod tests { + use super::FirewallRuleList; + + #[test] + fn unspecified_rules_requires_action() { + let result = serde_json::from_str::( + r#"{"unspecifiedRules":{"direction":"Inbound"},"rules":[]}"#, + ); + + assert!(result.is_err()); } } diff --git a/resources/windows_firewall/tests/windows_firewall_get.tests.ps1 b/resources/windows_firewall/tests/windows_firewall_get.tests.ps1 index daf95e307..0ca97677a 100644 --- a/resources/windows_firewall/tests/windows_firewall_get.tests.ps1 +++ b/resources/windows_firewall/tests/windows_firewall_get.tests.ps1 @@ -56,10 +56,11 @@ Describe 'Microsoft.Windows/FirewallRuleList - get operation' -Skip:(!$IsWindows $result.PSObject.Properties.Name | Should -Not -Contain 'direction' } - It 'fails when rules array is empty' { + It 'accepts an empty rules array' { $json = '{"rules":[]}' - $out = $json | dsc resource get -r $resourceType -f - 2>&1 - $LASTEXITCODE | Should -Not -Be 0 + $out = $json | dsc resource get -r $resourceType -f - 2>$testdrive/error.log + $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) + ($out | ConvertFrom-Json).actualState.rules | Should -BeNullOrEmpty } It 'handles multiple rules in a single request' { diff --git a/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 b/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 index 607236491..ae9861fa5 100644 --- a/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 +++ b/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 @@ -46,10 +46,11 @@ Describe 'Microsoft.Windows/FirewallRuleList - set operation' -Skip:(!$isElevate $LASTEXITCODE | Should -Not -Be 0 } - It 'fails when rules array is empty' -Skip:(!$isElevated) { + It 'accepts an empty rules array' -Skip:(!$isElevated) { $json = '{"rules":[]}' - $out = $json | dsc resource set -r $resourceType -f - 2>&1 - $LASTEXITCODE | Should -Not -Be 0 + $out = $json | dsc resource set -r $resourceType -f - 2>$testdrive/error.log + $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) + ($out | ConvertFrom-Json).afterState.rules | Should -BeNullOrEmpty } It 'updates an existing rule' -Skip:(!$isElevated) { @@ -146,7 +147,7 @@ Describe 'Microsoft.Windows/FirewallRuleList - set operation' -Skip:(!$isElevate } } -Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRulesAction (what-if)' -Skip:(!$isElevated) { +Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRules (what-if)' -Skip:(!$isElevated) { BeforeDiscovery { $isElevated = if ($IsWindows) { ([Security.Principal.WindowsPrincipal][Security.Principal.WindowsIdentity]::GetCurrent()).IsInRole( @@ -158,6 +159,10 @@ Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRulesAction (what-if)' BeforeAll { $testRuleName = 'DSC-WindowsFirewall-Unspecified-Test' + $inboundDomainRule = 'DSC-WindowsFirewall-Scope-Inbound-Domain' + $outboundDomainRule = 'DSC-WindowsFirewall-Scope-Outbound-Domain' + $inboundPrivateRule = 'DSC-WindowsFirewall-Scope-Inbound-Private' + $allProfilesRule = 'DSC-WindowsFirewall-Scope-Inbound-All' function Initialize-TestFirewallRule { $existing = Get-NetFirewallRule -Name $testRuleName -ErrorAction SilentlyContinue @@ -168,19 +173,49 @@ Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRulesAction (what-if)' } } + function Initialize-ScopeFirewallRules { + Remove-NetFirewallRule -Name 'DSC-WindowsFirewall-Scope-*' -ErrorAction SilentlyContinue + New-NetFirewallRule -Name $inboundDomainRule -DisplayName $inboundDomainRule -Direction Inbound -Profile Domain -Action Allow -Enabled True | Out-Null + New-NetFirewallRule -Name $outboundDomainRule -DisplayName $outboundDomainRule -Direction Outbound -Profile Domain -Action Allow -Enabled True | Out-Null + New-NetFirewallRule -Name $inboundPrivateRule -DisplayName $inboundPrivateRule -Direction Inbound -Profile Private -Action Allow -Enabled True | Out-Null + New-NetFirewallRule -Name $allProfilesRule -DisplayName $allProfilesRule -Direction Inbound -Profile Any -Action Allow -Enabled True | Out-Null + } + + function Get-UnspecifiedWhatIfRuleNames { + param( + [Parameter(Mandatory)] + [hashtable]$UnspecifiedRules, + + [array]$Rules = @() + ) + + $json = @{ + unspecifiedRules = $UnspecifiedRules + rules = $Rules + } | ConvertTo-Json -Compress -Depth 5 + + $result = windows_firewall set -w --input $json 2>$testdrive/error.log | ConvertFrom-Json + $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) + + return @($result.rules | + Where-Object { $_._metadata.whatIf -match 'Would (disable|remove) unspecified firewall rule' } | + ForEach-Object { $_.name }) + } + Initialize-TestFirewallRule } AfterAll { Remove-NetFirewallRule -Name $testRuleName -ErrorAction SilentlyContinue + Remove-NetFirewallRule -Name 'DSC-WindowsFirewall-Scope-*' -ErrorAction SilentlyContinue } - It 'does not affect unspecified rules when unspecifiedRulesAction is ignore' -Skip:(!$isElevated) { + It 'does not affect unspecified rules when action is ignore' -Skip:(!$isElevated) { Initialize-TestFirewallRule # Specify a different rule name so $testRuleName is "unspecified" $json = @{ - unspecifiedRulesAction = 'ignore' + unspecifiedRules = @{ action = 'ignore' } rules = @(@{ name = 'SomeOtherRuleThatMayNotExist' direction = 'Inbound' @@ -198,7 +233,7 @@ Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRulesAction (what-if)' $unspecifiedEntries | Should -BeNullOrEmpty } - It 'does not affect unspecified rules when unspecifiedRulesAction is omitted' -Skip:(!$isElevated) { + It 'does not affect unspecified rules when unspecifiedRules is omitted' -Skip:(!$isElevated) { Initialize-TestFirewallRule $json = @{ @@ -219,12 +254,121 @@ Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRulesAction (what-if)' $unspecifiedEntries | Should -BeNullOrEmpty } - It 'reports would disable unspecified rules when unspecifiedRulesAction is disable' -Skip:(!$isElevated) { + It 'requires action when unspecifiedRules is used' -Skip:(!$isElevated) { + $json = @{ + unspecifiedRules = @{ direction = 'Inbound' } + rules = @() + } | ConvertTo-Json -Compress -Depth 5 + + $json | dsc resource set -r 'Microsoft.Windows/FirewallRuleList' -f - 2>$testdrive/error.log | Out-Null + $LASTEXITCODE | Should -Not -Be 0 + Get-Content -Raw $testdrive/error.log | Should -Match 'action' + } + + It 'filters unspecified rules by direction' -ForEach @( + @{ + Direction = 'Inbound' + IncludedRules = @( + 'DSC-WindowsFirewall-Scope-Inbound-Domain' + 'DSC-WindowsFirewall-Scope-Inbound-Private' + ) + ExcludedRule = 'DSC-WindowsFirewall-Scope-Outbound-Domain' + } + @{ + Direction = 'Outbound' + IncludedRules = @('DSC-WindowsFirewall-Scope-Outbound-Domain') + ExcludedRule = 'DSC-WindowsFirewall-Scope-Inbound-Domain' + } + ) -Skip:(!$isElevated) { + Initialize-ScopeFirewallRules + + $affectedNames = Get-UnspecifiedWhatIfRuleNames -UnspecifiedRules @{ + action = 'disable' + direction = $Direction + } + + foreach ($includedRule in $IncludedRules) { + $affectedNames | Should -Contain $includedRule + } + $affectedNames | Should -Not -Contain $ExcludedRule + } + + It 'filters unspecified rules by profiles and includes rules that apply to all profiles' -Skip:(!$isElevated) { + Initialize-ScopeFirewallRules + + $affectedNames = Get-UnspecifiedWhatIfRuleNames -UnspecifiedRules @{ + action = 'disable' + profiles = @('Domain') + } + + $affectedNames | Should -Contain $inboundDomainRule + $affectedNames | Should -Contain $outboundDomainRule + $affectedNames | Should -Contain $allProfilesRule + $affectedNames | Should -Not -Contain $inboundPrivateRule + } + + It 'matches any profile listed in the profiles filter' -Skip:(!$isElevated) { + Initialize-ScopeFirewallRules + + $affectedNames = Get-UnspecifiedWhatIfRuleNames -UnspecifiedRules @{ + action = 'disable' + profiles = @('Domain', 'Private') + } + + $affectedNames | Should -Contain $inboundDomainRule + $affectedNames | Should -Contain $outboundDomainRule + $affectedNames | Should -Contain $inboundPrivateRule + } + + It 'combines direction and profiles when filtering unspecified rules' -Skip:(!$isElevated) { + Initialize-ScopeFirewallRules + + $affectedNames = Get-UnspecifiedWhatIfRuleNames -UnspecifiedRules @{ + action = 'disable' + direction = 'Inbound' + profiles = @('Domain') + } + + $affectedNames | Should -Contain $inboundDomainRule + $affectedNames | Should -Contain $allProfilesRule + $affectedNames | Should -Not -Contain $outboundDomainRule + $affectedNames | Should -Not -Contain $inboundPrivateRule + } + + It 'applies remove to an empty rules list only within the filtered scope' -Skip:(!$isElevated) { + Initialize-ScopeFirewallRules + + $affectedNames = Get-UnspecifiedWhatIfRuleNames -UnspecifiedRules @{ + action = 'remove' + direction = 'Outbound' + profiles = @('Domain') + } + + $affectedNames | Should -Contain $outboundDomainRule + $affectedNames | Should -Not -Contain $inboundDomainRule + $affectedNames | Should -Not -Contain $inboundPrivateRule + $affectedNames | Should -Not -Contain $allProfilesRule + } + + It 'does not act on a declared rule that matches the unspecified rule scope' -Skip:(!$isElevated) { + Initialize-ScopeFirewallRules + + $affectedNames = Get-UnspecifiedWhatIfRuleNames -UnspecifiedRules @{ + action = 'disable' + direction = 'Inbound' + profiles = @('Domain') + } -Rules @(@{ name = $inboundDomainRule }) + + $affectedNames | Should -Not -Contain $inboundDomainRule + $affectedNames | Should -Contain $allProfilesRule + } + + It 'reports would disable unspecified rules when action is disable' -Skip:(!$isElevated) { Initialize-TestFirewallRule # Specify only testRuleName; all other rules are "unspecified" and should be disabled $json = @{ - unspecifiedRulesAction = 'disable' + unspecifiedRules = @{ action = 'disable' } rules = @(@{ name = $testRuleName enabled = $true @@ -252,7 +396,7 @@ Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRulesAction (what-if)' $actual.Enabled | Should -Be 'True' } - It 'skips already-disabled rules when unspecifiedRulesAction is disable' -Skip:(!$isElevated) { + It 'skips already-disabled rules when action is disable' -Skip:(!$isElevated) { Initialize-TestFirewallRule # Disable the test rule so it is already disabled Set-NetFirewallRule -Name $testRuleName -Enabled False @@ -262,7 +406,7 @@ Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRulesAction (what-if)' New-NetFirewallRule -Name $otherRuleName -DisplayName $otherRuleName -Direction Inbound -Action Allow -Protocol TCP -LocalPort 32790 -Enabled True -ErrorAction SilentlyContinue | Out-Null $json = @{ - unspecifiedRulesAction = 'disable' + unspecifiedRules = @{ action = 'disable' } rules = @(@{ name = $otherRuleName enabled = $true @@ -279,7 +423,7 @@ Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRulesAction (what-if)' Remove-NetFirewallRule -Name $otherRuleName -ErrorAction SilentlyContinue } - It 'reports would remove unspecified rules when unspecifiedRulesAction is remove' -Skip:(!$isElevated) { + It 'reports would remove unspecified rules when action is remove' -Skip:(!$isElevated) { Initialize-TestFirewallRule # Specify a different rule so testRuleName is "unspecified" @@ -287,7 +431,7 @@ Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRulesAction (what-if)' $knownRule = (Get-NetFirewallRule | Select-Object -First 1).Name $json = @{ - unspecifiedRulesAction = 'remove' + unspecifiedRules = @{ action = 'remove' } rules = @(@{ name = $knownRule enabled = $true diff --git a/resources/windows_firewall/windows_firewall.dsc.resource.json b/resources/windows_firewall/windows_firewall.dsc.resource.json index 6a587fcd9..ebac33af1 100644 --- a/resources/windows_firewall/windows_firewall.dsc.resource.json +++ b/resources/windows_firewall/windows_firewall.dsc.resource.json @@ -6,7 +6,7 @@ "Windows", "Firewall" ], - "version": "0.2.1", + "version": "0.3.0", "get": { "executable": "windows_firewall", "args": [ @@ -59,16 +59,49 @@ "rules" ], "properties": { - "unspecifiedRulesAction": { - "type": "string", - "title": "Unspecified rules action", - "description": "The action to take on firewall rules not explicitly listed in the rules array. 'ignore' (default) leaves them unchanged, 'disable' disables them, and 'remove' deletes them.", - "default": "ignore", - "enum": [ - "ignore", - "disable", - "remove" - ] + "unspecifiedRules": { + "type": "object", + "title": "Unspecified rules", + "description": "Defines the action and optional scope for firewall rules not explicitly listed in the rules array. When both direction and profiles are specified, a rule must match both filters.", + "additionalProperties": false, + "required": [ + "action" + ], + "properties": { + "action": { + "type": "string", + "title": "Action", + "description": "The action to take on matching unspecified firewall rules. 'ignore' leaves them unchanged, 'disable' disables them, and 'remove' deletes them.", + "enum": [ + "ignore", + "disable", + "remove" + ] + }, + "direction": { + "type": "string", + "title": "Direction", + "description": "Limits the action to unspecified rules with this traffic direction.", + "enum": [ + "Inbound", + "Outbound" + ] + }, + "profiles": { + "type": "array", + "title": "Profiles", + "description": "Limits the action to unspecified rules that apply to any of these firewall profiles.", + "items": { + "type": "string", + "enum": [ + "Domain", + "Private", + "Public", + "All" + ] + } + } + } }, "rules": { "type": "array", From ecec15ffb5eb1c26eea704b7d9e90da7356b0d5f Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL HE/HIM) (from Dev Box)" Date: Tue, 11 Aug 2026 16:34:52 -0700 Subject: [PATCH 2/7] Address firewall scope review feedback Reject empty unspecified rule profile filters in the schema and runtime, and localize the VariantClear warning. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- resources/windows_firewall/locales/en-us.toml | 2 ++ resources/windows_firewall/src/firewall.rs | 24 +++++++++++++++---- .../tests/windows_firewall_set.tests.ps1 | 14 +++++++++++ .../windows_firewall.dsc.resource.json | 1 + 4 files changed, 37 insertions(+), 4 deletions(-) diff --git a/resources/windows_firewall/locales/en-us.toml b/resources/windows_firewall/locales/en-us.toml index 36ab2a559..3ed8e9e6f 100644 --- a/resources/windows_firewall/locales/en-us.toml +++ b/resources/windows_firewall/locales/en-us.toml @@ -26,8 +26,10 @@ ruleUpdateFailed = "Failed to update firewall rule '%{name}': %{error}" ruleReadFailed = "Failed to read firewall rule '%{name}': %{error}" portsNotAllowed = "Ports cannot be specified for firewall rule '%{name}' because protocol %{protocol} does not support ports" invalidProfiles = "Invalid profiles value '%{value}'. Valid values are Domain, Private, Public, or All" +emptyUnspecifiedProfiles = "The unspecified rules profiles filter cannot be empty" invalidInterfaceType = "Invalid interface type '%{value}'. Valid values are RemoteAccess, Wireless, Lan, or All" invalidProtocol = "Invalid protocol number '%{value}'. Must be between 0 and 256" +variantClearFailed = "Warning: VariantClear failed with HRESULT: %{hresult}" [firewall_helper] whatIfCreateRule = "Would create firewall rule '%{name}'" diff --git a/resources/windows_firewall/src/firewall.rs b/resources/windows_firewall/src/firewall.rs index 26a7c0e7b..1c0c82b7c 100644 --- a/resources/windows_firewall/src/firewall.rs +++ b/resources/windows_firewall/src/firewall.rs @@ -38,10 +38,13 @@ impl SafeVariant { impl Drop for SafeVariant { fn drop(&mut self) { if let Err(e) = unsafe { VariantClear(&mut self.0) } { - crate::write_error(&format!( - "Warning: VariantClear failed with HRESULT: {:#010x}", - e.code().0 as u32 - )); + crate::write_error( + t!( + "firewall.variantClearFailed", + hresult = format!("{:#010x}", e.code().0 as u32) + ) + .as_ref(), + ); } } } @@ -264,6 +267,9 @@ fn rule_matches_unspecified_scope( } if let Some(profiles) = unspecified_rules.profiles.as_ref() { + if profiles.is_empty() { + return Err(t!("firewall.emptyUnspecifiedProfiles").to_string().into()); + } let requested_mask = profiles_to_mask(profiles)?; let rule_mask = profiles_to_mask(rule.profiles.as_deref().unwrap_or_default())?; if requested_mask & rule_mask == 0 { @@ -832,4 +838,14 @@ mod tests { .unwrap() ); } + + #[test] + fn unspecified_rule_profile_scope_rejects_empty_filter() { + let filter = scope(None, Some(&[])); + + assert!( + rule_matches_unspecified_scope(&rule(RuleDirection::Inbound, &["Domain"]), &filter) + .is_err() + ); + } } diff --git a/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 b/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 index ae9861fa5..4cc1dcdc4 100644 --- a/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 +++ b/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 @@ -265,6 +265,20 @@ Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRules (what-if)' -Skip Get-Content -Raw $testdrive/error.log | Should -Match 'action' } + It 'rejects an empty unspecifiedRules profiles filter' -Skip:(!$isElevated) { + $json = @{ + unspecifiedRules = @{ + action = 'disable' + profiles = @() + } + rules = @() + } | ConvertTo-Json -Compress -Depth 5 + + $json | dsc resource set -r 'Microsoft.Windows/FirewallRuleList' -f - 2>$testdrive/error.log | Out-Null + $LASTEXITCODE | Should -Not -Be 0 + Get-Content -Raw $testdrive/error.log | Should -Match 'profiles' + } + It 'filters unspecified rules by direction' -ForEach @( @{ Direction = 'Inbound' diff --git a/resources/windows_firewall/windows_firewall.dsc.resource.json b/resources/windows_firewall/windows_firewall.dsc.resource.json index ebac33af1..58c6f7dc5 100644 --- a/resources/windows_firewall/windows_firewall.dsc.resource.json +++ b/resources/windows_firewall/windows_firewall.dsc.resource.json @@ -91,6 +91,7 @@ "type": "array", "title": "Profiles", "description": "Limits the action to unspecified rules that apply to any of these firewall profiles.", + "minItems": 1, "items": { "type": "string", "enum": [ From 448be82ba31755b913d1e60f826694c2ca0bea53 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL HE/HIM) (from Dev Box)" Date: Tue, 11 Aug 2026 17:22:39 -0700 Subject: [PATCH 3/7] Fix platform-specific changed coverage Merge coverage from every platform when measuring changed Rust code while retaining Linux-only data for the full-codebase metric. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/rust.yml | 46 +++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index bc40ac391..aa200bbc5 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -304,11 +304,16 @@ jobs: coverage-report: if: github.event_name == 'pull_request' - # Use Linux coverage only: merging all platforms inflates total line count - # because each platform has platform-specific source files (Windows adds ~4500 - # lines from registry/service/DISM resources). Single-platform coverage matches - # local `build.ps1 -codecoverage` results and avoids misleadingly low percentages. - needs: [linux-build, linux-pester] + # Use all platforms for changed-code coverage so platform-specific files are + # included. Keep full-codebase coverage Linux-only to avoid inflating its + # denominator with platform-specific sources. + needs: + - linux-build + - linux-pester + - macos-build + - macos-pester + - windows-build + - windows-pester runs-on: ubuntu-latest permissions: pull-requests: write @@ -321,7 +326,7 @@ jobs: - name: Download coverage artifacts uses: actions/download-artifact@v4 with: - pattern: 'linux*coverage' + pattern: '*coverage' path: coverage-data - name: Consolidate coverage data @@ -339,21 +344,33 @@ jobs: $baseSha = $mergeBase } - # Find all available lcov.info files from coverage artifacts + # Changed-code coverage uses every platform so platform-specific Rust + # files are analyzed. Full-codebase coverage remains Linux-only to + # avoid inflating its denominator with platform-specific sources. $lcovFiles = Get-ChildItem -Path 'coverage-data' -Filter 'lcov.info' -Recurse $pesterLcovFiles = Get-ChildItem -Path 'coverage-data' -Filter 'pester-lcov.info' -Recurse $allLcovFiles = @($lcovFiles) + @($pesterLcovFiles) | Where-Object { $_ } + $linuxLcovFiles = @($allLcovFiles | Where-Object { + $_.FullName -match '[/\\]linux-[^/\\]+-coverage[/\\]' + -or $_.FullName -match '[/\\]linux-coverage[/\\]' + }) if ($allLcovFiles.Count -eq 0) { Write-Warning 'No coverage data found from any platform.' "coverage_failed=true" | Out-File -Append -Encoding utf8 -FilePath $env:GITHUB_OUTPUT return } + if ($linuxLcovFiles.Count -eq 0) { + Write-Warning 'No Linux coverage data found for the full-codebase report.' + "coverage_failed=true" | Out-File -Append -Encoding utf8 -FilePath $env:GITHUB_OUTPUT + return + } "coverage_failed=false" | Out-File -Append -Encoding utf8 -FilePath $env:GITHUB_OUTPUT - Write-Verbose -Verbose "Found $($allLcovFiles.Count) LCOV file(s) to merge" + Write-Verbose -Verbose "Found $($allLcovFiles.Count) cross-platform LCOV file(s)" + Write-Verbose -Verbose "Found $($linuxLcovFiles.Count) Linux LCOV file(s)" - # Merge all LCOV files into a single consolidated report + # Merge all platforms for changed-code coverage. $mergedLcovPath = Join-Path $PWD 'merged-lcov.info' if ($allLcovFiles.Count -eq 1) { Copy-Item -Path $allLcovFiles[0].FullName -Destination $mergedLcovPath @@ -361,8 +378,15 @@ jobs: Merge-LcovFile -Path ($allLcovFiles | ForEach-Object { $_.FullName }) -OutputPath $mergedLcovPath -Verbose } - # Full codebase coverage report (always computed) - $fullReport = Get-FullCodeCoverageReport -LcovPath $mergedLcovPath -Verbose + # Merge Linux coverage separately for the full-codebase report. + $linuxMergedLcovPath = Join-Path $PWD 'linux-merged-lcov.info' + if ($linuxLcovFiles.Count -eq 1) { + Copy-Item -Path $linuxLcovFiles[0].FullName -Destination $linuxMergedLcovPath + } else { + Merge-LcovFile -Path ($linuxLcovFiles | ForEach-Object { $_.FullName }) -OutputPath $linuxMergedLcovPath -Verbose + } + + $fullReport = Get-FullCodeCoverageReport -LcovPath $linuxMergedLcovPath -Verbose "full_percentage=$($fullReport.Percentage)" | Out-File -Append -Encoding utf8 -FilePath $env:GITHUB_OUTPUT "full_covered=$($fullReport.CoveredLines)" | Out-File -Append -Encoding utf8 -FilePath $env:GITHUB_OUTPUT From 9d69854aa131434f768742e4a4a714b29616c559 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL HE/HIM) (from Dev Box)" Date: Tue, 11 Aug 2026 20:00:20 -0700 Subject: [PATCH 4/7] Fix cross-platform coverage reporting Correct the PowerShell coverage artifact predicate and initialize firewall Pester skip conditions before Describe discovery so elevated Windows CI executes the suites. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/rust.yml | 4 +-- .../tests/windows_firewall_set.tests.ps1 | 25 ++++++------------- .../tests/windows_firewall_whatif.tests.ps1 | 18 ++++++------- 3 files changed, 19 insertions(+), 28 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index aa200bbc5..ab97e1893 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -351,8 +351,8 @@ jobs: $pesterLcovFiles = Get-ChildItem -Path 'coverage-data' -Filter 'pester-lcov.info' -Recurse $allLcovFiles = @($lcovFiles) + @($pesterLcovFiles) | Where-Object { $_ } $linuxLcovFiles = @($allLcovFiles | Where-Object { - $_.FullName -match '[/\\]linux-[^/\\]+-coverage[/\\]' - -or $_.FullName -match '[/\\]linux-coverage[/\\]' + ($_.FullName -match '[/\\]linux-[^/\\]+-coverage[/\\]') -or + ($_.FullName -match '[/\\]linux-coverage[/\\]') }) if ($allLcovFiles.Count -eq 0) { diff --git a/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 b/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 index 4cc1dcdc4..f39bcb175 100644 --- a/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 +++ b/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 @@ -1,16 +1,16 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. -Describe 'Microsoft.Windows/FirewallRuleList - set operation' -Skip:(!$isElevated) { - BeforeDiscovery { - $isElevated = if ($IsWindows) { - ([Security.Principal.WindowsPrincipal][Security.Principal.WindowsIdentity]::GetCurrent()).IsInRole( - [Security.Principal.WindowsBuiltInRole]::Administrator) - } else { - $false - } +BeforeDiscovery { + $isElevated = if ($IsWindows) { + ([Security.Principal.WindowsPrincipal][Security.Principal.WindowsIdentity]::GetCurrent()).IsInRole( + [Security.Principal.WindowsBuiltInRole]::Administrator) + } else { + $false } +} +Describe 'Microsoft.Windows/FirewallRuleList - set operation' -Skip:(!$isElevated) { BeforeAll { $resourceType = 'Microsoft.Windows/FirewallRuleList' $testRuleName = 'DSC-WindowsFirewall-Set-Test' @@ -148,15 +148,6 @@ Describe 'Microsoft.Windows/FirewallRuleList - set operation' -Skip:(!$isElevate } Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRules (what-if)' -Skip:(!$isElevated) { - BeforeDiscovery { - $isElevated = if ($IsWindows) { - ([Security.Principal.WindowsPrincipal][Security.Principal.WindowsIdentity]::GetCurrent()).IsInRole( - [Security.Principal.WindowsBuiltInRole]::Administrator) - } else { - $false - } - } - BeforeAll { $testRuleName = 'DSC-WindowsFirewall-Unspecified-Test' $inboundDomainRule = 'DSC-WindowsFirewall-Scope-Inbound-Domain' diff --git a/resources/windows_firewall/tests/windows_firewall_whatif.tests.ps1 b/resources/windows_firewall/tests/windows_firewall_whatif.tests.ps1 index 5bd0f085e..f04f0f046 100644 --- a/resources/windows_firewall/tests/windows_firewall_whatif.tests.ps1 +++ b/resources/windows_firewall/tests/windows_firewall_whatif.tests.ps1 @@ -1,17 +1,17 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. -Describe 'windows_firewall config whatif tests' -Skip:(!$isElevated -or !$hasNetSecurity) { - BeforeDiscovery { - $isElevated = if ($IsWindows) { - ([Security.Principal.WindowsPrincipal][Security.Principal.WindowsIdentity]::GetCurrent()).IsInRole( - [Security.Principal.WindowsBuiltInRole]::Administrator) - } else { - $false - } - $hasNetSecurity = $null -ne (Get-Command 'Get-NetFirewallRule' -ErrorAction SilentlyContinue) +BeforeDiscovery { + $isElevated = if ($IsWindows) { + ([Security.Principal.WindowsPrincipal][Security.Principal.WindowsIdentity]::GetCurrent()).IsInRole( + [Security.Principal.WindowsBuiltInRole]::Administrator) + } else { + $false } + $hasNetSecurity = $null -ne (Get-Command 'Get-NetFirewallRule' -ErrorAction SilentlyContinue) +} +Describe 'windows_firewall config whatif tests' -Skip:(!$isElevated -or !$hasNetSecurity) { BeforeAll { $testRuleName = 'DSC-WindowsFirewall-WhatIf-Test' From 38bfb274a18eb2947e9e65fe489752851d303159 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL HE/HIM) (from Dev Box)" Date: Tue, 11 Aug 2026 21:29:20 -0700 Subject: [PATCH 5/7] Guard firewall tests on NetSecurity Skip firewall set and what-if suites when any cmdlet required for setup or cleanup is unavailable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../tests/windows_firewall_set.tests.ps1 | 10 ++++++++-- .../tests/windows_firewall_whatif.tests.ps1 | 6 +++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 b/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 index f39bcb175..dfa348901 100644 --- a/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 +++ b/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 @@ -8,9 +8,15 @@ BeforeDiscovery { } else { $false } + $hasNetSecurity = @( + 'Get-NetFirewallRule' + 'New-NetFirewallRule' + 'Remove-NetFirewallRule' + 'Set-NetFirewallRule' + ).Where({ $null -eq (Get-Command $_ -ErrorAction SilentlyContinue) }).Count -eq 0 } -Describe 'Microsoft.Windows/FirewallRuleList - set operation' -Skip:(!$isElevated) { +Describe 'Microsoft.Windows/FirewallRuleList - set operation' -Skip:(!$isElevated -or !$hasNetSecurity) { BeforeAll { $resourceType = 'Microsoft.Windows/FirewallRuleList' $testRuleName = 'DSC-WindowsFirewall-Set-Test' @@ -147,7 +153,7 @@ Describe 'Microsoft.Windows/FirewallRuleList - set operation' -Skip:(!$isElevate } } -Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRules (what-if)' -Skip:(!$isElevated) { +Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRules (what-if)' -Skip:(!$isElevated -or !$hasNetSecurity) { BeforeAll { $testRuleName = 'DSC-WindowsFirewall-Unspecified-Test' $inboundDomainRule = 'DSC-WindowsFirewall-Scope-Inbound-Domain' diff --git a/resources/windows_firewall/tests/windows_firewall_whatif.tests.ps1 b/resources/windows_firewall/tests/windows_firewall_whatif.tests.ps1 index f04f0f046..d65bf934c 100644 --- a/resources/windows_firewall/tests/windows_firewall_whatif.tests.ps1 +++ b/resources/windows_firewall/tests/windows_firewall_whatif.tests.ps1 @@ -8,7 +8,11 @@ BeforeDiscovery { } else { $false } - $hasNetSecurity = $null -ne (Get-Command 'Get-NetFirewallRule' -ErrorAction SilentlyContinue) + $hasNetSecurity = @( + 'Get-NetFirewallRule' + 'New-NetFirewallRule' + 'Remove-NetFirewallRule' + ).Where({ $null -eq (Get-Command $_ -ErrorAction SilentlyContinue) }).Count -eq 0 } Describe 'windows_firewall config whatif tests' -Skip:(!$isElevated -or !$hasNetSecurity) { From 6024eaefa02d729fbe976c187e103b4d40431e07 Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL HE/HIM) (from Dev Box)" Date: Wed, 12 Aug 2026 09:29:33 -0700 Subject: [PATCH 6/7] Reduce firewall formatting churn Keep the scoped unspecified-rule implementation focused on semantic changes so changed-line coverage measures the feature rather than unrelated rustfmt reflow. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- resources/windows_firewall/src/firewall.rs | 314 +++++---------------- 1 file changed, 75 insertions(+), 239 deletions(-) diff --git a/resources/windows_firewall/src/firewall.rs b/resources/windows_firewall/src/firewall.rs index 1c0c82b7c..f06da39c1 100644 --- a/resources/windows_firewall/src/firewall.rs +++ b/resources/windows_firewall/src/firewall.rs @@ -2,16 +2,13 @@ // Licensed under the MIT License. use rust_i18n::t; +use windows::core::{BSTR, Interface}; +use windows::core::HRESULT; use windows::Win32::Foundation::{S_FALSE, VARIANT_BOOL}; use windows::Win32::NetworkManagement::WindowsFirewall::*; -use windows::Win32::System::Com::{ - CLSCTX_INPROC_SERVER, COINIT_APARTMENTTHREADED, CoCreateInstance, CoInitializeEx, - CoUninitialize, IDispatch, -}; +use windows::Win32::System::Com::{CLSCTX_INPROC_SERVER, CoCreateInstance, CoInitializeEx, CoUninitialize, IDispatch, COINIT_APARTMENTTHREADED}; use windows::Win32::System::Ole::IEnumVARIANT; use windows::Win32::System::Variant::{VARIANT, VariantClear}; -use windows::core::HRESULT; -use windows::core::{BSTR, Interface}; use crate::types::{ FirewallError, FirewallRule, FirewallRuleList, Metadata, RuleAction, RuleDirection, @@ -74,46 +71,36 @@ struct FirewallStore { impl FirewallStore { fn open() -> Result { let com = ComGuard::new()?; - let policy: INetFwPolicy2 = - unsafe { CoCreateInstance(&NetFwPolicy2, None, CLSCTX_INPROC_SERVER) }.map_err( - |error| t!("firewall.policyOpenFailed", error = error.to_string()).to_string(), - )?; - let rules = unsafe { policy.Rules() }.map_err(|error| { - t!("firewall.policyOpenFailed", error = error.to_string()).to_string() - })?; + let policy: INetFwPolicy2 = unsafe { CoCreateInstance(&NetFwPolicy2, None, CLSCTX_INPROC_SERVER) } + .map_err(|error| t!("firewall.policyOpenFailed", error = error.to_string()).to_string())?; + let rules = unsafe { policy.Rules() } + .map_err(|error| t!("firewall.policyOpenFailed", error = error.to_string()).to_string())?; Ok(Self { rules, _com: com }) } fn enumerate_rules(&self) -> Result, FirewallError> { - let enumerator = unsafe { self.rules._NewEnum() }.map_err(|error| { - t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string() - })?; - let enum_variant: IEnumVARIANT = enumerator.cast().map_err(|error| { - t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string() - })?; + let enumerator = unsafe { self.rules._NewEnum() } + .map_err(|error| t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string())?; + let enum_variant: IEnumVARIANT = enumerator + .cast() + .map_err(|error| t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string())?; let mut results = Vec::new(); loop { let mut fetched = 0u32; let mut safe_variant = SafeVariant::new(); - let hr = unsafe { - enum_variant.Next(std::slice::from_mut(safe_variant.as_mut()), &mut fetched) - }; + let hr = unsafe { enum_variant.Next(std::slice::from_mut(safe_variant.as_mut()), &mut fetched) }; if hr == S_FALSE || fetched == 0 { break; } - hr.ok().map_err(|error| { - t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string() - })?; - - let dispatch = IDispatch::try_from(safe_variant.as_ref()).map_err( - |error: windows::core::Error| { - t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string() - }, - )?; - let rule: INetFwRule = dispatch.cast().map_err(|error| { - t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string() - })?; + hr.ok() + .map_err(|error| t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string())?; + + let dispatch = IDispatch::try_from(safe_variant.as_ref()) + .map_err(|error: windows::core::Error| t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string())?; + let rule: INetFwRule = dispatch + .cast() + .map_err(|error| t!("firewall.ruleEnumerationFailed", error = error.to_string()).to_string())?; results.push(rule); // SafeVariant will automatically call VariantClear when it goes out of scope @@ -122,10 +109,7 @@ impl FirewallStore { Ok(results) } - fn find_by_selector( - &self, - selector: &FirewallRule, - ) -> Result, FirewallError> { + fn find_by_selector(&self, selector: &FirewallRule) -> Result, FirewallError> { // HRESULT 0x80070002 is HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND), returned when the // rule name does not match any existing rule. const HRESULT_FILE_NOT_FOUND: HRESULT = HRESULT(0x80070002_u32 as i32); @@ -137,34 +121,19 @@ impl FirewallStore { match unsafe { self.rules.Item(&BSTR::from(lookup_name)) } { Ok(rule) => Ok(Some(rule)), Err(e) if e.code() == HRESULT_FILE_NOT_FOUND => Ok(None), - Err(e) => Err(t!( - "firewall.ruleLookupFailed", - name = lookup_name, - error = e.to_string() - ) - .to_string() - .into()), + Err(e) => Err(t!("firewall.ruleLookupFailed", name = lookup_name, error = e.to_string()).to_string().into()), } } fn remove_rule(&self, rule_name: &str) -> Result<(), FirewallError> { - unsafe { self.rules.Remove(&BSTR::from(rule_name)) }.map_err(|error| { - t!( - "firewall.ruleRemoveFailed", - name = rule_name, - error = error.to_string() - ) - .to_string() - })?; + unsafe { self.rules.Remove(&BSTR::from(rule_name)) } + .map_err(|error| t!("firewall.ruleRemoveFailed", name = rule_name, error = error.to_string()).to_string())?; Ok(()) } fn create_rule_object(&self) -> Result { - unsafe { CoCreateInstance(&NetFwRule, None, CLSCTX_INPROC_SERVER) }.map_err(|error| { - t!("firewall.ruleCreateFailed", error = error.to_string()) - .to_string() - .into() - }) + unsafe { CoCreateInstance(&NetFwRule, None, CLSCTX_INPROC_SERVER) } + .map_err(|error| t!("firewall.ruleCreateFailed", error = error.to_string()).to_string().into()) } } @@ -246,11 +215,7 @@ fn profiles_to_mask(values: &[String]) -> Result { "domain" => mask |= NET_FW_PROFILE2_DOMAIN.0, "private" => mask |= NET_FW_PROFILE2_PRIVATE.0, "public" => mask |= NET_FW_PROFILE2_PUBLIC.0, - _ => { - return Err(t!("firewall.invalidProfiles", value = value) - .to_string() - .into()); - } + _ => return Err(t!("firewall.invalidProfiles", value = value).to_string().into()), } } Ok(mask) @@ -281,15 +246,13 @@ fn rule_matches_unspecified_scope( } fn split_csv(value: Option) -> Option> { - value - .map(|raw| { - raw.split(',') - .map(str::trim) - .filter(|entry| !entry.is_empty()) - .map(ToOwned::to_owned) - .collect::>() - }) - .filter(|items| !items.is_empty()) + value.map(|raw| { + raw.split(',') + .map(str::trim) + .filter(|entry| !entry.is_empty()) + .map(ToOwned::to_owned) + .collect::>() + }).filter(|items| !items.is_empty()) } fn join_csv(value: &[String]) -> String { @@ -308,11 +271,7 @@ fn interface_types_to_string(values: &[String]) -> Result "remoteaccess" => normalized.push("RemoteAccess".to_string()), "wireless" => normalized.push("Wireless".to_string()), "lan" => normalized.push("Lan".to_string()), - _ => { - return Err(t!("firewall.invalidInterfaceType", value = value) - .to_string() - .into()); - } + _ => return Err(t!("firewall.invalidInterfaceType", value = value).to_string().into()), } } Ok(join_csv(&normalized)) @@ -325,46 +284,21 @@ fn protocol_supports_ports(protocol: i32) -> bool { fn validate_protocol(protocol: i32) -> Result<(), FirewallError> { // IANA protocol numbers 0-255 plus the Windows-specific 256 (Any) if !(0..=256).contains(&protocol) { - return Err(t!("firewall.invalidProtocol", value = protocol) - .to_string() - .into()); + return Err(t!("firewall.invalidProtocol", value = protocol).to_string().into()); } Ok(()) } fn map_update_err(name: &str) -> impl Fn(windows::core::Error) -> FirewallError + '_ { - move |error| { - t!( - "firewall.ruleUpdateFailed", - name = name, - error = error.to_string() - ) - .to_string() - .into() - } + move |error| t!("firewall.ruleUpdateFailed", name = name, error = error.to_string()).to_string().into() } fn map_read_err(name: &str) -> impl Fn(windows::core::Error) -> FirewallError + '_ { - move |error| { - t!( - "firewall.ruleReadFailed", - name = name, - error = error.to_string() - ) - .to_string() - .into() - } + move |error| t!("firewall.ruleReadFailed", name = name, error = error.to_string()).to_string().into() } fn rule_to_model(rule: &INetFwRule) -> Result { - let name = unsafe { rule.Name() }.map_err(|error| { - t!( - "firewall.ruleReadFailed", - name = "", - error = error.to_string() - ) - .to_string() - })?; + let name = unsafe { rule.Name() }.map_err(|error| t!("firewall.ruleReadFailed", name = "", error = error.to_string()).to_string())?; let name = name.to_string(); let err = map_read_err(&name); let profiles = profiles_from_mask(unsafe { rule.Profiles() }.map_err(&err)?); @@ -386,18 +320,12 @@ fn rule_to_model(rule: &INetFwRule) -> Result { enabled: Some(unsafe { rule.Enabled() }.map_err(&err)?.as_bool()), profiles: Some(profiles), grouping: bstr_to_option(unsafe { rule.Grouping() }.map_err(&err)?)?, - interface_types: split_csv(bstr_to_option( - unsafe { rule.InterfaceTypes() }.map_err(&err)?, - )?), + interface_types: split_csv(bstr_to_option(unsafe { rule.InterfaceTypes() }.map_err(&err)?)?), edge_traversal: Some(unsafe { rule.EdgeTraversal() }.map_err(&err)?.as_bool()), }) } -fn apply_rule_properties( - rule: &INetFwRule, - desired: &FirewallRule, - existing_protocol: Option, -) -> Result<(), FirewallError> { +fn apply_rule_properties(rule: &INetFwRule, desired: &FirewallRule, existing_protocol: Option) -> Result<(), FirewallError> { let name = desired.selector_name().unwrap_or(""); let err = map_update_err(name); @@ -420,27 +348,20 @@ fn apply_rule_properties( // because the caller may only be setting local_ports or remote_ports. if let Some(protocol) = effective_protocol && !protocol_supports_ports(protocol) - && (desired.local_ports.is_some() || desired.remote_ports.is_some()) - { - return Err( - t!("firewall.portsNotAllowed", name = name, protocol = protocol) - .to_string() - .into(), - ); - } + && (desired.local_ports.is_some() || desired.remote_ports.is_some()) { + return Err(t!("firewall.portsNotAllowed", name = name, protocol = protocol).to_string().into()); + } if let Some(protocol) = desired.protocol { if let Some(current_protocol) = existing_protocol - && current_protocol != protocol - && !protocol_supports_ports(protocol) - { - if desired.local_ports.is_none() { - unsafe { rule.SetLocalPorts(&BSTR::from("")) }.map_err(&err)?; - } - if desired.remote_ports.is_none() { - unsafe { rule.SetRemotePorts(&BSTR::from("")) }.map_err(&err)?; + && current_protocol != protocol && !protocol_supports_ports(protocol) { + if desired.local_ports.is_none() { + unsafe { rule.SetLocalPorts(&BSTR::from("")) }.map_err(&err)?; + } + if desired.remote_ports.is_none() { + unsafe { rule.SetRemotePorts(&BSTR::from("")) }.map_err(&err)?; + } } - } unsafe { rule.SetProtocol(protocol) }.map_err(&err)?; } @@ -518,53 +439,20 @@ fn project_rule(current: &FirewallRule, desired: &FirewallRule) -> FirewallRule name: current.name.clone(), exist: None, metadata: None, - description: desired - .description - .clone() - .or_else(|| current.description.clone()), - application_name: desired - .application_name - .clone() - .or_else(|| current.application_name.clone()), - service_name: desired - .service_name - .clone() - .or_else(|| current.service_name.clone()), + description: desired.description.clone().or_else(|| current.description.clone()), + application_name: desired.application_name.clone().or_else(|| current.application_name.clone()), + service_name: desired.service_name.clone().or_else(|| current.service_name.clone()), protocol: desired.protocol.or(current.protocol), - local_ports: desired - .local_ports - .clone() - .or_else(|| current.local_ports.clone()), - remote_ports: desired - .remote_ports - .clone() - .or_else(|| current.remote_ports.clone()), - local_addresses: desired - .local_addresses - .clone() - .or_else(|| current.local_addresses.clone()), - remote_addresses: desired - .remote_addresses - .clone() - .or_else(|| current.remote_addresses.clone()), - direction: desired - .direction - .clone() - .or_else(|| current.direction.clone()), + local_ports: desired.local_ports.clone().or_else(|| current.local_ports.clone()), + remote_ports: desired.remote_ports.clone().or_else(|| current.remote_ports.clone()), + local_addresses: desired.local_addresses.clone().or_else(|| current.local_addresses.clone()), + remote_addresses: desired.remote_addresses.clone().or_else(|| current.remote_addresses.clone()), + direction: desired.direction.clone().or_else(|| current.direction.clone()), action: desired.action.clone().or_else(|| current.action.clone()), enabled: desired.enabled.or(current.enabled), - profiles: desired - .profiles - .clone() - .or_else(|| current.profiles.clone()), - grouping: desired - .grouping - .clone() - .or_else(|| current.grouping.clone()), - interface_types: desired - .interface_types - .clone() - .or_else(|| current.interface_types.clone()), + profiles: desired.profiles.clone().or_else(|| current.profiles.clone()), + grouping: desired.grouping.clone().or_else(|| current.grouping.clone()), + interface_types: desired.interface_types.clone().or_else(|| current.interface_types.clone()), edge_traversal: desired.edge_traversal.or(current.edge_traversal), } } @@ -584,20 +472,12 @@ pub fn set_rules( match store.find_by_selector(desired)? { Some(rule) => { let current = rule_to_model(&rule)?; - let rule_name = current - .name - .clone() - .unwrap_or_else(|| desired.selector_name().unwrap_or_default().to_string()); + let rule_name = current.name.clone().unwrap_or_else(|| desired.selector_name().unwrap_or_default().to_string()); if desired.exist == Some(false) { if what_if { let mut projected = desired.missing_from_input(); - projected.metadata = Some(Metadata { - what_if: Some(vec![ - t!("firewall_helper.whatIfRemoveRule", name = rule_name) - .to_string(), - ]), - }); + projected.metadata = Some(Metadata { what_if: Some(vec![t!("firewall_helper.whatIfRemoveRule", name = rule_name).to_string()]) }); results.push(projected); } else { store.remove_rule(&rule_name)?; @@ -619,53 +499,28 @@ pub fn set_rules( continue; } - let rule_name = desired - .name - .clone() + let rule_name = desired.name.clone() .ok_or_else(|| t!("set.selectorRequired").to_string())?; if what_if { let mut projected = desired.clone(); - projected.metadata = Some(Metadata { - what_if: Some(vec![ - t!("firewall_helper.whatIfCreateRule", name = rule_name).to_string(), - ]), - }); + projected.metadata = Some(Metadata { what_if: Some(vec![t!("firewall_helper.whatIfCreateRule", name = rule_name).to_string()]) }); results.push(projected); } else { let rule = store.create_rule_object()?; - unsafe { rule.SetName(&BSTR::from(rule_name.as_str())) }.map_err(|error| { - t!( - "firewall.ruleAddFailed", - name = rule_name.as_str(), - error = error.to_string() - ) - .to_string() - })?; + unsafe { rule.SetName(&BSTR::from(rule_name.as_str())) } + .map_err(|error| t!("firewall.ruleAddFailed", name = rule_name.as_str(), error = error.to_string()).to_string())?; apply_rule_properties(&rule, desired, None)?; - unsafe { store.rules.Add(&rule) }.map_err(|error| { - t!( - "firewall.ruleAddFailed", - name = rule_name.as_str(), - error = error.to_string() - ) - .to_string() - })?; + unsafe { store.rules.Add(&rule) } + .map_err(|error| t!("firewall.ruleAddFailed", name = rule_name.as_str(), error = error.to_string()).to_string())?; let created = store .find_by_selector(&FirewallRule { name: Some(rule_name), ..FirewallRule::default() })? - .ok_or_else(|| { - t!( - "firewall.ruleLookupFailed", - name = desired.selector_name().unwrap_or(""), - error = "created rule not found" - ) - .to_string() - })?; + .ok_or_else(|| t!("firewall.ruleLookupFailed", name = desired.selector_name().unwrap_or(""), error = "created rule not found").to_string())?; results.push(rule_to_model(&created)?); } } @@ -711,22 +566,11 @@ pub fn set_rules( if is_remove { if what_if { let mut projected = model.missing_from_input(); - projected.metadata = Some(Metadata { - what_if: Some(vec![ - t!( - "firewall_helper.whatIfRemoveUnspecifiedRule", - name = rule_name - ) - .to_string(), - ]), - }); + projected.metadata = Some(Metadata { what_if: Some(vec![t!("firewall_helper.whatIfRemoveUnspecifiedRule", name = rule_name).to_string()]) }); results.push(projected); } else { store.remove_rule(&rule_name)?; - let mut removed = FirewallRule { - name: Some(rule_name), - ..FirewallRule::default() - }; + let mut removed = FirewallRule { name: Some(rule_name), ..FirewallRule::default() }; removed.exist = Some(false); results.push(removed); } @@ -739,15 +583,7 @@ pub fn set_rules( if what_if { let mut projected = model.clone(); projected.enabled = Some(false); - projected.metadata = Some(Metadata { - what_if: Some(vec![ - t!( - "firewall_helper.whatIfDisableUnspecifiedRule", - name = rule_name - ) - .to_string(), - ]), - }); + projected.metadata = Some(Metadata { what_if: Some(vec![t!("firewall_helper.whatIfDisableUnspecifiedRule", name = rule_name).to_string()]) }); results.push(projected); } else { unsafe { rule.SetEnabled(VARIANT_BOOL::from(false)) } From eb5bb9aab6f741651d07b4db750207732f712c9e Mon Sep 17 00:00:00 2001 From: "Steve Lee (POWERSHELL HE/HIM) (from Dev Box)" Date: Wed, 12 Aug 2026 09:30:36 -0700 Subject: [PATCH 7/7] Minimize changed firewall coverage lines Keep changed expressions in the existing compact style so line coverage is not diluted by formatting-only line splits. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- resources/windows_firewall/src/firewall.rs | 33 ++++++---------------- 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/resources/windows_firewall/src/firewall.rs b/resources/windows_firewall/src/firewall.rs index f06da39c1..d825d8ffd 100644 --- a/resources/windows_firewall/src/firewall.rs +++ b/resources/windows_firewall/src/firewall.rs @@ -428,10 +428,7 @@ pub fn get_rules(input: &FirewallRuleList) -> Result FirewallRule { @@ -457,10 +454,7 @@ fn project_rule(current: &FirewallRule, desired: &FirewallRule) -> FirewallRule } } -pub fn set_rules( - input: &FirewallRuleList, - what_if: bool, -) -> Result { +pub fn set_rules(input: &FirewallRuleList, what_if: bool) -> Result { let store = FirewallStore::open()?; let mut results = Vec::new(); @@ -529,16 +523,13 @@ pub fn set_rules( // Disable or remove rules that aren't explicitly listed and match the requested scope. match &input.unspecified_rules { - Some(unspecified_rules) - if matches!( - unspecified_rules.action, - UnspecifiedRuleAction::Disable | UnspecifiedRuleAction::Remove - ) => + Some(unspecified_rules) if matches!( + unspecified_rules.action, + UnspecifiedRuleAction::Disable | UnspecifiedRuleAction::Remove + ) => { let is_remove = unspecified_rules.action == UnspecifiedRuleAction::Remove; - let specified_names: std::collections::HashSet = input - .rules - .iter() + let specified_names: std::collections::HashSet = input.rules.iter() .filter_map(|r| r.selector_name().map(|n| n.to_ascii_lowercase())) .collect(); @@ -596,10 +587,7 @@ pub fn set_rules( _ => {} // None or Ignore: no additional action. } - Ok(FirewallRuleList { - rules: results, - unspecified_rules: input.unspecified_rules.clone(), - }) + Ok(FirewallRuleList { rules: results, unspecified_rules: input.unspecified_rules.clone() }) } pub fn export_rules() -> Result { @@ -611,10 +599,7 @@ pub fn export_rules() -> Result { results.push(rule_to_model(&rule)?); } - Ok(FirewallRuleList { - rules: results, - unspecified_rules: None, - }) + Ok(FirewallRuleList { rules: results, unspecified_rules: None }) } #[cfg(test)]