From 1e5000dc8c1b3e1378e28d2c543b258108b4773d Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Wed, 12 Aug 2026 23:12:57 +0000 Subject: [PATCH 1/3] Use JSON Schema defaults in synthetic test get_diff (#1670) * Use JSON Schema defaults in synthetic test get_diff Update get_diff() to accept an optional JSON Schema parameter via the new get_diff_with_schema() function. When a property exists in the expected (desired) state but is missing from the actual state, the function now checks the schema for a 'default' value for that property. If the expected value matches the schema default, it is not reported as differing. This improves synthetic test accuracy for resources that don't return properties whose values match the schema-defined defaults. - Add get_diff_with_schema() with optional schema parameter - Keep get_diff() as a convenience wrapper (no schema) - Update invoke_synthetic_test to retrieve and pass the resource schema - Update DscResource synthetic test path for adapted resources - Add get_schema_default() helper to extract defaults from JSON Schema - Add Test/SchemaDefault test resource and dsctest subcommand - Add Rust unit tests for schema default comparison logic - Add Pester integration tests for end-to-end validation Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Address PR feedback: restrict visibility and avoid redundant serialization - Change get_diff_with_schema from pub to pub(crate) since it is only used within the dsc-lib crate - Read schema from RESOURCE_SCHEMAS cache directly (returns Value) instead of round-tripping through get_schema -> String -> from_str. Only calls get_schema to populate the cache on a miss. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Add FirewallRuleList Pester tests for schema default fix (#1666) Add tests verifying that unspecifiedRulesAction set to the schema default value 'ignore' is no longer reported as drift in synthetic test. Non-default values ('disable', 'remove') are still correctly flagged. Tests require elevation to create/remove firewall rules and are skipped when not running as Administrator. Fixes #1666 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Fix CI: skip firewall schema default tests when NetSecurity module unavailable Move -Skip to Describe block and check for Get-NetFirewallRule cmdlet availability in BeforeDiscovery. This prevents BeforeAll/AfterAll from running on CI runners without the NetSecurity module. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Mikey Lombardi (He/Him) --------- Co-authored-by: Steve Lee (POWERSHELL HE/HIM) (from Dev Box) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Mikey Lombardi (He/Him) --- dsc/tests/dsc_schema_default.tests.ps1 | 54 ++++++++ .../src/dscresources/command_resource.rs | 11 +- lib/dsc-lib/src/dscresources/dscresource.rs | 127 +++++++++++++++++- .../windows_firewall_schema_default.tests.ps1 | 105 +++++++++++++++ tools/dsctest/dsctest.dsc.manifests.json | 39 ++++++ tools/dsctest/src/args.rs | 7 + tools/dsctest/src/main.rs | 17 +++ tools/dsctest/src/schema_default.rs | 14 ++ 8 files changed, 369 insertions(+), 5 deletions(-) create mode 100644 dsc/tests/dsc_schema_default.tests.ps1 create mode 100644 resources/windows_firewall/tests/windows_firewall_schema_default.tests.ps1 create mode 100644 tools/dsctest/src/schema_default.rs diff --git a/dsc/tests/dsc_schema_default.tests.ps1 b/dsc/tests/dsc_schema_default.tests.ps1 new file mode 100644 index 000000000..286e76eda --- /dev/null +++ b/dsc/tests/dsc_schema_default.tests.ps1 @@ -0,0 +1,54 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +Describe 'Synthetic test uses schema defaults' { + It 'Property matching schema default is not reported as differing' { + $out = '{"name":"test","enabled":true}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json + $LASTEXITCODE | Should -Be 0 + $out.inDesiredState | Should -Be $true + $out.differingProperties | Should -BeNullOrEmpty + } + + It 'Property differing from schema default is reported as differing' { + $out = '{"name":"test","enabled":false}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json + $LASTEXITCODE | Should -Be 0 + $out.inDesiredState | Should -Be $false + $out.differingProperties | Should -Contain 'enabled' + } + + It 'Integer property matching schema default is not reported as differing' { + $out = '{"name":"test","count":5}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json + $LASTEXITCODE | Should -Be 0 + $out.inDesiredState | Should -Be $true + $out.differingProperties | Should -BeNullOrEmpty + } + + It 'Integer property differing from schema default is reported as differing' { + $out = '{"name":"test","count":10}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json + $LASTEXITCODE | Should -Be 0 + $out.inDesiredState | Should -Be $false + $out.differingProperties | Should -Contain 'count' + } + + It 'Multiple properties matching schema defaults are not reported as differing' { + $out = '{"name":"test","enabled":true,"count":5}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json + $LASTEXITCODE | Should -Be 0 + $out.inDesiredState | Should -Be $true + $out.differingProperties | Should -BeNullOrEmpty + } + + It 'Mix of matching and non-matching defaults reports only non-matching' { + $out = '{"name":"test","enabled":true,"count":10}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json + $LASTEXITCODE | Should -Be 0 + $out.inDesiredState | Should -Be $false + $out.differingProperties | Should -Contain 'count' + $out.differingProperties | Should -Not -Contain 'enabled' + } + + It 'Property present in both expected and actual is compared normally' { + $out = '{"name":"test"}' | dsc resource test -r Test/SchemaDefault -f - | ConvertFrom-Json + $LASTEXITCODE | Should -Be 0 + $out.inDesiredState | Should -Be $true + $out.differingProperties | Should -BeNullOrEmpty + } +} diff --git a/lib/dsc-lib/src/dscresources/command_resource.rs b/lib/dsc-lib/src/dscresources/command_resource.rs index 6674d4924..5b4479b72 100644 --- a/lib/dsc-lib/src/dscresources/command_resource.rs +++ b/lib/dsc-lib/src/dscresources/command_resource.rs @@ -12,7 +12,7 @@ use crate::{configure::{config_doc::{ExecutionKind, SecurityContextKind}, config use crate::dscerror::DscError; use crate::locked_insert; use super::{ - dscresource::{get_diff, redact, DscResource}, + dscresource::{get_diff, get_diff_with_schema, redact, DscResource}, invoke_result::{ DeleteResult, DeleteResultKind, ExportResult, GetResult, ResolveResult, SetResult, TestResult, ValidateResult, @@ -454,7 +454,14 @@ fn invoke_synthetic_test(resource: &DscResource, expected: &str, target_resource } }; let expected_value: Value = serde_json::from_str(expected)?; - let diff_properties = get_diff(&expected_value, &actual_state); + let cached_resource = target_resource.unwrap_or(resource); + let schema: Option = get_resource_schema(&cached_resource.type_name, &cached_resource.version) + .or_else(|| { + // Populate the cache on a miss, then read from cache + get_schema(resource, target_resource).ok(); + get_resource_schema(&cached_resource.type_name, &cached_resource.version) + }); + let diff_properties = get_diff_with_schema(&expected_value, &actual_state, schema.as_ref()); Ok(TestResult::Resource(ResourceTestResponse { desired_state: expected_value, actual_state, diff --git a/lib/dsc-lib/src/dscresources/dscresource.rs b/lib/dsc-lib/src/dscresources/dscresource.rs index 8c2566610..84a6480b7 100644 --- a/lib/dsc-lib/src/dscresources/dscresource.rs +++ b/lib/dsc-lib/src/dscresources/dscresource.rs @@ -470,7 +470,12 @@ impl Invoke for DscResource { response.actual_state } }; - let diff_properties = get_diff( &desired_state, &actual_state); + let schema: Option = if let Some(s) = &self.schema { + serde_json::to_value(s).ok() + } else { + self.schema().ok().and_then(|s| serde_json::from_str(&s).ok()) + }; + let diff_properties = get_diff_with_schema( &desired_state, &actual_state, schema.as_ref()); desired_state = redact(&desired_state); let test_result = TestResult::Resource(ResourceTestResponse { desired_state, @@ -647,6 +652,24 @@ pub fn get_adapter_input_kind(adapter: &DscResource) -> Result Vec { + get_diff_with_schema(expected, actual, None) +} + +#[must_use] +/// Performs a comparison of two JSON Values using an optional JSON Schema. +/// If a property exists in `expected` but not in `actual`, the schema's `default` value +/// for that property is used for comparison when available. +/// +/// # Arguments +/// +/// * `expected` - The expected value +/// * `actual` - The actual value +/// * `schema` - Optional JSON Schema to look up default values for missing properties +/// +/// # Returns +/// +/// An array of top level properties that differ, if any +pub(crate) fn get_diff_with_schema(expected: &Value, actual: &Value, schema: Option<&Value>) -> Vec { let mut diff_properties: Vec = Vec::new(); if expected.is_null() { return diff_properties; @@ -702,8 +725,16 @@ pub fn get_diff(expected: &Value, actual: &Value) -> Vec { diff_properties.push(key.to_string()); } } else { - info!("{}", t!("dscresources.dscresource.diffKeyMissing", key = key)); - diff_properties.push(key.to_string()); + // Property not in actual - check schema for a default value + if let Some(default_value) = get_schema_default(schema, key) { + if value != &default_value { + info!("{}", t!("dscresources.dscresource.diffKeyMissing", key = key)); + diff_properties.push(key.to_string()); + } + } else { + info!("{}", t!("dscresources.dscresource.diffKeyMissing", key = key)); + diff_properties.push(key.to_string()); + } } } else { info!("{}", t!("dscresources.dscresource.diffKeyNotObject", key = key)); @@ -716,6 +747,23 @@ pub fn get_diff(expected: &Value, actual: &Value) -> Vec { diff_properties } +/// Looks up the default value for a property from a JSON Schema. +/// +/// # Arguments +/// +/// * `schema` - Optional JSON Schema value +/// * `property_name` - The property name to look up +/// +/// # Returns +/// +/// The default value if found in the schema's properties definition, otherwise None +fn get_schema_default(schema: Option<&Value>, property_name: &str) -> Option { + let schema = schema?; + let properties = schema.get("properties")?.as_object()?; + let property_schema = properties.get(property_name)?.as_object()?; + property_schema.get("default").cloned() +} + /// Validates the properties of a resource against its schema. /// /// # Arguments @@ -926,3 +974,76 @@ fn different_array_with_nested_array() { let array_two = vec![json!("a"), json!(1), json!({"a":"b"}), json!(vec![json!("a"), json!(2)])]; assert_eq!(is_same_array(&array_one, &array_two), false); } + +#[test] +fn diff_with_schema_default_matches_expected() { + use serde_json::json; + let expected = json!({"name": "test", "enabled": true}); + let actual = json!({"name": "test"}); + let schema = json!({ + "type": "object", + "properties": { + "name": { "type": "string" }, + "enabled": { "type": "boolean", "default": true } + } + }); + let diff = get_diff_with_schema(&expected, &actual, Some(&schema)); + assert!(diff.is_empty(), "Expected no diff when expected matches schema default, got: {diff:?}"); +} + +#[test] +fn diff_with_schema_default_differs_from_expected() { + use serde_json::json; + let expected = json!({"name": "test", "enabled": false}); + let actual = json!({"name": "test"}); + let schema = json!({ + "type": "object", + "properties": { + "name": { "type": "string" }, + "enabled": { "type": "boolean", "default": true } + } + }); + let diff = get_diff_with_schema(&expected, &actual, Some(&schema)); + assert_eq!(diff, vec!["enabled".to_string()]); +} + +#[test] +fn diff_with_schema_no_default_reports_missing_property() { + use serde_json::json; + let expected = json!({"name": "test", "enabled": true}); + let actual = json!({"name": "test"}); + let schema = json!({ + "type": "object", + "properties": { + "name": { "type": "string" }, + "enabled": { "type": "boolean" } + } + }); + let diff = get_diff_with_schema(&expected, &actual, Some(&schema)); + assert_eq!(diff, vec!["enabled".to_string()]); +} + +#[test] +fn diff_without_schema_reports_missing_property() { + use serde_json::json; + let expected = json!({"name": "test", "enabled": true}); + let actual = json!({"name": "test"}); + let diff = get_diff_with_schema(&expected, &actual, None); + assert_eq!(diff, vec!["enabled".to_string()]); +} + +#[test] +fn diff_with_schema_default_integer() { + use serde_json::json; + let expected = json!({"name": "test", "count": 5}); + let actual = json!({"name": "test"}); + let schema = json!({ + "type": "object", + "properties": { + "name": { "type": "string" }, + "count": { "type": "integer", "default": 5 } + } + }); + let diff = get_diff_with_schema(&expected, &actual, Some(&schema)); + assert!(diff.is_empty(), "Expected no diff when expected matches schema default integer, got: {diff:?}"); +} diff --git a/resources/windows_firewall/tests/windows_firewall_schema_default.tests.ps1 b/resources/windows_firewall/tests/windows_firewall_schema_default.tests.ps1 new file mode 100644 index 000000000..fa0d6aa46 --- /dev/null +++ b/resources/windows_firewall/tests/windows_firewall_schema_default.tests.ps1 @@ -0,0 +1,105 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaults' -Skip:(!$canRunFirewallTests) { + BeforeDiscovery { + $canRunFirewallTests = $IsWindows -and + (Get-Command Get-NetFirewallRule -ErrorAction Ignore) -and + ([Security.Principal.WindowsPrincipal][Security.Principal.WindowsIdentity]::GetCurrent()).IsInRole( + [Security.Principal.WindowsBuiltInRole]::Administrator) + } + + BeforeAll { + $resourceType = 'Microsoft.Windows/FirewallRuleList' + $testRuleName = 'DSC-WindowsFirewall-SchemaDefault-Test' + + # Ensure a known rule exists for testing + $existing = Get-NetFirewallRule -Name $testRuleName -ErrorAction Ignore + if (-not $existing) { + New-NetFirewallRule -Name $testRuleName -DisplayName $testRuleName ` + -Direction Inbound -Action Allow -Protocol TCP -LocalPort 32921 ` + -Enabled True -PolicyStore PersistentStore | Out-Null + } + } + + AfterAll { + Remove-NetFirewallRule -Name $testRuleName -ErrorAction Ignore + } + + It 'unspecifiedRulesAction set to default "ignore" does not report as differing' { + $json = @{ + unspecifiedRulesAction = 'ignore' + rules = @(@{ + name = $testRuleName + direction = 'Inbound' + action = 'Allow' + protocol = 6 + localPorts = '32921' + enabled = $true + }) + } | ConvertTo-Json -Compress -Depth 5 + $out = $json | dsc resource test -r $resourceType -f - 2>$testdrive/error.log + $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) + + $result = $out | ConvertFrom-Json + $result.inDesiredState | Should -Be $true + $result.differingProperties | Should -Not -Contain 'unspecifiedRulesAction' + } + + It 'unspecifiedRulesAction omitted does not report as differing' { + $json = @{ + rules = @(@{ + name = $testRuleName + direction = 'Inbound' + action = 'Allow' + protocol = 6 + localPorts = '32921' + enabled = $true + }) + } | ConvertTo-Json -Compress -Depth 5 + $out = $json | dsc resource test -r $resourceType -f - 2>$testdrive/error.log + $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) + + $result = $out | ConvertFrom-Json + $result.inDesiredState | Should -Be $true + $result.differingProperties | Should -Not -Contain 'unspecifiedRulesAction' + } + + It 'non-default unspecifiedRulesAction "disable" is reported as differing' { + $json = @{ + unspecifiedRulesAction = 'disable' + rules = @(@{ + name = $testRuleName + direction = 'Inbound' + action = 'Allow' + protocol = 6 + localPorts = '32921' + enabled = $true + }) + } | ConvertTo-Json -Compress -Depth 5 + $out = $json | dsc resource test -r $resourceType -f - 2>$testdrive/error.log + $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) + + $result = $out | ConvertFrom-Json + $result.differingProperties | Should -Contain 'unspecifiedRulesAction' + } + + It 'non-default unspecifiedRulesAction "remove" is reported as differing' { + $json = @{ + unspecifiedRulesAction = 'remove' + rules = @(@{ + name = $testRuleName + direction = 'Inbound' + action = 'Allow' + protocol = 6 + localPorts = '32921' + enabled = $true + }) + } | ConvertTo-Json -Compress -Depth 5 + $out = $json | dsc resource test -r $resourceType -f - 2>$testdrive/error.log + $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) + + $result = $out | ConvertFrom-Json + $result.differingProperties | Should -Contain 'unspecifiedRulesAction' + } +} diff --git a/tools/dsctest/dsctest.dsc.manifests.json b/tools/dsctest/dsctest.dsc.manifests.json index 258bf0d17..e5bcc8a3c 100644 --- a/tools/dsctest/dsctest.dsc.manifests.json +++ b/tools/dsctest/dsctest.dsc.manifests.json @@ -280,6 +280,45 @@ } } }, + { + "$schema": "https://aka.ms/dsc/schemas/v3/bundled/resource/manifest.json", + "type": "Test/SchemaDefault", + "version": "0.1.0", + "get": { + "executable": "dsctest", + "args": [ + "schema-default", + { + "jsonInputArg": "--input", + "mandatory": true + } + ] + }, + "schema": { + "embedded": { + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "required": ["name"], + "additionalProperties": false, + "properties": { + "name": { + "type": "string", + "description": "The name of the resource instance." + }, + "enabled": { + "type": "boolean", + "description": "Whether the resource is enabled.", + "default": true + }, + "count": { + "type": "integer", + "description": "The count value.", + "default": 5 + } + } + } + } + }, { "$schema": "https://aka.ms/dsc/schemas/v3/bundled/resource/manifest.json", "type": "Test/InDesiredState", diff --git a/tools/dsctest/src/args.rs b/tools/dsctest/src/args.rs index 18287e764..135572bbe 100644 --- a/tools/dsctest/src/args.rs +++ b/tools/dsctest/src/args.rs @@ -20,6 +20,7 @@ pub enum Schemas { Operation, RefreshEnv, RestartRequired, + SchemaDefault, Set, Sleep, StateAndDiff, @@ -157,6 +158,12 @@ pub enum SubCommand { input: String, }, + #[clap(name = "schema-default", about = "Test resource for schema default values in synthetic test")] + SchemaDefault { + #[clap(name = "input", short, long, help = "The input to the schema-default command as JSON")] + input: String, + }, + #[clap(name = "schema", about = "Get the JSON schema for a subcommand")] Schema { #[clap(name = "subcommand", short, long, help = "The subcommand to get the schema for")] diff --git a/tools/dsctest/src/main.rs b/tools/dsctest/src/main.rs index dc5c56f7f..a7e8f4ebe 100644 --- a/tools/dsctest/src/main.rs +++ b/tools/dsctest/src/main.rs @@ -16,6 +16,7 @@ mod operation; mod adapter; mod refresh_env; mod restart_required; +mod schema_default; mod set; mod sleep; mod state_and_diff; @@ -41,6 +42,7 @@ use crate::metadata::Metadata; use crate::operation::Operation; use crate::refresh_env::RefreshEnv; use crate::restart_required::RestartRequired; +use crate::schema_default::SchemaDefault; use crate::set::{Set, invoke_set}; use crate::sleep::Sleep; use crate::state_and_diff::StateAndDiff; @@ -288,6 +290,18 @@ fn main() { }; serde_json::to_string(&restart_required).unwrap() }, + SubCommand::SchemaDefault { input } => { + let schema_default = match serde_json::from_str::(&input) { + Ok(sd) => sd, + Err(err) => { + eprintln!("Error JSON does not match schema: {err}"); + std::process::exit(1); + } + }; + // Only return 'name' in the output - omit 'enabled' and 'count' + // to test schema default comparison + serde_json::json!({"name": schema_default.name}).to_string() + }, SubCommand::Schema { subcommand } => { let schema = match subcommand { Schemas::Adapter => { @@ -335,6 +349,9 @@ fn main() { Schemas::RestartRequired => { schema_for!(RestartRequired) }, + Schemas::SchemaDefault => { + schema_for!(SchemaDefault) + }, Schemas::Set => { schema_for!(Set) }, diff --git a/tools/dsctest/src/schema_default.rs b/tools/dsctest/src/schema_default.rs new file mode 100644 index 000000000..6a661de48 --- /dev/null +++ b/tools/dsctest/src/schema_default.rs @@ -0,0 +1,14 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema)] +pub struct SchemaDefault { + pub name: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub enabled: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub count: Option, +} From 11ed73f9f889a41d943308359a217bb6135af518 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Fri, 14 Aug 2026 13:23:18 -0700 Subject: [PATCH 2/3] Add scoped unspecified firewall rule handling and allow empty rules (#1671) * 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> * 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> * 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> * 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> * 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> * 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> * 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> --------- Co-authored-by: Steve Lee (POWERSHELL HE/HIM) (from Dev Box) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/rust.yml | 46 +++- Cargo.lock | 2 +- resources/windows_firewall/Cargo.toml | 2 +- resources/windows_firewall/locales/en-us.toml | 4 +- resources/windows_firewall/src/firewall.rs | 140 ++++++++++-- resources/windows_firewall/src/types.rs | 34 ++- .../tests/windows_firewall_get.tests.ps1 | 7 +- .../tests/windows_firewall_set.tests.ps1 | 215 +++++++++++++++--- .../tests/windows_firewall_whatif.tests.ps1 | 22 +- .../windows_firewall.dsc.resource.json | 56 ++++- 10 files changed, 438 insertions(+), 90 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index bc40ac391..ab97e1893 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 diff --git a/Cargo.lock b/Cargo.lock index 9a5818818..3aa53020f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4256,7 +4256,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..3ed8e9e6f 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] @@ -28,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 f5c6714a6..d825d8ffd 100644 --- a/resources/windows_firewall/src/firewall.rs +++ b/resources/windows_firewall/src/firewall.rs @@ -10,7 +10,10 @@ use windows::Win32::System::Com::{CLSCTX_INPROC_SERVER, CoCreateInstance, CoInit use windows::Win32::System::Ole::IEnumVARIANT; use windows::Win32::System::Variant::{VARIANT, VariantClear}; -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 +35,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(), + ); } } } @@ -212,6 +221,30 @@ fn profiles_to_mask(values: &[String]) -> Result { 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() { + 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 { + return Ok(false); + } + } + + Ok(true) +} + fn split_csv(value: Option) -> Option> { value.map(|raw| { raw.split(',') @@ -381,10 +414,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 +428,7 @@ pub fn get_rules(input: &FirewallRuleList) -> Result FirewallRule { @@ -426,10 +455,6 @@ fn project_rule(current: &FirewallRule, desired: &FirewallRule) -> FirewallRule } pub fn set_rules(input: &FirewallRuleList, what_if: bool) -> Result { - if input.rules.is_empty() { - return Err(t!("set.rulesArrayEmpty").to_string().into()); - } - let store = FirewallStore::open()?; let mut results = Vec::new(); @@ -496,10 +521,14 @@ pub fn set_rules(input: &FirewallRuleList, what_if: bool) -> Result { - let is_remove = matches!(&input.unspecified_rules_action, Some(UnspecifiedRulesAction::Remove)); + // 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,11 +545,15 @@ pub fn set_rules(input: &FirewallRuleList, what_if: bool) -> 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 +599,74 @@ 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() + ); + } + + #[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/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..dfa348901 100644 --- a/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 +++ b/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 @@ -1,16 +1,22 @@ # 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 } + $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 -or !$hasNetSecurity) { BeforeAll { $resourceType = 'Microsoft.Windows/FirewallRuleList' $testRuleName = 'DSC-WindowsFirewall-Set-Test' @@ -46,10 +52,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,18 +153,13 @@ Describe 'Microsoft.Windows/FirewallRuleList - set operation' -Skip:(!$isElevate } } -Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRulesAction (what-if)' -Skip:(!$isElevated) { - BeforeDiscovery { - $isElevated = if ($IsWindows) { - ([Security.Principal.WindowsPrincipal][Security.Principal.WindowsIdentity]::GetCurrent()).IsInRole( - [Security.Principal.WindowsBuiltInRole]::Administrator) - } else { - $false - } - } - +Describe 'Microsoft.Windows/FirewallRuleList - unspecifiedRules (what-if)' -Skip:(!$isElevated -or !$hasNetSecurity) { 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 +170,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 +230,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 +251,135 @@ 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 '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' + 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 +407,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 +417,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 +434,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 +442,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/tests/windows_firewall_whatif.tests.ps1 b/resources/windows_firewall/tests/windows_firewall_whatif.tests.ps1 index 5bd0f085e..d65bf934c 100644 --- a/resources/windows_firewall/tests/windows_firewall_whatif.tests.ps1 +++ b/resources/windows_firewall/tests/windows_firewall_whatif.tests.ps1 @@ -1,17 +1,21 @@ # 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 = @( + '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) { BeforeAll { $testRuleName = 'DSC-WindowsFirewall-WhatIf-Test' diff --git a/resources/windows_firewall/windows_firewall.dsc.resource.json b/resources/windows_firewall/windows_firewall.dsc.resource.json index 6a587fcd9..58c6f7dc5 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,50 @@ "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.", + "minItems": 1, + "items": { + "type": "string", + "enum": [ + "Domain", + "Private", + "Public", + "All" + ] + } + } + } }, "rules": { "type": "array", From 5f074875c7fb2a37438f50de2bf78ece344111ba Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 17 Aug 2026 18:28:56 +0000 Subject: [PATCH 3/3] Ignore write-only properties in schema-aware diffs (#1674) * Ignore write-only properties in schema diffs Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Resolve local refs for write-only properties Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Omit write-only firewall instructions from set output Co-authored-by: SteveL-MSFT <11859881+SteveL-MSFT@users.noreply.github.com> --------- Co-authored-by: Steve Lee (POWERSHELL HE/HIM) (from Dev Box) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: SteveL-MSFT <11859881+SteveL-MSFT@users.noreply.github.com> --- lib/dsc-lib/src/dscresources/dscresource.rs | 118 +++++++++++++++++- resources/windows_firewall/src/firewall.rs | 2 +- .../windows_firewall_schema_default.tests.ps1 | 30 +++-- .../tests/windows_firewall_set.tests.ps1 | 13 ++ .../windows_firewall.dsc.resource.json | 1 + 5 files changed, 147 insertions(+), 17 deletions(-) diff --git a/lib/dsc-lib/src/dscresources/dscresource.rs b/lib/dsc-lib/src/dscresources/dscresource.rs index 84a6480b7..29eda6a08 100644 --- a/lib/dsc-lib/src/dscresources/dscresource.rs +++ b/lib/dsc-lib/src/dscresources/dscresource.rs @@ -11,7 +11,7 @@ use rust_i18n::t; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use serde_json::{Map, Value}; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::path::PathBuf; use tracing::{debug, info, trace, warn}; @@ -657,14 +657,15 @@ pub fn get_diff(expected: &Value, actual: &Value) -> Vec { #[must_use] /// Performs a comparison of two JSON Values using an optional JSON Schema. -/// If a property exists in `expected` but not in `actual`, the schema's `default` value -/// for that property is used for comparison when available. +/// Properties whose schema sets `writeOnly` to `true` are ignored. If a property exists +/// in `expected` but not in `actual`, the schema's `default` value for that property is +/// used for comparison when available. /// /// # Arguments /// /// * `expected` - The expected value /// * `actual` - The actual value -/// * `schema` - Optional JSON Schema to look up default values for missing properties +/// * `schema` - Optional JSON Schema to identify write-only properties and default values /// /// # Returns /// @@ -691,6 +692,10 @@ pub(crate) fn get_diff_with_schema(expected: &Value, actual: &Value, schema: Opt } for (key, value) in &*map { + if is_schema_write_only(schema, key) { + continue; + } + if is_secure_value(value) { // skip secure values as they are not comparable continue; @@ -726,7 +731,7 @@ pub(crate) fn get_diff_with_schema(expected: &Value, actual: &Value, schema: Opt } } else { // Property not in actual - check schema for a default value - if let Some(default_value) = get_schema_default(schema, key) { + if let Some(default_value) = get_schema_default(schema, key) { if value != &default_value { info!("{}", t!("dscresources.dscresource.diffKeyMissing", key = key)); diff_properties.push(key.to_string()); @@ -764,6 +769,42 @@ fn get_schema_default(schema: Option<&Value>, property_name: &str) -> Option, property_name: &str) -> bool { + let Some(schema) = schema else { + return false; + }; + let Some(mut property_schema) = schema + .get("properties") + .and_then(Value::as_object) + .and_then(|properties| properties.get(property_name)) + else { + return false; + }; + let mut visited_references = HashSet::new(); + + loop { + if property_schema.get("writeOnly").and_then(Value::as_bool) == Some(true) { + return true; + } + + let Some(reference) = property_schema.get("$ref").and_then(Value::as_str) else { + return false; + }; + let Some(pointer) = reference.strip_prefix('#') else { + return false; + }; + if !visited_references.insert(pointer) { + return false; + } + let Some(resolved_schema) = schema.pointer(pointer) else { + return false; + }; + property_schema = resolved_schema; + } +} + /// Validates the properties of a resource against its schema. /// /// # Arguments @@ -1023,6 +1064,73 @@ fn diff_with_schema_no_default_reports_missing_property() { assert_eq!(diff, vec!["enabled".to_string()]); } +#[test] +fn diff_with_schema_write_only_ignores_differing_property() { + use serde_json::json; + let expected = json!({"name": "test", "action": "remove"}); + let actual = json!({"name": "test", "action": "ignore"}); + let schema = json!({ + "type": "object", + "properties": { + "name": { "type": "string" }, + "action": { "type": "string", "writeOnly": true } + } + }); + let diff = get_diff_with_schema(&expected, &actual, Some(&schema)); + assert!(diff.is_empty(), "Expected write-only property to be ignored, got: {diff:?}"); +} + +#[test] +fn diff_with_schema_write_only_ignores_missing_property() { + use serde_json::json; + let expected = json!({"name": "test", "action": "remove"}); + let actual = json!({"name": "test"}); + let schema = json!({ + "type": "object", + "properties": { + "name": { "type": "string" }, + "action": { "type": "string", "writeOnly": true } + } + }); + let diff = get_diff_with_schema(&expected, &actual, Some(&schema)); + assert!(diff.is_empty(), "Expected write-only property to be ignored, got: {diff:?}"); +} + +#[test] +fn diff_with_schema_write_only_local_ref_ignores_missing_property() { + use serde_json::json; + let expected = json!({"name": "test", "action": "remove"}); + let actual = json!({"name": "test"}); + let schema = json!({ + "type": "object", + "properties": { + "name": { "type": "string" }, + "action": { "$ref": "#/$defs/action" } + }, + "$defs": { + "action": { "type": "string", "writeOnly": true } + } + }); + let diff = get_diff_with_schema(&expected, &actual, Some(&schema)); + assert!(diff.is_empty(), "Expected referenced write-only property to be ignored, got: {diff:?}"); +} + +#[test] +fn diff_with_schema_write_only_false_reports_missing_property() { + use serde_json::json; + let expected = json!({"name": "test", "action": "remove"}); + let actual = json!({"name": "test"}); + let schema = json!({ + "type": "object", + "properties": { + "name": { "type": "string" }, + "action": { "type": "string", "writeOnly": false } + } + }); + let diff = get_diff_with_schema(&expected, &actual, Some(&schema)); + assert_eq!(diff, vec!["action".to_string()]); +} + #[test] fn diff_without_schema_reports_missing_property() { use serde_json::json; diff --git a/resources/windows_firewall/src/firewall.rs b/resources/windows_firewall/src/firewall.rs index d825d8ffd..4815e0e3c 100644 --- a/resources/windows_firewall/src/firewall.rs +++ b/resources/windows_firewall/src/firewall.rs @@ -587,7 +587,7 @@ pub fn set_rules(input: &FirewallRuleList, what_if: bool) -> Result {} // None or Ignore: no additional action. } - Ok(FirewallRuleList { rules: results, unspecified_rules: input.unspecified_rules.clone() }) + Ok(FirewallRuleList { rules: results, unspecified_rules: None }) } pub fn export_rules() -> Result { diff --git a/resources/windows_firewall/tests/windows_firewall_schema_default.tests.ps1 b/resources/windows_firewall/tests/windows_firewall_schema_default.tests.ps1 index fa0d6aa46..9c2279cb2 100644 --- a/resources/windows_firewall/tests/windows_firewall_schema_default.tests.ps1 +++ b/resources/windows_firewall/tests/windows_firewall_schema_default.tests.ps1 @@ -26,9 +26,11 @@ Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaul Remove-NetFirewallRule -Name $testRuleName -ErrorAction Ignore } - It 'unspecifiedRulesAction set to default "ignore" does not report as differing' { + It 'unspecifiedRules action "ignore" does not report as differing' { $json = @{ - unspecifiedRulesAction = 'ignore' + unspecifiedRules = @{ + action = 'ignore' + } rules = @(@{ name = $testRuleName direction = 'Inbound' @@ -43,10 +45,10 @@ Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaul $result = $out | ConvertFrom-Json $result.inDesiredState | Should -Be $true - $result.differingProperties | Should -Not -Contain 'unspecifiedRulesAction' + $result.differingProperties | Should -Not -Contain 'unspecifiedRules' } - It 'unspecifiedRulesAction omitted does not report as differing' { + It 'unspecifiedRules omitted does not report as differing' { $json = @{ rules = @(@{ name = $testRuleName @@ -62,12 +64,14 @@ Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaul $result = $out | ConvertFrom-Json $result.inDesiredState | Should -Be $true - $result.differingProperties | Should -Not -Contain 'unspecifiedRulesAction' + $result.differingProperties | Should -Not -Contain 'unspecifiedRules' } - It 'non-default unspecifiedRulesAction "disable" is reported as differing' { + It 'unspecifiedRules action "disable" is ignored for comparison' { $json = @{ - unspecifiedRulesAction = 'disable' + unspecifiedRules = @{ + action = 'disable' + } rules = @(@{ name = $testRuleName direction = 'Inbound' @@ -81,12 +85,15 @@ Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaul $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) $result = $out | ConvertFrom-Json - $result.differingProperties | Should -Contain 'unspecifiedRulesAction' + $result.inDesiredState | Should -Be $true + $result.differingProperties | Should -Not -Contain 'unspecifiedRules' } - It 'non-default unspecifiedRulesAction "remove" is reported as differing' { + It 'unspecifiedRules action "remove" is ignored for comparison' { $json = @{ - unspecifiedRulesAction = 'remove' + unspecifiedRules = @{ + action = 'remove' + } rules = @(@{ name = $testRuleName direction = 'Inbound' @@ -100,6 +107,7 @@ Describe 'Microsoft.Windows/FirewallRuleList - synthetic test with schema defaul $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) $result = $out | ConvertFrom-Json - $result.differingProperties | Should -Contain 'unspecifiedRulesAction' + $result.inDesiredState | Should -Be $true + $result.differingProperties | Should -Not -Contain 'unspecifiedRules' } } diff --git a/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 b/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 index dfa348901..453cd30c1 100644 --- a/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 +++ b/resources/windows_firewall/tests/windows_firewall_set.tests.ps1 @@ -59,6 +59,19 @@ Describe 'Microsoft.Windows/FirewallRuleList - set operation' -Skip:(!$isElevate ($out | ConvertFrom-Json).afterState.rules | Should -BeNullOrEmpty } + It 'does not return unspecifiedRules in the after state' -Skip:(!$isElevated) { + $json = @{ + rules = @() + unspecifiedRules = @{ + action = 'ignore' + } + } | ConvertTo-Json -Compress -Depth 5 + $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.PSObject.Properties.Name | Should -Not -Contain 'unspecifiedRules' + } + It 'updates an existing rule' -Skip:(!$isElevated) { Initialize-TestFirewallRule $json = @{ rules = @(@{ name = $testRuleName; description = 'Updated by DSC test'; enabled = $false }) } | ConvertTo-Json -Compress -Depth 5 diff --git a/resources/windows_firewall/windows_firewall.dsc.resource.json b/resources/windows_firewall/windows_firewall.dsc.resource.json index 58c6f7dc5..d24b1eea9 100644 --- a/resources/windows_firewall/windows_firewall.dsc.resource.json +++ b/resources/windows_firewall/windows_firewall.dsc.resource.json @@ -63,6 +63,7 @@ "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.", + "writeOnly": true, "additionalProperties": false, "required": [ "action"