Manage AD Accounts - Part 2

Why run an AI code review?

In Post 1 I walked through what the AD Account Manager does — a PowerShell GUI for managing contractor accounts with role-based access control, multi-OU search, and audit logging. Before publishing that code to GitHub, I wanted to catch anything I'd missed.

Anything touching Active Directory and running with domain credentials deserves more than a quick look. The code handles authentication, authorization, and account modifications — if there are bugs here, they affect real user accounts. I ran the full 618-line script through Claude Opus and worked through the output. This post documents everything it found.

Critical findings

Critical LDAP filter injection via string interpolation

The search sanitizer strips characters like (, ), &, and | — but it misses single quotes and backticks. The sanitized term then gets interpolated directly into the AD filter string:

$filter = "... (Name -like '*$term*' -or SamAccountName -like '*$term*')"

A search term containing a single quote could break out of the -like pattern and append arbitrary filter clauses. The fix is straightforward: add ' and ` to the character strip list, and optionally switch from PowerShell's -Filter to -LDAPFilter with proper RFC 4515 escaping for a smaller injection surface.

Critical Privileged group check uses regex, not exact match

Two places in the script check whether an account belongs to a privileged group by using -match against the Distinguished Names in memberOf:

# In Test-CanModifyAccount and Get-ExpiringUsers
if ($targetUser.memberOf -match "Domain Admins")

-match is a regex substring operator, not an equality check. The string "Administrators" matches any DN containing that word — including CN=Hyper-V Administrators, CN=Storage Administrators, and any custom group with "Administrators" in the name. This creates false positives that block legitimate helpdesk operations. The fix is to resolve each DN to its group name and compare with -eq, or compare against the group's full Distinguished Name directly.

Critical Script path resolution fails in some deployment contexts

The script locates config.json using $MyInvocation.MyCommand.Path. When Intune deploys a script via powershell.exe -Command rather than -File, that value is $null. Split-Path -Parent $null returns an empty string, and Join-Path "" "config.json" resolves to the current directory — which in a SYSTEM-context Intune execution is often C:\Windows\System32. The config won't be found, or worse, a stray file there would be loaded. The fix is to use $PSScriptRoot with a fallback guard.

High severity findings

High Role check uses group name, not SID

Group membership is determined by comparing resolved group names against the names in config.json. Group names are mutable — they can be renamed. In a multi-domain forest, the same name can exist in different domains. The correct approach is to compare against group SIDs, which are immutable and unique across forests. An even faster option: read the SIDs from the current user's Windows identity token instead of making AD queries at all.

High Permission check and write operation are not atomic (TOCTOU)

Both action buttons call Test-CanModifyAccount and then, if allowed, perform the write in a separate step. Between those two calls — including the time the user spends reading the confirmation dialog — the target account's group membership could change. It's a small window in a GUI tool, but for a security-sensitive tool the pattern is wrong. The write should be performed under the same authorization context as the check, or the check should be re-run immediately before the write inside the same try block.

High SuperAdmin bypasses all account protections including self-modification

Test-CanModifyAccount returns Allowed=$true for SuperAdmin before checking anything else — including whether the target is a Domain Admin or whether it's the operator's own account. A SuperAdmin can disable their own account or disable other SuperAdmin accounts. For a contractor management tool this level of bypass is excessive. At minimum, self-modification should be blocked for all roles.

High Audit log is world-readable and not tamper-protected

%ProgramData% is readable by all local users by default. The log records usernames, account names, OU paths from error messages, and security denial reasons — useful intelligence for a malicious local user. It can also be appended to or truncated by any user with write access. The fix is to set an explicit ACL on the log directory after creating it, restricting access to SYSTEM and administrators. Alternatively, write to the Windows Event Log, which has built-in tamper protection.

Medium severity findings

Medium Contractor detection logic produces false positives

Test-IsContractor returns true if an account has either an expiry date set or "Contractor" in the Description. The -or means any full-time employee account with an expiry date set — interns, seasonal staff, accounts with password policy exceptions — shows up as a contractor. And "Contractor" anywhere in the Description field matches, including "Not a Contractor" or "Former Contractor." The ContractorManager role's entire authorization boundary depends on this function being accurate.

Medium UI state is stale after successful actions

After a successful expiry update, the info panel still shows the old date. After disabling an account, it remains in the dropdown and $script:currentSam still points to it — a second click on "Disable Account" will attempt the operation again. The contractor counter is also not decremented. The fix is to clear the selection state and refresh the relevant UI elements immediately after each successful write operation.

Medium Date picker has no minimum date — can set expiry in the past

There is no MinDate set on the DateTimePicker. Setting an expiry date in the past immediately locks the account at next authentication — likely unintentional for a typical helpdesk operator. Setting $datePicker.MinDate = (Get-Date).Date.AddDays(1) prevents this class of mistake at the UI level.

Medium No form or timer disposal — GDI handle leak

ShowDialog() requires the caller to dispose the form when it closes. Without explicit disposal, WinForms objects hold onto GDI handles. In a long-running PowerShell session this accumulates. The search debounce timer also needs to be disposed. Wrapping the ShowDialog() call in a try/finally block with $form.Dispose() and $searchTimer.Dispose() is the fix.

Medium Show All and form load block the UI thread — no caching

The form's Shown event loads all contractors synchronously, and "Show All" does the same. With multiple OUs and a slow domain controller, this can freeze the UI for several seconds. There's also no caching — every click on "Show All" hits AD again. A short TTL cache (60 seconds) would make repeated use much faster, and a BackgroundWorker would prevent the UI freeze.

Best practice findings

Low $input shadows a PowerShell automatic variable

The parameter in Sanitize-LDAPInput is named $input, which is a reserved automatic variable in PowerShell (the pipeline enumerator). It works inside the function because the parameter declaration takes precedence, but it will confuse static analyzers and can cause subtle behavior if the function is ever refactored to accept pipeline input. Rename it to $searchTerm or $rawInput.

Low Unapproved PowerShell verbs

Log-Action and Sanitize-LDAPInput both use verbs that aren't on PowerShell's approved verb list. This triggers PSScriptAnalyzer warnings and looks sloppy in a portfolio context. Write-ActionLog and Protect-LDAPInput are clean alternatives.

Low Missing #Requires directives

The script has no #Requires block. Adding #Requires -Version 5.1 and #Requires -Modules ActiveDirectory at the top gives PowerShell early-exit failure messages instead of cryptic runtime errors. The #Requires -Modules directive also makes the manual Import-Module try/catch block unnecessary.

Low Inconsistent use of return vs exit at script scope

Config errors use return to exit the script; the access-denied check uses exit. At script scope these are usually equivalent, but return will exit the caller's scope if the script is dot-sourced — a subtle breakage. Using exit 1 consistently for all early termination paths is cleaner and unambiguous.

What's next

The next post covers the fixes — what I changed, what I decided to leave alone, and one or two cases where the AI review flagged something as a bug that was actually intentional. Once the fixes are in, the script goes up on GitHub. The goal is that the published version is something I'd be comfortable explaining line by line in an interview.