diff --git a/docs/qe-reports/openhuman/00-executive-summary.md b/docs/qe-reports/openhuman/00-executive-summary.md new file mode 100644 index 00000000..b3b73e0e --- /dev/null +++ b/docs/qe-reports/openhuman/00-executive-summary.md @@ -0,0 +1,128 @@ +# OpenHuman — QE Executive Summary + +**Subject**: `tinyhumansai/openhuman` (Early Beta) +**Snapshot**: 2026-05-20 +**Method**: Seven specialist QE agents ran in parallel against `/tmp/openhuman` (cloned from `main`). Each produced an evidence-based report with file:line citations. This document is the cross-cutting synthesis. +**Source clone**: `/tmp/openhuman` — repo hash from cloned `main`. + +--- + +## TL;DR + +| Dimension | Score (1=worst, 10=best) | One-line verdict | +|---|---|---| +| **Code Quality** | 6/10 | Concentrated, mechanically fixable debt — five god-files do most of the damage | +| **Security** | 7.5/10 | Strong defenses overall; two one-line HIGH issues in the cloud-deploy path | +| **Performance** | 5/10 | Good hygiene in places; five systemic patterns that will hurt under real load | +| **QX (Quality Experience)** | 6.5/10 | Distinctive strengths in trust + boot UX; weakness in i18n delivery vs ambition | +| **Complexity** | 6/10 | Three named refactors would drop it to ~4/10 | +| **Dependencies** | 6/10 | Competent baseline; reproducibility, layering, and one license item to fix | +| **SFDIPOT (test surface)** | — | Test investment is happy-path-heavy; adversarial/concurrency/resource-pressure gaps | + +**Composite QE verdict**: `6.2 / 10` — credible early-beta posture for a project of this surface area (a Tauri desktop AI assistant with 118 OAuth connectors, a wallet, embedded CEF, in-process skills sandbox, MCP client+server, and auto-update for both shell and core). The product **is not** in trouble, but it carries five named risks that will become release-blocking if not paid down before GA. + +--- + +## Top 10 Cross-Cutting Risks (ranked by blast radius × likelihood) + +| # | Risk | Severity | Where | Effort to Fix | +|---|------|---------|-------|---------------| +| 1 | **RPC mutations default-on** — `rpc_mutations_enabled = true`, makes `update.run` / `update.apply` callable by any bearer-authed client. On Docker/cloud `0.0.0.0` deployments this is a credible auth'd downgrade/force-restart vector. | **HIGH** | `src/openhuman/config/schema/update.rs:53-55` | One line | +| 2 | **Variable-time token comparison** — bearer-token compared with `==` on `&str`. Source code literally comments that the fix is "one-line." | **HIGH** | `src/core/auth.rs:210-212` | One line | +| 3 | **Mutex poison bricks IPC** — `std::sync::Mutex` with 17 production `.lock().unwrap()` in the most callback-heavy Tauri host file. A single panic in any DOM-driven callback poisons the mutex → all subsequent IPC fails. `parking_lot::Mutex` is already a dependency. | **HIGH** | `app/src-tauri/src/webview_accounts/mod.rs:23` (+17 call sites) | Hours — swap import | +| 4 | **CEF `--remote-debugging-port=9222`** exposes the embedded Chromium to any local process via CDP, including OAuth-webview cookies and privileged-frame JS. | **HIGH** | tauri.conf / vendored Tauri-CEF fork | Days — gate behind dev flag | +| 5 | **Prompt-injection surface vs detector** — 118 connectors funnel untrusted text into one detector module; very few negative tests in repo. | **HIGH** | (system-level) | Weeks — adversarial corpus + property tests | +| 6 | **Skills supply chain unsigned** — `SKILLS_REGISTRY_URL` accepts arbitrary HTTP URLs, `SKILLS_LOCAL_DIR` any local path, no signing observed. Skills run in-process with the wallet and 118 OAuth tokens. | **HIGH** | env config + skills loader | Weeks — sigstore/minisign + provenance | +| 7 | **Five god-files / god-functions** — `webview_account_open` 840 LOC, `Agent::turn` 967 LOC, `Conversations.tsx` 2,125 LOC + 39 hooks, `AIPanel.tsx` 2,696 LOC, `apply_env_overrides_from` 720 LOC. Single-render crash in `Conversations.tsx` blanks the whole app (only one root `Sentry.ErrorBoundary`). | **MED** | listed in report 01/06 | 1–2 weeks per file | +| 8 | **Performance landmines under load** — WhatsApp bulk ingest is per-row INSERT with no transaction/`prepare_cached` + correlated COUNT/MAX subqueries; `Conversations.tsx` recomputes derived state every render with only one `React.memo` in the entire codebase; audio transcription does `Array.from(Uint8Array(blob))` IPC (~8× payload bloat); `sync_vault` blocking I/O in async; no code-splitting (`three` ~600 KB in main bundle). | **MED** | see report 03 | 2–4 days | +| 9 | **Non-reproducible Rust build** — `whisper-rs-sys` is git-pinned to `branch = "main"` with **no rev** in both `Cargo.toml` files. Plus two independent `Cargo.lock` files (no workspace), and two pnpm lockfiles that disagree (`vite@8.0.10` vs `vite@7.3.2`). | **MED** | `Cargo.toml`, `app/src-tauri/Cargo.toml`, lockfiles | Days — unify workspace + pin revs | +| 10 | **No backup or migration rollback for `~/.openhuman`** — single SQLite holds the irreplaceable Memory Tree; no rollback path on failed migration; backfill binaries can mutate the same DB concurrently with the live desktop. | **MED** | (system-level) | Weeks — backup + migration safety harness | + +--- + +## Where the Product is Strong (do not undo these) + +These came up across multiple reports and represent positive signals worth preserving as you grow: + +- **Trust posture, best-in-class.** Default-deny consent gate, PII-stripped Sentry, GA event allowlist, in-product `WhatLeavesLink` disclosure — `features/privacy/whatLeavesItems.ts:11-31`, `services/analytics.ts:5-26`. (Caveat: `OPENHUMAN_ANALYTICS_ENABLED=true` default contradicts the marketing — flip it.) +- **Boot UX & crash recovery.** `PersistRehydrationScreen.tsx:14,47-50` has a 10s deadline with a recovery CTA; `ErrorFallbackScreen.tsx` is self-contained with three real recovery actions. +- **SSRF guard with DNS-rebinding protection** — `src/openhuman/tools/impl/network/url_guard.rs`. Real defense, not theater. +- **Docker hardening** — `read_only`, `cap_drop: ALL`, `no-new-privileges` in compose. +- **Tauri capability scoping** — caller-label verification on `webview_recipe_event` is correctly done. +- **Real test density** — ~86 k LOC of tests, ~20 Rust integration + ~55 WDIO E2E specs. Adversarial gap, but the baseline exists. +- **A11y discipline** — zero `
` across 382 `.tsx` files; real `useT()` adoption at most points that matter. + +--- + +## Quick Wins (≤ 1 day each, high yield) + +Pick these up before anything bigger. + +1. **Fix SEC-02**: replace `==` with `subtle::ConstantTimeEq` in `src/core/auth.rs:210-212`. (1 line) +2. **Fix SEC-01**: flip `rpc_mutations_enabled` default to `false`; opt-in via env. `src/openhuman/config/schema/update.rs:53-55`. (1 line + docs) +3. **Replace `std::sync::Mutex` with `parking_lot::Mutex`** in `webview_accounts/mod.rs` — eliminates the panic-poison-bricks-IPC class entirely. `parking_lot` is already a dep. +4. **Flip `OPENHUMAN_ANALYTICS_ENABLED` default to `false`** in `.env.example` to match privacy marketing. +5. **Add `role="alert"`** to the chat send-error banner — `app/src/pages/Conversations.tsx:1959-1963` — and `aria-invalid` propagation in `components/ui/Input.tsx:20-23`. (~5 lines total) +6. **Stop the 25 MB IPC JSON transcode** — switch `Conversations.tsx:823` to the existing file-path audio API. (~10 lines) +7. **Pin `whisper-rs-sys`** to a specific git `rev` in both `Cargo.toml` files. (2 lines) +8. **Drop module-wide `#![allow(dead_code)]`** in `src/openhuman/mod.rs:16` — let the compiler tell you what's actually dead. +9. **Unblock the `pr-quality.yml` CI gate** — remove `continue-on-error: true` from at least the lint/typecheck/test jobs. They are currently advisory-only. +10. **Wrap the 5 most-likely-to-crash UI subtrees in their own `ErrorBoundary`** (Conversations thread list, composer, streaming pane, agent-profile editor, voice). Right now one Sentry boundary at the root means a Conversations render error blanks the entire app. + +--- + +## Recommended 30 / 60 / 90 Day Plan + +**30 days — stop the bleeding** +- Land all 10 quick wins above. +- Build an **adversarial corpus** for the prompt-injection detector (target ≥ 500 cases across 118 connectors); wire it as a CI gate. +- Add `cargo audit` to CI (the security scanner could not run it locally; it must run in your pipeline). +- Gate CEF remote-debugging behind a build flag — off in release. +- Make `OPENHUMAN_CORE_TOKEN` required at startup; refuse to listen on `0.0.0.0` without one. Fail closed. + +**60 days — pay down the structural debt** +- Refactor `Conversations.tsx` (2,125 LOC, 39 hooks) into thread-list / composer / streaming / agent-profile / voice subtrees, each with its own `ErrorBoundary` and `React.memo` discipline. +- Refactor `Agent::turn` (967 LOC) by extracting the four obvious sub-state-machines. +- Decompose `webview_account_open` (840 LOC) and `apply_env_overrides_from` (720 LOC). +- Replace `Result<_, String>` with typed error enums in `core/observability.rs` and the 1,062 sites that follow. +- Migrate off `ethers-rs` (deprecated upstream) to `alloy`. +- Convert the repo to a real Cargo workspace; unify the two `Cargo.lock`s; resolve the `vite` lockfile disagreement. + +**90 days — durable resilience** +- Backup + migration safety harness for `~/.openhuman` (single SQLite holding the Memory Tree); reject concurrent backfill-binary writes when the desktop has the DB open. +- Sign skills (sigstore/minisign + provenance); allowlist `SKILLS_REGISTRY_URL` schemes. +- Bring `prepare_cached` + transactional batching to all ingest paths (WhatsApp ingest is the worst, but it's a pattern). +- Add a **real-VM install test** to CI for both Linux and macOS — current `installer-smoke.yml` is `--dry-run` only. +- Adversarial / concurrency / resource-pressure tests for the JSON-RPC `/rpc` endpoint and the QuickJS skills sandbox. +- Verify Remotion license posture (`remotion/package.json` declares `UNLICENSED`; Remotion's commercial tier triggers above 3 staff). + +--- + +## What We Did Not Cover (recommended follow-up) + +- **`cargo audit`** — could not run in the agent environment; needs to run in your CI. +- **Mutation testing** — none performed; would clarify whether the 86 k LOC of tests actually exercise behavior or just types. +- **License audit at the transitive level** — name-heuristic scan only; no SBOM-grade verification (GTK/cairo/glib LGPL are fine if dynamically linked, but worth confirming for OpenSSL on the Tauri build). +- **Threat-model walkthrough** — the static security audit is necessary but not sufficient for a desktop AI app with this surface; a structured STRIDE/LINDDUN session on the IPC + skills + wallet boundary is the highest-leverage next QE activity. + +--- + +## Detailed Reports + +| # | File | Owner Agent | Key Finding | +|---|------|-------------|-------------| +| 01 | [`01-code-quality.md`](./01-code-quality.md) | qe-code-reviewer | Mutex-poison bricks IPC; five god-files concentrate the debt | +| 02 | [`02-security.md`](./02-security.md) | qe-security-scanner | 0 CRIT / 2 HIGH / 5 MED / 4 LOW / 5 INFO; strong baseline | +| 03 | [`03-performance.md`](./03-performance.md) | qe-performance-reviewer | Five systemic patterns; ~2 engineer-days for the top 10 | +| 04 | [`04-qx-experience.md`](./04-qx-experience.md) | qe-qx-partner | Best-in-class trust posture; i18n delivery mismatch | +| 05 | [`05-product-factors-sfdipot.md`](./05-product-factors-sfdipot.md) | qe-product-factors-assessor | Test investment is happy-path-heavy; adversarial gap dominates | +| 06 | [`06-complexity-hotspots.md`](./06-complexity-hotspots.md) | qe-code-complexity | 483 k production LOC; 3 named refactors → 4/10 | +| 07 | [`07-dependencies.md`](./07-dependencies.md) | qe-dependency-mapper | Non-reproducible Rust build; lockfile disagreements; ethers deprecated | + +--- + +## Closing Note + +OpenHuman is ambitious — a private, local-first, personal AI assistant with the surface area of a small OS. The QE evidence suggests a team that **knows what it's doing** (trust posture, SSRF guard, Tauri capability scoping, real test density) but is at the point in scale where the structural debt and a few sharp-edged defaults are starting to bite faster than they can be paid down by hand. + +The top 10 quick wins are all under a day each. The 30-day plan is achievable by a small team. After that, the bigger refactors (`Conversations.tsx`, `Agent::turn`, repo workspace unification, adversarial test corpus) are the ones that determine whether OpenHuman holds together through GA. diff --git a/docs/qe-reports/openhuman/01-code-quality.md b/docs/qe-reports/openhuman/01-code-quality.md new file mode 100644 index 00000000..396698cc --- /dev/null +++ b/docs/qe-reports/openhuman/01-code-quality.md @@ -0,0 +1,252 @@ +# OpenHuman — Code Quality & Code Smell Review + +**Reviewer:** AQE v3 Code Reviewer (sampled, evidence-based) +**Scope sampled:** 25 files across `src/` (Rust core), `app/src-tauri/src/` (Tauri host), `app/src/` (React/TS) +**Method:** size-ranked sweeps + grep for smell indicators + targeted line reads. Not exhaustive. + +--- + +## Executive Summary + +Severity-ranked (Critical → Info): + +1. **[HIGH] One Tauri host file is a 4,450-line god module.** `app/src-tauri/src/webview_accounts/mod.rs` owns process state, IPC commands, CDP wiring, notification routing, and webview lifecycle in a single file. The single function `webview_account_open` is **840 lines** (line 13033). This blocks safe parallel work, hides invariants, and is the single largest maintainability risk in the codebase. +2. **[HIGH] `std::sync::Mutex` + `.lock().unwrap()` pattern in the Tauri host.** `webview_accounts/mod.rs` uses `use std::sync::Mutex;` (line 23) and contains **17 production-path `.lock().unwrap()` calls** (e.g., lines 1119, 1636, 1845, 2655, 2669, 2677, 3040, 3054, 3073, 3083). A single panic while a lock is held permanently poisons it — every subsequent IPC command from React will then crash the host. `parking_lot::Mutex` (which doesn't poison) is **already a direct dep** (`Cargo.toml`) and is the obvious fix. +3. **[HIGH] The agent turn loop is a 967-line god-method.** `src/openhuman/agent/harness/session/turn.rs:71` — `Agent::turn` runs from line 71 to ~1037. The same file holds three other 250+-line methods (`inject_agent_experience_context` 345 lines L1038, `emit_progress` 265 lines L1383, `build_system_prompt` 308 lines L1838). The orchestrator-of-everything pattern makes it hard to test branches in isolation and concentrates ownership of unrelated concerns (KV-cache prefix policy, prompt construction, integration hydration, memory injection). +4. **[HIGH] React page-level components carry too much state.** `app/src/pages/Conversations.tsx` is a single 1,919-line component (L206 → L2124) with **29 hook calls** inside one function body. `app/src/components/settings/panels/AIPanel.tsx` is 2,696 lines containing a 690-line `BackgroundLoopControls` (L2948) and a 521-line `AIPanel` (L4109). These will not survive their next major refactor without regressions. +5. **[MEDIUM] Env-driven config schema loader is a 720-line method.** `src/openhuman/config/schema/load.rs:5732` — `apply_env_overrides_from` is 720 lines in one function. Any new env var lands in a fan-out of `if let Some(...)` arms with no test partitioning. +6. **[MEDIUM] Inconsistent error-type strategy.** 392 files use `anyhow`, only 8 use `thiserror`, and **1,062 functions return `Result<..., String>`** (greppable in `src/`). Stringly-typed errors propagate context loss and force callers into substring sniffing — this is exactly what `src/core/observability.rs::expected_error_kind` (L137) and `is_session_expired_message` (L227) are doing for ~700 lines: classifying *strings* into error kinds. This is the long-tail cost of stringly-typed errors made concrete. +7. **[MEDIUM] Pervasive `#![allow(dead_code)]` suppression.** `src/openhuman/mod.rs:16` blankets the entire `openhuman` module subtree with `#![allow(dead_code)]`. 37 occurrences of `#[allow(dead_code)]` and 110 total `#[allow(...)]` attributes across `src/` mean the compiler's own dead-code detector is silenced — actual dead code is invisible. +8. **[MEDIUM] Comment-driven feature toggles via `[#1123]`.** `app/src/App.tsx` has 7 commented-out blocks tagged `[#1123]` (L23, 48, 50, 125, 161, 194, 215) and the same pattern in `Conversations.tsx` (L14, 21, 23) and `pages/Accounts.tsx`. Dead code preserved as comments is harder to keep current than removing it and pulling from git history. +9. **[LOW] Hardcoded transport endpoints.** `src/openhuman/tools/impl/browser/types.rs:33` hardcodes `http://127.0.0.1:8787/v1/actions` as a default. `src/openhuman/webview_apis/client.rs:142` constructs `ws://127.0.0.1:{port}/` from an env var with no fallback validation. Both are leaky abstractions for tests/dev that have escaped into production defaults. +10. **[LOW] Documentation outweighs code in places.** `AGENTS.md` is 649 lines; `CLAUDE.md` is 311. Some module docs (`src/openhuman/mod.rs`, `app/src-tauri/src/webview_accounts/mod.rs`) are genuinely good; others (e.g., `src/openhuman/agent/harness/session/turn.rs:51-68`) read like docstrings written to compensate for function size rather than to document an interface. + +**Top-level finding:** the codebase is *not* sloppy — naming is good, error handling is principled in most modules, doc-comments are present, and the test scaffolding is substantial (~85k LOC of test code vs ~360k LOC of prod Rust = ~24% test-LOC ratio, healthy for a Rust codebase). The structural problems are concentrated in a small number of hotspot files that have grown organically into god-objects. + +--- + +## Quantitative Metrics + +| Metric | Value | Note | +|---|---|---| +| Rust files (prod + test) | 1,323 | under `src/`, `packages/`, `app/src-tauri/` | +| TS/TSX files (prod + test) | 895 | under `app/src/`, `packages/` | +| Rust prod LOC | ~360k | excludes `*_test*.rs` and `tests.rs` | +| Rust test LOC | ~86k | `*_test*.rs` + `tests.rs` files | +| TS prod LOC | ~110k | excludes `__tests__/` and `*.test.*` | +| Workspace structure | **single crate** | no `[workspace]` in root `Cargo.toml`; 74 `pub mod` lines in one `openhuman/mod.rs` | +| Direct Rust deps | 144 | top-level entries in `Cargo.toml` | +| `Result<_, String>` signatures | 1,062 | non-test Rust | +| `anyhow::Result` users | 392 files | predominant | +| `thiserror` users | 8 files | the typed-error story is barely adopted | +| `.unwrap()` total | 3,009 | mostly inside `#[cfg(test)]` blocks (good) | +| `.unwrap()` in prod paths | ~120–150 | concentrated in webview_accounts (49) + a few benchmarks/schemas | +| `.expect(` total | 730 | most are fixture/test code | +| `panic!(` calls | 75 | none in critical hot paths sampled | +| `let _ = ` (discarded result) | 640 | many are fire-and-forget event emits — semi-legitimate | +| TS `as any` | 21 | mostly in `app/src/polyfills.ts` for global injection | +| TS `@ts-ignore` | 4 | all in test files | +| TS `console.*` in prod | 195 | predominantly `console.warn/error` — acceptable | +| TODO/FIXME/HACK | 20 | low density, mostly with linked issue numbers | + +### Largest Rust production files (top 10) +``` +4450 app/src-tauri/src/webview_accounts/mod.rs +3846 app/src-tauri/src/lib.rs +2702 src/core/observability.rs +2345 src/openhuman/memory/tree/read_rpc.rs +2181 src/openhuman/agent/harness/session/turn.rs +2093 app/src-tauri/src/whatsapp_scanner/mod.rs +2013 src/openhuman/config/schema/load.rs +2005 src/openhuman/inference/provider/compatible.rs +1889 src/openhuman/composio/ops.rs +1792 src/openhuman/channels/providers/web.rs +``` + +### Largest TS/TSX production files (top 10) +``` +2696 app/src/components/settings/panels/AIPanel.tsx +2261 app/src/lib/i18n/en.ts (translation strings — legitimate) +2125 app/src/pages/Conversations.tsx +2111 app/src/lib/i18n/ko.ts (translation strings — legitimate) +1489 app/src/services/webviewAccountService.ts + 997 app/src/pages/Skills.tsx + 923 app/src/providers/ChatRuntimeProvider.tsx + 920 app/src/components/composio/ComposioConnectModal.tsx + 906 app/src/features/human/Mascot/yellow/MascotCharacter.tsx + 857 app/src/components/settings/panels/VoicePanel.tsx +``` + +--- + +## Top 10 Specific Issues (file:line citations) + +### 1. `webview_accounts/mod.rs` is a 4,450-line god module + +`app/src-tauri/src/webview_accounts/mod.rs:1-3239` (prod section) + tests below. + +The module docstring at lines 1-19 honestly admits the scope: "Hosts third-party web apps … recipe injection … per-account session isolation … notification bypass." That's at least five bounded contexts in one file: + +- Provider URL registry (`provider_url` L48) +- CDP browser session management +- Notification forwarding (`forward_native_notification` L1112, 329 lines) +- Webview lifecycle commands (`webview_account_open` L13033, **840 lines**) +- DND / mute / focus preference state + +The single function `webview_account_open` at 840 lines is the centerpiece — it spawns child webviews, injects scripts, registers CDP sessions, wires notification handlers, and stores state in three different `Mutex>` fields. This cannot be unit-tested. + +**Fix:** Split into `mod.rs` (re-exports + state struct), `commands.rs` (Tauri `#[command]` entries), `lifecycle.rs` (open/close/prewarm), `notifications.rs` (forward + bypass prefs), `recipes.rs` (script injection). Keep the state struct as the only cross-cutting type. + +--- + +### 2. `std::sync::Mutex` + `.lock().unwrap()` panic-amplifier in Tauri host + +`app/src-tauri/src/webview_accounts/mod.rs:23` declares `use std::sync::Mutex;`. The production section (lines 1-3239, before `#[cfg(test)]`) contains **17 calls to `.lock().unwrap()`**: + +``` +1119: state.notification_bypass.lock().unwrap().clone(); +1636: app_state.inner.lock().unwrap().get(account_id).cloned(); +1845: state.inner.lock().unwrap(); +2655: state.inner.lock().unwrap().remove(&args.account_id); +2669: state.browser_ids.lock().unwrap().remove(&args.account_id); +2677: state.cdp_sessions.lock().unwrap().remove(&args.account_id); +2745, 2755, 2763, 2896, 2937, 2993, 3010, +3040, 3054, 3073, 3083 (notification bypass prefs) +``` + +Pattern recurs in `app/src-tauri/src/lib.rs` (13 calls), `screen_capture/mod.rs` (6), `meet_call/mod.rs` (6). + +**Risk:** If any closure invoked while holding `state.inner` panics — e.g., a logging macro chokes on a malformed UTF-8 payload from `webview_recipe_event` (which is fed by untrusted DOM content from third-party sites like Slack/Discord/WhatsApp) — the mutex is poisoned. Every subsequent IPC command that tries to lock it will panic at the `.unwrap()`. Recovery requires restarting the app process. + +**Fix:** Replace `use std::sync::Mutex;` with `use parking_lot::Mutex;` (already in `Cargo.toml`). `parking_lot::Mutex::lock()` returns the guard directly — no `unwrap()` needed, no poisoning. This is a mechanical change with a high blast-radius improvement. + +--- + +### 3. `Agent::turn` is a 967-line method + +`src/openhuman/agent/harness/session/turn.rs:71` — `pub async fn turn(&mut self, user_message: &str) -> Result` runs to ~line 1037. + +Same file: +- L1038: `inject_agent_experience_context` — 345 lines +- L1383: `emit_progress` — 265 lines +- L1838: `build_system_prompt` — 308 lines + +The docstring at L51-68 lists six numbered responsibilities (initialization, prompt construction, context injection, execution loop, synthesis, background tasks). That's a Single Responsibility Principle violation enumerated in the comment. + +**Fix:** Extract each numbered step into a private method on `Agent` (e.g., `fn maybe_resume_transcript`, `fn maybe_bake_system_prompt`, `async fn execution_loop`). Each step is independently testable — and a 9-step state machine surfaces naturally. + +--- + +### 4. `apply_env_overrides_from` is 720 lines in `config/schema/load.rs:5732` + +`src/openhuman/config/schema/load.rs:5732` — `fn apply_env_overrides_from(&mut self, env: &(dyn EnvLookup + Send + Sync))` is **720 lines** long. With 144 direct Cargo deps and a sprawling settings surface, the codebase has a lot of env vars to handle — but doing them all in one method means no test partitioning, no documentation per group, and merge conflicts whenever two PRs add env vars. + +**Fix:** Group env vars by domain (e.g., `apply_inference_env`, `apply_observability_env`, `apply_voice_env`) and have the top-level method delegate. Each sub-method can have a corresponding `#[test]` that pokes specific keys. + +--- + +### 5. `pages/Conversations.tsx` is a 1,919-line single component with 29 hooks + +`app/src/pages/Conversations.tsx:206` defines `const Conversations = ({...}) => {` and the body extends to L2124. Inside that single function: + +- 29 hook calls (`useState`, `useEffect`, `useMemo`, `useCallback`, `useRef`) — counted via grep within the component body +- L2125: `export const AgentChatPanel = () => ;` exists *only* to expose a variant alias + +When this component has 29 stateful concerns mixed with chat send/receive, MIC composer, autocomplete polling (`AUTOCOMPLETE_POLL_DEBOUNCE_MS` L100), thread management, agent profile editing, prompt-injection guard, voice STT, and Redux dispatch — every renderer rerun touches all of them. Memoization at the top level is masked. + +**Fix:** Pull subsystems into `useConversationsChatState`, `useConversationsAutocomplete`, `useConversationsVoice` hooks under `app/src/pages/conversations/hooks/`. The shell component becomes a layout coordinator. + +--- + +### 6. `AIPanel.tsx` packs 4 distinct components into 2,696 lines + +`app/src/components/settings/panels/AIPanel.tsx`: +- L2650: `ProviderKeyDialog` — 127 lines +- L2948: `BackgroundLoopControls` — **690 lines** +- L3727: `CustomRoutingDialog` — 326 lines +- L4109: `AIPanel` — 521 lines + +Plus two large hooks (`useAISettings` L272, `useOllamaStatus` L387, `useInstalledModels` L436) that should be in `hooks/` rather than embedded in the panel. + +**Fix:** Split into a directory: +``` +panels/ai/ + AIPanel.tsx + ProviderKeyDialog.tsx + BackgroundLoopControls.tsx + CustomRoutingDialog.tsx + hooks/useAISettings.ts + hooks/useOllamaStatus.ts + hooks/useInstalledModels.ts +``` + +--- + +### 7. Stringly-typed errors infect the entire core + +1,062 functions return `Result<..., String>` in non-test Rust code under `src/`. The cost is concretely visible: + +`src/core/observability.rs:137-516` — there are **8 different "is_xxx_message"** classifier functions: + +- `expected_error_kind` L137 +- `is_session_expired_message` L227 +- `is_loopback_unavailable` L271 +- `is_network_unreachable_message` L299 +- `is_transient_upstream_http_message` L347 +- `is_backend_user_error_message` L384 +- `is_provider_user_state_message` L420 +- `is_local_ai_capability_unavailable_message` L516 + +Every one of them does substring matching on error *strings*. This is the dual of the upstream choice to throw away typed errors. The same substring-matching pattern appears in `is_transient_provider_http_failure` (L812) and `is_max_iterations_event` (L845) but reading Sentry events. + +**Fix:** Introduce `thiserror` enums at the integration boundaries (provider errors, channel errors, memory errors, tool errors). The classifier file then matches on enum variants, not strings, and the matchers can never silently drift when a vendor changes a message. + +--- + +### 8. Module-wide `#![allow(dead_code)]` defeats the compiler + +`src/openhuman/mod.rs:16` — `#![allow(dead_code)]` at module level disables dead-code warnings for the entire 75-module subtree (74 `pub mod` lines in the same file). The justification comment ("Many types/functions are intended for future use or integration with the frontend") is the canonical anti-pattern: the compiler is the only tool that reliably detects truly-unreachable code, and you have turned it off everywhere. + +37 separate `#[allow(dead_code)]` and 110 `#[allow(...)]` attributes underneath compound the issue — they're locally justified, but the umbrella `#![allow(dead_code)]` makes them redundant *and* prevents anyone from cleaning them up without a flag day. + +**Fix:** Remove the module-level allow. Let the compiler emit warnings. Either delete what is unused, mark individual items `#[allow(dead_code)]` with a comment, or keep public exports referenced by a doctest. Yes, this is a flag-day cleanup, but the long-term cost of not having dead-code signal is steeper. + +--- + +### 9. Commented-out code as feature flags (`[#1123]` markers) + +`app/src/App.tsx` lines 23, 48, 50, 125, 161, 194, 215 — seven commented-out blocks tagged `[#1123]`. Same pattern in `app/src/pages/Conversations.tsx` L14, L21, L23, and `app/src/pages/Accounts.tsx` L8, L10, L111+. + +The comment-tag is well-meant (each one references the issue removing welcome-agent onboarding), but git history already records that. Long-lived comments rot faster than code because nobody runs `cargo check` against them. + +**Fix:** Delete the commented blocks. If a rollback is needed, `git revert` is one command. + +--- + +### 10. Hardcoded loopback URLs/ports in production defaults + +- `src/openhuman/tools/impl/browser/types.rs:33` — `endpoint: "http://127.0.0.1:8787/v1/actions".into()`. A hardcoded loopback URL with a magic port as a struct default. If two browser tools collide on 8787, the second one silently 502s. +- `src/openhuman/webview_apis/client.rs:142` — `let url = format!("ws://127.0.0.1:{port}/");` where `port` comes from `OPENHUMAN_WEBVIEW_APIS_PORT` (`webview_apis/mod.rs:10`). No bounds check on the env var; an empty string will panic the URL parse downstream. + +**Fix:** Lift these into a `BrowserToolConfig` / `WebviewApisConfig` populated by the config loader (the same loader that already handles env overrides, see issue #4). Validate at parse time. + +--- + +## Strengths Observed (what they're doing right) + +1. **Test scaffolding is generous.** ~86k LOC of Rust test code against ~360k LOC of prod is healthy. Files like `src/openhuman/security/policy_tests.rs` (1,497 LOC) and `src/openhuman/composio/ops_test.rs` (1,534 LOC) show real coverage discipline. +2. **Module-level docstrings are real.** E.g., `app/src-tauri/src/webview_accounts/mod.rs:1-19` explains the architecture concisely with an ASCII flow diagram. `src/openhuman/agent/harness/session/turn.rs:1-18` lists the public surface up-front. Many crates would kill for this. +3. **TODOs reference issue numbers.** Of the 20 TODO/FIXME markers in the codebase, most have linked issue IDs: `TODO(#1339)`, `TODO(phase-5)`, `TODO(composio-retry-dedup)`. This is unusually disciplined. +4. **Domain segmentation is broadly correct.** 75 modules under `src/openhuman/` cleanly map to capability areas (memory, channels, composio, inference, voice, security). The boundaries are real even if individual modules have grown too large. +5. **Error-classification is centralized, not scattered.** Even though `observability.rs` is 2,702 lines, it is the *one* place where error-string heuristics live, rather than being duplicated across providers. +6. **TypeScript discipline is high.** Only 21 `as any` (most in `polyfills.ts` for legitimate global injection), 4 `@ts-ignore` (all in tests), and no `@ts-nocheck`. Compare to typical Electron+React apps where these run into hundreds. +7. **Notification routing is feature-flagged correctly.** `webview_accounts/mod.rs:1146-1158` — the `forward_native_notification` function gates on `NotificationSettingsState::enabled()` and exits early when off. Defensive coding done right. +8. **Test/prod split via `#[cfg(test)]` inline modules is consistent.** The 3,009 `.unwrap()` calls compress to ~120-150 in actual production paths once `#[cfg(test)]` blocks are excluded — the test scaffold is doing its job. +9. **No `unimplemented!()` or `todo!()` macros in prod.** Zero occurrences across `src/` and `app/src-tauri/src/`. This means no half-finished code paths can panic the agent loop. +10. **Inference provider module is decomposed.** Even though `compatible.rs` is 2,005 lines, the surrounding directory has nine sibling files (`compatible_parse.rs`, `compatible_stream.rs`, `compatible_types.rs`, `reliable.rs`, `router.rs`, `factory.rs`, etc.) — the directory was clearly designed with separation in mind even if `compatible.rs` itself never got split. + +--- + +## Risk Score: **6 / 10** + +The codebase is in the upper half of its peer group. Test density is good, naming is sound, TS strictness is enforced, and most modules show genuine architectural intent. The risk drivers are concentrated rather than diffuse: a small handful of god-files (`webview_accounts/mod.rs`, `agent/harness/session/turn.rs`, `pages/Conversations.tsx`, `panels/AIPanel.tsx`, `config/schema/load.rs`) plus one systemic choice (`Result<_, String>` everywhere, dead-code warnings disabled module-wide). These are addressable with mechanical refactors — no architectural rewrite needed. The single most acute *operational* risk is the `std::sync::Mutex` + `.lock().unwrap()` pattern in the Tauri host: a one-line panic in a third-party-DOM-driven callback poisons a lock and bricks the IPC layer until the user restarts the app. That alone justifies a sprint of focused cleanup before the next release. diff --git a/docs/qe-reports/openhuman/02-security.md b/docs/qe-reports/openhuman/02-security.md new file mode 100644 index 00000000..3f59cdff --- /dev/null +++ b/docs/qe-reports/openhuman/02-security.md @@ -0,0 +1,242 @@ +# OpenHuman Security Audit + +**Audit Date**: 2026-05-20 +**Auditor**: V3 QE Security Scanner (agentic-qe) +**Project Version**: 0.54.3 (per `app/src-tauri/tauri.conf.json`) +**Scope**: OWASP Top 10 + AI/LLM-specific + Tauri/desktop + supply chain +**Methodology**: Static analysis (grep + targeted file reads), `pnpm audit`, manual code review of auth/IPC/network surfaces + +--- + +## Executive Summary + +OpenHuman is a Tauri-based desktop AI assistant with a Rust core (`openhuman-core` JSON-RPC server) and a TypeScript/React frontend. The codebase shows a **strong baseline security posture** with several pieces of evidence: + +- **Per-process random 256-bit bearer tokens** for the core RPC, 0o600 file perms on Unix (`src/core/auth.rs:104-269`). +- **SSRF defence in depth** with DNS-rebinding protection on outbound HTTP tools (`src/openhuman/tools/impl/network/url_guard.rs`). +- **Prompt-injection detector + enforcement** wired into the agent bus, web channel, and local inference paths (`src/openhuman/prompt_injection/detector.rs`). +- **Tauri capabilities are explicitly scoped** to specific commands and webview labels; recipe events are bound to the caller webview label. +- **Hardened Docker compose**: `read_only`, `no-new-privileges`, `cap_drop: ALL`, tmpfs, mem/cpu limits (`docker-compose.yml:25-50`). +- **No secrets committed**: the suspicious 12.9KB `.env.example` is documentation only — every value is blank or commented out. `scripts/ci-secrets.example.json` is also all-blank. + +**Critical findings**: **0** +**High findings**: **2** +**Medium findings**: **5** +**Low findings**: **4** +**Info / observations**: **5** + +The two `HIGH` findings are not exploitable in isolation but combine into a credible remote-code-execution path when the core is deployed in Docker/cloud mode (the `0.0.0.0` bind path that the project explicitly supports). + +--- + +## Findings Table + +| ID | Severity | Category | File:Line | Description | Recommendation | +|----|----------|----------|-----------|-------------|----------------| +| SEC-01 | HIGH | Auth / Update / RCE | `src/openhuman/config/schema/update.rs:53-55`, `src/openhuman/update/ops.rs:21-39` | `rpc_mutations_enabled` defaults to `true` — any bearer-authenticated RPC client can call `update.apply`/`update.run`, downloading and executing a new core binary. On Docker/cloud deployments (`OPENHUMAN_CORE_HOST=0.0.0.0`) this is an authenticated remote-code-execution path. `.env.example:79-81` acknowledges the risk but does not change the default. | Flip the default to `false`. Keep desktop-Tauri (where the shell controls the token) opt-in via a feature flag or runtime check (`if bind_host == 127.0.0.1 { allow }`). | +| SEC-02 | HIGH | Auth / Timing side-channel | `src/core/auth.rs:210-212` | `bearer_matches` uses `==` (variable-time `&str` comparison). Token comparison is non-constant-time. The code comment even calls this out: *"adding constant-time semantics later is a one-line change."* For a 64-hex (256-bit) token over LAN/cloud this is a *theoretical* timing oracle, but it is reachable on `0.0.0.0` deployments. | Replace with `subtle::ConstantTimeEq` or `ring::constant_time::verify_slices_are_equal`. One-line fix. | +| SEC-03 | MEDIUM | XSS / Injection | `app/src/features/human/Mascot/backend/BackendMascot.tsx:144`, `:87` | `dangerouslySetInnerHTML` with backend-fetched SVG (`app/src/services/mascotService.ts:19-24`) plus a live `slot.innerHTML = inner` swap on viseme change. SVG can contain `