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

Throw on unbound user-provided scriptblocks #2551

Merged
merged 2 commits into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/Main.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ function Add-ShouldOperator {
[switch] $SupportsArrayInput
)

Assert-BoundScriptBlockInput -ScriptBlock $Test

$entry = [PSCustomObject]@{
Test = $Test
SupportsArrayInput = [bool]$SupportsArrayInput
Expand Down Expand Up @@ -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
Expand Down
21 changes: 21 additions & 0 deletions src/Pester.Runtime.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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'")
}
}
2 changes: 2 additions & 0 deletions src/functions/Context.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions src/functions/Describe.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions src/functions/It.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 4 additions & 0 deletions src/functions/SetupTeardown.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
$Scriptblock
)
Assert-DescribeInProgress -CommandName BeforeEach
Assert-BoundScriptBlockInput -ScriptBlock $Scriptblock

New-EachTestSetup -ScriptBlock $Scriptblock
}
Expand Down Expand Up @@ -123,6 +124,7 @@ function AfterEach {
$Scriptblock
)
Assert-DescribeInProgress -CommandName AfterEach
Assert-BoundScriptBlockInput -ScriptBlock $Scriptblock

New-EachTestTeardown -ScriptBlock $Scriptblock
}
Expand Down Expand Up @@ -198,6 +200,7 @@ function BeforeAll {
[Scriptblock]
$Scriptblock
)
Assert-BoundScriptBlockInput -ScriptBlock $Scriptblock

New-OneTimeTestSetup -ScriptBlock $Scriptblock
}
Expand Down Expand Up @@ -265,6 +268,7 @@ function AfterAll {
$Scriptblock
)
Assert-DescribeInProgress -CommandName AfterAll
Assert-BoundScriptBlockInput -ScriptBlock $Scriptblock

New-OneTimeTestTeardown -ScriptBlock $Scriptblock
}
1 change: 1 addition & 0 deletions src/functions/assert/Collection/Should-All.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
[String]$Because
)

Assert-BoundScriptBlockInput -ScriptBlock $FilterScript

$Expected = $FilterScript
$collectedInput = Collect-Input -ParameterInput $Actual -PipelineInput $local:Input -IsPipelineInput $MyInvocation.ExpectingInput
Expand Down
2 changes: 2 additions & 0 deletions src/functions/assert/Collection/Should-Any.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions src/functions/assert/Exception/Should-Throw.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
44 changes: 44 additions & 0 deletions tst/Pester.RSpec.ts.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand All @@ -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" {
Expand Down Expand Up @@ -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*'
}
}
}
11 changes: 11 additions & 0 deletions tst/functions/Add-ShouldOperator.ts.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 7 additions & 2 deletions tst/functions/Context.Tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand All @@ -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*'
}
}
5 changes: 5 additions & 0 deletions tst/functions/Describe.Tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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*'
}
}


Expand Down
24 changes: 24 additions & 0 deletions tst/functions/It.Tests.ps1
Original file line number Diff line number Diff line change
@@ -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*'
}
}
12 changes: 11 additions & 1 deletion tst/functions/Mock.Tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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' {
Expand Down
19 changes: 19 additions & 0 deletions tst/functions/SetupTeardown.Tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions tst/functions/assert/Collection/Should-All.Tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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*'
}
}
6 changes: 6 additions & 0 deletions tst/functions/assert/Collection/Should-Any.Tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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*'
}
}
7 changes: 6 additions & 1 deletion tst/functions/assert/Exception/Should-Throw.Tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ Describe "Should-Throw" {
}

It "Passes when non-terminating exception is thrown" {

{ Write-Error "fail!" } | Should-Throw
}

Expand All @@ -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])
Expand Down