diff --git a/NTFSSecurity/AccessCmdlets/AddAccess.cs b/NTFSSecurity/AccessCmdlets/AddAccess.cs index 249874d..550905f 100644 --- a/NTFSSecurity/AccessCmdlets/AddAccess.cs +++ b/NTFSSecurity/AccessCmdlets/AddAccess.cs @@ -234,11 +234,22 @@ namespace NTFSSecurity } // A bad value named on the command line is already bound when BeginProcessing runs, so it is reported there - // as a terminating error, the way the binder itself would have reported it for a typed parameter. + // as a terminating error, the way the binder itself would have reported it for a typed parameter. A named + // $null is bound too, whether it came from a literal $null, a variable, the alias -AccessControlType, + // splatting, or $PSDefaultParameterValues (MyInvocation.BoundParameters always has the canonical name), and + // is an error the same way; a $null that a piped object's property carries is not bound yet at this point, + // so it keeps the parameter's own default instead, as before. private void ValidateNamedValue(object value, string parameterName) { if (value == null) { + if (!MyInvocation.BoundParameters.ContainsKey(parameterName)) + { + return; + } + + var nullException = new ArgumentException(ParameterConversion.DescribeNull(parameterName)); + ThrowTerminatingError(new ErrorRecord(nullException, "InvalidParameterValue", ErrorCategory.InvalidArgument, value)); return; } diff --git a/NTFSSecurity/AccessCmdlets/RemoveAccess.cs b/NTFSSecurity/AccessCmdlets/RemoveAccess.cs index d2ac020..a9d3a74 100644 --- a/NTFSSecurity/AccessCmdlets/RemoveAccess.cs +++ b/NTFSSecurity/AccessCmdlets/RemoveAccess.cs @@ -245,11 +245,22 @@ namespace NTFSSecurity } // A bad value named on the command line is already bound when BeginProcessing runs, so it is reported there - // as a terminating error, the way the binder itself would have reported it for a typed parameter. + // as a terminating error, the way the binder itself would have reported it for a typed parameter. A named + // $null is bound too, whether it came from a literal $null, a variable, the alias -AccessControlType, + // splatting, or $PSDefaultParameterValues (MyInvocation.BoundParameters always has the canonical name), and + // is an error the same way; a $null that a piped object's property carries is not bound yet at this point, + // so it keeps the parameter's own default instead, as before. private void ValidateNamedValue(object value, string parameterName) { if (value == null) { + if (!MyInvocation.BoundParameters.ContainsKey(parameterName)) + { + return; + } + + var nullException = new ArgumentException(ParameterConversion.DescribeNull(parameterName)); + ThrowTerminatingError(new ErrorRecord(nullException, "InvalidParameterValue", ErrorCategory.InvalidArgument, value)); return; } diff --git a/NTFSSecurity/AuditCmdlets/AddAudit.cs b/NTFSSecurity/AuditCmdlets/AddAudit.cs index fa3f8da..560d333 100644 --- a/NTFSSecurity/AuditCmdlets/AddAudit.cs +++ b/NTFSSecurity/AuditCmdlets/AddAudit.cs @@ -238,11 +238,22 @@ namespace NTFSSecurity } // A bad value named on the command line is already bound when BeginProcessing runs, so it is reported there - // as a terminating error, the way the binder itself would have reported it for a typed parameter. + // as a terminating error, the way the binder itself would have reported it for a typed parameter. A named + // $null is bound too, whether it came from a literal $null, a variable, the alias -AccessControlType, + // splatting, or $PSDefaultParameterValues (MyInvocation.BoundParameters always has the canonical name), and + // is an error the same way; a $null that a piped object's property carries is not bound yet at this point, + // so it keeps the parameter's own default instead, as before. private void ValidateNamedValue(object value, string parameterName) { if (value == null) { + if (!MyInvocation.BoundParameters.ContainsKey(parameterName)) + { + return; + } + + var nullException = new ArgumentException(ParameterConversion.DescribeNull(parameterName)); + ThrowTerminatingError(new ErrorRecord(nullException, "InvalidParameterValue", ErrorCategory.InvalidArgument, value)); return; } diff --git a/NTFSSecurity/AuditCmdlets/RemoveAudit.cs b/NTFSSecurity/AuditCmdlets/RemoveAudit.cs index 6d308e1..b1de294 100644 --- a/NTFSSecurity/AuditCmdlets/RemoveAudit.cs +++ b/NTFSSecurity/AuditCmdlets/RemoveAudit.cs @@ -249,11 +249,22 @@ namespace NTFSSecurity } // A bad value named on the command line is already bound when BeginProcessing runs, so it is reported there - // as a terminating error, the way the binder itself would have reported it for a typed parameter. + // as a terminating error, the way the binder itself would have reported it for a typed parameter. A named + // $null is bound too, whether it came from a literal $null, a variable, the alias -AccessControlType, + // splatting, or $PSDefaultParameterValues (MyInvocation.BoundParameters always has the canonical name), and + // is an error the same way; a $null that a piped object's property carries is not bound yet at this point, + // so it keeps the parameter's own default instead, as before. private void ValidateNamedValue(object value, string parameterName) { if (value == null) { + if (!MyInvocation.BoundParameters.ContainsKey(parameterName)) + { + return; + } + + var nullException = new ArgumentException(ParameterConversion.DescribeNull(parameterName)); + ThrowTerminatingError(new ErrorRecord(nullException, "InvalidParameterValue", ErrorCategory.InvalidArgument, value)); return; } diff --git a/NTFSSecurity/ParameterConversion.cs b/NTFSSecurity/ParameterConversion.cs index bb6241f..9adb60b 100644 --- a/NTFSSecurity/ParameterConversion.cs +++ b/NTFSSecurity/ParameterConversion.cs @@ -50,6 +50,19 @@ namespace NTFSSecurity } } + /// + /// Builds the message for a parameter whose value is explicitly null although it was bound on the command + /// line (a literal $null, a variable that holds $null, or a null entry of $PSDefaultParameterValues): "The + /// value for parameter -AccessType is null. Valid values: Allow, Deny." A piped property that is null is + /// not an error there: it means the property was not specified and the parameter's own default applies; + /// only the cmdlet knows, from MyInvocation.BoundParameters, whether a null came from such a named value. + /// + /// The name of the parameter, without the leading dash, for the message. + internal static string DescribeNull(string parameterName) + { + return string.Format("The value for parameter -{0} is null. {1}", parameterName, DescribeValidValues(typeof(T))); + } + // The valid names of the enumeration, in declaration order, with a note for a flags enumeration that a // combination of them is written separated with commas. private static string DescribeValidValues(Type enumType) diff --git a/Tests/ParameterSets.Access.Tests.ps1 b/Tests/ParameterSets.Access.Tests.ps1 index af20c36..c3fd1c9 100644 --- a/Tests/ParameterSets.Access.Tests.ps1 +++ b/Tests/ParameterSets.Access.Tests.ps1 @@ -1384,6 +1384,16 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = 'Typoo' } } + # R1-2: a named $null (a literal $null, a variable, an alias, splatting, or $PSDefaultParameterValues) must + # stay a terminating error, the same as a bad named value; only a $null that a piped object's property + # carries keeps the parameter's own default. + $accessNamedNullCases = foreach ($cmdlet in 'Add-NTFSAccess', 'Remove-NTFSAccess') { + @{ Cmdlet = $cmdlet; Parameter = 'AccessType' } + @{ Cmdlet = $cmdlet; Parameter = 'InheritanceFlags' } + @{ Cmdlet = $cmdlet; Parameter = 'PropagationFlags' } + @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo' } + } + $accessValidAccessTypeShapeCases = @( @{ Description = 'the exact name'; Value = 'Deny' } @{ Description = 'a different case'; Value = 'deny' } @@ -1612,6 +1622,98 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF } } + # R1-2: rc8 ended a named $null (for example a variable that misspells a property, such as -AccessType + # $rule.AccessTpye) with a terminating ParameterBindingFailed; the object-typed parameter accepted it and + # silently kept the default instead. A named $null is now reported the same way as a bad named value. + Context 'A named $null is a terminating error' { + It 'Should throw a terminating error for a named $null of -, and change nothing' -ForEach $accessNamedNullCases { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'NamedNull' -Directory + $before = (Get-Acl -LiteralPath $folder).Sddl + $namedArguments = @{ Path = $folder; Account = 'S-1-1-0'; AccessRights = 'ReadData' } + $namedArguments[$Parameter] = $null + + $caughtId = $null + $caughtMessage = $null + try { + & $Cmdlet @namedArguments -ErrorAction Stop + } + catch { + $caughtId = $_.FullyQualifiedErrorId + $caughtMessage = $_.Exception.Message + } + + if ($Parameter -eq 'AppliesTo') { + # AppliesTo is Mandatory: PowerShell's own binder already rejects a named $null before the cmdlet + # runs, with its own error id, unrelated to this fix; characterized here so that the row is not + # silently missing from the table. + $caughtId | Should -BeLike 'ParameterArgumentValidationErrorNullNotAllowed,*' + } + else { + $caughtId | Should -BeLike 'InvalidParameterValue,*' + $caughtMessage | Should -Match ([regex]::Escape($Parameter)) + $caughtMessage | Should -Match 'is null' + } + (Get-Acl -LiteralPath $folder).Sddl | Should -BeExactly $before + } + + It 'Should throw a terminating InvalidParameterValue error for a named $null of -AccessControlType (the alias), and change nothing' -ForEach @( + @{ Cmdlet = 'Add-NTFSAccess' } + @{ Cmdlet = 'Remove-NTFSAccess' } + ) { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'NamedNullAlias' -Directory + $before = (Get-Acl -LiteralPath $folder).Sddl + + $caughtId = $null + try { + & $Cmdlet -Path $folder -Account 'S-1-1-0' -AccessRights ReadData -AccessControlType $null -ErrorAction Stop + } + catch { + $caughtId = $_.FullyQualifiedErrorId + } + + $caughtId | Should -BeLike 'InvalidParameterValue,*' + (Get-Acl -LiteralPath $folder).Sddl | Should -BeExactly $before + } + + It 'Should throw a terminating InvalidParameterValue error for a $null of -AccessType from $PSDefaultParameterValues, and change nothing' { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'NamedNullDefault' -Directory + $before = (Get-Acl -LiteralPath $folder).Sddl + $PSDefaultParameterValues['Add-NTFSAccess:AccessType'] = $null + + $caughtId = $null + try { + Add-NTFSAccess -Path $folder -Account 'S-1-1-0' -AccessRights ReadData -ErrorAction Stop + } + catch { + $caughtId = $_.FullyQualifiedErrorId + } + finally { + $PSDefaultParameterValues.Remove('Add-NTFSAccess:AccessType') + } + + $caughtId | Should -BeLike 'InvalidParameterValue,*' + (Get-Acl -LiteralPath $folder).Sddl | Should -BeExactly $before + } + + It 'Should still give - its own default for a piped $null, unaffected by the named $null fix' -ForEach $accessNamedNullCases { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'PipedNullUnaffected' -Directory + $extra = @{ } + $extra[$Parameter] = $null + + [pscustomobject] (@{ Path = $folder; Account = 'S-1-1-0'; AccessRights = 'ReadData' } + $extra) | + & $Cmdlet -ErrorVariable pipedErrors -ErrorAction SilentlyContinue + + if ($Parameter -eq 'AppliesTo') { + # AppliesTo is Mandatory: a piped $null is the pre-existing binder validation error, unrelated to this fix. + $pipedErrors | Should -HaveCount 1 + $pipedErrors[0].FullyQualifiedErrorId | Should -BeLike 'ParameterArgumentValidationErrorNullNotAllowed,*' + } + else { + $pipedErrors | Should -BeNullOrEmpty + } + } + } + Context 'Choosing the parameter set with a mix of string and enum values' { It 'Should reject a piped object whose AppliesTo is combined with InheritanceFlags as with an AmbiguousParameterSet error' -ForEach $accessAmbiguousValueCases { $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'AmbiguousPipedMix' -Directory diff --git a/Tests/ParameterSets.Audit.Tests.ps1 b/Tests/ParameterSets.Audit.Tests.ps1 index 046bfd4..fb2cb0d 100644 --- a/Tests/ParameterSets.Audit.Tests.ps1 +++ b/Tests/ParameterSets.Audit.Tests.ps1 @@ -1054,6 +1054,16 @@ Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationF @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = 'Typoo' } } + # R1-2: a named $null (a literal $null, a variable, splatting, or $PSDefaultParameterValues) must stay a + # terminating error, the same as a bad named value; only a $null that a piped object's property carries + # keeps the parameter's own default. + $auditNamedNullCases = foreach ($cmdlet in 'Add-NTFSAudit', 'Remove-NTFSAudit') { + @{ Cmdlet = $cmdlet; Parameter = 'AuditFlags' } + @{ Cmdlet = $cmdlet; Parameter = 'InheritanceFlags' } + @{ Cmdlet = $cmdlet; Parameter = 'PropagationFlags' } + @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo' } + } + $auditValidAuditFlagsShapeCases = @( @{ Description = 'the exact name'; Value = 'Failure' } @{ Description = 'a different case'; Value = 'failure' } @@ -1294,6 +1304,82 @@ Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationF } } + # R1-2: rc8 ended a named $null with a terminating ParameterBindingFailed; the object-typed parameter accepted + # it and silently kept the default instead. A named $null is now reported the same way as a bad named value. + Context 'A named $null is a terminating error' { + It 'Should throw a terminating error for a named $null of -, and change nothing' -ForEach $auditNamedNullCases { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'NamedNull' -Directory + $before = if ($canReadAudit) { Get-TestAuditSddl -Path $folder } + $namedArguments = @{ Path = $folder; Account = 'S-1-1-0'; AccessRights = 'ReadData' } + $namedArguments[$Parameter] = $null + + $caughtId = $null + $caughtMessage = $null + try { + & $Cmdlet @namedArguments -ErrorAction Stop + } + catch { + $caughtId = $_.FullyQualifiedErrorId + $caughtMessage = $_.Exception.Message + } + + if ($Parameter -eq 'AppliesTo') { + # AppliesTo is Mandatory: PowerShell's own binder already rejects a named $null before the cmdlet + # runs, with its own error id, unrelated to this fix; characterized here so that the row is not + # silently missing from the table. + $caughtId | Should -BeLike 'ParameterArgumentValidationErrorNullNotAllowed,*' + } + else { + $caughtId | Should -BeLike 'InvalidParameterValue,*' + $caughtMessage | Should -Match ([regex]::Escape($Parameter)) + $caughtMessage | Should -Match 'is null' + } + if ($canReadAudit) { + Get-TestAuditSddl -Path $folder | Should -BeExactly $before + } + } + + It 'Should throw a terminating InvalidParameterValue error for a $null of -AuditFlags from $PSDefaultParameterValues, and change nothing' { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'NamedNullDefault' -Directory + $before = if ($canReadAudit) { Get-TestAuditSddl -Path $folder } + $PSDefaultParameterValues['Add-NTFSAudit:AuditFlags'] = $null + + $caughtId = $null + try { + Add-NTFSAudit -Path $folder -Account 'S-1-1-0' -AccessRights ReadData -ErrorAction Stop + } + catch { + $caughtId = $_.FullyQualifiedErrorId + } + finally { + $PSDefaultParameterValues.Remove('Add-NTFSAudit:AuditFlags') + } + + $caughtId | Should -BeLike 'InvalidParameterValue,*' + if ($canReadAudit) { + Get-TestAuditSddl -Path $folder | Should -BeExactly $before + } + } + + It 'Should still give - its own default for a piped $null, unaffected by the named $null fix' -ForEach $auditNamedNullCases -Skip:(-not $canReadAudit) { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'PipedNullUnaffected' -Directory + $extra = @{ } + $extra[$Parameter] = $null + + [pscustomobject] (@{ Path = $folder; Account = 'S-1-1-0'; AccessRights = 'ReadData' } + $extra) | + & $Cmdlet -ErrorVariable pipedErrors -ErrorAction SilentlyContinue + + if ($Parameter -eq 'AppliesTo') { + # AppliesTo is Mandatory: a piped $null is the pre-existing binder validation error, unrelated to this fix. + $pipedErrors | Should -HaveCount 1 + $pipedErrors[0].FullyQualifiedErrorId | Should -BeLike 'ParameterArgumentValidationErrorNullNotAllowed,*' + } + else { + $pipedErrors | Should -BeNullOrEmpty + } + } + } + Context 'Choosing the parameter set with a mix of string and enum values' { It 'Should reject a piped object whose AppliesTo is combined with InheritanceFlags as with an AmbiguousParameterSet error' -ForEach $auditAmbiguousValueCases { $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'AmbiguousPipedMix' -Directory