Repository navigation
feat(web): support WebSocket subprotocols - #1970
MuNeNiCK (MuNeNiCK) wants to merge 4 commits into
Conversation
|
Automated review will not run because this contributor is not yet eligible under the automation policy. Contributors become eligible after one qualifying IronRDP pull request is merged into |
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Hi! Thank you for the PR.
I think this is something I would like to have in the main "iron-remote-desktop" API, not as an extension. Could you change that before we merge?
Thank you!
| ] } | ||
| js-sys = "0.3" | ||
| gloo-net = { version = "0.7", default-features = false, features = ["websocket", "http", "io-util"] } | ||
| gloo-net = { version = "0.7", default-features = false, features = ["websocket", "http", "io-util", "json"] } |
There was a problem hiding this comment.
question: Why json feature is necessary now?
There was a problem hiding this comment.
The json feature was required only because gloo-net::WebSocket::open_with_protocols serializes the protocol slice through Serde. I changed the implementation to construct the browser WebSocket with web_sys::WebSocket::new_with_str_sequence and then wrap it with the existing gloo WebSocket TryFrom implementation. The json, serde, and serde_json dependencies are no longer added.
There was a problem hiding this comment.
🟢 Approval recommended
The implementation correctly preserves the default connection path, with only a non-blocking documentation-format finding.
Pull request overview
Adds configurable WebSocket subprotocol negotiation to the RDP web backend while preserving existing behavior when none are configured.
Changes:
- Exposes
webSocketProtocols()for consumers. - Validates and forwards protocol arrays when opening WebSockets.
- Documents the configuration helper.
File summaries
| File | Description |
|---|---|
web-client/iron-remote-desktop-rdp/src/main.ts |
Adds the configuration extension. |
web-client/iron-remote-desktop-rdp/README.md |
Documents subprotocol configuration. |
crates/ironrdp-web/src/session.rs |
Parses protocols and selects the appropriate WebSocket constructor. |
crates/ironrdp-web/Cargo.toml |
Enables required gloo-net JSON support. |
Cargo.lock |
Records transitive serialization dependencies. |
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 1
- Review effort level: Balanced
| WebSocket endpoints use subprotocol negotiation for application protocol | ||
| selection, versioning, and handshake authentication. Pass | ||
| `webSocketProtocols()` to the configuration builder when the endpoint requires | ||
| one or more subprotocols during the opening handshake: |
There was a problem hiding this comment.
Fixed. The documentation is now in the main iron-remote-desktop README, and each sentence is kept on one source line.
d3e5730 to
2329a44
Compare
2329a44 to
5043766
Compare
|
Thank you! I moved the option into the main |
5043766 to
5b0eaef
Compare
There was a problem hiding this comment.
PR #1970 threads an optional WebSocket subprotocol list through the Rust SessionBuilder trait, wasm bridge, TS Config/ConfigBuilder, and remote-desktop.service, opening the socket via web_sys::WebSocket::new_with_str_sequence when protocols are set and preserving WebSocket::open otherwise. The design matches the maintainer's request to place the option in the main iron-remote-desktop API, and no Cargo dependency changes were needed. Verified independently: the internal::web_sys::js_sys path resolves, the empty-list fallback is behavior-preserving, and Config is only built via ConfigBuilder.build() so the new required configOptions field is not a practical break. Three low-severity issues stand: all-or-nothing validation of the protocols array fails silently with only a warn, the new WebSocket-open failure path is wrapped twice with identical context, and the js_sys::Array is needlessly round-tripped through Vec<String>.
| /// Optional | ||
| fn websocket_protocols(&self, protocols: js_sys::Array) -> Self { | ||
| let protocols = protocols | ||
| .iter() | ||
| .map(|protocol| protocol.as_string()) | ||
| .collect::<Option<Vec<_>>>(); | ||
|
|
||
| if let Some(protocols) = protocols { | ||
| self.0.borrow_mut().websocket_protocols = protocols; | ||
| } else { | ||
| warn!("WebSocket protocols must be strings"); | ||
| } | ||
|
|
||
| self.clone() | ||
| } |
There was a problem hiding this comment.
[skeptical] Non-string protocol entries silently discard the whole list with no caller-visible error — low 🟡 — If any element of the js_sys::Array is not a string, collect::<Option<Vec<_>>> yields None and the setter only logs warn!("WebSocket protocols must be strings"), leaving any previously stored value untouched. A JS consumer bypassing the TS string[] type gets a builder that appears configured but connects with no or stale subprotocols, surfacing later as an opaque handshake failure. Failing loudly or documenting the all-or-nothing warn-only behavior would make the misconfiguration visible.
There was a problem hiding this comment.
Thank you — this is a valid finding. Fixed in d843982. Invalid protocol entries are recorded as an error and returned from connect(), rather than retaining the previous list or silently connecting without subprotocols. A subsequent valid list, including an empty list, clears the error while preserving the fluent setter API.
I verified the generated WASM bridge with a mocked WebSocket: invalid input never opens a socket, valid-to-invalid configuration does not reuse stale protocols, and subsequent valid/empty configuration recovers correctly.
| let ws = if websocket_protocols.is_empty() { | ||
| WebSocket::open(&proxy_address) | ||
| } else { | ||
| let protocols = js_sys::Array::from_iter(websocket_protocols.iter().map(JsValue::from)); | ||
| let socket = web_sys::WebSocket::new_with_str_sequence(&proxy_address, &protocols) | ||
| .map_err(|error| anyhow::anyhow!("couldn't open WebSocket: {error:?}"))?; | ||
| WebSocket::try_from(socket) | ||
| } | ||
| .context("couldn't open WebSocket")?; |
There was a problem hiding this comment.
[skeptical] new_with_str_sequence failure message is wrapped twice with the same context — low 🟡 — On the non-empty protocols branch, the new_with_str_sequence error is already mapped to anyhow!("couldn't open WebSocket: {error:?}") and the unified result then passes through .context("couldn't open WebSocket"), producing a doubled prefix on the new failure path. Either the inner map_err or the outer context alone suffices, which matters because the adjacent NOTE already observes WebSocket open failures are poorly diagnosed.
There was a problem hiding this comment.
I checked this path in the original implementation. The ? immediately after new_with_str_sequence(...).map_err(...) returns from the enclosing async connect() method, so a constructor error never reaches the outer .context("couldn't open WebSocket"). Fromanyhow::Error for IronError also preserves the source without adding context.
The outer context is used when WebSocket::open or WebSocket::try_from fails; the inner mapping is used when the browser constructor fails. I verified these paths in a minimal Rust reproduction and checked constructor/setup failures through the generated WASM bridge with a mocked WebSocket. The prefix appears once in each case, so I am keeping the existing error handling.
| let ws = if websocket_protocols.is_empty() { | ||
| WebSocket::open(&proxy_address) | ||
| } else { | ||
| let protocols = js_sys::Array::from_iter(websocket_protocols.iter().map(JsValue::from)); | ||
| let socket = web_sys::WebSocket::new_with_str_sequence(&proxy_address, &protocols) | ||
| .map_err(|error| anyhow::anyhow!("couldn't open WebSocket: {error:?}"))?; | ||
| WebSocket::try_from(socket) | ||
| } | ||
| .context("couldn't open WebSocket")?; |
There was a problem hiding this comment.
[code-compressor] Vec<String> round-trip of js_sys::Array can be replaced by storing the array — low 🟡 — The builder decodes the incoming js_sys::Array into a Vec<String> (SessionBuilderInner field, its Default entry, and the clone in the builder getter), then re-encodes it with Array::from_iter at connect time. Storing the array directly (e.g. Option<js_sys::Array>, a cheap JsValue clone) keeps the as_string validation and the empty/None fallback to WebSocket::open while removing the Vec field, its Default entry, the decode-clone, and the re-encode. Maintainability-only; no behavior change.
There was a problem hiding this comment.
Addressed in d843982. The builder now retains a validated js_sys::Array instead of round-tripping through Vec. The setter creates a fresh array, checks each entry with is_string(), and pushes the validated value into that array. This avoids decoding to Rust strings and reconstructing a JS array at connect time.
The independent copy is intentional: Array::clone() shares the same JS object, which would let later caller mutations alter an already validated configuration. Copying the entries preserves the current snapshot behavior. Omitted/empty lists still use WebSocket::open, and invalid entries are retained as a connect() error.
The generated-WASM checks cover caller mutation, replacement of the configured list, and an accessor-backed entry being read only once. These passed.
|
Hi! MuNeNiCK (@MuNeNiCK) |
|
Hi Benoît Cortier (@CBenoit), thank you for the reminder. I checked all three automated findings and replied to each thread. The follow-up fixes are now pushed in d843982. Invalid protocol entries now fail explicitly at connect(), and the builder stores an independent, validated JS array instead of round-tripping through Vec. The independent copy preserves the existing behavior when callers later mutate their input array. Omitted and empty lists keep the default connection path. The duplicate-error-context finding is a false positive: the inner ? exits connect() before the outer context can run. I verified that the error prefix occurs once, so that handling is unchanged. Validation passed: the WASM build, targeted WASM Clippy, Rust formatting, TypeScript checks, web lint, all 18 existing web tests, and 12 checks against the generated WASM bridge with a mocked WebSocket. Those checks cover input mutation, invalid-input rejection, recovery through valid/empty configuration, and constructor/setup errors. The changes are ready for your review. Thank you! |
There was a problem hiding this comment.
PR adds optional WebSocket subprotocol negotiation to the iron-remote-desktop API, flowing from ConfigBuilder.withWebSocketProtocols through SessionBuilder.webSocketProtocols to a web_sys::WebSocket::new_with_str_sequence connection path in ironrdp-web. Implementation is correct on independent review: validation defers non-string entries to an explicit connect() error, the JS array is defensively copied, and omitted/empty lists keep the default WebSocket::open path. Two low-severity maintainability findings are published: the new validation/error-recovery paths have no automated test coverage (only mock updates in an unrelated test file), and the Result<Option<Array>> storage plus connect-time empty-array filter is a redundant layer that can be removed by normalizing empty arrays to Ok(None) in the setter without behavior change.
| /// Optional | ||
| fn websocket_protocols(&self, protocols: js_sys::Array) -> Self { | ||
| // Copy validated values so later changes to the caller's array cannot change the configuration. | ||
| let protocols = protocols | ||
| .iter() | ||
| .try_fold(js_sys::Array::new(), |copy, protocol| { | ||
| if !protocol.is_string() { | ||
| return Err("websocket protocols must be strings"); | ||
| } | ||
|
|
||
| copy.push(&protocol); | ||
| Ok(copy) | ||
| }) | ||
| .map(Some); | ||
|
|
||
| if let Err(error) = protocols { | ||
| warn!(%error, "Invalid WebSocket protocols"); | ||
| } | ||
|
|
||
| self.0.borrow_mut().websocket_protocols = protocols; | ||
|
|
||
| self.clone() |
There was a problem hiding this comment.
[skeptical] WebSocket protocol validation and error paths lack test coverage — low 🟡 — The new setter validates entries, retains invalid input as an error state, and defers failure to connect(), with snapshot-copy semantics; none of this is covered by committed tests. Only MockSessionBuilder stubs and empty-array literals in enableFileTransfer.test.ts were added, which pass through plumbing without exercising validation, error retention/recovery, or copy behavior. The mocked-WASM checks cited in PR discussion are not in the repository.
There was a problem hiding this comment.
Addressed in 99b54c2. Added 15 committed regression tests in crates/ironrdp-web/tests/websocket_protocols.cjs. They exercise the actual exported WASM SessionBuilder with mocked browser objects: non-string rejection before opening a socket, retained errors across repeated connections, recovery through valid/empty setters, defensive copying and single reads of accessor entries, protocol ordering, omitted/empty defaults, and constructor/wrapper error propagation.
The tests are wired into cargo xtask web check, so the existing Web Client CI job runs them. The crate README also documents standalone execution. Locally, the full web check passed (15 WASM API tests plus the 18 existing Vitest tests, builds, type checks, and lints); formatting and targeted Clippy checks passed as well.
| server_domain: Option<String>, | ||
| password: Option<String>, | ||
| proxy_address: Option<String>, | ||
| websocket_protocols: Result<Option<js_sys::Array>, &'static str>, |
There was a problem hiding this comment.
[code-compressor] Empty-array normalization would remove the redundant Option layer — low 🟡 — websocket_protocols stores Result<Option<js_sys::Array>, &'static str>, but empty arrays are documented to behave the same as omission. connect() must therefore strip Some(empty) with a runtime .filter(length != 0) (line 504). Storing an empty array as Ok(None) in the setter is behavior-preserving — it selects the default handshake and clears any prior invalid-input error — and removes the Option wrapper and the connect-time filter, simplifying the connect path.
There was a problem hiding this comment.
Addressed in 99b54c2. The setter now normalizes an empty validated array to Ok(None), and connect() no longer needs the empty-array filter. Empty input still clears an earlier validation error and selects the default one-argument WebSocket constructor; the committed WASM tests cover both behaviors.
The Option type remains to represent the default configuration without allocating a JS array for an untouched builder; the redundant Some(empty) state and connect-time filtering are removed.
There was a problem hiding this comment.
The PR adds WebSocket subprotocol support through the main iron-remote-desktop API (Rust trait, WASM bridge, TS Config/ConfigBuilder/service) with a validated, defensively-copied js_sys::Array in ironrdp-web, plus 15 committed Node regression tests wired into cargo xtask web check. The change is correct and well-tested; all five specialist candidates are valid but low severity. Two duplicate findings about the required webSocketProtocols field on the exported Config constructor are merged into one finding (semver/API friction only, since the documented ConfigBuilder path is unaffected). The non-array-input finding is valid but refined: non-array direct-WASM inputs bypass the documented connect() error contract, while the TS layer guards normal callers. The remaining two findings (duplicated WebSocket error prefix across short-circuiting paths, and a test pinning single-read iteration order) are accepted as valid low-severity maintainability/test-fragility concerns.
| readonly password: string; | ||
| readonly destination: string; | ||
| readonly proxyAddress: string; | ||
| readonly webSocketProtocols: string[]; |
There was a problem hiding this comment.
[skeptical + code-compressor] Required webSocketProtocols field on exported Config is a breaking, boilerplate-forcing TS API change — low 🟡 — Config is exported from the package (src/main.ts line 15) and its configOptions type now requires webSocketProtocols: string[] (constructor at line 21), so consumers who construct Config directly get a TypeScript compile error on upgrade - a semver-relevant surface change not called out anywhere. It also forces webSocketProtocols: [] on every hand-built config (three additions in enableFileTransfer.test.ts) even though empty and omitted select the identical default WebSocket path in ironrdp-web. The documented ConfigBuilder path is unaffected since build() always supplies the field. Typing it optional and coercing to [] in remote-desktop.service.ts would avoid the break and the boilerplate.
There was a problem hiding this comment.
Fixed in 69e5594. webSocketProtocols is now optional both on Config and in its constructor options. The constructor defaults it to [], and the service also uses ?? [] to support existing hand-built config objects. The three unrelated empty-array additions in the existing tests have been removed.
Added constructor compatibility tests and service forwarding coverage. The full web check passes, including type checks for the old constructor/options and hand-built config forms, 21 Vitest tests, and 21 WASM API tests.
| fn websocket_protocols(&self, protocols: js_sys::Array) -> Self { | ||
| // Copy validated values so later changes to the caller's array cannot change the configuration. | ||
| let protocols = protocols | ||
| .iter() | ||
| .try_fold(js_sys::Array::new(), |copy, protocol| { | ||
| if !protocol.is_string() { | ||
| return Err("websocket protocols must be strings"); | ||
| } | ||
|
|
||
| copy.push(&protocol); | ||
| Ok(copy) | ||
| }) | ||
| .map(|protocols| (protocols.length() != 0).then_some(protocols)); |
There was a problem hiding this comment.
[skeptical] Protocol setter validates element string-ness but never checks the input is an Array — low 🟡 — The setter checks only that each element is_string(), and wasm-bindgen passes js_sys::Array parameters through without an instanceof check, so a direct WASM caller passing a string or arbitrary object is not rejected with the documented 'websocket protocols must be strings' connect() error. Such inputs instead bypass the documented invalid-entries contract (typically failing at setter time with an iterator TypeError from the values()-based iteration, or being silently reinterpreted by exotic array-likes). An Array.isArray-style check in the setter would close the gap cheaply; TS callers are already guarded by the string[] interface type.
There was a problem hiding this comment.
Fixed in 69e5594. The setter now checks Array::is_array before accessing length or entries. Non-array inputs retain an error state and cause connect() to reject with websocket protocols must be an array; they cannot silently replace the configuration with default or character-by-character protocols.
Committed WASM tests cover strings, objects, array-like objects, null, undefined, and typed arrays, plus stale-setting replacement, repeated rejection, and recovery through valid/empty setters. These pass through the actual exported bridge without throwing in the setter.
| let ws = if let Some(protocols) = websocket_protocols { | ||
| let socket = web_sys::WebSocket::new_with_str_sequence(&proxy_address, &protocols) | ||
| .map_err(|error| anyhow::anyhow!("couldn't open WebSocket: {error:?}"))?; | ||
| WebSocket::try_from(socket) | ||
| } else { | ||
| WebSocket::open(&proxy_address) | ||
| } | ||
| .context("couldn't open WebSocket")?; |
There was a problem hiding this comment.
[skeptical] Duplicated couldn't-open-WebSocket prefix hides that the outer context does not cover constructor failure — low 🟡 — Syntactically the trailing .context("couldn't open WebSocket")? appears to wrap all WebSocket construction paths, but the ? after new_with_str_sequence(...).map_err(...) returns from connect() directly, so constructor failures skip the outer context and rely on a hand-written prefix with a Debug-formatted JsValue. Current behavior is correct and pinned by committed tests (exactly one prefix), but the prefix string now exists in two paths with different error shapes (flat anyhow message vs. context chain), and a future edit to the outer context string will silently diverge the constructor-failure message. Routing the constructor error through a single context point or a shared constant would remove the trap.
There was a problem hiding this comment.
Addressed in 69e5594. Browser-constructor errors and gloo-wrapper errors now flow through the single trailing .context("couldn't open WebSocket"). The branch no longer returns early with a manually duplicated prefix.
Expanded the WASM error tests to cover constructor and wrapper-setup failures with both omitted and configured protocols. All four cases preserve the underlying error details and contain exactly one WebSocket error prefix.
| test('each entry is read once and its validated value is stored', async (t) => { | ||
| const b = builder(t); | ||
| const input = ['binary']; | ||
| let reads = 0; | ||
| Object.defineProperty(input, 0, { | ||
| get() { | ||
| return ++reads === 1 ? 'binary' : 42; | ||
| }, | ||
| }); | ||
| b.webSocketProtocols(input).free(); | ||
| assert.deepEqual((await offered(b)).protocols, ['binary']); | ||
| assert.equal(reads, 1); | ||
| }); |
There was a problem hiding this comment.
[code-compressor] Test pins single-read iteration order, a non-contractual implementation detail — low 🟡 — This test installs a getter on the input array and asserts reads === 1, locking the setter to a single pass over the caller's array. That access pattern is an internal detail of the try_fold implementation, not part of the documented API (values are validated, copied, and offered in order). A behavior-preserving refactor such as an all(is_string) pre-pass before copying would fail CI despite identical observable behavior. Keeping the fixture and the stored-value assertion while dropping the reads counter preserves full defensive-copy coverage via the adjacent mutation tests.
There was a problem hiding this comment.
Addressed in 69e5594. Removed the assertion about the number of getter reads and renamed the test around the observable requirement: an accessor cannot substitute an unvalidated value into the stored protocols.
The changing-value getter fixture and saved-value assertion remain intentional. A validation pre-pass followed by reading the caller's array again can copy a different, unvalidated value, so that would not preserve behavior for this input. The test now checks that outcome without prescribing an exact read count.
Motivation
WebSocket subprotocol negotiation is a standard part of the opening handshake. Endpoints commonly use it for application protocol selection, version negotiation, and handshake authentication, especially in browser clients that cannot attach arbitrary request headers.
ironrdp-webcurrently creates its WebSocket internally with only the proxy URL, so consumers cannot connect to endpoints that require one or more subprotocols.Changes
webSocketProtocols()configuration extension from the RDP web packageironrdp-webWebSocket::open_with_protocolswhen protocols are configuredWebSocket::openpath when the option is omitted or emptyValidation
cargo xtask wasm check -vcargo xtask web check -vcargo xtask check fmt -vcargo xtask check lints -vcargo xtask check locks -v