Fix RDS logon report to read RMM vars from $env: namespace

[mnelsondtc]

Summary

Follow-up fix to PR #52 (merged): bring msft-windows-rds-logon-report.ps1 to the same RMM hardening baseline as the app-putty scripts (#78).

Root cause: NinjaOne passes RMM custom variables as environment variables, not as PowerShell scope variables. The merged version of the script read $RMM, $DaysBack, and $OutputFolder from script scope, so anything configured in the Ninja Variables tab was silently ignored. The -NonInteractive auto-detect added during the original PR let the script run, but the configured variables still did not reach it.

Changes

• All RMM inputs now read from $env: namespace: $env:RMM, $env:Description, $env:RMMScriptPath, $env:DaysBack, $env:OutputFolder.
• [string]::IsNullOrWhiteSpace() guards on each env-var read so empty strings (common from RMM passthrough) are treated as unset and the default is applied.
• Main body wrapped in try { ... } catch { } finally { ... } with explicit $exitCode tracking. Stop-Transcript now runs in finally, so an unhandled exception in the XML parse loop (or anywhere else) can no longer leak an open transcript.
• Inner exit 1 calls replaced with throw so the catch block centralizes failure handling.
• Catch block emits $_.ScriptStackTrace for breadcrumbs when something fails mid-parse.
• Explicit exit $exitCode at end of script so NinjaOne reflects success/failure accurately.
• [SUCCESS] / [FAILURE] markers in stdout for quick at-a-glance job results.
• Permission hint for Security log access (Event Log Readers / SYSTEM) rolled into the thrown error message instead of a separate Write-Host.

This is structurally the same pattern that landed in PR #78 for app-putty/putty-install.ps1 and app-putty/putty-configure-logging.ps1.

Backwards compatibility

Anyone running interactively with $RMM=1; .\script.ps1 (scope-var style) now needs to use $env:RMM='1'; .\script.ps1 instead. Same convention as the app-putty scripts. The Read-Host interactive path is unchanged for $env:RMM not set.

Test plan

• Run from NinjaOne as SYSTEM with DaysBack and OutputFolder set in the Variables tab; confirm the configured values are honored (filename uses requested span, CSV lands in configured folder)
• Run from NinjaOne with no custom variables defined; confirm defaults apply (90 days, C:\temp) and the job completes with [SUCCESS] + exit 0
• Run interactively (elevated) on an RDS Session Host using $env:RMM='1'; .\script.ps1; confirm CSV is produced
• Force a failure (e.g. point $env:OutputFolder at an unwritable path); confirm [FAILURE] + stack trace + exit 1
• Confirm transcript at C:\Windows\logs\msft-windows-rds-logon-report.log is closed cleanly in all of the above

[claude]

⚠️ Code review skipped — your organization's overage spend limit has been reached.

Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.

Once credits are available, push a new commit or reopen this pull request to trigger a review.

[Gumbees]

Review: PR #79 .. RDS logon report $env: hardening
Author: mnelsondtc → main, +149/-136, one file in msft-windows/ (msft-windows-rds-logon-report.ps1). HEAD 75bf7022.

What it does: brings msft-windows-rds-logon-report.ps1 to the same RMM hardening baseline as the app-putty scripts in PR #78. Sweeps bare $RMM / $DaysBack / $OutputFolder to $env: namespace, adds [string]::IsNullOrWhiteSpace guards, wraps the main body in try { ... } catch { } finally { Stop-Transcript }, replaces inner exit 1 with throw, tracks $exitCode, and exits explicitly. Follow-up to merged PR #52, addresses the convention drift PR #50's CLAUDE.md/template change never swept through legacy scripts.

The hardening pattern lands clean on every axis the prior reviews care about. But the compliance posture of the script's output makes the existing default unmergeable as-is.

Blockers (must fix before merge)

1. PII written to a world-readable temp directory by default
   msft-windows-rds-logon-report.ps1:58:

   $OutputFolder = if ([string]::IsNullOrWhiteSpace($env:OutputFolder)) { 'C:\temp' } else { $env:OutputFolder }

   :222:

   "TimeCreated,Computer,EventId,EventType,User,SessionID,SourceIP" | Set-Content -Path $csvPath -Encoding UTF8

   The CSV contains User, SourceIP, and SessionID for every RDS logon event in the window. Those are PII for dental-practice tenants (HIPAA-adjacent) and CUI-adjacent on the DoD/CMMC client subset. The default path is C:\temp .. host-local, world-readable by inherited ACL, no DTC-canonical hardened root. Any local user on the box (RDP session user, helpdesk that connected, attacker that landed unprivileged) can read every prior report.

   %WINDIR%\Temp is the same shape and not an acceptable destination either. Both inherit ACLs that don't gate read access to the auditing material.

   Fix shape:

   • Move the default to a hardened DTC-canonical root .. e.g. %ProgramData%\DTC\rds-reports\ (created with New-Item + an explicit Set-Acl that restricts read to Administrators and the service account that consumes the CSV). The DTC RMM state-storage convention is %PROGRAMDATA%\dtc\ per recent device-state-DB conversations .. align this script's report path with that pattern.
   • Validate $env:OutputFolder when set .. reject paths that escape the hardened root (Resolve-Path and confirm the resolved path starts with the canonical prefix; reject UNC paths \\share\... unless explicitly whitelisted). Right now ..\..\..\Windows\System32 or \\evil-share\loot gets created and written to silently.

2. PII not redacted at the report layer
   :222 ships raw User, SourceIP, SessionID columns. No option to hash the user identifier (e.g. SHA256 of domain\user for trend analysis without revealing the principal), no option to truncate or anonymize the source IP, no $env:RedactPII toggle. The report's read-side is "audit who logged in where, when" .. it can deliver that with non-reversible identifiers in most analysis modes. Pick a default redaction posture and add an env-var override for the cases that need the raw values.

   Fix shape:

   • Default: redact User to a hash ((Get-FileHash -Algorithm SHA256 ...) against a tenant-salted input) and zero the last octet of SourceIP (192.0.2.123 → 192.0.2.0).
   • $env:RedactPII = "0" opt-out for engineers who need raw values during an investigation, with the in-clear path explicitly noted in the operator's hand-off doc and reviewed by Mike Schepers before that mode ships.
   • Either way, the CSV header should reflect what's in the column (UserHash, SourceIPSubnet) when redaction is on, so a downstream reader knows what they're looking at.

Real gaps (please address)

3. Backwards-compat note missing from the PR body
   Any NinjaOne deployment that had DaysBack or OutputFolder set in the Variables tab was silently running with the script's defaults (90, C:\temp) because the original script read the bare-variable form. After this merges, those values take effect for the first time. An operator who set DaysBack=365 expecting a year of data and got 90 days will suddenly get a much larger CSV, possibly at a different path. One sentence in the body before merge.

4. Operational-scope gap in the PR body
   Body doesn't say who consumes the CSV, where it lands, or what the read-path looks like. Once blocker 1 is addressed and the destination is canonical, the body should state the contract: who reads these reports, on what cadence, and how they're rotated/purged. A logging/audit control without a read-loop is shelfware.

5. Unguarded [int]$env:DaysBack (~line 60)
   Throws on non-numeric, no bounds check. Negative DaysBack queries the future and returns an empty CSV silently. [int]::TryParse + range guard (1..365) would be cleaner. Pre-existing pattern across scripts, not a new regression in this PR.

Nits

• Helper function Get-FilteredEvents inside the main try rather than at top of file. Works fine (PowerShell hoists function defs), only called from one site, but conventionally function definitions live at the top of the file above the main flow.
• PascalCase on $Description/$DaysBack/$OutputFolder/$LogPath is intentional template mirroring (RMM-facing variable names), not a casing violation. Flagged for clarity.

Style notes (informational)

• Full $env: sweep, string compare -ne "1", IsNullOrWhiteSpace guards, try { ... } catch { } finally { Stop-Transcript } with $transcriptStarted guard, inner exit 1 → throw, $exitCode tracked, single exit $exitCode at end .. all the hardening axes from PR #78's post-review state land clean here. The shape of the code itself is correct.
• Template adherence is right. VARIABLES spelled correctly, three-section structure intact, header doc claims match code reality.
• Folder placement (msft-windows/) and filename (kebab-case) both correct.
• SYSTEM-context call-out is correctly inverted .. this script must run as SYSTEM to read the Security event log; the throw message explicitly calls that out (different from chrome-remote-desktop's SYSTEM-refuse pattern, correctly inverted for the use case).
• XML parse loop is defensive .. if ($d.Name) guard, machine-account skip, Get-FilteredEvents returns empty array on no-events.

Recommendation

REQUEST CHANGES.

The code-quality hardening is excellent and lands the PR #78 pattern cleanly. But the default output destination (C:\temp) and the absence of any PII-redaction layer in the CSV make this script a HIPAA / CMMC compliance liability the moment it runs against a dental practice's RDS host or a DoD-client jump box. Pin the default to a hardened DTC-canonical root, validate operator-supplied paths against escape, and add a redaction default (with an opt-out env-var for investigations). Once the output posture is right, blockers 3 and 4 are body-only edits and the rest is mergeable.

🤖 Reviewed by apis + 3-agent team (diff/intent, security/correctness, style/conventions). Posted as REQUEST CHANGES at Nate's direction .. the security slice's PII / temp-directory concerns are blocking for this PR.