Skip to content

review.md

Latest commit

 

History

History
318 lines (248 loc) · 9.95 KB

review.md

File metadata and controls

318 lines (248 loc) · 9.95 KB

comon Code Review (Revision 2)

Overview

Project: comon - A cross-platform TUI for Codex usage statistics and rate limits
Language: Rust (edition 2021)
Architecture: Modular async application with ratatui-based UI
Review Date: Updated after security hardening

Changes Since Last Review

The codebase has been significantly hardened with security improvements and resource management fixes.

Fixed Issues

Issue Status Implementation
Unbounded channels Fixed Bounded channels with capacity 64 (app/mod.rs:72-75)
Resource leak on exit Fixed Watch channel for graceful shutdown (app/mod.rs:76, 235)
Blocking I/O in async Fixed spawn_blocking for file operations (app/mod.rs:89-94)
Typo "horisontal" Fixed Corrected to "horizontal" (ui/mod.rs:135)
Missing request timeout Fixed 15s timeout with cleanup (codex_rpc/mod.rs:218-225)

New Security Hardening

Protection Module Details
RPC line size limit codex_rpc 1MB max per line (MAX_RPC_LINE_BYTES)
RPC buffer overflow codex_rpc Kills process if buffer exceeds 2MB
Pending request limit codex_rpc Max 64 concurrent RPC requests
Session file size limit usage 64MB per file (MAX_SESSION_FILE_BYTES)
Total scan limit usage 256MB total (MAX_SESSION_TOTAL_BYTES)
File count limit usage 10,000 files max (MAX_SESSION_FILES_SCANNED)
Model count limit usage 5,000 distinct models (MAX_DISTINCT_MODELS)
Symlink protection usage Refuses to follow symlinks
Special file protection usage Skips FIFOs, devices, etc.
Control char filtering ui truncate_middle strips control chars

Current Architecture Assessment

app/mod.rs (372 lines)

Strengths:

  • Clean shutdown coordination via watch::channel
  • Bounded channels prevent memory exhaustion
  • Workers properly respond to shutdown signal
  • File I/O offloaded to blocking tasks

Code Quality:

// Excellent: Graceful shutdown propagation
tokio::select! {
    _ = shutdown_rx.changed() => break,
    _ = interval.tick() => {}
    recv = usage_refresh_rx.recv() => {
        if recv.is_none() { break; }
    }
}
// Good: Blocking I/O in spawn_blocking
let snapshot = tokio::task::spawn_blocking({
    let codex_home = codex_home.clone();
    move || crate::usage::compute_snapshot(usage_days, &codex_home, None)
})
.await
.unwrap_or_else(|err| Err(anyhow!("usage snapshot task failed: {err}")));

codex_rpc/mod.rs (335 lines)

Strengths:

  • Manual line parsing with size limits prevents buffer overflow attacks
  • Process killed on protocol violations (malicious server protection)
  • Request timeout prevents indefinite hangs
  • Pending request limit prevents resource exhaustion

Code Quality:

// Excellent: Defense against malicious/broken servers
if line.len() > MAX_RPC_LINE_BYTES {
    let mut child = rpc.child.lock().await;
    let _ = child.kill().await;
    return;
}

// Protect against unbounded growth if server omits newlines
if buf.len() > MAX_RPC_LINE_BYTES * 2 {
    let mut child = rpc.child.lock().await;
    let _ = child.kill().await;
    return;
}
// Good: Request limit with proper timeout handling
{
    let pending_len = self.pending.lock().await.len();
    if pending_len >= MAX_PENDING_REQUESTS {
        return Err(anyhow!("too many pending RPC requests"));
    }
}

match timeout(REQUEST_TIMEOUT, rx).await {
    Ok(Ok(v)) => Ok(v),
    Ok(Err(_)) => Err(anyhow!("request canceled")),
    Err(_) => {
        let _ = self.pending.lock().await.remove(&id);
        Err(anyhow!("RPC request timeout after {:?}", REQUEST_TIMEOUT))
    }
}

usage/mod.rs (926 lines)

Strengths:

  • Comprehensive resource limits prevent DoS via crafted session directories
  • Symlink and special file rejection blocks path traversal attacks
  • Model count capping prevents unbounded HashMap growth
  • Per-file and total byte limits bound memory usage

Code Quality:

// Excellent: Layered defense against malicious session files
let ft = match entry.file_type() {
    Ok(ft) => ft,
    Err(_) => continue,
};
// Never follow symlinks or read special files from untrusted sessions tree
if ft.is_symlink() || !ft.is_file() {
    continue;
}

let meta = match entry.metadata() {
    Ok(m) => m,
    Err(_) => continue,
};
let len = meta.len();
if len == 0 || len > MAX_SESSION_FILE_BYTES {
    continue;
}
if total_bytes_scanned.saturating_add(len) > MAX_SESSION_TOTAL_BYTES {
    continue;
}
// Good: Bounded model tracking
if model_totals.len() <= MAX_DISTINCT_MODELS
    || model_totals.contains_key(&model)
{
    *model_totals.entry(model).or_insert(0) += delta.input + delta.output;
} else {
    *model_totals.entry("other".to_string()).or_insert(0) +=
        delta.input + delta.output;
}

ui/mod.rs (1509 lines)

Strengths:

  • Control character filtering in truncate_middle prevents terminal injection
  • Unicode-aware character counting

Code Quality:

// Good: Sanitized string truncation
fn truncate_middle(value: &str, max: usize) -> String {
    let cleaned: String = value.chars().filter(|c| !c.is_control()).collect();
    if cleaned.chars().count() <= max {
        return cleaned;
    }
    // ... Unicode-safe truncation
}

Remaining Observations

Minor Issues

1. Unused Parameter Still Present (usage/mod.rs:424)

fn build_snapshot(
    _updated_at_ms: i64,  // Still unused

Impact: Low - cosmetic only.

2. Large UI Module

At 1509 lines, ui/mod.rs remains monolithic. Consider splitting if further features are added.

3. day_dir_for_key Path Components (usage/mod.rs:858-864)

let year = parts.next().unwrap_or("1970");
let month = parts.next().unwrap_or("01");
let day = parts.next().unwrap_or("01");

These fallbacks are safe but could be documented as intentional behavior for malformed day keys.

Potential Enhancements (Non-Critical)

  1. Structured logging: Consider adding tracing for debugging production issues.

  2. Metrics export: The usage data structure could support JSON export for external tools.

  3. Configuration file: Hard-coded limits could be made configurable for power users.

Security Analysis

Attack Surface

Vector Protection Status
Malicious RPC server Line limits, buffer caps, process kill Protected
Crafted session files Size limits, count limits, symlink rejection Protected
Path traversal Symlink rejection, no directory escape Protected
Memory exhaustion Bounded channels, model caps, byte limits Protected
Terminal injection Control char filtering Protected
Hang/timeout DoS Request timeouts, graceful shutdown Protected

Threat Model Coverage

  1. Untrusted CODEX_HOME: Sessions directory could contain attacker-controlled files.

    • Mitigated by: file size limits, symlink rejection, special file rejection, total byte cap.

  2. Malicious Codex app-server: RPC server could send oversized or malformed responses.

    • Mitigated by: line size limit, buffer overflow protection, process termination.

  3. Resource exhaustion: Large number of files/models could exhaust memory.

    • Mitigated by: file count limit, model count limit, bounded channels.

  4. Untrusted CLI paths: User-provided --cwd/--project could be invalid or surprising.

    • Mitigated by: validating paths exist and are directories, plus best-effort canonicalization.

Code Quality Metrics (Updated)

Aspect Previous Current Notes
Architecture Good Good Clean separation maintained
Error Handling Good Good Consistent anyhow usage
Memory Safety Good Excellent Comprehensive bounds checking
Async Correctness Fair Good spawn_blocking for I/O
Security Fair Excellent Multiple defense layers
Performance Fair Good Bounded resources
Maintainability Fair Fair UI still large
Test Coverage Poor Poor No tests yet
Documentation Poor Fair Constants are self-documenting

Recommendations

Priority 1: Testing

Add unit tests for:

  • usage::compute_snapshot with crafted JSONL files
  • codex_rpc::parse_rate_limits with edge cases
  • ui::truncate_middle with Unicode and control chars
  • Resource limit enforcement

Priority 2: Documentation

  • Add rustdoc to public types (LocalUsageSnapshot, AccountRateLimits, Config)
  • Document the security model in README or SECURITY.md

Priority 3: Future Hardening

  • Consider rate-limiting RPC requests
  • Add file descriptor limits for session scanning
  • Consider sandboxing the Codex app-server subprocess

Summary

The codebase has undergone significant security hardening since the initial review. All critical and high-priority issues have been addressed:

  • Bounded channels prevent memory exhaustion
  • Graceful shutdown prevents resource leaks
  • Blocking I/O is properly offloaded
  • RPC protocol violations terminate the malicious process
  • Session file scanning is bounded by multiple limits
  • Symlink/special file attacks are blocked
  • Terminal injection is prevented by control char filtering

Overall Assessment: Production-ready. The security posture is now robust against common attack vectors. Primary remaining work is adding test coverage.

Appendix: Constants Reference

// codex_rpc/mod.rs
const MAX_RPC_LINE_BYTES: usize = 1024 * 1024;      // 1MB
const REQUEST_TIMEOUT: Duration = Duration::from_secs(15);
const MAX_PENDING_REQUESTS: usize = 64;

// usage/mod.rs
const MAX_ACTIVITY_GAP_MS: i64 = 2 * 60 * 1000;     // 2 minutes
const MAX_SESSION_FILE_BYTES: u64 = 64 * 1024 * 1024;   // 64MB
const MAX_SESSION_TOTAL_BYTES: u64 = 256 * 1024 * 1024; // 256MB
const MAX_SESSION_FILES_SCANNED: usize = 10_000;
const MAX_DISTINCT_MODELS: usize = 5_000;

// app/mod.rs (implicit)
const CHANNEL_CAPACITY: usize = 64;  // mpsc::channel capacity