# Repartee Review

Date: 2026-04-13
Scope: code quality, memory management, engineering practices, security
Status: in progress

## Method

- Review architecture and high-risk modules first
- Inspect code paths for process/session handling, networking, storage, crypto, scripting, and web auth
- Verify static quality gates with `make clippy` and `make test`
- Record findings incrementally during the review

## Rust-Only Audit

Date: 2026-04-13
Scope: native Rust code only (`src/`), excluding `web-ui/`
Status: in progress

### Method

- Re-check high-risk Rust modules with emphasis on correctness, ownership, async boundaries, persistence, and local attack surface
- Focus on `app`, `session`, `storage`, `e2e`, `scripting`, `shell`, `irc`, and process lifecycle paths
- Re-run Rust quality gates after the focused review

## Early Observations

- The project has a clear architecture split between `app`, `state`, `ui`, `irc`, `storage`, `web`, and `session`
- The repository enforces several strong conventions in `AGENTS.md`: `make`-based workflow, zero-warning `clippy`, `color-eyre` and `thiserror`, and a ban on hardcoded app name strings
- The codebase includes multiple risk-heavy areas that merit deeper review: detached session protocol, embedded shell/PTY, Lua scripting, TLS/websocket server, SQLite encryption, and end-to-end crypto

## Review Log

### 2026-04-13 1

- Loaded repository guidance and Rust review skills to frame the audit
- Read the architecture documentation and confirmed the main review targets:
  session transport, async event loop, web auth, storage writer, scripting, IRC protocol handling, and crypto

### 2026-04-13 2

- Static quality gate:
  `make clippy` completed successfully with no warnings
- Memory-management posture is generally deliberate:
  Linux builds use `jemalloc`, the storage writer uses bounded batching, and the project already documents a previous real-world OOM investigation together with the mitigation
- Positive findings:
  the async writer isolates SQLite work behind `spawn_blocking`,
  the shell code separates blocking PTY reads onto OS threads,
  and the E2E identity generator uses `Zeroizing` for seed material
- Security findings identified for deeper emphasis in the final section:
  1. WebSocket sessions are validated only by bearer token and the stored client IP is not enforced during validation
  2. The WebSocket token is passed in the query string (`/ws?token=...`), which increases accidental leakage risk through logs, proxies, browser tooling, and copied URLs
  3. Web shell screen updates are broadcast through the shared broadcaster instead of being scoped to the owning session
  4. `SwitchBuffer` in the web command handler mutates global active buffer state despite the protocol comment claiming session-local behavior
  5. E2E private keys and session keys are stored as raw SQLite BLOBs in the main database schema, independent of message-log encryption

### 2026-04-13 3

- Evidence collected for the main security concerns:
  `src/app/web.rs` changes global active buffer state on `WebCommand::SwitchBuffer`
  and also opens or resizes shell PTYs keyed only by session UUID
- `src/app/shell.rs` broadcasts `WebEvent::ShellScreen` through the general broadcaster and derives the outgoing `buffer_id` for web PTYs from the globally active buffer
- `src/web/ws.rs` validates a token from the query string and then subscribes the client to the shared broadcaster
- `src/web/auth.rs` stores the IP inside the session object but `validate()` does not compare it against the current client address
- `src/storage/db.rs` and `src/e2e/keyring.rs` persist `privkey` and multiple `sk` fields directly in SQLite tables; `src/storage/mod.rs` shows that the optional storage encryption concerns message logging, not a database-wide encryption layer

### 2026-04-13 4

- Dynamic verification:
  `make test` completed successfully
  with `1017 passed`, `0 failed`

## Final Assessment

### Overall quality

- Strong codebase overall
- Architecture is coherent and explicit
- Module boundaries are mostly sensible
- Testing coverage is far above average for an application of this size
- Static hygiene is strong:
  `clippy` passes cleanly and the repository clearly enforces local discipline

### Memory management

- Better than average for a long-running Rust TUI application
- Positive points:
  Linux-specific `jemalloc` allocator choice is deliberate and documented
  `src/main.rs`
- Storage writes are bounded and offloaded from async execution to blocking workers
  `src/storage/writer.rs`
- The project documents and tests a prior OOM class instead of ignoring it
- Secret handling is mixed:
  transient seed generation uses `Zeroizing`,
  but persisted E2E secrets are stored in plaintext SQLite BLOBs

### Engineering practices

- Generally strong:
  explicit error propagation,
  tracing-based logging,
  separation of UI-agnostic state,
  protocol tests,
  and dedicated modules for storage, web, session, and crypto
- Notable caveat:
  comments and protocol intent diverge from implementation in the web layer,
  which is a correctness and review-discipline issue, not just a security bug

## Findings

### Critical

1. Web shell output is broadcast to all web clients instead of being session-scoped.
   Evidence:
   `src/app/shell.rs` emits `WebEvent::ShellScreen` through the shared broadcaster for web PTYs,
   and `src/web/ws.rs` only filters events that carry `session_id`.
   `ShellScreen` has no `session_id`, so authenticated web clients can receive another client's shell screen.

2. Web buffer switching is implemented as global state mutation although the protocol says it is session-local.
   Evidence:
   `src/app/web.rs` calls `self.state.set_active_buffer(&buffer_id)` for `WebCommand::SwitchBuffer`.
   This can desynchronize TUI and web behavior, and directly feeds the shell broadcast bug because web shell output uses the globally active buffer as the outgoing `buffer_id`.

3. E2E private keys and session keys are stored unencrypted in SQLite.
   Evidence:
   `src/storage/db.rs` defines `privkey` and `sk` columns as plain BLOBs,
   and `src/e2e/keyring.rs` writes raw key material into them.
   `src/storage/mod.rs` only enables per-message encryption for message logging,
   not for the keyring tables.

### High

4. Web session validation stores the client IP but does not enforce it during validation.
   Evidence:
   `src/web/auth.rs` saves `ip` inside the session object,
   but `validate()` checks only token presence and age.
   A stolen token remains reusable from a different client address until expiry.

5. WebSocket bearer token is transported in the query string.
   Evidence:
   `src/web/ws.rs` expects `GET /ws?token=...`.
   This increases token exposure through copied URLs, browser tooling, intermediary logs, and reverse proxies.

## Positive notes

- Unsafe code surface is very small and localized to POSIX process primitives
- Lua scripting removes several dangerous globals and isolates script environments before exposing app APIs
- The async writer, shell PTY handling, and session protocol show awareness of blocking vs async boundaries
- The project already practices postmortem-style engineering for performance regressions instead of relying on guesswork

## Recommended priority

1. Fix web event scoping:
   add session targeting for `ShellScreen` and stop deriving its `buffer_id` from global active buffer state
2. Separate per-web-session active buffer from global `AppState.active_buffer_id`
3. Protect E2E secret material at rest:
   separate encrypted key store or wrap keyring rows with authenticated encryption
4. Bind sessions to client properties or rotate them aggressively after WebSocket upgrade
5. Replace query-string token auth with cookie or header-based authentication

## Verdict

- Code quality: high
- Memory management: above average, with one important caveat around secret persistence rather than heap safety
- Best practices: generally strong, but weakened by web-layer inconsistencies between declared design and real behavior
- Security: moderate risk today because the web subsystem contains real confidentiality issues despite otherwise solid engineering

## Remediation Update

Status: fixed in working tree

- Web shell isolation:
  `ShellScreen` events are now session-targeted and filtered per session,
  removing the shared-broadcast leak
- Web active buffer model:
  web clients now maintain server-side per-session active buffer state instead of mutating global `AppState.active_buffer_id` on `SwitchBuffer`
- Web auth transport:
  WebSocket auth no longer relies on `?token=...`;
  login sets an `HttpOnly`, `Secure`, `SameSite=Strict` cookie and the web client reconnect path uses a non-secret local session hint instead of storing a bearer token
- Session validation:
  server-side session validation now checks both token and client IP
- E2E secrets at rest:
  the app now uses an encrypted keyring path for persisted `privkey` and `sk` fields;
  the stored blobs are wrapped before writing and decrypted on read,
  while legacy plaintext rows remain readable for compatibility
- Local session/file hardening:
  the native app now enforces owner-only permissions for `~/.repartee`,
  `sessions`,
  `certs`,
  `config.toml`,
  `.env`,
  generated TLS key material,
  and session socket paths;
  shim attach additionally verifies that the connecting Unix-socket peer has the same local UID before accepting the session

## Remediation Verification

- `cargo fmt` completed successfully
- `make clippy` completed successfully after the fixes
- `make test` completed successfully after the fixes with:
  `1020 passed`, `0 failed`
- `make wasm` is still blocked by tooling:
  the target invokes `trunk`, but the current environment passes `--no-color 1`,
  which this installed `trunk` rejects before frontend compilation starts

## Rust-Only Audit

Scope:

- native Rust code only (`src/`)
- excluding `web-ui/`
- same criteria as the previous audit:
  code quality,
  memory management,
  programming best practices,
  and security

Method:

- re-read the highest-risk Rust modules:
  app loop, session attach/detach, scripting, shell/PTy, storage, E2E crypto, image preview, TLS, config/env
- re-check panic/unsafe surface and async/blocking boundaries
- verify with native quality gates after analysis

### Rust Findings

#### High

1. Local attach socket is not hardened and the protocol has no peer authentication.
   Evidence:
   [src/constants.rs](/Users/k/dev/repartee/src/constants.rs:82) creates `~/.repartee/sessions` with plain `create_dir_all`,
   [src/app/session.rs](/Users/k/dev/repartee/src/app/session.rs:16) repeats the same pattern and binds `{pid}.sock`,
   while [src/app/session.rs](/Users/k/dev/repartee/src/app/session.rs:55) immediately trusts the first message on connect as terminal metadata.
   The framing layer in [src/session/protocol.rs](/Users/k/dev/repartee/src/session/protocol.rs:97) limits size correctly, but it does not authenticate the peer.
   On systems with permissive `umask`, another local user may be able to enumerate or connect to a live session socket and control the attached terminal.

2. Secrets written by native Rust paths rely on default filesystem permissions instead of explicit restrictive modes.
   Evidence:
   [src/config/env.rs](/Users/k/dev/repartee/src/config/env.rs:34) writes `.env` with `std::fs::write`,
   and [src/web/tls.rs](/Users/k/dev/repartee/src/web/tls.rs:32) writes the generated private key PEM with `std::fs::write`.
   Both files can contain sensitive material (`WEB_PASSWORD`, SASL credentials, TLS private key), but unlike the storage key path used elsewhere in the codebase, these writes do not enforce owner-only permissions.
   This is a concrete local-security weakness and also a best-practice gap.

3. Image preview fetch path accepts arbitrary remote URLs without local-network or loopback protections.
   Evidence:
   [src/app/input.rs](/Users/k/dev/repartee/src/app/input.rs:563) extracts URLs directly from message text,
   [src/app/image.rs](/Users/k/dev/repartee/src/app/image.rs:33) passes them to the preview worker,
   and [src/image_preview/fetch.rs](/Users/k/dev/repartee/src/image_preview/fetch.rs:155) performs unrestricted `reqwest` GET requests for both the original URL and resolved `og:image`.
   Size and timeout limits are present, which is good, but there is no denylist for `localhost`, RFC1918, link-local, or metadata-service style targets.
   Practically this means a malicious message can turn the local client into a limited SSRF gadget if the user previews the link.

#### Medium

4. PTY command parsing is safe from shell injection but not robust for quoted arguments.
   Evidence:
   [src/shell/mod.rs](/Users/k/dev/repartee/src/shell/mod.rs:71) uses `split_whitespace()` before `portable-pty::CommandBuilder`.
   This avoids shell interpretation, which is the right security posture, but it also breaks legitimate quoted arguments and diverges from user expectations for shell-like commands.
   It is a correctness and UX issue, not a direct security flaw.

5. Secret material is handled safely enough for ordinary Rust ownership, but memory hardening is uneven.
   Evidence:
   [src/e2e/crypto/ecdh.rs](/Users/k/dev/repartee/src/e2e/crypto/ecdh.rs:29) uses `Zeroizing` for ephemeral seed generation,
   while session keys and other long-lived in-memory secrets are still stored in ordinary containers in the E2E manager.
   I did not find a memory safety bug here, but the project is not consistently applying zeroization or secrecy wrappers to all sensitive buffers.
   Treat this as hardening debt rather than an immediate defect.

### Rust Positives

- Unsafe usage is minimal and localized to narrow POSIX/process boundaries, chiefly liveness checks and fork/session setup.
- The project is generally strong on async hygiene:
  blocking SQLite and image decode work is moved off the async runtime instead of being hidden inside `async fn`.
- Message framing in [src/session/protocol.rs](/Users/k/dev/repartee/src/session/protocol.rs:91) has an explicit upper bound, which is important for detach/attach robustness.
- The E2E code shows good cryptographic structure overall:
  typed wrappers,
  explicit error handling,
  AEAD separation,
  and targeted use of `Zeroizing`.
- Panic surface is low for production code.
  Most `unwrap`/`expect` use is confined to tests or invariant-style initialization.

### Rust Verdict

- Code quality:
  high
- Memory management:
  above average,
  with good async/blocking discipline and no obvious ownership leaks,
  but with room for stronger secret-memory hygiene
- Best practices:
  strong overall,
  weakened mainly by filesystem permission handling and a few local-security assumptions
- Security:
  materially better than the previously audited web path,
  but still not fully hardened for hostile multi-user local environments or untrusted preview URLs

### Rust Verification

- `make clippy` completed successfully
- `make test` completed successfully:
  `1024 passed`, `0 failed`

### Rust Remediation Update

- Completed:
  hardened local filesystem permissions for session state and sensitive config material
- Completed:
  session attach now rejects Unix-socket peers owned by a different local UID
- Compatibility note:
  these changes do not block the legitimate owner from reattaching;
  they only remove cross-user access and insecure inherited permissions
