Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need rule which catches assignments to powershell builtin variables #858

Open
JamesWTruher opened this issue Jan 26, 2018 · 7 comments
Open

Comments

@JamesWTruher
Copy link
Contributor

JamesWTruher commented Jan 26, 2018

assignments to variables like $args and $_, etc should be caught.
see #265

@bergmeister
Copy link
Collaborator

bergmeister commented Jan 28, 2018

@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
Copy link
Collaborator

bergmeister commented Jan 30, 2018

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
Copy link

Your implementation does not seem to consider these cases:

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

@bergmeister
Copy link
Collaborator

bergmeister commented Jan 31, 2018

@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
Copy link

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
Copy link

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
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants