Throw on unbound user-provided scriptblocks (#2551)

* Block unbound scriptblocks as input

* Improve error message for unbound scriptblock input to clarify potential issues

Co-authored-by: Jakub Jareš <[email protected]>

---------

Co-authored-by: Jakub Jareš <[email protected]>

src/Main.ps1

@@ -80,6 +80,8 @@ function Add-ShouldOperator {
         [switch] $SupportsArrayInput
     )
 
+    Assert-BoundScriptBlockInput -ScriptBlock $Test
+
     $entry = [PSCustomObject]@{
         Test               = $Test
         SupportsArrayInput = [bool]$SupportsArrayInput
@@ -1260,6 +1262,8 @@ function BeforeDiscovery {
         [ScriptBlock]$ScriptBlock
     )
 
+    Assert-BoundScriptBlockInput -ScriptBlock $ScriptBlock
+
     if ($ExecutionContext.SessionState.PSVariable.Get('invokedViaInvokePester')) {
         if ($state.CurrentBlock.IsRoot -and -not $state.CurrentBlock.FrameworkData.MissingParametersProcessed) {
             # For undefined parameters in container, add parameter's default value to Data

src/Pester.Runtime.ps1

@@ -2474,6 +2474,10 @@ function New-BlockContainerObject {
         default { throw [System.ArgumentOutOfRangeException]'' }
     }
 
+    if ($item -is [scriptblock]) {
+        Assert-BoundScriptBlockInput -ScriptBlock $item
+    }
+
     $c = [Pester.ContainerInfo]::Create()
     $c.Type = $type
     $c.Item = $item
@@ -2604,3 +2608,20 @@ function Add-MissingContainerParameters ($RootBlock, $Container, $CallingFunctio
 
     $RootBlock.FrameworkData.MissingParametersProcessed = $true
 }
+
+function Assert-BoundScriptBlockInput {
+    param(
+        [Parameter(Mandatory = $true)]
+        [ScriptBlock] $ScriptBlock
+    )
+    $internalSessionState = $script:ScriptBlockSessionStateInternalProperty.GetValue($ScriptBlock, $null)
+    if ($null -eq $internalSessionState) {
+        $maxLength = 250
+        $prettySb = (Format-Nicely2 $ScriptBlock) -replace '\s{2,}', ' '
+        if ($prettySb.Length -gt $maxLength) {
+            $prettySb = "$($prettySb.Remove($maxLength))..."
+        }
+
+        throw [System.ArgumentException]::new("Unbound scriptblock is not allowed, because it would run inside of Pester session state and produce unexpected results. See https://github.com/pester/Pester/issues/2411 for more details and workarounds. ScriptBlock: '$prettySb'")
+    }
+}

src/functions/Context.ps1

@@ -105,6 +105,8 @@
         }
     }
 
+    Assert-BoundScriptBlockInput -ScriptBlock $Fixture
+
     if ($ExecutionContext.SessionState.PSVariable.Get('invokedViaInvokePester')) {
         if ($state.CurrentBlock.IsRoot -and -not $state.CurrentBlock.FrameworkData.MissingParametersProcessed) {
             # For undefined parameters in container, add parameter's default value to Data

src/functions/Describe.ps1

@@ -113,6 +113,8 @@
         }
     }
 
+    Assert-BoundScriptBlockInput -ScriptBlock $Fixture
+
     if ($ExecutionContext.SessionState.PSVariable.Get('invokedViaInvokePester')) {
         if ($state.CurrentBlock.IsRoot -and -not $state.CurrentBlock.FrameworkData.MissingParametersProcessed) {
             # For undefined parameters in container, add parameter's default value to Data

src/functions/It.ps1

@@ -152,6 +152,8 @@
         }
     }
 
+    Assert-BoundScriptBlockInput -ScriptBlock $Test
+
     if ($PSBoundParameters.ContainsKey('ForEach')) {
         if ($null -eq $ForEach -or 0 -eq @($ForEach).Count) {
             if ($PesterPreference.Run.FailOnNullOrEmptyForEach.Value -and -not $AllowNullOrEmptyForEach) {

src/functions/SetupTeardown.ps1

@@ -57,6 +57,7 @@
         $Scriptblock
     )
     Assert-DescribeInProgress -CommandName BeforeEach
+    Assert-BoundScriptBlockInput -ScriptBlock $Scriptblock
 
     New-EachTestSetup -ScriptBlock $Scriptblock
 }
@@ -123,6 +124,7 @@ function AfterEach {
         $Scriptblock
     )
     Assert-DescribeInProgress -CommandName AfterEach
+    Assert-BoundScriptBlockInput -ScriptBlock $Scriptblock
 
     New-EachTestTeardown -ScriptBlock $Scriptblock
 }
@@ -198,6 +200,7 @@ function BeforeAll {
         [Scriptblock]
         $Scriptblock
     )
+    Assert-BoundScriptBlockInput -ScriptBlock $Scriptblock
 
     New-OneTimeTestSetup -ScriptBlock $Scriptblock
 }
@@ -265,6 +268,7 @@ function AfterAll {
         $Scriptblock
     )
     Assert-DescribeInProgress -CommandName AfterAll
+    Assert-BoundScriptBlockInput -ScriptBlock $Scriptblock
 
     New-OneTimeTestTeardown -ScriptBlock $Scriptblock
 }

src/functions/assert/Collection/Should-All.ps1

@@ -45,6 +45,7 @@
         [String]$Because
     )
 
+    Assert-BoundScriptBlockInput -ScriptBlock $FilterScript
 
     $Expected = $FilterScript
     $collectedInput = Collect-Input -ParameterInput $Actual -PipelineInput $local:Input -IsPipelineInput $MyInvocation.ExpectingInput

src/functions/assert/Collection/Should-Any.ps1

@@ -44,6 +44,8 @@
         [String]$Because
     )
 
+    Assert-BoundScriptBlockInput -ScriptBlock $FilterScript
+
     $Expected = $FilterScript
     $collectedInput = Collect-Input -ParameterInput $Actual -PipelineInput $local:Input -IsPipelineInput $MyInvocation.ExpectingInput
     $Actual = $collectedInput.Actual

src/functions/assert/Exception/Should-Throw.ps1

@@ -67,6 +67,8 @@ function Should-Throw {
     $collectedInput = Collect-Input -ParameterInput $ScriptBlock -PipelineInput $local:Input -IsPipelineInput $MyInvocation.ExpectingInput -UnrollInput
     $ScriptBlock = $collectedInput.Actual
 
+    Assert-BoundScriptBlockInput -ScriptBlock $ScriptBlock
+
     $errorThrown = $false
     $err = $null
     try {

tst/Pester.RSpec.ts.ps1

@@ -1668,6 +1668,12 @@ i -PassThru:$PassThru {
                 }
             }
         }
+
+        t "Does not accept unbound scriptblocks" {
+            # Would execute in Pester's internal module state
+            $ex = { New-PesterContainer -ScriptBlock ([ScriptBlock]::Create('$true')) } | Verify-Throw
+            $ex.Exception.Message | Verify-Like 'Unbound scriptblock*'
+        }
     }
 
     b "BeforeDiscovery" {
@@ -1691,6 +1697,15 @@ i -PassThru:$PassThru {
             $r.Containers[0].Blocks[0].Tests[0].Result | Verify-Equal "Passed"
             $r.Containers[0].Blocks[1].Tests[0].Result | Verify-Equal "Passed"
         }
+
+        t "Does not accept unbound scriptblocks" {
+            # Would execute in Pester's internal module state
+            $sb = { BeforeDiscovery ([ScriptBlock]::Create('$true')) }
+            $container = New-PesterContainer -ScriptBlock $sb
+            $r = Invoke-Pester -Container $container -PassThru
+            $r.Containers[0].Result | Verify-Equal 'Failed'
+            $r.Containers[0].ErrorRecord.Exception.Message | Verify-Like 'Unbound scriptblock*'
+        }
     }
 
     b "Parametric tests" {
@@ -2922,4 +2937,33 @@ i -PassThru:$PassThru {
             $pwd.Path | Verify-Equal $beforePWD
         }
     }
+
+    b 'Unbound scriptblocks' {
+        # Would execute in Pester's internal module state
+        t 'Throws when provided to Run.ScriptBlock' {
+            $sb = [scriptblock]::Create('')
+            $conf = New-PesterConfiguration
+            $conf.Run.ScriptBlock = $sb
+            $conf.Run.Throw = $true
+            $conf.Output.CIFormat = 'None'
+
+            $ex = { Invoke-Pester -Configuration $conf } | Verify-Throw
+            $ex.Exception.Message | Verify-Like '*Unbound scriptblock*'
+        }
+
+        t 'Throws when provided to Run.Container' {
+            $c = [Pester.ContainerInfo]::Create()
+            $c.Type = 'ScriptBlock'
+            $c.Item = [scriptblock]::Create('')
+            $c.Data = @{}
+
+            $conf = New-PesterConfiguration
+            $conf.Run.Container = $c
+            $conf.Run.Throw = $true
+            $conf.Output.CIFormat = 'None'
+
+            $ex = { Invoke-Pester -Configuration $conf } | Verify-Throw
+            $ex.Exception.Message | Verify-Like '*Unbound scriptblock*'
+        }
+    }
 }

tst/functions/Add-ShouldOperator.ts.ps1

@@ -70,6 +70,17 @@ i -PassThru:$PassThru {
         }
     }
 
+    b 'Add-ShouldOperator input validation' {
+        Get-Module Pester | Remove-Module
+        Import-Module "$PSScriptRoot\..\..\bin\Pester.psd1"
+
+        t 'Does not allow unbound scriptblocks' {
+            # Would execute in Pester's internal module state
+            $ex = { Add-ShouldOperator -Name DenyUnbound -Test ([ScriptBlock]::Create('$true')) } | Verify-Throw
+            $ex.Exception.Message | Verify-Like 'Unbound scriptblock*'
+        }
+    }
+
     b 'Executing custom Should assertions' {
         # Testing paramter and output syntax described in docs (https://pester.dev/docs/assertions/custom-assertions)
         Get-Module Pester | Remove-Module

tst/functions/Context.Tests.ps1

@@ -7,7 +7,7 @@ Describe 'Testing Context' {
 
                 }
             }
-        } | should -Throw  'Test fixture name has multiple lines and no test fixture is provided. (Have you provided a name for the test group?)'
+        } | Should -Throw  'Test fixture name has multiple lines and no test fixture is provided. (Have you provided a name for the test group?)'
     }
 
     It "Has a name that looks like a script block" {
@@ -17,6 +17,11 @@ Describe 'Testing Context' {
 
                 }
             }
-        } | should -Throw  'No test fixture is provided. (Have you put the open curly brace on the next line?)'
+        } | Should -Throw  'No test fixture is provided. (Have you put the open curly brace on the next line?)'
+    }
+
+    It 'Throws when provided unbound scriptblock' {
+        # Unbound scriptblocks would execute in Pester's internal module state
+        { Context 'c' -Fixture ([scriptblock]::Create('')) } | Should -Throw -ExpectedMessage 'Unbound scriptblock*'
     }
 }

tst/functions/Describe.Tests.ps1

@@ -36,6 +36,11 @@ Describe 'Testing Describe' {
             }
         } | Should -Throw 'Test fixture name has multiple lines and no test fixture is provided. (Have you provided a name for the test group?)'
     }
+
+    It 'Throws when provided unbound scriptblock' {
+        # Unbound scriptblocks would execute in Pester's internal module state
+        { Describe 'd' -Fixture ([scriptblock]::Create('')) } | Should -Throw -ExpectedMessage 'Unbound scriptblock*'
+    }
 }
 
 

tst/functions/It.Tests.ps1

@@ -0,0 +1,24 @@
+Set-StrictMode -Version Latest
+
+Describe 'Testing It' {
+    It 'Throws when missing name' {
+        { It {
+
+            'something'
+            }
+        } | Should -Throw -ExpectedMessage 'Test name has multiple lines and no test scriptblock is provided*'
+    }
+
+    It 'Throws when missing scriptblock' {
+        { It 'runs a test'
+            {
+                # This scriptblock is a new statement as scriptblock didn't start on It-line nor used a backtick
+            }
+        } | Should -Throw -ExpectedMessage 'No test scriptblock is provided*'
+    }
+
+    It 'Throws when provided unbound scriptblock' {
+        # Unbound scriptblocks would execute in Pester's internal module state
+        { It 'i' -Test ([scriptblock]::Create('')) } | Should -Throw -ExpectedMessage 'Unbound scriptblock*'
+    }
+}

tst/functions/Mock.Tests.ps1

@@ -2366,15 +2366,25 @@ Describe 'Naming conflicts in mocked functions' {
 }
 
 Describe 'Passing unbound script blocks as mocks' {
-    It 'Does not produce an error' {
+    BeforeAll {
         function TestMe {
             'Original'
         }
+    }
+    It 'Does not produce an error' {
         $scriptBlock = [scriptblock]::Create('"Mocked"')
 
         { Mock TestMe $scriptBlock } | Should -Not -Throw
         TestMe | Should -Be Mocked
     }
+
+    It 'Should not execute in Pester internal state' {
+        $filter = [scriptblock]::Create('if ("pester" -eq $ExecutionContext.SessionState.Module) { throw "executed parameter filter in internal state" } else { $true }')
+        $scriptBlock = [scriptblock]::Create('if ("pester" -eq $ExecutionContext.SessionState.Module) { throw "executed mock in internal state" } else { "Mocked" }')
+
+        { Mock -CommandName TestMe -ParameterFilter $filter -MockWith $scriptBlock } | Should -Not -Throw
+        TestMe -SomeParam | Should -Be Mocked
+    }
 }
 
 Describe 'Should -Invoke when mock called outside of It block' {

tst/functions/SetupTeardown.Tests.ps1

@@ -190,6 +190,25 @@ Describe 'Finishing TestGroup Setup and Teardown tests' {
     }
 }
 
+Describe 'Unbound scriptsblocks as input' {
+    # Unbound scriptblocks would execute in Pester's internal module state
+    BeforeAll {
+        $sb = [scriptblock]::Create('')
+        $expectedMessage = 'Unbound scriptblock*'
+    }
+    It 'Throws when provided to BeforeAll' {
+        { BeforeAll -Scriptblock $sb } | Should -Throw -ExpectedMessage $expectedMessage
+    }
+    It 'Throws when provided to AfterAll' {
+        { AfterAll -Scriptblock $sb } | Should -Throw -ExpectedMessage $expectedMessage
+    }
+    It 'Throws when provided to BeforeEach' {
+        { BeforeEach -Scriptblock $sb } | Should -Throw -ExpectedMessage $expectedMessage
+    }
+    It 'Throws when provided to AfterEach' {
+        { AfterEach -Scriptblock $sb } | Should -Throw -ExpectedMessage $expectedMessage
+    }
+}
 
 # if ($PSVersionTable.PSVersion.Major -ge 3) {
 #     # TODO: this depends on the old pester internals it would be easier to test in P

tst/functions/assert/Collection/Should-All.Tests.ps1

@@ -57,4 +57,10 @@ Expected [int] 2, but got [int] 1." -replace "`r`n", "`n")
     It 'It fails when the only item not matching the filter is 0' {
         { 0 | Should-All -FilterScript { $_ -gt 0 } } | Verify-AssertionFailed
     }
+
+    It 'Throws when provided unbound scriptblock' {
+        # Unbound scriptblocks would execute in Pester's internal module state
+        $ex = { 1 | Should-All ([scriptblock]::Create('')) } | Verify-Throw
+        $ex.Exception.Message | Verify-Like 'Unbound scriptblock*'
+    }
 }

tst/functions/assert/Collection/Should-Any.Tests.ps1

@@ -61,4 +61,10 @@ Expected [int] 2, but got [int] 1." -replace "`r`n", "`n")
     It "Accepts FilterScript and Actual by position" {
         Should-Any { $true } 1, 2
     }
+
+    It 'Throws when provided unbound scriptblock' {
+        # Unbound scriptblocks would execute in Pester's internal module state
+        $ex = { 1 | Should-Any ([scriptblock]::Create('')) } | Verify-Throw
+        $ex.Exception.Message | Verify-Like 'Unbound scriptblock*'
+    }
 }

tst/functions/assert/Exception/Should-Throw.Tests.ps1

@@ -10,7 +10,6 @@ Describe "Should-Throw" {
     }
 
     It "Passes when non-terminating exception is thrown" {
-
         { Write-Error "fail!" } | Should-Throw
     }
 
@@ -23,6 +22,12 @@ Describe "Should-Throw" {
             Should-Throw 'MockErrorMessage' 'MockErrorId' ([Microsoft.PowerShell.Commands.WriteErrorException]) 'MockBecauseString'
     }
 
+    It 'Throws when provided unbound scriptblock' {
+        # Unbound scriptblocks would execute in Pester's internal module state
+        $ex = { ([scriptblock]::Create('')) | Should-Throw } | Verify-Throw
+        $ex.Exception.Message | Verify-Like 'Unbound scriptblock*'
+    }
+
     Context "Filtering with exception type" {
         It "Passes when exception has the expected type" {
             { throw [ArgumentException]"A is null!" } | Should-Throw -ExceptionType ([ArgumentException])