From ec58a18c28d11dbbeca2753474fe73d4d870647c Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Wed, 21 Oct 2026 17:41:53 +0000 Subject: [PATCH] fix(access): build the message of a bad parameter value even when the text of the value throws The message of the error is built inside the catch block of the conversion, and Describe formatted the one element of a collection of one element outside of any guard. An element whose ToString throws (a script method, for example) ended the command with a terminating ExtendedTypeSystemException instead of the InvalidParameterValue error for that item, and a piped command stopped at that object, where the pages say that the error is reported for just that item. Describe now falls back to the type name whatever the text or the enumeration of the value does, for the five parameters of the four cmdlets alike. Tests, written first: 24 rows failed on the previous build in all four configurations and none fails now (an array of one object whose ToString throws, piped for every parameter and named for one parameter of each kind). New rows also pin what the previous commit claimed but did not test: the message names the first ten elements of a list and ends with an ellipsis from the eleventh, and a sequence of 100,000 elements is read only up to its first elements (a counting sequence, named and piped). A build without the two limits fails 4 of them and a build with a limit of nine fails 6, in all four configurations. The comment of the nesting limit now says exactly what it counts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- NTFSSecurity/ParameterConversion.cs | 59 ++++++++++++++++------------ Tests/ParameterSets.Access.Tests.ps1 | 52 +++++++++++++++++++++++- Tests/ParameterSets.Audit.Tests.ps1 | 10 ++++- 3 files changed, 94 insertions(+), 27 deletions(-) diff --git a/NTFSSecurity/ParameterConversion.cs b/NTFSSecurity/ParameterConversion.cs index 11ee2fa..e6a4a01 100644 --- a/NTFSSecurity/ParameterConversion.cs +++ b/NTFSSecurity/ParameterConversion.cs @@ -82,8 +82,9 @@ namespace NTFSSecurity return string.Format("The value for parameter -{0} is null. {1}", parameterName, DescribeValidValues(typeof(T))); } - // How many collections that hold one collection each are opened before the argument counts as a list: enough - // for any real argument, and a stop for a collection that holds itself. + // The most levels of nested collections that the conversion looks into, counting the argument itself: a value + // that is wrapped in more of them counts as a list. Enough for any real argument, and a stop for a collection + // that holds itself. private const int MaxNesting = 32; // The one value that the conversion takes from the argument of an enumeration that is not a flags enumeration: @@ -151,8 +152,23 @@ namespace NTFSSecurity } // The value as a message names it: a collection of one element by that element (also when it is a collection - // itself), a collection of several elements by its first ten, anything else by its text. + // itself), a collection of several elements by its first ten, anything else by its text. The message is built + // inside a catch block, so this method must not fail whatever the text or the enumeration of a value does; a + // value that cannot be formatted is named by its type. private static string Describe(object value) + { + try + { + return DescribeValue(value); + } + catch (Exception) + { + var unwrapped = value is PSObject ? ((PSObject)value).BaseObject : value; + return unwrapped == null ? string.Empty : unwrapped.GetType().FullName; + } + } + + private static string DescribeValue(object value) { var current = value; @@ -168,36 +184,29 @@ namespace NTFSSecurity var shown = new System.Collections.Generic.List(); var more = false; - try + foreach (var item in collection) { - foreach (var item in collection) - { - if (shown.Count == 10) - { - more = true; - break; - } - - shown.Add(item); - } - - if (shown.Count == 1 && !more) + if (shown.Count == 10) { - current = shown[0]; - continue; + more = true; + break; } - if (shown.Count == 0) - { - return string.Format("{0}", current); - } + shown.Add(item); + } - return string.Join(", ", shown.ConvertAll(item => string.Format("{0}", item))) + (more ? ", ..." : string.Empty); + if (shown.Count == 1 && !more) + { + current = shown[0]; + continue; } - catch (Exception) + + if (shown.Count == 0) { - return string.Format("{0}", value); + return string.Format("{0}", current); } + + return string.Join(", ", shown.ConvertAll(item => string.Format("{0}", item))) + (more ? ", ..." : string.Empty); } return string.Format("{0}", value); diff --git a/Tests/ParameterSets.Access.Tests.ps1 b/Tests/ParameterSets.Access.Tests.ps1 index 4a0a2c6..5693ee4 100644 --- a/Tests/ParameterSets.Access.Tests.ps1 +++ b/Tests/ParameterSets.Access.Tests.ps1 @@ -1346,6 +1346,10 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF $accessDeepScope = , $accessDeepScope } + # The message of the error names the value, and building it must not fail when the text of an element does. + $accessThrowingElement = [pscustomobject]@{ Unrelated = 1 } + $accessThrowingElement | Add-Member -MemberType ScriptMethod -Name ToString -Value { throw 'ToString throws' } -Force + $accessBadShapes = @( @{ Shape = 'a misspelled name'; Value = 'Typoo'; ValueText = 'Typoo' } @{ Shape = 'a blank string'; Value = ''; ValueText = $null } @@ -1353,6 +1357,7 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF @{ Shape = 'an undefined number'; Value = 99; ValueText = '99' } @{ Shape = 'a hashtable'; Value = @{ Unrelated = 1 }; ValueText = $null } @{ Shape = 'an array of one misspelled name'; Value = @('Typoo'); ValueText = 'Typoo' } + @{ Shape = 'an array of one object whose ToString throws'; Value = @($accessThrowingElement); ValueText = $null } ) $accessTypeBadShapes = @( @{ Shape = 'a misspelled name'; Value = 'Denyy'; ValueText = 'Denyy' } @@ -1361,6 +1366,7 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF @{ Shape = 'an undefined number'; Value = 99; ValueText = '99' } @{ Shape = 'a hashtable'; Value = @{ Unrelated = 1 }; ValueText = $null } @{ Shape = 'an array of one misspelled name'; Value = @('Denyy'); ValueText = 'Denyy' } + @{ Shape = 'an array of one object whose ToString throws'; Value = @($accessThrowingElement); ValueText = $null } @{ Shape = 'a comma list of two names'; Value = 'Allow,Deny'; ValueText = 'Allow,Deny' } @{ Shape = 'an array of two names'; Value = @('Allow', 'Deny'); ValueText = 'Allow, Deny' } @{ Shape = 'an array of one comma list'; Value = @('Allow,Deny'); ValueText = 'Allow,Deny' } @@ -1382,7 +1388,7 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF @{ Shape = 'an array nested 40 levels deep'; Value = $accessDeepScope; ValueText = $null } ) - # The cross product of the two cmdlets that share these parameters and every bad shape: 2 * (13 + 3 * 6 + 6) = 74 rows. + # The cross product of the two cmdlets that share these parameters and every bad shape: 2 * (14 + 3 * 7 + 6) = 82 rows. $accessInvalidParameterCases = foreach ($cmdlet in 'Add-NTFSAccess', 'Remove-NTFSAccess') { foreach ($shapeCase in $accessTypeBadShapes) { @{ Cmdlet = $cmdlet; Parameter = 'AccessType'; Shape = $shapeCase.Shape; Value = $shapeCase.Value; ValueText = $shapeCase.ValueText } @@ -1397,6 +1403,15 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF } } + # The message names a list by its first ten elements: ten are named in full, eleven end with an ellipsis. The + # elements of a flags parameter are converted one by one, so these numbers fail there too (4 is not a flag). + $accessInvalidParameterCases += @( + @{ Cmdlet = 'Add-NTFSAccess'; Parameter = 'AccessType'; Shape = 'ten numbers'; Value = 1..10; ValueText = '1, 2, 3, 4, 5, 6, 7, 8, 9, 10' } + @{ Cmdlet = 'Add-NTFSAccess'; Parameter = 'AccessType'; Shape = 'eleven numbers'; Value = 1..11; ValueText = '1, 2, 3, 4, 5, 6, 7, 8, 9, 10, ...' } + @{ Cmdlet = 'Add-NTFSAccess'; Parameter = 'InheritanceFlags'; Shape = 'ten numbers'; Value = 1..10; ValueText = '1, 2, 3, 4, 5, 6, 7, 8, 9, 10' } + @{ Cmdlet = 'Add-NTFSAccess'; Parameter = 'InheritanceFlags'; Shape = 'eleven numbers'; Value = 1..11; ValueText = '1, 2, 3, 4, 5, 6, 7, 8, 9, 10, ...' } + ) + $accessSdInvalidParameterCases = foreach ($cmdlet in 'Add-NTFSAccess', 'Remove-NTFSAccess') { @{ Cmdlet = $cmdlet; Parameter = 'AccessType'; Value = 'Denyy'; ValueText = 'Denyy' } @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = 'Typoo'; ValueText = 'Typoo' } @@ -1424,6 +1439,8 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = [System.Collections.Generic.HashSet[string]]::new([string[]]@('ThisFolderAndSubfolders', 'ThisFolderAndFiles')) } @{ Cmdlet = $cmdlet; Parameter = 'AccessType'; Value = $accessDeepAccessType } @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = $accessDeepScope } + @{ Cmdlet = $cmdlet; Parameter = 'AccessType'; Value = @($accessThrowingElement) } + @{ Cmdlet = $cmdlet; Parameter = 'InheritanceFlags'; Value = @($accessThrowingElement) } } # A named $null (a literal $null, a variable, an alias, splatting, or $PSDefaultParameterValues) must @@ -1722,6 +1739,39 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF } } + # The conversion reads at most two elements of a sequence to know that it is a list, and the message names ten, so a + # sequence that is long (or has no end) is not read to its end. + Context 'A long sequence of values' { + It 'Should read only the first elements of a long sequence given as -AccessType, and name its first ten in the error' -ForEach @( + @{ Mode = 'named' } + @{ Mode = 'piped' } + ) { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'LongSequence' -Directory + $before = (Get-Acl -LiteralPath $folder).Sddl + $counter = @{ Reads = 0 } + $count = { $counter.Reads++; 'Allow' }.GetNewClosure() + $sequence = [System.Linq.Enumerable]::Select([int[]] (1..100000), [Func[int, string]] $count) + + $message = $null + if ($Mode -eq 'named') { + try { + Add-NTFSAccess -Path $folder -Account 'S-1-1-0' -AccessRights ReadData -AccessType $sequence -ErrorAction Stop + } + catch { + $message = $_.Exception.Message + } + } + else { + [pscustomobject]@{ Path = $folder; Account = 'S-1-1-0'; AccessRights = 'ReadData'; AccessType = $sequence } | Add-NTFSAccess -ErrorVariable pipelineErrors -ErrorAction SilentlyContinue + $message = @($pipelineErrors)[0].Exception.Message + } + + $message | Should -Match ('The value ''' + ('Allow, ' * 10) + '\.\.\.''') + $counter.Reads | Should -BeLessThan 50 + (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. diff --git a/Tests/ParameterSets.Audit.Tests.ps1 b/Tests/ParameterSets.Audit.Tests.ps1 index 5207d18..6c4aab4 100644 --- a/Tests/ParameterSets.Audit.Tests.ps1 +++ b/Tests/ParameterSets.Audit.Tests.ps1 @@ -1011,6 +1011,10 @@ Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationF $auditDeepScope = , $auditDeepScope } + # The message of the error names the value, and building it must not fail when the text of an element does. + $auditThrowingElement = [pscustomobject]@{ Unrelated = 1 } + $auditThrowingElement | Add-Member -MemberType ScriptMethod -Name ToString -Value { throw 'ToString throws' } -Force + $auditBadShapes = @( @{ Shape = 'a misspelled name'; Value = 'Typoo'; ValueText = 'Typoo' } @{ Shape = 'a blank string'; Value = ''; ValueText = $null } @@ -1018,6 +1022,7 @@ Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationF @{ Shape = 'an undefined number'; Value = 99; ValueText = '99' } @{ Shape = 'a hashtable'; Value = @{ Unrelated = 1 }; ValueText = $null } @{ Shape = 'an array of one misspelled name'; Value = @('Typoo'); ValueText = 'Typoo' } + @{ Shape = 'an array of one object whose ToString throws'; Value = @($auditThrowingElement); ValueText = $null } ) $auditFlagsBadShapes = @( @{ Shape = 'a misspelled name'; Value = 'Failurre'; ValueText = 'Failurre' } @@ -1026,9 +1031,10 @@ Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationF @{ Shape = 'an undefined number'; Value = 99; ValueText = '99' } @{ Shape = 'a hashtable'; Value = @{ Unrelated = 1 }; ValueText = $null } @{ Shape = 'an array of one misspelled name'; Value = @('Failurre'); ValueText = 'Failurre' } + @{ Shape = 'an array of one object whose ToString throws'; Value = @($auditThrowingElement); ValueText = $null } ) - # The cross product of the two cmdlets that share these parameters and every bad shape: 2 * (6 + 3 * 6 + 6) = 60 rows. + # The cross product of the two cmdlets that share these parameters and every bad shape: 2 * (7 + 3 * 7 + 6) = 68 rows. # AppliesTo names one value, not a combination: PowerShell combines the numbers of a list bit by bit, so that one # of the values wins silently or a scope that the caller did not name is chosen. AuditFlags and the two flags # parameters take a list. Every kind of collection counts, also a collection of one element that holds a list. @@ -1077,6 +1083,8 @@ Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationF @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = @('ThisFolderAndSubfolders,ThisFolderAndFiles') } @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = [System.Collections.Generic.HashSet[string]]::new([string[]]@('ThisFolderAndSubfolders', 'ThisFolderAndFiles')) } @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = $auditDeepScope } + @{ Cmdlet = $cmdlet; Parameter = 'AuditFlags'; Value = @($auditThrowingElement) } + @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = @($auditThrowingElement) } } # A named $null (a literal $null, a variable, splatting, or $PSDefaultParameterValues) must stay a