guru-connect/reports/review-2026-06-05/grok-agent.md

GuruConnect agent review — grok — 2026-06-05

All required files read via read_file (absolute paths, full content via large limit where needed; identity.json read first per project rules). No other files explored, no modifications made, no further tools used after the mandated reads. Review strictly limited to the 34 listed source files + identity.

User context (from mandatory identity read): Mike Swanson (mike, admin) on GURU-5070.

Prioritized Findings

[SEVERITY: CRITICAL] transport/websocket.rs:44-66 (and cross-references: main.rs:389 (support_code path), config.rs:441 (default), session/mod.rs:119 (connect call), viewer/transport.rs:65 (viewer token), install.rs:462 (protocol URL token), enroll.rs:195 (body but related))

Problem: Authentication material (api_key (post-enroll cak_), support_code, machine_uid, hostname, agent_id) is placed directly into the WebSocket connection URL as query parameters (agent_id=...&api_key=...&support_code=...&machine_uid=...). This is done before connect_async. Even over wss://, query strings are commonly logged (server access logs, reverse proxies, CDNs, browser history/debug tools, Referer leakage). support_code is the binding secret for attended/temp sessions; cak_ (as api_key) is the long-lived per-machine operating credential. No post-TLS initial auth frame or subprotocol is used.

Concrete fix: Perform the unauthenticated WS connect (or use a distinct unauth bootstrap), then immediately send the first protobuf Message carrying credentials (or a signed token) as binary payload. Alternatively, use a WebSocket subprotocol for auth handshake or HTTP upgrade header (where supported). Remove all secrets from the URL. Update all call sites and the server contract.

[SEVERITY: CRITICAL] transport/websocket.rs:104-143 (try_recv) + 147-169 (recv); session/mod.rs:385-392 (call site in main loop); viewer/transport.rs:27-60 (similar pattern)

Problem: try_recv does tokio::task::block_in_place(|| Handle::current().block_on(async { ... timeout(..., stream.next()) })) (and recv does direct block_on under lock). This is called from the main session loop (run_with_tray) on every iteration and from viewer receive task. Anti-pattern in async (especially current-thread or work-stealing runtimes); risks thread starvation, deadlocks under contention, or breaking cooperative scheduling. The 1ms timeout poll + VecDeque buffering compounds it. connected flag is racy with the stream lock.

Concrete fix: Use purely async non-blocking patterns (e.g., tokio::time::timeout directly on the locked stream without block_in_place, or a dedicated recv task that forwards over an mpsc channel to the session). Make WebSocketTransport fully async-friendly; expose a proper Stream or async recv only.

[SEVERITY: HIGH] session/mod.rs:625 (handle_consent_request) + 602-656 (the await + comment) + 337-359 (main loop) + 441-454 (heartbeats only in Idle)

Problem: handle_consent_request does spawn_blocking for the MessageBoxW (correct for blocking UI) but the caller .awaits it directly inside the main run_with_tray loop. During the entire user think-time (up to server CONSENT_TIMEOUT_SECS ~60s), the loop is suspended: no heartbeats, no status, no try_recv processing, no tray polling, no service_shutdown checks. Comments acknowledge this but claim it's "safe" because of server timeouts; the loop also does not send heartbeats while in the consent path (only Idle/Streaming branches do). A slow/denied consent can cause server-side timeout of the agent or missed stop signals.

Concrete fix: Drive consent off the main loop (e.g., spawn a task that owns the transport send for the response only; keep the loop pumping heartbeats/status/service flag via a separate small state machine or channel). Or restructure so consent does not block the pump.

[SEVERITY: HIGH] config.rs:264-282 (extract_support_code) + 248-252 (RunMode::TempSupport) + 369-372 (main) + session/mod.rs:782 + 839 (is_support_session) + transport/websocket.rs:57-59 (passes in query) + main.rs:301-304 (filename detection)

Problem: 6-digit numeric support codes are extracted from the executable filename (split on -/_/. or last 6 chars) with no checksum, expiry, or strength check in the agent. The code is then passed verbatim in the WS URL query for session binding. Filename-based secrets are easily guessable/brute-forceable (especially if server has no strong rate limiting or short TTL on codes) and leak via process listings, installers, or shared binaries. No binding proof (e.g., HMAC) is added client-side.

Concrete fix: Treat support codes as high-entropy one-time secrets only (server-enforced); do not derive from or embed in filenames for production attended sessions. Add client-side length/charset validation and consider a short MAC over the code + nonce if the wire protocol allows. Document that binding/enforcement is server-side only.

[SEVERITY: HIGH] main.rs:548-554 + 570-575 (LoadCakError paths in resolve_agent_credential) + credential_store.rs:169-195 (load_cak) + 109-154 (store_cak with C1 readback) — good intent but ...; cross with service/mod.rs:414 (run_managed_agent_service) and main.rs:482 (run_permanent_agent_managed)

Problem: The detailed LoadCakError classification (Io permission_denied vs. Decrypt tamper vs. plain Io) and fail-fast "refuse to re-enroll" logic is excellent and correctly wired for the SYSTEM service path. However, on the interactive PermanentAgent first-run path (run_permanent_agent_managed), if the service install fails (not elevated), it falls back to run_agent_mode(None) which will hit the same "store present but unreadable" or "no credential" error and can still attempt enrollment in some flows. The ACL is applied via icacls Command (credential_store.rs:345) after create_dir_all but before secret write (good TOCTOU ordering), yet any failure in icacls (localized SIDs, path quoting, policy) silently leaves a weaker store. store_cak does immediate C1 readback, which is strong.

Concrete fix: Make the interactive managed fallback path also refuse enrollment if a store exists but is unreadable in-context (surface the exact LoadCakError message). Add verification that the post-icacls ACL actually denies non-admins (test read as low user) or at least log the raw icacls stdout on failure. Consider a pure-Win32 ACL path for the directory as belt-and-suspenders.

[SEVERITY: MEDIUM] encoder/h264.rs:368-375 (ProcessInput) + 310 (ProcessOutput) + 236-252 (reinit) + many unsafe blocks; decoder.rs:176-204 (similar NOTACCEPTING/NEED_MORE_INPUT/ STREAM_CHANGE dance) + 278-297 (negotiate loop with negotiated guard); encoder/mod.rs:105-132 (factory fallback)

Problem: The H.264 MF paths are heavily unsafe COM with manual streaming state (streaming, stream IDs, force_keyframe). The first-cut synchronous drain/retry logic for MF_E_NOTACCEPTING / MF_E_TRANSFORM_NEED_MORE_INPUT / STREAM_CHANGE / TYPE_NOT_SET is complex and has a "negotiated" guard to avoid infinite loops, but a single missed state transition, dimension change mid-stream, or MFT that behaves differently on real hardware can drop frames or panic/unwrap in the unsafe paths. Encoder factory falls back to raw on init error (good), but per-frame errors in encode surface and can stall the session loop (session/mod.rs:552: if let Ok(encoded) = ... silently drops). No sequence number or integrity on the wire frames beyond protobuf.

Concrete fix: Add defensive checks (e.g., never trust MFT-reported sizes without validation against capture; bound the drain loops). Make H.264 failures increment a counter and force a codec renegotiation or hard switch to raw after N consecutive errors instead of per-frame silent drop. Add end-to-end frame sequence + checksum (or rely on transport) for wire integrity. Validate all unwrap_or / expect in the MF paths.

[SEVERITY: MEDIUM] session/mod.rs:153-187 (panic::catch_unwind around capture::primary_display / create_capturer / encoder / InputController) + 551 (capture in loop); capture/dxgi.rs:332 (Drop ReleaseFrame) + gdi.rs (manual ReleaseDC etc.); many .ok() on Release

Problem: Capture/encoder/input init is wrapped in catch_unwind to prevent a bad driver/DXGI from taking down the whole agent (good pragmatism), with GDI fallback. However, the runtime capture path (capturer.capture()) is not protected the same way; a panic there drops the frame but the ? / if let in the loop can leave state inconsistent. DXGI/GDI Drops do best-effort .ok() ReleaseFrame / DeleteObject etc.; a prior Acquire without matching Release (or double-release on error paths) can leak or crash the device. last_frame in DxgiCapturer is dead.

Concrete fix: Wrap the per-frame capture() + encode() in the streaming branch with a similar catch_unwind + "mark capturer bad, fall back or idle" recovery. Make resource release strict (e.g., use scopeguard or ensure ReleaseFrame is called exactly once per successful Acquire, even on early returns). Remove dead last_frame.

[SEVERITY: MEDIUM] config.rs:290-348 (read_embedded_config) + 314 (rposition for MAGIC_MARKER) + 325 (length from bytes) + 338 (serde_json::from_slice); 383 (load priority); main.rs:300 (has_embedded_config check)

Problem: Executable is mutated by appending GURUCONFIG + u32 LE length + JSON. The reader seeks from end (last 64KB), does rposition, then trusts the length field and does an unchecked slice + from_slice. A malicious or corrupted appended blob (truncated length, huge length, or JSON that deserializes to attacker-controlled server_url + enrollment_key) can cause the agent to connect to an attacker relay or exfiltrate the machine. No signature, no upper bound on length beyond the search buffer, no validation that the marker is in the expected "appended" region (post-PE).

Concrete fix: Add a strict max size for embedded config (e.g., 16KB). After locating the marker, validate that the config_start is after the PE end (or at least sanity-check the JSON fields: server_url must be wss/https to a known allowlist, enrollment_key length/format). Prefer an embedded signature (ed25519 over the config blob) verified at load time before trusting any enrollment material or server_url.

[SEVERITY: MEDIUM] identity.rs:249-331 (run_powershell + CIM queries for machine_uid) + 192 (primary_disk_serial script) + 221 (query_cim_property); 289 (kill on timeout)

Problem: machine_uid (used for enrollment dedup + connect identity) is derived by shelling out to powershell.exe -Command "..." (hardcoded scripts, no user data interpolated — good) with a 10s wall-clock timeout + kill. Output is captured but never logged (good). However, the scripts rely on CIM/WMI providers that can be slow, disabled, or return localized data; primary_disk_serial parses Index<TAB>serial with splitn. A wedged provider or unexpected output can cause fallback to persisted_uid (random + file). On re-image the MachineGuid path is intentionally volatile (documented), but the whole path is used for server-side collision/revocation decisions.

Concrete fix: Add stronger output validation (e.g., serials must look like plausible hardware IDs, not empty after normalize). Consider a pure-Rust or narrow WMI binding for the two scalar reads to remove the shell dependency entirely. Cache failures explicitly and surface "machine_uid used fallback (see logs)" in AgentStatus so operators know the dedup signal is weak.

[SEVERITY: MEDIUM] update.rs:163-184 (verify_checksum) + 318 (perform_update) + 189 (TODO comment) + 63 (dev_insecure_tls in download); same pattern in enroll.rs:285-304 + 296 (dev_insecure_tls)

Problem: Update integrity is a SHA-256 checksum delivered over the same channel as the binary (explicitly noted in comments as "transport integrity, not tamper defense"). dev_insecure_tls (debug + env only) disables cert verification for the version check + download. On success the old binary is renamed to .old and new copied in place while running; restart is detached. No signature verification (the TODO acknowledges this). A compromised/evil server (or MITM when insecure) can serve a matching checksum + malicious binary.

Concrete fix: Add (or implement the TODO for) an embedded public key + signature over the manifest + binary (or at minimum over the checksum + version). Keep the TLS bypass strictly debug-only and consider removing the env escape hatch. Verify signature before install_update.

[SEVERITY: LOW] (multiple files) pervasive #[allow(dead_code)], unwrap_or_default, best-effort .ok(), and incomplete paths (chat, multi-display dirty rects, VP9/HEVC, X buttons, etc.); config.rs:442 ("dev-key" default); hard-coded prod URLs in several places.

Problem: Indicates incomplete implementation surface. Dead paths + silent ignores can hide bugs. The dev-key default in env fallback is a footgun if a managed config load fails.

Concrete fix: Audit and either implement or remove the dead_code items. Remove or guard the "dev-key" default behind a stronger "debug build only" + explicit warning. Centralize server URLs.

Overall Assessment

The reviewed agent code demonstrates strong awareness of the Windows security model (SYSTEM service for SAS + managed agent, DPAPI+ACL credential store with readback verification + TOCTOU ordering, explicit consent for attended sessions via topmost MessageBox, tiered SAS delivery, panic-catch around risky capture/encode init, modifier release on focus loss/blur, focus-gated global keyboard hook for system keys, re-image-stable machine_uid derivation with documented fallbacks, and detailed error classification for enrollment/credential paths). The protobuf wire protocol, idle-vs-streaming resource release, codec negotiation with hardware guard + raw fallback, and heartbeat/status discipline are all reasonable. Many sharp edges (COM lifetime, DXGI access lost, MF streaming back-pressure) have explicit handling or guards.

However, the implementation is still "first-cut / Task N" in several critical areas (H.264 is compile-verified only; many #[allow(dead_code)]; async/sync mixing is fragile). The review surface is entirely client-side; trust is placed heavily in the (unreviewed) relay for session binding, consent gating, revocation, and auth enforcement.

The single most important thing to fix: Move all authentication and session-binding material (api_key/cak_, support_code, machine_uid) out of WebSocket URL query parameters (transport/websocket.rs and all call sites) into a post-connect authenticated protobuf message (or proper WS subprotocol). This is the highest-leverage security win: it directly mitigates secret leakage that enables session takeover, unauthorized access, and impersonation across both persistent and attended flows, independent of TLS. Everything else (async hygiene, consent loop suspension, H.264 robustness, update signing) is important but secondary to not putting the crown jewels in the most commonly logged part of the connection.

All findings are derived strictly from the provided file contents.