Merge remote security fixes with local specs

agent/src/session/mod.rs

@@ -555,8 +555,14 @@ impl SessionManager {
             access
         );
 
-        // The MessageBox blocks the calling thread; run it on the blocking pool
-        // so the agent's async loop is not stalled and heartbeats keep flowing.
+        // The MessageBox blocks the calling thread, so it runs on the blocking
+        // pool to avoid stalling the tokio runtime. Note, however, that the main
+        // session loop `.await`s this method (see the ConsentRequest arm), so
+        // the loop is SUSPENDED for the user's entire think-time and does NOT
+        // process or respond to server heartbeats while the dialog is open.
+        // This is safe because CONSENT_TIMEOUT_SECS (60s, server-side) is within
+        // the server's 90s HEARTBEAT_TIMEOUT_SECS: the prompt resolves before the
+        // server would consider the agent dead, so the session is not torn down.
         let granted = tokio::task::spawn_blocking(move || prompt_consent(&technician_name, access))
             .await
             .unwrap_or_else(|e| {

agent/src/update.rs

@@ -10,6 +10,25 @@ use tracing::{error, info, warn};
 
 use crate::build_info;
 
+/// Whether to disable TLS certificate verification for update traffic.
+///
+/// Returns `true` ONLY in a debug build (`cfg!(debug_assertions)`) when the
+/// `GURUCONNECT_DEV_INSECURE_TLS` environment variable is set. The `cfg!` gate
+/// is compiled out of release builds, so a shipped agent ALWAYS verifies certs
+/// regardless of environment — a MITM cannot serve a forged update binary over
+/// an unverified channel. The env var lets a developer test against a
+/// self-signed server without weakening production.
+fn dev_insecure_tls() -> bool {
+    if cfg!(debug_assertions) && std::env::var("GURUCONNECT_DEV_INSECURE_TLS").is_ok() {
+        warn!(
+            "TLS certificate verification DISABLED (dev-insecure mode) — DO NOT use in production"
+        );
+        true
+    } else {
+        false
+    }
+}
+
 /// Version information from the server
 #[derive(Debug, Clone, serde::Deserialize)]
 pub struct VersionInfo {
@@ -42,7 +61,7 @@ pub async fn check_for_update(server_base_url: &str) -> Result<Option<VersionInf
     info!("Checking for updates at {}", url);
 
     let client = reqwest::Client::builder()
-        .danger_accept_invalid_certs(true) // For self-signed certs in dev
+        .danger_accept_invalid_certs(dev_insecure_tls())
         .build()?;
 
     let response = client
@@ -108,7 +127,7 @@ pub async fn download_update(version_info: &VersionInfo) -> Result<PathBuf> {
     info!("Downloading update from {}", version_info.download_url);
 
     let client = reqwest::Client::builder()
-        .danger_accept_invalid_certs(true)
+        .danger_accept_invalid_certs(dev_insecure_tls())
         .build()?;
 
     let response = client
@@ -134,6 +153,13 @@ pub async fn download_update(version_info: &VersionInfo) -> Result<PathBuf> {
 }
 
 /// Verify downloaded file checksum
+///
+/// NOTE: This is a transport-integrity check (catches truncated/corrupted
+/// downloads), NOT a tamper defense. The expected checksum arrives over the
+/// same channel as the binary, so an attacker who can serve a forged binary
+/// can also serve a matching checksum. Tamper resistance comes from verifying
+/// the TLS certificate of the update server (see `dev_insecure_tls`) and, as a
+/// future hardening step, an embedded-public-key signature over the artifact.
 pub fn verify_checksum(file_path: &PathBuf, expected_sha256: &str) -> Result<bool> {
     info!("Verifying checksum...");
 
@@ -160,6 +186,9 @@ pub fn verify_checksum(file_path: &PathBuf, expected_sha256: &str) -> Result<boo
 /// Perform the actual update installation
 /// This renames the current executable and copies the new one in place
 pub fn install_update(temp_path: &PathBuf) -> Result<PathBuf> {
+    // TODO(security): defense-in-depth — verify an embedded-public-key signature
+    // over the update binary/manifest before install_update; see
+    // reports/2026-05-30-gc-audit.md
     info!("Installing update...");
 
     // Get current executable path
@@ -321,4 +350,31 @@ mod tests {
         assert!(!is_newer_version("0.1.0", "0.2.0"));
         assert!(is_newer_version("0.2.0-abc123", "0.1.0-def456"));
     }
+
+    /// In a release build (`debug_assertions` off), `dev_insecure_tls()` MUST
+    /// return false regardless of the env var — the shipped agent can never
+    /// accept invalid certs. In a debug build, it returns true only when
+    /// `GURUCONNECT_DEV_INSECURE_TLS` is set; we cannot assert the env-var path
+    /// here without mutating process-global state (which would race other
+    /// tests), so we only assert the invariant that holds in the current
+    /// build profile.
+    #[test]
+    fn test_dev_insecure_tls_release_is_always_false() {
+        if !cfg!(debug_assertions) {
+            // Release/test-release profile: must be false no matter the env.
+            assert!(
+                !dev_insecure_tls(),
+                "release build must never disable TLS verification"
+            );
+        } else {
+            // Debug profile: with the env var unset, must still be false.
+            // (We avoid setting it to prevent cross-test interference.)
+            if std::env::var("GURUCONNECT_DEV_INSECURE_TLS").is_err() {
+                assert!(
+                    !dev_insecure_tls(),
+                    "debug build without the env var must verify TLS"
+                );
+            }
+        }
+    }
 }

reports/2026-05-30-gc-audit.md

@@ -0,0 +1,120 @@
+# GuruConnect Audit Report — 2026-05-30
+
+**Auditor:** Claude (claude-opus-4-8[1m])
+**Passes:** Security & Remote-Session Integrity (`--pass=security` only)
+**Previous audit:** 2026-05-29 (the audit that defined the v2 secure-session-core work)
+**Scope note:** Single-pass security re-audit to formally verify the v2 secure-session-core
+rebuild closed the three relay CRITICALs. Independent re-derivation against the code — not an
+echo of the 2026-05-30 Tasks 3–5 code review.
+
+---
+
+## Executive Summary
+
+| Pass | Total | Critical | High | Medium | Low | Info |
+|------|-------|----------|------|--------|-----|------|
+| Security & Session | 5 | 0 | 1 | 0 | 1 | 3 |
+
+**The three 2026-05-29 audit CRITICALs are CLOSED with no bypass.** The relay/server plane is
+clean. The one serious finding is **net-new and outside the relay plane** — the Windows agent
+auto-update path disables TLS verification (MITM → RCE).
+
+**Requires action:** [HIGH] Agent auto-update disables TLS cert verification → MITM-to-RCE.
+
+---
+
+## The three audit CRITICALs — verification
+
+### CRITICAL #1 — "any JWT joins any session" → **CLOSED (no bypass)**
+- `viewer_ws_handler` (`server/src/relay/mod.rs:463`) calls `validate_viewer_token`, which enforces
+  signature + expiry + `purpose=="viewer"` (`server/src/auth/jwt.rs:268,284`). A login JWT lacks the
+  `ViewerClaims` shape and is rejected (test `test_login_token_rejected_as_viewer_token`).
+- Session binding: `claim_session_id != requested_session_id → 403` (`relay/mod.rs:494-501`).
+- Mint authz: `mint_viewer_token` (`server/src/api/sessions.rs:114-131`) — `is_admin() ||
+  has_permission("control")` → Control; else `has_permission("view")` → ViewOnly; else 403.
+- View-only enforcement: access mode is in the **signed** claim; relay drops
+  `MouseEvent`/`KeyEvent`/`SpecialKey` when `!access.can_control()` (`relay/mod.rs:1313-1335`).
+
+### CRITICAL #2 — "viewer-WS blacklist bypass" → **CLOSED (no bypass)**, documented residual
+- `token_blacklist.is_revoked(&token)` consulted on the viewer WS path before upgrade
+  (`relay/mod.rs:476`).
+- **Residual [TRACKED — v2-secure-session-core plan.md, MEDIUM]:** logout
+  (`server/src/api/auth_logout.rs:64`) revokes only the login JWT; minted viewer tokens are not
+  blacklisted, so a leaked viewer token stays valid until natural expiry (5-min TTL,
+  `jwt.rs:56`). Known follow-up; the enforcement plumbing is correct for any future force-revoke.
+
+### CRITICAL #3 — "JWT accepted as agent key" → **CLOSED (no bypass), fails closed**
+- `validate_agent_api_key` (`relay/mod.rs:384`) has no JWT branch — only a per-agent `cak_` key
+  (`verify_agent_key`, `server/src/auth/agent_keys.rs:71`; SHA-256 vs `connect_agent_keys`,
+  `revoked_at IS NULL`) or the deprecated shared `AGENT_API_KEY` (WARNING-logged).
+- Identity binding fails closed: unresolved machine → 503 (`relay/mod.rs:276-286`); a client-supplied
+  `agent_id` is overridden by the key's machine identity (`relay/mod.rs:263-270`).
+
+---
+
+## Pass 5: Security & Remote-Session Integrity
+
+### [HIGH] Agent auto-update disables TLS certificate verification → MITM-to-RCE
+**File:** `agent/src/update.rs:45` and `:111` (live path: `agent/src/session/mod.rs:461-468,695-702`)
+**Detail:** Both the `check_for_update` and `download_update` reqwest clients are built with
+`.danger_accept_invalid_certs(true)` ("for self-signed certs in dev"). This is the **live** auto-update
+path — the agent session loop calls `check_for_update → perform_update → download_update →
+install_update → restart_with_new_version` at runtime. A network MITM (the relay endpoint is a public
+hostname) can serve an arbitrary `guruconnect-update.exe`; the agent writes it over its own binary and
+executes it — full code execution on every managed endpoint. `verify_checksum` (`update.rs:294`) does
+NOT mitigate: the expected SHA-256 (`version_info.checksum_sha256`) arrives in the **same** `/api/version`
+JSON over the unverified TLS connection, so the attacker controls both binary and checksum.
+**Recommendation:** Remove `danger_accept_invalid_certs(true)` from both clients (gate behind a dev-only
+`cfg`/env flag never set in release). Defense-in-depth: sign the update binary/manifest with an embedded
+public key and verify the signature before `install_update`, so update trust does not rest solely on TLS.
+
+### [LOW] Chat message content logged at INFO
+**File:** `server/src/relay/mod.rs:776` and `:1373`
+**Detail:** Full chat text is logged (`Chat from client/technician: {content}`). Support-session chat can
+carry sensitive data (spoken passwords, account numbers) — PII/secret-bearing content in server logs.
+**Recommendation:** Drop content from the log line (log length or hash, or move to `trace!`).
+
+### [INFO] Bootstrap admin password logged on credentials-file write failure
+**File:** `server/src/main.rs:200-201`
+**Detail:** Self-documented one-time fallback at first boot if `.admin-credentials` write fails; password
+is meant to be changed immediately. Awareness only, not a defect.
+
+### [INFO] Viewer-token mint not scoped to a specific machine/session ACL — **[TRACKED — SPEC-002 Phase 4]**
+**File:** `server/src/api/sessions.rs:27,114`
+**Detail:** Any `control`-permission user can mint a control token for any live session (no per-machine /
+per-client ACL). Intentional for Phase 1; per-tenant narrowing deferred to Phase 4.
+
+### [INFO] Verified-sound areas
+Single-use support codes (atomic `consume_for_bind`, `support_codes.rs:240` / `db/support_codes.rs:110`);
+consent gate (viewer refused until `allows_viewer()`, `session/mod.rs:393`; deny/timeout tears down before
+the relay loop, no pre-consent window); frame caps (agent 4 MiB / viewer 64 KiB, `relay/mod.rs:330,524`);
+input throttle (200 ev/s token bucket + non-blocking `try_send`); rate limiting wired + proxy-aware
+(`main.rs:317-370`, `utils/ip_extract.rs:147`); `JWT_SECRET` length-checked ≥32 at startup; Argon2id
+passwords; no secret/token/code logging; parameterized runtime sqlx (no `format!`-built SQL); `wss://` default.
+
+---
+
+## Verdict
+
+| CRITICAL | Verdict | Enforced at |
+|----------|---------|-------------|
+| #1 any-JWT-joins-any-session | **CLOSED** | `api/sessions.rs:114` (authz) + `relay/mod.rs:463,494` (token type + session bind) |
+| #2 viewer-WS blacklist bypass | **CLOSED** (TTL-bounded residual tracked) | `relay/mod.rs:476` |
+| #3 JWT-accepted-as-agent-key | **CLOSED** | `relay/mod.rs:384` (no JWT branch; fail-closed binding `:276`) |
+
+**Net-new findings:** CRITICAL 0 · HIGH 1 · MEDIUM 0 · LOW 1 · INFO 2.
+**Overall posture:** The relay/server plane is clean — all three CRITICALs genuinely closed, fail-closed
+session/consent/code paths, bounded abuse surface, correct proxy-aware rate limiting, no SQL injection or
+credential logging. The one serious net-new issue is on the Windows **agent** (auto-update TLS bypass →
+MITM-RCE), independent of the v2 secure-session-core work and in no existing spec — fix before the next
+agent release.
+
+---
+
+## Recommended Action Order
+1. **[HIGH]** Remove `danger_accept_invalid_certs(true)` from `agent/src/update.rs` (+ sign update binary).
+2. **[LOW]** Stop logging chat content (`relay/mod.rs:776,1373`).
+3. **[TRACKED/MEDIUM]** Viewer-token logout revocation (existing plan.md follow-up).
+
+*Note: only `--pass=security` was run. The API-surface, Rust-quality, TypeScript, protocol-integrity,
+docs-reconciliation, and CI/CD passes were not executed this run.*

server/src/api/auth_logout.rs

@@ -60,10 +60,27 @@ pub async fn logout(
     // Extract token from headers
     let token = extract_token_from_headers(request.headers())?;
 
-    // Add token to blacklist
+    // Add the login JWT to the blacklist.
     state.token_blacklist.revoke(&token).await;
 
-    info!("User {} logged out (token revoked)", user.username);
+    // Also revoke any outstanding session-scoped VIEWER tokens this user minted
+    // (CRITICAL #2). The login-JWT blacklist alone leaves a viewer token minted
+    // before logout valid for the rest of its 5-minute TTL, keeping a live
+    // viewer/remote-control plane open after logout. `user.user_id` is the `sub`
+    // the viewer tokens were registered under (the same claim stamped into them).
+    // The viewer WS already blacklist-checks the exact token string, so adding
+    // them here is sufficient — no WS change needed. take_for_user drains and
+    // clears the registry entry; expired tokens are pruned (not returned).
+    let viewer_tokens = state.viewer_tokens.take_for_user(&user.user_id);
+    let revoked_viewer_count = viewer_tokens.len();
+    for viewer_token in viewer_tokens {
+        state.token_blacklist.revoke(&viewer_token).await;
+    }
+
+    info!(
+        "User {} logged out (login token revoked, {} viewer token(s) revoked)",
+        user.username, revoked_viewer_count
+    );
 
     Ok(Json(LogoutResponse {
         message: "Logged out successfully".to_string(),
@@ -196,3 +213,74 @@ pub async fn cleanup_blacklist(
         remaining_count: remaining,
     }))
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::auth::{JwtConfig, TokenBlacklist, ViewerAccess, ViewerTokenRegistry};
+    use std::time::Duration;
+    use uuid::Uuid;
+
+    /// End-to-end (component-level) proof of CRITICAL #2: a viewer token minted
+    /// under a user, then revoked at logout via the registry drain, is rejected
+    /// by the SAME blacklist check the viewer WS runs (`is_revoked`).
+    ///
+    /// This exercises the real mint → register → logout-drain → blacklist path
+    /// without the HTTP/DB plumbing of the full handler. Uses local component
+    /// instances only (no process-global env), so it is parallel-safe.
+    #[tokio::test]
+    async fn logout_revokes_minted_viewer_token() {
+        let jwt = JwtConfig::new("test-secret-at-least-32-chars-long!!".to_string(), 24);
+        let registry = ViewerTokenRegistry::new();
+        let blacklist = TokenBlacklist::new();
+
+        let user_sub = Uuid::new_v4().to_string();
+        let session_id = Uuid::new_v4();
+        let tenant_id = Uuid::new_v4();
+
+        // Mint a viewer token and register it under the user (as mint_viewer_token does).
+        let viewer_token = jwt
+            .create_viewer_token(&user_sub, session_id, tenant_id, ViewerAccess::Control)
+            .unwrap();
+        registry.register(&user_sub, &viewer_token, Duration::from_secs(300));
+
+        // The viewer WS check (is_revoked) passes BEFORE logout — token is live.
+        assert!(!blacklist.is_revoked(&viewer_token).await);
+
+        // Logout drains the user's viewer tokens into the blacklist (handler logic).
+        for tok in registry.take_for_user(&user_sub) {
+            blacklist.revoke(&tok).await;
+        }
+
+        // After logout the same WS check now REJECTS the viewer token.
+        assert!(blacklist.is_revoked(&viewer_token).await);
+        // The token also remains a structurally valid viewer JWT (not expired),
+        // proving revocation — not natural expiry — is what blocks it.
+        assert!(jwt.validate_viewer_token(&viewer_token).is_ok());
+    }
+
+    /// A different user's logout must NOT revoke this user's viewer token.
+    #[tokio::test]
+    async fn logout_does_not_revoke_other_users_viewer_token() {
+        let jwt = JwtConfig::new("test-secret-at-least-32-chars-long!!".to_string(), 24);
+        let registry = ViewerTokenRegistry::new();
+        let blacklist = TokenBlacklist::new();
+
+        let user_a = Uuid::new_v4().to_string();
+        let user_b = Uuid::new_v4().to_string();
+        let session_id = Uuid::new_v4();
+        let tenant_id = Uuid::new_v4();
+
+        let token_b = jwt
+            .create_viewer_token(&user_b, session_id, tenant_id, ViewerAccess::ViewOnly)
+            .unwrap();
+        registry.register(&user_b, &token_b, Duration::from_secs(300));
+
+        // user_a logs out — drains only user_a's (empty) token set.
+        for tok in registry.take_for_user(&user_a) {
+            blacklist.revoke(&tok).await;
+        }
+
+        // user_b's token is untouched.
+        assert!(!blacklist.is_revoked(&token_b).await);
+    }
+}

server/src/api/sessions.rs

@@ -160,6 +160,18 @@ pub async fn mint_viewer_token(
             )
         })?;
 
+    // Register the minted token under the authenticated user's `sub` so logout
+    // can revoke it (CRITICAL #2): a viewer token minted before logout would
+    // otherwise stay valid for the rest of its 5-minute TTL. The registry prunes
+    // expired entries on insert, so it cannot grow unbounded. The token string
+    // is held transiently in memory (already handled server-side) and never
+    // logged. `user.user_id` is the `sub` claim stamped into the viewer token.
+    state.viewer_tokens.register(
+        &user.user_id,
+        &token,
+        std::time::Duration::from_secs(crate::auth::jwt::VIEWER_TOKEN_TTL_SECS as u64),
+    );
+
     tracing::info!(
         "User {} minted a {} viewer token for session {} (agent {})",
         user.username,

server/src/auth/mod.rs

@@ -7,10 +7,12 @@ pub mod agent_keys;
 pub mod jwt;
 pub mod password;
 pub mod token_blacklist;
+pub mod viewer_token_registry;
 
 pub use jwt::{Claims, JwtConfig, ViewerAccess};
 pub use password::{generate_random_password, hash_password, verify_password};
 pub use token_blacklist::TokenBlacklist;
+pub use viewer_token_registry::ViewerTokenRegistry;
 
 use axum::{
     extract::FromRequestParts,
@@ -58,30 +60,6 @@ impl From<Claims> for AuthenticatedUser {
     }
 }
 
-/// Authenticated agent from API key
-#[derive(Debug, Clone)]
-#[allow(dead_code)] // TODO(native-remote-control): consumed by the integration API; see docs/specs/native-remote-control/
-pub struct AuthenticatedAgent {
-    pub agent_id: String,
-    pub org_id: String,
-}
-
-/// JWT configuration stored in app state
-#[derive(Clone)]
-#[allow(dead_code)] // TODO(native-remote-control): consumed by the integration API; see docs/specs/native-remote-control/
-pub struct AuthState {
-    pub jwt_config: Arc<JwtConfig>,
-}
-
-impl AuthState {
-    #[allow(dead_code)] // TODO(native-remote-control): consumed by the integration API; see docs/specs/native-remote-control/
-    pub fn new(jwt_secret: String, expiry_hours: i64) -> Self {
-        Self {
-            jwt_config: Arc::new(JwtConfig::new(jwt_secret, expiry_hours)),
-        }
-    }
-}
-
 /// Extract authenticated user from request
 #[axum::async_trait]
 impl<S> FromRequestParts<S> for AuthenticatedUser
@@ -169,14 +147,3 @@ where
         }
     }
 }
-
-/// Validate an agent API key (placeholder for MVP)
-#[allow(dead_code)] // TODO(native-remote-control): consumed by the integration API; see docs/specs/native-remote-control/
-pub fn validate_agent_key(_api_key: &str) -> Option<AuthenticatedAgent> {
-    // TODO: Implement actual API key validation against database
-    // For now, accept any key for agent connections
-    Some(AuthenticatedAgent {
-        agent_id: "mvp-agent".to_string(),
-        org_id: "mvp-org".to_string(),
-    })
-}

server/src/auth/viewer_token_registry.rs

@@ -0,0 +1,186 @@
+//! Per-user registry of outstanding viewer tokens.
+//!
+//! Minted viewer tokens ([`super::jwt::ViewerClaims`]) are short-lived
+//! (`VIEWER_TOKEN_TTL_SECS`, 5 min) and session-scoped. The login-JWT blacklist
+//! revokes the LOGIN token on logout, but a viewer token minted before logout
+//! stays valid for the rest of its TTL — letting a just-logged-out user keep a
+//! live remote-control plane until natural expiry.
+//!
+//! This registry closes that gap with the least-invasive mechanism: when a
+//! viewer token is minted it is registered here under the minting user's `sub`;
+//! on logout, the user's outstanding viewer tokens are taken from the registry
+//! and added to the SAME exact-string blacklist the viewer WebSocket already
+//! checks (`relay::viewer_ws_handler` -> `TokenBlacklist::is_revoked`). No
+//! change to the WS path is required.
+//!
+//! Design notes:
+//! - The registry holds bearer-token strings transiently in memory. The server
+//!   already handles these tokens; they are never logged or otherwise exposed
+//!   by this module.
+//! - Entries are pruned-on-access (insert and take) by expiry, so the map can
+//!   never grow unbounded even if a user never logs out — expired viewer tokens
+//!   self-evict on the next operation that touches the map.
+//! - Locking is a poison-tolerant `std::sync::Mutex` (matching the rate-limiter
+//!   pattern: `lock().unwrap_or_else(|e| e.into_inner())`); a panic while the
+//!   lock is held must not wedge logout/mint. No `.unwrap()` on lock paths.
+
+use std::collections::HashMap;
+use std::sync::{Arc, Mutex};
+use std::time::{Duration, Instant};
+
+/// A single registered viewer token: the token string and the instant at which
+/// it expires (mint time + TTL).
+#[derive(Clone)]
+struct Entry {
+    token: String,
+    expires_at: Instant,
+}
+
+/// Per-user registry of outstanding (un-expired, un-revoked) viewer tokens.
+///
+/// Cheap to clone (shares the inner map via `Arc`), so it can live in
+/// `AppState` and be cloned with it like the other shared state.
+#[derive(Clone, Default)]
+pub struct ViewerTokenRegistry {
+    /// Map of user `sub` -> that user's outstanding viewer tokens.
+    inner: Arc<Mutex<HashMap<String, Vec<Entry>>>>,
+}
+
+impl ViewerTokenRegistry {
+    /// Create an empty registry.
+    pub fn new() -> Self {
+        Self::default()
+    }
+
+    /// Register a freshly minted viewer token under its owning user (`sub`),
+    /// expiring `ttl` from now.
+    ///
+    /// Prunes expired entries for this user on insert so the per-user vector and
+    /// the overall map stay bounded. The token string is stored verbatim (it is
+    /// the exact string the viewer WS blacklist-checks); it is never logged.
+    pub fn register(&self, sub: &str, token: &str, ttl: Duration) {
+        let now = Instant::now();
+        let expires_at = now + ttl;
+        let mut map = self.inner.lock().unwrap_or_else(|e| e.into_inner());
+
+        let entries = map.entry(sub.to_string()).or_default();
+        // Prune-on-insert: drop this user's already-expired tokens.
+        entries.retain(|e| e.expires_at > now);
+        entries.push(Entry {
+            token: token.to_string(),
+            expires_at,
+        });
+    }
+
+    /// Remove and return all currently-valid viewer tokens for `sub`.
+    ///
+    /// Expired tokens are pruned (not returned) — they are already invalid at
+    /// the JWT layer, so there is no value in blacklisting them. The user's
+    /// entry is cleared from the map regardless (taking everything out). Returns
+    /// an empty vec if the user has no registered tokens.
+    pub fn take_for_user(&self, sub: &str) -> Vec<String> {
+        let now = Instant::now();
+        let mut map = self.inner.lock().unwrap_or_else(|e| e.into_inner());
+
+        match map.remove(sub) {
+            Some(entries) => entries
+                .into_iter()
+                .filter(|e| e.expires_at > now)
+                .map(|e| e.token)
+                .collect(),
+            None => Vec::new(),
+        }
+    }
+
+    /// Number of users currently tracked (test/observability helper). Does not
+    /// prune; reflects the raw map size.
+    #[cfg(test)]
+    fn user_count(&self) -> usize {
+        let map = self.inner.lock().unwrap_or_else(|e| e.into_inner());
+        map.len()
+    }
+
+    /// Number of registered (un-pruned) tokens for a user (test helper).
+    #[cfg(test)]
+    fn token_count(&self, sub: &str) -> usize {
+        let map = self.inner.lock().unwrap_or_else(|e| e.into_inner());
+        map.get(sub).map(|v| v.len()).unwrap_or(0)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn register_then_take_returns_and_clears() {
+        let reg = ViewerTokenRegistry::new();
+        reg.register("user-a", "tok-1", Duration::from_secs(300));
+        reg.register("user-a", "tok-2", Duration::from_secs(300));
+
+        assert_eq!(reg.token_count("user-a"), 2);
+
+        let mut taken = reg.take_for_user("user-a");
+        taken.sort();
+        assert_eq!(taken, vec!["tok-1".to_string(), "tok-2".to_string()]);
+
+        // Taking clears the user's entry.
+        assert_eq!(reg.token_count("user-a"), 0);
+        assert_eq!(reg.user_count(), 0);
+        assert!(reg.take_for_user("user-a").is_empty());
+    }
+
+    #[test]
+    fn take_for_unknown_user_is_empty() {
+        let reg = ViewerTokenRegistry::new();
+        assert!(reg.take_for_user("nobody").is_empty());
+    }
+
+    #[test]
+    fn tokens_are_scoped_per_user() {
+        let reg = ViewerTokenRegistry::new();
+        reg.register("user-a", "a-tok", Duration::from_secs(300));
+        reg.register("user-b", "b-tok", Duration::from_secs(300));
+
+        assert_eq!(reg.take_for_user("user-a"), vec!["a-tok".to_string()]);
+        // user-b is untouched.
+        assert_eq!(reg.take_for_user("user-b"), vec!["b-tok".to_string()]);
+    }
+
+    #[test]
+    fn expired_tokens_are_not_returned_by_take() {
+        let reg = ViewerTokenRegistry::new();
+        // Zero TTL -> already expired by the time take runs.
+        reg.register("user-a", "expired", Duration::from_secs(0));
+        reg.register("user-a", "live", Duration::from_secs(300));
+
+        let taken = reg.take_for_user("user-a");
+        assert_eq!(taken, vec!["live".to_string()]);
+    }
+
+    #[test]
+    fn expired_tokens_are_pruned_on_insert() {
+        let reg = ViewerTokenRegistry::new();
+        // Register an already-expired token, then a live one for the same user.
+        reg.register("user-a", "expired", Duration::from_secs(0));
+        // First insert leaves 1 entry; the second insert prunes the expired one
+        // before pushing, so only the live token remains.
+        reg.register("user-a", "live", Duration::from_secs(300));
+
+        assert_eq!(reg.token_count("user-a"), 1);
+        assert_eq!(reg.take_for_user("user-a"), vec!["live".to_string()]);
+    }
+
+    #[test]
+    fn multiple_logins_accumulate_then_all_taken() {
+        // A user with several concurrent logins mints several viewer tokens;
+        // logout must revoke ALL of them at once.
+        let reg = ViewerTokenRegistry::new();
+        for i in 0..5 {
+            reg.register("user-a", &format!("tok-{i}"), Duration::from_secs(300));
+        }
+        assert_eq!(reg.token_count("user-a"), 5);
+        assert_eq!(reg.take_for_user("user-a").len(), 5);
+        assert_eq!(reg.user_count(), 0);
+    }
+}

server/src/main.rs

@@ -37,7 +37,10 @@ use tower_http::trace::TraceLayer;
 use tracing::{info, Level};
 use tracing_subscriber::FmtSubscriber;
 
-use auth::{generate_random_password, hash_password, AuthenticatedUser, JwtConfig, TokenBlacklist};
+use auth::{
+    generate_random_password, hash_password, AuthenticatedUser, JwtConfig, TokenBlacklist,
+    ViewerTokenRegistry,
+};
 
 /// Root of the static asset tree, relative to the server's working directory.
 /// Holds the agent `downloads/` tree AND the v2 SPA build under `app/`.
@@ -66,6 +69,13 @@ pub struct AppState {
     db: Option<db::Database>,
     pub jwt_config: Arc<JwtConfig>,
     pub token_blacklist: TokenBlacklist,
+    /// Per-user registry of outstanding session-scoped viewer tokens. Minting a
+    /// viewer token registers it here under the minting user's `sub`; logout
+    /// drains the user's registered viewer tokens into `token_blacklist` so a
+    /// just-logged-out user cannot keep a live viewer/remote-control plane until
+    /// the token's natural 5-minute expiry. The viewer WS already blacklist-
+    /// checks the exact token string, so no WS change is needed.
+    pub viewer_tokens: ViewerTokenRegistry,
     /// Optional API key for persistent agents (env: AGENT_API_KEY)
     pub agent_api_key: Option<String>,
     /// Prometheus metrics
@@ -291,6 +301,7 @@ async fn main() -> Result<()> {
 
     // Create application state
     let token_blacklist = TokenBlacklist::new();
+    let viewer_tokens = ViewerTokenRegistry::new();
 
     let state = AppState {
         sessions,
@@ -298,6 +309,7 @@ async fn main() -> Result<()> {
         db: database,
         jwt_config,
         token_blacklist,
+        viewer_tokens,
         agent_api_key,
         metrics,
         registry,
@@ -443,7 +455,10 @@ async fn main() -> Result<()> {
         // fallback below — CLAUDE.md documents this as the public download URL.
         // `nest_service` is matched BEFORE `fallback_service`, so these binaries
         // are served from disk and never fall through to the SPA index.html.
-        .nest_service("/downloads", ServeDir::new(format!("{STATIC_DIR}/downloads")))
+        .nest_service(
+            "/downloads",
+            ServeDir::new(format!("{STATIC_DIR}/downloads")),
+        )
         // NOTE: there are intentionally no /login, /dashboard, /users routes.
         // The v2 SPA (BrowserRouter) owns those paths and resolves them via the
         // fallback_service below; registering server-side handlers for them would
@@ -909,4 +924,3 @@ async fn trigger_machine_update(
         ))
     }
 }
-

server/src/relay/mod.rs

@@ -561,7 +561,7 @@ async fn handle_agent_connection(
     if let Some(ref code) = support_code {
         // Check if the code is cancelled or invalid
         if support_codes.is_cancelled(code).await {
-            warn!("Agent tried to connect with cancelled code: {}", code);
+            warn!("Agent tried to connect with a cancelled support code");
             // Send disconnect message to agent
             let disconnect_msg = proto::Message {
                 payload: Some(proto::message::Payload::Disconnect(proto::Disconnect {
@@ -740,7 +740,7 @@ async fn handle_agent_connection(
             interval.tick().await;
             if let Some(ref code) = support_code_check {
                 if support_codes_check.is_cancelled(code).await {
-                    info!("Support code {} was cancelled, disconnecting agent", code);
+                    info!("Support code was cancelled, disconnecting agent");
                     // Send disconnect message
                     let disconnect_msg = proto::Message {
                         payload: Some(proto::message::Payload::Disconnect(proto::Disconnect {
@@ -772,8 +772,11 @@ async fn handle_agent_connection(
                                 let _ = frame_tx.send(data.to_vec());
                             }
                             Some(proto::message::Payload::ChatMessage(chat)) => {
-                                // Broadcast chat message to all viewers
-                                info!("Chat from client: {}", chat.content);
+                                // Broadcast chat message to all viewers. Do NOT log
+                                // the chat body: support-session chat can carry
+                                // secrets/PII. Log only the (non-sensitive) length
+                                // so relay activity stays observable.
+                                info!("Chat from client ({} chars)", chat.content.len());
                                 let _ = frame_tx.send(data.to_vec());
                             }
                             Some(proto::message::Payload::AgentStatus(status)) => {
@@ -917,7 +920,7 @@ async fn handle_agent_connection(
                 let _ = db::support_codes::mark_code_completed(db.pool(), code).await;
             }
 
-            info!("Support code {} marked as completed", code);
+            info!("Support code marked as completed");
         }
     }
 
@@ -1369,8 +1372,10 @@ async fn handle_viewer_connection(
                             }
                             Some(proto::message::Payload::ChatMessage(chat)) => {
                                 // Forward chat message to agent (not throttled —
-                                // not an injected-input vector). Bounded send.
-                                info!("Chat from technician: {}", chat.content);
+                                // not an injected-input vector). Bounded send. Do
+                                // NOT log the chat body (secrets/PII); log only the
+                                // (non-sensitive) length so the relay is observable.
+                                info!("Chat from technician ({} chars)", chat.content.len());
                                 let _ = input_tx.try_send(data.to_vec());
                             }
                             _ => {}

server/src/utils/ip_extract.rs

@@ -154,6 +154,14 @@ pub fn client_ip(peer: &SocketAddr, headers: &HeaderMap, trusted: &TrustedProxie
     }
 
     // Trusted peer: prefer the single-value X-Real-IP if the proxy set it.
+    //
+    // SECURITY: we take X-Real-IP verbatim here, trusting it as set by the
+    // reverse proxy. The proxy (NPM) MUST overwrite it from the real TCP peer:
+    //   proxy_set_header X-Real-IP $remote_addr;
+    // It must NOT pass through a client-supplied X-Real-IP. A trusted peer that
+    // forwards an attacker-controlled value would let the client spoof the IP
+    // used for rate-limiting and audit logging. The trusted-proxy gate above
+    // only authenticates the immediate hop, not the contents of this header.
     if let Some(ip) = header_single_ip(headers, X_REAL_IP) {
         return ip;
     }

specs/v2-secure-session-core/plan.md

@@ -1,5 +1,13 @@
 # v2 Secure Session Core — Implementation Plan
 
+> **STATUS 2026-05-30: Tasks 1–7 IMPLEMENTED + DEPLOYED. Tasks 3–5 now CODE-REVIEWED — verdict
+> APPROVE-WITH-FIXES (no CRITICAL/HIGH).** Compile-verified on GURU-5070: `cargo fmt --check` clean,
+> `clippy -D warnings` 0 warnings, `cargo test --workspace` 89 pass. The 3 audit CRITICALs verified
+> closed with no bypass; all security paths fail closed. Non-blocking follow-ups tracked: viewer-token
+> logout revocation (MEDIUM, TTL-bounded), delete the dead `validate_agent_key` "accept-any" placeholder
+> (MEDIUM), `X-Real-IP`/consent-comment/support-code-log hygiene (LOW). **Remaining for Phase-1 exit:
+> Task 8 (e2e verification + `/gc-audit --pass=security` re-audit).**
+>
 > Spec created: 2026-05-29
 > Status: in progress — Tasks 1-4 IMPLEMENTED 2026-05-29 (Task 4 self-reviewed, pending Code Review;
 > Tasks 1-3 code-reviewed APPROVED). Task 4 completes the KEYSTONE (secure auth/session core). Viewer-token authz