Skip to content

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

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

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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