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/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.