Browse Source

fix(access): end a collection that holds itself and name the typo in the message of a list

The check that rejects a list for -AccessType and -AppliesTo opened every level
of a collection of one element, so that a collection that holds itself never
returned. It opens at most 32 levels now and counts a deeper nesting as a list,
and it stops reading a collection after the second element, so that a long or
endless sequence is not read to its end.

The message names what the caller wrote: the one element of a collection of one
element (also when that element is a collection itself), the first ten elements
of a longer collection followed by "...", and otherwise the text of the value.
Before, a collection of one misspelled name was named by its type.

Tests, written first: 28 rows failed on the previous build in all four
configurations and none fails now. They cover an array of one misspelled name,
a collection nested 40 levels deep for both parameters of both cmdlets, and an
array that holds an array of one name, which stays valid. The assertion of the
value text expects "The value '<text>'", because the old pattern could also
match the name of a type. The comments say that PowerShell combines the numbers
of a list bit by bit, which is what it does, and not that it adds them.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: AI Assistant <ai@example.com>
pull/131/head
Raimund Andree 1 week ago
parent
commit
da41a8e3ee
  1. 120
      NTFSSecurity/ParameterConversion.cs
  2. 26
      Tests/ParameterSets.Access.Tests.ps1
  3. 21
      Tests/ParameterSets.Audit.Tests.ps1

120
NTFSSecurity/ParameterConversion.cs

@ -17,10 +17,10 @@ namespace NTFSSecurity
/// <summary>
/// Converts the raw value of a parameter to T, the way the PowerShell binder converts an argument for a
/// parameter of that type: names in any case, a comma-separated list for a flags enumeration, a number, or
/// an enumeration instance. An enumeration that is not a flags enumeration takes one value: PowerShell would
/// add up the numbers of a list for it, so that one of the values wins silently or a value that the caller
/// did not name is chosen, so a list is rejected for it, in whatever kind of collection it comes. A
/// collection of one element converts as that element.
/// an enumeration instance. An enumeration that is not a flags enumeration takes one value: PowerShell
/// combines the numbers of a list for it bit by bit, so that one of the values wins silently or a value that
/// the caller did not name is chosen, so a list is rejected for it, in whatever kind of collection it comes.
/// A collection of one element converts as that element.
/// </summary>
/// <param name="value">
/// The raw value of the parameter. Not null: the caller keeps the default of the parameter for null instead
@ -40,13 +40,13 @@ namespace NTFSSecurity
if (!typeof(T).IsDefined(typeof(FlagsAttribute), false))
{
string list;
convertible = TakeOneValue(value, out list);
if (list != null)
bool isList;
convertible = TakeOneValue(value, out isList);
if (isList)
{
throw new ArgumentException(string.Format(
"The value '{0}' for parameter -{1} is not valid: the parameter takes one value, not a list. {2}",
list, parameterName, DescribeValidValues(typeof(T))));
Describe(value), parameterName, DescribeValidValues(typeof(T))));
}
}
@ -64,7 +64,7 @@ namespace NTFSSecurity
catch (Exception ex)
{
throw new ArgumentException(
string.Format("The value '{0}' for parameter -{1} is not valid. {2}", value, parameterName, DescribeValidValues(typeof(T))),
string.Format("The value '{0}' for parameter -{1} is not valid. {2}", Describe(value), parameterName, DescribeValidValues(typeof(T))),
ex);
}
}
@ -82,30 +82,31 @@ 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.
private const int MaxNesting = 32;
// The one value that the conversion takes from the argument of an enumeration that is not a flags enumeration:
// the argument itself, or the only element of a collection of any kind (a wrapped PSObject counts as what it
// wraps, and a collection that holds a collection is opened too). When the argument holds more than one
// value, list is its text for the message: a string that holds a comma, a collection of several elements, or
// a collection of one element that holds such a list; 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.
private static object TakeOneValue(object value, out string list)
{
list = null;
// wraps, and a collection that holds a collection is opened too). isList is true when the argument holds more
// than one value: a string that holds a comma, a collection of several elements (two are enough to know, so
// 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.
private static object TakeOneValue(object value, out bool isList)
{
isList = false;
var current = value;
while (true)
for (var depth = 0; depth < MaxNesting; depth++)
{
var unwrapped = current is PSObject ? ((PSObject)current).BaseObject : current;
var text = unwrapped as string;
if (text != null)
{
if (text.IndexOf(',') >= 0)
{
list = text;
}
isList = text.IndexOf(',') >= 0;
return unwrapped;
}
@ -115,12 +116,19 @@ namespace NTFSSecurity
return unwrapped;
}
var items = new System.Collections.Generic.List<object>();
var count = 0;
object only = null;
try
{
foreach (var item in collection)
{
items.Add(item);
count++;
if (count > 1)
{
break;
}
only = item;
}
}
catch (Exception)
@ -128,19 +136,71 @@ namespace NTFSSecurity
return value;
}
if (items.Count == 1)
if (count == 1)
{
current = items[0];
current = only;
continue;
}
if (items.Count > 1)
{
list = string.Join(", ", items.ConvertAll(item => string.Format("{0}", item)));
isList = count > 1;
return value;
}
isList = true;
return value;
}
// 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.
private static string Describe(object value)
{
var current = value;
for (var depth = 0; depth < MaxNesting; depth++)
{
var unwrapped = current is PSObject ? ((PSObject)current).BaseObject : current;
var collection = unwrapped as IEnumerable;
if (unwrapped is string || collection == null || unwrapped is IDictionary)
{
return string.Format("{0}", current);
}
var shown = new System.Collections.Generic.List<object>();
var more = false;
try
{
foreach (var item in collection)
{
if (shown.Count == 10)
{
more = true;
break;
}
shown.Add(item);
}
if (shown.Count == 1 && !more)
{
current = shown[0];
continue;
}
if (shown.Count == 0)
{
return string.Format("{0}", current);
}
return string.Join(", ", shown.ConvertAll(item => string.Format("{0}", item))) + (more ? ", ..." : string.Empty);
}
catch (Exception)
{
return string.Format("{0}", value);
}
}
return string.Format("{0}", value);
}
// The valid names of the enumeration, in declaration order, with a note for a flags enumeration that a

26
Tests/ParameterSets.Access.Tests.ps1

@ -1337,12 +1337,22 @@ Describe 'Get-NTFSEffectiveAccess' {
# piped value the binder cannot convert to the enumeration is an error instead of a silently kept default.
Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationFlags, and AppliesTo' {
BeforeDiscovery {
# A collection nested 40 levels deep is a list too: the conversion opens at most 32 levels, which also ends a
# collection that holds itself (a test with such a collection would not end if the limit were lost).
$accessDeepAccessType = 'Deny'
$accessDeepScope = 'FilesOnly'
foreach ($level in 1..40) {
$accessDeepAccessType = , $accessDeepAccessType
$accessDeepScope = , $accessDeepScope
}
$accessBadShapes = @(
@{ Shape = 'a misspelled name'; Value = 'Typoo'; ValueText = 'Typoo' }
@{ Shape = 'a blank string'; Value = ''; ValueText = $null }
@{ Shape = 'a white-space string'; Value = ' '; ValueText = $null }
@{ 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' }
)
$accessTypeBadShapes = @(
@{ Shape = 'a misspelled name'; Value = 'Denyy'; ValueText = 'Denyy' }
@ -1350,26 +1360,29 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF
@{ Shape = 'a white-space string'; Value = ' '; ValueText = $null }
@{ 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 = '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' }
@{ Shape = 'an array that holds an array of two names'; Value = , @('Allow', 'Deny'); ValueText = 'Allow, Deny' }
@{ Shape = 'a hash set of two names'; Value = [System.Collections.Generic.HashSet[string]]::new([string[]]@('Allow', 'Deny')); ValueText = $null }
@{ Shape = 'the keys of a hashtable'; Value = (@{ Allow = 1; Deny = 2 }).Keys; ValueText = $null }
@{ Shape = 'an array nested 40 levels deep'; Value = $accessDeepAccessType; ValueText = $null }
)
# AccessType and AppliesTo name one value each, not a combination: PowerShell would add the numbers of a list, so that
# one of the values wins silently or, for AppliesTo, a scope that the caller did not name is chosen. Every kind of
# collection counts, also a collection of one element that holds a list.
# AccessType and AppliesTo name one value each, not a combination: PowerShell combines the numbers of a list bit by
# bit, so that one of the values wins silently or, for AppliesTo, a scope that the caller did not name is chosen.
# Every kind of collection counts, also a collection of one element that holds a list.
$accessAppliesToListShapes = @(
@{ Shape = 'a comma list of two names'; Value = 'ThisFolderAndSubfolders,ThisFolderAndFiles'; ValueText = 'ThisFolderAndSubfolders,ThisFolderAndFiles' }
@{ Shape = 'an array of two names'; Value = @('ThisFolderAndSubfolders', 'ThisFolderAndFiles'); ValueText = 'ThisFolderAndSubfolders, ThisFolderAndFiles' }
@{ Shape = 'an array of one comma list'; Value = @('ThisFolderAndSubfolders,ThisFolderAndFiles'); ValueText = 'ThisFolderAndSubfolders,ThisFolderAndFiles' }
@{ Shape = 'a hash set of two names'; Value = [System.Collections.Generic.HashSet[string]]::new([string[]]@('ThisFolderAndSubfolders', 'ThisFolderAndFiles')); ValueText = $null }
@{ Shape = 'the keys of a hashtable'; Value = (@{ ThisFolderAndSubfolders = 1; ThisFolderAndFiles = 2 }).Keys; ValueText = $null }
@{ 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 * (11 + 3 * 5 + 5) = 62 rows.
# The cross product of the two cmdlets that share these parameters and every bad shape: 2 * (13 + 3 * 6 + 6) = 74 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 }
@ -1409,6 +1422,8 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF
@{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = @('ThisFolderAndSubfolders', 'ThisFolderAndFiles') }
@{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = @('ThisFolderAndSubfolders,ThisFolderAndFiles') }
@{ 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 }
}
# A named $null (a literal $null, a variable, an alias, splatting, or $PSDefaultParameterValues) must
@ -1429,6 +1444,7 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF
@{ Description = 'an array of one name'; Value = @('Deny') }
@{ Description = 'a list of one name'; Value = [System.Collections.Generic.List[string]]::new([string[]]@('Deny')) }
@{ Description = 'a hash set of one name'; Value = [System.Collections.Generic.HashSet[string]]::new([string[]]@('Deny')) }
@{ Description = 'an array that holds an array of one name'; Value = , @('Deny') }
)
$accessValidFlagsShapeCases = @(
@ -1527,7 +1543,7 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF
$pipelineErrors[0].TargetObject | Should -Be $path
$pipelineErrors[0].Exception.Message | Should -Match ([regex]::Escape($Parameter))
if ($ValueText) {
$pipelineErrors[0].Exception.Message | Should -Match ([regex]::Escape($ValueText))
$pipelineErrors[0].Exception.Message | Should -Match ("The value '" + [regex]::Escape($ValueText) + "'")
}
(Get-Acl -LiteralPath $path).Sddl | Should -BeExactly $before
$result | Should -Not -BeNullOrEmpty

21
Tests/ParameterSets.Audit.Tests.ps1

@ -1004,12 +1004,20 @@ Describe 'Get-NTFSOrphanedAudit' {
# privilege, unlike most of the tests above.
Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationFlags, and AppliesTo' {
BeforeDiscovery {
# A collection nested 40 levels deep is a list too: the conversion opens at most 32 levels, which also ends a
# collection that holds itself.
$auditDeepScope = 'FilesOnly'
foreach ($level in 1..40) {
$auditDeepScope = , $auditDeepScope
}
$auditBadShapes = @(
@{ Shape = 'a misspelled name'; Value = 'Typoo'; ValueText = 'Typoo' }
@{ Shape = 'a blank string'; Value = ''; ValueText = $null }
@{ Shape = 'a white-space string'; Value = ' '; ValueText = $null }
@{ 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' }
)
$auditFlagsBadShapes = @(
@{ Shape = 'a misspelled name'; Value = 'Failurre'; ValueText = 'Failurre' }
@ -1017,18 +1025,20 @@ Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationF
@{ Shape = 'a white-space string'; Value = ' '; ValueText = $null }
@{ 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' }
)
# The cross product of the two cmdlets that share these parameters and every bad shape: 2 * (5 + 3 * 5 + 5) = 50 rows.
# AppliesTo names one value, not a combination: PowerShell would add the numbers of a list, 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.
# The cross product of the two cmdlets that share these parameters and every bad shape: 2 * (6 + 3 * 6 + 6) = 60 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.
$auditAppliesToListShapes = @(
@{ Shape = 'a comma list of two names'; Value = 'ThisFolderAndSubfolders,ThisFolderAndFiles'; ValueText = 'ThisFolderAndSubfolders,ThisFolderAndFiles' }
@{ Shape = 'an array of two names'; Value = @('ThisFolderAndSubfolders', 'ThisFolderAndFiles'); ValueText = 'ThisFolderAndSubfolders, ThisFolderAndFiles' }
@{ Shape = 'an array of one comma list'; Value = @('ThisFolderAndSubfolders,ThisFolderAndFiles'); ValueText = 'ThisFolderAndSubfolders,ThisFolderAndFiles' }
@{ Shape = 'a hash set of two names'; Value = [System.Collections.Generic.HashSet[string]]::new([string[]]@('ThisFolderAndSubfolders', 'ThisFolderAndFiles')); ValueText = $null }
@{ Shape = 'the keys of a hashtable'; Value = (@{ ThisFolderAndSubfolders = 1; ThisFolderAndFiles = 2 }).Keys; ValueText = $null }
@{ Shape = 'an array nested 40 levels deep'; Value = $auditDeepScope; ValueText = $null }
)
$auditInvalidParameterCases = foreach ($cmdlet in 'Add-NTFSAudit', 'Remove-NTFSAudit') {
@ -1066,6 +1076,7 @@ Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationF
@{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = @('ThisFolderAndSubfolders', 'ThisFolderAndFiles') }
@{ 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 }
}
# A named $null (a literal $null, a variable, splatting, or $PSDefaultParameterValues) must stay a
@ -1181,7 +1192,7 @@ Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationF
$invalidValueErrors[0].CategoryInfo.Category | Should -Be 'InvalidArgument'
$invalidValueErrors[0].Exception.Message | Should -Match ([regex]::Escape($Parameter))
if ($ValueText) {
$invalidValueErrors[0].Exception.Message | Should -Match ([regex]::Escape($ValueText))
$invalidValueErrors[0].Exception.Message | Should -Match ("The value '" + [regex]::Escape($ValueText) + "'")
}
if ($canReadAudit) {
Get-TestAuditSddl -Path $path | Should -BeExactly $before

Loading…
Cancel
Save