Skip to content

fix: systematic deep dive audit -- security, safety, docs, module split#59

Merged
AndrewAltimit merged 2 commits intomainfrom
fix/deep-dive-systematic-fixes
Mar 9, 2026
Merged

fix: systematic deep dive audit -- security, safety, docs, module split#59
AndrewAltimit merged 2 commits intomainfrom
fix/deep-dive-systematic-fixes

Conversation

@AndrewAltimit
Copy link
Copy Markdown
Owner

Summary

Comprehensive audit addressing security, safety, code quality, and documentation issues identified during a deep dive analysis of the entire codebase.

Security fixes

  • PSK auth requires TLS: Client refuses to transmit PSK without TLS; listener rejects PSK auth without TLS. Prevents plaintext key leakage
  • Auth timeout: 30-second timeout on remote terminal client authentication prevents indefinite hangs from malicious/unresponsive servers
  • HTTP header size limit: 16KB MAX_HEADER_SIZE prevents memory exhaustion from malicious servers. Also accepts LF-only line endings per RFC 7230 S3.5

Safety fixes

  • SDL3 texture Drop: Explicit Drop impl clears textures before texture_creator, removing fragile struct field-ordering UB dependency
  • PSP SAFETY docs: Added # Safety docs to 22 unsafe fn declarations and // SAFETY: comments to 10 bare unsafe blocks across PSP backend and plugin

Code quality

  • WM module split: Extracted drag_resize.rs (953 lines) from manager.rs (2710 to 1821 lines) -- drag/resize state machine, click dispatch, edge snapping, position clamping
  • Dead code removal: Removed unused read_u16_be (demux_lite) and selected_is_dir (file_manager)
  • Video cancellation: Tail probe thread now checks cancellation flag via cancelled_flag() accessor threaded through fetch_range

Documentation sync

  • Fixed "17 skins" to "18 skins" across 8 files (CLAUDE.md, AGENTS.md, README.md, design.md, getting-started.md, testing-gap-analysis.md, psp-modernization-plan.md, site/index.html)
  • Fixed widget counts: "27" / "20+" / "30+" to "32 widgets" to match actual module count
  • Fixed ADR-003 title: "Four Traits" to "Five Traits" (AudioBackend was added)
  • Updated security.md: PSK requires TLS, 30s auth timeout documented
  • Fixed site/demo/index.html: Altimit skin in correct dropdown group

Build verification

  • PSP EBOOT: builds successfully
  • WASM release: builds successfully
  • Desktop: 5,392 tests pass, clippy clean, fmt clean
  • All pre-commit hooks pass

Test plan

  • cargo test --workspace -- 5,392 tests, 0 failures
  • cargo clippy --workspace -- -D warnings -- clean
  • cargo fmt --all -- --check -- clean
  • PSP EBOOT build (cargo +nightly psp --release)
  • WASM build (./scripts/build-wasm.sh --release)
  • Pre-commit hooks pass

Generated with Claude Code

AI Agent Bot and others added 2 commits March 9, 2026 12:15
… split

Security fixes:
- Refuse PSK auth without TLS (client returns error, listener rejects)
- Add 30-second auth timeout to remote terminal client
- Add 16KB HTTP header size limit to prevent memory exhaustion
- Accept LF-only line endings in HTTP responses (RFC 7230 §3.5)

Safety fixes:
- Add explicit Drop impl for SdlBackend to clear textures before
  texture_creator, removing fragile field-ordering UB dependency
- Add SAFETY docs to 14 unsafe fn declarations in PSP threading.rs
- Add SAFETY docs to 8 unsafe fn declarations in PSP plugin decode.rs
- Add SAFETY comments to 10 bare unsafe blocks in PSP TLS/networking

Code quality:
- Extract drag_resize.rs from WM manager.rs (2710→1821 lines)
- Remove dead code: read_u16_be (demux_lite), selected_is_dir (file_manager)
- Add cancellation token to video streaming tail probe thread
- Wire cancelled_flag() through fetch_range for cooperative abort
- Update PSP Cargo.lock

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix "17 skins" → "18 skins" across all docs (CLAUDE.md, AGENTS.md,
  README.md, design.md, getting-started.md, testing-gap-analysis.md,
  psp-modernization-plan.md, site/index.html)
- Fix "27 widgets" / "20+ widgets" / "30+ widgets" → "32 widgets"
  across all docs to match actual widget module count
- Fix AGENTS.md skin counts: "11 external, 17 built-in" → "12 external,
  18 built-in"
- Fix README.md crate table: "12 external + 7 built-in" → "12 external
  + 18 built-in"
- Fix ADR-003 title: "Four Traits" → "Five Traits" (AudioBackend added)
- Fix psp-modernization-plan: "30 extended" → "39 extended"
- Update security.md: document PSK requires TLS, 30s auth timeout
- Fix site/demo/index.html: move Altimit to "New Skins" group to match
  www/index.html dropdown

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 9, 2026

Gemini AI Code Review

Issues (if any)

  • [WARNING] crates/oasis-app/src/tv_controller/download.rs:96 - Uses unstable let_chains syntax

    • if let Some(flag) = cancel && flag.load(...) will fail to compile on stable Rust.
    • Use if cancel.is_some_and(|f| f.load(std::sync::atomic::Ordering::Acquire)) instead if #![feature(let_chains)] is not globally enabled.
  • [WARNING] crates/oasis-net/src/client.rs:115 - Uses unstable let_chains syntax

    • && let Some(started) = self.auth_started && ... will fail to compile on stable Rust.
    • Use if self.state == ClientState::Authenticating && self.auth_started.is_some_and(|s| s.elapsed().as_secs() >= AUTH_TIMEOUT_SECS) instead.

Previous Issues (for incremental reviews)

(none)

Suggestions (if any)

(none)

Notes

  • Security improvements for TLS PSK enforcement and HTTP header limits are solid. The early abort in read_response combined with the exact limit check in parse_response efficiently prevents memory exhaustion.
  • The drag_resize.rs extraction cleanly modularizes the window manager without losing state invariants.
  • Explicit Drop impl in SdlBackend is a good safety addition to prevent field-ordering UB.

Reaction


Generated by Gemini AI (gemini-3.1-pro-preview). Supplementary to human reviews.

@AndrewAltimit
Copy link
Copy Markdown
Owner Author

Review Response Agent (Iteration 1)

Status: No changes needed

Fixed Issues

  • (none)

Ignored Issues

  • let_chains syntax in download.rs:96 and client.rs:115 — Gemini flagged these as requiring unstable #![feature(let_chains)], but let_chains was stabilized in Rust 1.87.0. The project's MSRV is 1.91.0 and uses edition 2024 (requiring 1.85+), so this syntax compiles fine on stable Rust. False positives.

Deferred to Human

  • (none)

Notes

  • The Gemini review was lightweight (154 words) and only contained two warnings, both false positives about stable Rust feature availability. The review's positive notes about TLS PSK enforcement, drag_resize.rs extraction, and the SdlBackend Drop impl are reasonable.

The agent reviewed feedback but determined no code changes were required.

@AndrewAltimit AndrewAltimit merged commit fc3b789 into main Mar 9, 2026
9 checks passed
@AndrewAltimit AndrewAltimit deleted the fix/deep-dive-systematic-fixes branch March 9, 2026 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant