[Piero (pierophp)]

Two halves of the same problem: a rejected clipboard operation is both invisible to the backend waiting on it and fatal to the session.

ironrdp-server: a clipboard error ends client_loop

dispatch_server_events propagates every CliprdrServer error with ?:

ClipboardMessage::SendFileContentsRequest(request) => cliprdr.request_file_contents(request),
...
}
.map_err_kind("failed to send clipboard event", ServerErrorKind::Pdu)?;

That error unwinds through dispatch_events → client_loop → client_accepted → accept_finalize, and the accept loop logs "Connection error", resets the static channels and calls on_disconnected. The session is torn down.

The things that can trigger it are all ordinary: a file contents request that became stale because the remote clipboard changed under it, a capability that was never negotiated, MAX_PENDING_FILE_REQUESTS backpressure, a malformed flag combination. None is a reason to disconnect a peer whose display, audio and input are fine.

Three lines above, ClipboardMessage::Error already takes the opposite view:

ClipboardMessage::Error(error) => {
    error!(?error, "Handling clipboard event");
    continue;
}

This makes the two paths agree.

ironrdp-cliprdr: a rejection strands its caller

request_file_contents has ten rejection paths and all of them return a bare PduError. The error carries no stream id, so an embedder that surfaces it has no way to map it back to the transfer that failed — whatever is awaiting that stream just hangs until an unrelated timeout fires.

The crate already solves this elsewhere. FormatListResponse::Fail does:

// Notify backend for each pending request so it can clean up
// (e.g. reject pending download promises in WASM).
for stream_id in stream_ids {
    self.backend.on_file_contents_response(FileContentsResponse::new_error(stream_id));
}

Every rejection in request_file_contents now does the same before returning. The Err is unchanged — this is strictly additive for existing callers.

Tests

• crates/ironrdp-testsuite-core/tests/clipboard/file_contents_state_machine.rs — a_rejected_request_fails_its_stream_on_the_backend (out-of-bounds index) and a_request_rejected_before_ready_fails_its_stream_on_the_backend (require_ready, the path most likely to strand a caller). Both assert the error is still returned and that the backend saw an error response for the right stream id.
• crates/ironrdp-server/src/server.rs — a_refused_clipboard_message_does_not_end_the_client_loop drives dispatch_server_events with a request the channel refuses and asserts RunState::Continue.

All 211 existing clipboard tests and the 15 ironrdp-server lib tests pass unchanged.

🤖 Generated with Claude Code

[Greg Lamberson (glamberson)]

Thanks for this, it's a well-argued fix, and splitting it into the two halves makes it easy to follow. The premise holds on master. dispatch_server_events applies ? to the result of every CliprdrServer call, including request_file_contents, so a refused request ends client_loop, while the ClipboardMessage::Error arm just above it already continues. Making the two paths agree is the right call.

I ran the new tests and the fmt and lints gates on the branch and they pass. Both halves are properly covered too, since making the reject macro stop notifying fails the cliprdr test, and swapping the new continue for a returned error fails the server test. That's good test work.

I did notice one rejection path that isn't converted yet. The check on request.flags.validate() still returns a bare error, and a malformed flag combination is one of the triggers the description lists. With a temporary test using SIZE and RANGE together, and again with neither set, the backend isn't told the stream failed. It matters a bit more once the server swallows the error, because a request only enters sent_file_contents_requests late in the function, after all validation, so nothing else can fail one that was rejected early, the timeout sweep included, and whatever waits on that stream would wait forever. Routing that check through the same macro should close it, and the SIZE and RANGE case would make a natural third test.

Two small things. CliprdrBackend::on_file_contents_response is documented as the remote's response to a previous request, but it's now also called for requests the channel itself rejects, so a sentence there would help implementors. And the same ? on request_file_contents exists in ironrdp-client (rdp.rs) and ironrdp-web (session.rs), which is outside this PR but the same pattern.

[Piero (pierophp)]

Thanks for the careful read, and especially for the mutation testing — checking that breaking the macro and swapping the continue actually fails the new tests is exactly the check I'd want someone to run on this.

You're right about the flags path, and I traced the consequence you described before fixing it. sent_file_contents_requests is only populated at the very end of request_file_contents, after every validation has passed, and the timeout sweep iterates precisely that map. So a request rejected by flags.validate() is invisible to the one mechanism that retires a stream nobody answered. Combined with the server now swallowing the error, nothing surfaces the failure at all — the caller waits forever. That's the same bug the PR set out to fix, hiding in the one path I hadn't converted.

Fixed in a23f0b2.

flags.validate() now goes through the macro

// [MS-RDPECLIP] 2.2.5.3 - Validate flags are spec-compliant
if let Err(e) = request.flags.validate() {
    reject_file_contents_request!(self, request.stream_id, e);
}

validate() returns Err(&'static str) and the macro already takes the description as an expression, so it dropped straight in. The returned Err is unchanged, same as the other ten paths.

Third test

a_malformed_flag_combination_fails_its_stream_on_the_backend covers both branches of validate() — SIZE | RANGE together, and neither set — since they're distinct rejections even though they share a return. Beyond asserting the backend saw the error, it also asserts sent_file_contents_requests is still empty:

assert!(
    cliprdr.__test_sent_file_contents_requests().is_empty(),
    "{case}: a rejected request is never tracked, so the timeout sweep cannot fail it"
);

That's the part I wanted written down rather than left in a commit message: it pins why the notification is mandatory here, so a future change that reorders validation against tracking has a test explaining the constraint.

I ran the same mutation check you did. Reverting the macro back to the bare return Err fails the new test (both set: the backend must be told the request failed, left: 0, right: 1) while the two existing rejection tests still pass — confirming this case genuinely had no coverage before.

Backend doc

CliprdrBackend::on_file_contents_response now says the response may be synthesized locally, and lists all three cases rather than just the new one — a validation rejection, a request still pending when a format list fails, and a transfer that outlives the timeout. The last two predate this PR and were equally undocumented, so it seemed better to describe the actual contract: this method is the single place a stream id is retired, and implementors shouldn't assume the remote answered.

Out of scope

Agreed on ironrdp-client (rdp.rs) and ironrdp-web (session.rs) — same ?, same consequence, separate PR. Worth noting session.rs is already inconsistent with itself: the FileContentsRequestSend call site a few lines below handles the error with a match and only logs it, so the fix there is partly a matter of making the two agree.

[github-actions]

This pull request may overlap with #1981.

Both change Cliprdr::request_file_contents in crates/ironrdp-cliprdr/src/lib.rs. #1981 makes clipDataId-locked requests validate against their locked file-list snapshot, while this PR rewrites the rejection handling of the same validation checks, including the file-list bounds block, so they share scope and likely conflict.

This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide.

Note

LLM-assisted content (no human feedback).

[github-actions]

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 master. Maintainer review is required.

[Greg Lamberson (glamberson)]

Hi Piero (@pierophp) , I hope you can clear the CI failures and get these merged. I'm not at my computer to look at them, but I think these are pretty minor issues. Marc-Andre has been working through the backlog, and I suggested he prioritize yours. Thanks.