PowerShell / PSScriptAnalyzer

Download ScriptAnalyzer from PowerShellGallery
https://www.powershellgallery.com/packages/PSScriptAnalyzer/
MIT License
1.85k stars 375 forks source link

Need rule which catches assignments to powershell builtin variables #858

Open JamesWTruher opened 6 years ago

JamesWTruher commented 6 years ago

assignments to variables like $args and $_, etc should be caught. see https://github.com/PowerShell/PSScriptAnalyzer/issues/265

bergmeister commented 6 years ago

@JamesWTruher This looks like a duplicate of #712 In the past there was already a duplicate issue #808 which is worth a read because it has a refernce to a longer discussion in the original PowerShell repo why it was chosen that a PSSA warning was desired instead of changing the implementation.

bergmeister commented 6 years ago

Just FYI that I am currently implemting the rule in this branch. The requirements seem to be relatively clear this time to get started. I plan to split it into 2 rules: one type of variable to definetly avoid like $false because it will throw an error and one to re-consider usage for e.g. $_

LaurentDardenne commented 6 years ago

Your implementation does not seem to consider these cases:

function Test { 
 param ($args,$input=10) 

 $args=5
 $input
 $args
}
bergmeister commented 6 years ago

@LaurentDardenne What I currrently have on my branch is just a simple prototype where I added a couple of variables so that I could manually test if the detection logic works well. Before submitting a PR there will be more work to analyze the variables and there will probably a split into 2 categories: One is variables that are definitely not allowed to be assigned like $false (because PS would throw) and a subset of automatic variables where the user should make a conscious choice if he/she really wants to override them or not (like e.g. $input) and suppress the warning. I am happy to take opinions which variables should go into the 'naughty list' and which not. To be honest, it would make it easier for me if people commented with a list of variables that they definitely do or do not want to be warned about.

LaurentDardenne commented 6 years ago

For the code, the FindAll() call looks for AssignmentStatementAst and not ParameterAst, it may be a similar control but in a different context.

According to this:

TOPIC
    about_Automatic_Variables

SHORT DESCRIPTION
    Describes variables that store state information for Windows PowerShell.
    These variables are created and maintained by Windows PowerShell.

Except for $OFS which sets a processing, I do not see for the moment, other variables of this list for which one authorized an assignment. $Matches and $PSBoundParameters can be modified but by method calls.

Maybe $Matches could be excluded to allow this type of code :

#Version 1
function Test($p) {
 $p -match "e"
 return $matches
}
Test 'abc'
'test' -match "e"
Test 'abc'

#Version 2
function Test($p) {
 $matches=$null 
 $p -match "e"
 return $matches
}
Test 'abc'
'test' -match "e"
Test 'abc'

And these Plaster, PSSA

Maybe $PSBoundParameters could be excluded to allow this type of code and :


# # ==============================================================
# @ID       $Id: GetChildItemExtension.ps1 1291 2012-11-09 19:59:07Z ww $
# @created  2011-07-01
# @project  http://cleancode.sourceforge.net/
# ==============================================================

...
        # 2011.08.04 msorens bug fix: no parameters threw null exception
        if ($PSBoundParameters.Count -eq 0) { $PSBoundParameters = @{} }

        $wrappedCmd = $ExecutionContext.InvokeCommand.GetCommand( `
            'Get-ChildItem', [System.Management.Automation.CommandTypes]::Cmdlet)
        $filters = @("$wrappedCmd @PSBoundParameters")
...
LaurentDardenne commented 6 years ago

Can be considered the following cases ?

#With Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules.dll, ‎3 ‎february 18, ‏‎13:18:28
#https://ci.appveyor.com/project/PowerShell/psscriptanalyzer/build/1.0.2099/job/seht4ms91da3mbss
$sb2={
$T=@('Foo','Foo')
$Error,$?=$T
}
Invoke-ScriptAnalyzer -ScriptDefinition $sb2.tostring()

$sb22={
$T=@('Foo','Foo')
$Ok,$?=$T
}
Invoke-ScriptAnalyzer -ScriptDefinition $sb22.tostring()

$sb3={
 $Error,$?='Foo','Foo'
}
Invoke-ScriptAnalyzer -ScriptDefinition $sb3.tostring()

$sb33={
 $ok,$?='Foo','Foo'
}
Invoke-ScriptAnalyzer -ScriptDefinition $sb33.tostring()

$Texte=@'
 foreach ($Error in 0..5)
 {
   "Error=$Error"
 }
'@
Invoke-ScriptAnalyzer -ScriptDefinition $Texte

$Texte=@'
  $local:Error=10
'@
Invoke-ScriptAnalyzer -ScriptDefinition $Texte

$Texte=@'
 Invoke-Command -ComputerName '.' -ScriptBlock {$pwd=$using:pid}
'@
Invoke-ScriptAnalyzer -ScriptDefinition $Texte

$Texte=@'
 $Sb={$pwd=$using:pid}
 Invoke-Command -ComputerName '.' -ScriptBlock $sb
'@
Invoke-ScriptAnalyzer -ScriptDefinition $Texte
LaurentDardenne commented 6 years ago

Another extreme case :

function Test{ param([ref]$v) $v.value;$v.value=''}
Test ([ref]$PID) #Exception read only
Test ([ref]$pwd)
Test ([ref]$profile)

Although the title indicates 'assignment', does this rule apply to all modifications of builtin variables?