iter-61w: security hardening (auth, refstore, terminal escapes) + bulk-packet skip + kb refresh#88
Merged
Conversation
Theme A — Daemon auth hardening: - Constant-time token compare via subtle::ConstantTimeEq - RefStore capped at MAX_REFS=50_000, resolver.len()<=4096 - nav_url truncated to MAX_NAV_URL_LEN=4096 chars - Subscriber resourceType validated against WATCHED_RESOURCE_TYPES Theme B — Operator surface: - core::util::terminal::sanitize_for_terminal wraps eprintln of attacker-influenced text in eval/js_helpers/network_events - FF_RDP_TRACE_RAW=1 emits startup warning - Dispatcher mutex locks recover from poisoning + log once Theme C — Bulk packet skip: - Transport detects leading 'b', skips bulk frame body - Surfaces typed ProtocolError::BulkPacketUnsupported - Daemon logs once and continues Theme E — LongString cap hoist: - 16 MiB MAX_FETCH cap before any allocation in full_string - Returns RdpError::InvalidPacket on overflow / u64 too large Theme F — wait_for_doc_complete deadline: - Deadline check moved to top of outer loop (pre-drain) Theme D — kb refresh: - lessons-learned: rewrite/annotate 4 entries (closed-in 61n/q/r/v) - open-gaps: new "Closed gaps" section, 6 entries moved - ff-rdp-wins §4: wired in iter-61t - watcher.md: method support matrix added - new wired-vs-primitive.md snapshot - stability-roadmap: 61t..61w map + 61m..61s post-mortem Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 20 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 trial, open-source, and free plans. In all cases, review capacity refills continuously over time. 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 (24)
✨ 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 daemon/CLI against several security-audit findings (auth timing, bounded in-memory stores, terminal escape injection), adds forward-compatible handling for Firefox “bulk” frames in the transport layer, and refreshes/annotates the KB roadmap/docs to reflect the iter-61m..61w arc.
Changes:
- Add terminal-output sanitization and apply it to attacker-influenced stderr paths; warn when raw tracing is enabled.
- Teach the transport to detect/skip
bulk ...frames and surface a typedProtocolError::BulkPacketUnsupportedwhile keeping the stream readable. - Add/refine multiple daemon hardening measures (constant-time auth compare, bounded RefStore / subscriber type validation, nav URL truncation) and refresh related KB pages.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| kb/rdp/from-our-codebase/wired-vs-primitive.md | New reference snapshot clarifying which stability-roadmap primitives are wired vs still primitive. |
| kb/rdp/from-our-codebase/open-gaps.md | Move resolved items into “Closed gaps” with closed-in annotations. |
| kb/rdp/from-our-codebase/lessons-learned.md | Update/annotate lessons with resolved status and current behavior. |
| kb/rdp/ff-rdp-wins.md | Annotate consoleActor staleness fix as wired in iter-61t. |
| kb/rdp/actors/watcher.md | Add a method support matrix for WatcherFront/spec/wiring status. |
| kb/iterations/stability-roadmap.md | Extend roadmap through iter-61w and add post-mortem notes on wiring deferrals. |
| kb/iterations/iteration-61w-security-hardening-and-cleanup.md | Mark iter-61w tasks/acceptance criteria as completed. |
| crates/ff-rdp-core/src/util/terminal.rs | New sanitize_for_terminal helper + unit tests. |
| crates/ff-rdp-core/src/util/mod.rs | Export util module. |
| crates/ff-rdp-core/src/transport.rs | Add bulk-frame skip logic + tests; document frame formats. |
| crates/ff-rdp-core/src/lib.rs | Export util module and re-export sanitize_for_terminal. |
| crates/ff-rdp-core/src/error.rs | Add ProtocolError::BulkPacketUnsupported. |
| crates/ff-rdp-core/src/actors/string.rs | Add MAX_FETCH cap enforcement in LongStringActor::full_string + tests. |
| crates/ff-rdp-cli/src/main.rs | Emit a stderr warning when FF_RDP_TRACE_RAW is set. |
| crates/ff-rdp-cli/src/error.rs | Map BulkPacketUnsupported to Internal for direct-connect CLI callers. |
| crates/ff-rdp-cli/src/daemon/server.rs | Daemon hardening: constant-time auth compare, RefStore bounds, resourceType validation, bulk error logging, poison-mutex recovery macro usage. |
| crates/ff-rdp-cli/src/daemon/buffer.rs | Truncate stored navigation URLs to a maximum length. |
| crates/ff-rdp-cli/src/commands/network_events.rs | Sanitize attacker-influenced exception text printed to stderr. |
| crates/ff-rdp-cli/src/commands/navigate.rs | Move timeout check earlier in wait loop + add regression test. |
| crates/ff-rdp-cli/src/commands/js_helpers.rs | Sanitize exception text printed to stderr. |
| crates/ff-rdp-cli/src/commands/eval.rs | Sanitize exception text printed to stderr. |
| crates/ff-rdp-cli/Cargo.toml | Add subtle dependency via workspace. |
| Cargo.toml | Add workspace dependency pin for subtle = "2". |
| Cargo.lock | Lockfile update for subtle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
116
to
125
| fn register(&mut self, entries: Vec<(String, String)>) { | ||
| if self.refs.len() >= Self::MAX_REFS { | ||
| return; | ||
| } | ||
| for (id, resolver) in entries { | ||
| if resolver.len() > 4096 { | ||
| continue; | ||
| } | ||
| self.refs.insert(id, resolver); | ||
| } |
Comment on lines
+114
to
+115
| /// (len >= [`Self::MAX_REFS`]) or when any resolver string exceeds 4096 | ||
| /// bytes, to bound memory usage and prevent injection of oversized payloads. |
Comment on lines
+27
to
+41
| /// Logs a `tracing::error!` **once** (guarded by `POISON_LOGGED`) so that | ||
| /// the incident is visible in traces without flooding logs on every call. | ||
| macro_rules! lock_or_recover { | ||
| ($mutex:expr) => {{ | ||
| static POISON_LOGGED: std::sync::atomic::AtomicBool = | ||
| std::sync::atomic::AtomicBool::new(false); | ||
| $mutex.lock().unwrap_or_else(|p| { | ||
| if !POISON_LOGGED.swap(true, std::sync::atomic::Ordering::Relaxed) { | ||
| tracing::error!( | ||
| "daemon: mutex poisoned — recovering inner value; \ | ||
| daemon state may be inconsistent" | ||
| ); | ||
| } | ||
| p.into_inner() | ||
| }) |
Comment on lines
+122
to
+128
| // Truncate the URL to bound memory usage and prevent very long URLs | ||
| // from being stored in the boundary log. | ||
| let url = if url.len() > MAX_NAV_URL_LEN { | ||
| url.chars().take(MAX_NAV_URL_LEN).collect() | ||
| } else { | ||
| url | ||
| }; |
| .map(|c| { | ||
| // `is_ascii_control()` returns true for 0x00–0x1F and 0x7F. | ||
| // We want to allow LF (0x0A) and TAB (0x09), so exclude those. | ||
| if c.is_ascii() && c != '\n' && c != '\t' && (c.is_ascii_control() || c == '\x7f') { |
Comment on lines
+94
to
+110
| fn full_string_rejects_length_above_max_fetch_with_zero_allocation() { | ||
| // u64::MAX is far above MAX_FETCH; the error must be returned before | ||
| // any allocation or network I/O — demonstrated by using a port that | ||
| // nothing is listening on (no connect attempt should be made either, | ||
| // since we short-circuit in full_string before calling substring). | ||
| use std::net::TcpListener; | ||
| let listener = TcpListener::bind("127.0.0.1:0").unwrap(); | ||
| let port = listener.local_addr().unwrap().port(); | ||
| // Accept in a thread so the client connect doesn't block — though we | ||
| // expect full_string to error before it even calls substring/connect. | ||
| let _handle = std::thread::spawn(move || { | ||
| let _ = listener.accept(); // may or may not be called | ||
| }); | ||
|
|
||
| let mut transport = | ||
| RdpTransport::connect_raw("127.0.0.1", port, Duration::from_secs(5)).unwrap(); | ||
| // Use a length larger than MAX_FETCH but representable as u64. |
Comment on lines
+120
to
+133
| fn full_string_rejects_u64_max_with_zero_allocation() { | ||
| // u64::MAX cannot even be represented as usize on 32-bit platforms | ||
| // (usize::try_from will fail). The check must fire before any alloc. | ||
| use std::net::TcpListener; | ||
| let listener = TcpListener::bind("127.0.0.1:0").unwrap(); | ||
| let port = listener.local_addr().unwrap().port(); | ||
| let _handle = std::thread::spawn(move || { | ||
| let _ = listener.accept(); | ||
| }); | ||
|
|
||
| let mut transport = | ||
| RdpTransport::connect_raw("127.0.0.1", port, Duration::from_secs(5)).unwrap(); | ||
| let err = LongStringActor::full_string(&mut transport, "longstr1", u64::MAX).unwrap_err(); | ||
| assert!( |
Comment on lines
+1366
to
+1368
| // Spawn a server that sends many rapid dom-loading events but never | ||
| // sends dom-complete. The events will flood the receiver channel so | ||
| // the old (post-drain) deadline check could be starved. |
- daemon/server.rs RefStore::register: enforce MAX_REFS per insert so a single batch can't push the store past the cap; align doc comment with actual per-entry skip behaviour; clarify lock_or_recover! logs once per call site (the static is macro-local). - daemon/buffer.rs MAX_NAV_URL_LEN: truncate by byte index on a UTF-8 char boundary so the documented byte bound holds for non-ASCII URLs. - core/util/terminal.rs: drop redundant '\x7f' check (covered by is_ascii_control). - core/actors/string.rs + cli/commands/navigate.rs: tighten test comments to match what they actually verify (no allocation-counting; the pre-loaded mpsc channel — not the server thread — supplies the dom-loading flood). - kb/iterations/iteration-61w: mark the five ACs whose tests were not written in this PR as unchecked, with a short follow-up note each; rename the ticked ACs to the real test function names that landed; drop AC count from 12/12 to 7/12; flip status to in-progress. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
iter-61w landed the security-hardening code but only 5 of 12 promised tests. The "honest commits" iteration is the right home for closing that gap, so add a new theme with each missing test named and located, plus an AC that re-ticks iter-61w to [12/12] once they land. 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
Closes the security-audit findings against the daemon and transport, hardens small operational hazards, adds typed bulk-packet rejection for forward compatibility, and refreshes stale kb pages.
Theme A — Daemon auth hardening
subtle::ConstantTimeEq)RefStorebounded:MAX_REFS=50_000,resolver.len() <= 4096NavBoundary.urltruncated toMAX_NAV_URL_LEN=4096resourceTypevalidated againstWATCHED_RESOURCE_TYPESTheme B — Operator-surface hygiene
ff_rdp_core::util::terminal::sanitize_for_terminalreplaces control chars (except\n/\t) with?eprintln!ineval,js_helpers,network_eventsFF_RDP_TRACE_RAW=1prints a one-line startup warninglock().unwrap()→ poison-recoveringlock_or_recover!macro (logs once)Theme C — Bulk packet typed rejection
transport::recv_fromdetects leadingb, parses header, discards bodyProtocolError::BulkPacketUnsupported { actor, kind, length }Theme E — LongString cap hoist (post-61v FINDING-N1)
LongStringActor::full_stringenforcesMAX_FETCH=16 MiBbefore any allocationu64::MAXreturns typed error with zero allocationTheme F — wait_for_doc_complete deadline (post-61v FINDING-N4)
timeout_ms + 100msTheme D — Documentation refresh
lessons-learned.md: rewroteasync-eval-doesnt-resolve-promises(closed iter-61r); annotated 3 more entries with closed-inopen-gaps.md: 6 resolved entries moved to new## Closed gapsff-rdp-wins.md§4 annotated wired in iter-61twatcher.md: method support matrix addedwired-vs-primitive.mdsnapshot of 61p/q/r primitivesstability-roadmap.md: 61t..61w map + 61m..61s deferral post-mortemUnbounded-insert audit (theme A follow-up)
Audited daemon for unbounded inserts beyond the four addressed above; remaining are bounded by upstream Firefox (one entry per console message / one per nav event in a buffer already capped by ring size). No additional caps needed.
Test plan
cargo fmt && cargo clippy --workspace --all-targets -- -D warnings && cargo test --workspace -q— clean (753 tests pass)FF_RDP_LIVE_TESTS=1 cargo test-live(recommended before merge)