From 62699eeb7b98febc86d0a2ea1d8266f958e0a85c Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Wed, 21 Oct 2026 12:44:08 +0000 Subject: [PATCH] fix(access): report a failing change of a descriptor as an error per descriptor In the four -SecurityDescriptor parameter sets of Add-NTFSAccess, Remove-NTFSAccess, Add-NTFSAudit, and Remove-NTFSAudit, a change that throws, such as a Deny entry without rights, ended the cmdlet with a terminating error, while the -Path parameter sets already wrote an AddAceError or RemoveAceError for the failing item and went on to the next one. The two kinds of parameter sets now behave the same way. Each descriptor's change is wrapped in its own try/catch: a failure writes a non-terminating AddAceError or RemoveAceError, category WriteError, that targets the descriptor, and the cmdlet continues with the next descriptor in the pipeline instead of stopping. -PassThru stays outside the try, so what a later command raises when the cmdlet writes its output is never caught there. The existing TestAuditSection check of the audit cmdlets, and everything else about a successful change, is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- NTFSSecurity/AccessCmdlets/AddAccess.cs | 10 +- NTFSSecurity/AccessCmdlets/RemoveAccess.cs | 10 +- NTFSSecurity/AuditCmdlets/AddAudit.cs | 10 +- NTFSSecurity/AuditCmdlets/RemoveAudit.cs | 10 +- Tests/ParameterSets.Access.Tests.ps1 | 104 +++++++++++++++++++++ Tests/ParameterSets.Audit.Tests.ps1 | 81 ++++++++++++++++ 6 files changed, 221 insertions(+), 4 deletions(-) diff --git a/NTFSSecurity/AccessCmdlets/AddAccess.cs b/NTFSSecurity/AccessCmdlets/AddAccess.cs index 03c907b..249874d 100644 --- a/NTFSSecurity/AccessCmdlets/AddAccess.cs +++ b/NTFSSecurity/AccessCmdlets/AddAccess.cs @@ -210,7 +210,15 @@ namespace NTFSSecurity { foreach (var sd in securityDescriptors) { - FileSystemAccessRule2.AddFileSystemAccessRule(sd, account.ToList(), accessRights, accessTypeValue, inheritance, propagation); + try + { + FileSystemAccessRule2.AddFileSystemAccessRule(sd, account.ToList(), accessRights, accessTypeValue, inheritance, propagation); + } + catch (Exception ex) + { + WriteError(new ErrorRecord(ex, "AddAceError", ErrorCategory.WriteError, sd)); + continue; + } if (passThru == true) { diff --git a/NTFSSecurity/AccessCmdlets/RemoveAccess.cs b/NTFSSecurity/AccessCmdlets/RemoveAccess.cs index c0884b1..d2ac020 100644 --- a/NTFSSecurity/AccessCmdlets/RemoveAccess.cs +++ b/NTFSSecurity/AccessCmdlets/RemoveAccess.cs @@ -221,7 +221,15 @@ namespace NTFSSecurity { foreach (var sd in securityDescriptors) { - FileSystemAccessRule2.RemoveFileSystemAccessRule(sd, account.ToList(), accessRights, accessTypeValue, inheritance, propagation, removeSpecific); + try + { + FileSystemAccessRule2.RemoveFileSystemAccessRule(sd, account.ToList(), accessRights, accessTypeValue, inheritance, propagation, removeSpecific); + } + catch (Exception ex) + { + WriteError(new ErrorRecord(ex, "RemoveAceError", ErrorCategory.WriteError, sd)); + continue; + } if (passThru == true) { diff --git a/NTFSSecurity/AuditCmdlets/AddAudit.cs b/NTFSSecurity/AuditCmdlets/AddAudit.cs index 43ca42f..fa3f8da 100644 --- a/NTFSSecurity/AuditCmdlets/AddAudit.cs +++ b/NTFSSecurity/AuditCmdlets/AddAudit.cs @@ -214,7 +214,15 @@ namespace NTFSSecurity continue; } - FileSystemAuditRule2.AddFileSystemAuditRule(sd, account.ToList(), accessRights, auditFlagsValue, inheritance, propagation); + try + { + FileSystemAuditRule2.AddFileSystemAuditRule(sd, account.ToList(), accessRights, auditFlagsValue, inheritance, propagation); + } + catch (Exception ex) + { + WriteError(new ErrorRecord(ex, "AddAceError", ErrorCategory.WriteError, sd)); + continue; + } if (passThru == true) { diff --git a/NTFSSecurity/AuditCmdlets/RemoveAudit.cs b/NTFSSecurity/AuditCmdlets/RemoveAudit.cs index 2814010..6d308e1 100644 --- a/NTFSSecurity/AuditCmdlets/RemoveAudit.cs +++ b/NTFSSecurity/AuditCmdlets/RemoveAudit.cs @@ -225,7 +225,15 @@ namespace NTFSSecurity continue; } - FileSystemAuditRule2.RemoveFileSystemAuditRule(sd, account.ToList(), accessRights, auditFlagsValue, inheritance, propagation, removeSpecific); + try + { + FileSystemAuditRule2.RemoveFileSystemAuditRule(sd, account.ToList(), accessRights, auditFlagsValue, inheritance, propagation, removeSpecific); + } + catch (Exception ex) + { + WriteError(new ErrorRecord(ex, "RemoveAceError", ErrorCategory.WriteError, sd)); + continue; + } if (passThru == true) { diff --git a/Tests/ParameterSets.Access.Tests.ps1 b/Tests/ParameterSets.Access.Tests.ps1 index 8d3094c..af20c36 100644 --- a/Tests/ParameterSets.Access.Tests.ps1 +++ b/Tests/ParameterSets.Access.Tests.ps1 @@ -418,6 +418,57 @@ Describe 'Add-NTFSAccess' { (Get-Acl -LiteralPath $second).Sddl | Should -BeExactly $before[1] } } + + # Item 5 of Decision 27: a failing change to one of several descriptors (a Deny entry without rights, which .NET + # refuses to build, the same as Access.Tests.ps1:774 pins for -Path) reports an error for that descriptor and goes + # on with the next one, instead of ending the cmdlet with a terminating error. + Context 'A failing change to one of several descriptors' { + It 'Should write an AddAceError for the descriptor whose change fails in the SDSimple set, continue with the next, and return -PassThru only for the one that succeeded' { + $first = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorSimpleFirst' + $second = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorSimpleSecond' + $descriptors = @(Get-NTFSSecurityDescriptor -Path $first, $second -ErrorAction Stop) + $before = $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Access') + + $rows = @( + [pscustomobject]@{ SecurityDescriptor = $descriptors[0]; Account = 'S-1-1-0'; AccessRights = 'None'; AccessType = 'Deny'; AppliesTo = 'ThisFolderOnly' } + [pscustomobject]@{ SecurityDescriptor = $descriptors[1]; Account = 'S-1-1-0'; AccessRights = 'ReadData'; AccessType = 'Deny'; AppliesTo = 'ThisFolderOnly' } + ) + + $result = @($rows | Add-NTFSAccess -PassThru -ErrorVariable sdErrors -ErrorAction SilentlyContinue) + + $sdErrors | Should -HaveCount 1 + $sdErrors[0].FullyQualifiedErrorId | Should -BeLike 'AddAceError,*' + $sdErrors[0].CategoryInfo.Category | Should -Be 'WriteError' + $sdErrors[0].TargetObject | Should -Be $descriptors[0] + $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Access') | Should -BeExactly $before + $result | Should -Not -BeNullOrEmpty + $result | ForEach-Object -Process { $_.FullName | Should -Be $second } + @($result | Where-Object -FilterScript { $_.Account.Sid -eq 'S-1-1-0' -and $_.AccessControlType -eq 'Deny' }) | Should -HaveCount 1 + } + + It 'Should write an AddAceError for the descriptor whose change fails in the SDComplex set, continue with the next, and return -PassThru only for the one that succeeded' { + $first = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorComplexFirst' + $second = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorComplexSecond' + $descriptors = @(Get-NTFSSecurityDescriptor -Path $first, $second -ErrorAction Stop) + $before = $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Access') + + $rows = @( + [pscustomobject]@{ SecurityDescriptor = $descriptors[0]; Account = 'S-1-1-0'; AccessRights = 'None'; AccessType = 'Deny' } + [pscustomobject]@{ SecurityDescriptor = $descriptors[1]; Account = 'S-1-1-0'; AccessRights = 'ReadData'; AccessType = 'Deny' } + ) + + $result = @($rows | Add-NTFSAccess -PassThru -ErrorVariable sdErrors -ErrorAction SilentlyContinue) + + $sdErrors | Should -HaveCount 1 + $sdErrors[0].FullyQualifiedErrorId | Should -BeLike 'AddAceError,*' + $sdErrors[0].CategoryInfo.Category | Should -Be 'WriteError' + $sdErrors[0].TargetObject | Should -Be $descriptors[0] + $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Access') | Should -BeExactly $before + $result | Should -Not -BeNullOrEmpty + $result | ForEach-Object -Process { $_.FullName | Should -Be $second } + @($result | Where-Object -FilterScript { $_.Account.Sid -eq 'S-1-1-0' -and $_.AccessControlType -eq 'Deny' }) | Should -HaveCount 1 + } + } } Describe 'Remove-NTFSAccess' { @@ -736,6 +787,59 @@ Describe 'Remove-NTFSAccess' { } } } + + # Item 5 of Decision 27: a failing change to one of several descriptors (a Deny entry without rights, which .NET + # refuses to build, the same as Access.Tests.ps1:571 pins for -Path) reports an error for that descriptor and goes + # on with the next one, instead of ending the cmdlet with a terminating error. + Context 'A failing change to one of several descriptors' { + It 'Should write a RemoveAceError for the descriptor whose change fails in the SDSimple set, continue with the next, and return -PassThru only for the one that succeeded' { + $first = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorSimpleFirst' + $second = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorSimpleSecond' + Add-NTFSAccess -Path $first, $second -Account 'S-1-1-0' -AccessRights ReadData -AccessType Deny -AppliesTo ThisFolderOnly -ErrorAction Stop + $descriptors = @(Get-NTFSSecurityDescriptor -Path $first, $second -ErrorAction Stop) + $before = $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Access') + + $rows = @( + [pscustomobject]@{ SecurityDescriptor = $descriptors[0]; Account = 'S-1-1-0'; AccessRights = 'None'; AccessType = 'Deny'; AppliesTo = 'ThisFolderOnly' } + [pscustomobject]@{ SecurityDescriptor = $descriptors[1]; Account = 'S-1-1-0'; AccessRights = 'ReadData'; AccessType = 'Deny'; AppliesTo = 'ThisFolderOnly' } + ) + + $result = @($rows | Remove-NTFSAccess -PassThru -ErrorVariable sdErrors -ErrorAction SilentlyContinue) + + $sdErrors | Should -HaveCount 1 + $sdErrors[0].FullyQualifiedErrorId | Should -BeLike 'RemoveAceError,*' + $sdErrors[0].CategoryInfo.Category | Should -Be 'WriteError' + $sdErrors[0].TargetObject | Should -Be $descriptors[0] + $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Access') | Should -BeExactly $before + $result | Should -Not -BeNullOrEmpty + $result | ForEach-Object -Process { $_.FullName | Should -Be $second } + @($result | Where-Object -FilterScript { $_.Account.Sid -eq 'S-1-1-0' }) | Should -BeNullOrEmpty + } + + It 'Should write a RemoveAceError for the descriptor whose change fails in the SDComplex set, continue with the next, and return -PassThru only for the one that succeeded' { + $first = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorComplexFirst' + $second = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorComplexSecond' + Add-NTFSAccess -Path $first, $second -Account 'S-1-1-0' -AccessRights ReadData -AccessType Deny -ErrorAction Stop + $descriptors = @(Get-NTFSSecurityDescriptor -Path $first, $second -ErrorAction Stop) + $before = $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Access') + + $rows = @( + [pscustomobject]@{ SecurityDescriptor = $descriptors[0]; Account = 'S-1-1-0'; AccessRights = 'None'; AccessType = 'Deny' } + [pscustomobject]@{ SecurityDescriptor = $descriptors[1]; Account = 'S-1-1-0'; AccessRights = 'ReadData'; AccessType = 'Deny' } + ) + + $result = @($rows | Remove-NTFSAccess -PassThru -ErrorVariable sdErrors -ErrorAction SilentlyContinue) + + $sdErrors | Should -HaveCount 1 + $sdErrors[0].FullyQualifiedErrorId | Should -BeLike 'RemoveAceError,*' + $sdErrors[0].CategoryInfo.Category | Should -Be 'WriteError' + $sdErrors[0].TargetObject | Should -Be $descriptors[0] + $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Access') | Should -BeExactly $before + $result | Should -Not -BeNullOrEmpty + $result | ForEach-Object -Process { $_.FullName | Should -Be $second } + @($result | Where-Object -FilterScript { $_.Account.Sid -eq 'S-1-1-0' }) | Should -BeNullOrEmpty + } + } } Describe 'Clear-NTFSAccess' { diff --git a/Tests/ParameterSets.Audit.Tests.ps1 b/Tests/ParameterSets.Audit.Tests.ps1 index a4c6a7a..046bfd4 100644 --- a/Tests/ParameterSets.Audit.Tests.ps1 +++ b/Tests/ParameterSets.Audit.Tests.ps1 @@ -357,6 +357,55 @@ Describe 'Add-NTFSAudit' { (Get-TestAccountAuditRule -Acl $sd.SecurityDescriptor -Account $probe) | Should -HaveCount 1 } } + + # Item 5 of Decision 27: a failing change to one of several descriptors (an audit entry without rights, which + # .NET refuses to build, the same as Audit.Tests.ps1:141 pins for -Path) reports an error for that descriptor and + # goes on with the next one, instead of ending the cmdlet with a terminating error. + Context 'A failing change to one of several descriptors' { + It 'Should write an AddAceError for the descriptor whose change fails in the SDSimple set, continue with the next, and return -PassThru only for the one that succeeded' -Skip:(-not $canReadAudit) { + $first = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorSimpleFirst' + $second = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorSimpleSecond' + $descriptors = @(Get-NTFSSecurityDescriptor -Path $first, $second -ErrorAction Stop) + $before = $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Audit') + + $rows = @( + [pscustomobject]@{ SecurityDescriptor = $descriptors[0]; Account = $probe; AccessRights = 'None'; AuditFlags = 'Success'; AppliesTo = 'ThisFolderOnly' } + [pscustomobject]@{ SecurityDescriptor = $descriptors[1]; Account = $probe; AccessRights = 'ReadData'; AuditFlags = 'Success'; AppliesTo = 'ThisFolderOnly' } + ) + + $result = @($rows | Add-NTFSAudit -PassThru -ErrorVariable sdErrors -ErrorAction SilentlyContinue) + + $sdErrors | Should -HaveCount 1 + $sdErrors[0].FullyQualifiedErrorId | Should -BeLike 'AddAceError,*' + $sdErrors[0].CategoryInfo.Category | Should -Be 'WriteError' + $sdErrors[0].TargetObject | Should -Be $descriptors[0] + $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Audit') | Should -BeExactly $before + $result | Should -Not -BeNullOrEmpty + $result | ForEach-Object -Process { $_.FullName | Should -Be $second } + } + + It 'Should write an AddAceError for the descriptor whose change fails in the SDComplex set, continue with the next, and return -PassThru only for the one that succeeded' -Skip:(-not $canReadAudit) { + $first = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorComplexFirst' + $second = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorComplexSecond' + $descriptors = @(Get-NTFSSecurityDescriptor -Path $first, $second -ErrorAction Stop) + $before = $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Audit') + + $rows = @( + [pscustomobject]@{ SecurityDescriptor = $descriptors[0]; Account = $probe; AccessRights = 'None'; AuditFlags = 'Success' } + [pscustomobject]@{ SecurityDescriptor = $descriptors[1]; Account = $probe; AccessRights = 'ReadData'; AuditFlags = 'Success' } + ) + + $result = @($rows | Add-NTFSAudit -PassThru -ErrorVariable sdErrors -ErrorAction SilentlyContinue) + + $sdErrors | Should -HaveCount 1 + $sdErrors[0].FullyQualifiedErrorId | Should -BeLike 'AddAceError,*' + $sdErrors[0].CategoryInfo.Category | Should -Be 'WriteError' + $sdErrors[0].TargetObject | Should -Be $descriptors[0] + $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Audit') | Should -BeExactly $before + $result | Should -Not -BeNullOrEmpty + $result | ForEach-Object -Process { $_.FullName | Should -Be $second } + } + } } Describe 'Remove-NTFSAudit' { @@ -605,6 +654,38 @@ Describe 'Remove-NTFSAudit' { Get-TestDaclSddl -Path $file | Should -BeExactly $daclBefore } } + + # Item 5 of Decision 27: a failing change to one of several descriptors (an audit entry without rights, which + # .NET refuses to build, the same as Audit.Tests.ps1:141 pins for -Path) reports an error for that descriptor and + # goes on with the next one, instead of ending the cmdlet with a terminating error. Only the SDComplex set is + # listed as open in Quality-Gate-Test-Round-2026-10-20-ParameterSets.csv (row 26); SDSimple is already closed. + Context 'A failing change to one of several descriptors' { + It 'Should write a RemoveAceError for the descriptor whose change fails in the SDComplex set, continue with the next, and return -PassThru only for the one that succeeded' -Skip:(-not $canReadAudit) { + $first = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorComplexFirst' + $second = New-TestSandboxItem -Sandbox $sandbox -Name 'SdErrorComplexSecond' + # A keeper entry for a different account on both items: audit entries are not inherited by default, so + # without one, -PassThru of the descriptor that succeeds would show nothing once its only entry is gone. + Add-NTFSAudit -Path $first, $second -Account $probe -AccessRights ReadData -AuditFlags Success -ErrorAction Stop + Add-NTFSAudit -Path $first, $second -Account $keeper -AccessRights Delete -AuditFlags Success -ErrorAction Stop + $descriptors = @(Get-NTFSSecurityDescriptor -Path $first, $second -ErrorAction Stop) + $before = $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Audit') + + $rows = @( + [pscustomobject]@{ SecurityDescriptor = $descriptors[0]; Account = $probe; AccessRights = 'None'; AuditFlags = 'Success' } + [pscustomobject]@{ SecurityDescriptor = $descriptors[1]; Account = $probe; AccessRights = 'ReadData'; AuditFlags = 'Success' } + ) + + $result = @($rows | Remove-NTFSAudit -PassThru -ErrorVariable sdErrors -ErrorAction SilentlyContinue) + + $sdErrors | Should -HaveCount 1 + $sdErrors[0].FullyQualifiedErrorId | Should -BeLike 'RemoveAceError,*' + $sdErrors[0].CategoryInfo.Category | Should -Be 'WriteError' + $sdErrors[0].TargetObject | Should -Be $descriptors[0] + $descriptors[0].SecurityDescriptor.GetSecurityDescriptorSddlForm('Audit') | Should -BeExactly $before + $result | Should -Not -BeNullOrEmpty + $result | ForEach-Object -Process { $_.FullName | Should -Be $second } + } + } } Describe 'Clear-NTFSAudit' {