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

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(
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).