iter-54: protocol correctness & robustness#57
Merged
Conversation
- transport: cap RDP frame at 64 MiB (MAX_FRAME_BYTES); reject oversized length prefixes with ProtocolError::FrameTooLarge before any allocation. - actor/root/console: correlate replies by absence of `type` field where safe (root.listTabs, console eval ack); removes the listTabs retry hack and the eval-loop event-sniff workaround. - console.evaluateJSAsync: abort with EvalNavigatedDuringEval when tabNavigated/willNavigate arrives mid-eval (was hanging until socket read timeout). - object: add ObjectActor::release and ScopedGrip wrapper for releasing server-side actor IDs (object/longString grips) without silent failures from Drop. - network: unwrap longString grips in response bodies via LongStringActor::full_string, capped at MAX_FRAME_BYTES; surface a truncated flag on ResponseContent. 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 selected for processing (4)
📝 WalkthroughWalkthroughThis PR implements protocol-correctness hardening across ff-rdp-core: introducing frame size limits and navigation-abort errors, fixing reply/push-event correlation via type-field semantics, updating console evaluation and root list tabs to skip push events, adding object release mechanisms, and unwrapping longString network responses with truncation tracking. ChangesProtocol Correctness & Robustness (Iteration 54)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ff-rdp-core/src/actors/console.rs`:
- Around line 169-173: The handler currently aborts eval on any
"tabNavigated"/"willNavigate" regardless of origin; narrow the check to only
treat navigation events coming from the evaluated actor/target by verifying the
message's "from" field matches the actor id being evaluated (e.g., compare
msg.from / message.from against the evaluated actor identifier used in this
handler) before returning Err(ProtocolError::EvalNavigatedDuringEval); update
the conditional in the console message handling (where msg_type is compared) to
include that "from" equality check so unrelated navigations on the same
connection don't abort the eval.
In `@crates/ff-rdp-core/src/actors/object.rs`:
- Around line 120-141: The release method currently only releases Grip::Object
actors; extend the match in pub fn release(self, transport: &mut RdpTransport)
-> Result<Grip, ProtocolError> to also handle Grip::LongString (and any similar
actor-bearing grip variants) by extracting the actor id and calling the
corresponding long-string release helper (e.g. LongStringActor::release or the
existing long-string release function) with the same error-handling policy used
for ObjectActor::release (silently swallow ActorError/unknownActor, return other
transport errors), then return Ok(self.inner) as before.
- Around line 137-138: The current match arm in the object handling code
swallows every ProtocolError::ActorError; instead, only ignore the specific
"unknownActor" case. Update the match in the function where
ProtocolError::ActorError is handled (the match containing Ok(()) |
Err(ProtocolError::ActorError { .. }) => {}) to pattern-match the ActorError
payload and only treat the "unknownActor" variant/code/message as non-fatal
(e.g., Err(ProtocolError::ActorError { code, .. }) if code == "unknownActor" =>
{}), and let all other ActorError instances fall through to the Err(e) => return
Err(e) branch so real protocol errors are propagated. Ensure you use the actual
field/variant names of ProtocolError::ActorError as defined in its enum to
implement the specific check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ffd30a2-d680-48c8-a5b0-92b5d12dc630
📒 Files selected for processing (10)
crates/ff-rdp-cli/src/commands/connect_tab.rscrates/ff-rdp-core/src/actor.rscrates/ff-rdp-core/src/actors/console.rscrates/ff-rdp-core/src/actors/network.rscrates/ff-rdp-core/src/actors/object.rscrates/ff-rdp-core/src/actors/root.rscrates/ff-rdp-core/src/error.rscrates/ff-rdp-core/src/lib.rscrates/ff-rdp-core/src/transport.rskb/iterations/iteration-54-protocol-correctness.md
There was a problem hiding this comment.
Pull request overview
This PR hardens ff-rdp-core’s Firefox RDP handling for better protocol correctness and resilience against malformed/malicious peers, while also improving behavior around interleaved push events and large payloads.
Changes:
- Add a 64 MiB maximum RDP frame size and reject oversized frames pre-allocation (
FrameTooLarge). - Improve handling of interleaved push events for
root.listTabsandconsole.evaluateJSAsync, including aborting eval on navigation events. - Add longString unwrapping for network response bodies (with truncation reporting) and introduce
ObjectActor::release+ScopedGrip.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| kb/iterations/iteration-54-protocol-correctness.md | Adds iteration plan/acceptance criteria for the protocol-hardening work. |
| crates/ff-rdp-core/src/transport.rs | Introduces MAX_FRAME_BYTES and rejects oversized frames; adds a unit test. |
| crates/ff-rdp-core/src/lib.rs | Re-exports ScopedGrip from actors::object. |
| crates/ff-rdp-core/src/error.rs | Adds EvalNavigatedDuringEval and FrameTooLarge protocol errors. |
| crates/ff-rdp-core/src/actors/root.rs | Removes the listTabs retry hack; filters root push events by presence of type. |
| crates/ff-rdp-core/src/actors/object.rs | Adds ObjectActor::release and ScopedGrip for explicit server-actor release. |
| crates/ff-rdp-core/src/actors/network.rs | Unwraps longString response bodies via LongStringActor::full_string; adds truncated. |
| crates/ff-rdp-core/src/actors/console.rs | Filters eval ack by “no type”; aborts eval wait on navigation events; adds tests. |
| crates/ff-rdp-core/src/actor.rs | Documents why generic actor_request does not universally filter by missing type. |
| crates/ff-rdp-cli/src/commands/connect_tab.rs | Minor formatting/indentation adjustment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
165
to
173
| loop { | ||
| let msg = transport.recv()?; | ||
| let msg_type = msg.get("type").and_then(Value::as_str).unwrap_or_default(); | ||
|
|
||
| // Skip console push events forwarded by the daemon — same reason as above. | ||
| if msg_type == "consoleAPICall" || msg_type == "pageError" { | ||
| continue; | ||
| // Watch for navigation events that signal the eval result will never arrive. | ||
| // These are push events (they carry `type`) from the target actor. | ||
| if msg_type == "tabNavigated" || msg_type == "willNavigate" { | ||
| return Err(ProtocolError::EvalNavigatedDuringEval); | ||
| } |
Comment on lines
+130
to
+138
| /// Other transport-level errors are returned to the caller. | ||
| /// | ||
| /// Returns the inner grip so the caller can still inspect it after release. | ||
| pub fn release(self, transport: &mut RdpTransport) -> Result<Grip, ProtocolError> { | ||
| if let Grip::Object { ref actor, .. } = self.inner { | ||
| let actor_id = actor.as_ref().to_owned(); | ||
| match ObjectActor::release(transport, &actor_id) { | ||
| Ok(()) | Err(ProtocolError::ActorError { .. }) => {} | ||
| Err(e) => return Err(e), |
Comment on lines
+120
to
+126
| /// Consume the wrapper and release the server-side actor. | ||
| /// | ||
| /// For `Grip::Object` variants, sends `release` to the grip actor so | ||
| /// Firefox can free the associated server-side memory immediately. | ||
| /// For all other variants (primitives, longStrings) this is effectively | ||
| /// a no-op — longString actors are reclaimed automatically by Firefox's | ||
| /// GC on the next cycle, so explicit release is not required. |
Comment on lines
+43
to
+49
| /// grip returned by `evaluateJSAsync`. In long-lived daemon connections | ||
| /// these actors accumulate and are never reclaimed. Sending `release` | ||
| /// to the grip actor asks Firefox to destroy it. | ||
| /// | ||
| /// Note: the connection dropping (end of `--no-daemon` mode) also | ||
| /// releases all actors implicitly, so calling this is only necessary in | ||
| /// daemon mode. |
Comment on lines
16
to
+26
| /// Response content from a network event. | ||
| #[derive(Debug, Clone)] | ||
| pub struct ResponseContent { | ||
| /// MIME type of the response. | ||
| pub mime_type: String, | ||
| /// Response body size in bytes. | ||
| pub size: u64, | ||
| /// Response body text (may be absent for binary content). | ||
| pub text: Option<String>, | ||
| /// True when the body was truncated at [`MAX_FRAME_BYTES`] due to size limits. | ||
| pub truncated: bool, |
- console: scope eval-abort navigation events to evaluated console actor - ScopedGrip::release: only swallow unknownActor; release longString actors too - ResponseContent: mark #[non_exhaustive] to allow non-breaking extension - docs: drop CLI-specific --no-daemon reference from core crate - docs: fix EvalNavigatedDuringEval reference in evaluate_js_async docs Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Accidentally bundled in previous commit; restore as untracked. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tick scope checkboxes that landed; document deferred items (actor_request type-skip, live e2e fixtures, daemon ScopedGrip wiring, startListeners removal) for follow-up in iter-55. 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
ProtocolError::FrameTooLargebefore allocation).typefield inroot.listTabsandconsoleeval ack — removes thelistTabsretry hack and the eval-loop event-sniff workaround.evaluateJSAsyncwithEvalNavigatedDuringEvalwhentabNavigated/willNavigatearrives mid-eval (no longer hangs until socket timeout).ObjectActor::releaseand aScopedGripwrapper for releasing server-side actor IDs (object / longString grips) without silentDropfailures.longStringgrips in network response bodies viaLongStringActor::full_string, capped atMAX_FRAME_BYTES, with atruncatedflag onResponseContent.Pure
ff-rdp-corerefactor — no CLI surface changes.Task 6 (remove legacy
startListeners) was deferred: it lives inff-rdp-cli/src/daemon/server.rsand is exercised by 10+ daemon e2e fixtures. Removing it is a CLI-visible change and out of scope for this pure-core iteration; will be split into its own iteration.Test plan
cargo fmtcargo clippy --workspace --all-targets -- -D warningscargo test --workspace -q— all unit + integration tests pass; live-Firefox tests remain ignored as beforeevaluateJSAsyncaborts ontabNavigatedandwillNavigate; eval ack skips push events; network response unwrapslongStringgrips inline + via grip path🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes