Couple of advices

[PrzemyslawKlys]

Hi,

I've been going thru your source code and thought you could use couple of advices:

• Remove Write-Output - it has no use in PS 5.1 except for write-output -NoEnumerate. Equivalent is to use just a string without anything

It would look like this:

function Find-LogonScriptCredentials {
    [CmdletBinding()]
    param(
        [Parameter(Mandatory = $true)]
        [array]$LogonScripts
    )
    foreach ($script in $LogonScripts) {
        Write-Verbose -Message "Checking $($Script.FullName) for credentials.."
        $Credentials = Get-Content -Path $script.FullName | Select-String -Pattern "/user:" -AllMatches
        if ($Credentials) {
            "`n[!] CREDENTIALS FOUND!"
            "- File: $($script.FullName)"
            $Credentials | ForEach-Object {
                "`t- Credential: $_"
            }
            ""
        }
    } 
}

However maybe you meant to display it on screen then you should use Write-Host. Using Write-Output displays to screen and as an object which may not be something people expect.

Consider this:

function Test {
    [cmdletBinding()]
    param(

    )

    Write-Output "Test"
    Write-Output "Test2"
}

function Test1 {
    [cmdletBinding()]
    param(

    )

    "Test"
    "Test2"
}

function TestWrite {
    [cmdletBinding()]
    param(

    )

    Write-Host "Test"
    Write-Host "Test2"
}


$TestOutput = Test
$TestOutput
$TestOutput.Count

$TestOutput = Test1
$TestOutput
$TestOutput.Count

$TestOutput = TestWrite
$TestOutput
$TestOutput.Count

I am not sure if it would be useful to get a bunch of strings from a command. If anything you can always use Write-Host to display to console, and then use [PSCustomObject] to actually return data to user that is "eatable"

function TestWrite {
    [cmdletBinding()]
    param(

    )

    Write-Host "Test"
    Write-Host "Test2"

    [PSCustomObject] @{
        Test  = "Test"
        Test2 = "Test2"
    }
}

If however your intention was as is - sorry :-) Just ignore me

The other one:

function Find-UNCScripts {
    [CmdletBinding()]
    param(
        [Parameter(Mandatory = $true)]
        [array]$LogonScripts
    )

    $UNCFiles = @()
    foreach ($script in $LogonScripts) {
        $UNCFiles += Get-Content $script.FullName | Select-String -Pattern '\\.*\.\w+' | foreach {$_.Matches.Value}
    }
    Write-Verbose "[+] UNC scripts:"
    $UNCFiles | ForEach-Object {
        Write-Verbose -Message "$_"
    }
    return $UNCFiles
}

You can simplify it:

$UNCFiles = foreach ($script in $LogonScripts) {
    Get-Content $script.FullName | Select-String -Pattern '\\.*\.\w+' | ForEach-Object { $_.Matches.Value }
}

It's better, more readable and faster. You could also do:

[Array] $UNCFiles = foreach ($script in $LogonScripts) {
    Get-Content $script.FullName | Select-String -Pattern '\\.*\.\w+' | ForEach-Object { $_.Matches.Value }
}

This one makes sure that regardless if you have one entry in array or 0 or 5 the 'Count' always works.

When you use += on @() you're rebuilding array every time. For small runs it doesn't matter but if you loop long enough it will slow down considerably.

Finally you don't have to use 'return XYZ' if it's in the end of the function. Just use $UncFiles and it will return it as an object. return XYZ makes most sense if it's in the middle somewhere.

Have fun :) And sorry for interupting.

[techspence]

@PrzemyslawKlys I really appreciate you taking the time to look at my ugly code. :) Thank you for the advice and recommended changes. I've implemented all of them (729b7b7). I do plan to make the output a bit nicer in the form of an object as well as some other enhancements. Once again, thank you!