From b436423ab832678db25bded71c55d7e2a5b4a63b Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Wed, 21 Oct 2026 18:03:56 +0000 Subject: [PATCH] fix(access): count a sequence as a list when its enumerator fails after the second element The check of a list for -AccessType and -AppliesTo stops reading a collection after the second element. The loop then releases the enumerator, and when that failed the catch handed the collection to the conversion of PowerShell with isList still false. PowerShell combines the numbers of a list bit by bit, so a sequence of Allow and Deny whose enumerator throws in Dispose created a Deny entry, and one of SubfoldersOnly and FilesOnly a scope that nobody named. The catch keeps what the loop knows: two elements read make a list, which is rejected like every other list. Only a custom enumerator can do this; the last review of round 2 found it. Tests, written first: 4 rows failed on the previous commit in all four configurations and none fails now (a compiled stand-in whose enumerator throws in Dispose, for AccessType and AppliesTo, named and piped). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- NTFSSecurity/ParameterConversion.cs | 4 +- Tests/ParameterSets.Access.Tests.ps1 | 92 ++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/NTFSSecurity/ParameterConversion.cs b/NTFSSecurity/ParameterConversion.cs index dc45ed5..f7d1e49 100644 --- a/NTFSSecurity/ParameterConversion.cs +++ b/NTFSSecurity/ParameterConversion.cs @@ -94,7 +94,7 @@ namespace NTFSSecurity // a long or endless sequence is not read to its end), a collection of one element that holds such a list, or // collections nested deeper than MaxNesting; the returned value is then not used. A dictionary, an empty // collection, or a collection that cannot be enumerated is returned as it is, for the conversion to reject or - // convert with its own error. + // convert with its own error; one whose enumeration fails after its second element is a list all the same. private static object TakeOneValue(object value, out bool isList) { isList = false; @@ -134,6 +134,8 @@ namespace NTFSSecurity } catch (Exception) { + // The loop releases the enumerator when it stops after the second element, and that can fail. + isList = count > 1; return value; } diff --git a/Tests/ParameterSets.Access.Tests.ps1 b/Tests/ParameterSets.Access.Tests.ps1 index 8136b65..4950ccc 100644 --- a/Tests/ParameterSets.Access.Tests.ps1 +++ b/Tests/ParameterSets.Access.Tests.ps1 @@ -1775,6 +1775,98 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF } } + # A list is rejected when the conversion has read its second element. The loop that reads it then releases the + # enumerator, and an enumerator that fails there must not turn the list into a value that PowerShell converts. + Context 'A sequence whose enumerator fails when it is released' { + BeforeAll { + if (-not ('NtfsSecurityTests.SequenceWhoseEnumeratorFailsOnDispose' -as [type])) { + Add-Type -TypeDefinition @' +namespace NtfsSecurityTests +{ + public sealed class SequenceWhoseEnumeratorFailsOnDispose : System.Collections.IEnumerable + { + private readonly object[] items; + + public SequenceWhoseEnumeratorFailsOnDispose(params object[] items) + { + this.items = items; + } + + public System.Collections.IEnumerator GetEnumerator() + { + return new Enumerator(this.items); + } + + private sealed class Enumerator : System.Collections.IEnumerator, System.IDisposable + { + private readonly object[] items; + private int index = -1; + + public Enumerator(object[] items) + { + this.items = items; + } + + public object Current + { + get { return this.items[this.index]; } + } + + public bool MoveNext() + { + this.index++; + return this.index < this.items.Length; + } + + public void Reset() + { + this.index = -1; + } + + public void Dispose() + { + throw new System.InvalidOperationException("The enumerator fails when it is released."); + } + } + } +} +'@ + } + } + + It 'Should reject a list of two values for given as , and change nothing' -ForEach @( + @{ Parameter = 'AccessType'; Mode = 'named'; Values = @('Allow', 'Deny') } + @{ Parameter = 'AccessType'; Mode = 'piped'; Values = @('Allow', 'Deny') } + @{ Parameter = 'AppliesTo'; Mode = 'named'; Values = @('SubfoldersOnly', 'FilesOnly') } + @{ Parameter = 'AppliesTo'; Mode = 'piped'; Values = @('SubfoldersOnly', 'FilesOnly') } + ) { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'ReleaseFails' -Directory + $before = (Get-Acl -LiteralPath $folder).Sddl + $sequence = [NtfsSecurityTests.SequenceWhoseEnumeratorFailsOnDispose]::new([object[]] $Values) + + $caughtId = $null + if ($Mode -eq 'named') { + $namedArguments = @{ Path = $folder; Account = 'S-1-1-0'; AccessRights = 'ReadData' } + $namedArguments[$Parameter] = $sequence + try { + Add-NTFSAccess @namedArguments -ErrorAction Stop + } + catch { + $caughtId = $_.FullyQualifiedErrorId + } + } + else { + $properties = @{ Path = $folder; Account = 'S-1-1-0'; AccessRights = 'ReadData' } + $properties[$Parameter] = $sequence + [pscustomobject] $properties | Add-NTFSAccess -ErrorVariable pipelineErrors -ErrorAction SilentlyContinue + $caughtId = @($pipelineErrors)[0].FullyQualifiedErrorId + } + + $caughtId | Should -BeLike 'InvalidParameterValue,*' + (Get-Acl -LiteralPath $folder).Sddl | Should -BeExactly $before + } + } + # A typed parameter ends a named $null (for example a variable that misspells a property, such as -AccessType # $rule.AccessTpye) with a terminating ParameterBindingFailed; the object-typed parameter accepts it, so the # cmdlet must not keep the default silently. A named $null is reported the same way as a bad named value.