Skip to content

Fix pre-existing PSCustomObject vs string comparison at Set-RiskRating call sites #279

@SamErde

Description

@SamErde

Summary

During review of PR #277 (SID lookup caching), a pre-existing bug was identified in Private\Set-RiskRating.ps1 at all Get-SidObjectClass call sites that use Pattern A (6 of 7 sites).

Problem

Get-SidObjectClass returns the result of Select-Object objectClass, which is a PSCustomObject with an objectClass property — not a plain string. The call sites then compare this object directly against strings:

\ = Get-SidObjectClass -Sid \
if (\.objectClass -eq 'group') { ... }   # Pattern B — correct
if (\ -eq 'group') { ... }               # Pattern A — PSCustomObject vs string: always false

Pattern B (ESC5, line ~430) correctly uses \.objectClass; Pattern A (6 call sites) compares the whole PSCustomObject, so those -eq 'group' / -eq 'user' comparisons always return \False.

History

This bug was present before PR #277 (the original Get-ADObject | Select-Object objectClass pattern had the same semantics). It was preserved intentionally in #277 to avoid broadening scope. A Copilot review comment noted it; it was classified as out-of-scope and acknowledged.

Fix

At each Pattern A call site, change:

# Before
\ = Get-SidObjectClass -Sid \
if (\ -eq 'group') { ... }

# After
\ = Get-SidObjectClass -Sid \
if (\.objectClass -eq 'group') { ... }

There are 6 Pattern A call sites in Set-RiskRating.ps1 (and the matching 6 in Invoke-Locksmith.ps1 for parity).

Impact

Because these comparisons always return \False, any risk-rating logic that depends on the SID being a group or user (Pattern A paths) is currently ineffective. The actual risk scores may be incorrect for those scenarios.

Labels

bug, risk-rating

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions