PR 3/4: Fix logic bugs found during PSScriptAnalyzer review#30
Open
StrongWind1 wants to merge 14 commits intoCompassSecurity:mainfrom
Open
PR 3/4: Fix logic bugs found during PSScriptAnalyzer review#30StrongWind1 wants to merge 14 commits intoCompassSecurity:mainfrom
StrongWind1 wants to merge 14 commits intoCompassSecurity:mainfrom
Conversation
Run PSScriptAnalyzer with -Fix to automatically remove 695 instances of trailing whitespace across all .psm1 and .ps1 files. Rule: PSAvoidTrailingWhitespace (Information) No functional changes — whitespace-only diff.
Swap $consentCount -eq $null to $null -eq $consentCount in the consent count fallback logic (lines 4228-4229, 4672-4673). When $null is on the right side of -eq and the left operand is an array, PowerShell filters the array instead of performing a null check. Placing $null on the left ensures correct behavior regardless of the operand type. Rule: PSPossibleIncorrectComparisonWithNull (Warning)
Remove variables that are assigned but never read: - check_EnterpriseApps.psm1: $Owners (line 382), $EntraConnectApp (line 755) — flag set but never checked, warning added directly - check_PIM.psm1: $CapIssues (line 486) — boolean never read - EntraTokenAid.psm1: $AuthError init (line 210), $xms_cc (line 1645) — value extracted but never used - shared_Functions.psm1: $AuthCheck (line 5078) — auth test result discarded, error handling is in the catch block - Send-GraphBatchRequest.psm1: $ResultsList and $MoreNextLinks (lines 346-347) — lists initialized but never populated Rule: PSUseDeclaredVarsMoreThanAssignments (Warning)
Replace 8 empty catch blocks with Write-Verbose messages that log the suppressed exception. These catches are intentional (best-effort parsing of HTTP status codes, response bodies, date strings, and OS detection) but the empty blocks masked errors during debugging. The verbose messages only appear when -Verbose is used, so there is zero impact on normal operation. Affected files: - Send-ApiRequest.psm1: HTTP status, response body, JSON parsing - check_Tenant.psm1: date string parsing (4 instances) - shared_Functions.psm1: OS detection fallback Rule: PSAvoidUsingEmptyCatchBlock (Warning)
Add SuppressMessageAttribute for 14 parameters flagged as unused. Most are false positives where parameters are accessed by nested functions via parent scope (check_Roles, check_AppRegistrations, check_CAPs use inner Get-ObjectDetails/GetObjectInfo functions). Others are part of the shared module calling interface where the orchestrator passes all data to every module uniformly. Each suppression includes a comment explaining why the parameter is accepted but not directly referenced. Rule: PSReviewUnusedParameter (Warning)
- check_CAPs.psm1: Is-Empty -> Test-IsEmpty 'Is' is not an approved verb; 'Test' is the standard for boolean checks. - check_PIM.psm1: Parse-ISO8601Duration -> ConvertFrom-ISO8601Duration 'Parse' is not an approved verb; 'ConvertFrom' is the standard for converting input formats to PowerShell objects. Both are internal functions with all call sites updated. Rule: PSUseApprovedVerbs (Warning)
The ContainsKey guard used $group.Id (the outer loop variable) instead of $OwnedGroup.Id / $ParentGroup.Id (the inner loop variable). The guard almost always passed since the outer group exists in AllGroupsHT, but the subsequent lookup fetched data for the wrong group — returning null properties when the inner group ID was not present in the hashtable. Affected lines: 765 (eligible owner), 796 (eligible member)
The active roles section checked roleHashTable.ContainsKey before accessing role details, but the eligible roles section did not. If an eligible role assignment referenced a role ID not in the hashtable, $RoleDetails would be $null, causing null property access on RoleId, RoleName, and RoleType. Added the same guard pattern used by the active roles section.
Four debug/trace messages in the owned-objects processing loop referenced $user.Id (from an earlier data-collection loop) instead of $item.Id (the current processing loop variable). This caused debug output to show the wrong user ID when troubleshooting. Lines: 435, 444, 452, 470
…tsHTML Two fixes in check_AppRegistrations.psm1: 1. Remove 'Enabled' from format-table for SP owners (line 883). The PSCustomObject has DisplayName, Foreign, PublisherName, and OwnersCount but no Enabled property, producing an empty column. 2. Initialize $AppendixSecretsHTML to empty string before use with += operator (line 1123). Without initialization, the first += on $null creates an array instead of a string, causing malformed HTML when concatenated with $GLOBALJavaScript on line 1137.
The 'role' vs 'roles' label for Azure IAM role summaries omitted $Tier3Count from the total. A user with 1 Tier0 + 1 Tier3 role (2 total) would incorrectly see 'role' instead of 'roles'. The Entra role equivalent correctly excludes Tier3 since Entra roles have no Tier3 classification.
Lines 660, 664: Changed $_.Foreign -eq "True" to $_.Foreign -eq $true. The Foreign property is a boolean, and while PowerShell's type coercion makes string comparison work by accident, explicit boolean comparison is correct and consistent with lines 638-641 which already use -contains $true.
The variable was used with += without prior initialization. While PowerShell treats $null + int as int, explicit initialization to 0 is correct and consistent with the equivalent pattern in check_Users.psm1 line 240 ($TotalTransitiveMemberRelations = 0).
…licies The += on $GroupScriptWarningList (line 4767) was dead code: the variable is a List[string] initialized in check_Groups.psm1, but Get-ConditionalAccessPolicies runs from run_EntraFalcon.ps1 before check_Groups, so $GroupScriptWarningList is $null at call time. The += created a throwaway local array that was never returned or read. The warning is already handled correctly downstream: Get-ConditionalAccessPolicies sets $GLOBALPermissionForCaps = $false, and check_Groups.psm1 line 426-427 checks that flag and adds the appropriate warning to the properly initialized list.
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
1 participant
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.
Branch:
fix/pssa-logic-bugsBase:
main(stacked on PR 1 + PR 2)This is the third of four PRs from my code review. While running PSScriptAnalyzer and doing an AI review, I came across several genuine logic bugs, including wrong variable references, missing null checks, and a few things that only happen to work by accident. Each fix is its own commit with a clear explanation. None of these change assessment logic or scoring, they're all about correctness in edge cases and report output.
Feel free to ignore any of these!
What this fixes (8 commits, 10 bugs)
Wrong variable in ContainsKey check for PIM group lookups in
check_Groups.psm1at lines 765 and 796. TheContainsKeyguard was checking$group.Id(the outer loop variable) but the data access used$OwnedGroup.Idand$ParentGroup.Id(the inner loop variable). So the guard was protecting against the wrong key. If the inner group ID wasn't in the hashtable,$infowould quietly become$nulland properties likeSecurityEnabledandisAssignableToRolewould be missing from the output. The hashtable docs recommendContainsKey()specifically to avoid this kind of silent null access, but it only helps if you check the right key!Missing null safety for eligible Azure role lookups in
shared_Functions.psm1at line 4575. The active roles section (line 4530) already correctly calls$roleHashTable.ContainsKey($roleId)before accessing role details, but the eligible roles section didn't have the same guard. If an eligible assignment referenced a role ID not in the lookup table,$RoleDetailswould be$nulland accessing.RoleId,.RoleName,.RoleTypewould all quietly return$null. As the $null deep dive covers, accessing a property on an object that doesn't have it just gives you$nullwith no error. I added the same guard pattern that the active roles section already uses since that felt like the most consistent way to handle it.Debug logs reference wrong loop variable in
check_Users.psm1at lines 435, 444, 452, and 470. Four debug/trace messages inside the main user processing loop (foreach ($item in $AllUsers)) referenced$user.Id, which is a variable from an earlier data collection loop. Because of how PowerShell scoping works,$userstill holds its value from the last iteration of that earlier loop, so every debug message would show the same wrong user ID. One of those things that's easy to miss but easy to fix.Format Table references nonexistent property and uninitialized HTML variable in
check_AppRegistrations.psm1at lines 883 and 1127. Two issues in one commit. The TXT report for SP owners usedformat-table -Property DisplayName,Enabled,Foreign,...but the PSCustomObject doesn't actually have anEnabledproperty (it hasDisplayName,Foreign,PublisherName, andOwnersCount) so theEnabledcolumn was showing up empty for every row. Also,$AppendixSecretsHTMLwas used with+=without ever being initialized. As the assignment operators docs cover, when+=is applied to$nullwith an array value (which is whatConvertTo-Htmlreturns), it creates anObject[]instead of a string. When this later gets concatenated with JavaScript on line 1137, the HTML output can come out garbled.Missing
$Tier3Countin Azure role pluralization inshared_Functions.psm1at line 4349. The "role" vs "roles" label check summed up Tier0, Tier1, Tier2, and Unknown counts but forgot to include Tier3. So a user with 1 Tier0 and 1 Tier3 role (2 roles total) would see "1 (Tier0), 1 (Tier3) Azure role assigned" instead of "roles". The Entra equivalent correctly has no Tier3 since Entra roles don't use that tier, but Azure roles do.Boolean compared to string
"True"instead of$trueincheck_AppRegistrations.psm1at lines 660 and 664. TheForeignproperty is a boolean, but these two lines compared it to the string"True". As the type conversion docs cover, PowerShell coerces the right hand side to match the left hand side type, so this happens to work, but it's a bit brittle and inconsistent with lines 638 through 641 in the same file which correctly use-contains $true. Swapped to$trueto keep things consistent.Uninitialized variable used with
+=incheck_Groups.psm1at line 469.$TotalGroupMembers += $group.Countwithout prior initialization. The $null deep dive notes that when a$nullvalue is used in a numeric equation the results can be invalid without giving an error. The equivalent pattern incheck_Users.psm1already does this correctly with an explicit= 0initialization, so I matched that here.Dead warning append in wrong scope in
shared_Functions.psm1at line 4767.$GroupScriptWarningList += "Coverage gap..."insideGet-ConditionalAccessPolicies, but this function runs fromrun_EntraFalcon.ps1beforecheck_Groups.psm1initializes$GroupScriptWarningList. As the assignment operators docs cover, compound assignment operators don't use dynamic scoping and the variable is always in the current scope. So+=created a throwaway local array that was never returned and the warning just got lost. The good news is thatcheck_Groups.psm1at lines 426 and 427 already handles this correctly via the$GLOBALPermissionForCapsflag, so I just removed the dead line.This is PR 3 of 4, stacked on PR 1 (whitespace) and PR 2 (code quality).
All four branches merge cleanly into
mainand I've tested every merge order combination. However, you can cherry pick individual commits if you would like.Thanks so much for the great tool, these are all pretty minor issues and the codebase is really solid overall. I love the HTML reports and this tool is super useful!