guru-connect/reports/review-2026-06-05/SYNTHESIS-three-way.md

# GuruConnect — Three-Way Independent Review (Claude + Gemini + Grok)

**Date:** 2026-06-05
**Reviewers:** Gemini `gemini-3.1-pro-preview`, Grok 4.3, Claude (synthesis + cross-check)
**Scope:** server (42 Rust files), agent (34 Rust files), dashboard (66 TS/TSX files). Each model read the source via its own `read_file` tool (no embedding). Grok's server pass was split A/B because 42 files exceeded its read-file capacity.
**Raw reports:** `gemini-{server,agent,dashboard}.md`, `grok-server-a.md`, `grok-server-b.md`, `grok-agent.md`, `grok-dashboard.md` (same folder).

Findings where **both** external models independently agree are marked **[CONVERGENT]** — higher confidence. Claude's cross-checks/adjustments are in _italics_.

---

## The one cross-cutting theme: secrets in WebSocket URL query strings

This is the **single highest-signal finding** — flagged independently as CRITICAL on **all three surfaces**:

- **Agent → relay** (`agent/src/transport/websocket.rs`): `api_key`/`cak_`, `support_code`, `machine_uid` in the WS URL query. **[CONVERGENT — both models' #1 for the agent]**
- **Dashboard → relay** (`dashboard/src/features/sessions/JoinSessionModal.tsx` `buildViewerUrl`): the session-scoped **viewer token** (the actual remote-control capability) in `?token=`. **[CONVERGENT — both models' #1 for the dashboard]**

Query strings leak into reverse-proxy/relay access logs, browser history, and copy-paste. Over `wss://` the body is encrypted but the URL is routinely logged.

_Claude cross-check: this is partly **by current design** — `CLAUDE.md` security rules explicitly mandate "Viewer WebSocket: JWT token required in `token` query parameter." So this isn't an accidental bug; it's a spec decision both external models reject. **Recommendation: revisit that spec rule** — move auth to the first post-connect protobuf frame or `Sec-WebSocket-Protocol`. Server-side the viewer token is already hardened (minted, session-scoped, signature+purpose+`session_id`-binding checked, blacklist-checked on the WS plane — confirmed in `grok-server-b`), so the residual risk is purely the transport channel. Fix this once, on both planes._

---

## CRITICAL

| # | Area | Finding | Source |
|---|------|---------|--------|
| C1 | agent + dashboard | Secrets/tokens in WS query string (see theme above) | **[CONVERGENT]** Gemini+Grok, both surfaces |
| C2 | `server/src/api/downloads.rs` | Unauthenticated download endpoints: hardcoded prod relay URL + `"managed-agent"` default API-key fallback; support-download docstring says "embeds code" but only renames the file (temp sessions get no embedded config). | **[CONVERGENT]** Gemini-server (key), Grok-server-a (key + URL + docstring gap) |
| C3 | `server/src/api/downloads.rs` | `Response::builder()…body(…).unwrap()` on attacker-controlled `Content-Disposition` filename (`sanitize_filename` doesn't escape `"`/control chars) → **unauthenticated panic / worker DoS**. | Grok-server-a |
| C4 | `agent/src/transport/websocket.rs` | `block_in_place(|| Handle::block_on(...))` inside the main async session loop (and viewer recv) — async/sync mixing → thread starvation / deadlock risk under contention; racy `connected` flag. | Grok-agent |
| C5 | `agent/src/update.rs` | Auto-update verified by **SHA-256 only over the same channel** as the binary — no signature. A malicious/MITM relay can push SYSTEM-level code to the whole fleet. | **[CONVERGENT]** Gemini-agent (#1) + Grok-agent (HIGH) |

_Claude note on C5: code carries an explicit `TODO` acknowledging the missing signature, and there's a debug-only `dev_insecure_tls` escape hatch. Both models rate this top-tier; it's the highest-impact agent issue after the query-string transport._

---

## HIGH

| # | Area | Finding | Source |
|---|------|---------|--------|
| H1 | `server/src/api/auth.rs` (login/change_password) | **No rate limiting / lockout on the human login path**, despite sophisticated per-`(site_code,ip)` + timing-equalizer protection on the machine-enroll path. Unlimited online brute-force / credential stuffing. | **[CONVERGENT]** Gemini-server + Grok-server-a |
| H2 | bootstrap admin password | First-run writes the generated admin **plaintext password to `.admin-credentials`** in CWD, with an `info!` **log fallback** on write failure. | **[CONVERGENT]** Gemini-server + Grok-server-b (Grok's "most important") |
| H3 | `server/src/api/auth_logout.rs:116` | `revoke_user_tokens` is a **501 stub** but the comment claims partial behavior — an admin "revoke any user's tokens" endpoint that does nothing. | Grok-server-a |
| H4 | `server/src/auth/token_blacklist.rs:85` | `cleanup_expired` re-runs **full JWT signature verification** on every blacklisted token (stores whole tokens in RAM) instead of comparing a stored `exp`. | Grok-server-a |
| H5 | self-service role guard | Server **does not block self-role-demotion** (only self-_delete_ is blocked); the lock-out guard is **client-side only** in `EditUserModal.tsx`. | **[CONVERGENT]** Gemini-dashboard + Grok-dashboard (both cite the in-code server comment) |
| H6 | dashboard auth lifetime | JWT in `sessionStorage`, blindly attached as Bearer; **no `exp` handling, refresh, or idle timeout** — XSS/workstation compromise → long-lived privileged access + ability to mint per-session control tokens. | **[CONVERGENT]** Gemini-dashboard + Grok-dashboard |
| H7 | `agent/src/session/mod.rs` consent | Attended-session consent (`MessageBoxW`) is `.await`ed **inside the main loop** → for up to ~60s no heartbeats / status / stop-signal processing. | **[CONVERGENT]** Gemini-agent + Grok-agent |
| H8 | `agent/src/credential_store.rs` | `cak_` store ACL set by shelling out to bare `icacls` (PATH-search) from a SYSTEM context → LPE if `icacls.exe` is hijacked; silent weaker store on failure. | Gemini-agent (+ Grok-agent notes the silent-failure variant) |

_Claude cross-check on H2: the server **deploys on Linux** (Ubuntu 22.04 per `CLAUDE.md`), so the `0o600` chmod **is** effective and Grok's "Windows ACL is a no-op" framing doesn't apply to the deployed server. The real residual risk is the plaintext-on-disk + the `info!` log-fallback path. I'd keep this **HIGH**, not CRITICAL._

---

## MEDIUM (consolidated)

- **Weak password policy** — min length 8 only, no complexity/upper-bound/breach check. **[CONVERGENT]** (Gemini-server, Grok-server-a)
- **Input validation caps** — unbounded/uncontrolled `machine_uid`, `hostname`, `site_code`, password length on unauthenticated enroll; no control-char stripping before DB / rate-limit keys. (Grok-server-a)
- **Agent embedded-config trust** — `GURUCONFIG` blob (server_url + enrollment_key) appended to the EXE is read with an unchecked length field and `from_slice`, no signature/allowlist → attacker blob can repoint the agent to a hostile relay. (Grok-agent)
- **`support_code` derived from EXE filename** with no checksum/strength, then sent in the query string. (Grok-agent)
- **Audit-trail correctness** — `CONNECTION_REJECTED_*` events insert a random `session_id` (dangling vs `connect_sessions`) instead of `NULL` like the enrollment/removal paths. (Grok-server-b) _Ties into the earlier in-product audit assessment: the write path works but has rough edges._
- **Panic sites on response/header construction** — `prometheus_metrics` `registry.lock().unwrap()`/`encode().unwrap()`; `security_headers` `from_static(...).unwrap()`. (Grok-server-b)
- **Declared-but-missing viewer input rate limit** — `VIEWER_INPUT_EVENTS_PER_SEC = 200` constant + comment promise coalescing/drop, but the path is a plain `mpsc(64)` with only backpressure; **[CONVERGENT-ish]** Gemini-server raised the same aggregate-flood concern. (Grok-server-b, Gemini-server)
- **Non-atomic user update** — dashboard does `updateUser` then `setUserPermissions` as two calls; partial-failure leaves drift. **[CONVERGENT]** (Gemini-dashboard, Grok-dashboard) → wants a single transactional `PATCH /api/users/:id`.
- **Hand-maintained TS type mirrors** of Rust API structs → silent type drift. **[CONVERGENT]** (both dashboard reviews) → generate via `ts-rs`/`specta`.
- **Premature `clearSession()` on any `getMe` failure** (incl. network status 0) → flaky-connection logout loops. (Gemini-dashboard)
- **In-memory rate-limit/lockout state** — reset on every restart/deploy → attacker gets a fresh brute-force window. (Grok-server-b; Gemini-server LOW)
- **H.264 MFT robustness / capture resource release** — complex unsafe COM streaming-state machine, per-frame silent drops, best-effort `.ok()` releases (leak/double-release risk); per-frame `capture()` not under the same `catch_unwind` as init. (Grok-agent)
- **Error-body passthrough** — dashboard surfaces raw server error text in toasts → potential internal-detail leak. (Grok-dashboard)

## LOW / maintainability
- Redundant post-decode `exp` checks (`leeway=0`) in `jwt.rs`. (Grok-server-a)
- `set_client_access` stores client UUIDs without existence check (dangling refs). (Grok-server-a)
- Large `#[allow(dead_code)]` surface ("TODO(native-remote-control)", Phase 4) — risk that future activation enables un-reviewed paths. (Grok both server + agent)
- Audit-log table grows unbounded, no pruning/retention. (Gemini-server)
- `react-query` polling is aggressive with no `document.hidden` pause/jitter. (Grok-dashboard)
- `stubs.ts` dead scaffolding still in the barrel export. (Grok-dashboard)
- Duplicated error envelopes (`ErrorResponse` vs `ApiError`) + repeated role allow-lists across handlers. (Grok-server-a)
- Deprecated `document.execCommand("copy")` clipboard fallback. (Gemini-dashboard)

---

## What all three reviewers independently praised (don't regress these)
Argon2id + timing-equalizer on the unauthenticated enroll path; fully parameterized sqlx (no SQLi surface); per-agent `cak_` identity binding that stops a client from seizing another machine's session; atomic single-use support-code consumption at bind time; trusted-proxy-aware `client_ip` extractor as the single root for rate-limit + audit; consent gate before any viewer join/stream; reattach/supersede/reap TOCTOU hardening via recheck-under-write-lock; bounded WS message sizes on both planes; minted session-scoped viewer tokens with blacklist + session-binding checks; DPAPI + ACL credential store with readback verification; thoughtful one-time-secret UX (codes/keys/passwords) and dialog focus management on the dashboard.

---

## Recommended fix order (Claude)
1. **C1 / query-string secrets** — one architectural change, both planes; revisit the `CLAUDE.md` viewer-token-in-query rule. Highest leverage.
2. **C2 + C3** — harden the unauthenticated `downloads.rs` surface (remove default key + hardcoded URL, kill the `.unwrap()` panic, fix support-embed gap). Cheap, high exposure.
3. **H1** — add login rate-limit/lockout (reuse the enroll limiter). Cheap, high value.
4. **C5 / update signing** — embed a public key, sign the update manifest+binary. Fleet-wide RCE prevention.
5. **C4 + H7** — fix agent async/sync mixing and move consent off the main loop.
6. **H2, H3, H4, H5, H6** — bootstrap-secret handling, finish/remove the logout stub, cheap blacklist cleanup, server-side self-demotion guard, dashboard token lifetime.
7. MEDIUM/LOW as capacity allows; prioritize embedded-config signing and the audit `session_id`-NULL fix.