docs(review): three-way (Claude+Gemini+Grok) codebase review + remediation plan

Independent reviews of server/agent/dashboard by Gemini 3.1 Pro and
Grok 4.3, each reading source via its own read_file tool, plus a Claude
synthesis and a phased remediation plan. Top convergent finding:
secrets/tokens in WebSocket URL query strings across agent + dashboard.
See SYNTHESIS-three-way.md and REMEDIATION-PLAN.md.

reports/review-2026-06-05/REMEDIATION-PLAN.md

@@ -0,0 +1,110 @@
+# GuruConnect — Remediation Plan (from the 2026-06-05 three-way review)
+
+Derived from `SYNTHESIS-three-way.md` (Claude + Gemini + Grok). Findings are grouped into
+shippable phases. Each phase is sized to a GuruConnect spec and should go through the
+standard pipeline (spec → three-way design review → build → three-way code review → deploy →
+validate). Phase numbers are priority order, not strict dependency order — P0 and parts of
+P4/P5 can run in parallel with the larger P1/P2/P3 efforts.
+
+Effort key: **S** = <0.5 day, **M** = 0.5–2 days, **L** = >2 days / multi-component.
+Finding IDs (C#/H#) reference the synthesis report.
+
+---
+
+## P0 — Emergency hardening pass (proposed SPEC-019)
+Small, self-contained, high-value. No protocol or cross-component coordination needed. Ship first.
+
+| Finding | File(s) | Action | Effort |
+|---|---|---|---|
+| **C2** | `server/src/api/downloads.rs` | Remove the `"managed-agent"` default API-key fallback and the hardcoded `wss://connect.azcomputerguru.com/...` URL; move both into `Config`/`AppState` and have the download handlers take `State`. Either implement real support-config embedding (MAGIC_MARKER + len + JSON, matching the permanent path) or correct the module docstring to stop claiming it. | M |
+| **C3** | `server/src/api/downloads.rs` | Replace every `Response::builder()…body(…).unwrap()` with a real error path (500 JSON). Reject/percent-encode (RFC 5987 `filename*=`) `Content-Disposition` values; cap input length before sanitizing. Closes an **unauthenticated panic/DoS**. | S |
+| **H1** | `server/src/api/auth.rs` (login, change_password) | Add rate-limit + failure-lockout on the login path, reusing the existing `state.rate_limits` machinery from the enroll path. Key on `(username.lower(), client_ip)`. Return 429 after threshold; record failures after constant-time work. | M |
+| **H2** | `server/src/main.rs` (bootstrap ~198–225) | Stop writing the generated admin **plaintext** to `.admin-credentials`; remove the `info!` log fallback. Prefer operator-supplied initial password via env/one-time flag (error on zero-user start if absent), or print once to stdout with a "copy now, never persisted" warning. _(Server deploys on Linux so 0o600 works — the residual risk is on-disk plaintext + the log path, not the Windows-ACL angle.)_ | S |
+| **H3** | `server/src/api/auth_logout.rs:116` | The `revoke_user_tokens` 501 stub claims partial behavior it doesn't have. Either delete the route until the session-tracking table exists, or implement the minimal real version and make the comment/error match reality. | S |
+| **H5** | `server/src/api/users.rs` + `auth` extractor | Enforce the **self-role-demotion / self-disable guard server-side** (currently only self-_delete_ is blocked; the lockout guard is client-only in `EditUserModal.tsx`). Mirror the last-admin/self guards already shipped in GuruRMM SPEC-027. | S |
+
+**Validation:** unauthenticated download fuzz (quotes/control chars in `company`/`site`), login brute-force returns 429, fresh-server bootstrap leaves no secret on disk/in logs, self-demote returns 4xx server-side.
+
+---
+
+## P1 — WebSocket auth handshake redesign (proposed SPEC-020) — the big one
+**C1 — the top convergent finding.** Secrets/tokens currently travel in WS URL query strings on
+both planes (agent: `api_key`/`support_code`/`machine_uid`; viewer: the session-scoped control token).
+
+This is partly **by current design** — `guru-connect/CLAUDE.md` Security Rules mandate
+"Viewer WebSocket: JWT token required in `token` query parameter." Both external models reject
+that. **First action: a design decision to change that spec rule.** Server-side the viewer token
+is already hardened (minted, session-bound, signature/purpose/`session_id` checked, blacklist on
+the WS plane) — only the transport channel is the problem.
+
+Workstream (L, cross-component — run a three-way *design* review before building):
+1. `proto/guruconnect.proto`: add an initial `AuthHandshake` message (agent creds / viewer token + binding).
+2. `server/src/relay/mod.rs`: accept the WS upgrade unauthenticated, then require the auth frame as the first message; fail-closed on timeout/missing. Keep all existing binding/blacklist/consent checks.
+3. `agent/src/transport/websocket.rs` (+ call sites: `session/mod.rs`, `viewer/transport.rs`, `install.rs`): connect, then send creds in the first protobuf frame. Remove secrets from the URL.
+4. `dashboard/src/features/sessions/JoinSessionModal.tsx` (`buildViewerUrl`) + `api/sessions.ts`: send the viewer token via the handshake; stop surfacing the full token-bearing URL as a copyable field.
+5. Update `guru-connect/CLAUDE.md` security rules to match.
+
+**Interim mitigation (until P1 ships):** make minted viewer tokens single-use-on-first-attach and shorten TTL; ensure the relay/NPM proxy never logs full request URLs.
+
+---
+
+## P2 — Update & embedded-config integrity (proposed SPEC-021)
+| Finding | File(s) | Action | Effort |
+|---|---|---|---|
+| **C5** | `agent/src/update.rs` | SHA-256 over the same channel is not tamper defense (the code's own TODO says so). Embed an Ed25519 public key; sign the update manifest + binary at build time; **verify signature before `install_update`**. Lock `dev_insecure_tls` to debug builds and consider removing the env escape hatch. | M |
+| **MED** | `agent/src/config.rs` (`read_embedded_config`) | Same crypto primitive: sign the appended `GURUCONFIG` blob (Ed25519) and verify before trusting `server_url`/`enrollment_key`. Add a strict max length, validate the marker sits after the PE end, and allowlist `server_url` scheme/host. Prevents an attacker blob from repointing the agent to a hostile relay. | M |
+
+Bundle these because they share the signing key infrastructure and the build-pipeline signing step. The Pluto build server / Gitea webhook pipeline will need a signing key (store in SOPS vault).
+
+---
+
+## P3 — Agent runtime correctness (proposed SPEC-022)
+| Finding | File(s) | Action | Effort |
+|---|---|---|---|
+| **C4** | `agent/src/transport/websocket.rs` | Remove `block_in_place(|| Handle::block_on(...))` from the main session loop and viewer recv. Use a dedicated recv task forwarding over `mpsc`, or async `timeout` directly on the locked stream. Fix the racy `connected` flag. | M |
+| **H7** | `agent/src/session/mod.rs` | Drive attended-consent off the main loop (spawn a task that owns only the response send) so heartbeats/status/stop-signals keep flowing during the ~60s think-time. | M |
+| **H8** | `agent/src/credential_store.rs:~309/345` | Call `icacls` by absolute path (`C:\Windows\System32\icacls.exe`) or use Win32 `SetNamedSecurityInfoW` directly; verify the resulting ACL actually denies non-admins (or log raw `icacls` output on failure) instead of silently leaving a weaker store. | S |
+| **MED** | `agent/src/encoder/h264.rs`, `decoder.rs`, `capture/{dxgi,gdi}.rs`, `session/mod.rs` | Wrap the per-frame `capture()`/`encode()` in the same `catch_unwind` + "mark capturer bad, fall back/idle" recovery used at init; make COM `ReleaseFrame`/`DeleteObject` strict (scopeguard, exactly-once per Acquire); after N consecutive H.264 errors force renegotiation/hard-switch to raw instead of per-frame silent drop; remove dead `last_frame`. | L |
+
+---
+
+## P4 — Auth/session lifetime & dashboard hardening (proposed SPEC-023)
+| Finding | File(s) | Action | Effort |
+|---|---|---|---|
+| **H4** | `server/src/auth/token_blacklist.rs:85` | Store `(token, exp)` and make `cleanup_expired` a pure time comparison instead of re-verifying every JWT signature. Capture `exp` at revoke time (cheap unsigned parse). | S |
+| **H6** | `dashboard/src/auth/AuthProvider.tsx`, `api/client.ts` | Parse/respect token `exp`; add proactive refresh-or-logout and an idle-activity timeout. Longer-term: move the JWT to an `httpOnly; Secure; SameSite=Strict` cookie managed by the server (needs a server-side cookie/refresh endpoint — coordinate with P1). | M |
+| **MED** | `dashboard/src/auth/AuthProvider.tsx` (`restore`) | Only `clearSession()` on an explicit `401`; on network (status 0) / 5xx keep the token and show an offline/retry state. Stops flaky-connection logout loops. | S |
+| **MED** | `server/src/api/users.rs` + `dashboard/src/features/users/hooks.ts` | Add a transactional `PATCH /api/users/:id` that updates core fields + permissions atomically; switch the dashboard off the two-call `updateUser`→`setUserPermissions` sequence. | M |
+
+---
+
+## P5 — Hygiene & maintainability (rolling, no dedicated spec)
+- **MED** Password policy: complexity + upper bound + breach-list (`zxcvbn`); raise min to 12. (`auth.rs`, `users.rs`, `enroll.rs`)
+- **MED** Input validation: cap + control-char-strip `machine_uid`/`hostname`/`site_code`/password on the unauthenticated enroll path before DB / rate-limit keys. (`enroll.rs`)
+- **MED** Audit correctness: `CONNECTION_REJECTED_*` events should insert `session_id = NULL` (like enrollment/removal), not a random UUID. Add `log_connection_rejected` or make `log_event` take `Option<Uuid>`. (`relay/mod.rs`, `db/events.rs`) — _also resolves the dangling-row issue noted in the earlier in-product audit assessment._
+- **MED** Generate dashboard TS types from the Rust API (`ts-rs`/`specta`) to kill type drift; interim: a snapshot-diff test. (`dashboard/src/api/types.ts`)
+- **MED** Wire the declared `VIEWER_INPUT_EVENTS_PER_SEC = 200` viewer-input rate limit (token bucket / coalesce latest mouse-move) or update the comment to match the backpressure-only reality. (`relay/mod.rs`)
+- **LOW** Remove panic sites on response/header construction (`prometheus_metrics` locks/encode, `security_headers` `from_static().unwrap()`). (`main.rs`, `middleware/security_headers.rs`)
+- **LOW** Document (or persist) that in-memory rate-limit/lockout state resets on restart; consider a bounded TTL table in Postgres for enroll/code-validate. (`rate_limit.rs`)
+- **LOW** Redundant post-decode `exp` checks; consider a small configurable leeway. (`auth/jwt.rs`)
+- **LOW** `set_client_access` should verify client UUIDs exist before storing. (`users.rs`)
+- **LOW** Dashboard polling: pause `refetchInterval` when `document.hidden`, add jitter/backoff. (`features/*/hooks.ts`)
+- **LOW** Remove `stubs.ts` from the barrel export; audit + delete or feature-gate the large `#[allow(dead_code)]` "native-remote-control"/Phase-4 surface as it's activated. (dashboard + server/agent)
+- **LOW** Standardize on the `ApiError` envelope server-wide; centralize role/permission allow-lists. (`api/*`)
+- **LOW** Dashboard `rowKey` on `m.id` while everything else keys on `agent_id` — standardize on `agent_id`. (`MachinesPage.tsx`)
+- **LOW** Treat unknown/plain-text server errors as opaque in the dashboard toasts (don't dump raw bodies). (`api/client.ts`)
+- **LOW** Drop the deprecated `document.execCommand("copy")` clipboard fallback. (`useClipboard.ts`)
+
+---
+
+## Suggested sequencing
+1. **P0** immediately (one hardening spec, ~2–3 days total, all small).
+2. Kick off the **P1** design review in parallel (it's the highest-impact and longest-lead).
+3. **P2** and **P3** next (agent-side; can overlap once P1 design is settled, since P1 also touches the agent transport).
+4. **P4** (some of it — httpOnly cookie — depends on P1's server auth changes).
+5. **P5** folded into whichever spec touches the relevant file, or a periodic cleanup pass.
+
+## Tracking
+Create coord todos (project_key `guruconnect`) for P0–P4 as parent items, with the individual
+findings as sub-tasks, plus Gitea issues on `azcomputerguru/guru-connect` for the CRITICAL/HIGH
+items so they're referenced from commits (`Fixes #N`).

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

@@ -0,0 +1,96 @@
+# 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.

reports/review-2026-06-05/files-agent.txt

@@ -0,0 +1,34 @@
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/bin/sas_service.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/capture/display.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/capture/dxgi.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/capture/gdi.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/capture/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/chat/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/config.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/consent/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/credential_store.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/encoder/capability.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/encoder/color.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/encoder/h264.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/encoder/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/encoder/raw.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/enroll.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/identity.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/input/keyboard.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/input/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/input/mouse.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/install.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/main.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/sas_client.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/service/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/session/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/startup.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/transport/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/transport/websocket.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/tray/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/update.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/viewer/decoder.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/viewer/input.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/viewer/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/viewer/render.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/viewer/transport.rs

reports/review-2026-06-05/files-dashboard.txt

@@ -0,0 +1,66 @@
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/App.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/auth.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/client.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/codes.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/index.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/machines.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/sessions.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/stubs.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/types.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/users.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/auth/AdminRoute.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/auth/AuthContext.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/auth/AuthProvider.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/auth/ProtectedRoute.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/layout/AppShell.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/layout/PageHeader.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/layout/Sidebar.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/layout/Topbar.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/layout/icons.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Badge.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Button.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/ConfirmDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Drawer.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Input.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Modal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Panel.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Spinner.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/States.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/StatusDot.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Table.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/TableSkeleton.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/dialogStack.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/status.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/toast-context.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/toast.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/auth/LoginPage.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/codes/CancelCodeDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/codes/GenerateCodeModal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/codes/SupportCodesPage.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/codes/hooks.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/BulkRemoveMachinesDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/DeleteMachineDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/KeyRevealModal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/MachineDetailDrawer.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/MachineKeysModal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/MachinesPage.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/RemoveMachineDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/hooks.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/sessions/EndSessionDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/sessions/JoinSessionModal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/sessions/RemoveSessionDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/sessions/SessionsPage.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/sessions/hooks.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/CreateUserModal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/DeleteUserDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/EditUserModal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/PasswordField.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/PermissionsField.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/UsersPage.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/hooks.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/password.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/lib/time.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/lib/useClipboard.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/lib/useRelayStatus.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/main.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/vite-env.d.ts

reports/review-2026-06-05/files-server-a.txt

@@ -0,0 +1,21 @@
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/auth.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/auth_logout.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/changelog.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/downloads.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/enroll.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/machine_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/releases.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/removal.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/sessions.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/sites.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/users.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/agent_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/enrollment_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/jwt.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/password.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/token_blacklist.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/viewer_token_registry.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/config.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/agent_keys.rs

reports/review-2026-06-05/files-server-b.txt

@@ -0,0 +1,21 @@
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/enrollment_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/events.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/machines.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/releases.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/sessions.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/sites.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/support_codes.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/tenancy.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/users.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/main.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/metrics/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/middleware/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/middleware/rate_limit.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/middleware/security_headers.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/relay/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/session/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/support_codes.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/utils/ip_extract.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/utils/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/utils/validation.rs

reports/review-2026-06-05/files-server.txt

@@ -0,0 +1,42 @@
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/auth.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/auth_logout.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/changelog.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/downloads.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/enroll.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/machine_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/releases.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/removal.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/sessions.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/sites.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/users.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/agent_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/enrollment_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/jwt.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/password.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/token_blacklist.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/viewer_token_registry.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/config.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/agent_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/enrollment_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/events.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/machines.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/releases.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/sessions.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/sites.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/support_codes.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/tenancy.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/users.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/main.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/metrics/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/middleware/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/middleware/rate_limit.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/middleware/security_headers.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/relay/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/session/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/support_codes.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/utils/ip_extract.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/utils/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/utils/validation.rs

reports/review-2026-06-05/gemini-agent.md

@@ -0,0 +1,25 @@
+# GuruConnect agent review — gemini — 2026-06-05
+
+This code review of the GuruConnect agent (v2/SPEC-016/SPEC-018) identifies several critical and high-severity findings related to session security, privilege escalation, and resource management.
+
+### Prioritized Findings List
+
+| SEVERITY | FILE:LINE | FINDING | CONCRETE FIX |
+| :--- | :--- | :--- | :--- |
+| **CRITICAL** | `websocket.rs:43` | **Insecure Auth via Query String.** API keys and JWT tokens are passed in the URL query string. These are often logged by reverse proxies (nginx/IIS) and visible in process monitors, leading to session takeover or credential theft. | Move credentials to the `Authorization` header or the Protobuf handshake payload. |
+| **CRITICAL** | `sas_service.rs:136` | **Command Injection in SYSTEM SAS Service.** The named pipe server reads a string, trims it, and matches it against `"sas"`. However, if the command protocol evolves or is misused, a lack of strict length/format validation on a SYSTEM-owned pipe is a high-risk surface for local privilege escalation (LPE). | Use a fixed-size byte enum or a proper Protobuf-serialized command structure for IPC. |
+| **HIGH** | `update.rs:197` | **Insecure Auto-Update (No Signature).** Updates are verified via SHA-256 checksum sent over the same channel as the binary. A MITM attacker can serve a malicious binary + matching checksum. | Implement RSA/Ed25519 signature verification using an embedded public key before `install_update`. |
+| **HIGH** | `credential_store.rs:309` | **LPE via `icacls` Subprocess.** The agent calls `icacls.exe` to secure the `cak_` store. If an attacker places a malicious `icacls.exe` in the search path (before the agent's working directory or in a user-writable path), the SYSTEM service will execute it. | Use the absolute path `C:\Windows\System32\icacls.exe` or use Win32 `SetNamedSecurityInfoW` API directly. |
+| **HIGH** | `session.rs:498` | **Session DoS via Blocking Consent.** `prompt_consent` runs on `spawn_blocking`, but the main loop `await`s it. While heartbeats might survive a 60s timeout, a user ignoring the box stalls all other control messages (like Disconnect or AdminRestart). | Run the consent prompt as a background task that sends the response to the server without stalling the main message-processing loop. |
+| **MEDIUM** | `main.rs:260` | **Service Run Panic Boundary.** `run_managed_agent_service` uses `catch_unwind` but runs inside a `tokio` runtime. Panics in spawned tasks or certain async contexts may still escape or cause the runtime to hang without triggering the SCM recovery. | Ensure the top-level `block_on` is wrapped, but also add a custom `PanicHook` to ensure the process exits cleanly so the SCM can restart it. |
+| **MEDIUM** | `sas_service.rs:46` | **Pipe Squatting/Race.** The SAS service pipe uses a hardcoded name and `PIPE_UNLIMITED_INSTANCES`. An attacker starting before the service could squat the name. | Use `FILE_FLAG_FIRST_PIPE_INSTANCE` when creating the first pipe instance to ensure the service owns the name. |
+| **LOW** | `config.rs:335` | **Config Exposure.** `EmbeddedConfig` contains `api_key` and `enrollment_key`. While it's in the binary, it's easily extractable via strings/binwalk. | The spec mentions `cek_` is low-sensitivity, but `api_key` (legacy) should be scrubbed from the binary after the first successful enrollment. |
+
+---
+
+### Overall Assessment
+The codebase is well-structured and follows the SPEC-016/018 requirements closely, particularly the DPAPI integration and the transition to a SYSTEM service. However, it relies heavily on "security by obscurity" in its transport (credentials in URLs) and lacks a robust trust anchor for auto-updates. The use of shell-outs (`icacls`, `powershell`) from a SYSTEM context is a significant LPE risk vector.
+
+### Single Most Important Thing to Fix
+**Implement Cryptographic Signature Verification for Auto-Updates.** 
+The current checksum-only approach allows any attacker capable of intercepting the WebSocket or API traffic (e.g., via a compromised relay or DNS) to gain SYSTEM-level code execution on the entire agent fleet by pushing a "mandatory" update.

reports/review-2026-06-05/gemini-agent.md.err

@@ -0,0 +1,3 @@
+[ask-gemini] file not found: agent/src/bin/sas_service.rs
+EXIT=2 files=34
+EXIT=0 files=34

reports/review-2026-06-05/gemini-dashboard.md

@@ -0,0 +1,46 @@
+# GuruConnect dashboard review — gemini — 2026-06-05
+
+This code review focuses on the provided TypeScript/React dashboard files for GuruConnect. The codebase is generally well-structured with a strong emphasis on accessibility and clean state management using TanStack Query. However, several high-priority security and correctness issues were identified.
+
+### Prioritized Findings
+
+1. **[SEVERITY: HIGH] Non-Atomic User Updates**
+   *   **File:** `3_hooks.ts:50` (in `useUpdateUser`)
+   *   **Problem:** Updating a user's core profile and their permissions involves two sequential API calls (`updateUser` then `setUserPermissions`). If the first succeeds but the second fails (e.g., network drop, server error), the user is left in an inconsistent state. The UI attempts to warn the user, but this does not prevent the underlying data drift.
+   *   **Fix:** Implement a single atomic `PATCH /api/users/:id` endpoint on the server that accepts both core fields and permissions in one transaction. Update the frontend to use this single call.
+
+2. **[SEVERITY: HIGH] JWT Exposure in SessionStorage**
+   *   **File:** `AuthProvider.tsx:16`
+   *   **Problem:** The authentication JWT is stored in `sessionStorage`. While this prevents persistence across browser restarts, it remains fully accessible to any script on the same origin. An XSS vulnerability anywhere in the dashboard would allow an attacker to steal the operator's session token.
+   *   **Fix:** Move JWT handling to an `httpOnly`, `Secure`, `SameSite=Strict` cookie managed by the server. Remove token management from the frontend entirely, letting the browser attach the cookie automatically.
+
+3. **[SEVERITY: HIGH] Client-Side Enforcement of Security Guards**
+   *   **File:** `EditUserModal.tsx:48`
+   *   **Problem:** The logic preventing an admin from demoting themselves (which would lock them out of the admin plane) is implemented strictly in the frontend. The code explicitly notes that "the server ... does NOT currently block a self-role-demotion." Relying on the client for authorization logic is a security risk.
+   *   **Fix:** Implement the self-demotion and self-disable guards on the server within the `AdminUser` extractor or the update handler. The server must be the final authority on these state transitions.
+
+4. **[SEVERITY: HIGH] Sensitive Tokens in URL Query Strings**
+   *   **File:** `JoinSessionModal.tsx:34` (in `buildViewerUrl`)
+   *   **Problem:** The session-scoped viewer token is passed to the relay via a WebSocket query parameter (`token=...`). URLs are frequently logged by reverse proxies, load balancers, and browser history, potentially leaking the short-lived but powerful viewer tokens.
+   *   **Fix:** Pass the token via the `Sec-WebSocket-Protocol` header or a custom `Authorization` header if the WebSocket client/relay supports it. If query params must be used, ensure the relay is configured to never log the full URL.
+
+5. **[SEVERITY: MEDIUM] Hand-Maintained API Type Mirrors**
+   *   **File:** `types.ts:1`
+   *   **Problem:** All API response shapes are hand-maintained mirrors of the Rust source. This is highly prone to "type drift," where a backend change (e.g., a field rename or type change) is not reflected in the frontend, leading to silent runtime failures or `undefined` values in the UI.
+   *   **Fix:** Use a tool like `ts-rs` or `specta` in the Rust backend to automatically generate TypeScript definitions from the actual API structs. This ensures the frontend and backend are always in sync.
+
+6. **[SEVERITY: MEDIUM] Premature Session Clearing on Network Failure**
+   *   **File:** `AuthProvider.tsx:55` (in `restore`)
+   *   **Problem:** During the initial app load, if the `getMe` call fails for *any* reason (including a transient network error with status 0), the `clearSession()` function is called, wiping the user's token. A technician on a flaky connection would be logged out repeatedly.
+   *   **Fix:** Only call `clearSession()` if the error status is explicitly `401 Unauthorized`. For network errors (status 0) or server errors (5xx), the app should keep the token and show a "Retry" or "Offline" state.
+
+7. **[SEVERITY: LOW] Deprecated Clipboard API Usage**
+   *   **File:** `useClipboard.ts:25`
+   *   **Problem:** The hook uses `document.execCommand("copy")` as a fallback for non-secure contexts. This API is deprecated and has inconsistent support in modern browsers.
+   *   **Fix:** Rely exclusively on the `navigator.clipboard` API. Since the dashboard should only be served over HTTPS, the "non-secure context" fallback is largely unnecessary.
+
+### Overall Assessment
+The GuruConnect dashboard is architecturally sound and leverages modern React best practices effectively. The UI components are robust and accessibility-conscious. However, the security model relies too heavily on client-side logic and `sessionStorage`, which are vulnerable points in a remote-access platform. The most critical area for improvement is moving security-sensitive state transitions (like user role management) and session handling (JWT storage and token transmission) to more secure, server-enforced patterns.
+
+### The Single Most Important Fix
+**Move the JWT to an `httpOnly` cookie.** This single change significantly hardens the platform against session hijacking via XSS, which is the most likely attack vector for a browser-based support console.

reports/review-2026-06-05/gemini-dashboard.md.err

@@ -0,0 +1,3 @@
+[ask-gemini] file not found: dashboard/src/App.tsx
+EXIT=2 files=66
+EXIT=0 files=66

reports/review-2026-06-05/gemini-server.md

@@ -0,0 +1,29 @@
+# GuruConnect server review — gemini — 2026-06-05
+
+I have completed an independent code review of the GuruConnect server based on the provided Rust source files. Below is the prioritized list of findings, categorized by severity, followed by an overall assessment and the most critical recommendation.
+
+### **Priority Findings List**
+
+| SEVERITY | FILE:LINE | FINDING | CONCRETE FIX |
+| :--- | :--- | :--- | :--- |
+| **CRITICAL** | `downloads.rs:142` | **Insecure Default API Key:** The permanent agent download endpoint uses a hardcoded default `"managed-agent"` API key if none is provided. This is a weak, well-known credential that grants any agent immediate persistent access if enabled on the server. | Remove the default fallback. Require a valid, high-entropy API key or a session-scoped enrollment key for all permanent agent downloads. |
+| **CRITICAL** | `main.rs:163` | **Sensitive Credentials in Local File:** On first run, the server writes the initial admin password to `.admin-credentials`. While it attempts to set `0o600` permissions, this file remains a high-value target for local privilege escalation or accidental exposure in backups/logs. | Provide the initial admin password via a one-time environment variable or secure secret manager. If a file must be used, ensure it is deleted automatically after the first successful login. |
+| **HIGH** | `main.rs:430+` | **Missing Rate Limiting on Admin CRUD:** Sensitive admin endpoints (`/api/users`, `/api/releases`, etc.) lack per-IP rate limiting. A compromised low-privilege account or a brute-force attack on the admin plane could cause DoS or data corruption. | Apply the `RateLimiter` middleware to all `/api/users`, `/api/releases`, and `/api/sites` management routes. |
+| **HIGH** | `removal.rs:104` | **TOCTOU in Machine Removal:** The legacy hard-delete path for machines performs a `get_machine_by_agent_id` lookup, then an optional `uninstall` command, then a `DELETE`. A machine could reconnect or be modified between the lookup and the delete, leading to inconsistent state or missed audits. | Wrap the removal logic in a database transaction and use `SELECT ... FOR UPDATE` or conditional `DELETE` statements (e.g., `WHERE agent_id = $1 AND updated_at = $2`) to ensure atomicity. |
+| **HIGH** | `relay.rs:664` | **Unbounded Input Queue in Viewer:** The viewer's input channel has a capacity of 64 messages. While `try_send` is used (dropping excess), a large number of viewers could still pressure the agent's inbound bandwidth if not throttled correctly at the broadcast level. | Implement a global per-session input rate limit (not just per-viewer) to protect the agent from aggregate input flooding by multiple technicians. |
+| **MEDIUM** | `auth.rs:320` | **Weak Password Policy:** The server only enforces a minimum length of 8 characters for new passwords. It does not check for complexity, common patterns, or use a "pwned password" list. | Integrate a password strength library (e.g., `zxcvbn`) and reject common passwords. Increase the minimum length to 12+ characters. |
+| **MEDIUM** | `jwt.rs:207` | **Lack of Token Refresh Mechanism:** The server relies on 24-hour JWTs with no explicit refresh token flow. This encourages long-lived sessions and increases the impact of a stolen token. | Implement short-lived access tokens (e.g., 15 mins) and long-lived refresh tokens stored securely (e.g., `HttpOnly` cookies). |
+| **MEDIUM** | `downloads.rs:188` | **Path Sanitization Limit:** `sanitize_filename` truncates at 32 characters but doesn't check for reserved Windows filenames (e.g., `CON`, `PRN`, `NUL`). | Use a more robust sanitization library or explicitly reject reserved Windows filenames to prevent issues on the client-side download. |
+| **LOW** | `rate_limit.rs:94` | **In-Memory Rate Limit Persistence:** Rate limit state is purely in-memory. A server restart clears all lockouts/counters, allowing an attacker to resume brute-forcing immediately. | (Optional) Periodically persist the `RateLimitState` to the database or use a fast-access cache like Redis for distributed/persistent rate limiting. |
+| **LOW** | `events.rs:252` | **Audit Log Growth:** The `connect_session_events` table grows indefinitely with every connection and admin action. There is no automated pruning or archival logic visible. | Implement a background task to prune or archive audit events older than a configured retention period (e.g., 90 days). |
+
+---
+
+### **Overall Assessment**
+The GuruConnect server demonstrates a strong focus on security fundamentals, particularly in its use of **Argon2id** for password/enrollment hashing, **SHA-256** for per-agent keys, and the implementation of a **session-scoped viewer token** model that successfully closes common relay-hijacking vulnerabilities. The architecture is modular and tenancy-ready, and the "fail-closed" approach to authentication (especially in the `agent_ws_handler`) is commendable.
+
+However, the presence of an insecure default API key in the download path and the reliance on local files for sensitive bootstrap credentials represent significant "low-hanging fruit" for attackers. The concurrency controls in the removal paths also show potential for TOCTOU races that could complicate auditing and state management in high-load environments.
+
+### **Single Most Important Thing to Fix**
+**Remove the default `"managed-agent"` API key fallback in `downloads.rs:142`.** 
+This is a "backdoor" by default; any permanent agent download without an explicit key results in a credential that an attacker can predict and use to attempt unauthorized machine registration if the server's `AGENT_API_KEY` environment variable happens to match this weak default. All credentials should be high-entropy and unique.

reports/review-2026-06-05/gemini-server.md.err

@@ -0,0 +1,3 @@
+[ask-gemini] file not found: server/src/api/auth.rs
+EXIT=2 files=42
+EXIT=0 files=42

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

@@ -0,0 +1,83 @@
+# 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 `.await`s 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.

reports/review-2026-06-05/grok-agent.md.err

@@ -0,0 +1,3 @@
+[ask-grok] file not found: agent/src/bin/sas_service.rs
+EXIT=2 files=34
+EXIT=0 files=34

reports/review-2026-06-05/grok-dashboard.md

@@ -0,0 +1,111 @@
+# GuruConnect dashboard review — grok — 2026-06-05
+
+**Independent code review: GuruConnect dashboard (TypeScript/React operator console)**
+
+All 66 files were read via `read_file` (absolute paths exactly as specified). Review is strictly limited to the provided files. No modifications were made. Backend (Rust relay, protobuf wire protocol, DB, JWT minting/claims, session manager, support-code redemption, SQL, agent auth) is out of scope; findings are restricted to what is observable or implied by the dashboard code + its API client.
+
+Findings are prioritized (CRITICAL → HIGH → MEDIUM → LOW). Format per request: `[SEVERITY: X], file:line, problem, concrete fix`. Focus areas were applied literally. Only concrete, evidenced issues are listed.
+
+### CRITICAL
+
+**[SEVERITY: CRITICAL], dashboard/src/features/sessions/JoinSessionModal.tsx:29-36**  
+The short-lived viewer token (which the relay uses to grant `control` or `view_only` access to a live remote session, binding to a specific `session_id`) is placed directly into the query string when building the WS viewer URL:
+
+```ts
+const qs = new URLSearchParams({
+  session_id: token.session_id,
+  viewer_name: viewerName,
+  token: token.token,   // !!!
+});
+return `${wsProto}//${window.location.host}/ws/viewer?${qs.toString()}`;
+```
+(See: buildViewerUrl, the effect that calls mintViewerToken, the "Relay viewer URL" FieldReveal copy target, JoinSessionModal:108-112 comments, sessionsApi.mintViewerToken, and the fact that the relay reads ViewerParams from query per the comment.)
+
+**Problem**: Query-string tokens appear in browser history, server access logs (relay + any reverse proxy), proxy logs, copy-paste artifacts, and can be bookmarked/shared. This is the capability that actually attaches a viewer to an agent's screen/stream. Even with short `expires_in_secs`, a leaked token for a live attended or managed session enables immediate session viewing or takeover. The client has no additional binding (e.g., origin, viewer IP) visible here.
+
+**Fix**: Move the token out of the query string. Preferred: pass via `Authorization: Bearer` header on the WS upgrade or a subprotocol (`Sec-WebSocket-Protocol`). If the current relay wire format requires query params for the viewer plane, (a) make the minted token one-time or single-use after first successful attach, (b) bind it to the caller's IP or a very short TTL (<< current), and (c) document the exposure + require operators to treat the entire URL as a single-use secret. At minimum, stop surfacing the full URL as a casually copyable field.
+
+**[SEVERITY: CRITICAL], dashboard/src/auth/AuthProvider.tsx:7 (STORAGE_KEY), 16 (sessionStorage.getItem + tokenRef), 66-73 (login), 42-64 (restore), 75-84 (logout), combined with client.ts:86-87 and all privileged API calls**  
+The primary dashboard JWT (used for `GET /me`, listing machines/sessions/codes, `POST /codes`, `POST /sessions/:id/viewer-token` (the control/view mint), admin user/machine operations, etc.) has no client-visible expiration handling, refresh, max lifetime, or idle timeout. It lives in `sessionStorage` (survives reloads within the tab lifetime) + an in-memory ref and is blindly attached as `Bearer` on every request.
+
+**Problem**: Compromise of the operator workstation (XSS in the console, malware, devtools exfil, physical access while tab is open, or shoulder-surf of a token during debugging) yields long-lived privileged access. Because `mintViewerToken` re-uses the same bearer, the attacker can also obtain per-session control/view tokens for any machine/session the account can see. Logout is best-effort; 401 is the only forced teardown. No `exp` parsing or proactive re-auth is present.
+
+**Fix**: Server should issue short-lived access tokens (or rotate the main token) + refresh tokens (or a refresh endpoint that requires the old token + rotation). Client must: (a) parse and respect `exp` (or an explicit `expires_in`) from LoginResponse, (b) proactively refresh or force logout before expiry, (c) add an idle/activity timeout that clears the session after N minutes of inactivity, and (d) consider binding the token to the browser tab lifetime more strictly. Treat the current long-lived bearer as equivalent to persistent remote access creds.
+
+### HIGH
+
+**[SEVERITY: HIGH], dashboard/src/features/codes/api/codes.ts:37-38 (cancelCode) + SupportCodesPage.tsx:21-24 (canCancel) + 114-136 (actions) + GenerateCodeModal + hooks**  
+Support code value (the `XXX-XXX-XXX` string read aloud to the end user, the single-use bearer for attended session start) is used as a path segment: `` `/api/codes/${encodeURIComponent(code)}/cancel` ``. The client also renders `code.session_id`, `client_machine`, `client_name`, `connected_at`, and consent state directly from the polled list response. No additional client-side binding verification or proof-of-possession step exists before offering cancel or trusting "connected" state.
+
+**Problem**: An attacker with a dashboard JWT (see CRITICAL auth finding) can enumerate active codes via `listCodes` and cancel pending/connected ones (DoS on support workflows) or observe which codes have bound to which sessions/machines. The code-to-session binding and single-use guard are entirely server-side; the dashboard is a pure reflection of whatever the `/codes` and `/sessions` responses claim. A server-side race or mis-binding between code redemption and session creation would be invisible to (and unmitigated by) the client.
+
+**Fix**: (Defense-in-depth on client) Treat the human code as a high-value secret in the UI: minimize how long it is rendered in clear, add explicit "this code is now bound to session X for machine Y — confirm before cancel" extra confirmation when status==="connected", and surface the exact `session_id` + consent_state with a warning on the cancel dialog. Server must remain authoritative for binding and single-use enforcement; client should not be the only thing preventing reuse.
+
+**[SEVERITY: HIGH], dashboard/src/features/machines/MachinesPage.tsx:56 (isAdmin), 228-250 (conditional Keys/Remove buttons), 380-396 (modals), UsersPage.tsx:42+161 (self checks), EditUserModal.tsx:92-120 (lockSelfDemotion + isSelf guards), AdminRoute.tsx:15-56, AuthContext.tsx:10+93 (isAdmin = role === "admin"), and equivalent hasPermission checks throughout**  
+All privileged UI affordances (bulk remove, per-agent key mint/revoke, user create/edit/delete, admin routes) are gated exclusively by client-side `isAdmin` (exact string compare on `user.role`) or `hasPermission(p)` (`user?.permissions.includes(p) ?? false`). These derive the `canControl`/`canView` passed to JoinSessionModal and the selection rails.
+
+**Problem**: These are only UI hints. A desync (role string casing, permission array containing non-canonical values, future role like "super-admin", or a bug in the AuthProvider memo) can show/hide actions incorrectly. Self-lockout guards (disable own account, demote self from admin) are client-only in the modal; the server comment in the code acknowledges it does not currently block self-demotion. While the server re-enforces (AdminUser extractor, 403s, self-delete 400), the client state is what the operator sees and what drives which buttons are even rendered.
+
+**Fix**: Keep the client gates but make them derive from a single, narrow, well-commented `can*` helper that exactly mirrors the documented server rules (or fetch effective capabilities from `/me` or a dedicated endpoint). Add explicit client-side rejection + toast for the self-demotion/disable cases even if the server would later 4xx. Widen the `Role | string` / `Permission | string` handling only after logging the unexpected value.
+
+### MEDIUM
+
+**[SEVERITY: MEDIUM], dashboard/src/api/client.ts:65-82 (extractError), 108-113 (401 path), 118-127 (success body handling) + callers that surface raw messages**  
+Error normalization handles two envelopes but falls back to raw `res.text()` (trimmed if <300 chars) for non-JSON and for certain machines routes (plain `&'static str`). Mutation error toasts (e.g. DeleteMachineDialog, BulkRemove, users hooks) directly render `err.message`.
+
+**Problem**: Server error bodies can leak internal details, hostnames, IPs (from events), or implementation strings into the operator UI and any logs/toasts that capture them. No redaction or classification of error text is performed before display. 401 after the handler still throws, which is mostly fine but means some error paths can double-surface.
+
+**Fix**: In `extractError` (or a post-processing step in the toast surfaces), treat unknown/plain-text errors as opaque. Prefix or categorize them ("Server error: ...") rather than dumping the body verbatim. Review the Rust error responses for machines/users/sessions/codes to ensure they never emit sensitive fields on failure paths.
+
+**[SEVERITY: MEDIUM], dashboard/src/features/machines/MachinesPage.tsx:372 (rowKey={(m) => m.id}), 66-88 (selected Set keyed by agent_id + reconciliation), 78-88 (useEffect that reconciles against live agent_ids), hooks.ts:12-18, api/machines.ts:18-19 (paths use agent_id), and Machine type (both `id` and `agent_id` present)**  
+React table keys use `m.id` while selection state, bulk operations, detail drawers, key modals, remove flows, filters, and all API paths consistently use `agent_id`. Reconciliation logic keys off `agent_id`.
+
+**Problem**: If `id` (DB/synthetic) and `agent_id` (agent-reported stable identity) can ever diverge in lifetime or uniqueness (soft-delete + re-enroll, purge paths, historical rows), React list reconciliation, selection survival across polls, and "remove selected" correctness become fragile. The bulk bar and dialogs already have careful visible-set logic; a key mismatch adds another surface for state skew.
+
+**Fix**: Standardize on the stable business key (`agent_id`) for `rowKey`, React keys, and selection. Keep `id` only for display or history joins if the server distinguishes them. Add a comment in types.ts and the page explaining the two identifiers.
+
+**[SEVERITY: MEDIUM], dashboard/src/features/users/hooks.ts:58-85 (useUpdateUser — two sequential awaits), 82 (onSettled always invalidates) + EditUserModal:122-134 (sendPermissions logic) + types.ts:99-104 (UpdateUserRequest)**  
+Update is split (core fields then optional permissions PUT). On permissions failure it throws a synthetic Error; `onSettled` (not `onSuccess`) invalidates.
+
+**Problem**: Partial success window exists (role/enabled changed, permissions not yet or failed). The error message rewrite is good, but an operator who closes the modal or navigates away during the window sees inconsistent state until the next refetch. No client-side transaction or "permissions pending" indicator.
+
+**Fix**: Either make the server accept a single combined update, or surface an explicit "core saved; retry permissions" state in the modal (keep it open, highlight the PermissionsField) instead of fully rejecting the mutation. The current `onSettled` + rewritten error is already better than most, but the UX for the partial case is still a toast + stale-looking row until poll.
+
+### LOW / Architecture & Maintainability
+
+**[SEVERITY: LOW], dashboard/src/api/stubs.ts:1-18 + api/index.ts:6 (export * as stubsApi)**  
+Stubs for sessions/users that return `unknown[]` are still exported under `stubsApi` (with comments "Pass 1", "do NOT build UI against these yet"). Real implementations live in sessions.ts / users.ts and are imported directly by features.
+
+**Problem**: Dead/exported-but-unused surface that can mislead readers or cause accidental import of the stub versions. Minor namespace pollution.
+
+**Fix**: Delete or clearly mark as `@deprecated / internal scaffolding` and remove from the barrel export. (Trivial.)
+
+**[SEVERITY: LOW], dashboard/src/features/*/hooks.ts (multiple): refetchInterval + staleTime hardcoded (machines 20s/10s, sessions 8s/4s, codes 7s/3.5s, health 15s), no `enabled: !document.hidden` or jitter, no error backoff**  
+Polling is aggressive and constant.
+
+**Problem**: Unnecessary load on the relay when the tab is backgrounded or the operator is idle on another page. No adaptive behavior.
+
+**Fix**: Add a small `useVisibility` or `document.visibilityState` guard to pause refetchInterval when hidden. Consider light jitter or a single shared "activity" signal. Keep the intervals tight for consent/watch use cases but make them configurable or at least documented.
+
+**[SEVERITY: LOW], dashboard/src/api/types.ts (entire file, especially ROLES/PERMISSIONS/ROLE_DEFAULT_PERMISSIONS, all the `| string` widenings, hand-maintained comment at top) + consumers that do exact string compares or `.includes`**  
+Types are manually kept in sync with server/src/api/*.rs. Many fields are widened to tolerate drift.
+
+**Problem**: Classic source-of-truth split. A new role/permission, a renamed field, or a new CodeStatus will either cause runtime surprises or require coordinated changes across Rust + this file. The defensive `| string` + filter-to-canonical in EditUserModal etc. mitigate but do not eliminate the maintenance tax.
+
+**Fix**: (Long-term) Generate the TS types from the Rust API (OpenAPI, prost, or a small schema export). In the interim, add a test or build step that diffs the TypeScript literals against a checked-in snapshot from the server.
+
+Additional minor notes (not elevated to findings):
+- `useClipboard` execCommand fallback is deprecated but acceptable for an internal tool in non-secure contexts.
+- No `dangerouslySetInnerHTML`; React text is used safely.
+- Dialog stack + focus trap + inert + restore in Modal/Drawer is correctly implemented and shared.
+- Password generation (Web Crypto + rejection sampling) + one-time copy + wipe-on-close is done well.
+- `encodeURIComponent` is consistently used on all path segments (agent_id, code, session id, user id, key id).
+- StrictMode double-mint guards in GenerateCodeModal and JoinSessionModal are present and correct.
+
+### Overall Assessment
+
+The dashboard is a clean, well-factored React + react-query SPA with strong separation between API client, typed feature hooks, and presentational components. UI/UX details around one-time secrets (codes, keys, passwords, viewer tokens), consent state, self-lockout, stacked dialogs, and partial-failure reconciliation are thoughtful and above average for an internal operator tool. The posture "client renders what the server says; server is the authority" is consistently followed (AdminRoute, permission checks, etc. are treated as hints + defense-in-depth).
+
+The code is skeptical of its own state in the right places (reconciliation effects, mintedFor refs, onSettled invalidation, `| string` widenings) but trusts the authenticated API responses for the security-critical bindings (code → session, viewer token access mode, consent_state, machine ownership).
+
+**Single most important thing to fix**: The CRITICAL placement of viewer tokens (the actual remote-control capability tokens) into query strings in `JoinSessionModal.buildViewerUrl`. This is the direct bridge from the operator console to live agent sessions. Everything else (auth lifetime, client-side gates, support-code exposure) is important but secondary to preventing easy leakage of tokens that let an observer or attacker attach to a victim's desktop/session. Fix the wire format or the mint+display path for these tokens first.

reports/review-2026-06-05/grok-dashboard.md.err

@@ -0,0 +1 @@
+EXIT=0 files=66

reports/review-2026-06-05/grok-server-a.md

@@ -0,0 +1,62 @@
+# GuruConnect server review — grok — 2026-06-05
+
+**All specified files have been read via the `read_file` tool (absolute paths, with follow-up offset reads for the two files whose initial reads were truncated by the default ~1000-line limit: `api/enroll.rs` and a no-op offset on the short `api/mod.rs`). No other files were read. No modifications were made (no writes, edits, terminal mutations, etc.). Review is strictly limited to the content of the 21 listed files.**
+
+### Prioritized Findings
+
+**[SEVERITY: CRITICAL], projects/msp-tools/guru-connect/server/src/api/downloads.rs:94 (and 101, 139, 146, 163, 190, 232, ~same pattern in viewer/support/agent paths)**  
+`Response::builder()... .header(CONTENT_DISPOSITION, format!("attachment; filename=\"{}\"", filename)) ... .body(...).unwrap()` (and equivalent for error bodies). `sanitize_filename` (236) only maps chars and truncates; it does not escape `"` or other header-special chars. A `company`/`site` query param containing `"` (or other values that make the header invalid) causes the builder to fail; the `.unwrap()` panics the handler task. These endpoints are unauthenticated/public.  
+**Concrete fix:** Never `.unwrap()` a `Response::builder()`. Use `axum::response::Response::builder()` with proper error path returning a 500 JSON/body. For `Content-Disposition`, either reject inputs containing `"`, `\`, or control chars before formatting, or use RFC 5987/8187 `filename*=` encoding (or `percent_encoding`). Add server-side length caps before embedding/sanitizing.
+
+**[SEVERITY: CRITICAL], projects/msp-tools/guru-connect/server/src/api/downloads.rs:169 (hardcoded), +169-179 (embedding), +107-149 (support path), +78-104 (viewer)**  
+`download_agent` hardcodes `server_url: "wss://connect.azcomputerguru.com/ws/agent"` and falls back to `api_key: "managed-agent"`. The top-of-file comment claims support downloads are "with embedded code"; the implementation only mutates the filename and serves the raw base binary (no `MAGIC_MARKER` + length + JSON append like the permanent path at 196-198). `download_*` handlers take only `Query`, no `State`/`AppState`/`Config`. Dev/staging builds will embed prod relay URLs; temp support sessions get no embedded config at all.  
+**Concrete fix:** Add the required server URL + default API key (and any other embed params) to `Config`/`AppState`. Make all three download handlers take `State`, read the values, and (for support) perform equivalent embedding of a support-oriented config (or a minimal `{code, server_url}`) using the same MAGIC format. Remove the hardcoded prod string. Update the module docstring.
+
+**[SEVERITY: HIGH], projects/msp-tools/guru-connect/server/src/api/auth.rs:41 (login), 205 (change_password), +86 (verify), no rate-limiter use anywhere in the file**  
+No rate limiting, progressive backoff, or account lockout on the primary password auth path (contrast with the sophisticated per-`(site_code, ip)` + timing-equalizer + failure recording in `enroll.rs:265`). Unlimited online brute-force / password stuffing is possible against any username. `get_user_by_username` + `verify_password` cost is paid on every attempt.  
+**Concrete fix:** Inject a rate-limiter (modeled on `state.rate_limits.enroll`) into `AppState`. Key on `(username.lower(), client_ip)` (or global + per-user). Record failures on bad password (and perhaps unknown user after constant-time work). Enforce in `login` (and consider `change_password`). Return 429 + opaque message after threshold. Add the same to the auth extractor or middleware for defense-in-depth.
+
+**[SEVERITY: HIGH], projects/msp-tools/guru-connect/server/src/auth/token_blacklist.rs:85 (`cleanup_expired`)**  
+`retain(|token| jwt_config.validate_token(token).is_ok())` re-runs full signature verification + claim parsing + the redundant post-decode `exp` check on every token in the `HashSet` (full JWT strings stored verbatim). Cleanup is admin-triggered only; a large backlog (many users, long 24h tokens) does expensive crypto work and keeps the whole token (claims + sig) in RAM until then.  
+**Concrete fix:** Change the internal storage to `(token: String, exp: i64)` (capture `exp` at `revoke` time from a cheap unsigned parse or from the minting site, since we only ever insert tokens we just validated). Cleanup then becomes a pure time comparison; no `validate_token` calls. (The existing `is_revoked` path still needs the string for the WS check.)
+
+**[SEVERITY: HIGH], projects/msp-tools/guru-connect/server/src/api/auth_logout.rs:116 (`revoke_user_tokens`)**  
+Handler is a complete stub: checks `is_admin()`, logs a warning that it is "NOT IMPLEMENTED", and unconditionally returns 501. The preceding comment claims "This currently only revokes the admin's own token as a demonstration" — the code does neither. The TODO describes exactly the missing session-tracking table. An admin endpoint documented for revoking other users' tokens does nothing.  
+**Concrete fix:** Either (a) delete the route + handler until the prerequisite table exists, or (b) implement the minimal version that at least revokes the caller's own token (or all currently-blacklistable artifacts for the target via the viewer registry + current blacklist) and make the comment + error message match reality. Do not leave a 501 admin "revoke anyone" endpoint that claims partial behavior.
+
+**[SEVERITY: MEDIUM], projects/msp-tools/guru-connect/server/src/api/downloads.rs:169 + enroll.rs:169 (and similar), config.rs:36**  
+`jwt_secret` and `database_url` are `Option` in `Config::load()` with silent fallbacks. `download_agent` never consults config at all. Several paths (login, mint viewer token, agent key issuance) will fail at runtime (or embed wrong values) if secrets are missing. No startup enforcement or `debug`-only relaxation.  
+**Concrete fix:** In `Config::load()`, require `JWT_SECRET` (and `DATABASE_URL` if not in a documented dev mode). Surface a clear error. Wire the resolved values into `AppState` and the download paths (see CRITICAL above). Consider a `require_jwt_secret` helper used by `JwtConfig::new` call sites.
+
+**[SEVERITY: MEDIUM], projects/msp-tools/guru-connect/server/src/api/* (multiple) + auth/* consistency**  
+Duplicated error shapes: `api/auth.rs` + `auth_logout` use `ErrorResponse { error: String }`; `machine_keys`, `enroll`, `sessions`, `sites`, `removal` define/use a richer `ApiError { detail, error_code, status_code }` (sometimes re-exported). Role/permission allow-lists are repeated verbatim in `users.rs` (122, 210, 339, 511). Boilerplate `db unavailable` / `map_err(500)` blocks appear in almost every handler.  
+**Concrete fix:** Standardize on the `ApiError` envelope everywhere (documented in `.claude/standards`). Extract small helpers (`require_db`, `map_db_err`, `require_admin` already partially exist). Centralize the `["admin","operator","viewer"]` + permission lists (or load from a const table). Use `thiserror` + `IntoResponse` for the common 4xx/5xx cases to kill the duplication.
+
+**[SEVERITY: MEDIUM], projects/msp-tools/guru-connect/server/src/api/enroll.rs:255 (and similar length checks), users.rs:133/378, auth.rs:271**  
+Only a minimum length (8) for passwords; no upper bound, no complexity, no control-char stripping on usernames/hostnames/machine_uid/site_code before DB or rate-limiter keys. `machine_uid` and `hostname` from unauthenticated enroll can be arbitrarily long (subject only to HTTP limits).  
+**Concrete fix:** Add reasonable caps (e.g. 128-256 chars) + sanitization (trim + reject controls / NUL) on all user-controlled identifiers early, before rate-limit keys or DB. Reject or truncate passwords >128. Make the checks consistent and return the standard `ApiError`.
+
+**[SEVERITY: MEDIUM], projects/msp-tools/guru-connect/server/src/api/sessions.rs:169 (`mint_viewer_token` registration) + auth_logout.rs:74 (take_for_user + revoke)**  
+The registry + blacklist cross-revoke for viewer tokens (addressing the "CRITICAL #2" scenario documented in comments and the test in `auth_logout.rs:231`) is only as good as the `sub` matching and the fact that the WS also calls `is_revoked` on the exact string. The mint path trusts `user.user_id` from the login JWT; no additional binding to the specific dashboard session that minted it. A compromised long-lived login JWT can still mint viewer tokens until it is revoked.  
+**Concrete fix:** (Defense-in-depth) Consider also stamping a `jti` (or short session id) from the login token into the viewer token claims (or the registry entry) and checking it on the WS side, or shortening the login JWT TTL + forcing refresh. At minimum, ensure the test coverage stays and that the registry prune + blacklist path is exercised on every logout.
+
+**[SEVERITY: LOW], projects/msp-tools/guru-connect/server/src/db/agent_keys.rs + api/machine_keys.rs:215 (key_belongs_to_machine)**  
+Good scoping (the revoke checks ownership via the FK before `revoke_agent_key`), but `keyed_machine_ids` (158) and the startup use are only in comments here. No visible index or query hint for the `revoked_at IS NULL` filter in hot paths.  
+**Concrete fix:** Ensure a partial index on `(machine_id) WHERE revoked_at IS NULL` (or equivalent) exists in migrations; the code already relies on it for the "active only" semantics.
+
+**[SEVERITY: LOW], projects/msp-tools/guru-connect/server/src/api/users.rs:574 (client_ids parsing) + 589 (set without existence check)**  
+`set_client_access` parses the supplied strings as UUIDs and stores them. No verification that the UUIDs actually exist in any clients table (or that the caller is allowed to see them). Silent creation of dangling references.  
+**Concrete fix:** After parsing, do a cheap `SELECT ... WHERE id = ANY(...)` (or equivalent) and reject the whole set if any are unknown (or at least log). This is the same pattern used for machine/site existence checks elsewhere.
+
+**[SEVERITY: LOW], projects/msp-tools/guru-connect/server/src/auth/jwt.rs:212 + 289 (post-decode exp checks) + 200 (validation config)**  
+Redundant `if claims.exp < now` after `validate_exp = true` + decode. `leeway = 0` is strict (good), but clock skew between dashboard and server is possible. Viewer tokens have a separate validator.  
+**Concrete fix:** Remove the redundant checks (or keep one with a tiny documented leeway for prod clock drift). Consider exposing a small configurable leeway.
+
+Other lower-severity notes (duplication of `get_user_permissions(...).unwrap_or_default()`, best-effort audit writes that can fail silently after the main action, `OptionalUser` and dead-code annotations for future native remote control, provisional collision heuristic in enroll with extensive comments, etc.) exist but are not prioritized above.
+
+### Overall Assessment
+The reviewed surface (auth layer, JWT + viewer tokens + logout revocation, per-agent + per-site enrollment keys, the public enroll path, admin CRUD for machines/users/releases/sites/keys, downloads, and supporting DB helpers) demonstrates careful, defense-in-depth thinking in several high-risk areas: Argon2id + timing equalization for the unauthenticated enroll path, explicit cross-site + collision gating with audits, parameterized queries everywhere (no SQLi surface), plaintext secrets returned exactly once and never logged, IDOR scoping on key revocation, tiered `control` vs `view_only` stamped inside signed viewer tokens, and the registry+blacklist mechanism to close the post-logout viewer token window. The code is heavily commented with references to prior audit findings ("CRITICAL #1", "CRITICAL #2", SPEC-016 items) and includes component tests that exercise the exact revocation paths.
+
+Weaknesses are primarily in the **unauthenticated download surface** (panic vectors + hardcoded prod config + docstring/implementation mismatch), absence of brute-force protection on the human login path (despite sophisticated protection on the machine enroll path), boilerplate and inconsistent error handling, and reliance on in-memory structures (full JWTs, viewer registry) whose cleanup/lifetime is only partially exercised in the provided code. Core relay/session-manager state, the actual WS handlers that consume viewer tokens, and the support-code issuance/matching logic live outside these files, so races, deadlocks, or binding integrity issues in the live control plane cannot be assessed from this slice alone.
+
+**Single most important thing to fix:** The downloads handlers (`download_viewer`/`support`/`agent`). They are unauthenticated, accept attacker-controlled query parameters directly into header construction and (for the agent path) into embedded config, perform no `State`-driven configuration, contain multiple `.unwrap()` panic sites on the response builder, and have a documented-vs-actual behavior gap for support sessions. A single malformed or malicious download request can panic a worker, serve binaries pointing at the wrong relay, or (if embedding is required for temp sessions) produce non-functional agents. This is a high-exposure, low-trust boundary that should be hardened first.

reports/review-2026-06-05/grok-server-a.md.err

@@ -0,0 +1 @@
+EXIT=0 files=21

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

@@ -0,0 +1,49 @@
+# 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.

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

@@ -0,0 +1 @@
+EXIT=0 files=21

reports/review-2026-06-05/grok-server.md

@@ -0,0 +1,2 @@
+# GuruConnect server review — grok — 2026-06-05
+

reports/review-2026-06-05/run-review.sh

@@ -0,0 +1,16 @@
+#!/usr/bin/env bash
+# run-review.sh <gemini|grok> <subsystem> <files-list> <out-file>
+set -uo pipefail
+TOOL="$1"; SUB="$2"; LIST="$3"; OUT="$4"
+ROOT="/d/claudetools"
+case "$TOOL" in
+  gemini) SCRIPT="$ROOT/.claude/skills/agy/scripts/ask-gemini.sh" ;;
+  grok)   SCRIPT="$ROOT/.claude/skills/grok/scripts/ask-grok.sh" ;;
+  *) echo "bad tool"; exit 2 ;;
+esac
+INSTR="You are doing an independent code review of the GuruConnect ${SUB} (a Rust/TypeScript remote-support/remote-control platform: agent <-> relay server <-> browser viewer, protobuf wire protocol, JWT auth, support-code session binding). Review ONLY the files provided. Produce a prioritized findings list. For EACH finding give: [SEVERITY: CRITICAL/HIGH/MEDIUM/LOW], file:line, the problem, and a concrete fix. Focus on: (1) security — auth/JWT, session takeover, support-code binding, input validation, SQL injection, secret handling, IDOR/authorization, rate-limit bypass; (2) correctness & concurrency — relay/session state races, deadlocks, panics/unwraps, error handling, resource leaks; (3) protocol/wire integrity; (4) architecture & maintainability. Be concrete and skeptical. End with a short overall assessment and the single most important thing to fix."
+mapfile -t FILES < "$LIST"
+echo "# GuruConnect ${SUB} review — ${TOOL} — 2026-06-05" > "$OUT"
+echo "" >> "$OUT"
+bash "$SCRIPT" review-files -i "$INSTR" "${FILES[@]}" >> "$OUT" 2>> "${OUT}.err"
+echo "EXIT=$? files=${#FILES[@]}" >> "${OUT}.err"