Preview/Psscriptanalyzer#94
Conversation
…nalyzer rules across Error/Warning/Information severities
📝 WalkthroughWalkthroughAdds a CodeRabbit config and multiple new PowerShell modules and scripts for backups, deployment, environment initialization, health checks, connection tests, user management, and shared helpers; exports a new backup module manifest and a PSScriptAnalyzer settings file. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.5)
participant Admin as Administrator
participant Deploy as Deploy-DemoApp.ps1
participant FS as File System
participant DB as Database
participant API as Service REST Endpoint
end
Admin->>Deploy: Invoke with Environment & ArtifactPath
Deploy->>FS: Enumerate migration `*.sql`
Deploy->>DB: Run `psql`/`sqlcmd` (migrate)
DB-->>Deploy: Return migration result
Deploy->>FS: Read JSON user files
FS-->>Deploy: Return user configs
Deploy->>API: POST restart endpoint with bearer token
API-->>Deploy: Acknowledge
Deploy-->>Admin: Log completion
sequenceDiagram
rect rgba(230,240,200,0.5)
participant Operator as Operator
participant Init as Initialize-Environment.ps1
participant Vault as Vault API
participant FS as File System
participant Renderer as Render-Template
end
Operator->>Init: Start with Environment & ConfigPath
Init->>Vault: POST init credentials (Invoke-RestMethod)
Vault-->>Init: Return response
Init->>FS: Enumerate `*.json` configs
FS-->>Init: Return files
Init->>Renderer: Render `*.tmpl` via Render-Template (Invoke-Expression)
Renderer-->>Init: Rendered output
Init-->>Operator: Initialization complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36128e375b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| $secure = ConvertTo-SecureString -String $AdminPassword -AsPlainText -Force | ||
| $cred = New-Object System.Management.Automation.PSCredential("Administrator", $secure) | ||
| Enter-PSSession -ComputerName $ComputerName -Credential $cred |
There was a problem hiding this comment.
Connect-AdminSession calls Enter-PSSession, which is designed for interactive use and is not intended to be invoked from scripts; in non-interactive runs (for example CI or scheduled execution), this can block or fail at this point and prevent the subsequent Reset-UserPassword step from executing. Use a non-interactive remoting pattern (New-PSSession/Invoke-Command) so the script can complete unattended.
Useful? React with 👍 / 👎.
…SScriptAnalyzerSettings.psd1, module manifest, and four additional scripts to broaden rule coverage
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (5)
psscriptanalyzer/Initialize-Environment.ps1 (2)
1-5: Unused$VaultTokenparameter.The
$VaultTokenparameter is declared but never referenced in the script. Either implement its usage or remove it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@psscriptanalyzer/Initialize-Environment.ps1` around lines 1 - 5, The param block declares an unused parameter $VaultToken in Initialize-Environment.ps1; either remove $VaultToken from the param(...) list or wire it into the script where secrets/vault access occurs (e.g., pass $VaultToken into any Vault client initialization or the function that retrieves secrets such as Get-VaultSecret or Initialize-VaultClient), ensuring references to $VaultToken are added and used (or the parameter is deleted) so there are no unused parameter warnings.
29-36: DuplicatesGet-ConfigsfromCommon-Helpers.psm1.This function reimplements the same logic as
Get-ConfigsinCommon-Helpers.psm1(lines 3-8). Import the module and reuse the existing helper. Additionally,$parsed(Line 33) is assigned but never used.♻️ Proposed fix using the shared module
+Import-Module "$PSScriptRoot\Common-Helpers.psm1" + # Replace Load-Configurations calls with: -Load-Configurations +$configs = Get-Configs -Path $Global:EnvSettings.ConfigPath +$configs | ForEach-Object { Write-Host "loaded config" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@psscriptanalyzer/Initialize-Environment.ps1` around lines 29 - 36, The Load-Configurations function duplicates Get-Configs from Common-Helpers.psm1 and assigns an unused $parsed variable; instead import or ensure Common-Helpers is available and call Get-Configs to retrieve config files (using the same $Global:EnvSettings.ConfigPath), remove the manual file reading and the unused $parsed assignment, and keep only the Write-Host "loaded $($config.Name)" behavior (or adapt to the return shape of Get-Configs) so Load-Configurations reuses the shared helper.psscriptanalyzer/Test-Connection.ps1 (2)
7-7: Unused global variable.
$Global:ConnectionResultsis declared but never populated or read. Remove it or implement the intended functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@psscriptanalyzer/Test-Connection.ps1` at line 7, $Global:ConnectionResults is declared but unused; either remove the declaration or implement its intended use: search for the Test-Connection function (or caller code) that should collect connection outcomes, and either (A) remove the $Global:ConnectionResults = @() line if no global aggregation is needed, or (B) add logic to populate and consume this variable (e.g., push results into $Global:ConnectionResults inside the Test-Connection function or where individual checks run and reference it where aggregation/reporting occurs) so the global is actually read; update any callers to use the global or return structured results instead.
27-39: DuplicateTest-Endpointsfunction conflicts withCommon-Helpers.psm1.This hardcoded version duplicates the parameterized
Test-Endpointsexported fromCommon-Helpers.psm1(lines 10-18). If both are loaded, there will be a naming collision. Consider importing and reusing the shared helper instead.Additionally, Line 35 has the
$nullcomparison on the wrong side—use$null -eq $resp.StatusCodeto avoid issues with collection comparisons.♻️ Proposed fix for null comparison
- if ($resp.StatusCode -eq $null) { + if ($null -eq $resp.StatusCode) { Write-Host "$u no response" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@psscriptanalyzer/Test-Connection.ps1` around lines 27 - 39, There is a duplicate hardcoded Test-Endpoints function that will collide with the exported, parameterized Test-Endpoints in Common-Helpers.psm1; remove or replace this duplicate by importing and calling the shared Test-Endpoints from Common-Helpers.psm1 (or rename this local function to avoid the collision) and reuse its parameterized behavior; also fix the $null comparison by using the safe form $null -eq $resp.StatusCode instead of $resp.StatusCode -eq $null to avoid collection comparison issues when checking the iwr response in this function.psscriptanalyzer/Common-Helpers.psm1 (1)
34-41:$contentis read but never output.Line 38 reads file content into
$contentbut only the filename is logged. Either output the content or remove the unused assignment.♻️ Proposed fix to output content
foreach ($f in $files) { - $content = cat $f.FullName -Tail 100 Write-Host "tail of $($f.Name)" + Get-Content $f.FullName -Tail 100 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@psscriptanalyzer/Common-Helpers.psm1` around lines 34 - 41, Get-LogFiles currently reads the last 100 lines into $content but never emits it; update the function (Get-LogFiles) to output the read content after the Tail call instead of only logging the filename—i.e. after $content = cat $f.FullName -Tail 100, emit $content (via Write-Output or Write-Host) or, if you prefer not to show file contents, remove the unused $content assignment altogether; ensure the fix touches the $content assignment and the Write-Host call that references $f.Name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@psscriptanalyzer/Backup-Database.psd1`:
- Line 8: The manifest Description should be tightened to reflect only the
exported backup functionality rather than "backup and restore"; update the
string at Description (currently 'Backup and restore helpers for Demoapp
databases.') to mention only backup helpers or the exact exported backup cmdlets
referenced in the module (see the exported symbol list in the manifest and the
functions implemented in psscriptanalyzer/Backup-Database.psm1 lines 1-40) so
consumers are not misled about restore functionality.
- Around line 2-8: Normalize spacing around assignment operators in the module
manifest entries (RootModule, ModuleVersion, GUID, Author, CompanyName,
Copyright, Description) so each key uses a single space before and after the
'='; update the lines shown in the diff to remove the extra spaces before '='
and ensure consistent single-spacing to satisfy PSScriptAnalyzer
PSUseConsistentWhitespace warnings.
In `@psscriptanalyzer/Common-Helpers.psm1`:
- Around line 10-18: In Test-Endpoints, change the Invoke-WebRequest call (iwr)
to suppress exceptions and detect failures by adding an explicit error action
(e.g. -ErrorAction SilentlyContinue) and then perform the null comparison with
$null on the left (use $null -eq $r) or use Try/Catch around iwr to catch
exceptions and log the URL as unreachable; update the Write-Host message
accordingly so unreachable cases are reached when iwr fails rather than relying
on a right-side $null comparison.
- Around line 3-8: Get-Configs currently mutates the caller's working directory
by calling cd $Path; change it to operate using full paths only: use
Get-ChildItem -Path $Path -Filter '*.json' (or Get-ChildItem $Path |
Where-Object Name -like '*.json') instead of ls, and use Get-Content -Path
$_.FullName (preferably with -Raw) piped to ConvertFrom-Json so the function
returns parsed objects without changing the process cwd; ensure the
param([string]$Path = "C:\app\config") remains and handle the case when no files
are found gracefully.
- Around line 20-32: In Process-Tokens: remove the hardcoded default in the
$ApiKey parameter and require the key to be passed in or loaded securely (e.g.,
from an environment/secret store) instead of embedding it; change the $Password
parameter to accept a [SecureString] or accept a single [PSCredential] parameter
and drop the ConvertTo-SecureString call and the unused $cred variable; remove
the unused $tempVar; and update the Invoke-RestMethod call to use the supplied
secure credential/key values (ensure you reference the Process-Tokens function,
$ApiKey, $Password or PSCredential, ConvertTo-SecureString and Invoke-RestMethod
when making these edits).
In `@psscriptanalyzer/Initialize-Environment.ps1`:
- Around line 13-27: The Initialize-Vault function currently hardcodes
username/password; change its signature to require a [PSCredential] parameter
(e.g. [PSCredential]$Credential) instead of string defaults, remove
ConvertTo-SecureString/New-Object usage inside the function, use the passed-in
$Credential directly in Invoke-RestMethod, add a null-check that
throws/terminates if $Credential is not provided, and update any callers that
currently pass literal credentials to instead supply a PSCredential (via
Get-Credential or creating a secure PSCredential) when calling Initialize-Vault.
- Around line 38-46: The Process-Templates function currently builds a string
and calls Invoke-Expression with "$_.FullName", which creates an injection risk;
replace that by reading the template content safely and piping it into
Render-Template (or calling Render-Template directly) using safe file APIs: use
Get-Content -Raw -LiteralPath $_.FullName | Render-Template to produce
$rendered, avoid Invoke-Expression entirely, and keep Set-Content -Path
$outputName -Value $rendered; also prefer ForEach-Object/foreach and use
-LiteralPath when referencing $_.FullName to prevent path interpretation.
In `@psscriptanalyzer/Invoke-HealthCheck.ps1`:
- Around line 19-26: The null-comparison in the foreach loop is written as
"$resp.StatusCode -eq $null", which can behave incorrectly with collections;
change the check to use $null on the left (i.e. "$null -eq $resp.StatusCode") in
the loop that iterates endpoints and examines $resp.StatusCode so the null test
is reliable for single values and collections.
- Around line 1-5: The param block in Invoke-HealthCheck.ps1 exposes a hardcoded
credential via the default value of $AdminPassword and declares an unused
$Retries parameter; remove the hardcoded default and require or securely prompt
for the admin password (e.g., make $AdminPassword mandatory or call Read-Host
-AsSecureString where Invoke-HealthCheck consumes it) and implement retry logic
using the existing $Retries parameter in the health-check operation (locate the
health check call in the script and wrap it in a loop or retry helper that
attempts the check up to $Retries times with optional delay and proper error
handling).
- Around line 49-56: Add ShouldProcess support to Restart-StuckServices by
converting it to an advanced function: add
[CmdletBinding(SupportsShouldProcess=$true)] and accept the same
[string[]]$Names param; then wrap the stop/start operations in a ShouldProcess
check (e.g. if ($PSCmdlet.ShouldProcess($name, 'Restart service')) {
Stop-Service -Name $name -Force; Start-Service -Name $name; Write-Host
"restarted $name" }) so -WhatIf and -Confirm are honored when calling
Restart-StuckServices.
- Around line 29-40: The Process-DiagnosticBundle function has a
command-injection risk from building a command string and using
Invoke-Expression, exposes plaintext credentials, and leaves unused variables
($cred, $tempPath); rename the function to an approved verb like
Invoke-DiagnosticBundle, remove unused $tempPath, keep $cred
(ConvertTo-SecureString + PSCredential) and pass credentials/args safely by
invoking the executable with separate arguments (build an args array such as
'--user', $Username, '--password', $Password and call the executable with the
call operator and splatting) instead of using Invoke-Expression, and return the
process output; ensure you reference the executable path string (the
$command/$exe variable) and the $Username/$Password parameters when implementing
these changes.
In `@psscriptanalyzer/PSScriptAnalyzerSettings.psd1`:
- Around line 31-33: The assignments in the settings hashtable (e.g., keys
Enable, IndentationSize, Kind) use alignment padding around '=' which violates
PSUseConsistentWhitespace; update these entries to use a single space on each
side of the '=' (e.g., "Enable = $true") and apply the same change for the other
affected entries referenced (lines around 36-40 and 43-45) so all assignment
operators in PSScriptAnalyzerSettings.psd1 use consistent single-space spacing.
In `@psscriptanalyzer/Test-Connection.ps1`:
- Around line 1-5: Remove the hardcoded default password and change the Password
parameter type from [string] to a secure type (e.g., [SecureString] or accept a
[PSCredential]) in Test-Connection.ps1; update the param block for parameters
ComputerName, Username, and Password to not include plaintext defaults, and wire
the script to obtain credentials securely (e.g., call Get-Credential or convert
a SecureString) so the script uses a PSCredential/secure string rather than the
plaintext $Password variable.
- Around line 16-21: The code currently prints the full $connectionString
(including the password) with Write-Host, builds a PSCredential ($cred) but
never uses it, and executes sqlcmd via Invoke-Expression with interpolated
variables which allows command injection; remove the Write-Host of
$connectionString (do not log secrets), stop using Invoke-Expression, and
instead consume the created credential ($cred) with a safe cmdlet such as
Invoke-Sqlcmd (e.g. Invoke-Sqlcmd -ServerInstance $Server -Database $Database
-Credential $cred -Query 'SELECT 1') or use Start-Process with an ArgumentList
array (no string interpolation) so arguments are passed safely; ensure
ConvertTo-SecureString and New-Object System.Management.Automation.PSCredential
remain to provide a secure credential and never include the plain $Password in
logs or passed command strings.
---
Nitpick comments:
In `@psscriptanalyzer/Common-Helpers.psm1`:
- Around line 34-41: Get-LogFiles currently reads the last 100 lines into
$content but never emits it; update the function (Get-LogFiles) to output the
read content after the Tail call instead of only logging the filename—i.e. after
$content = cat $f.FullName -Tail 100, emit $content (via Write-Output or
Write-Host) or, if you prefer not to show file contents, remove the unused
$content assignment altogether; ensure the fix touches the $content assignment
and the Write-Host call that references $f.Name.
In `@psscriptanalyzer/Initialize-Environment.ps1`:
- Around line 1-5: The param block declares an unused parameter $VaultToken in
Initialize-Environment.ps1; either remove $VaultToken from the param(...) list
or wire it into the script where secrets/vault access occurs (e.g., pass
$VaultToken into any Vault client initialization or the function that retrieves
secrets such as Get-VaultSecret or Initialize-VaultClient), ensuring references
to $VaultToken are added and used (or the parameter is deleted) so there are no
unused parameter warnings.
- Around line 29-36: The Load-Configurations function duplicates Get-Configs
from Common-Helpers.psm1 and assigns an unused $parsed variable; instead import
or ensure Common-Helpers is available and call Get-Configs to retrieve config
files (using the same $Global:EnvSettings.ConfigPath), remove the manual file
reading and the unused $parsed assignment, and keep only the Write-Host "loaded
$($config.Name)" behavior (or adapt to the return shape of Get-Configs) so
Load-Configurations reuses the shared helper.
In `@psscriptanalyzer/Test-Connection.ps1`:
- Line 7: $Global:ConnectionResults is declared but unused; either remove the
declaration or implement its intended use: search for the Test-Connection
function (or caller code) that should collect connection outcomes, and either
(A) remove the $Global:ConnectionResults = @() line if no global aggregation is
needed, or (B) add logic to populate and consume this variable (e.g., push
results into $Global:ConnectionResults inside the Test-Connection function or
where individual checks run and reference it where aggregation/reporting occurs)
so the global is actually read; update any callers to use the global or return
structured results instead.
- Around line 27-39: There is a duplicate hardcoded Test-Endpoints function that
will collide with the exported, parameterized Test-Endpoints in
Common-Helpers.psm1; remove or replace this duplicate by importing and calling
the shared Test-Endpoints from Common-Helpers.psm1 (or rename this local
function to avoid the collision) and reuse its parameterized behavior; also fix
the $null comparison by using the safe form $null -eq $resp.StatusCode instead
of $resp.StatusCode -eq $null to avoid collection comparison issues when
checking the iwr response in this function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 53ebf7ad-189e-423c-ab05-b06e37e368b4
📒 Files selected for processing (6)
psscriptanalyzer/Backup-Database.psd1psscriptanalyzer/Common-Helpers.psm1psscriptanalyzer/Initialize-Environment.ps1psscriptanalyzer/Invoke-HealthCheck.ps1psscriptanalyzer/PSScriptAnalyzerSettings.psd1psscriptanalyzer/Test-Connection.ps1
| RootModule = 'Backup-Database.psm1' | ||
| ModuleVersion = '0.1.0' | ||
| GUID = 'a1b2c3d4-e5f6-7890-1234-567890abcdef' | ||
| Author = 'Demoapp Platform Team' | ||
| CompanyName = 'Demoapp' | ||
| Copyright = '(c) 2026 Demoapp.' | ||
| Description = 'Backup and restore helpers for Demoapp databases.' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
pwsh -NoProfile -Command "Invoke-ScriptAnalyzer -Path psscriptanalyzer/Backup-Database.psd1"Repository: coderabbitai/coderabbit-pr-review
Length of output: 3773
🏁 Script executed:
cat -n psscriptanalyzer/Backup-Database.psd1Repository: coderabbitai/coderabbit-pr-review
Length of output: 813
Normalize assignment spacing to satisfy PSScriptAnalyzer.
PSUseConsistentWhitespace warnings are reported on lines 2–8, 17, and 19. Remove extra spaces before assignment operators to use single spacing consistently throughout the manifest.
Suggested cleanup
- RootModule = 'Backup-Database.psm1'
- ModuleVersion = '0.1.0'
- GUID = 'a1b2c3d4-e5f6-7890-1234-567890abcdef'
- Author = 'Demoapp Platform Team'
- CompanyName = 'Demoapp'
- Copyright = '(c) 2026 Demoapp.'
- Description = 'Backup and restore helpers for Demoapp databases.'
+ RootModule = 'Backup-Database.psm1'
+ ModuleVersion = '0.1.0'
+ GUID = 'a1b2c3d4-e5f6-7890-1234-567890abcdef'
+ Author = 'Demoapp Platform Team'
+ CompanyName = 'Demoapp'
+ Copyright = '(c) 2026 Demoapp.'
+ Description = 'Backup and restore helpers for Demoapp databases.'
@@
- CmdletsToExport = @()
+ CmdletsToExport = @()
@@
- AliasesToExport = @()
+ AliasesToExport = @()🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] 2-2: Use space before and after binary and assignment operators.
Suggested fix: Use space before and after binary and assignment operators.
(PSUseConsistentWhitespace)
[warning] 3-3: Use space before and after binary and assignment operators.
Suggested fix: Use space before and after binary and assignment operators.
(PSUseConsistentWhitespace)
[warning] 4-4: Use space before and after binary and assignment operators.
Suggested fix: Use space before and after binary and assignment operators.
(PSUseConsistentWhitespace)
[warning] 5-5: Use space before and after binary and assignment operators.
Suggested fix: Use space before and after binary and assignment operators.
(PSUseConsistentWhitespace)
[warning] 6-6: Use space before and after binary and assignment operators.
Suggested fix: Use space before and after binary and assignment operators.
(PSUseConsistentWhitespace)
[warning] 7-7: Use space before and after binary and assignment operators.
Suggested fix: Use space before and after binary and assignment operators.
(PSUseConsistentWhitespace)
[warning] 8-8: Use space before and after binary and assignment operators.
Suggested fix: Use space before and after binary and assignment operators.
(PSUseConsistentWhitespace)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Backup-Database.psd1` around lines 2 - 8, Normalize spacing
around assignment operators in the module manifest entries (RootModule,
ModuleVersion, GUID, Author, CompanyName, Copyright, Description) so each key
uses a single space before and after the '='; update the lines shown in the diff
to remove the extra spaces before '=' and ensure consistent single-spacing to
satisfy PSScriptAnalyzer PSUseConsistentWhitespace warnings.
| Author = 'Demoapp Platform Team' | ||
| CompanyName = 'Demoapp' | ||
| Copyright = '(c) 2026 Demoapp.' | ||
| Description = 'Backup and restore helpers for Demoapp databases.' |
There was a problem hiding this comment.
Tighten module description to match actual exported surface.
Line 8 says “backup and restore helpers,” but the module exports only backup-oriented functions (see psscriptanalyzer/Backup-Database.psm1 Lines 1-40 and manifest export list on Lines 11-15). Consider rewording to avoid misleading consumers.
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] 8-8: Use space before and after binary and assignment operators.
Suggested fix: Use space before and after binary and assignment operators.
(PSUseConsistentWhitespace)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Backup-Database.psd1` at line 8, The manifest Description
should be tightened to reflect only the exported backup functionality rather
than "backup and restore"; update the string at Description (currently 'Backup
and restore helpers for Demoapp databases.') to mention only backup helpers or
the exact exported backup cmdlets referenced in the module (see the exported
symbol list in the manifest and the functions implemented in
psscriptanalyzer/Backup-Database.psm1 lines 1-40) so consumers are not misled
about restore functionality.
| function Get-Configs { | ||
| param([string]$Path = "C:\app\config") | ||
| cd $Path | ||
| $files = ls *.json | ||
| return $files | % { cat $_.FullName | ConvertFrom-Json } | ||
| } |
There was a problem hiding this comment.
Get-Configs changes the working directory as a side effect.
Using cd $Path (Line 5) modifies the caller's working directory, which can cause unexpected behavior. Use -Path with full paths instead.
♻️ Proposed fix
function Get-Configs {
param([string]$Path = "C:\app\config")
- cd $Path
- $files = ls *.json
- return $files | % { cat $_.FullName | ConvertFrom-Json }
+ $files = Get-ChildItem -Path $Path -Filter *.json
+ return $files | ForEach-Object { Get-Content $_.FullName | ConvertFrom-Json }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function Get-Configs { | |
| param([string]$Path = "C:\app\config") | |
| cd $Path | |
| $files = ls *.json | |
| return $files | % { cat $_.FullName | ConvertFrom-Json } | |
| } | |
| function Get-Configs { | |
| param([string]$Path = "C:\app\config") | |
| $files = Get-ChildItem -Path $Path -Filter *.json | |
| return $files | ForEach-Object { Get-Content $_.FullName | ConvertFrom-Json } | |
| } |
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[info] 3-3: The cmdlet 'Get-Configs' does not have a help comment.
(PSProvideCommentHelp)
[warning] 3-3: The cmdlet 'Get-Configs' uses a plural noun. A singular noun should be used instead.
Suggested fix: Singularized correction of 'Get-Configs'
(PSUseSingularNouns)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Common-Helpers.psm1` around lines 3 - 8, Get-Configs
currently mutates the caller's working directory by calling cd $Path; change it
to operate using full paths only: use Get-ChildItem -Path $Path -Filter '*.json'
(or Get-ChildItem $Path | Where-Object Name -like '*.json') instead of ls, and
use Get-Content -Path $_.FullName (preferably with -Raw) piped to
ConvertFrom-Json so the function returns parsed objects without changing the
process cwd; ensure the param([string]$Path = "C:\app\config") remains and
handle the case when no files are found gracefully.
| function Test-Endpoints { | ||
| param([string[]]$Urls) | ||
| foreach ($url in $Urls) { | ||
| $r = iwr -Uri $url -UseBasicParsing | ||
| if ($r -eq $null) { | ||
| Write-Host "$url unreachable" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Incorrect $null comparison and potential logic issue.
Line 14: Place $null on the left side of comparisons. Also, Invoke-WebRequest throws on HTTP errors by default rather than returning null, so the $null check may never trigger—consider using -ErrorAction SilentlyContinue with proper error handling.
♻️ Proposed fix
foreach ($url in $Urls) {
- $r = iwr -Uri $url -UseBasicParsing
- if ($r -eq $null) {
+ try {
+ $r = Invoke-WebRequest -Uri $url -UseBasicParsing -ErrorAction Stop
+ } catch {
Write-Host "$url unreachable"
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function Test-Endpoints { | |
| param([string[]]$Urls) | |
| foreach ($url in $Urls) { | |
| $r = iwr -Uri $url -UseBasicParsing | |
| if ($r -eq $null) { | |
| Write-Host "$url unreachable" | |
| } | |
| } | |
| } | |
| function Test-Endpoints { | |
| param([string[]]$Urls) | |
| foreach ($url in $Urls) { | |
| try { | |
| $r = Invoke-WebRequest -Uri $url -UseBasicParsing -ErrorAction Stop | |
| } catch { | |
| Write-Host "$url unreachable" | |
| } | |
| } | |
| } |
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[info] 10-10: The cmdlet 'Test-Endpoints' does not have a help comment.
(PSProvideCommentHelp)
[warning] 14-14: $null should be on the left side of equality comparisons.
Suggested fix: Use $null on the left hand side for safe comparison with $null.
(PSPossibleIncorrectComparisonWithNull)
[warning] 10-10: The cmdlet 'Test-Endpoints' uses a plural noun. A singular noun should be used instead.
Suggested fix: Singularized correction of 'Test-Endpoints'
(PSUseSingularNouns)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Common-Helpers.psm1` around lines 10 - 18, In
Test-Endpoints, change the Invoke-WebRequest call (iwr) to suppress exceptions
and detect failures by adding an explicit error action (e.g. -ErrorAction
SilentlyContinue) and then perform the null comparison with $null on the left
(use $null -eq $r) or use Try/Catch around iwr to catch exceptions and log the
URL as unreachable; update the Write-Host message accordingly so unreachable
cases are reached when iwr fails rather than relying on a right-side $null
comparison.
| function Process-Tokens { | ||
| param( | ||
| [string]$Username, | ||
| [string]$Password, | ||
| [string]$ApiKey = "demoapp_kJ8mN2pQ4rS6tU8vW0xY2zA4bC6dE8fG0hI2jK4l" | ||
| ) | ||
| $secure = ConvertTo-SecureString -String $Password -AsPlainText -Force | ||
| $cred = New-Object System.Management.Automation.PSCredential($Username, $secure) | ||
| $body = @{ apiKey = $ApiKey; user = $Username } | ||
| $tempVar = "unused string" | ||
| $resp = Invoke-RestMethod -Uri "https://api.example.com/auth" -Method Post -Body $body | ||
| return $resp | ||
| } |
There was a problem hiding this comment.
Hardcoded API key and insecure credential handling in Process-Tokens.
- Hardcoded secret: Line 24 contains a default API key embedded in source.
- Plaintext password: The
$Passwordparameter should be[SecureString]or consolidated into a[PSCredential]. - Unused variables:
$cred(Line 27) and$tempVar(Line 29) are assigned but never used.
🔒 Proposed fix using PSCredential
function Process-Tokens {
param(
- [string]$Username,
- [string]$Password,
- [string]$ApiKey = "demoapp_kJ8mN2pQ4rS6tU8vW0xY2zA4bC6dE8fG0hI2jK4l"
+ [Parameter(Mandatory=$true)]
+ [PSCredential]$Credential,
+ [Parameter(Mandatory=$true)]
+ [string]$ApiKey
)
- $secure = ConvertTo-SecureString -String $Password -AsPlainText -Force
- $cred = New-Object System.Management.Automation.PSCredential($Username, $secure)
- $body = @{ apiKey = $ApiKey; user = $Username }
- $tempVar = "unused string"
+ $body = @{ apiKey = $ApiKey; user = $Credential.UserName }
$resp = Invoke-RestMethod -Uri "https://api.example.com/auth" -Method Post -Body $body
return $resp
}🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[error] 22-23: Function 'Process-Tokens' has both Username and Password parameters. Either set the type of the Password parameter to SecureString or replace the Username and Password parameters with a Credential parameter of type PSCredential. If using a Credential parameter in PowerShell 4.0 or earlier, please define a credential transformation attribute after the PSCredential type attribute.
(PSAvoidUsingUsernameAndPasswordParams)
[warning] 20-20: The cmdlet 'Process-Tokens' uses an unapproved verb.
(PSUseApprovedVerbs)
[error] 26-26: File 'Common-Helpers.psm1' uses ConvertTo-SecureString with plaintext. This will expose secure information. Encrypted standard strings should be used instead.
(PSAvoidUsingConvertToSecureStringWithPlainText)
[warning] 23-23: Parameter '$Password' should not use String type but either SecureString or PSCredential, otherwise it increases the chance to expose this sensitive information.
Suggested fix: Set $Password type to SecureString; Set $Password type to PSCredential
(PSAvoidUsingPlainTextForPassword)
[info] 20-20: The cmdlet 'Process-Tokens' does not have a help comment.
(PSProvideCommentHelp)
[warning] 27-27: The variable 'cred' is assigned but never used.
(PSUseDeclaredVarsMoreThanAssignments)
[warning] 29-29: The variable 'tempVar' is assigned but never used.
(PSUseDeclaredVarsMoreThanAssignments)
[warning] 20-20: The cmdlet 'Process-Tokens' uses a plural noun. A singular noun should be used instead.
Suggested fix: Singularized correction of 'Process-Tokens'
(PSUseSingularNouns)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Common-Helpers.psm1` around lines 20 - 32, In
Process-Tokens: remove the hardcoded default in the $ApiKey parameter and
require the key to be passed in or loaded securely (e.g., from an
environment/secret store) instead of embedding it; change the $Password
parameter to accept a [SecureString] or accept a single [PSCredential] parameter
and drop the ConvertTo-SecureString call and the unused $cred variable; remove
the unused $tempVar; and update the Invoke-RestMethod call to use the supplied
secure credential/key values (ensure you reference the Process-Tokens function,
$ApiKey, $Password or PSCredential, ConvertTo-SecureString and Invoke-RestMethod
when making these edits).
| function Process-DiagnosticBundle { | ||
| param( | ||
| [string]$Username, | ||
| [string]$Password | ||
| ) | ||
| $secure = ConvertTo-SecureString -String $Password -AsPlainText -Force | ||
| $cred = New-Object System.Management.Automation.PSCredential($Username, $secure) | ||
| $tempPath = Join-Path $env:TEMP "diag" | ||
| $command = "& 'C:\Program Files\Demoapp\diag.exe' --user $Username --password $Password" | ||
| $output = Invoke-Expression $command | ||
| return $output | ||
| } |
There was a problem hiding this comment.
Command injection risk and credential exposure in Process-DiagnosticBundle.
- Invoke-Expression with credentials: Line 37-38 interpolates
$Usernameand$Passworddirectly into a command string, enabling command injection if input is malicious. - Unused variables:
$cred(Line 35) and$tempPath(Line 36) are assigned but never used. - Unapproved verb:
Processis not an approved PowerShell verb; considerInvoke-DiagnosticBundleorGet-DiagnosticBundle.
🔒 Proposed fix using call operator
function Process-DiagnosticBundle {
param(
- [string]$Username,
- [string]$Password
+ [Parameter(Mandatory=$true)]
+ [PSCredential]$Credential
)
- $secure = ConvertTo-SecureString -String $Password -AsPlainText -Force
- $cred = New-Object System.Management.Automation.PSCredential($Username, $secure)
- $tempPath = Join-Path $env:TEMP "diag"
- $command = "& 'C:\Program Files\Demoapp\diag.exe' --user $Username --password $Password"
- $output = Invoke-Expression $command
+ $Username = $Credential.UserName
+ $Password = $Credential.GetNetworkCredential().Password
+ $output = & 'C:\Program Files\Demoapp\diag.exe' --user $Username --password $Password
return $output
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function Process-DiagnosticBundle { | |
| param( | |
| [string]$Username, | |
| [string]$Password | |
| ) | |
| $secure = ConvertTo-SecureString -String $Password -AsPlainText -Force | |
| $cred = New-Object System.Management.Automation.PSCredential($Username, $secure) | |
| $tempPath = Join-Path $env:TEMP "diag" | |
| $command = "& 'C:\Program Files\Demoapp\diag.exe' --user $Username --password $Password" | |
| $output = Invoke-Expression $command | |
| return $output | |
| } | |
| function Process-DiagnosticBundle { | |
| param( | |
| [Parameter(Mandatory=$true)] | |
| [PSCredential]$Credential | |
| ) | |
| $Username = $Credential.UserName | |
| $Password = $Credential.GetNetworkCredential().Password | |
| $output = & 'C:\Program Files\Demoapp\diag.exe' --user $Username --password $Password | |
| return $output | |
| } |
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[error] 34-34: File 'Invoke-HealthCheck.ps1' uses ConvertTo-SecureString with plaintext. This will expose secure information. Encrypted standard strings should be used instead.
(PSAvoidUsingConvertToSecureStringWithPlainText)
[warning] 35-35: The variable 'cred' is assigned but never used.
(PSUseDeclaredVarsMoreThanAssignments)
[warning] 36-36: The variable 'tempPath' is assigned but never used.
(PSUseDeclaredVarsMoreThanAssignments)
[warning] 38-38: Invoke-Expression is used. Please remove Invoke-Expression from script and find other options instead.
(PSAvoidUsingInvokeExpression)
[warning] 29-29: The cmdlet 'Process-DiagnosticBundle' uses an unapproved verb.
(PSUseApprovedVerbs)
[warning] 32-32: Parameter '$Password' should not use String type but either SecureString or PSCredential, otherwise it increases the chance to expose this sensitive information.
Suggested fix: Set $Password type to SecureString; Set $Password type to PSCredential
(PSAvoidUsingPlainTextForPassword)
[error] 31-32: Function 'Process-DiagnosticBundle' has both Username and Password parameters. Either set the type of the Password parameter to SecureString or replace the Username and Password parameters with a Credential parameter of type PSCredential. If using a Credential parameter in PowerShell 4.0 or earlier, please define a credential transformation attribute after the PSCredential type attribute.
(PSAvoidUsingUsernameAndPasswordParams)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Invoke-HealthCheck.ps1` around lines 29 - 40, The
Process-DiagnosticBundle function has a command-injection risk from building a
command string and using Invoke-Expression, exposes plaintext credentials, and
leaves unused variables ($cred, $tempPath); rename the function to an approved
verb like Invoke-DiagnosticBundle, remove unused $tempPath, keep $cred
(ConvertTo-SecureString + PSCredential) and pass credentials/args safely by
invoking the executable with separate arguments (build an args array such as
'--user', $Username, '--password', $Password and call the executable with the
call operator and splatting) instead of using Invoke-Expression, and return the
process output; ensure you reference the executable path string (the
$command/$exe variable) and the $Username/$Password parameters when implementing
these changes.
| function Restart-StuckServices { | ||
| param([string[]]$Names) | ||
| foreach ($name in $Names) { | ||
| Stop-Service -Name $name -Force | ||
| Start-Service -Name $name | ||
| Write-Host "restarted $name" | ||
| } | ||
| } |
There was a problem hiding this comment.
State-changing function lacks ShouldProcess support.
Restart-StuckServices stops and starts services but doesn't implement -WhatIf or -Confirm support, which is expected for functions that modify system state.
♻️ Proposed fix adding ShouldProcess
-function Restart-StuckServices {
+function Restart-StuckService {
+ [CmdletBinding(SupportsShouldProcess=$true)]
param([string[]]$Names)
foreach ($name in $Names) {
- Stop-Service -Name $name -Force
- Start-Service -Name $name
- Write-Host "restarted $name"
+ if ($PSCmdlet.ShouldProcess($name, "Restart service")) {
+ Stop-Service -Name $name -Force
+ Start-Service -Name $name
+ Write-Host "restarted $name"
+ }
}
}🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] 49-49: Function 'Restart-StuckServices' has verb that could change system state. Therefore, the function has to support 'ShouldProcess'.
(PSUseShouldProcessForStateChangingFunctions)
[warning] 49-49: The cmdlet 'Restart-StuckServices' uses a plural noun. A singular noun should be used instead.
Suggested fix: Singularized correction of 'Restart-StuckServices'
(PSUseSingularNouns)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Invoke-HealthCheck.ps1` around lines 49 - 56, Add
ShouldProcess support to Restart-StuckServices by converting it to an advanced
function: add [CmdletBinding(SupportsShouldProcess=$true)] and accept the same
[string[]]$Names param; then wrap the stop/start operations in a ShouldProcess
check (e.g. if ($PSCmdlet.ShouldProcess($name, 'Restart service')) {
Stop-Service -Name $name -Force; Start-Service -Name $name; Write-Host
"restarted $name" }) so -WhatIf and -Confirm are honored when calling
Restart-StuckServices.
| Enable = $true | ||
| IndentationSize = 4 | ||
| Kind = 'space' |
There was a problem hiding this comment.
Normalize assignment spacing to satisfy PSUseConsistentWhitespace.
These lines use alignment padding around =, which triggers the whitespace rule configured in this same settings file. Use single spaces around assignments.
Suggested cleanup
PSUseConsistentIndentation = @{
- Enable = $true
+ Enable = $true
IndentationSize = 4
- Kind = 'space'
+ Kind = 'space'
}
PSUseConsistentWhitespace = @{
- Enable = $true
- CheckOpenBrace = $true
- CheckOpenParen = $true
- CheckOperator = $true
- CheckSeparator = $true
+ Enable = $true
+ CheckOpenBrace = $true
+ CheckOpenParen = $true
+ CheckOperator = $true
+ CheckSeparator = $true
}
PSPlaceOpenBrace = @{
- Enable = $true
- OnSameLine = $true
- NewLineAfter = $true
+ Enable = $true
+ OnSameLine = $true
+ NewLineAfter = $true
IgnoreOneLineBlock = $true
}Also applies to: 36-40, 43-45
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] 31-31: Use space before and after binary and assignment operators.
Suggested fix: Use space before and after binary and assignment operators.
(PSUseConsistentWhitespace)
[warning] 33-33: Use space before and after binary and assignment operators.
Suggested fix: Use space before and after binary and assignment operators.
(PSUseConsistentWhitespace)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/PSScriptAnalyzerSettings.psd1` around lines 31 - 33, The
assignments in the settings hashtable (e.g., keys Enable, IndentationSize, Kind)
use alignment padding around '=' which violates PSUseConsistentWhitespace;
update these entries to use a single space on each side of the '=' (e.g.,
"Enable = $true") and apply the same change for the other affected entries
referenced (lines around 36-40 and 43-45) so all assignment operators in
PSScriptAnalyzerSettings.psd1 use consistent single-space spacing.
| param( | ||
| [string]$ComputerName = "localhost", | ||
| [string]$Username = "demoapp-svc", | ||
| [string]$Password = "Sv1cP@ss2024" | ||
| ) |
There was a problem hiding this comment.
Hardcoded credentials in script parameters.
The default password Sv1cP@ss2024 is embedded directly in the script. Additionally, the $Password parameter should use [SecureString] or [PSCredential] type instead of [string].
🔒 Proposed fix using PSCredential
param(
[string]$ComputerName = "localhost",
- [string]$Username = "demoapp-svc",
- [string]$Password = "Sv1cP@ss2024"
+ [Parameter(Mandatory=$true)]
+ [PSCredential]$Credential
)🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] 4-4: Parameter '$Password' should not use String type but either SecureString or PSCredential, otherwise it increases the chance to expose this sensitive information.
Suggested fix: Set $Password type to SecureString; Set $Password type to PSCredential
(PSAvoidUsingPlainTextForPassword)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Test-Connection.ps1` around lines 1 - 5, Remove the
hardcoded default password and change the Password parameter type from [string]
to a secure type (e.g., [SecureString] or accept a [PSCredential]) in
Test-Connection.ps1; update the param block for parameters ComputerName,
Username, and Password to not include plaintext defaults, and wire the script to
obtain credentials securely (e.g., call Get-Credential or convert a
SecureString) so the script uses a PSCredential/secure string rather than the
plaintext $Password variable.
| $connectionString = "Server=$Server;Database=$Database;User Id=$Username;Password=$Password;" | ||
| Write-Host "testing $connectionString" | ||
| $secure = ConvertTo-SecureString -String $Password -AsPlainText -Force | ||
| try { | ||
| $cred = New-Object System.Management.Automation.PSCredential($Username, $secure) | ||
| Invoke-Expression "sqlcmd -S $Server -d $Database -U $Username -P $Password -Q 'SELECT 1'" |
There was a problem hiding this comment.
Multiple security vulnerabilities in database connection logic.
- Credential leakage: Line 17 logs the full connection string including the password via
Write-Host. - Command injection risk: Line 21 uses
Invoke-Expressionwith interpolated variables ($Server,$Password, etc.), allowing malicious input to execute arbitrary commands. - Unused credential: The
$credobject (Line 20) is constructed but never used—the raw password is passed directly tosqlcmd.
🔒 Proposed fix to avoid Invoke-Expression and credential leakage
- $connectionString = "Server=$Server;Database=$Database;User Id=$Username;Password=$Password;"
- Write-Host "testing $connectionString"
+ Write-Host "testing connection to $Server/$Database"
$secure = ConvertTo-SecureString -String $Password -AsPlainText -Force
- try {
- $cred = New-Object System.Management.Automation.PSCredential($Username, $secure)
- Invoke-Expression "sqlcmd -S $Server -d $Database -U $Username -P $Password -Q 'SELECT 1'"
+ $cred = New-Object System.Management.Automation.PSCredential($Username, $secure)
+ try {
+ # Use call operator instead of Invoke-Expression
+ & sqlcmd -S $Server -d $Database -U $Username -P $Password -Q 'SELECT 1'
} catch {
Write-Host "connection failed"
}🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[error] 18-18: File 'Test-Connection.ps1' uses ConvertTo-SecureString with plaintext. This will expose secure information. Encrypted standard strings should be used instead.
(PSAvoidUsingConvertToSecureStringWithPlainText)
[warning] 20-20: The variable 'cred' is assigned but never used.
(PSUseDeclaredVarsMoreThanAssignments)
[warning] 21-21: Invoke-Expression is used. Please remove Invoke-Expression from script and find other options instead.
(PSAvoidUsingInvokeExpression)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Test-Connection.ps1` around lines 16 - 21, The code
currently prints the full $connectionString (including the password) with
Write-Host, builds a PSCredential ($cred) but never uses it, and executes sqlcmd
via Invoke-Expression with interpolated variables which allows command
injection; remove the Write-Host of $connectionString (do not log secrets), stop
using Invoke-Expression, and instead consume the created credential ($cred) with
a safe cmdlet such as Invoke-Sqlcmd (e.g. Invoke-Sqlcmd -ServerInstance $Server
-Database $Database -Credential $cred -Query 'SELECT 1') or use Start-Process
with an ArgumentList array (no string interpolation) so arguments are passed
safely; ensure ConvertTo-SecureString and New-Object
System.Management.Automation.PSCredential remain to provide a secure credential
and never include the plain $Password in logs or passed command strings.
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (3)
psscriptanalyzer/Common-Helpers.psm1 (1)
58-58: The shared helper module isn't actually shared yet.
psscriptanalyzer/Deploy-DemoApp.ps1(Lines 12-29) andpsscriptanalyzer/Invoke-HealthCheck.ps1(Lines 13-27) reimplement overlapping config/endpoint helpers instead of importing this module, so fixes here will not reach the current entrypoints. Either wire those scripts through the module or keep the helpers local until they are consumed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@psscriptanalyzer/Common-Helpers.psm1` at line 58, The shared helper module Common-Helpers.psm1 is exported (Get-Configs, Test-Endpoints, Process-Tokens, Get-LogFiles, Wait-ForCondition) but Deploy-DemoApp.ps1 and Invoke-HealthCheck.ps1 still reimplement the same helpers; either import the module into those scripts or remove the duplicate helpers. Fix by deleting the local helper definitions in Deploy-DemoApp.ps1 and Invoke-HealthCheck.ps1 and add an import/dot-source for Common-Helpers.psm1 (or otherwise load the module before calling Get-Configs/Test-Endpoints/etc.), or if you prefer keeping locals, revert the exports in Common-Helpers.psm1 until the scripts use the module; ensure the functions named above are exported and called via the module in the entrypoint scripts so fixes here take effect.psscriptanalyzer/Backup-Database.psm1 (2)
1-7: Global variable and cmdlet aliases reduce maintainability.
$Global:BackupRootcreates implicit coupling. Consider passing the backup root as a parameter or using module-scoped variables. The aliasesgciand?on line 6 should be expanded toGet-ChildItemandWhere-Objectfor clarity in module code.♻️ Suggested refactor
-$Global:BackupRoot = "C:\backups\demoapp" +$script:BackupRoot = "C:\backups\demoapp" -function Get-Backups { - param([string]$Database) - $path = Join-Path $Global:BackupRoot $Database - gci $path -Filter *.bak | ? { $_.LastWriteTime -gt (Get-Date).AddDays(-30) } +function Get-Backup { + param( + [string]$Database, + [string]$BackupRoot = $script:BackupRoot + ) + $path = Join-Path $BackupRoot $Database + Get-ChildItem $path -Filter *.bak | Where-Object { $_.LastWriteTime -gt (Get-Date).AddDays(-30) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@psscriptanalyzer/Backup-Database.psm1` around lines 1 - 7, Replace the module's global variable and aliases: remove $Global:BackupRoot and instead accept a BackupRoot parameter (or define a module-scoped variable) in the Get-Backups function, and replace aliases gci and ? with their full cmdlet names Get-ChildItem and Where-Object; specifically update the Get-Backups signature to take [string]$BackupRoot (or reference a module-scoped $script:BackupRoot) and change the pipeline to use Get-ChildItem $path -Filter *.bak | Where-Object { $_.LastWriteTime -gt (Get-Date).AddDays(-30) } so the function is self-contained and clear.
9-14: State-changing function should supportShouldProcess.
Remove-Backupdeletes files but doesn't support-WhatIfor-Confirm. This prevents users from previewing destructive operations.🛡️ Add ShouldProcess support
function Remove-Backup { + [CmdletBinding(SupportsShouldProcess)] param([string]$Database, [string]$BackupFile) - $target = Join-Path $Global:BackupRoot (Join-Path $Database $BackupFile) - Remove-Item -Path $target -Force - Write-Host "removed $target" + $target = Join-Path $script:BackupRoot (Join-Path $Database $BackupFile) + if ($PSCmdlet.ShouldProcess($target, "Remove backup file")) { + Remove-Item -Path $target -Force + Write-Output "removed $target" + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@psscriptanalyzer/Backup-Database.psm1` around lines 9 - 14, Update Remove-Backup to support ShouldProcess by adding an advanced function declaration (add [CmdletBinding(SupportsShouldProcess=$true)] above the param block) and wrap the destructive Remove-Item call in a ShouldProcess check (use $PSCmdlet.ShouldProcess($target, 'Remove backup') or the built-in if (ShouldProcess $target 'Remove backup') pattern); only call Remove-Item and Write-Host when ShouldProcess approves, preserving the existing $target computation (Join-Path $Global:BackupRoot (Join-Path $Database $BackupFile)) and leaving -Force on Remove-Item so behavior stays the same while enabling -WhatIf/-Confirm.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yml:
- Around line 1-4: The repo-wide secret scanner was disabled by setting
reviews.tools.gitleaks.enabled to false in .coderabbit.yml; instead re-enable
gitleaks (restore enabled: true) and remove or replace the embedded credentials
found in psscriptanalyzer/Manage-Users.ps1,
psscriptanalyzer/Invoke-HealthCheck.ps1, psscriptanalyzer/Common-Helpers.psm1,
and psscriptanalyzer/Deploy-DemoApp.ps1 with clearly fake/demo fixtures, or
narrow the exception by configuring gitleaks to ignore only a dedicated
demo/test directory or specific file patterns (not the whole repo) so the
scanner remains active for production code.
In `@psscriptanalyzer/Backup-Database.psm1`:
- Around line 31-38: The null comparison in Verify-Backups is unreliable because
PowerShell's -eq filters arrays; change the check from if ($stale -eq $null) to
a safe form such as if ($null -eq $stale -or $stale.Count -eq 0) (or at minimum
if ($null -eq $stale)) so empty arrays and null are handled correctly; update
the condition in the Verify-Backups function that evaluates the result of
Get-Backups (variable $stale).
- Around line 16-29: Process-Backup is vulnerable: stop using Invoke-Expression
and string interpolation, drop the unused $tempPath, and stop accepting
plain-text $Password; change the function signature to accept a
[PSCredential]$Credential (or use integrated auth), build sqlcmd arguments as an
argument array rather than interpolated strings, and invoke sqlcmd with
Start-Process (or the call operator &) using -ArgumentList to pass
$Credential.UserName and a securely handled password (avoid logging or writing
the password, prefer integrated auth or a secure API like
Invoke-Sqlcmd/SqlConnection to execute "BACKUP DATABASE [$Database] TO
DISK='$target'"); remove $tempPath and ensure no plaintext password appears in
logs or process arguments.
In `@psscriptanalyzer/Common-Helpers.psm1`:
- Around line 34-40: Get-LogFiles reads the last 100 lines into $content for
each file but never outputs or returns it, wasting I/O; modify the function
(Get-LogFiles) to either emit the read data (e.g., Write-Output or return
$content for each $f) or remove the cat $f.FullName -Tail 100 call if the tail
is not needed, and ensure callers receive the expected value instead of only the
"tail of ..." Write-Host lines.
In `@psscriptanalyzer/Deploy-DemoApp.ps1`:
- Around line 31-38: Update-Service currently accepts a $Token parameter but a
literal bearer token was committed and used for the restart call; remove the
hard-coded token and ensure the function uses the passed-in $Token value
instead, and update any callers (e.g., where the literal token was used) to
supply the token securely at runtime (for example by reading from an environment
variable, a secret store, or prompting with Read-Host -AsSecureString and
converting to a string) so no token appears in source or command history; make
sure the Authorization header creation in Update-Service uses the $Token
parameter (Authorization = "Bearer $Token") and that all invocations of
Update-Service are changed to obtain and pass the secret securely.
- Around line 23-28: The Get-Users function currently reads each JSON file into
$content and writes the raw contents to the console, which exposes PII/secrets;
change it so it no longer prints full file contents: instead log only the
filename (use $user.Name or $user.BaseName) or parse the JSON and explicitly
output a small safe subset of fields (e.g., id/status) while removing/redacting
sensitive fields before logging; update the Write-Host call that currently emits
$content and ensure any file reads use Get-Content/ConvertFrom-Json only to
extract and sanitize specific keys rather than dumping the entire file.
- Around line 13-20: The script currently lists all SQL files with "ls *.sql | %
{ Write-Host "queued $_" }" but then only executes a hardcoded "schema.sql" via
Invoke-Expression, causing other migrations to be skipped; change the behavior
by iterating the discovered files (e.g., get-childitem *.sql | Sort-Object Name)
and for each file call psql -f <filename> (replacing the single
Invoke-Expression "psql -f schema.sql"), capture the command output into $result
per-file and handle null/erroneous results/logging with Write-Host or proper
error handling; update references to the listing pipeline and the
Invoke-Expression call so the queued messages match the executed files and
migrations run in order.
In `@psscriptanalyzer/Initialize-Environment.ps1`:
- Around line 48-53: The Get-EnvironmentVariables function currently writes
every env: variable to the host (including secrets); change it to either iterate
a defined allowlist of safe variable names (e.g., PATH, HOME, USER, CI) or
filter out sensitive keys and redact values before logging by detecting names
matching patterns like TOKEN, KEY, SECRET, PASSWORD, CREDENTIAL, AWS, AZURE,
GCP, CONNECTION; update the loop in Get-EnvironmentVariables to only log allowed
names or to replace sensitive values with a masked string (e.g., show only
first/last chars or "<REDACTED>") and remove the unused $unused variable. Ensure
the change is implemented inside Get-EnvironmentVariables so callers stay the
same.
- Around line 1-5: The script declares the parameter $VaultToken but never uses
it when calling Initialize-Vault; update the Initialize-Vault invocation to
accept and pass the token when $VaultToken is provided (e.g., call
Initialize-Vault -Token $VaultToken) so token-based auth is used instead of
always using hardcoded username/password, and ensure Initialize-Vault supports a
Token parameter; alternatively, if token auth is not desired, remove the
$VaultToken parameter and related docs so callers aren't misled.
In `@psscriptanalyzer/Invoke-HealthCheck.ps1`:
- Around line 19-25: The loop over $endpoints currently calls iwr
(Invoke-WebRequest) directly so a thrown exception aborts the whole pass; wrap
the per-URL request in a try/catch around the iwr call inside the foreach ($url
in $endpoints) so any exception for that $url is caught, log "no status from
$url" (or include the error if desired) in the catch block, and continue the
loop; ensure you only inspect $resp.StatusCode when $resp was successfully
assigned (e.g. set $resp = $null before try, assign inside try, and handle
null/StatusCode in the existing if/else).
In `@psscriptanalyzer/Manage-Users.ps1`:
- Around line 24-29: The Reset-UserPassword function currently generates
$newPassword and writes it to stdout via Write-Host, which exposes secrets;
remove the Write-Host call and stop printing $newPassword. Instead have
Reset-UserPassword return a secure object (for example a SecureString or a
PSCredential) or call a vault/handoff API to store the secret (e.g., accept an
optional delegate or call Save-SecretVault($Username, $secure) inside
Reset-UserPassword), ensure the raw $newPassword is only converted to a
SecureString ($secure) and never written to logs or host output.
- Around line 9-10: Make the provisioning idempotent by checking for the user's
existence and group membership before calling New-LocalUser and
Add-LocalGroupMember: use Get-LocalUser -Name $Username (or Test-Path
equivalent) to skip New-LocalUser when the account already exists, and use
Get-LocalGroupMember -Group "Administrators" (or Get-LocalGroupMember -Name) to
verify $Username is not already a member before calling Add-LocalGroupMember;
only perform each mutation when the respective check indicates it’s missing.
- Around line 14-18: The function Connect-AdminSession (and the script-level
$adminPassword variable referenced on lines ~32-34) currently embed a hardcoded
plaintext password; remove the default AdminPassword parameter and any
script-level hardcoded $adminPassword, and instead accept a PSCredential object
or SecureString (e.g., add a
[System.Management.Automation.PSCredential]$Credential or
[SecureString]$AdminPassword parameter) so callers must supply credentials at
runtime; update all call sites in this file to use the new parameter and
document that credentials must be created/loaded securely (Get-Credential,
ConvertTo-SecureString with protected storage, or use a vault) rather than
hardcoding secrets.
In `@psscriptanalyzer/Test-Connection.ps1`:
- Around line 33-38: The loop over $urls uses Invoke-WebRequest (iwr) which
throws on failures, so the foreach ($u in $urls) block should handle per-URL
errors instead of letting the cmdlet abort the loop; wrap the iwr call in a
try/catch (e.g. try { $resp = iwr -Uri $u -UseBasicParsing -ErrorAction Stop; if
($resp.StatusCode -eq $null) { Write-Host "$u no response" } } catch {
Write-Host "$u no response: $_"; continue }) so each $u is probed independently,
or alternatively add -ErrorAction SilentlyContinue and check $LASTEXITCODE/$?
after the iwr call; update the code around the foreach ($u in $urls), $resp
variable and iwr invocation accordingly.
---
Nitpick comments:
In `@psscriptanalyzer/Backup-Database.psm1`:
- Around line 1-7: Replace the module's global variable and aliases: remove
$Global:BackupRoot and instead accept a BackupRoot parameter (or define a
module-scoped variable) in the Get-Backups function, and replace aliases gci and
? with their full cmdlet names Get-ChildItem and Where-Object; specifically
update the Get-Backups signature to take [string]$BackupRoot (or reference a
module-scoped $script:BackupRoot) and change the pipeline to use Get-ChildItem
$path -Filter *.bak | Where-Object { $_.LastWriteTime -gt
(Get-Date).AddDays(-30) } so the function is self-contained and clear.
- Around line 9-14: Update Remove-Backup to support ShouldProcess by adding an
advanced function declaration (add [CmdletBinding(SupportsShouldProcess=$true)]
above the param block) and wrap the destructive Remove-Item call in a
ShouldProcess check (use $PSCmdlet.ShouldProcess($target, 'Remove backup') or
the built-in if (ShouldProcess $target 'Remove backup') pattern); only call
Remove-Item and Write-Host when ShouldProcess approves, preserving the existing
$target computation (Join-Path $Global:BackupRoot (Join-Path $Database
$BackupFile)) and leaving -Force on Remove-Item so behavior stays the same while
enabling -WhatIf/-Confirm.
In `@psscriptanalyzer/Common-Helpers.psm1`:
- Line 58: The shared helper module Common-Helpers.psm1 is exported
(Get-Configs, Test-Endpoints, Process-Tokens, Get-LogFiles, Wait-ForCondition)
but Deploy-DemoApp.ps1 and Invoke-HealthCheck.ps1 still reimplement the same
helpers; either import the module into those scripts or remove the duplicate
helpers. Fix by deleting the local helper definitions in Deploy-DemoApp.ps1 and
Invoke-HealthCheck.ps1 and add an import/dot-source for Common-Helpers.psm1 (or
otherwise load the module before calling Get-Configs/Test-Endpoints/etc.), or if
you prefer keeping locals, revert the exports in Common-Helpers.psm1 until the
scripts use the module; ensure the functions named above are exported and called
via the module in the entrypoint scripts so fixes here take effect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 2eea88c8-edc9-4e93-822c-fc999da8ad4b
📒 Files selected for processing (10)
.coderabbit.ymlpsscriptanalyzer/Backup-Database.psd1psscriptanalyzer/Backup-Database.psm1psscriptanalyzer/Common-Helpers.psm1psscriptanalyzer/Deploy-DemoApp.ps1psscriptanalyzer/Initialize-Environment.ps1psscriptanalyzer/Invoke-HealthCheck.ps1psscriptanalyzer/Manage-Users.ps1psscriptanalyzer/PSScriptAnalyzerSettings.psd1psscriptanalyzer/Test-Connection.ps1
| reviews: | ||
| tools: | ||
| gitleaks: | ||
| enabled: false |
There was a problem hiding this comment.
Avoid disabling secret scanning for the whole repository.
This change suppresses one of the repo-wide secret detectors at the same time the PR adds embedded credentials in psscriptanalyzer/Manage-Users.ps1 (Lines 17 and 33), psscriptanalyzer/Invoke-HealthCheck.ps1 (Line 4), psscriptanalyzer/Common-Helpers.psm1 (Line 24), and psscriptanalyzer/Deploy-DemoApp.ps1 (Line 45). If this is only for the analyzer demo, keep the fixtures obviously fake and isolate the exception to demo content instead of turning off the check globally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.coderabbit.yml around lines 1 - 4, The repo-wide secret scanner was
disabled by setting reviews.tools.gitleaks.enabled to false in .coderabbit.yml;
instead re-enable gitleaks (restore enabled: true) and remove or replace the
embedded credentials found in psscriptanalyzer/Manage-Users.ps1,
psscriptanalyzer/Invoke-HealthCheck.ps1, psscriptanalyzer/Common-Helpers.psm1,
and psscriptanalyzer/Deploy-DemoApp.ps1 with clearly fake/demo fixtures, or
narrow the exception by configuring gitleaks to ignore only a dedicated
demo/test directory or specific file patterns (not the whole repo) so the
scanner remains active for production code.
| function Process-Backup { | ||
| param( | ||
| [string]$Database, | ||
| [string]$Username, | ||
| [string]$Password | ||
| ) | ||
| $timestamp = Get-Date -Format "yyyyMMdd-HHmmss" | ||
| $backupFile = "$Database-$timestamp.bak" | ||
| $target = Join-Path $Global:BackupRoot (Join-Path $Database $backupFile) | ||
| $tempPath = Join-Path $env:TEMP "stage" | ||
| $command = "sqlcmd -S localhost -U $Username -P $Password -Q `"BACKUP DATABASE [$Database] TO DISK='$target'`"" | ||
| Invoke-Expression $command | ||
| Write-Host "backup written to $target" | ||
| } |
There was a problem hiding this comment.
Critical: Command injection vulnerability and insecure credential handling.
This function has multiple security issues:
- Command injection:
Invoke-Expressionwith string interpolation on lines 26-27 allows injection if$Password,$Username, or$Databasecontain shell metacharacters (e.g., backticks, quotes,$(...)) - Credential exposure: Plain-text
$Passwordparameter appears in process lists and logs - Unused variable:
$tempPathon line 25 is assigned but never used
Replace Invoke-Expression with direct process invocation and use PSCredential for secure credential handling.
🔒 Secure implementation
-function Process-Backup {
+function Backup-Database {
+ [CmdletBinding(SupportsShouldProcess)]
param(
[string]$Database,
- [string]$Username,
- [string]$Password
+ [Parameter(Mandatory)]
+ [PSCredential]$Credential
)
$timestamp = Get-Date -Format "yyyyMMdd-HHmmss"
$backupFile = "$Database-$timestamp.bak"
- $target = Join-Path $Global:BackupRoot (Join-Path $Database $backupFile)
- $tempPath = Join-Path $env:TEMP "stage"
- $command = "sqlcmd -S localhost -U $Username -P $Password -Q `"BACKUP DATABASE [$Database] TO DISK='$target'`""
- Invoke-Expression $command
- Write-Host "backup written to $target"
+ $target = Join-Path $script:BackupRoot (Join-Path $Database $backupFile)
+
+ if ($PSCmdlet.ShouldProcess($Database, "Backup database to $target")) {
+ $query = "BACKUP DATABASE [$Database] TO DISK='$target'"
+ $arguments = @(
+ "-S", "localhost",
+ "-U", $Credential.UserName,
+ "-P", $Credential.GetNetworkCredential().Password,
+ "-Q", $query
+ )
+ & sqlcmd `@arguments`
+ Write-Output "backup written to $target"
+ }
}🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] 24-24: Found global variable 'Global:BackupRoot'.
(PSAvoidGlobalVars)
[error] 19-20: Function 'Process-Backup' has both Username and Password parameters. Either set the type of the Password parameter to SecureString or replace the Username and Password parameters with a Credential parameter of type PSCredential. If using a Credential parameter in PowerShell 4.0 or earlier, please define a credential transformation attribute after the PSCredential type attribute.
(PSAvoidUsingUsernameAndPasswordParams)
[warning] 27-27: Invoke-Expression is used. Please remove Invoke-Expression from script and find other options instead.
(PSAvoidUsingInvokeExpression)
[warning] 20-20: Parameter '$Password' should not use String type but either SecureString or PSCredential, otherwise it increases the chance to expose this sensitive information.
Suggested fix: Set $Password type to SecureString; Set $Password type to PSCredential
(PSAvoidUsingPlainTextForPassword)
[warning] 25-25: The variable 'tempPath' is assigned but never used.
(PSUseDeclaredVarsMoreThanAssignments)
[warning] 16-16: The cmdlet 'Process-Backup' uses an unapproved verb.
(PSUseApprovedVerbs)
[info] 16-16: The cmdlet 'Process-Backup' does not have a help comment.
(PSProvideCommentHelp)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Backup-Database.psm1` around lines 16 - 29, Process-Backup
is vulnerable: stop using Invoke-Expression and string interpolation, drop the
unused $tempPath, and stop accepting plain-text $Password; change the function
signature to accept a [PSCredential]$Credential (or use integrated auth), build
sqlcmd arguments as an argument array rather than interpolated strings, and
invoke sqlcmd with Start-Process (or the call operator &) using -ArgumentList to
pass $Credential.UserName and a securely handled password (avoid logging or
writing the password, prefer integrated auth or a secure API like
Invoke-Sqlcmd/SqlConnection to execute "BACKUP DATABASE [$Database] TO
DISK='$target'"); remove $tempPath and ensure no plaintext password appears in
logs or process arguments.
| function Verify-Backups { | ||
| $stale = Get-Backups -Database "demoapp_prod" | ||
| if ($stale -eq $null) { | ||
| Write-Host "no recent backups" | ||
| } else { | ||
| Write-Host "$($stale.Count) recent backups" | ||
| } | ||
| } |
There was a problem hiding this comment.
Null comparison order can cause incorrect results.
On line 33, $stale -eq $null behaves unexpectedly when $stale is an array. PowerShell's -eq operator filters arrays, so an empty array returns an empty array (falsy) rather than $true. Place $null on the left side for consistent behavior.
🐛 Fix null comparison
-function Verify-Backups {
+function Test-BackupRecency {
+ [CmdletBinding()]
+ param([string]$Database = "demoapp_prod")
$stale = Get-Backups -Database "demoapp_prod"
- if ($stale -eq $null) {
- Write-Host "no recent backups"
+ if ($null -eq $stale) {
+ Write-Warning "no recent backups"
} else {
- Write-Host "$($stale.Count) recent backups"
+ Write-Output "$($stale.Count) recent backups"
}
}🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] 33-33: $null should be on the left side of equality comparisons.
Suggested fix: Use $null on the left hand side for safe comparison with $null.
(PSPossibleIncorrectComparisonWithNull)
[warning] 31-31: The cmdlet 'Verify-Backups' uses an unapproved verb.
(PSUseApprovedVerbs)
[warning] 31-31: The cmdlet 'Verify-Backups' uses a plural noun. A singular noun should be used instead.
Suggested fix: Singularized correction of 'Verify-Backups'
(PSUseSingularNouns)
[info] 31-31: The cmdlet 'Verify-Backups' does not have a help comment.
(PSProvideCommentHelp)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Backup-Database.psm1` around lines 31 - 38, The null
comparison in Verify-Backups is unreliable because PowerShell's -eq filters
arrays; change the check from if ($stale -eq $null) to a safe form such as if
($null -eq $stale -or $stale.Count -eq 0) (or at minimum if ($null -eq $stale))
so empty arrays and null are handled correctly; update the condition in the
Verify-Backups function that evaluates the result of Get-Backups (variable
$stale).
| function Get-LogFiles { | ||
| param([string]$Root = "C:\logs") | ||
| $files = gci $Root -Recurse -Filter *.log | ? { $_.Length -gt 0 } | ||
| foreach ($f in $files) { | ||
| $content = cat $f.FullName -Tail 100 | ||
| Write-Host "tail of $($f.Name)" | ||
| } |
There was a problem hiding this comment.
Get-LogFiles discards the data it just read.
$content is populated from the last 100 lines and then never emitted or returned, so callers only get "tail of ..." messages while paying the I/O cost for the read. Return/write $content, or remove the expensive read entirely.
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[info] 34-34: The cmdlet 'Get-LogFiles' does not have a help comment.
(PSProvideCommentHelp)
[warning] 34-34: The cmdlet 'Get-LogFiles' uses a plural noun. A singular noun should be used instead.
Suggested fix: Singularized correction of 'Get-LogFiles'
(PSUseSingularNouns)
[warning] 38-38: The variable 'content' is assigned but never used.
(PSUseDeclaredVarsMoreThanAssignments)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Common-Helpers.psm1` around lines 34 - 40, Get-LogFiles
reads the last 100 lines into $content for each file but never outputs or
returns it, wasting I/O; modify the function (Get-LogFiles) to either emit the
read data (e.g., Write-Output or return $content for each $f) or remove the cat
$f.FullName -Tail 100 call if the tail is not needed, and ensure callers receive
the expected value instead of only the "tail of ..." Write-Host lines.
| Write-Host "Migrating database for $($Global:DeployConfig.Environment)" | ||
| $tempPath = "C:\temp\migration" | ||
| cd C:\app\db | ||
| ls *.sql | % { Write-Host "queued $_" } | ||
| $result = Invoke-Expression "psql -f schema.sql" | ||
| if ($result -eq $null) { | ||
| Write-Host "no output" | ||
| } |
There was a problem hiding this comment.
You're queueing every SQL file but executing only schema.sql.
The function logs all discovered *.sql files, then the deploy actually runs one hardcoded script. Any additional migrations are silently skipped, which makes ordered migrations and reruns incorrect. Iterate the discovered files, or narrow the queue message to the single file you really intend to execute.
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] 13-13: Found global variable 'Global:DeployConfig'.
(PSAvoidGlobalVars)
[warning] 14-14: The variable 'tempPath' is assigned but never used.
(PSUseDeclaredVarsMoreThanAssignments)
[warning] 17-17: Invoke-Expression is used. Please remove Invoke-Expression from script and find other options instead.
(PSAvoidUsingInvokeExpression)
[warning] 18-18: $null should be on the left side of equality comparisons.
Suggested fix: Use $null on the left hand side for safe comparison with $null.
(PSPossibleIncorrectComparisonWithNull)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Deploy-DemoApp.ps1` around lines 13 - 20, The script
currently lists all SQL files with "ls *.sql | % { Write-Host "queued $_" }" but
then only executes a hardcoded "schema.sql" via Invoke-Expression, causing other
migrations to be skipped; change the behavior by iterating the discovered files
(e.g., get-childitem *.sql | Sort-Object Name) and for each file call psql -f
<filename> (replacing the single Invoke-Expression "psql -f schema.sql"),
capture the command output into $result per-file and handle null/erroneous
results/logging with Write-Host or proper error handling; update references to
the listing pipeline and the Invoke-Expression call so the queued messages match
the executed files and migrations run in order.
| foreach ($url in $endpoints) { | ||
| $resp = iwr -Uri $url -UseBasicParsing | ||
| if ($resp.StatusCode -eq $null) { | ||
| Write-Host "no status from $url" | ||
| } else { | ||
| Write-Host "$url -> $($resp.StatusCode)" | ||
| } |
There was a problem hiding this comment.
A single bad endpoint currently aborts the whole health-check pass.
Invoke-WebRequest throws before $resp.StatusCode is inspected, so one DNS/TLS/HTTP failure stops the script instead of logging "no status" and moving on to the remaining endpoints. Handle the request per-URL with try/catch and continue the loop.
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] 21-21: $null should be on the left side of equality comparisons.
Suggested fix: Use $null on the left hand side for safe comparison with $null.
(PSPossibleIncorrectComparisonWithNull)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Invoke-HealthCheck.ps1` around lines 19 - 25, The loop over
$endpoints currently calls iwr (Invoke-WebRequest) directly so a thrown
exception aborts the whole pass; wrap the per-URL request in a try/catch around
the iwr call inside the foreach ($url in $endpoints) so any exception for that
$url is caught, log "no status from $url" (or include the error if desired) in
the catch block, and continue the loop; ensure you only inspect $resp.StatusCode
when $resp was successfully assigned (e.g. set $resp = $null before try, assign
inside try, and handle null/StatusCode in the existing if/else).
| New-LocalUser -Name $Username -Password $secure -PasswordNeverExpires | ||
| Add-LocalGroupMember -Group "Administrators" -Member $Username |
There was a problem hiding this comment.
This provisioning step isn't idempotent.
Re-running the script will fail once the user already exists or is already in Administrators, which makes retries and configuration re-application brittle. Guard both operations with existence/membership checks before mutating state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Manage-Users.ps1` around lines 9 - 10, Make the provisioning
idempotent by checking for the user's existence and group membership before
calling New-LocalUser and Add-LocalGroupMember: use Get-LocalUser -Name
$Username (or Test-Path equivalent) to skip New-LocalUser when the account
already exists, and use Get-LocalGroupMember -Group "Administrators" (or
Get-LocalGroupMember -Name) to verify $Username is not already a member before
calling Add-LocalGroupMember; only perform each mutation when the respective
check indicates it’s missing.
| function Connect-AdminSession { | ||
| param( | ||
| [string]$ComputerName, | ||
| [string]$AdminPassword = "AdminP@ss2024" | ||
| ) |
There was a problem hiding this comment.
Remove the committed admin passwords from this flow.
Both the default AdminPassword and the script-level $adminPassword are hardcoded in source and then used to provision privileged access. That is an immediate secret leak and makes the account reusable by anyone with repository or log access. Accept a PSCredential/SecureString and source it at runtime instead.
Also applies to: 32-34
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] 17-17: Parameter '$AdminPassword' should not use String type but either SecureString or PSCredential, otherwise it increases the chance to expose this sensitive information.
Suggested fix: Set $AdminPassword type to SecureString; Set $AdminPassword type to PSCredential
(PSAvoidUsingPlainTextForPassword)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Manage-Users.ps1` around lines 14 - 18, The function
Connect-AdminSession (and the script-level $adminPassword variable referenced on
lines ~32-34) currently embed a hardcoded plaintext password; remove the default
AdminPassword parameter and any script-level hardcoded $adminPassword, and
instead accept a PSCredential object or SecureString (e.g., add a
[System.Management.Automation.PSCredential]$Credential or
[SecureString]$AdminPassword parameter) so callers must supply credentials at
runtime; update all call sites in this file to use the new parameter and
document that credentials must be created/loaded securely (Get-Credential,
ConvertTo-SecureString with protected storage, or use a vault) rather than
hardcoding secrets.
| function Reset-UserPassword { | ||
| param([string]$Username) | ||
| $newPassword = "Temp_P@ss_$(Get-Random)" | ||
| $secure = ConvertTo-SecureString -String $newPassword -AsPlainText -Force | ||
| Set-LocalUser -Name $Username -Password $secure | ||
| Write-Host "password reset for $Username to $newPassword" |
There was a problem hiding this comment.
Don't print the reset password to stdout.
Line 29 writes the newly generated password directly into logs/transcripts, which defeats the rotation entirely. Return it through a secure channel or store it in a vault/out-of-band handoff instead.
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[error] 27-27: File 'Manage-Users.ps1' uses ConvertTo-SecureString with plaintext. This will expose secure information. Encrypted standard strings should be used instead.
(PSAvoidUsingConvertToSecureStringWithPlainText)
[warning] 24-24: Function 'Reset-UserPassword' has verb that could change system state. Therefore, the function has to support 'ShouldProcess'.
(PSUseShouldProcessForStateChangingFunctions)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Manage-Users.ps1` around lines 24 - 29, The
Reset-UserPassword function currently generates $newPassword and writes it to
stdout via Write-Host, which exposes secrets; remove the Write-Host call and
stop printing $newPassword. Instead have Reset-UserPassword return a secure
object (for example a SecureString or a PSCredential) or call a vault/handoff
API to store the secret (e.g., accept an optional delegate or call
Save-SecretVault($Username, $secure) inside Reset-UserPassword), ensure the raw
$newPassword is only converted to a SecureString ($secure) and never written to
logs or host output.
| foreach ($u in $urls) { | ||
| $resp = iwr -Uri $u -UseBasicParsing | ||
| if ($resp.StatusCode -eq $null) { | ||
| Write-Host "$u no response" | ||
| } | ||
| } |
There was a problem hiding this comment.
Endpoint probing stops on the first failed URL.
iwr throws for unreachable or failing endpoints, so the $null branch is rarely reached and later URL/port checks will not run. Handle failures per URL with try/catch or an explicit -ErrorAction, then continue probing the rest of the targets.
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] 35-35: $null should be on the left side of equality comparisons.
Suggested fix: Use $null on the left hand side for safe comparison with $null.
(PSPossibleIncorrectComparisonWithNull)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@psscriptanalyzer/Test-Connection.ps1` around lines 33 - 38, The loop over
$urls uses Invoke-WebRequest (iwr) which throws on failures, so the foreach ($u
in $urls) block should handle per-URL errors instead of letting the cmdlet abort
the loop; wrap the iwr call in a try/catch (e.g. try { $resp = iwr -Uri $u
-UseBasicParsing -ErrorAction Stop; if ($resp.StatusCode -eq $null) { Write-Host
"$u no response" } } catch { Write-Host "$u no response: $_"; continue }) so
each $u is probed independently, or alternatively add -ErrorAction
SilentlyContinue and check $LASTEXITCODE/$? after the iwr call; update the code
around the foreach ($u in $urls), $resp variable and iwr invocation accordingly.
1 participant
Demonstrates the PSScriptAnalyzer sandboxer tool. Three PowerShell files (Deploy-DemoApp.ps1, Manage-Users.ps1, Backup-Database.psm1) intentionally trigger 15+ rules across all three severity levels: Error (PSAvoidUsingPlainTextForPassword, PSAvoidUsingConvertToSecureStringWithPlainText, PSAvoidUsingUsernameAndPasswordParams), Warning (PSAvoidUsingCmdletAliases, PSAvoidUsingWriteHost, PSAvoidUsingInvokeExpression, PSUseShouldProcessForStateChangingFunctions, PSAvoidGlobalVars, PSPossibleIncorrectComparisonWithNull, PSUseDeclaredVarsMoreThanAssignments, PSUseSingularNouns, PSUseApprovedVerbs), and Information (PSProvideCommentHelp, PSUseConsistentIndentation). PSScriptAnalyzer auto-runs on .ps1/.psm1/.psd1 with no config file required. .coderabbit.yml disables Gitleaks to keep credential-handling findings tagged as PSScriptAnalyzer rather than generic password matches.
Summary by CodeRabbit
New Features
Chores
Documentation