🐛 fix(scripts): resolve follow-up PowerShell audit findings#16
Merged
Conversation
Fix invalid PowerShell syntax, broken remoting flow, runtime break usage, placeholder-driven failures, and plaintext credential export defaults found in the repository audit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve remaining PR review feedback for pipeline output, unused variables, progress preference restoration, preserved error records, DNS lookup API usage, SID comments, and PSSession cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| BestPractice | 1 minor |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull request overview
This PR continues the repository-wide PowerShell audit cleanup by removing invalid/unsafe patterns (interactive remoting, Invoke-Expression, break misuse), tightening error handling, and improving parameter validation to reduce runtime failures and security risk.
Changes:
- Replaces unsafe/invalid constructs (e.g.,
Invoke-Expression, interactiveEnter-PSSession,breakin non-loop contexts) with safer alternatives and correct control flow. - Improves function/script robustness via parameter validation, structured error handling (
throw,try/finally), and session cleanup. - Adds/updates output shaping and security-oriented options (e.g., optional plaintext password export with warnings, LAPS password redaction by default).
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Windows/Push-DNSClientServerAddresses.ps1 | Makes PSSession cleanup more resilient by suppressing session-removal errors. |
| Windows/New-gMSA.ps1 | Adds comment-based help + validation; removes hardcoded domain/OU paths by parameterizing them. |
| Windows/Get-SharesAndPermissions.ps1 | Refactors CIM calls to support optional credential use and emits structured permission objects. |
| Windows/Activate and Get License.ps1 | Replaces Invoke-Expression with direct invocation and validates product key format. |
| Profile and Prompt/PSProfileBase.ps1 | Avoids executing Oh My Posh generated code; uses built-in prompt logic instead. |
| Microsoft 365/Convert-HybridGroupToCloud.ps1 | Replaces non-terminating Write-Error + break with throw and fixes a variable reference. |
| Exchange/Rename-RecipientByCsv.ps1 | Fixes PSBoundParameters.ContainsKey usage and makes CSV-load failure terminating. |
| Exchange/Parse-TransportLogs.ps1 | Replaces obsolete DNS API call with GetHostEntry/HostName. |
| Entra/New-EntraUserFromCSV.ps1 | Adds opt-in plaintext password export behavior with warnings; adjusts result schema accordingly. |
| DDI/Remove-DhcpAllLeases.ps1 | Fixes invalid break by returning early on abort. |
| DDI/Get Hostnames from CSV IP Addresses.ps1 | Improves error reporting by passing the full ErrorRecord to Write-Error. |
| Active Directory/Set DNS Server Zone Settings via Registry.ps1 | Removes interactive remoting usage; executes remote work via Invoke-Command with cleanup. |
| Active Directory/Get-LAPSPasswords.ps1 | Converts one-liner into a function with safer default output (redacted password unless requested). |
| Active Directory/Get-ADUserTransitiveGroupMembership.ps1 | Ensures ProgressPreference is restored via try/finally around connectivity checks. |
| Active Directory/Get-ADObjectFromPipeline.ps1 | Removes unused state and outputs resolved objects directly from the process block. |
| Active Directory/Export-AllADUserTransitiveGroupMemberships.ps1 | Restores ProgressPreference via try/finally around GC connectivity checks. |
| Active Directory/Domain Services/DNSZonesRemote.ps1 | Replaces interactive remoting with Invoke-Command and adds session cleanup in finally. |
| Active Directory/AD Users/Get-LockedOutLocation.ps1 | Converts module import failure from break to a terminating error (throw). |
| Active Directory/AD Users/Get-ADDirectReport.ps1 | Fixes examples/recursive function name and adjusts verbose error handling. |
| Active Directory/AD Groups/Archive-ObsoleteGroups.ps1 | Moves AD module import to after parameter parsing. |
Comment on lines
+191
to
+197
| function Prompt { | ||
| $LastCommand = Get-History -Count 1 -ErrorAction SilentlyContinue | ||
| Write-Host "$AdminStatus " -ForegroundColor "$AdminStatusColor" -NoNewline | ||
| Write-Host "[$($LastCommand.Id +1)] $([math]::Ceiling($LastCommand.Duration.TotalMilliseconds))ms " -NoNewline -ForegroundColor White | ||
| Write-Host "$FolderGlyph $($PWD.ToString() -ireplace [regex]::Escape($HOME),'~')" -ForegroundColor Yellow | ||
| Write-Host '>' -NoNewline -ForegroundColor White | ||
| return ' ' |
Comment on lines
40
to
+50
| param ( | ||
| [Parameter(Mandatory=$true)][string]$gMSA, | ||
| [Parameter(Mandatory=$true)][array]$Servers, | ||
| [Parameter(Mandatory=$true)][System.Management.Automation.PSCredential]$Credential | ||
| [Parameter(Mandatory=$true)] | ||
| [ValidateLength(1,15)] | ||
| [string]$gMSA, | ||
|
|
||
| [Parameter(Mandatory=$true)] | ||
| [ValidateNotNullOrEmpty()] | ||
| [string[]]$Servers, | ||
|
|
||
| [Parameter(Mandatory=$true)] | ||
| [System.Management.Automation.PSCredential]$Credential, |
| } | ||
| } | ||
| } Catch { | ||
| Write-Host "Failed to enter the PSSession for $server. Skipping." -ForegroundColor DarkYellow |
| } | ||
| } | ||
| } Catch { | ||
| Write-Host -ForegroundColor DarkYellow "Failed to enter the PSSession for $server. Skipping." |
Comment on lines
71
to
+74
| # Output the current object with the properties Name, SamAccountName, Mail and Manager | ||
| Get-ADUser -Identity $PSItem -Properties mail, manager, DistinguishedName | Select-Object -Property Name, SamAccountName, DistinguishedName, Mail, @{ Name = 'Manager'; Expression = { (Get-ADUser -Identity $psitem.manager).samaccountname } } | Where-Object { $_.DistinguishedName -like '*,OU=Employees,OU=People,DC=DOMAINNAME,DC=org' } | ||
| Get-ADUser -Identity $PSItem -Properties mail, manager, DistinguishedName | Select-Object -Property Name, SamAccountName, DistinguishedName, Mail, @{ Name = 'Manager'; Expression = { (Get-ADUser -Identity $psitem.manager).samaccountname } } | ||
| # Gather DirectReports under the current object and so on... | ||
| Get-ADDirectReports -Identity $PSItem -Recurse | ||
| Get-ADDirectReport -Identity $PSItem -Recurse |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation