PowerShell / PSScriptAnalyzer

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

PSUseUsingScopeModifierInNewRunspaces has false positive when using -ArgumentList on Invoke-Command #1504

Open jegannathanmaniganadan opened 4 years ago

jegannathanmaniganadan commented 4 years ago

Steps to reproduce

 # $sb = {
Invoke-Command -Session $psSession -ArgumentList $path -ErrorAction Stop -ScriptBlock {
    Param ($Foo)

    return $Foo
}}

 # Invoke-ScriptAnalyzer -ScriptDefinition [scriptblock]$sb | ft -a

RuleName                              Severity ScriptName Line Message
--------                              -------- ---------- ---- -------
PSUseUsingScopeModifierInNewRunspaces Warning             3    The variable '$Foo' is not declared within this ScriptBlock, and is missing the 'Using:' scope modifier.
PSUseUsingScopeModifierInNewRunspaces Warning             5    The variable '$Foo' is not declared within this ScriptBlock, and is missing the 'Using:' scope modifier.

Expected behavior

$Foo should not get flagged

Actual behavior

$Foo is being flagged violating PSUseUsingScopeModifierInNewRunspaces.

Environment data

# $PSVersionTable

Name                           Value
----                           -----
PSVersion                      5.1.18362.752
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.18362.752
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

 # (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }
1.19.0

@mattpwhite

bergmeister commented 4 years ago

I agree this is a false positive. If the ArgumentList parameter is used and there is a Param block, then PSSA should not warn for variables in the Param block. WDYT @Jawz84 ?

For the moment I suggest you either suppress the warning for this specific case

$sb = {
    [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseUsingScopeModifierInNewRunspaces', '', Justification = 'Using ArgumentList')]
    Param()

    Invoke-Command -Session $psSession -ArgumentList $path -ErrorAction Stop -ScriptBlock {
        Param ($Foo)

        return $Foo
    }
}

Invoke-ScriptAnalyzer -ScriptDefinition $sb.ToString()

Or alternatively you could use the $using: pattern as the rule recommends:

$sb = {
    Invoke-Command -Session $psSession -ErrorAction Stop -ScriptBlock {
        return $using:path 
    }
}

Invoke-ScriptAnalyzer -ScriptDefinition $sb.ToString()
Jawz84 commented 4 years ago

Nice find @manigandan-jegannathan-developer. This is a false positive, agreed. @bergmeister showed a work around. I am in the middle of moving/reconstruction /w my new house, so it will be some time before I could start working on this.

Jawz84 commented 4 years ago

I've seen that this problem also arises in this example:

$sb = {Start-ThreadJob -ScriptBlock {param($foo) $foo} -ArgumentList 1|wait-job|receive-job} 
Invoke-ScriptAnalyzer -ScriptDefinition $sb.tostring()

And likewise for Start-Job. ForEach-Parallel does not support the -ArgumentList parameter in the -Parallel parameter set, so the problem does not arise, as the param() block just makes no sense there. It's probably not too hard to fix. Would need to look at the code tho.

jegannathanmaniganadan commented 4 years ago

If the ArgumentList parameter is used and there is a Param block

Or you can find whether the parameter is from Param block of the immediate script block. If so then do not warn. Would not that work ?

rjmholt commented 4 years ago

Rather than look at the invoking command, I would just build a dictionary from the param block of parameters to ignore when visiting a scriptblock

jantari commented 4 years ago

Somewhat related, the following error is also a false positive that I don't understand how it made it into a stable release of PSSA:

Location : ./scripts/windows/Find-ModuleUpdates.ps1 [64, 30]
RuleName : PSUseUsingScopeModifierInNewRunspaces
Severity : Warning
Message  : The variable '$ENV:COMPUTERNAME' is not declared within this ScriptBlock, 
           and is missing the 'Using:' scope modifier.

I don't think there's a problem with using $env: variables on remote systems. If you want to prevent uninitialized/null variables in peoples' scripts then just warn if they're not doing a Set-StrictMode -Version 1.0 (or higher) in the beginning. This makes sure the script exits when it encounters an unset/uninitialized variable.

PS: I customize the output format of PSSA a little bit, but just the Location property, this doesn't affect the Rule processing and messages.

EDIT: After disabling this buggy rule in my tests I instantly had 65 warnings less .......... 🙄

alainQtec commented 2 years ago

This is a false positive, agreed. You can either suppress the warning, use the $using: pattern as the rule recommends, or try this work around :

$sb1 = [ScriptBlock]::Create({
          Param ($Foo)
          return $Foo
    }
)
$sb2 = [ScriptBlock]::Create( { Invoke-Command -Session $psSession -ArgumentList $path -ErrorAction Stop -ScriptBlock $sb1 } )
Invoke-ScriptAnalyzer -ScriptDefinition $sb2 | ft -a
aluty commented 1 year ago

Suppression did not work so worked around:

$results = Invoke-Command -ComputerName $computerNames -ScriptBlock {
    # Avoiding PSScriptAnalyzer Warning PSUseUsingScopeModifierInNewRunspaces : The variable '$env:COMPUTERNAME' is not declared within this ScriptBlock, and is missing the 'Using:' scope modifier.
    $computerName = $env:COMPUTERNAME
    Write-Verbose -Message "Running on $computerName as $(whoami)" -Verbose
iRon7 commented 11 months ago

Aside from the false positive, this rule doesn't return a unique RuleSuppressionID. In fact the RuleSuppressionID is always: PSUseUsingScopeModifierInNewRunspaces where I would like to be specific, like:

[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseUsingScopeModifierInNewRunspaces', 'Foo', Justification = 'Using ArgumentList')]
johnso515 commented 9 months ago

It seems that three years later this still has not been fixed. That is not good