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

GuruConnect server review — grok — 2026-06-05

Code review complete. I used read_file on every listed absolute path exactly as specified and performed the review across all of them. No files were created, edited, or otherwise modified.

I reviewed only the provided files (GuruConnect server Rust sources for the relay, session manager, DB layer, auth-adjacent middleware, support codes, IP extraction, validation, metrics, and main wiring). Focus was strictly on the requested categories: (1) security (JWT/viewer tokens, session binding/takeover, support-code single-use + binding, authz/IDOR, input validation, SQLi, secrets, rate-limit bypass via IP extraction or otherwise); (2) correctness/concurrency (races in relay/session state, reattach/supersede/reap, lock ordering, panics/unwraps, error handling, resource leaks, consent gate); (3) protocol/wire integrity (bounds, decode paths, protobuf usage); (4) architecture/maintainability (defensive patterns, tenancy stubs, dead code, best-effort DB, config sensitivity).

All DB access uses parameterized sqlx::query* + .bind (no SQLi surface visible). No compile-time query! macros. Protobuf via prost with explicit size caps on WS upgrades. Viewer auth path (post-Task work) requires a session-scoped minted viewer token (not a raw login JWT), with signature/expiry/purpose validation, explicit blacklist check on the WS plane, and a claim session_id == requested session binding check that returns 403 on mismatch. Agent auth for cak_ (per-agent) keys performs identity binding (overrides client-supplied agent_id with the key's canonical machine agent_id; suppresses client machine_uid for keyed agents so it cannot override the key→machine binding). Support-code consumption is atomic under the in-memory write lock (consume_for_bind only from Pending) + best-effort durable DB conditional UPDATE (consumed_at IS NULL AND status='pending' AND (no expiry or future)). Reattach/supersede/reap use snapshot + remove_session_if recheck under the write lock to close TOCTOU. Consent gate (attended/support-code sessions) is enforced in join_session (!allows_viewer() returns None) and driven to completion in the agent handler before the main relay loop or StartStream. Rate limiting + lockouts (login, change-password, code-validate, enroll) are per-route, use the shared trusted-proxy-aware client_ip extractor, and include pre-handler lockout checks. Bounded WS message/frame sizes on both planes (4 MiB agent, 64 KiB viewer). Input flood constant declared (200/s) with "drop excess" intent.

Prioritized findings (only issues; no padding with "looks good" items). Lines are from the exact file contents returned by the tool calls. Paths use the server/src/... form for the crate layout (matching the listed absolute paths under projects/msp-tools/guru-connect/server/src).

• [SEVERITY: HIGH] server/src/main.rs:198-225 (admin bootstrap block) + 216-223 (fallback): On first run (no users), a 16-char random password is generated, hashed, and the plaintext is written to a file named .admin-credentials in the server's current working directory. The set_permissions(0o600) is inside #[cfg(unix)] only; on Windows (the reported environment) it is a no-op and the file has default ACLs. On fs::write failure the plaintext is emitted via info! (visible in logs). The file is left on disk for the operator to read once and delete. This is a high-privilege bootstrap secret persisted in an uncontrolled location with weak cross-platform protection and a log fallback path.

  Concrete fix: Do not write secrets to CWD files. Either (a) require the operator to supply the initial admin password via env/one-time flag/secret manager and error if none present on zero-user startup, or (b) print the password exactly once to stdout (with a strong "copy now" warning) and never persist it; remove the file-write path entirely. Apply Windows ACLs (e.g., via icacls or the windows crate) when the Unix path is taken. Never info! (or any log level) the generated password—only the "written to file" or "change immediately" guidance.

• [SEVERITY: MEDIUM] server/src/relay/mod.rs:144-157 and parallel sites (e.g. 203-216, 319-334): CONNECTION_REJECTED_* audit events (no-auth, invalid code, invalid API key) are emitted via db::events::log_event(..., Uuid::new_v4(), ...) (non-null session_id). This inserts rows into connect_session_events with a random session_id that has no row in connect_sessions (the FK is nullable, but these are semantically dangling). Contrast with log_enrollment_event / log_admin_removal which correctly pass NULL for session-less events. Pollutes the audit trail and complicates joins/queries that assume a valid session anchor.

  Concrete fix: Add (or extend) a log_connection_rejected helper (or make log_event accept Option<Uuid> for session_id and update its INSERT) and pass None for these pre-session auth failures. Keep the rich details JSON + ip_address. Update the callers in agent_ws_handler (and any viewer equivalents).

• [SEVERITY: MEDIUM] server/src/support_codes.rs:182-183 (validate_code) + 181 (the server_url field in CodeValidation): The public (rate-limited but unauthenticated) GET /api/codes/:code/validate preview always returns a hardcoded server_url: "wss://connect.azcomputerguru.com/ws/support" when the code is Pending or Connected. This path does not exist in the mounted routes (/ws/agent, /ws/viewer); the modern attended flow uses a minted session-scoped viewer token on /ws/viewer. Any consumer of the validation response (or docs/tests) gets a wrong target. Additionally, validate_code (preview) intentionally accepts both Pending and Connected; this is fine for preview but means a correctly guessed/high-entropy code that has already been bound will still return valid: true + the (wrong) URL + session_id until the code is completed/cancelled.

  Concrete fix: Remove server_url from CodeValidation (or make it a server-configured value derived from the incoming request or a single source of truth that matches the actual WS mounts and the viewer-token flow). Keep the preview behavior as-is (it must not consume), but consider returning a generic "already in use" or omitting the session_id for Connected codes if the preview is only meant to tell a technician "this code is claimable now."

• [SEVERITY: MEDIUM] server/src/main.rs:652-658 (prometheus_metrics) + server/src/middleware/security_headers.rs:49-51 (and similar .parse().unwrap() sites): registry.lock().unwrap() + encode(&mut buffer, &registry).unwrap(). A poisoned lock or encode failure panics the handler. In security_headers, multiple HeaderValue::from_static(...).unwrap() (or equivalent parse().unwrap()) on literals for CSP, X-Frame-Options, etc. A future edit that introduces an invalid literal turns every response into a panic.

  Concrete fix: Use if let Ok(lock) = ... or .lock().expect("registry poisoned") + graceful error response for metrics. For headers, use const HeaderValue or .expect("statically valid security header") / unwrap_or_else(|_| /* static fallback */) so construction cannot panic a live response.

• [SEVERITY: MEDIUM] server/src/rate_limit.rs:140,212,238,346 etc. (all the Mutex maps) + main.rs wiring + 97 (MAX_TRACKED_IPS): Rate limiters (RateLimiter, FailureLockout, EnrollLimiter) and lockout state are purely in-memory (Arc<Mutex<HashMap<IpAddr, ...>>> or composite (site_code, IpAddr)). Process restart (deploy, crash, OOM) clears all windows and active lockouts. The 15-minute code-validate and enroll lockouts therefore give an attacker a fresh window on every restart. Pruning at MAX_TRACKED_IPS and opportunistic window expiry exist, but persistence is absent. The client_ip extractor (trusted-proxies gate) is the single root for all of this plus audit; defaults are safe (loopback only) and the logic + tests are strong, but any misconfiguration of CONNECT_TRUSTED_PROXIES (or a future multi-hop proxy) bypasses the per-IP controls for the entire fleet.

  Concrete fix: Document the in-memory limitation explicitly (restarts reset brute-force windows). For the support-code and enroll surfaces, consider a small persistent store (bounded TTL table in the existing Postgres, or a sidecar) keyed the same way, or accept the risk and keep restart frequency low. Keep the trusted-proxy extraction as the single source of truth (already is) and add a startup warning + metrics for "number of trusted proxies" and "XFF used vs. direct."

• [SEVERITY: MEDIUM] server/src/relay/mod.rs:51 (VIEWER_INPUT_EVENTS_PER_SEC) + comment at top of file + visible input path (input_forward task, join_session returning raw InputSender, mpsc(64) in register_agent): The constant and comment state that viewer→agent input (mouse/key SendInput) is capped at 200 events/sec per viewer with excess "DROPPED (coalesced away)" to prevent a compromised viewer from flooding the target. In the provided code the path is a plain tokio::sync::mpsc channel (buffer 64) pumped verbatim by the spawned forwarder; no token bucket, per-second counter, or explicit drop/coalesce is visible at the point where viewers receive their InputSender or in the pump. Bounded buffer gives backpressure (senders slow down), but a burst up to the buffer depth (or sustained rate limited only by WS + agent processing) can still be injected.

  Concrete fix: Implement the declared policy where the InputSender is handed out (in the viewer connection handler, not shown in full but referenced) or in the forwarder: a small per-viewer (or per-session) rate limiter that drops or coalesces (e.g., keep latest mouse move) when over the cap. Wire the existing constant. If backpressure alone is intentional, update the comment to match reality and shrink the buffer.

• [SEVERITY: LOW] server/src/db/users.rs:238-269 (user_has_client_access) + callers marked dead_code: The function correctly implements "admin or (explicit client in list) or (zero restrictions = legacy all-access)". It does a get_user_by_id (for role) + exact match query + count query on every call. When multi-tenancy (Phase 4) activates and/or more surfaces use this for history/machine/session reads, any missed call site or path that takes a raw agent_id / session id from user input without going through an equivalent check is an IDOR. Several machine/session history endpoints and the client_access surfaces are still #[allow(dead_code)].

  Concrete fix: Extract a small can_access_machine / can_access_session helper (incorporating current_tenant_id() once Phase 4 lands). Prefer query-level filters (join or WHERE with the user's client list or "no restrictions" subquery) over N+1 post-filter in handlers. Add explicit tests for a restricted non-admin user being denied another client's machine history / session list.

• [SEVERITY: LOW] server/src/main.rs:589-611 (CORS layer) + 583 (security headers order): CORS is narrowly scoped (exact prod origin + localhosts, credentials true, limited methods/headers) — good. However, the layer ordering (security headers after some routes, TraceLayer, etc.) and the fact that the SPA fallback + /downloads nest + wildcard 404s exist means any future public asset or mis-mounted route could leak. CSP still includes 'unsafe-inline' for script/style (required for the current dashboard) and ws: wss:.

  Concrete fix: Keep the narrow allowlist. Consider moving to a nonce- or hash-based CSP for the SPA if feasible in a future iteration (reduces reliance on unsafe-inline). Ensure the wildcard any 404s (/api/*rest, /ws/*rest) stay in place so unknown API/WS paths never reach the SPA index.html.

• [SEVERITY: LOW] Multiple files (e.g. relay/mod.rs:575, session/mod.rs:428, releases.rs:23, many #[allow(dead_code)] for "TODO(native-remote-control)" and integration API): Large surface of functions/fields/types marked dead_code because the "native remote control" / full integration API is not yet wired. This is maintainability debt and increases the chance that a future change will activate insecure or un-reviewed paths (e.g. the older support-code mark paths that are superseded by the single-use consume, the is_code_valid etc. that are no longer on the hot bind path).

  Concrete fix: As the integration work lands, either delete the superseded functions or gate them behind a feature flag + explicit review. Reduce the number of too_many_arguments allows by introducing the params structs already referenced in the comments.

Overall assessment: The reviewed code has clearly undergone deliberate, defense-in-depth security work on exactly the high-risk areas for a remote-support relay (viewer token binding + revocation + session claim enforcement, per-agent cak_ identity binding that prevents client-supplied values from seizing another machine's session, atomic single-use support-code consumption at bind time with durable DB marker, trusted-proxy-aware IP extraction as the root for rate limiting + audit, consent gate before any viewer join or streaming, reattach/supersede/reap TOCTOU hardening via recheck-under-write-lock, WS message bounds on both planes, and soft-delete + filtered queries for operator purge). DB access is uniformly parameterized. Concurrency uses a consistent lock order (sessions before agents/machine_uids) and the remove_session_if helper. No obvious SQL injection, trivial session takeover, or missing authz on the active paths in these files. Weaknesses are primarily operational (secret handling for bootstrap), process-lifetime-only state for rate limiting, a few audit correctness nits, a hardcoded/wrong URL in a public response, and declared-but-not-fully-visible input rate enforcement. The large number of #[allow(dead_code)] + "Task N / SPEC-016 / Phase 4" comments indicate this is an evolving, partially implemented surface—future work must re-review the newly activated paths.

Single most important thing to fix: The bootstrap admin credential handling in server/src/main.rs:198-225 (and the log fallback). It places a high-privilege random password on disk in the process CWD with platform-incomplete ACLs and a path that can emit the secret in logs. For a remote-access control plane product this is the clearest high-impact, low-effort operational security gap in the reviewed files. Replace the file-write + log-fallback with an operator-supplied value or strict one-time stdout emission that is never persisted.