iter-55: daemon hardening, module borders & agent-friendly docs#58
Merged
iter-55: daemon hardening, module borders & agent-friendly docs#58
Conversation
A. Daemon hardening:
- A1: 32-byte CSPRNG auth token in daemon.json + handshake on every client
- A2: Daemon log opened with 0o600 on Unix
- A3: tempfile::Builder for launch profile dirs (16 random bytes)
- A4: `daemon status` and `daemon stop` subcommands w/ graceful shutdown + SIGTERM fallback
- A5: 100ms TCP probe to Firefox port before spawning daemon
- A6: Fix dispatch.rs comment for non-follow console
B. Module borders:
- B1: FramedReader/FramedWriter typed framed halves; raw encode_frame/recv_from
imports removed from daemon; from_parts/into_parts → pub(crate)
- B2: escape_selector + PortOwner fields → pub(crate)
C. Agent-friendly docs:
- C1: launch wrapped in standard {results,total,meta} envelope
- C2: Output: schema added to ~13 subcommand long_about blocks
- C3: EXIT CODES section in root --help; AppError::Internal exit 1 (was 2)
- C4: README §Usage replaced with link-forward to `ff-rdp --help`
Multi-user threat model documented in kb/decision-log.md (before/after).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens the ff-rdp background daemon against local/multi-user threats, tightens module boundaries between the CLI daemon and ff-rdp-core’s wire/framing layer, and updates CLI/README documentation to be more agent-friendly and schema-driven.
Changes:
- Added daemon TCP auth via per-session random token stored in the daemon registry; added
ff-rdp daemon status|stopmanagement commands and a graceful shutdown path. - Introduced typed framed transport halves (
FramedReader/FramedWriter) and reduced visibility of internal helpers to better enforce module boundaries. - Standardized/help-documented JSON output shapes (including exit codes) and reduced README drift by linking to
ff-rdp --help.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Replaces long, drift-prone CLI usage listing with link-forward guidance and key flags. |
| kb/iterations/iteration-55-daemon-hardening-docs.md | Iteration write-up documenting threat model, tasks, and acceptance criteria for iter-55. |
| kb/iterations/iteration-54-protocol-correctness.md | Normalizes iteration status wording to completed. |
| kb/decision-log.md | Documents the local multi-user threat model and the before/after of daemon TCP auth. |
| crates/ff-rdp-core/src/transport.rs | Adds FramedReader/FramedWriter and RdpTransport::split; reduces raw framing exposure. |
| crates/ff-rdp-core/src/lib.rs | Re-exports framed halves alongside RdpTransport. |
| crates/ff-rdp-core/src/connection.rs | Adds RdpConnection::from_authenticated_transport for daemon-authenticated flows. |
| crates/ff-rdp-cli/src/port_owner.rs | Tightens PortOwner field visibility to pub(crate). |
| crates/ff-rdp-cli/src/main.rs | Aligns internal error exit code with documented exit code semantics (exit 1). |
| crates/ff-rdp-cli/src/dispatch.rs | Adds routing for `daemon status |
| crates/ff-rdp-cli/src/daemon/server.rs | Implements auth handshake, framed transport usage, and shutdown RPC handling. |
| crates/ff-rdp-cli/src/daemon/registry.rs | Adds auth token generation/storage in daemon.json with tests. |
| crates/ff-rdp-cli/src/daemon/process.rs | Adds cross-platform process termination helper and opens daemon log with restricted mode intent. |
| crates/ff-rdp-cli/src/daemon/client.rs | Sends auth frame before daemon use; adds Firefox port probe; implements `daemon status |
| crates/ff-rdp-cli/src/commands/launch.rs | Switches temp profile dir creation to tempfile::Builder for symlink-attack resistance. |
| crates/ff-rdp-cli/src/commands/js_helpers.rs | Makes escape_selector pub(crate). |
| crates/ff-rdp-cli/src/commands/connect_tab.rs | Adds daemon-auth handshake path before normal RDP handshake. |
| crates/ff-rdp-cli/src/cli/args.rs | Adds EXIT CODES and output schema blocks; adds daemon management subcommand definitions. |
| crates/ff-rdp-cli/Cargo.toml | Promotes getrandom and tempfile to runtime deps for daemon auth and secure temp dirs. |
| Cargo.toml | Adds workspace dependency on getrandom. |
| Cargo.lock | Locks new dependency resolution for getrandom (and related dependency graph changes). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+314
to
+320
| fn is_firefox_port_open(host: &str, port: u16) -> bool { | ||
| let addr_str = format!("{host}:{port}"); | ||
| let Ok(addr) = addr_str.parse::<std::net::SocketAddr>() else { | ||
| // Fallback: unresolved hostname — do not block, allow normal path. | ||
| return true; | ||
| }; | ||
| TcpStream::connect_timeout(&addr, Duration::from_millis(100)).is_ok() |
Comment on lines
+424
to
+434
| let uptime_seconds = match daemon_rpc(cli, &json!({"to": "daemon", "type": "status"})) { | ||
| Ok(resp) => resp.get("uptime_secs").and_then(Value::as_u64), | ||
| Err(_) => None, | ||
| }; | ||
| json!({ | ||
| "running": true, | ||
| "pid": info.pid, | ||
| "port": info.proxy_port, | ||
| "uptime_seconds": uptime_seconds, | ||
| "connections": null, | ||
| "firefox_connected": true, |
Comment on lines
+424
to
+434
| let uptime_seconds = match daemon_rpc(cli, &json!({"to": "daemon", "type": "status"})) { | ||
| Ok(resp) => resp.get("uptime_secs").and_then(Value::as_u64), | ||
| Err(_) => None, | ||
| }; | ||
| json!({ | ||
| "running": true, | ||
| "pid": info.pid, | ||
| "port": info.proxy_port, | ||
| "uptime_seconds": uptime_seconds, | ||
| "connections": null, | ||
| "firefox_connected": true, |
Comment on lines
+122
to
+123
| opts.open(path) | ||
| .with_context(|| format!("opening daemon log file {}", path.display())) |
| The daemon keeps a persistent Firefox connection and buffers events across | ||
| commands. It starts automatically on the first command that needs it. | ||
|
|
||
| Output (status): {\"results\": {\"running\": bool, \"pid\": N, \"port\": N, \"uptime_seconds\": N, \"connections\": N, \"firefox_connected\": bool}, \"total\": 1, \"meta\": {...}} |
Copilot review (PR #58): - daemon/client.rs is_firefox_port_open: use ToSocketAddrs so the 100ms fast-fail probe actually works for hostnames like "localhost" (was silently skipped for any non-IP-literal host) - daemon/client.rs run_daemon_status: surface stream_subscriber_count as "connections" and include daemon-side buffer_sizes; drop the misleading hardcoded firefox_connected:true. Help text updated to match. - daemon/process.rs open_log_file: 0o600 mode only applies on file creation; force-tighten existing logs via set_permissions on Unix. Local review: - launch::build_command: drop dead _temp_profile parameter (was renamed with leading underscore to suppress unused-warning; CLAUDE.md forbids this pattern). The CLI flag is unaffected — output still reports effective_temp_profile based on the original arg. - core/connection.rs from_authenticated_transport: parse firefox_version from the daemon-forwarded greeting instead of dropping it. Daemon path now retains the same firefox_version surface as the direct path. - daemon/client.rs daemon_rpc: replace 64-frame cap with a deadline-based loop so the daemon response isn't missed under heavy push traffic. - daemon/server.rs: add e2e tests for the auth handshake (wrong token → immediate close, right token → greeting forwarded). Required by the iter-55 plan A1 acceptance criteria. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
User-facing follow-up to iter-54. Three groups:
A. Daemon hardening
daemon.json; first client frame must be{auth: <token>}, mismatch closes the socket0o600on Unixtempfile::Builder(16 random bytes) for launch profile dirs — defeats same-UID symlink attacksff-rdp daemon status/ff-rdp daemon stopsubcommands; graceful shutdown RPC w/ 2s SIGTERM fallbackdispatch.rscomment for non-followconsoleB. Module borders
FramedReader/FramedWritertyped framed halves; daemon no longer reaches into core's wire layerfrom_parts/into_parts,escape_selector,PortOwnerfields all tightened topub(crate)C. Agent-friendly docs
launchwrapped in standard{results, total, meta}envelopeOutput:JSON schema lines added to ~13 subcommandlong_aboutblocksEXIT CODESsection in root--help;AppError::Internalnow exits1(was2)ff-rdp --helpto stop driftMulti-user threat model documented in
kb/decision-log.md(before/after).Deferred (require live Firefox/daemon for fixtures): A1 auth-rejection e2e, A4 daemon spawn/stop e2e, Group 0 carryover items (ScopedGrip wiring + live fixtures), C1 fixture update.
Test plan
cargo fmt --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace -q(207 + 203 + 9 unit/integration tests pass)doctorstill passes against a live Firefox (no auth noise on happy path)daemon status/daemon stopend-to-end on Linux/macOS/Windows