Suggestion - Code Refactor (Untested)

[ZanattaMichael]

https://github.com/itm4n/PrivescCheck/blob/a7b04e6ffc1bb44c9de5ed1fcc53a0db686354f3/PrivescCheck.ps1#L550

function Test-IsKnownService {

    [CmdletBinding()] param(
        [object]$Service
    )

    if (-not($Service)) { return $false }

    $ImagePath = $Service.ImagePath
    $SeparationCharacterSets = @('"', "'", ' ', "`"'", '" ', "' ", "`"' ")

    ForEach($SeparationCharacterSet in $SeparationCharacterSets) {

        $CandidatePaths = $ImagePath.Split($SeparationCharacterSet) | Where-Object {-not([String]::IsNullOrWhiteSpace($_)) }

        ForEach($CandidatePath in $CandidatePaths) {

            $TempPath = $([System.Environment]::ExpandEnvironmentVariables($CandidatePath))

            if (-not(Test-Path -LiteralPath $TempPath)) { continue }

            $File = Get-Item -LiteralPath $TempPath -ErrorAction SilentlyContinue -ErrorVariable ErrorGetItem 

            if ($File.VersionInfo.LegalCopyright -Like "*Microsoft Corporation*") { return $True }
            elseif ($null -eq $File) { continue }
            else { return $False }

        }
    }
}

[itm4n]

Thanks but can you provide more details please? What issue does it solve? What improvement does it bring? It's not obvious at first glance.

[ZanattaMichael]

Its' not solving any issues. It's a refactor to make the code more readable / maintainable. Hence makes it more testable.

[itm4n]

Ok I get it, but I hoped you would have explained why your changes are better. I'm not a "PowerShell SME" like you. 🙂 Anyway, there are definitely some good things such as -not([String]::IsNullOrWhiteSpace($_) to apply throughout the entire script for instance.

The only thing I'm not entirely sure about is Test-Path because this one does not handle errors. So you have to call it within a try / catch block as far as I know. 🤔

[ZanattaMichael]

No worries. The intention here to reduce the number of nested conditions/ loops, making the code easier to read. Easier code means more testable code. More testable code, means less work. Less work means more time doing other things.

For instance, if you have a condition that is uses as a dependency inside the function, you can invert the condition (making the function implicitly true). So the condition goes from if -eq $true { # Do Somthing }, to if -eq $false { # return to parent }. Using this approach in PowerShell makes the code a lot easier to write tests for, since all you need to test is the dependencies at the beginning of the function, prior to the execution of the code.

You are correct. Test-Path can throw non-/terminating errors. However you can still use Test-Path to test file directories by overwriting the Error Action Preference on the cmdlet to SilentlyContinue. This will still have the same effect.

[itm4n]

Thanks for your explanations.

Yeah, I know about -ErrorAction SilentlyContinue but in the case of Test-Path, it does not work, hence the need of a try / catch block. As far as I remember... I might be wrong.

I leave this issue open for now. The entire code needs to be refactored anyway. I will need to review it completely at some point.

[itm4n]

I refactored the entire code. Still have plenty more work to do.