Files
guru-connect/reports/review-2026-06-05/gemini-dashboard.md
Mike Swanson ded99c5882
Some checks failed
Build and Test / Security Audit (push) Successful in 9m2s
Build and Test / Build Server (Linux) (push) Failing after 16m9s
Build and Test / Build Agent (Windows) (push) Successful in 19m25s
Build and Test / Build Summary (push) Has been skipped
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.
2026-06-05 17:20:30 -07:00

5.3 KiB

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.