From a97e46fa80078613c16e9b5dec8c5fac1c527f2c Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Fri, 9 Oct 2026 07:39:49 +0000 Subject: [PATCH] test: guard folder deletion and ownership recovery failures Cover recursive, read-only, locked, long-path, and junction deletion in sandbox folders. Exercise denied ownership retries and both successful and failed restoration after an operation fails, without inconclusive results or a success-shaped hash. The tests fail when folder deletion and owner restoration are omitted: 16 expected failures in the controlled mutation run. Restored code passes all 66 focused tests where applicable in both editions and privilege configurations. No production behavior changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- Tests/FileHash.Tests.ps1 | 34 ++++++-- Tests/PathErrors.Tests.ps1 | 63 ++++++++++++++ Tests/Remove-Item2.Tests.ps1 | 154 ++++++++++++++++++++++++++++++++++- 3 files changed, 245 insertions(+), 6 deletions(-) diff --git a/Tests/FileHash.Tests.ps1 b/Tests/FileHash.Tests.ps1 index 221c7b8..821ed2f 100644 --- a/Tests/FileHash.Tests.ps1 +++ b/Tests/FileHash.Tests.ps1 @@ -98,8 +98,18 @@ Describe 'Get-FileHash2' { } Context 'When the file cannot be read after taking ownership' { - # Before 5.0.0, the account that ran the cmdlet stayed the owner when the second attempt failed. Only an - # elevated process can make another account the owner first, so the test runs in CI. + BeforeAll { + $privateData = (Get-Module -Name NTFSSecurity).PrivateData + $enablePrivileges = $privateData['EnablePrivileges'] + $privateData['EnablePrivileges'] = $false + } + + AfterAll { + $privateData['EnablePrivileges'] = $enablePrivileges + } + + # Disable automatic privileges so that the deny entry reaches the ownership retry even in an elevated process. + # Administrators is an assignable owner for that process, unlike TrustedInstaller. It 'Should restore the previous owner' -Skip:(-not $isElevated) { $denied = New-TestSandboxItem -Sandbox $sandbox -Name 'Denied' Assert-TestSandboxPath -Sandbox $sandbox -Path $denied @@ -107,14 +117,28 @@ Describe 'Get-FileHash2' { Add-NTFSAccess -Path $denied -Account 'S-1-1-0' -AccessRights ReadData -AccessType Deny $results = @(Get-FileHash2 -Path $denied -ErrorVariable hashErrors -ErrorAction SilentlyContinue) - if (-not $hashErrors) { - Set-ItResult -Inconclusive -Because 'the elevated process could read the file despite the deny entry' - } $hashErrors | Should -HaveCount 1 $hashErrors[0].FullyQualifiedErrorId | Should -BeLike 'GetHashError,*' $results | Should -BeNullOrEmpty (Get-NTFSOwner -Path $denied).Owner.Sid | Should -Be 'S-1-5-32-544' } + + It 'Should report both the failed read and the failed owner restoration without returning a hash' -Skip:(-not $isElevated) { + $denied = New-TestSandboxItem -Sandbox $sandbox -Name 'RestoreDenied' + Add-TestDenyRule -Sandbox $sandbox -Path $denied -Rights @{ 'S-1-1-0' = 'ReadData' } + $originalOwner = 'S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464' + Set-TestOwner -Sandbox $sandbox -Path $denied -Sid $originalOwner + (Get-Privileges | Where-Object -Property Privilege -EQ -Value 'Restore').PrivilegeState | Should -Be 'Disabled' + + $result = @(Get-FileHash2 -Path $denied -ErrorVariable hashErrors -ErrorAction SilentlyContinue) + + $result | Should -BeNullOrEmpty + $hashErrors | Should -HaveCount 2 + $hashErrors[0].FullyQualifiedErrorId | Should -BeLike 'RestoreOwnerError,*' + $hashErrors[1].FullyQualifiedErrorId | Should -BeLike 'GetHashError,*' + $hashErrors | ForEach-Object -Process { $_.TargetObject | Should -Be $denied } + (Get-NTFSOwner -Path $denied).Owner.Sid | Should -Be ([Security.Principal.WindowsIdentity]::GetCurrent().User.Value) + } } } diff --git a/Tests/PathErrors.Tests.ps1 b/Tests/PathErrors.Tests.ps1 index 3202c8e..eb613bd 100644 --- a/Tests/PathErrors.Tests.ps1 +++ b/Tests/PathErrors.Tests.ps1 @@ -13,6 +13,7 @@ param () BeforeDiscovery { Import-Module -Name (Join-Path -Path $PSScriptRoot -ChildPath 'TestHelpers.psm1') -Force $holdsSecurityPrivilege = Test-PrivilegeHeld -Name 'SeSecurityPrivilege' + $holdsRestorePrivilege = Test-PrivilegeHeld -Name 'SeRestorePrivilege' $currentUser = [System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value $readEntry = @{ Account = 'S-1-1-0'; AccessRights = 'ReadData' } # An audit entry on a file has no inheritance flags. @@ -151,6 +152,68 @@ Describe 'An item whose owner may not read its permissions' { } } +Describe 'A denied write and a denied ownership retry' { + It ' should keep the denied item unchanged, report , and process the next item' -ForEach @( + @{ Command = 'Add-NTFSAccess'; Parameters = @{ Account = 'S-1-1-0'; AccessRights = 'ReadData' }; ErrorId = 'AddAceError'; Operation = 'Add' } + @{ Command = 'Remove-NTFSAccess'; Parameters = @{ Account = 'S-1-1-0'; AccessRights = 'ReadData' }; ErrorId = 'RemoveAceError'; Operation = 'Remove' } + @{ Command = 'Clear-NTFSAccess'; Parameters = @{}; ErrorId = 'ClearAclError'; Operation = 'Clear' } + @{ Command = 'Disable-NTFSAccessInheritance'; Parameters = @{}; ErrorId = 'ModifySdError'; Operation = 'Disable' } + @{ Command = 'Enable-NTFSAccessInheritance'; Parameters = @{}; ErrorId = 'ModifySdError'; Operation = 'Enable' } + @{ Command = 'Set-NTFSInheritance'; Parameters = @{ AccessInheritanceEnabled = $false }; ErrorId = 'ModifySdError'; Operation = 'Disable' } + ) { + $blocked = New-TestSandboxItem -Sandbox $sandbox -Name 'RetryBlocked' + $next = New-TestSandboxItem -Sandbox $sandbox -Name 'RetryNext' + foreach ($path in $blocked, $next) { + if ($Operation -ne 'Add') { + Add-NTFSAccess -Path $path -Account 'S-1-1-0' -AccessRights ReadData -ErrorAction Stop + } + if ($Operation -eq 'Enable') { + Disable-NTFSAccessInheritance -Path $path -ErrorAction Stop + } + } + Block-TestWritePermission -Sandbox $sandbox -Path $blocked + $before = (Get-TestAcl -Path $blocked).Sddl + + & $Command -Path $blocked, $next @Parameters -ErrorVariable changeErrors -ErrorAction SilentlyContinue + + $changeErrors | Should -HaveCount 1 + $changeErrors[0].FullyQualifiedErrorId | Should -BeLike "$ErrorId,*" + $changeErrors[0].TargetObject | Should -Be $blocked + (Get-TestAcl -Path $blocked).Sddl | Should -BeExactly $before + $acl = Get-TestAcl -Path $next + $everyone = @($acl.GetAccessRules($true, $false, $sidType) | Where-Object -FilterScript { $_.IdentityReference.Value -eq 'S-1-1-0' }) + switch ($Operation) { + 'Add' { $everyone | Should -HaveCount 1 } + 'Remove' { $everyone | Should -BeNullOrEmpty } + 'Clear' { @($acl.GetAccessRules($true, $false, $sidType)) | Should -BeNullOrEmpty } + 'Disable' { $acl.AreAccessRulesProtected | Should -BeTrue } + 'Enable' { $acl.AreAccessRulesProtected | Should -BeFalse } + } + } +} + +Describe 'An owner that the process cannot restore without the Restore privilege' { + It 'Should report RestoreOwnerError after a successful ownership retry and continue with the next path' -Skip:(-not $holdsRestorePrivilege) { + $blocked = New-TestSandboxItem -Sandbox $sandbox -Name 'UnassignableOwner' + $next = New-TestSandboxItem -Sandbox $sandbox -Name 'NextOwner' + $user = [Security.Principal.WindowsIdentity]::GetCurrent().User.Value + Add-TestDenyRule -Sandbox $sandbox -Path $blocked -Rights @{ $user = 'ChangePermissions' } + $originalOwner = 'S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464' + Set-TestOwner -Sandbox $sandbox -Path $blocked -Sid $originalOwner + (Get-Privileges | Where-Object -Property Privilege -EQ -Value 'Restore').PrivilegeState | Should -Be 'Disabled' + + Add-NTFSAccess -Path $blocked, $next -Account 'S-1-1-0' -AccessRights ReadData -ErrorVariable changeErrors -ErrorAction SilentlyContinue + + $changeErrors | Should -HaveCount 1 + $changeErrors[0].FullyQualifiedErrorId | Should -BeLike 'RestoreOwnerError,*' + $changeErrors[0].CategoryInfo.Category | Should -Be 'WriteError' + $changeErrors[0].TargetObject | Should -Be $blocked + (Get-TestAcl -Path $blocked).GetOwner($sidType).Value | Should -Be $user + foreach ($path in $blocked, $next) { + @((Get-TestAcl -Path $path).GetAccessRules($true, $false, $sidType) | Where-Object -FilterScript { $_.IdentityReference.Value -eq 'S-1-1-0' }) | Should -HaveCount 1 + } + } +} Describe 'An item whose owner may not change its permissions' { # A deny entry for OWNER RIGHTS replaces the right of the owner to change the DACL. The cmdlets take ownership, # which Windows answers by removing the OWNER RIGHTS entries, write the DACL, and set the previous owner back. diff --git a/Tests/Remove-Item2.Tests.ps1 b/Tests/Remove-Item2.Tests.ps1 index 853c545..a64c420 100644 --- a/Tests/Remove-Item2.Tests.ps1 +++ b/Tests/Remove-Item2.Tests.ps1 @@ -8,17 +8,169 @@ param () Describe 'Remove-Item2' { BeforeAll { + Import-Module -Name (Join-Path -Path $PSScriptRoot -ChildPath 'TestHelpers.psm1') -Force $modulePath = Join-Path -Path $PSScriptRoot -ChildPath '..\NTFSSecurity\bin\Release\NTFSSecurity.psd1' Import-Module -Name $modulePath -Force -ErrorAction Stop + $sandbox = New-TestSandbox -Name 'RemoveItem' + Push-Location -LiteralPath $sandbox } AfterAll { + Pop-Location + Remove-TestSandbox -Sandbox $sandbox Remove-Module -Name NTFSSecurity -Force -ErrorAction SilentlyContinue } + Context 'Folders and their contents' { + It 'Should remove an empty folder and return its folder object with -PassThru' { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'Empty' -Directory + + $result = @(Remove-Item2 -Path $folder -PassThru -ErrorAction Stop) + + $folder | Should -Not -Exist + $result | Should -HaveCount 1 + $result[0] | Should -BeOfType [Alphaleonis.Win32.Filesystem.DirectoryInfo] + $result[0].FullName | Should -Be $folder + } + + It 'Should report DeleteError for a non-empty folder without -Recurse and continue with the next path' { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'NonEmpty' -Directory + $content = Join-Path -Path $folder -ChildPath 'Keep.txt' + $next = New-TestSandboxItem -Sandbox $sandbox -Name 'Next' + Assert-TestSandboxPath -Sandbox $sandbox -Path $content + Set-Content -LiteralPath $content -Value 'Keep' + + $result = @(Remove-Item2 -Path $folder, $next -PassThru -ErrorVariable removeErrors -ErrorAction SilentlyContinue) + + $removeErrors | Should -HaveCount 1 + $removeErrors[0].FullyQualifiedErrorId | Should -BeLike 'DeleteError,*' + $removeErrors[0].CategoryInfo.Category | Should -Be 'InvalidData' + $removeErrors[0].TargetObject | Should -Be $folder + Get-Content -LiteralPath $content | Should -Be 'Keep' + $next | Should -Not -Exist + $result | Should -HaveCount 1 + $result[0].FullName | Should -Be $next + } + + It 'Should remove a folder tree with -Recurse without touching its sibling' { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'Tree' -Directory + $nested = Join-Path -Path $folder -ChildPath 'Child\Grandchild' + $content = Join-Path -Path $nested -ChildPath 'Delete.txt' + $sibling = New-TestSandboxItem -Sandbox $sandbox -Name 'Sibling' + Assert-TestSandboxPath -Sandbox $sandbox -Path $nested, $content + New-Item -ItemType Directory -Path $nested -Force | Out-Null + Set-Content -LiteralPath $content -Value 'Delete' + + Remove-Item2 -Path $folder -Recurse -ErrorAction Stop + + $folder | Should -Not -Exist + Get-Content -LiteralPath $sibling | Should -Be 'Sibling' + } + + It 'Should leave a folder tree unchanged with -Recurse -Force -WhatIf and write nothing with -PassThru' { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'Preview' -Directory + $content = Join-Path -Path $folder -ChildPath 'Keep.txt' + Assert-TestSandboxPath -Sandbox $sandbox -Path $content + Set-Content -LiteralPath $content -Value 'Keep' + [IO.File]::SetAttributes($content, [IO.FileAttributes]::ReadOnly) + + $result = @(Remove-Item2 -Path $folder -Recurse -Force -WhatIf -PassThru -ErrorAction Stop) + + $result | Should -BeNullOrEmpty + Get-Content -LiteralPath $content | Should -Be 'Keep' + ([IO.File]::GetAttributes($content) -band [IO.FileAttributes]::ReadOnly) | Should -Not -Be 0 + } + + It 'Should remove a folder tree containing read-only files with -Recurse -Force' { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'ReadOnlyTree' -Directory + $content = Join-Path -Path $folder -ChildPath 'Child\ReadOnly.txt' + Assert-TestSandboxPath -Sandbox $sandbox -Path $content + New-Item -ItemType Directory -Path (Split-Path -Path $content -Parent) | Out-Null + Set-Content -LiteralPath $content -Value 'ReadOnly' + [IO.File]::SetAttributes($content, [IO.FileAttributes]::ReadOnly) + + Remove-Item2 -Path $folder -Recurse -Force -ErrorAction Stop + + $folder | Should -Not -Exist + } + + It 'Should write DeleteError when a descendant is open without delete sharing, not a successful -PassThru result' { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'LockedTree' -Directory + $content = Join-Path -Path $folder -ChildPath 'Locked.txt' + Assert-TestSandboxPath -Sandbox $sandbox -Path $content + Set-Content -LiteralPath $content -Value 'Locked' + $stream = [IO.File]::Open($content, [IO.FileMode]::Open, [IO.FileAccess]::Read, [IO.FileShare]::None) + try { + $result = @(Remove-Item2 -Path $folder -Recurse -Force -PassThru -ErrorVariable removeErrors -ErrorAction SilentlyContinue) + } + finally { + $stream.Dispose() + } + + $removeErrors | Should -HaveCount 1 + $removeErrors[0].FullyQualifiedErrorId | Should -BeLike 'DeleteError,*' + $removeErrors[0].TargetObject | Should -Be $folder + $result | Should -BeNullOrEmpty + Get-Content -LiteralPath $content | Should -Be 'Locked' + } + + It 'Should delete a junction with -Recurse without deleting or changing its target' { + $target = New-TestSandboxItem -Sandbox $sandbox -Name 'JunctionTarget' -Directory + $content = Join-Path -Path $target -ChildPath 'Keep.txt' + $link = Join-Path -Path $sandbox -ChildPath 'Junction' + Assert-TestSandboxPath -Sandbox $sandbox -Path $content, $link + Set-Content -LiteralPath $content -Value 'Keep' + $before = (Get-Acl -LiteralPath $target).Sddl + New-Item -ItemType Junction -Path $link -Value $target | Out-Null + + Remove-Item2 -Path $link -Recurse -Force -ErrorAction Stop + + $link | Should -Not -Exist + Get-Content -LiteralPath $content | Should -Be 'Keep' + (Get-Acl -LiteralPath $target).Sddl | Should -BeExactly $before + } + + It 'Should delete a folder tree whose path exceeds 260 characters' { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'LongTree' -Directory + $long = Join-Path -Path $folder -ChildPath (('A' * 100), ('B' * 100), ('C' * 100) -join '\') + Assert-TestSandboxPath -Sandbox $sandbox -Path $long + [IO.Directory]::CreateDirectory('\\?\' + $long) | Out-Null + [IO.File]::WriteAllText(('\\?\' + $long + '\Delete.txt'), 'Long') + $long.Length | Should -BeGreaterThan 260 + + Remove-Item2 -Path $folder -Recurse -Force -ErrorAction Stop + + $folder | Should -Not -Exist + } + } + + Context 'Read-only files' { + It 'Should refuse a read-only file without -Force, keep its contents and attribute, and return nothing' { + $file = New-TestSandboxItem -Sandbox $sandbox -Name 'ReadOnly' + [IO.File]::SetAttributes($file, [IO.FileAttributes]::ReadOnly) + + $result = @(Remove-Item2 -Path $file -PassThru -ErrorVariable removeErrors -ErrorAction SilentlyContinue) + + $removeErrors | Should -HaveCount 1 + $removeErrors[0].FullyQualifiedErrorId | Should -BeLike 'DeleteError,*' + $result | Should -BeNullOrEmpty + Get-Content -LiteralPath $file | Should -Be 'ReadOnly' + ([IO.File]::GetAttributes($file) -band [IO.FileAttributes]::ReadOnly) | Should -Not -Be 0 + } + + It 'Should remove a read-only file with -Force' { + $file = New-TestSandboxItem -Sandbox $sandbox -Name 'Forced' + [IO.File]::SetAttributes($file, [IO.FileAttributes]::ReadOnly) + + Remove-Item2 -Path $file -Force -ErrorAction Stop + + $file | Should -Not -Exist + } + } Context 'When called with -PassThur, the parameter name in 4.2.6 and earlier' { BeforeAll { - $path = Join-Path -Path $TestDrive -ChildPath 'PassThur.txt' + $path = Join-Path -Path $sandbox -ChildPath 'PassThur.txt' + Assert-TestSandboxPath -Sandbox $sandbox -Path $path Set-Content -LiteralPath $path -Value 'Remove-Item2 test' $removedItem = Remove-Item2 -Path $path -PassThur