Badgerati / Pode

Pode is a Cross-Platform PowerShell web framework for creating REST APIs, Web Sites, and TCP/SMTP servers
https://badgerati.github.io/Pode
MIT License
845 stars 92 forks source link

Remove Global Variables $TimerEvent and $WebEvent #1324

Closed mdaneri closed 4 months ago

mdaneri commented 4 months ago

Description of the Change

This request aims to remove the global variables $TimerEvent and $WebEvent from the codebase to enhance security. These variables will be passed to the runspaces using System.Management.Automation.Runspaces.SessionStateVariableEntry.

Background:

Global variables can lead to several issues, including namespace pollution, difficulty in tracking modifications, unintended side effects, and security risks. To address these concerns, we are refactoring the code to avoid the use of global variables.

Changes:

  1. Removed the global definitions of $TimerEvent and $WebEvent.
  2. Passed these variables to the runspaces using System.Management.Automation.Runspaces.SessionStateVariableEntry.

Code changes

Before

function Start-PodeAzFuncServer {
    param(
        [Parameter(Mandatory = $true)]
        $Data
    )
    ....
    # reset event data
    $global:WebEvent = @{
        OnEnd            = @()
        Auth             = @{}
        Response         = $response
        Request          = $request
        Lockable         = $PodeContext.Threading.Lockables.Global
        Path             = [string]::Empty
        Method           = $request.Method.ToLowerInvariant()
        Query            = $request.Query
        Endpoint         = @{
            Protocol = ($request.Url -split '://')[0]
            Address  = $null
            Name     = $null
        }
        ContentType      = $null
        ErrorType        = $null
        Cookies          = @{}
        PendingCookies   = @{}
        Parameters       = $null
        Data             = $null
        Files            = $null
        Streamed         = $false
        Route            = $null
        StaticContent    = $null
        Timestamp        = [datetime]::UtcNow
        TransferEncoding = $null
        AcceptEncoding   = $null
        Ranges           = $null
        Metadata         = @{}
    }
}

function New-PodeRunspaceState {
    # create the state, and add the pode modules
    $state = [initialsessionstate]::CreateDefault()
    $state.ImportPSModule($PodeContext.Server.PodeModule.DataPath)
    $state.ImportPSModule($PodeContext.Server.PodeModule.InternalPath)

    # load the vars into the share state
    $session = New-PodeStateContext -Context $PodeContext

    $variables = @(
        (New-Object System.Management.Automation.Runspaces.SessionStateVariableEntry -ArgumentList 'PodeContext', $session, $null),
        (New-Object System.Management.Automation.Runspaces.SessionStateVariableEntry -ArgumentList 'Console', $Host, $null),
        (New-Object System.Management.Automation.Runspaces.SessionStateVariableEntry -ArgumentList 'PODE_SCOPE_RUNSPACE', $true, $null)
    )

    foreach ($var in $variables) {
        $state.Variables.Add($var)
    }

    $PodeContext.RunspaceState = $state
}

After

function Start-PodeAzFuncServer {
    param(
        [Parameter(Mandatory = $true)]
        $Data
    )
    ....
    # reset event data
    $WebEvent += @{
        OnEnd            = @()
        Auth             = @{}
        Response         = $response
        Request          = $request
        Lockable         = $PodeContext.Threading.Lockables.Global
        Path             = [string]::Empty
        Method           = $request.Method.ToLowerInvariant()
        Query            = $request.Query
        Endpoint         = @{
            Protocol = ($request.Url -split '://')[0]
            Address  = $null
            Name     = $null
        }
        ContentType      = $null
        ErrorType        = $null
        Cookies          = @{}
        PendingCookies   = @{}
        Parameters       = $null
        Data             = $null
        Files            = $null
        Streamed         = $false
        Route            = $null
        StaticContent    = $null
        Timestamp        = [datetime]::UtcNow
        TransferEncoding = $null
        AcceptEncoding   = $null
        Ranges           = $null
        Metadata         = @{}
    }
}

function New-PodeRunspaceState {
    # create the state, and add the pode modules
    $state = [initialsessionstate]::CreateDefault()
    $state.ImportPSModule($PodeContext.Server.PodeModule.DataPath)
    $state.ImportPSModule($PodeContext.Server.PodeModule.InternalPath)

    # load the vars into the share state
    $session = New-PodeStateContext -Context $PodeContext

    $variables = @(
        (New-Object System.Management.Automation.Runspaces.SessionStateVariableEntry -ArgumentList 'PodeContext', $session, $null),
        (New-Object System.Management.Automation.Runspaces.SessionStateVariableEntry -ArgumentList 'Console', $Host, $null),
        (New-Object System.Management.Automation.Runspaces.SessionStateVariableEntry -ArgumentList 'TimerEvent', $TimerEvent, $null),
        (New-Object System.Management.Automation.Runspaces.SessionStateVariableEntry -ArgumentList 'WebEvent', $WebEvent, $null),
        (New-Object System.Management.Automation.Runspaces.SessionStateVariableEntry -ArgumentList 'PODE_SCOPE_RUNSPACE', $true, $null)
    )

    foreach ($var in $variables) {
        $state.Variables.Add($var)
    }

    $PodeContext.RunspaceState = $state
}
Badgerati commented 4 months ago

Ah, I thought this is what you might have been referring to from #1295.

This actually doesn't resolve this particular scoping issue with $TimerEvent and $WebEvent. For the serverless $WebEvent the child runspace pools are never actually created by Pode 😉 everything is invoked at the parent scope, which must mean that AzFunc/Lambda invokes PowerShell scripts themselves as another child runspace - which would explain why $global: is needed.

For $TimerEvent I'm still baffled however, unless $global: is used even the state variable trick doesn't make the variable accessible (results below from the timers.ps1 example):

Setting $TimerEvent at the runspace pool level also exposes a variable hijacking issue; if you were to add a custom property to $TimerEvent in a timer scriptblock, then it would still be accessible in other timers, more so using +=.

The only way to really remove them properly, specifically for TimerEvent and for serverless the WebEvent, would be to pass them as explicit parameters to the timer/route scriptblock - but to do this now would be a hefty breaking change, for timers specifically, as it would change their initialisation to need param($TimerEvent) in the scriptblock. Another thought I've had is to move timers into their own separate runspace pool - similar to schedules -see what happens then.

mdaneri commented 4 months ago

You are right. I ran some tests, and it doesn't work the way I expected :( I'm closing this Pull request, and I am going to open a bug. Because primarily from a security point of view, global variables shouldn't be used in this kind of product.