feat(acceptor)!: clamp honored client desktop size to an operator maximum#1404
Open
clintcan wants to merge 2 commits into
Open
feat(acceptor)!: clamp honored client desktop size to an operator maximum#1404clintcan wants to merge 2 commits into
clintcan wants to merge 2 commits into
Conversation
…imum Follow-up to Devolutions#1373. `set_honor_client_desktop_size` (acceptor) and `with_honor_client_desktop_size` (server builder) took a bare `bool`, so enabling the feature honored the client-requested desktop size bounded only by the protocol range [200, 8192]. The desktop size is a client-controlled `u16`, so a client could request e.g. 8192x8192 and drive the server's framebuffer/encoder allocation (~256 MiB per frame buffer) off an untrusted number — a mild resource-exhaustion vector on an opt-in, default-off path. Replace the `bool` with `Option<DesktopSize>` carrying an operator-set maximum: * `None` (default) — disabled; always enforce the server-provided size. * `Some(max)` — honor the client's request, clamped per dimension to `max`. The client can ask for a smaller desktop, never a larger one. The acceptor clamps the requested width/height to `max` before the existing `validate_desktop_size` protocol-range check, so the negotiated size can never exceed what the operator is willing to render (e.g. the host display's native resolution). Breaking (pre-v1): `RdpServerOptions::honor_client_desktop_size` is now `Option<DesktopSize>`; `Acceptor::set_honor_client_desktop_size` and `RdpServerBuilder::with_honor_client_desktop_size` take `Option<DesktopSize>` instead of `bool`. Callers migrate `true` -> `Some(max)` (with a chosen ceiling) and `false` -> `None`.
There was a problem hiding this comment.
Pull request overview
This PR hardens the “honor client desktop size” feature by replacing the previous boolean toggle with an operator-specified maximum desktop size, ensuring the acceptor never negotiates (and therefore never allocates for) a client-driven size above the configured ceiling.
Changes:
- Change
honor_client_desktop_sizefrombooltoOption<DesktopSize>in server options and builder API. - Forward the optional maximum into the acceptor via
set_honor_client_desktop_size(Option<DesktopSize>). - Clamp the client-requested GCC Core Data desktop size per-dimension to the operator maximum before protocol-range validation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/ironrdp-server/src/server.rs | Updates RdpServerOptions::honor_client_desktop_size to Option<DesktopSize> and documents the operator ceiling semantics. |
| crates/ironrdp-server/src/builder.rs | Updates builder internal state + public builder method signature/docs to accept Option<DesktopSize>. |
| crates/ironrdp-acceptor/src/connection.rs | Updates acceptor configuration + handshake logic to clamp requested desktop size to the operator maximum before validation and adoption. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+535
to
558
| if let Some(max) = self.honor_client_desktop_size { | ||
| let requested_width = gcc_blocks.core.desktop_width; | ||
| let requested_height = gcc_blocks.core.desktop_height; | ||
| let clamped_width = requested_width.min(max.width); | ||
| let clamped_height = requested_height.min(max.height); | ||
| if let Some(client_size) = validate_desktop_size(clamped_width, clamped_height) { | ||
| if client_size != self.desktop_size { | ||
| debug!( | ||
| requested = ?client_size, | ||
| requested = ?DesktopSize { width: requested_width, height: requested_height }, | ||
| max = ?max, | ||
| adopted = ?client_size, | ||
| previous = ?self.desktop_size, | ||
| "Honoring client-requested desktop size" | ||
| "Honoring client-requested desktop size (clamped to operator maximum)" | ||
| ); | ||
| self.desktop_size = client_size; | ||
| set_bitmap_desktop_size(&mut self.server_capabilities, client_size); | ||
| } | ||
| } else { | ||
| debug!( | ||
| width = gcc_blocks.core.desktop_width, | ||
| height = gcc_blocks.core.desktop_height, | ||
| width = requested_width, | ||
| height = requested_height, | ||
| "Client requested an out-of-range desktop size; keeping the server-provided size" | ||
| ); | ||
| } |
Comment on lines
+149
to
+153
| /// Pass `Some(max)` to honor the client's request, clamped per dimension to | ||
| /// `max`: the client can ask for a *smaller* desktop than `max`, but never a | ||
| /// larger one. The desktop size is a client-controlled `u16` bounded only by | ||
| /// the protocol ([200, 8192]); without a ceiling, a client could request | ||
| /// e.g. 8192×8192 and drive the server's framebuffer/encoder allocation off |
Comment on lines
+248
to
+252
| /// Pass `Some(max)` to honor the client's request, clamped per dimension to | ||
| /// `max`: the client may ask for a smaller desktop, but never a larger one. | ||
| /// The desktop size is a client-controlled `u16` bounded only by the | ||
| /// protocol ([200, 8192]); `max` is the ceiling the server is willing to | ||
| /// render (for instance the host display's native resolution) so an |
Copilot review: the setter/builder docs still said the client request is honored only when the *request* is within the protocol-legal range, but the implementation now clamps to the operator maximum first and validates the *clamped* size — so an oversized request is honored (clamped down), not rejected. Reworded both doc blocks to match. Also the out-of-range debug log printed only the raw requested size and blamed the client, though the range check runs on the clamped size (misleading if `max` is itself misconfigured). It now logs requested + clamped + max and says 'out of protocol range after clamping'.
Contributor
Author
|
Addressed the Copilot review in
|
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.
Follow-up to #1373 (the resource-hardening angle you flagged in review — thanks for the go-ahead 🙂).
Problem
#1373gated honor-client-desktop-size behind a barebool. With it on, the acceptor adopts the client-requested desktop size bounded only by the protocol range[200, 8192]. But the desktop size is a client-controlledu16, and the server still builds its framebuffer/encoder from the negotiated size — so a client could request e.g.8192x8192and drive the server's allocation off an untrusted number (~256 MiB per frame buffer). Mild, and only on an opt-in default-off path, but it's a resource-exhaustion vector driven purely by a number the client picks.Your review comment: "[200, 8192] is a protocol ceiling, not a resource guard … tracked the 'clamp/range policy rather than a bare bool' idea as a future follow-up (an operator-set max size)." This is that PR.
Change
Replace the
boolwithOption<DesktopSize>carrying an operator-set maximum:None(default) — disabled; always enforce the server-provided size (unchanged behavior).Some(max)— honor the client's request, clamped per dimension tomax. The client can ask for a smaller desktop, never a larger one.The acceptor clamps the requested
width/heighttomaxbefore the existingvalidate_desktop_sizeprotocol-range check, so the negotiated size can never exceed what the operator is willing to render — setmaxto the host display's native resolution (or whatever ceiling the server can afford).Touches:
ironrdp-acceptor: field +set_honor_client_desktop_size(Option<DesktopSize>)+ clamp-then-validate in the Client Core Data adopt path.ironrdp-server:RdpServerOptions::honor_client_desktop_size: Option<DesktopSize>+RdpServerBuilder::with_honor_client_desktop_size(Option<DesktopSize>).Breaking (pre-v1)
RdpServerOptions::honor_client_desktop_sizeis nowOption<DesktopSize>; the acceptor setter and the builder method takeOption<DesktopSize>instead ofbool. Callers migratetrue→Some(max)(choosing a ceiling) andfalse→None. Same set of downstream server impls as before (lamco-rdp-server, hypr-rdp, cosmic-ext-rdp-server, ARISU, macrdp).Design points for review
min(requested, max)), so a client over the cap gets the largest allowed size rather than falling back to the server default. Per-dimension clamping can shift the aspect ratio (e.g. client1600x1200, max1920x1080→1600x1080); the display handler + client-side scaling absorb that, and it keeps honoring useful right up to the ceiling. Happy to switch to reject-if-over-max (keep server size) if you'd prefer no aspect change.test = falseand there's no acceptor harness inironrdp-testsuite-core. The clamp is a two-lineminahead of the existingvalidate_desktop_size; I'm glad to add coverage wherever you'd want acceptor tests to live (a pureclamp_desktop_sizehelper + a testsuite-core case, if useful).Built +
clippy --features egfx -D warningsclean onironrdp-acceptor+ironrdp-server.