From 695d68736e5a2baaa6990487aa4e3b0f331d4a89 Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Mon, 5 Oct 2026 01:26:06 +0200 Subject: [PATCH 1/7] fix: continue with the next path in the *-Item2 cmdlets after an error Defect 18. Copy-Item2, Move-Item2, and Remove-Item2 left ProcessRecord with "return" when a path didn't exist and, for copy and move, when the destination file existed without -Force, so the remaining paths of the same -Path array were not processed. They now write the error and continue with the next path. Tests/ItemCmdlets.Tests.ps1: 5 tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- CHANGELOG.md | 3 ++ Docs/Cmdlets/Copy-Item2.md | 2 +- Docs/Cmdlets/Move-Item2.md | 2 +- Docs/Cmdlets/Remove-Item2.md | 2 +- NTFSSecurity/ItemCmdlets/CopyItem2.cs | 4 +-- NTFSSecurity/ItemCmdlets/MoveItem2.cs | 4 +-- NTFSSecurity/ItemCmdlets/RemoveItem2.cs | 2 +- NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml | 6 ++-- Tests/ItemCmdlets.Tests.ps1 | 38 ++++++++++++++++++++ 9 files changed, 52 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ee92b2..d11fc8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -112,5 +112,8 @@ The format is based on - Restore the `-RemoveSpecific` switch of `Remove-NTFSAccess`, which version 4.1 introduced but later versions lacked, and add it to `Remove-NTFSAudit`: with it, the cmdlets remove only an entry that matches exactly +- Fix `Copy-Item2`, `Move-Item2`, and `Remove-Item2`, which skipped the + remaining paths of `-Path` after a path that didn't exist or, for copy and + move, a file that already existed at the destination [Unreleased]: https://github.com/raandree/NTFSSecurity/compare/4.2.6...HEAD diff --git a/Docs/Cmdlets/Copy-Item2.md b/Docs/Cmdlets/Copy-Item2.md index 025dd6d..9bd30c6 100644 --- a/Docs/Cmdlets/Copy-Item2.md +++ b/Docs/Cmdlets/Copy-Item2.md @@ -186,7 +186,7 @@ By default this cmdlet returns nothing. With `-PassThru $true` it returns an `Al Before 5.0.0, copying a folder that contained files failed with a `CopyError` that reported a `DirectoryNotFoundException` for the first file in the folder. -If a path in `-Path` does not exist or the destination file exists and `-Force` is missing, the cmdlet writes a non-terminating error and skips the remaining paths that were passed in the same call. Items that arrive one by one through the pipeline are not affected, because each of them is processed separately. +If a path in `-Path` does not exist or the destination file exists and `-Force` is missing, the cmdlet writes a non-terminating error and continues with the next path. Before 5.0.0, it skipped the remaining paths that were passed in the same call. ## RELATED LINKS diff --git a/Docs/Cmdlets/Move-Item2.md b/Docs/Cmdlets/Move-Item2.md index addffd2..b4f3eff 100644 --- a/Docs/Cmdlets/Move-Item2.md +++ b/Docs/Cmdlets/Move-Item2.md @@ -186,7 +186,7 @@ By default this cmdlet returns nothing. With `-PassThru $true` it returns an `Al The cmdlet chooses between two mutually exclusive move options. Without `-Force` it moves with `CopyAllowed`, which permits a file to cross volume boundaries because Windows then copies and deletes it. With `-Force` it moves with `ReplaceExisting`, which overwrites the destination but does not request `CopyAllowed`, so a move across volumes can fail when `-Force` is specified. -If a path in `-Path` does not exist or the destination file exists and `-Force` is missing, the cmdlet writes a non-terminating error and skips the remaining paths that were passed in the same call. Items that arrive one by one through the pipeline are not affected, because each of them is processed separately. +If a path in `-Path` does not exist or the destination file exists and `-Force` is missing, the cmdlet writes a non-terminating error and continues with the next path. Before 5.0.0, it skipped the remaining paths that were passed in the same call. ## RELATED LINKS diff --git a/Docs/Cmdlets/Remove-Item2.md b/Docs/Cmdlets/Remove-Item2.md index 40d1bad..0669f84 100644 --- a/Docs/Cmdlets/Remove-Item2.md +++ b/Docs/Cmdlets/Remove-Item2.md @@ -179,7 +179,7 @@ By default this cmdlet returns nothing. With `-PassThru` it returns an `Alphaleo The module defines the aliases `rm2` and `del2` for this cmdlet. -A path that does not exist causes the error `FileNotFound`, and a deletion that the file system rejects causes a `DeleteError`. In both cases the cmdlet skips the remaining paths that were passed in the same call. Items that arrive one by one through the pipeline are not affected, because each of them is processed separately. +A path that does not exist causes the error `FileNotFound`, and a deletion that the file system rejects causes a `DeleteError`. In both cases the cmdlet continues with the next path. Before 5.0.0, a path that did not exist made the cmdlet skip the remaining paths that were passed in the same call. ## RELATED LINKS diff --git a/NTFSSecurity/ItemCmdlets/CopyItem2.cs b/NTFSSecurity/ItemCmdlets/CopyItem2.cs index 8e27a1b..3e391d9 100644 --- a/NTFSSecurity/ItemCmdlets/CopyItem2.cs +++ b/NTFSSecurity/ItemCmdlets/CopyItem2.cs @@ -71,7 +71,7 @@ namespace NTFSSecurity catch (System.IO.FileNotFoundException ex) { WriteError(new ErrorRecord(ex, "FileNotFound", ErrorCategory.ObjectNotFound, resolvedPath)); - return; + continue; } //destination is a directory @@ -88,7 +88,7 @@ namespace NTFSSecurity if (!force & File.Exists(actualDestination)) { WriteError(new ErrorRecord(new AlreadyExistsException(), "DestinationFileAlreadyExists", ErrorCategory.ResourceExists, actualDestination)); - return; + continue; } try diff --git a/NTFSSecurity/ItemCmdlets/MoveItem2.cs b/NTFSSecurity/ItemCmdlets/MoveItem2.cs index 4e93b51..2a360e1 100644 --- a/NTFSSecurity/ItemCmdlets/MoveItem2.cs +++ b/NTFSSecurity/ItemCmdlets/MoveItem2.cs @@ -71,7 +71,7 @@ namespace NTFSSecurity catch (System.IO.FileNotFoundException ex) { WriteError(new ErrorRecord(ex, "FileNotFound", ErrorCategory.ObjectNotFound, resolvedPath)); - return; + continue; } //destination is a directory @@ -88,7 +88,7 @@ namespace NTFSSecurity if (!force & File.Exists(actualDestination)) { WriteError(new ErrorRecord(new AlreadyExistsException(), "DestinationFileAlreadyExists", ErrorCategory.ResourceExists, actualDestination)); - return; + continue; } try diff --git a/NTFSSecurity/ItemCmdlets/RemoveItem2.cs b/NTFSSecurity/ItemCmdlets/RemoveItem2.cs index 0d3a135..de34ca0 100644 --- a/NTFSSecurity/ItemCmdlets/RemoveItem2.cs +++ b/NTFSSecurity/ItemCmdlets/RemoveItem2.cs @@ -66,7 +66,7 @@ namespace NTFSSecurity catch (System.IO.FileNotFoundException ex) { WriteError(new ErrorRecord(ex, "FileNotFound", ErrorCategory.ObjectNotFound, path)); - return; + continue; } try diff --git a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml index 826e823..957a61d 100644 --- a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml +++ b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml @@ -2174,7 +2174,7 @@ PS C:\> Set-NTFSSecurityDescriptor -SecurityDescriptor $sd `Copy-Item2` copies through the AlphaFS library (`Alphaleonis.Win32.Filesystem`), which is why it handles source and destination paths that exceed the 260-character `MAX_PATH` limit of the built-in `Copy-Item` cmdlet. Before 5.0.0, copying a folder that contained files failed with a `CopyError` that reported a `DirectoryNotFoundException` for the first file in the folder. - If a path in `-Path` does not exist or the destination file exists and `-Force` is missing, the cmdlet writes a non-terminating error and skips the remaining paths that were passed in the same call. Items that arrive one by one through the pipeline are not affected, because each of them is processed separately. + If a path in `-Path` does not exist or the destination file exists and `-Force` is missing, the cmdlet writes a non-terminating error and continues with the next path. Before 5.0.0, it skipped the remaining paths that were passed in the same call. @@ -6945,7 +6945,7 @@ PS C:\Data> Get-NTFSSecurityDescriptor `Move-Item2` moves through the AlphaFS library (`Alphaleonis.Win32.Filesystem`), which is why it handles source and destination paths that exceed the 260-character `MAX_PATH` limit of the built-in `Move-Item` cmdlet. The cmdlet chooses between two mutually exclusive move options. Without `-Force` it moves with `CopyAllowed`, which permits a file to cross volume boundaries because Windows then copies and deletes it. With `-Force` it moves with `ReplaceExisting`, which overwrites the destination but does not request `CopyAllowed`, so a move across volumes can fail when `-Force` is specified. - If a path in `-Path` does not exist or the destination file exists and `-Force` is missing, the cmdlet writes a non-terminating error and skips the remaining paths that were passed in the same call. Items that arrive one by one through the pipeline are not affected, because each of them is processed separately. + If a path in `-Path` does not exist or the destination file exists and `-Force` is missing, the cmdlet writes a non-terminating error and continues with the next path. Before 5.0.0, it skipped the remaining paths that were passed in the same call. @@ -7551,7 +7551,7 @@ PS C:\Data> Get-NTFSSecurityDescriptor `Remove-Item2` deletes through the AlphaFS library (`Alphaleonis.Win32.Filesystem`), which is why it reaches items whose path exceeds the 260-character `MAX_PATH` limit of the built-in `Remove-Item` cmdlet. Deletion is permanent; the cmdlet does not use the Recycle Bin. The module defines the aliases `rm2` and `del2` for this cmdlet. - A path that does not exist causes the error `FileNotFound`, and a deletion that the file system rejects causes a `DeleteError`. In both cases the cmdlet skips the remaining paths that were passed in the same call. Items that arrive one by one through the pipeline are not affected, because each of them is processed separately. + A path that does not exist causes the error `FileNotFound`, and a deletion that the file system rejects causes a `DeleteError`. In both cases the cmdlet continues with the next path. Before 5.0.0, a path that did not exist made the cmdlet skip the remaining paths that were passed in the same call. diff --git a/Tests/ItemCmdlets.Tests.ps1 b/Tests/ItemCmdlets.Tests.ps1 index 3a7ca2c..31a5f48 100644 --- a/Tests/ItemCmdlets.Tests.ps1 +++ b/Tests/ItemCmdlets.Tests.ps1 @@ -80,6 +80,44 @@ Describe 'Get-ChildItem2' { } } +Describe 'Copy-Item2, Move-Item2, and Remove-Item2 with several paths' { + BeforeEach { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'Several' -Directory + $missing = Join-Path -Path $folder -ChildPath 'Missing.txt' + $first = Join-Path -Path $folder -ChildPath 'First.txt' + $second = Join-Path -Path $folder -ChildPath 'Second.txt' + $destination = Join-Path -Path $folder -ChildPath 'Destination' + Assert-TestSandboxPath -Sandbox $sandbox -Path $missing, $first, $second, $destination + Set-Content -LiteralPath $first -Value 'First' + Set-Content -LiteralPath $second -Value 'Second' + New-Item -ItemType Directory -Path $destination | Out-Null + } + + # Before 5.0.0, the cmdlets stopped processing -Path at the first failing path. + It 'Remove-Item2 should continue after a path that does not exist' { + Remove-Item2 -Path $missing, $first -ErrorVariable itemErrors -ErrorAction SilentlyContinue + + $itemErrors | Should -HaveCount 1 + $first | Should -Not -Exist + } + + It '<_> should continue after a path that does not exist' -ForEach @('Copy-Item2', 'Move-Item2') { + & $_ -Path $missing, $first -Destination $destination -ErrorVariable itemErrors -ErrorAction SilentlyContinue + + $itemErrors | Should -HaveCount 1 + Join-Path -Path $destination -ChildPath 'First.txt' | Should -Exist + } + + It '<_> should continue after a file that exists at the destination' -ForEach @('Copy-Item2', 'Move-Item2') { + Set-Content -LiteralPath (Join-Path -Path $destination -ChildPath 'First.txt') -Value 'Existing' + + & $_ -Path $first, $second -Destination $destination -ErrorVariable itemErrors -ErrorAction SilentlyContinue + + $itemErrors | Should -HaveCount 1 + Join-Path -Path $destination -ChildPath 'Second.txt' | Should -Exist + } +} + Describe 'Copy-Item2' { Context 'When -Path is a folder with files and subfolders' { BeforeAll { From d679b06ea2b982500657fcd6469b5ebe1383af0a Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Mon, 5 Oct 2026 01:28:50 +0200 Subject: [PATCH 2/7] fix: stop processing a path that Remove-NTFSAccess could not read Defect 19, and the same gap in Remove-NTFSAudit. After the ReadFileError for a path that doesn't exist, both cmdlets went on with a null item: the removal failed with a NullReferenceException that they reported as a second, misleading RemoveAceError, and with -PassThru the null item stopped the command. Both now continue with the next path. Tests: Access.Tests.ps1 and Audit.Tests.ps1, 2 tests each. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- CHANGELOG.md | 3 +++ Docs/Cmdlets/Remove-NTFSAccess.md | 2 ++ Docs/Cmdlets/Remove-NTFSAudit.md | 2 ++ NTFSSecurity/AccessCmdlets/RemoveAccess.cs | 1 + NTFSSecurity/AuditCmdlets/RemoveAudit.cs | 1 + NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml | 2 ++ Tests/Access.Tests.ps1 | 19 +++++++++++++++++++ Tests/Audit.Tests.ps1 | 19 +++++++++++++++++++ 8 files changed, 49 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d11fc8f..b4146ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -115,5 +115,8 @@ The format is based on - Fix `Copy-Item2`, `Move-Item2`, and `Remove-Item2`, which skipped the remaining paths of `-Path` after a path that didn't exist or, for copy and move, a file that already existed at the destination +- Fix `Remove-NTFSAccess` and `Remove-NTFSAudit`, which went on with a path + that didn't exist, wrote a second, misleading `RemoveAceError`, and with + `-PassThru` stopped with a `NullReferenceException` [Unreleased]: https://github.com/raandree/NTFSSecurity/compare/4.2.6...HEAD diff --git a/Docs/Cmdlets/Remove-NTFSAccess.md b/Docs/Cmdlets/Remove-NTFSAccess.md index 9eda4d0..6aa0d3d 100644 --- a/Docs/Cmdlets/Remove-NTFSAccess.md +++ b/Docs/Cmdlets/Remove-NTFSAccess.md @@ -306,6 +306,8 @@ Removing rights from an entry that does not exist is not an error; the cmdlet le Before 5.0.0, the `-RemoveSpecific` switch was missing, although version 4.1 had introduced it. +A path that does not exist produces the non-terminating error `ReadFileError`, and the cmdlet continues with the next path. Before 5.0.0, the cmdlet also wrote a misleading `RemoveAceError` for that path, and with `-PassThru` it stopped with a `NullReferenceException`. + ## RELATED LINKS [Get-NTFSAccess](Get-NTFSAccess.md) diff --git a/Docs/Cmdlets/Remove-NTFSAudit.md b/Docs/Cmdlets/Remove-NTFSAudit.md index 846edeb..c116fa8 100644 --- a/Docs/Cmdlets/Remove-NTFSAudit.md +++ b/Docs/Cmdlets/Remove-NTFSAudit.md @@ -308,6 +308,8 @@ The cmdlet reports no error when no entry matches the supplied values. Compare t Before 5.0.0, the cmdlet had no `-RemoveSpecific` switch. +A path that does not exist produces the non-terminating error `ReadFileError`, and the cmdlet continues with the next path. Before 5.0.0, the cmdlet also wrote a misleading `RemoveAceError` for that path, and with `-PassThru` it stopped with a `NullReferenceException`. + ## RELATED LINKS [Get-NTFSAudit](Get-NTFSAudit.md) diff --git a/NTFSSecurity/AccessCmdlets/RemoveAccess.cs b/NTFSSecurity/AccessCmdlets/RemoveAccess.cs index 4ed939a..5115150 100644 --- a/NTFSSecurity/AccessCmdlets/RemoveAccess.cs +++ b/NTFSSecurity/AccessCmdlets/RemoveAccess.cs @@ -138,6 +138,7 @@ namespace NTFSSecurity catch (Exception ex) { WriteError(new ErrorRecord(ex, "ReadFileError", ErrorCategory.OpenError, path)); + continue; } if (ParameterSetName == "PathSimple") diff --git a/NTFSSecurity/AuditCmdlets/RemoveAudit.cs b/NTFSSecurity/AuditCmdlets/RemoveAudit.cs index d4a738d..9a4e3dc 100644 --- a/NTFSSecurity/AuditCmdlets/RemoveAudit.cs +++ b/NTFSSecurity/AuditCmdlets/RemoveAudit.cs @@ -137,6 +137,7 @@ namespace NTFSSecurity catch (Exception ex) { WriteError(new ErrorRecord(ex, "ReadFileError", ErrorCategory.OpenError, path)); + continue; } if (ParameterSetName == "PathSimple") diff --git a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml index 957a61d..1958dd4 100644 --- a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml +++ b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml @@ -8385,6 +8385,7 @@ PS C:\Data> Get-NTFSSecurityDescriptor If the ACL of an item cannot be written because access is denied, the cmdlet tries once more after making the current account the owner of the item, and restores the previous owner afterwards. Changing the owner of an item requires the Take Ownership and Restore privileges, so this fallback only succeeds in an elevated session of an account that holds them. Removing rights from an entry that does not exist is not an error; the cmdlet leaves the ACL unchanged. Before 5.0.0, the `-RemoveSpecific` switch was missing, although version 4.1 had introduced it. + A path that does not exist produces the non-terminating error `ReadFileError`, and the cmdlet continues with the next path. Before 5.0.0, the cmdlet also wrote a misleading `RemoveAceError` for that path, and with `-PassThru` it stopped with a `NullReferenceException`. @@ -9224,6 +9225,7 @@ PS C:\Data> Get-NTFSSecurityDescriptor If the security descriptor cannot be read or written because access is denied, the cmdlet takes ownership of the item, repeats the operation, and restores the previous owner. If the second attempt fails as well, the cmdlet writes an error, and the ownership change is not rolled back. The cmdlet reports no error when no entry matches the supplied values. Compare the result with `Get-NTFSAudit` to confirm that the entry is gone. Before 5.0.0, the cmdlet had no `-RemoveSpecific` switch. + A path that does not exist produces the non-terminating error `ReadFileError`, and the cmdlet continues with the next path. Before 5.0.0, the cmdlet also wrote a misleading `RemoveAceError` for that path, and with `-PassThru` it stopped with a `NullReferenceException`. diff --git a/Tests/Access.Tests.ps1 b/Tests/Access.Tests.ps1 index 6ebe074..11b11c8 100644 --- a/Tests/Access.Tests.ps1 +++ b/Tests/Access.Tests.ps1 @@ -149,6 +149,25 @@ Describe 'Get-NTFSSimpleAccess' { } Describe 'Remove-NTFSAccess' { + Context 'When a path does not exist' { + BeforeAll { + $missing = Join-Path -Path $sandbox -ChildPath 'Missing.txt' + } + + # Before 5.0.0, the cmdlet went on with the missing item and wrote a second, misleading RemoveAceError. + It 'Should write only the read error' { + Remove-NTFSAccess -Path $missing -Account 'Everyone' -AccessRights ReadData -ErrorVariable removeErrors -ErrorAction SilentlyContinue + + $removeErrors | Should -HaveCount 1 + $removeErrors[0].FullyQualifiedErrorId | Should -BeLike 'ReadFileError,*' + } + + It 'Should not stop with -PassThru' { + { Remove-NTFSAccess -Path $missing -Account 'Everyone' -AccessRights ReadData -PassThru -ErrorAction SilentlyContinue } | + Should -Not -Throw + } + } + Context 'With -RemoveSpecific' { BeforeEach { $removeFolder = New-TestSandboxItem -Sandbox $sandbox -Name 'RemoveSpecific' -Directory diff --git a/Tests/Audit.Tests.ps1 b/Tests/Audit.Tests.ps1 index 8797303..7345e13 100644 --- a/Tests/Audit.Tests.ps1 +++ b/Tests/Audit.Tests.ps1 @@ -148,6 +148,25 @@ Describe 'Get-NTFSOrphanedAudit' { } Describe 'Remove-NTFSAudit' { + Context 'When a path does not exist' { + BeforeAll { + $missing = Join-Path -Path $sandbox -ChildPath 'Missing.txt' + } + + # Before 5.0.0, the cmdlet went on with the missing item and wrote a second, misleading RemoveAceError. + It 'Should write only the read error' { + Remove-NTFSAudit -Path $missing -Account 'Everyone' -AccessRights ReadData -ErrorVariable removeErrors -ErrorAction SilentlyContinue + + $removeErrors | Should -HaveCount 1 + $removeErrors[0].FullyQualifiedErrorId | Should -BeLike 'ReadFileError,*' + } + + It 'Should not stop with -PassThru' { + { Remove-NTFSAudit -Path $missing -Account 'Everyone' -AccessRights ReadData -PassThru -ErrorAction SilentlyContinue } | + Should -Not -Throw + } + } + Context 'With -RemoveSpecific' { BeforeEach { $removeFolder = New-TestSandboxItem -Sandbox $sandbox -Name 'RemoveSpecific' -Directory From 46d71bb2e31fd41fbd690082f6c5a01d6b739d2e Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Mon, 5 Oct 2026 01:32:09 +0200 Subject: [PATCH 3/7] fix: write -PassThru of the inheritance cmdlets only after a change Defect 20 (#74). Enable-NTFSAccessInheritance, Disable-NTFSAccessInheritance, Enable-NTFSAuditInheritance, Disable-NTFSAuditInheritance, and Set-NTFSInheritance wrote the -PassThru object in a finally block. After a failed change, such as an audit change without the Security privilege, they returned the unchanged state, which made the inheritance look disabled; when the item could not be read at all, reading the state in the finally block threw and stopped the command. The object is now written only after a successful change. Tests/Inheritance.Tests.ps1: 5 tests. The audit tests need the missing privilege and skip in CI; the read-deny tests skip where the Backup privilege may bypass the deny entry (CI). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- CHANGELOG.md | 6 +++ Docs/Cmdlets/Disable-NTFSAccessInheritance.md | 2 + Docs/Cmdlets/Disable-NTFSAuditInheritance.md | 2 + Docs/Cmdlets/Enable-NTFSAccessInheritance.md | 2 + Docs/Cmdlets/Enable-NTFSAuditInheritance.md | 2 + Docs/Cmdlets/Set-NTFSInheritance.md | 2 + .../DisableAccessInheritance.cs | 9 ++--- .../DisableAuditInheritance.cs | 9 ++--- .../EnableAccessInheritance.cs | 9 ++--- .../EnableAuditInheritance.cs | 9 ++--- .../InheritanceCmdlets/SetInheritance.cs | 9 ++--- NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml | 5 +++ Tests/Inheritance.Tests.ps1 | 40 +++++++++++++++++++ 13 files changed, 81 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b4146ee..75c19be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -118,5 +118,11 @@ The format is based on - Fix `Remove-NTFSAccess` and `Remove-NTFSAudit`, which went on with a path that didn't exist, wrote a second, misleading `RemoveAceError`, and with `-PassThru` stopped with a `NullReferenceException` +- Fix `-PassThru` of `Enable-NTFSAccessInheritance`, + `Disable-NTFSAccessInheritance`, `Enable-NTFSAuditInheritance`, + `Disable-NTFSAuditInheritance`, and `Set-NTFSInheritance`, which returned + the unchanged state of an item also when the change failed, so that the + inheritance looked disabled + ([#74](https://github.com/raandree/NTFSSecurity/issues/74)) [Unreleased]: https://github.com/raandree/NTFSSecurity/compare/4.2.6...HEAD diff --git a/Docs/Cmdlets/Disable-NTFSAccessInheritance.md b/Docs/Cmdlets/Disable-NTFSAccessInheritance.md index 4166f56..fc19225 100644 --- a/Docs/Cmdlets/Disable-NTFSAccessInheritance.md +++ b/Docs/Cmdlets/Disable-NTFSAccessInheritance.md @@ -170,6 +170,8 @@ A path that does not exist produces a non-terminating error and the cmdlet conti Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was `$false`, and left them enabled. +Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. + ## RELATED LINKS [Enable-NTFSAccessInheritance](Enable-NTFSAccessInheritance.md) diff --git a/Docs/Cmdlets/Disable-NTFSAuditInheritance.md b/Docs/Cmdlets/Disable-NTFSAuditInheritance.md index 3e08779..18ad002 100644 --- a/Docs/Cmdlets/Disable-NTFSAuditInheritance.md +++ b/Docs/Cmdlets/Disable-NTFSAuditInheritance.md @@ -172,6 +172,8 @@ A path that does not exist produces a non-terminating error and the cmdlet conti Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was `$false`, and left them enabled. +Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. + ## RELATED LINKS [Enable-NTFSAuditInheritance](Enable-NTFSAuditInheritance.md) diff --git a/Docs/Cmdlets/Enable-NTFSAccessInheritance.md b/Docs/Cmdlets/Enable-NTFSAccessInheritance.md index dc029fc..0a8b6da 100644 --- a/Docs/Cmdlets/Enable-NTFSAccessInheritance.md +++ b/Docs/Cmdlets/Enable-NTFSAccessInheritance.md @@ -169,6 +169,8 @@ A path that does not exist produces a non-terminating error and the cmdlet conti Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was `$false`, and left them enabled. +Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. + ## RELATED LINKS [Disable-NTFSAccessInheritance](Disable-NTFSAccessInheritance.md) diff --git a/Docs/Cmdlets/Enable-NTFSAuditInheritance.md b/Docs/Cmdlets/Enable-NTFSAuditInheritance.md index f7c7704..4765a1b 100644 --- a/Docs/Cmdlets/Enable-NTFSAuditInheritance.md +++ b/Docs/Cmdlets/Enable-NTFSAuditInheritance.md @@ -171,6 +171,8 @@ A path that does not exist produces a non-terminating error and the cmdlet conti Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was `$false`, and left them enabled. +Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. + ## RELATED LINKS [Disable-NTFSAuditInheritance](Disable-NTFSAuditInheritance.md) diff --git a/Docs/Cmdlets/Set-NTFSInheritance.md b/Docs/Cmdlets/Set-NTFSInheritance.md index f9986a8..34c141f 100644 --- a/Docs/Cmdlets/Set-NTFSInheritance.md +++ b/Docs/Cmdlets/Set-NTFSInheritance.md @@ -196,6 +196,8 @@ Before 5.0.0, omitting `-AccessInheritanceEnabled` or `-AuditInheritanceEnabled` Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was `$false`, and left them enabled. +Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. + ## RELATED LINKS [Get-NTFSInheritance](Get-NTFSInheritance.md) diff --git a/NTFSSecurity/InheritanceCmdlets/DisableAccessInheritance.cs b/NTFSSecurity/InheritanceCmdlets/DisableAccessInheritance.cs index d196fb7..c5f301c 100644 --- a/NTFSSecurity/InheritanceCmdlets/DisableAccessInheritance.cs +++ b/NTFSSecurity/InheritanceCmdlets/DisableAccessInheritance.cs @@ -101,12 +101,11 @@ namespace NTFSSecurity WriteError(new ErrorRecord(ex, "ModifySdError", ErrorCategory.WriteError, path)); continue; } - finally + + // Only after a successful change, so that a failure doesn't report the unchanged state + if (passThru) { - if (passThru) - { - WriteObject(FileSystemInheritanceInfo.GetFileSystemInheritanceInfo(item)); - } + WriteObject(FileSystemInheritanceInfo.GetFileSystemInheritanceInfo(item)); } } } diff --git a/NTFSSecurity/InheritanceCmdlets/DisableAuditInheritance.cs b/NTFSSecurity/InheritanceCmdlets/DisableAuditInheritance.cs index ac603cf..f8dbcef 100644 --- a/NTFSSecurity/InheritanceCmdlets/DisableAuditInheritance.cs +++ b/NTFSSecurity/InheritanceCmdlets/DisableAuditInheritance.cs @@ -102,12 +102,11 @@ namespace NTFSSecurity WriteError(new ErrorRecord(ex, "ModifySdError", ErrorCategory.WriteError, path)); continue; } - finally + + // Only after a successful change, so that a failure doesn't report the unchanged state + if (passThru) { - if (passThru) - { - WriteObject(FileSystemInheritanceInfo.GetFileSystemInheritanceInfo(item)); - } + WriteObject(FileSystemInheritanceInfo.GetFileSystemInheritanceInfo(item)); } } } diff --git a/NTFSSecurity/InheritanceCmdlets/EnableAccessInheritance.cs b/NTFSSecurity/InheritanceCmdlets/EnableAccessInheritance.cs index 672b98f..de3b55a 100644 --- a/NTFSSecurity/InheritanceCmdlets/EnableAccessInheritance.cs +++ b/NTFSSecurity/InheritanceCmdlets/EnableAccessInheritance.cs @@ -101,12 +101,11 @@ namespace NTFSSecurity WriteError(new ErrorRecord(ex, "ModifySdError", ErrorCategory.WriteError, path)); continue; } - finally + + // Only after a successful change, so that a failure doesn't report the unchanged state + if (passThru) { - if (passThru) - { - WriteObject(FileSystemInheritanceInfo.GetFileSystemInheritanceInfo(item)); - } + WriteObject(FileSystemInheritanceInfo.GetFileSystemInheritanceInfo(item)); } } } diff --git a/NTFSSecurity/InheritanceCmdlets/EnableAuditInheritance.cs b/NTFSSecurity/InheritanceCmdlets/EnableAuditInheritance.cs index 3f29546..943234b 100644 --- a/NTFSSecurity/InheritanceCmdlets/EnableAuditInheritance.cs +++ b/NTFSSecurity/InheritanceCmdlets/EnableAuditInheritance.cs @@ -101,12 +101,11 @@ namespace NTFSSecurity WriteError(new ErrorRecord(ex, "ModifySdError", ErrorCategory.WriteError, path)); continue; } - finally + + // Only after a successful change, so that a failure doesn't report the unchanged state + if (passThru) { - if (passThru) - { - WriteObject(FileSystemInheritanceInfo.GetFileSystemInheritanceInfo(item)); - } + WriteObject(FileSystemInheritanceInfo.GetFileSystemInheritanceInfo(item)); } } } diff --git a/NTFSSecurity/InheritanceCmdlets/SetInheritance.cs b/NTFSSecurity/InheritanceCmdlets/SetInheritance.cs index 3877e43..1a07fa7 100644 --- a/NTFSSecurity/InheritanceCmdlets/SetInheritance.cs +++ b/NTFSSecurity/InheritanceCmdlets/SetInheritance.cs @@ -109,12 +109,11 @@ namespace NTFSSecurity WriteError(new ErrorRecord(ex, "ModifySdError", ErrorCategory.WriteError, path)); continue; } - finally + + // Only after a successful change, so that a failure doesn't report the unchanged state + if (passThru) { - if (passThru) - { - WriteObject(FileSystemInheritanceInfo.GetFileSystemInheritanceInfo(item)); - } + WriteObject(FileSystemInheritanceInfo.GetFileSystemInheritanceInfo(item)); } } } diff --git a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml index 1958dd4..a5b974e 100644 --- a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml +++ b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml @@ -2413,6 +2413,7 @@ PS C:\> Set-NTFSSecurityDescriptor -SecurityDescriptor $sd Blocking access inheritance requires permission to change the DACL of the item, which the owner of an item always has. If the descriptor cannot be opened, the cmdlet takes ownership of the item, applies the change, and sets the previous owner back. That fallback only succeeds when the account can take ownership of the item and restore the original owner; otherwise the cmdlet writes an error and continues with the next item. A path that does not exist produces a non-terminating error and the cmdlet continues with the remaining paths. Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was `$false`, and left them enabled. + Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. @@ -2658,6 +2659,7 @@ PS C:\> Set-NTFSSecurityDescriptor -SecurityDescriptor $sd If the descriptor cannot be opened because the account has no permission to the item, the cmdlet takes ownership of the item, applies the change, and sets the previous owner back. That fallback only succeeds when the account can take ownership of the item and restore the original owner; a missing Security privilege is not an access problem and is not repaired by it. A path that does not exist produces a non-terminating error and the cmdlet continues with the remaining paths. Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was `$false`, and left them enabled. + Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. @@ -3028,6 +3030,7 @@ PS C:\> Get-Privileges | Where-Object { $_.Privilege -in 'Backup', 'Restore', Restoring access inheritance requires permission to change the DACL of the item, which the owner of an item always has. If the descriptor cannot be opened, the cmdlet takes ownership of the item, applies the change, and sets the previous owner back. That fallback only succeeds when the account can take ownership of the item and restore the original owner; otherwise the cmdlet writes an error and continues with the next item. A path that does not exist produces a non-terminating error and the cmdlet continues with the remaining paths. Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was `$false`, and left them enabled. + Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. @@ -3273,6 +3276,7 @@ PS C:\> Set-NTFSSecurityDescriptor -SecurityDescriptor $sd If the descriptor cannot be opened because the account has no permission to the item, the cmdlet takes ownership of the item, applies the change, and sets the previous owner back. That fallback only succeeds when the account can take ownership of the item and restore the original owner; a missing Security privilege is not an access problem and is not repaired by it. A path that does not exist produces a non-terminating error and the cmdlet continues with the remaining paths. Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was `$false`, and left them enabled. + Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. @@ -9518,6 +9522,7 @@ PS C:\> Set-NTFSSecurityDescriptor -SecurityDescriptor $sd A path that does not exist produces a non-terminating error and the cmdlet continues with the remaining paths. Before 5.0.0, omitting `-AccessInheritanceEnabled` or `-AuditInheritanceEnabled` could fail with the error "Nullable object must have a value". Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was `$false`, and left them enabled. + Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. diff --git a/Tests/Inheritance.Tests.ps1 b/Tests/Inheritance.Tests.ps1 index ddcde8e..e809462 100644 --- a/Tests/Inheritance.Tests.ps1 +++ b/Tests/Inheritance.Tests.ps1 @@ -53,6 +53,46 @@ Describe 'Get-NTFSInheritance' { } } +Describe 'Inheritance cmdlets with -PassThru' { + BeforeDiscovery { + # With the Backup privilege, Windows may grant reading the security descriptor despite a deny entry. + $canBypassDeny = Test-PrivilegeHeld -Name 'SeBackupPrivilege' + } + + # Before 5.0.0, the cmdlets wrote the -PassThru object in a finally block, also after a failure (#74). + It '<_> should return nothing when the audit change fails' -Skip:$canChangeAudit -ForEach @( + 'Enable-NTFSAuditInheritance', 'Disable-NTFSAuditInheritance' + ) { + $file = New-TestSandboxItem -Sandbox $sandbox -Name 'PassThru' + + $result = @(& $_ -Path $file -PassThru -ErrorVariable inheritanceErrors -ErrorAction SilentlyContinue) + + $inheritanceErrors | Should -Not -BeNullOrEmpty + $result | Should -BeNullOrEmpty + } + + It 'Set-NTFSInheritance should return nothing when the audit change fails' -Skip:$canChangeAudit { + $file = New-TestSandboxItem -Sandbox $sandbox -Name 'PassThru' + + $result = @(Set-NTFSInheritance -Path $file -AuditInheritanceEnabled $false -PassThru -ErrorVariable inheritanceErrors -ErrorAction SilentlyContinue) + + $inheritanceErrors | Should -Not -BeNullOrEmpty + $result | Should -BeNullOrEmpty + } + + It '<_> should write an error and return nothing when the item cannot be read' -Skip:$canBypassDeny -ForEach @( + 'Enable-NTFSAccessInheritance', 'Disable-NTFSAccessInheritance' + ) { + $file = New-TestSandboxItem -Sandbox $sandbox -Name 'Denied' + Block-TestReadPermission -Sandbox $sandbox -Path $file + + $result = @(& $_ -Path $file -PassThru -ErrorVariable inheritanceErrors -ErrorAction SilentlyContinue) + + $inheritanceErrors | Should -Not -BeNullOrEmpty + $result | Should -BeNullOrEmpty + } +} + Describe 'Set-NTFSInheritance' { Context 'When -AccessInheritanceEnabled or -AuditInheritanceEnabled is omitted' { BeforeEach { From 776344518bb8372c95a6d3794f7e28cbfd0318dd Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Mon, 5 Oct 2026 01:34:36 +0200 Subject: [PATCH 4/7] fix: say that a missing New-NTFSHardLink target does not exist Defect 21. For a -Target that didn't exist, New-NTFSHardLink failed with "The target path exist, cannot create the link", the opposite of the cause. The message now names the target and says that it does not exist. Tests/Links.Tests.ps1 (new): 2 tests, one of them for a link that is created. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- CHANGELOG.md | 2 + Docs/Cmdlets/New-NTFSHardLink.md | 2 + NTFSSecurity/LinkCmdlets/NewHardLink.cs | 2 +- NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml | 1 + Tests/Links.Tests.ps1 | 44 ++++++++++++++++++++ 5 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 Tests/Links.Tests.ps1 diff --git a/CHANGELOG.md b/CHANGELOG.md index 75c19be..bac6de6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -124,5 +124,7 @@ The format is based on the unchanged state of an item also when the change failed, so that the inheritance looked disabled ([#74](https://github.com/raandree/NTFSSecurity/issues/74)) +- Fix the error of `New-NTFSHardLink` for a missing `-Target`, which said + that the target path existed [Unreleased]: https://github.com/raandree/NTFSSecurity/compare/4.2.6...HEAD diff --git a/Docs/Cmdlets/New-NTFSHardLink.md b/Docs/Cmdlets/New-NTFSHardLink.md index 16c4fdd..184f5a7 100644 --- a/Docs/Cmdlets/New-NTFSHardLink.md +++ b/Docs/Cmdlets/New-NTFSHardLink.md @@ -138,6 +138,8 @@ The cmdlet does not overwrite anything. If `-Path` already exists, or if `-Targe Because all names of a file share the same data, the number of hard links is a property of the file, not of an individual name. Use `Get-NTFSHardLink` to list them, and delete a link with `Remove-Item2` or `Remove-Item`, which removes only that name as long as other names remain. +Before 5.0.0, the error for a missing `-Target` said "The target path exist", the opposite of the cause. + ## RELATED LINKS [Get-NTFSHardLink](Get-NTFSHardLink.md) diff --git a/NTFSSecurity/LinkCmdlets/NewHardLink.cs b/NTFSSecurity/LinkCmdlets/NewHardLink.cs index 7f9f47d..cec9d5b 100644 --- a/NTFSSecurity/LinkCmdlets/NewHardLink.cs +++ b/NTFSSecurity/LinkCmdlets/NewHardLink.cs @@ -63,7 +63,7 @@ namespace NTFSSecurity throw new ArgumentException(string.Format("The file '{0}' does already exist, cannot create the link", path)); if (!TryGetFileSystemInfo2(target, out temp)) - throw new ArgumentException("The target path exist, cannot create the link"); + throw new ArgumentException(string.Format("The target '{0}' does not exist, cannot create the link", target)); else if (temp is DirectoryInfo) throw new ArgumentException("The target is not a file, cannot create the link"); diff --git a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml index a5b974e..aa824f6 100644 --- a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml +++ b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml @@ -7135,6 +7135,7 @@ PS C:\Data> Get-NTFSSecurityDescriptor Windows supports hard links only for files on the same NTFS volume. A link that points to a file on another volume, or a target on a file system that does not implement hard links, cannot be created. The cmdlet does not overwrite anything. If `-Path` already exists, or if `-Target` is missing or is a folder, the cmdlet reports an error and leaves the file system unchanged. Because all names of a file share the same data, the number of hard links is a property of the file, not of an individual name. Use `Get-NTFSHardLink` to list them, and delete a link with `Remove-Item2` or `Remove-Item`, which removes only that name as long as other names remain. + Before 5.0.0, the error for a missing `-Target` said "The target path exist", the opposite of the cause. diff --git a/Tests/Links.Tests.ps1 b/Tests/Links.Tests.ps1 new file mode 100644 index 0000000..f75509a --- /dev/null +++ b/Tests/Links.Tests.ps1 @@ -0,0 +1,44 @@ +<# + Tests the link cmdlets of the module built in NTFSSecurity\bin\Release in a sandbox folder. +#> +[Diagnostics.CodeAnalysis.SuppressMessageAttribute( + 'PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Pester shares variables between blocks.' +)] +param () + +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 'Links' + Push-Location -LiteralPath $sandbox +} + +AfterAll { + Pop-Location + Remove-TestSandbox -Sandbox $sandbox + Remove-Module -Name NTFSSecurity -Force -ErrorAction SilentlyContinue +} + +Describe 'New-NTFSHardLink' { + It 'Should create a hard link to a file' { + $target = New-TestSandboxItem -Sandbox $sandbox -Name 'Target' + $link = Join-Path -Path $sandbox -ChildPath 'Link.txt' + Assert-TestSandboxPath -Sandbox $sandbox -Path $link + + New-NTFSHardLink -Path $link -Target $target -ErrorAction Stop + + $link | Should -Exist + Get-Content -LiteralPath $link | Should -Be (Get-Content -LiteralPath $target) + } + + # Before 5.0.0, the error said "The target path exist" for a target that did not exist. + It 'Should report that a missing target does not exist' { + $missing = Join-Path -Path $sandbox -ChildPath 'Missing.txt' + $link = Join-Path -Path $sandbox -ChildPath 'MissingLink.txt' + Assert-TestSandboxPath -Sandbox $sandbox -Path $missing, $link + + { New-NTFSHardLink -Path $link -Target $missing -ErrorAction Stop } | Should -Throw -ExpectedMessage '*does not exist*' + $link | Should -Not -Exist + } +} From ad899940ea183e475082551249062f7bbe087481 Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Mon, 5 Oct 2026 01:37:05 +0200 Subject: [PATCH 5/7] fix: write no Get-FileHash2 result for a file that could not be read Found while fixing defect 3, in the same loop: the hash variable lived outside the loop, and after a GetHashError the cmdlet still wrote a result for the file, with the hash of the previous file. Each path now starts without a hash, and a failed read writes only the error. Tests/FileHash.Tests.ps1: 1 test with a file opened without sharing; like the other Get-FileHash2 tests, it skips in PowerShell 7 until the RIPEMD160 reference goes (decision D5). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- CHANGELOG.md | 2 ++ Docs/Cmdlets/Get-FileHash2.md | 4 ++-- NTFSSecurity/MiscCmdlets/GetFileHash2.cs | 8 +++++--- NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml | 4 ++-- Tests/FileHash.Tests.ps1 | 20 ++++++++++++++++++++ 5 files changed, 31 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bac6de6..1023796 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -126,5 +126,7 @@ The format is based on ([#74](https://github.com/raandree/NTFSSecurity/issues/74)) - Fix the error of `New-NTFSHardLink` for a missing `-Target`, which said that the target path existed +- Fix `Get-FileHash2`, which wrote a result for a file that it couldn't + read, with the hash of the previous file [Unreleased]: https://github.com/raandree/NTFSSecurity/compare/4.2.6...HEAD diff --git a/Docs/Cmdlets/Get-FileHash2.md b/Docs/Cmdlets/Get-FileHash2.md index 3867548..6dd281e 100644 --- a/Docs/Cmdlets/Get-FileHash2.md +++ b/Docs/Cmdlets/Get-FileHash2.md @@ -119,13 +119,13 @@ The cmdlet does not return access rules. For every hashed file it writes the fil The cmdlet works only in Windows PowerShell. In PowerShell 7, it fails for every algorithm with the error `Could not load type 'System.Security.Cryptography.RIPEMD160'`, because .NET no longer includes the RIPEMD-160 implementation that the cmdlet references. In PowerShell 7, use the built-in `Get-FileHash` cmdlet instead. -If the file cannot be opened because access is denied, the cmdlet takes ownership of the file with the account that runs it, calculates the hash, and restores the previous owner afterward. That fallback fails with a `GetHashError` when the account is not allowed to change the owner of the file. +If the file cannot be opened because access is denied, the cmdlet takes ownership of the file with the account that runs it, calculates the hash, and restores the previous owner afterward. That fallback fails with a `GetHashError` when the account is not allowed to change the owner of the file. A file that cannot be read produces a `GetHashError` and no result. The hash is returned as an uppercase hexadecimal string without separators, which differs from the lowercase output of some other hashing tools. Compare hash values case-insensitively. `MACTripleDES` is a keyed message authentication code that is created with a key that is generated for each call, so its result is not reproducible across invocations and is not suitable for comparing files. -Before 5.0.0, a folder in a `-Path` array stopped the processing of that array, so the files that followed the folder were not hashed. +Before 5.0.0, a folder in a `-Path` array stopped the processing of that array, so the files that followed the folder were not hashed, and a file that could not be read got a result with the hash of the previous file. ## RELATED LINKS diff --git a/NTFSSecurity/MiscCmdlets/GetFileHash2.cs b/NTFSSecurity/MiscCmdlets/GetFileHash2.cs index b717d2b..52a01b8 100644 --- a/NTFSSecurity/MiscCmdlets/GetFileHash2.cs +++ b/NTFSSecurity/MiscCmdlets/GetFileHash2.cs @@ -39,11 +39,11 @@ namespace NTFSSecurity protected override void ProcessRecord() { - string hash = string.Empty; - FileSystemInfo item = null; - foreach (var path in paths) { + string hash = null; + FileSystemInfo item = null; + try { item = GetFileSystemInfo2(path) as FileInfo; @@ -80,11 +80,13 @@ namespace NTFSSecurity catch (Exception ex2) { WriteError(new ErrorRecord(ex2, "GetHashError", ErrorCategory.WriteError, path)); + continue; } } catch (Exception ex) { WriteError(new ErrorRecord(ex, "GetHashError", ErrorCategory.WriteError, path)); + continue; } var result = new PSObject(item); diff --git a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml index aa824f6..23a434b 100644 --- a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml +++ b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml @@ -4150,10 +4150,10 @@ PS C:\> Disable-Privileges The cmdlet works only in Windows PowerShell. In PowerShell 7, it fails for every algorithm with the error `Could not load type 'System.Security.Cryptography.RIPEMD160'`, because .NET no longer includes the RIPEMD-160 implementation that the cmdlet references. In PowerShell 7, use the built-in `Get-FileHash` cmdlet instead. - If the file cannot be opened because access is denied, the cmdlet takes ownership of the file with the account that runs it, calculates the hash, and restores the previous owner afterward. That fallback fails with a `GetHashError` when the account is not allowed to change the owner of the file. + If the file cannot be opened because access is denied, the cmdlet takes ownership of the file with the account that runs it, calculates the hash, and restores the previous owner afterward. That fallback fails with a `GetHashError` when the account is not allowed to change the owner of the file. A file that cannot be read produces a `GetHashError` and no result. The hash is returned as an uppercase hexadecimal string without separators, which differs from the lowercase output of some other hashing tools. Compare hash values case-insensitively. `MACTripleDES` is a keyed message authentication code that is created with a key that is generated for each call, so its result is not reproducible across invocations and is not suitable for comparing files. - Before 5.0.0, a folder in a `-Path` array stopped the processing of that array, so the files that followed the folder were not hashed. + Before 5.0.0, a folder in a `-Path` array stopped the processing of that array, so the files that followed the folder were not hashed, and a file that could not be read got a result with the hash of the previous file. diff --git a/Tests/FileHash.Tests.ps1 b/Tests/FileHash.Tests.ps1 index 64d9cba..2ffb4c4 100644 --- a/Tests/FileHash.Tests.ps1 +++ b/Tests/FileHash.Tests.ps1 @@ -42,4 +42,24 @@ Describe 'Get-FileHash2' { $results[1].Hash | Should -BeExactly (Get-FileHash -LiteralPath $second -Algorithm SHA256).Hash } } + + Context 'When a file cannot be read' { + # Before 5.0.0, the cmdlet wrote a result for the file anyway, with the hash of the previous file. + It 'Should write an error and no result for the file' { + if ($PSVersionTable.PSEdition -eq 'Core') { + Set-ItResult -Skipped -Because 'Get-FileHash2 fails in PowerShell 7 until it no longer references RIPEMD160' + } + $locked = New-TestSandboxItem -Sandbox $sandbox -Name 'Locked' + $stream = [IO.File]::Open($locked, [IO.FileMode]::Open, [IO.FileAccess]::Read, [IO.FileShare]::None) + try { + $results = @(Get-FileHash2 -Path $first, $locked -ErrorVariable hashErrors -ErrorAction SilentlyContinue) + } + finally { + $stream.Dispose() + } + + $hashErrors | Should -HaveCount 1 + $results.Name | Should -Be @('One.txt') + } + } } From 7782f6146e8c078661f21ef67d4130cf737100b0 Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Mon, 5 Oct 2026 01:39:50 +0200 Subject: [PATCH 6/7] chore(memory-bank): record group C - activeContext.md: the results of ai/defects-c and the next step. - progress.md: group C is fixed on ai/defects-c (not merged yet). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- .memory-bank/activeContext.md | 6 ++++-- .memory-bank/progress.md | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.memory-bank/activeContext.md b/.memory-bank/activeContext.md index dfb9647..915ec7e 100644 --- a/.memory-bank/activeContext.md +++ b/.memory-bank/activeContext.md @@ -48,8 +48,10 @@ the PRs, and tags `5.0.0-rc2` after the merges. - `ai/defects-b` fixes defects 14 to 17 (`-AppliesTo` is mandatory in the `Simple` sets; `-RemoveSpecific` is back): Windows PowerShell 331 passed, 19 skipped; PowerShell 7 301 passed, 49 skipped (350 tests). +- `ai/defects-c` fixes defects 18 to 21, the same missing `continue` in + `Remove-NTFSAudit`, and a stale hash in `Get-FileHash2`. ## Next step -Group C (18 to 21) on `ai/defects-c`, then D, the E decisions, and the -bugs from the issue triage (`ai/issue-fixes`). +Group D (22 to 24) on `ai/defects-d`, then the E decisions and the bugs +from the issue triage (`ai/issue-fixes`). diff --git a/.memory-bank/progress.md b/.memory-bank/progress.md index 9621387..37f5297 100644 --- a/.memory-bank/progress.md +++ b/.memory-bank/progress.md @@ -141,7 +141,7 @@ Numbered as agreed with the maintainer; each is documented on its page. - Review of group B: no Blocker or Major; three Minor findings fixed, the rest are listed in the PR description. -#### C: Error handling +#### C: Error handling (fixed on `ai/defects-c`, not merged) - (18) `Copy-Item2`, `Move-Item2`, `Remove-Item2`: one failing path skips the rest of `-Path` (`return` instead of `continue`). From 26b9268122c8277b0db64bee15f60fba75a53b51 Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Mon, 5 Oct 2026 02:05:44 +0200 Subject: [PATCH 7/7] fix: restore the owner and write no -PassThru after a failed change The cmdlets that take ownership of an item to repeat a denied operation left the account that ran them as the owner when the second attempt failed as well. BaseCmdlet.InvokeAsOwner now restores the previous owner on every exit path and reports a failed restore as RestoreOwnerError. Add-NTFSAccess, Add-NTFSAudit, Remove-NTFSAccess, and Remove-NTFSAudit wrote the unchanged entries of an item with -PassThru after a failed change; they now continue with the next path. The inheritance tests assert the error identity and cover a missing path on every runner. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- .memory-bank/progress.md | 3 + CHANGELOG.md | 6 ++ Docs/Cmdlets/Add-NTFSAccess.md | 2 +- Docs/Cmdlets/Add-NTFSAudit.md | 4 +- Docs/Cmdlets/Clear-NTFSAudit.md | 2 +- Docs/Cmdlets/Remove-NTFSAccess.md | 2 +- Docs/Cmdlets/Remove-NTFSAudit.md | 4 +- Docs/Concepts.md | 3 +- NTFSSecurity/AccessCmdlets/AddAccess.cs | 14 ++-- NTFSSecurity/AccessCmdlets/ClearAccess.cs | 16 ++--- NTFSSecurity/AccessCmdlets/GetAccess.cs | 10 ++- .../AccessCmdlets/GetEffectiveAccess.cs | 12 ++-- .../AccessCmdlets/GetOrphanedAccess.cs | 12 ++-- NTFSSecurity/AccessCmdlets/RemoveAccess.cs | 14 ++-- NTFSSecurity/AuditCmdlets/AddAudit.cs | 14 ++-- NTFSSecurity/AuditCmdlets/ClearAudit.cs | 16 ++--- NTFSSecurity/AuditCmdlets/RemoveAudit.cs | 14 ++-- NTFSSecurity/BaseCmdlets.cs | 32 +++++++++ .../DisableAccessInheritance.cs | 12 ++-- .../DisableAuditInheritance.cs | 12 ++-- .../EnableAccessInheritance.cs | 12 ++-- .../EnableAuditInheritance.cs | 12 ++-- .../InheritanceCmdlets/GetInheritance.cs | 12 ++-- .../InheritanceCmdlets/SetInheritance.cs | 12 ++-- NTFSSecurity/MiscCmdlets/GetFileHash2.cs | 12 ++-- .../GetSecurityDescriptor.cs | 12 ++-- NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml | 14 ++-- Tests/Access.Tests.ps1 | 39 +++++++++++ Tests/FileHash.Tests.ps1 | 29 ++++++++ Tests/Inheritance.Tests.ps1 | 18 ++++- Tests/TestHelpers.psm1 | 70 +++++++++++++++++-- 31 files changed, 284 insertions(+), 162 deletions(-) diff --git a/.memory-bank/progress.md b/.memory-bank/progress.md index 37f5297..bba0bb4 100644 --- a/.memory-bank/progress.md +++ b/.memory-bank/progress.md @@ -150,6 +150,9 @@ Numbered as agreed with the maintainer; each is documented on its page. - (20) The inheritance cmdlets write `-PassThru` output in `finally`, even after a failure. - (21) `New-NTFSHardLink` says the target path exists when it doesn't. +- Review of group C: the add and remove cmdlets also wrote `-PassThru` + after a failed change, and a failed retry after taking ownership left the + owner changed; `BaseCmdlet.InvokeAsOwner` now restores it on every path. #### D: Metadata and cosmetics diff --git a/CHANGELOG.md b/CHANGELOG.md index 1023796..2c73265 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -128,5 +128,11 @@ The format is based on that the target path existed - Fix `Get-FileHash2`, which wrote a result for a file that it couldn't read, with the hash of the previous file +- Fix `-PassThru` of `Add-NTFSAccess`, `Add-NTFSAudit`, `Remove-NTFSAccess`, + and `Remove-NTFSAudit`, which returned the unchanged entries of an item + also when the change failed +- Fix the cmdlets that take ownership of an item to repeat an operation that + was denied: when the second attempt failed as well, the account that ran + the cmdlet stayed the owner of the item; now the previous owner is restored [Unreleased]: https://github.com/raandree/NTFSSecurity/compare/4.2.6...HEAD diff --git a/Docs/Cmdlets/Add-NTFSAccess.md b/Docs/Cmdlets/Add-NTFSAccess.md index 078f24e..b39a443 100644 --- a/Docs/Cmdlets/Add-NTFSAccess.md +++ b/Docs/Cmdlets/Add-NTFSAccess.md @@ -282,7 +282,7 @@ The scope of the ACE, piped by the property `AppliesTo` in the `Simple` paramete ### Security2.FileSystemAccessRule2 -With `-PassThru`, the cmdlet writes all access control entries of every processed item, explicit and inherited. Without `-PassThru` it writes nothing. +With `-PassThru`, the cmdlet writes all access control entries, explicit and inherited, of every item it changed; an item it could not change produces only an error. Before 5.0.0, `-PassThru` also wrote the unchanged entries of such an item. Without `-PassThru` it writes nothing. ## NOTES diff --git a/Docs/Cmdlets/Add-NTFSAudit.md b/Docs/Cmdlets/Add-NTFSAudit.md index 838800e..ad280e6 100644 --- a/Docs/Cmdlets/Add-NTFSAudit.md +++ b/Docs/Cmdlets/Add-NTFSAudit.md @@ -280,7 +280,7 @@ The value passed to `-AppliesTo` is converted to this type and binds by property ### Security2.FileSystemAuditRule2 -Without `-PassThru` the cmdlet writes nothing. With `-PassThru` the cmdlet writes all audit entries of the item or the security descriptor, explicit and inherited ones, as `Security2.FileSystemAuditRule2` objects. Before 5.0.0, the `SecurityDescriptor` sets wrote the access entries of the descriptor instead. +Without `-PassThru` the cmdlet writes nothing. With `-PassThru` the cmdlet writes all audit entries of the item or the security descriptor, explicit and inherited ones, as `Security2.FileSystemAuditRule2` objects. Before 5.0.0, the `SecurityDescriptor` sets wrote the access entries of the descriptor instead. An item whose audit entries could not be changed produces only an error; before 5.0.0, `-PassThru` also wrote its unchanged entries. ## NOTES @@ -288,7 +288,7 @@ When the module setting `EnablePrivileges` is `$true` (the default in the `Priva Writing the SACL requires the Security privilege (`SeSecurityPrivilege`, "Manage auditing and security log"), so run this cmdlet in an elevated session of an account that holds that privilege. Without it, the cmdlet writes a non-terminating `AddAceError` whose message states that a required privilege is not held by the client, and the item is left unchanged. -If the security descriptor cannot be read or written because access is denied, the cmdlet takes ownership of the item, repeats the operation, and restores the previous owner. If the second attempt fails as well, the cmdlet writes an error, and the ownership change is not rolled back. +If the security descriptor cannot be read or written because access is denied, the cmdlet takes ownership of the item, repeats the operation, and restores the previous owner. If the second attempt fails as well, the cmdlet restores the previous owner and writes an error. Before 5.0.0, the account that ran the cmdlet stayed the owner of the item in that case. `-Path` or `-SecurityDescriptor`, `-Account`, and `-AccessRights` are positional parameters at positions 1, 2, and 3, like in `Remove-NTFSAudit`. Before 5.0.0, `-Account` and `-AccessRights` were both declared at position 2, so a command that passed them by position failed. diff --git a/Docs/Cmdlets/Clear-NTFSAudit.md b/Docs/Cmdlets/Clear-NTFSAudit.md index 7acbcf7..7ec4adf 100644 --- a/Docs/Cmdlets/Clear-NTFSAudit.md +++ b/Docs/Cmdlets/Clear-NTFSAudit.md @@ -142,7 +142,7 @@ When the module setting `EnablePrivileges` is `$true` (the default in the `Priva Reading and writing the SACL requires the Security privilege (`SeSecurityPrivilege`, "Manage auditing and security log"), so run this cmdlet in an elevated session of an account that holds that privilege. Without that privilege the cmdlet reads the security descriptor without its SACL, finds no audit entries to remove, and finishes without an error although nothing was changed. `-DisableInheritance` fails in that situation with a `ClearAclError` whose message states that a required privilege is not held by the client. -If the security descriptor cannot be read or written because access is denied, the cmdlet takes ownership of the item, repeats the operation, and restores the previous owner. If the second attempt fails as well, the cmdlet writes an error, and the ownership change is not rolled back. +If the security descriptor cannot be read or written because access is denied, the cmdlet takes ownership of the item, repeats the operation, and restores the previous owner. If the second attempt fails as well, the cmdlet restores the previous owner and writes an error. Before 5.0.0, the account that ran the cmdlet stayed the owner of the item in that case. ## RELATED LINKS diff --git a/Docs/Cmdlets/Remove-NTFSAccess.md b/Docs/Cmdlets/Remove-NTFSAccess.md index 6aa0d3d..e2b4c18 100644 --- a/Docs/Cmdlets/Remove-NTFSAccess.md +++ b/Docs/Cmdlets/Remove-NTFSAccess.md @@ -294,7 +294,7 @@ The scope of the entry, piped by the property `AppliesTo` in the `Simple` parame ### Security2.FileSystemAccessRule2 -With `-PassThru`, the cmdlet writes all access control entries of every processed item, explicit and inherited. Without `-PassThru` it writes nothing. +With `-PassThru`, the cmdlet writes all access control entries, explicit and inherited, of every item it changed; an item it could not change produces only an error. Before 5.0.0, `-PassThru` also wrote the unchanged entries of such an item. Without `-PassThru` it writes nothing. ## NOTES diff --git a/Docs/Cmdlets/Remove-NTFSAudit.md b/Docs/Cmdlets/Remove-NTFSAudit.md index c116fa8..c8c7835 100644 --- a/Docs/Cmdlets/Remove-NTFSAudit.md +++ b/Docs/Cmdlets/Remove-NTFSAudit.md @@ -294,7 +294,7 @@ The value passed to `-AppliesTo` is converted to this type and binds by property ### Security2.FileSystemAuditRule2 -Without `-PassThru` the cmdlet writes nothing. With `-PassThru` the cmdlet writes all audit entries of the item or the security descriptor, explicit and inherited ones, as `Security2.FileSystemAuditRule2` objects. Before 5.0.0, the `Path` sets wrote the access entries of the item instead. +Without `-PassThru` the cmdlet writes nothing. With `-PassThru` the cmdlet writes all audit entries of the item or the security descriptor, explicit and inherited ones, as `Security2.FileSystemAuditRule2` objects. Before 5.0.0, the `Path` sets wrote the access entries of the item instead. An item whose audit entries could not be changed produces only an error; before 5.0.0, `-PassThru` also wrote its unchanged entries. ## NOTES @@ -302,7 +302,7 @@ When the module setting `EnablePrivileges` is `$true` (the default in the `Priva Reading and writing the SACL requires the Security privilege (`SeSecurityPrivilege`, "Manage auditing and security log"), so run this cmdlet in an elevated session of an account that holds that privilege. Without it, the cmdlet writes a non-terminating `RemoveAceError` whose message states that a required privilege is not held by the client, and the item is left unchanged. -If the security descriptor cannot be read or written because access is denied, the cmdlet takes ownership of the item, repeats the operation, and restores the previous owner. If the second attempt fails as well, the cmdlet writes an error, and the ownership change is not rolled back. +If the security descriptor cannot be read or written because access is denied, the cmdlet takes ownership of the item, repeats the operation, and restores the previous owner. If the second attempt fails as well, the cmdlet restores the previous owner and writes an error. Before 5.0.0, the account that ran the cmdlet stayed the owner of the item in that case. The cmdlet reports no error when no entry matches the supplied values. Compare the result with `Get-NTFSAudit` to confirm that the entry is gone. diff --git a/Docs/Concepts.md b/Docs/Concepts.md index aaa2a15..18f347c 100644 --- a/Docs/Concepts.md +++ b/Docs/Concepts.md @@ -199,7 +199,8 @@ state. When reading or changing an item fails with an access-denied error, most of these cmdlets make the current user the owner of the item, retry, and then -restore the previous owner. This requires the privileges above. +restore the previous owner, also when the retry fails. Before 5.0.0, a failed +retry left the current user as the owner. This requires the privileges above. Reading or changing audit entries always requires the Security privilege. Without it, the audit cmdlets fail, and `Get-NTFSEffectiveAccess` warns that diff --git a/NTFSSecurity/AccessCmdlets/AddAccess.cs b/NTFSSecurity/AccessCmdlets/AddAccess.cs index c22e0f5..4770aa9 100644 --- a/NTFSSecurity/AccessCmdlets/AddAccess.cs +++ b/NTFSSecurity/AccessCmdlets/AddAccess.cs @@ -138,23 +138,21 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - FileSystemAccessRule2.AddFileSystemAccessRule(item, account.ToList(), accessRights, accessType, inheritanceFlags, propagationFlags); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + FileSystemAccessRule2.AddFileSystemAccessRule(item, account.ToList(), accessRights, accessType, inheritanceFlags, propagationFlags); + }); } catch (Exception ex2) { WriteError(new ErrorRecord(ex2, "AddAceError", ErrorCategory.WriteError, path)); + continue; } } catch (Exception ex) { WriteError(new ErrorRecord(ex, "AddAceError", ErrorCategory.WriteError, path)); + continue; } if (passThru == true) diff --git a/NTFSSecurity/AccessCmdlets/ClearAccess.cs b/NTFSSecurity/AccessCmdlets/ClearAccess.cs index 20b1943..9e96a39 100644 --- a/NTFSSecurity/AccessCmdlets/ClearAccess.cs +++ b/NTFSSecurity/AccessCmdlets/ClearAccess.cs @@ -76,16 +76,12 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - FileSystemAccessRule2.RemoveFileSystemAccessRuleAll(item); - if (disableInheritance) - FileSystemInheritanceInfo.DisableAccessInheritance(item, true); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + FileSystemAccessRule2.RemoveFileSystemAccessRuleAll(item); + if (disableInheritance) + FileSystemInheritanceInfo.DisableAccessInheritance(item, true); + }); } catch (Exception ex2) { diff --git a/NTFSSecurity/AccessCmdlets/GetAccess.cs b/NTFSSecurity/AccessCmdlets/GetAccess.cs index 6b9339a..f010e18 100644 --- a/NTFSSecurity/AccessCmdlets/GetAccess.cs +++ b/NTFSSecurity/AccessCmdlets/GetAccess.cs @@ -104,12 +104,10 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - acl = FileSystemAccessRule2.GetFileSystemAccessRules(item, !excludeExplicit, !excludeInherited, getInheritedFrom); - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + acl = FileSystemAccessRule2.GetFileSystemAccessRules(item, !excludeExplicit, !excludeInherited, getInheritedFrom); + }); } catch (Exception ex2) { diff --git a/NTFSSecurity/AccessCmdlets/GetEffectiveAccess.cs b/NTFSSecurity/AccessCmdlets/GetEffectiveAccess.cs index 4fb3c23..d929bbf 100644 --- a/NTFSSecurity/AccessCmdlets/GetEffectiveAccess.cs +++ b/NTFSSecurity/AccessCmdlets/GetEffectiveAccess.cs @@ -133,14 +133,10 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - result = EffectiveAccess.GetEffectiveAccess(item, account, serverName); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + result = EffectiveAccess.GetEffectiveAccess(item, account, serverName); + }); } catch (Exception ex2) { diff --git a/NTFSSecurity/AccessCmdlets/GetOrphanedAccess.cs b/NTFSSecurity/AccessCmdlets/GetOrphanedAccess.cs index fbe88ce..94ad30c 100644 --- a/NTFSSecurity/AccessCmdlets/GetOrphanedAccess.cs +++ b/NTFSSecurity/AccessCmdlets/GetOrphanedAccess.cs @@ -48,14 +48,10 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - acl = FileSystemAccessRule2.GetFileSystemAccessRules(item, !ExcludeExplicit, !ExcludeInherited, getInheritedFrom); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + acl = FileSystemAccessRule2.GetFileSystemAccessRules(item, !ExcludeExplicit, !ExcludeInherited, getInheritedFrom); + }); } catch (Exception ex2) { diff --git a/NTFSSecurity/AccessCmdlets/RemoveAccess.cs b/NTFSSecurity/AccessCmdlets/RemoveAccess.cs index 5115150..bd4c04d 100644 --- a/NTFSSecurity/AccessCmdlets/RemoveAccess.cs +++ b/NTFSSecurity/AccessCmdlets/RemoveAccess.cs @@ -154,23 +154,21 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - FileSystemAccessRule2.RemoveFileSystemAccessRule(item, account.ToList(), accessRights, accessType, inheritanceFlags, propagationFlags, removeSpecific); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + FileSystemAccessRule2.RemoveFileSystemAccessRule(item, account.ToList(), accessRights, accessType, inheritanceFlags, propagationFlags, removeSpecific); + }); } catch (Exception ex2) { WriteError(new ErrorRecord(ex2, "RemoveAceError", ErrorCategory.WriteError, path)); + continue; } } catch (Exception ex) { WriteError(new ErrorRecord(ex, "RemoveAceError", ErrorCategory.WriteError, path)); + continue; } if (passThru == true) diff --git a/NTFSSecurity/AuditCmdlets/AddAudit.cs b/NTFSSecurity/AuditCmdlets/AddAudit.cs index 7470de6..4ce3edb 100644 --- a/NTFSSecurity/AuditCmdlets/AddAudit.cs +++ b/NTFSSecurity/AuditCmdlets/AddAudit.cs @@ -137,23 +137,21 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - FileSystemAuditRule2.AddFileSystemAuditRule(item, account.ToList(), accessRights, auditFlags, inheritanceFlags, propagationFlags); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + FileSystemAuditRule2.AddFileSystemAuditRule(item, account.ToList(), accessRights, auditFlags, inheritanceFlags, propagationFlags); + }); } catch (Exception ex2) { WriteError(new ErrorRecord(ex2, "AddAceError", ErrorCategory.WriteError, path)); + continue; } } catch (Exception ex) { WriteError(new ErrorRecord(ex, "AddAceError", ErrorCategory.WriteError, path)); + continue; } if (passThru == true) diff --git a/NTFSSecurity/AuditCmdlets/ClearAudit.cs b/NTFSSecurity/AuditCmdlets/ClearAudit.cs index c34bb29..622e5d4 100644 --- a/NTFSSecurity/AuditCmdlets/ClearAudit.cs +++ b/NTFSSecurity/AuditCmdlets/ClearAudit.cs @@ -76,16 +76,12 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - FileSystemAuditRule2.RemoveFileSystemAuditRuleAll(item); - if (disableInheritance) - FileSystemInheritanceInfo.DisableAuditInheritance(item, true); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + FileSystemAuditRule2.RemoveFileSystemAuditRuleAll(item); + if (disableInheritance) + FileSystemInheritanceInfo.DisableAuditInheritance(item, true); + }); } catch (Exception ex2) { diff --git a/NTFSSecurity/AuditCmdlets/RemoveAudit.cs b/NTFSSecurity/AuditCmdlets/RemoveAudit.cs index 9a4e3dc..f075b30 100644 --- a/NTFSSecurity/AuditCmdlets/RemoveAudit.cs +++ b/NTFSSecurity/AuditCmdlets/RemoveAudit.cs @@ -153,23 +153,21 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - FileSystemAuditRule2.RemoveFileSystemAuditRule(item, account.ToList(), accessRights, auditFlags, inheritanceFlags, propagationFlags, removeSpecific); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + FileSystemAuditRule2.RemoveFileSystemAuditRule(item, account.ToList(), accessRights, auditFlags, inheritanceFlags, propagationFlags, removeSpecific); + }); } catch (Exception ex2) { WriteError(new ErrorRecord(ex2, "RemoveAceError", ErrorCategory.WriteError, path)); + continue; } } catch (Exception ex) { WriteError(new ErrorRecord(ex, "RemoveAceError", ErrorCategory.WriteError, path)); + continue; } if (passThru == true) diff --git a/NTFSSecurity/BaseCmdlets.cs b/NTFSSecurity/BaseCmdlets.cs index 0a5eb49..8cdb84d 100644 --- a/NTFSSecurity/BaseCmdlets.cs +++ b/NTFSSecurity/BaseCmdlets.cs @@ -148,6 +148,38 @@ namespace NTFSSecurity return path; } #endregion + + #region InvokeAsOwner + /// + /// Takes ownership of the item, runs the action, and restores the previous owner on every exit path. + /// A failure to restore the owner is written as a RestoreOwnerError and doesn't hide an error of the action. + /// + /// The file or folder to take ownership of. + /// The path the user specified, used as the error target. + /// The operation to run while the current user owns the item. + protected void InvokeAsOwner(Alphaleonis.Win32.Filesystem.FileSystemInfo item, string path, Action action) + { + var previousOwner = FileSystemOwner.GetOwner(item).Owner; + + FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); + + try + { + action(); + } + finally + { + try + { + FileSystemOwner.SetOwner(item, previousOwner); + } + catch (Exception ex) + { + WriteError(new ErrorRecord(ex, "RestoreOwnerError", ErrorCategory.WriteError, path)); + } + } + } + #endregion } public class BaseCmdletWithPrivControl : BaseCmdlet diff --git a/NTFSSecurity/InheritanceCmdlets/DisableAccessInheritance.cs b/NTFSSecurity/InheritanceCmdlets/DisableAccessInheritance.cs index c5f301c..f459e61 100644 --- a/NTFSSecurity/InheritanceCmdlets/DisableAccessInheritance.cs +++ b/NTFSSecurity/InheritanceCmdlets/DisableAccessInheritance.cs @@ -81,14 +81,10 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - FileSystemInheritanceInfo.DisableAccessInheritance(item, removeInheritedAccessRules); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + FileSystemInheritanceInfo.DisableAccessInheritance(item, removeInheritedAccessRules); + }); } catch (Exception ex2) { diff --git a/NTFSSecurity/InheritanceCmdlets/DisableAuditInheritance.cs b/NTFSSecurity/InheritanceCmdlets/DisableAuditInheritance.cs index f8dbcef..bf51018 100644 --- a/NTFSSecurity/InheritanceCmdlets/DisableAuditInheritance.cs +++ b/NTFSSecurity/InheritanceCmdlets/DisableAuditInheritance.cs @@ -82,14 +82,10 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - FileSystemInheritanceInfo.DisableAuditInheritance(item, removeInheritedAccessRules); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + FileSystemInheritanceInfo.DisableAuditInheritance(item, removeInheritedAccessRules); + }); } catch (Exception ex2) { diff --git a/NTFSSecurity/InheritanceCmdlets/EnableAccessInheritance.cs b/NTFSSecurity/InheritanceCmdlets/EnableAccessInheritance.cs index de3b55a..1821e49 100644 --- a/NTFSSecurity/InheritanceCmdlets/EnableAccessInheritance.cs +++ b/NTFSSecurity/InheritanceCmdlets/EnableAccessInheritance.cs @@ -81,14 +81,10 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - FileSystemInheritanceInfo.EnableAccessInheritance(item, removeExplicitAccessRules); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + FileSystemInheritanceInfo.EnableAccessInheritance(item, removeExplicitAccessRules); + }); } catch (Exception ex2) { diff --git a/NTFSSecurity/InheritanceCmdlets/EnableAuditInheritance.cs b/NTFSSecurity/InheritanceCmdlets/EnableAuditInheritance.cs index 943234b..d3c753c 100644 --- a/NTFSSecurity/InheritanceCmdlets/EnableAuditInheritance.cs +++ b/NTFSSecurity/InheritanceCmdlets/EnableAuditInheritance.cs @@ -81,14 +81,10 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - FileSystemInheritanceInfo.EnableAuditInheritance(item, removeExplicitAccessRules); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + FileSystemInheritanceInfo.EnableAuditInheritance(item, removeExplicitAccessRules); + }); } catch (Exception ex2) { diff --git a/NTFSSecurity/InheritanceCmdlets/GetInheritance.cs b/NTFSSecurity/InheritanceCmdlets/GetInheritance.cs index df19f8d..07d510c 100644 --- a/NTFSSecurity/InheritanceCmdlets/GetInheritance.cs +++ b/NTFSSecurity/InheritanceCmdlets/GetInheritance.cs @@ -72,14 +72,10 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - inheritanceInfo = FileSystemInheritanceInfo.GetFileSystemInheritanceInfo(item); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + inheritanceInfo = FileSystemInheritanceInfo.GetFileSystemInheritanceInfo(item); + }); } catch (Exception ex2) { diff --git a/NTFSSecurity/InheritanceCmdlets/SetInheritance.cs b/NTFSSecurity/InheritanceCmdlets/SetInheritance.cs index 1a07fa7..3dcc8b3 100644 --- a/NTFSSecurity/InheritanceCmdlets/SetInheritance.cs +++ b/NTFSSecurity/InheritanceCmdlets/SetInheritance.cs @@ -89,14 +89,10 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - SetInheritanceState(item); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + SetInheritanceState(item); + }); } catch (Exception ex2) { diff --git a/NTFSSecurity/MiscCmdlets/GetFileHash2.cs b/NTFSSecurity/MiscCmdlets/GetFileHash2.cs index 52a01b8..2e46f78 100644 --- a/NTFSSecurity/MiscCmdlets/GetFileHash2.cs +++ b/NTFSSecurity/MiscCmdlets/GetFileHash2.cs @@ -68,14 +68,10 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - hash = ((FileInfo)item).GetHash(algorithm); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + hash = ((FileInfo)item).GetHash(algorithm); + }); } catch (Exception ex2) { diff --git a/NTFSSecurity/SecurityDescriptorCmdlets/GetSecurityDescriptor.cs b/NTFSSecurity/SecurityDescriptorCmdlets/GetSecurityDescriptor.cs index 0b9dc93..4d5e3f4 100644 --- a/NTFSSecurity/SecurityDescriptorCmdlets/GetSecurityDescriptor.cs +++ b/NTFSSecurity/SecurityDescriptorCmdlets/GetSecurityDescriptor.cs @@ -56,14 +56,10 @@ namespace NTFSSecurity { try { - var ownerInfo = FileSystemOwner.GetOwner(item); - var previousOwner = ownerInfo.Owner; - - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - - WriteObject(new FileSystemSecurity2(item)); - - FileSystemOwner.SetOwner(item, previousOwner); + InvokeAsOwner(item, path, () => + { + WriteObject(new FileSystemSecurity2(item)); + }); } catch (Exception ex2) { diff --git a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml index 23a434b..9425ccf 100644 --- a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml +++ b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml @@ -712,7 +712,7 @@ Security2.FileSystemAccessRule2 - With `-PassThru`, the cmdlet writes all access control entries of every processed item, explicit and inherited. Without `-PassThru` it writes nothing. + With `-PassThru`, the cmdlet writes all access control entries, explicit and inherited, of every item it changed; an item it could not change produces only an error. Before 5.0.0, `-PassThru` also wrote the unchanged entries of such an item. Without `-PassThru` it writes nothing. @@ -1493,7 +1493,7 @@ Security2.FileSystemAuditRule2 - Without `-PassThru` the cmdlet writes nothing. With `-PassThru` the cmdlet writes all audit entries of the item or the security descriptor, explicit and inherited ones, as `Security2.FileSystemAuditRule2` objects. Before 5.0.0, the `SecurityDescriptor` sets wrote the access entries of the descriptor instead. + Without `-PassThru` the cmdlet writes nothing. With `-PassThru` the cmdlet writes all audit entries of the item or the security descriptor, explicit and inherited ones, as `Security2.FileSystemAuditRule2` objects. Before 5.0.0, the `SecurityDescriptor` sets wrote the access entries of the descriptor instead. An item whose audit entries could not be changed produces only an error; before 5.0.0, `-PassThru` also wrote its unchanged entries. @@ -1501,7 +1501,7 @@ When the module setting `EnablePrivileges` is `$true` (the default in the `PrivateData` section of NTFSSecurity.psd1), this cmdlet tries to enable the Backup, Restore, Take Ownership, and Security privileges while it runs and disables the privileges it enabled when it finishes. These privileges are only available in an elevated session of an account that holds them, such as a member of the local Administrators group. If a privilege cannot be enabled, the cmdlet continues without it and writes a debug message. Writing the SACL requires the Security privilege (`SeSecurityPrivilege`, "Manage auditing and security log"), so run this cmdlet in an elevated session of an account that holds that privilege. Without it, the cmdlet writes a non-terminating `AddAceError` whose message states that a required privilege is not held by the client, and the item is left unchanged. - If the security descriptor cannot be read or written because access is denied, the cmdlet takes ownership of the item, repeats the operation, and restores the previous owner. If the second attempt fails as well, the cmdlet writes an error, and the ownership change is not rolled back. + If the security descriptor cannot be read or written because access is denied, the cmdlet takes ownership of the item, repeats the operation, and restores the previous owner. If the second attempt fails as well, the cmdlet restores the previous owner and writes an error. Before 5.0.0, the account that ran the cmdlet stayed the owner of the item in that case. `-Path` or `-SecurityDescriptor`, `-Account`, and `-AccessRights` are positional parameters at positions 1, 2, and 3, like in `Remove-NTFSAudit`. Before 5.0.0, `-Account` and `-AccessRights` were both declared at position 2, so a command that passed them by position failed. An audit entry alone does not create events. Windows writes the events to the security log only while the "Audit object access" policy, or the corresponding "Audit File System" advanced audit policy, is enabled for success, failure, or both. That policy is a Windows setting and is not managed by this module. @@ -1913,7 +1913,7 @@ PS C:\> Set-NTFSSecurityDescriptor -SecurityDescriptor $sd When the module setting `EnablePrivileges` is `$true` (the default in the `PrivateData` section of NTFSSecurity.psd1), this cmdlet tries to enable the Backup, Restore, Take Ownership, and Security privileges while it runs and disables the privileges it enabled when it finishes. These privileges are only available in an elevated session of an account that holds them, such as a member of the local Administrators group. If a privilege cannot be enabled, the cmdlet continues without it and writes a debug message. Reading and writing the SACL requires the Security privilege (`SeSecurityPrivilege`, "Manage auditing and security log"), so run this cmdlet in an elevated session of an account that holds that privilege. Without that privilege the cmdlet reads the security descriptor without its SACL, finds no audit entries to remove, and finishes without an error although nothing was changed. `-DisableInheritance` fails in that situation with a `ClearAclError` whose message states that a required privilege is not held by the client. - If the security descriptor cannot be read or written because access is denied, the cmdlet takes ownership of the item, repeats the operation, and restores the previous owner. If the second attempt fails as well, the cmdlet writes an error, and the ownership change is not rolled back. + If the security descriptor cannot be read or written because access is denied, the cmdlet takes ownership of the item, repeats the operation, and restores the previous owner. If the second attempt fails as well, the cmdlet restores the previous owner and writes an error. Before 5.0.0, the account that ran the cmdlet stayed the owner of the item in that case. @@ -8380,7 +8380,7 @@ PS C:\Data> Get-NTFSSecurityDescriptor Security2.FileSystemAccessRule2 - With `-PassThru`, the cmdlet writes all access control entries of every processed item, explicit and inherited. Without `-PassThru` it writes nothing. + With `-PassThru`, the cmdlet writes all access control entries, explicit and inherited, of every item it changed; an item it could not change produces only an error. Before 5.0.0, `-PassThru` also wrote the unchanged entries of such an item. Without `-PassThru` it writes nothing. @@ -9219,7 +9219,7 @@ PS C:\Data> Get-NTFSSecurityDescriptor Security2.FileSystemAuditRule2 - Without `-PassThru` the cmdlet writes nothing. With `-PassThru` the cmdlet writes all audit entries of the item or the security descriptor, explicit and inherited ones, as `Security2.FileSystemAuditRule2` objects. Before 5.0.0, the `Path` sets wrote the access entries of the item instead. + Without `-PassThru` the cmdlet writes nothing. With `-PassThru` the cmdlet writes all audit entries of the item or the security descriptor, explicit and inherited ones, as `Security2.FileSystemAuditRule2` objects. Before 5.0.0, the `Path` sets wrote the access entries of the item instead. An item whose audit entries could not be changed produces only an error; before 5.0.0, `-PassThru` also wrote its unchanged entries. @@ -9227,7 +9227,7 @@ PS C:\Data> Get-NTFSSecurityDescriptor When the module setting `EnablePrivileges` is `$true` (the default in the `PrivateData` section of NTFSSecurity.psd1), this cmdlet tries to enable the Backup, Restore, Take Ownership, and Security privileges while it runs and disables the privileges it enabled when it finishes. These privileges are only available in an elevated session of an account that holds them, such as a member of the local Administrators group. If a privilege cannot be enabled, the cmdlet continues without it and writes a debug message. Reading and writing the SACL requires the Security privilege (`SeSecurityPrivilege`, "Manage auditing and security log"), so run this cmdlet in an elevated session of an account that holds that privilege. Without it, the cmdlet writes a non-terminating `RemoveAceError` whose message states that a required privilege is not held by the client, and the item is left unchanged. - If the security descriptor cannot be read or written because access is denied, the cmdlet takes ownership of the item, repeats the operation, and restores the previous owner. If the second attempt fails as well, the cmdlet writes an error, and the ownership change is not rolled back. + If the security descriptor cannot be read or written because access is denied, the cmdlet takes ownership of the item, repeats the operation, and restores the previous owner. If the second attempt fails as well, the cmdlet restores the previous owner and writes an error. Before 5.0.0, the account that ran the cmdlet stayed the owner of the item in that case. The cmdlet reports no error when no entry matches the supplied values. Compare the result with `Get-NTFSAudit` to confirm that the entry is gone. Before 5.0.0, the cmdlet had no `-RemoveSpecific` switch. A path that does not exist produces the non-terminating error `ReadFileError`, and the cmdlet continues with the next path. Before 5.0.0, the cmdlet also wrote a misleading `RemoveAceError` for that path, and with `-PassThru` it stopped with a `NullReferenceException`. diff --git a/Tests/Access.Tests.ps1 b/Tests/Access.Tests.ps1 index 11b11c8..bede5be 100644 --- a/Tests/Access.Tests.ps1 +++ b/Tests/Access.Tests.ps1 @@ -6,6 +6,12 @@ )] param () +BeforeDiscovery { + Import-Module -Name (Join-Path -Path $PSScriptRoot -ChildPath 'TestHelpers.psm1') -Force + # With the Restore privilege, Windows may grant writing the DACL despite a deny entry. + $canBypassWriteDeny = Test-PrivilegeHeld -Name 'SeRestorePrivilege' +} + BeforeAll { Import-Module -Name (Join-Path -Path $PSScriptRoot -ChildPath 'TestHelpers.psm1') -Force $modulePath = Join-Path -Path $PSScriptRoot -ChildPath '..\NTFSSecurity\bin\Release\NTFSSecurity.psd1' @@ -168,6 +174,22 @@ Describe 'Remove-NTFSAccess' { } } + Context 'When the entries cannot be changed' { + # Before 5.0.0, -PassThru returned the unchanged entries of the item after the error. + It 'Should write an error and return nothing with -PassThru' -Skip:$canBypassWriteDeny { + $protected = New-TestSandboxItem -Sandbox $sandbox -Name 'RemoveProtected' + Assert-TestSandboxPath -Sandbox $sandbox -Path $protected + Add-NTFSAccess -Path $protected -Account 'Everyone' -AccessRights ReadData + Block-TestWritePermission -Sandbox $sandbox -Path $protected + + $result = @(Remove-NTFSAccess -Path $protected -Account 'Everyone' -AccessRights ReadData -PassThru -ErrorVariable removeErrors -ErrorAction SilentlyContinue) + + $removeErrors | Should -HaveCount 1 + $removeErrors[0].FullyQualifiedErrorId | Should -BeLike 'RemoveAceError,*' + $result | Should -BeNullOrEmpty + } + } + Context 'With -RemoveSpecific' { BeforeEach { $removeFolder = New-TestSandboxItem -Sandbox $sandbox -Name 'RemoveSpecific' -Directory @@ -202,6 +224,23 @@ Describe 'Remove-NTFSAccess' { } } +Describe 'Add-NTFSAccess' { + Context 'When the entries cannot be changed' { + # Before 5.0.0, -PassThru returned the unchanged entries of the item after the error. + It 'Should write an error and return nothing with -PassThru' -Skip:$canBypassWriteDeny { + $protected = New-TestSandboxItem -Sandbox $sandbox -Name 'AddProtected' + Assert-TestSandboxPath -Sandbox $sandbox -Path $protected + Block-TestWritePermission -Sandbox $sandbox -Path $protected + + $result = @(Add-NTFSAccess -Path $protected -Account 'Everyone' -AccessRights ReadData -PassThru -ErrorVariable addErrors -ErrorAction SilentlyContinue) + + $addErrors | Should -HaveCount 1 + $addErrors[0].FullyQualifiedErrorId | Should -BeLike 'AddAceError,*' + $result | Should -BeNullOrEmpty + } + } +} + Describe 'Security descriptor parameter sets' { BeforeAll { $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'ParameterSets' -Directory diff --git a/Tests/FileHash.Tests.ps1 b/Tests/FileHash.Tests.ps1 index 2ffb4c4..91eeb2c 100644 --- a/Tests/FileHash.Tests.ps1 +++ b/Tests/FileHash.Tests.ps1 @@ -6,6 +6,11 @@ )] param () +BeforeDiscovery { + Import-Module -Name (Join-Path -Path $PSScriptRoot -ChildPath 'TestHelpers.psm1') -Force + $isElevated = Test-IsElevated +} + BeforeAll { Import-Module -Name (Join-Path -Path $PSScriptRoot -ChildPath 'TestHelpers.psm1') -Force $modulePath = Join-Path -Path $PSScriptRoot -ChildPath '..\NTFSSecurity\bin\Release\NTFSSecurity.psd1' @@ -62,4 +67,28 @@ Describe 'Get-FileHash2' { $results.Name | Should -Be @('One.txt') } } + + 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. + It 'Should restore the previous owner' -Skip:(-not $isElevated) { + if ($PSVersionTable.PSEdition -eq 'Core') { + Set-ItResult -Skipped -Because 'Get-FileHash2 fails in PowerShell 7 until it no longer references RIPEMD160' + } + $denied = New-TestSandboxItem -Sandbox $sandbox -Name 'Denied' + Assert-TestSandboxPath -Sandbox $sandbox -Path $denied + Set-NTFSOwner -Path $denied -Account 'S-1-5-32-544' + 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' + } + } } diff --git a/Tests/Inheritance.Tests.ps1 b/Tests/Inheritance.Tests.ps1 index e809462..80d5fc3 100644 --- a/Tests/Inheritance.Tests.ps1 +++ b/Tests/Inheritance.Tests.ps1 @@ -80,7 +80,7 @@ Describe 'Inheritance cmdlets with -PassThru' { $result | Should -BeNullOrEmpty } - It '<_> should write an error and return nothing when the item cannot be read' -Skip:$canBypassDeny -ForEach @( + It '<_> should write an error and return nothing when the security descriptor cannot be read' -Skip:$canBypassDeny -ForEach @( 'Enable-NTFSAccessInheritance', 'Disable-NTFSAccessInheritance' ) { $file = New-TestSandboxItem -Sandbox $sandbox -Name 'Denied' @@ -88,7 +88,21 @@ Describe 'Inheritance cmdlets with -PassThru' { $result = @(& $_ -Path $file -PassThru -ErrorVariable inheritanceErrors -ErrorAction SilentlyContinue) - $inheritanceErrors | Should -Not -BeNullOrEmpty + $inheritanceErrors | Should -HaveCount 1 + $inheritanceErrors[0].FullyQualifiedErrorId | Should -BeLike 'ModifySdError,*' + $result | Should -BeNullOrEmpty + } + + It '<_> should write a read error and return nothing for a path that does not exist' -ForEach @( + 'Enable-NTFSAccessInheritance', 'Disable-NTFSAccessInheritance', + 'Enable-NTFSAuditInheritance', 'Disable-NTFSAuditInheritance' + ) { + $missing = Join-Path -Path $sandbox -ChildPath 'Missing.txt' + + $result = @(& $_ -Path $missing -PassThru -ErrorVariable inheritanceErrors -ErrorAction SilentlyContinue) + + $inheritanceErrors | Should -HaveCount 1 + $inheritanceErrors[0].FullyQualifiedErrorId | Should -BeLike 'ReadFileError,*' $result | Should -BeNullOrEmpty } } diff --git a/Tests/TestHelpers.psm1 b/Tests/TestHelpers.psm1 index 5f11a15..b47faaf 100644 --- a/Tests/TestHelpers.psm1 +++ b/Tests/TestHelpers.psm1 @@ -195,14 +195,70 @@ function Block-TestReadPermission { $Path ) + Add-TestDenyRule -Sandbox $Sandbox -Path $Path -Rights @{ 'S-1-3-4' = 'ReadPermissions' } +} + +function Block-TestWritePermission { + <# + .SYNOPSIS + Denies the owner of an item in the sandbox to change its permissions, so that writing its DACL fails. + .DESCRIPTION + Adds a deny entry for OWNER RIGHTS (S-1-3-4) with ChangePermissions, which replaces the implicit right of the + owner to write the DACL. Taking ownership drops that entry, so the current user is also denied + TakeOwnership; without a privilege, an attempt of the module to take ownership and try again fails as well. + Reading the item and its security descriptor still works, and Remove-TestSandbox deletes the item through + the rights on its folder. + #> + [CmdletBinding()] + param ( + [Parameter(Mandatory)] + [string] + $Sandbox, + + [Parameter(Mandatory)] + [string] + $Path + ) + + $currentUser = [System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value + Add-TestDenyRule -Sandbox $Sandbox -Path $Path -Rights @{ + 'S-1-3-4' = 'ChangePermissions' + $currentUser = 'TakeOwnership' + } +} + +function Add-TestDenyRule { + <# + .SYNOPSIS + Adds deny entries to an item in the sandbox in one write, so that an entry cannot block writing the next. + .PARAMETER Rights + The rights to deny, keyed by the SID of the account. + #> + [CmdletBinding()] + param ( + [Parameter(Mandatory)] + [string] + $Sandbox, + + [Parameter(Mandatory)] + [string] + $Path, + + [Parameter(Mandatory)] + [System.Collections.IDictionary] + $Rights + ) + Assert-TestSandboxPath -Sandbox $Sandbox -Path $Path $acl = Get-Acl -LiteralPath $Path - $ownerRights = New-Object -TypeName 'System.Security.Principal.SecurityIdentifier' -ArgumentList 'S-1-3-4' - $rule = New-Object -TypeName 'System.Security.AccessControl.FileSystemAccessRule' -ArgumentList ( - $ownerRights, [System.Security.AccessControl.FileSystemRights]::ReadPermissions, - [System.Security.AccessControl.AccessControlType]::Deny - ) - $acl.AddAccessRule($rule) + foreach ($sid in $Rights.Keys) { + $identity = New-Object -TypeName 'System.Security.Principal.SecurityIdentifier' -ArgumentList $sid + $rule = New-Object -TypeName 'System.Security.AccessControl.FileSystemAccessRule' -ArgumentList ( + $identity, [System.Security.AccessControl.FileSystemRights] $Rights[$sid], + [System.Security.AccessControl.AccessControlType]::Deny + ) + $acl.AddAccessRule($rule) + } Set-Acl -LiteralPath $Path -AclObject $acl } @@ -247,4 +303,4 @@ function Test-PrivilegeHeld { } Export-ModuleMember -Function New-TestSandbox, Assert-TestSandboxPath, Remove-TestSandbox, New-TestSandboxItem, - Block-TestReadPermission, Test-IsElevated, Test-PrivilegeHeld + Block-TestReadPermission, Block-TestWritePermission, Test-IsElevated, Test-PrivilegeHeld