Warn on integer ValidateRange bounds for floating-point parameters

Rules/AvoidValidateRangeTypeMismatch.cs

@@ -0,0 +1,163 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using System.Collections.Generic;
+using System.Globalization;
+using System.Linq;
+using System.Management.Automation.Language;
+using System.Reflection;
+using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
+#if !CORECLR
+using System.ComponentModel.Composition;
+#endif
+
+namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
+{
+    /// <summary>
+    /// AvoidValidateRangeTypeMismatch: Detect integer ValidateRange bounds on floating-point and decimal parameters.
+    /// </summary>
+#if !CORECLR
+    [Export(typeof(IScriptRule))]
+#endif
+    public sealed class AvoidValidateRangeTypeMismatch : IScriptRule
+    {
+        private static readonly HashSet<Type> FloatingPointAndDecimalTypes = new HashSet<Type>
+        {
+            typeof(double),
+            typeof(float),
+            typeof(decimal)
+        };
+
+        private static readonly HashSet<Type> IntegralTypes = new HashSet<Type>
+        {
+            typeof(byte),
+            typeof(sbyte),
+            typeof(short),
+            typeof(ushort),
+            typeof(int),
+            typeof(uint),
+            typeof(long),
+            typeof(ulong)
+        };
+
+        public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
+        {
+            if (ast is null)
+            {
+                throw new ArgumentNullException(Strings.NullAstErrorMessage);
+            }
+
+            IEnumerable<Ast> parameterAsts = ast.FindAll(testAst => testAst is ParameterAst, searchNestedScriptBlocks: true);
+            foreach (ParameterAst parameterAst in parameterAsts)
+            {
+                TypeConstraintAst typeConstraint = parameterAst.Attributes.OfType<TypeConstraintAst>().FirstOrDefault();
+                if (typeConstraint == null)
+                {
+                    continue;
+                }
+
+                Type parameterType = typeConstraint.TypeName.GetReflectionType();
+                if (parameterType == null)
+                {
+                    continue;
+                }
+
+                if (parameterType.IsArray)
+                {
+                    parameterType = parameterType.GetElementType();
+                }
+
+                parameterType = Nullable.GetUnderlyingType(parameterType) ?? parameterType;
+
+                if (!FloatingPointAndDecimalTypes.Contains(parameterType))
+                {
+                    continue;
+                }
+
+                foreach (AttributeAst attributeAst in parameterAst.Attributes.OfType<AttributeAst>())
+                {
+                    if (!IsValidateRangeAttribute(attributeAst)
+                        || attributeAst.PositionalArguments.Count < 2
+                        || !attributeAst.PositionalArguments.Take(2).All(IsIntegralBoundary))
+                    {
+                        continue;
+                    }
+
+                    yield return new DiagnosticRecord(
+                        String.Format(CultureInfo.CurrentCulture, Strings.AvoidValidateRangeTypeMismatchError, parameterAst.Name, typeConstraint.TypeName.FullName),
+                        attributeAst.Extent,
+                        GetName(),
+                        DiagnosticSeverity.Warning,
+                        fileName);
+                }
+            }
+        }
+
+        private static bool IsIntegralBoundary(ExpressionAst expressionAst)
+        {
+            if (expressionAst is ConstantExpressionAst constantExpressionAst)
+            {
+                return constantExpressionAst.Value != null && IntegralTypes.Contains(constantExpressionAst.Value.GetType());
+            }
+
+            if (expressionAst is ConvertExpressionAst convertExpressionAst)
+            {
+                return IntegralTypes.Contains(convertExpressionAst.Type.TypeName.GetReflectionType());
+            }
+
+            if (expressionAst is UnaryExpressionAst unaryExpressionAst
+                && (unaryExpressionAst.TokenKind == TokenKind.Minus || unaryExpressionAst.TokenKind == TokenKind.Plus))
+            {
+                return IsIntegralBoundary(unaryExpressionAst.Child);
+            }
+
+            if (expressionAst is MemberExpressionAst memberExpressionAst
+                && memberExpressionAst.Static
+                && memberExpressionAst.Expression is TypeExpressionAst typeExpressionAst
+                && memberExpressionAst.Member is StringConstantExpressionAst memberName)
+            {
+                Type type = typeExpressionAst.TypeName.GetReflectionType();
+                FieldInfo field = type?.GetField(memberName.Value, BindingFlags.Public | BindingFlags.Static | BindingFlags.IgnoreCase | BindingFlags.FlattenHierarchy);
+                return field != null && field.IsLiteral && IntegralTypes.Contains(field.FieldType);
+            }
+
+            return false;
+        }
+
+        private static bool IsValidateRangeAttribute(AttributeAst attributeAst)
+        {
+            return attributeAst.TypeName.GetReflectionAttributeType() == typeof(System.Management.Automation.ValidateRangeAttribute);
+        }
+
+        public string GetName()
+        {
+            return String.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidValidateRangeTypeMismatchName);
+        }
+
+        public string GetCommonName()
+        {
+            return Strings.AvoidValidateRangeTypeMismatchCommonName;
+        }
+
+        public string GetDescription()
+        {
+            return Strings.AvoidValidateRangeTypeMismatchDescription;
+        }
+
+        public SourceType GetSourceType()
+        {
+            return SourceType.Builtin;
+        }
+
+        public RuleSeverity GetSeverity()
+        {
+            return RuleSeverity.Warning;
+        }
+
+        public string GetSourceName()
+        {
+            return String.Format(CultureInfo.CurrentCulture, Strings.SourceName);
+        }
+    }
+}

Rules/Strings.resx

@@ -1221,6 +1221,18 @@
   <data name="AvoidMultipleTypeAttributesName" xml:space="preserve">
     <value>AvoidMultipleTypeAttributes</value>
   </data>
+  <data name="AvoidValidateRangeTypeMismatchCommonName" xml:space="preserve">
+    <value>Avoid mismatched ValidateRange boundary types.</value>
+  </data>
+  <data name="AvoidValidateRangeTypeMismatchDescription" xml:space="preserve">
+    <value>Integer ValidateRange boundaries can allow fractional values for floating-point and decimal parameters. Use floating-point or decimal boundaries that match the parameter type.</value>
+  </data>
+  <data name="AvoidValidateRangeTypeMismatchError" xml:space="preserve">
+    <value>ValidateRange boundaries should match the type of parameter '{0}' ({1}) to correctly restrict fractional values.</value>
+  </data>
+  <data name="AvoidValidateRangeTypeMismatchName" xml:space="preserve">
+    <value>AvoidValidateRangeTypeMismatch</value>
+  </data>
   <data name="AvoidUsingBrokenHashAlgorithmsCommonName" xml:space="preserve">
     <value>Avoid Using Broken Hash Algorithms</value>
   </data>

Tests/Rules/AvoidValidateRangeTypeMismatch.tests.ps1

@@ -0,0 +1,95 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the MIT License.
+
+Describe 'AvoidValidateRangeTypeMismatch' {
+    BeforeAll {
+        $ruleName = 'PSAvoidValidateRangeTypeMismatch'
+        Add-Type -TypeDefinition @'
+public class ValidateRangeBoundsBaseForScriptAnalyzer
+{
+    public const int Max = 100;
+}
+
+public class ValidateRangeBoundsForScriptAnalyzer : ValidateRangeBoundsBaseForScriptAnalyzer
+{
+    public static readonly int ReadonlyIntegral = 100;
+    public const double FloatingPoint = 100.0;
+    public static int ThrowingGetterInvocationCount;
+
+    public static int ThrowingProperty
+    {
+        get
+        {
+            ThrowingGetterInvocationCount++;
+            throw new System.InvalidOperationException();
+        }
+    }
+}
+
+namespace Contoso
+{
+    [System.AttributeUsage(System.AttributeTargets.All)]
+    public class ValidateRangeAttribute : System.Attribute
+    {
+        public ValidateRangeAttribute(int minRange, int maxRange)
+        {
+        }
+    }
+}
+'@
+    }
+
+    It 'reports integral boundaries for floating-point and decimal parameter types' {
+        $scripts = @(
+            'function Test { param([double][ValidateRange(1, 2)]$Value) }'
+            'function Test { param([single][ValidateRange(-1, 2)]$Value) }'
+            'function Test { param([decimal][ValidateRange(1, 2)]$Value) }'
+            'function Test { param([Nullable[double]][ValidateRange(1, 2)]$Value) }'
+            'function Test { param([Nullable[single]][ValidateRange(1, 2)]$Value) }'
+            'function Test { param([Nullable[decimal]][ValidateRange(1, 2)]$Value) }'
+            'function Test { param([double[]][ValidateRange(1, 2)]$Values) }'
+            'function Test { param([double][System.Management.Automation.ValidateRangeAttribute(1, 2)]$Value) }'
+            'function Test { param([double][ValidateRange([int]0.0, [int]100.0)]$Value) }'
+            'function Test { param([double][ValidateRange([int32]0.0, [int32]100.0)]$Value) }'
+            'function Test { param([double][ValidateRange(0, [int]::MaxValue)]$Value) }'
+            'function Test { param([double][ValidateRange(0, [ValidateRangeBoundsForScriptAnalyzer]::Max)]$Value) }'
+            'function Test { param([double][System.Management.Automation.ValidateRange(0, 100)]$Value) }'
+        )
+
+        foreach ($scriptDefinition in $scripts) {
+            $diagnostics = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule $ruleName
+            $diagnostics.Count | Should -Be 1
+            $diagnostics[0].Extent.Text | Should -Match 'ValidateRange'
+        }
+    }
+
+    It 'ignores matching floating-point boundaries and non-floating-point parameters' {
+        $scripts = @(
+            'function Test { param([double][ValidateRange(1.0, 2.0)]$Value) }'
+            'function Test { param([single][ValidateRange(1.0, 2.0)]$Value) }'
+            'function Test { param([decimal][ValidateRange(1.0, 2.0)]$Value) }'
+            'function Test { param([double][ValidateRange(1, 2.0)]$Value) }'
+            'function Test { param([double][Contoso.ValidateRange(0, 100)]$Value) }'
+            'function Test { param([int][ValidateRange(1, 2)]$Value) }'
+        )
+
+        foreach ($scriptDefinition in $scripts) {
+            Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule $ruleName | Should -BeNullOrEmpty
+        }
+    }
+
+    It 'ignores non-constant static member expressions without invoking getters' {
+        $scripts = @(
+            'function Test { param([double][ValidateRange(0, [ValidateRangeBoundsForScriptAnalyzer]::ThrowingProperty)]$Value) }'
+            'function Test { param([double][ValidateRange(0, [ValidateRangeBoundsForScriptAnalyzer]::ReadonlyIntegral)]$Value) }'
+            'function Test { param([double][ValidateRange(0, [ValidateRangeBoundsForScriptAnalyzer]::FloatingPoint)]$Value) }'
+        )
+
+        foreach ($scriptDefinition in $scripts) {
+            Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule $ruleName |
+                Where-Object RuleName -eq $ruleName | Should -BeNullOrEmpty
+        }
+
+        [ValidateRangeBoundsForScriptAnalyzer]::ThrowingGetterInvocationCount | Should -Be 0
+    }
+}

docs/Rules/AvoidValidateRangeTypeMismatch.md

@@ -0,0 +1,49 @@
+---
+description: Detect integer ValidateRange boundaries on floating-point and decimal parameters
+ms.topic: reference
+title: AvoidValidateRangeTypeMismatch
+---
+# AvoidValidateRangeTypeMismatch
+
+**Severity Level: Warning**
+
+**Default state: Always enabled**
+
+## Description
+
+Integer boundaries in a `ValidateRange` attribute can select integer validation behavior for
+floating-point and decimal parameters. This can allow fractional values outside the declared range.
+Use boundaries with the same numeric type as the parameter.
+
+## Example
+
+### Noncompliant
+
+```powershell
+function Set-Percentage
+{
+    param([double][ValidateRange(0, 100)]$Value)
+}
+```
+
+### Compliant
+
+```powershell
+function Set-Percentage
+{
+    param([double][ValidateRange(0.0, 100.0)]$Value)
+}
+```
+
+## Configure rule
+
+This rule is always enabled and isn't configurable. Use one of the following methods to avoid using
+this rule:
+
+- Create a custom rule configuration file to include only the rules you want or exclude the rules
+  you don't want.
+- Add the appropriate rule suppression attributes to your code to suppress the rule for specific
+  code blocks. For more information, see the _Suppressing rules_ section of [Using PSScriptAnalyzer][01].
+
+<!-- Link references -->
+[01]: ../using-scriptanalyzer.md#suppressing-rules

docs/Rules/README.md

@@ -52,6 +52,7 @@ title: List of PSScriptAnalyzer rules
 | [AvoidUsingUsernameAndPasswordParams][32]          | Error       | Always enabled |              |
 | [AvoidUsingWMICmdlet][33]                          | Warning     | Always enabled |              |
 | [AvoidUsingWriteHost][34]                          | Warning     | Always enabled |              |
+| [AvoidValidateRangeTypeMismatch][81]               | Warning     | Always enabled |              |
 | [DSCDscExamplesPresent][35]                        | Information | Always enabled |              |
 | [DSCDscTestsPresent][36]                           | Information | Always enabled |              |
 | [DSCReturnCorrectTypesForDSCFunctions][37]         | Information | Always enabled |              |
@@ -180,3 +181,4 @@ title: List of PSScriptAnalyzer rules
 [78]: UseToExportFieldsInManifest.md
 [79]: UseUsingScopeModifierInNewRunspaces.md
 [80]: UseUTF8EncodingForHelpFile.md
+[81]: AvoidValidateRangeTypeMismatch.md