feat(server)!: post-authentication connection binding for multi-user servers

[Piclaw (piclaw-bot)]

Summary

This replaces and consolidates #1409, #1410, #1411, and #1412 into a single branch from rcarmo:wrdp.

The split PR heads use wrdp/* branch names, but the fork now uses the canonical wrdp branch. Because Git cannot keep both refs/heads/wrdp and refs/heads/wrdp/* at the same time, this PR carries the updated commits and review fixes together.

Why

wrdp follows the xrdp-sesman multi-user architecture model, so it needs a way to hand authenticated credentials from the protocol handshake into the per-user session binder.

Changes

• Adds an async post-auth connection binder hook in ironrdp-server.
• Scopes reactivation credential reuse to the current TCP connection.
• Exposes CredSSP delegated credentials to server consumers via the same Credentials shape used by ClientInfo.
• Restores the client-facing denial PDU on credential reject/backend-error paths before returning the error.
• Clarifies binder docs so they refer to authenticated credentials being available, not only to configured validator acceptance.
• Skips binder re-execution during deactivation/reactivation so resize/reactivation does not restart per-user binding or replace handlers mid-session.

Copilot review follow-up

This includes fixes for the Copilot comments from the superseded PRs:

• Credential hand-over to sub-servers (Wrdp/canonical stack) #1409: denial PDU regression and binder doc wording.
• Add post-auth connection binder hook #1411: binder should not run again during reactivation.
• Wrdp/reactivation credential cache #1412: denial PDU regression and reactivation rebinding.
• Expose CredSSP delegated credentials to servers #1410 had no inline Copilot review comments.

Validation

• cargo check -p ironrdp-server -p ironrdp-acceptor
• git diff --check

[Copilot]

Pull request overview

This PR extends the IronRDP server/acceptor stack to support multi-user “sesman”-style architectures by handing authenticated credentials from the protocol handshake into a post-auth, per-user connection binding phase, while also handling deactivation/reactivation credential reuse safely on a per-TCP-connection basis.

Changes:

• Add a post-auth async ConnectionBinder hook to ironrdp-server to swap in per-user display/input handlers after credentials are available.
• Propagate CredSSP (NLA/Hybrid) delegated credentials into AcceptorResult::credentials so servers can validate/bind using the same Credentials shape as TLS ClientInfo.
• Introduce a per-connection authenticated-credentials cache to support deactivation/reactivation flows where clients may omit a second credentials PDU.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
docs/wrdp/reactivation-credential-cache.md	Documents the per-TCP-connection scope of the reactivation credential cache.
docs/wrdp/credssp-delegated-credentials.md	Documents CredSSP delegated credential handoff into AcceptorResult::credentials.
docs/wrdp/auth-delegation.md	Documents the post-auth connection binding model for multi-user servers.
crates/ironrdp-server/src/server.rs	Adds BoundConnection/ConnectionBinder, integrates binder + reactivation credential caching, and adds targeted tests.
crates/ironrdp-server/src/lib.rs	Re-exports the new binder/bound-connection API.
crates/ironrdp-server/src/builder.rs	Adds builder plumbing to configure the optional connection binder.
crates/ironrdp-acceptor/src/lib.rs	Captures CredSSP delegated credentials from the CredSSP sequence and stores them in the acceptor.
crates/ironrdp-acceptor/src/credssp.rs	Extends CredSSP result handling to return delegated credentials when the exchange finishes.
crates/ironrdp-acceptor/src/connection.rs	Updates AcceptorResult credential semantics and adds internal storage for CredSSP delegated credentials.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Benoît Cortier (CBenoit)]

Thank you!

This is covering a real dead angle in our API.

We generally prefer smaller, scoped PRs for each change, but since everything is intertwined I see where you come from.

The two added tests exercise resolve_authenticated_credentials in isolation with same-connection reuse and cross-connection isolation. Those are good and directly pin the #1412 scoping fix. However, the actual new surface is untested: the binder handler-replacement, the !reactivation skip (the #1411 fix), and the "binder set + no creds -> deny" branch. The reactivation-skip in particular is the behavior most likely to regress under future refactor, and it has zero coverage. A test with a fake ConnectionBinder asserting it fires once on initial accept and not on a synthesized reactivation AcceptorResult would lock the contract. This can be a follow up PR if you prefer.

[Benoît Cortier (CBenoit)]

thought: The Option param reads oddly at the call site (.with_connection_binder(Some(Arc::new(x)))), but it's consistent with with_credential_validator and with_connection_handler. This is consistent, but I’m wondering if we should change that. Out of scope for this PR.

[Piclaw (piclaw-bot)]

Agreed; I left this unchanged for this PR to stay consistent with the adjacent optional hooks and avoid mixing API ergonomics into the correctness fixes.

[Rui Carmo (rcarmo)]

Hey there Benoît Cortier (@CBenoit)! Apologies for the longish PR, yes--I did try four smaller ones earlier, but then it was just simpler to get it all out in one chunk. I made sure my agent explained that when it got them together because, well, Copilot actually pointed out part of the inter-dependencies :)

We'll add the follow-ups and tests for these in small, focused commits (sorry, had written PRs) - right now this is the minimal surface I need to get the "sesman" thing to work, and I think it really belongs here.

I revised the above - I think it would be better to have a complete single PR and get this tidied up, since the tests will be very few lines and easy to track.

And regarding zeroing the credentials... Yeah. I know. I hate cacheing them in the first place, but I need to get them over to PAM in the actual RDP server for the user (which is in another context), and Windows App on the Mac tends to reconnect quickly for some reason, and... well, you know how this sort of thing goes.

[Rui Carmo (rcarmo)]

OK, after a bit of to and fro, and while Piclaw (@piclaw-bot) tidies up the mess I made, I would like to know if you're interested in us looking at other ways of handling credentials - I am only looking at passwords for Linux, but I can do Yubikeys as well - later, though.

[Greg Lamberson (glamberson)]

I'm glad to see this land as a single coherent PR. wrdp is the second standalone validate-on-receipt server I've watched drive real requirements into ironrdp-server, and it sits on a design axis I've spent a while building out. I want to connect it to the prior work and then flag one decision worth settling before it sets.

Some context on where this sits. Last month I wrote #1357 ("Server-side credential handling: a design frame for the validate-on-receipt deployment class") to name this class. The class is the server that authenticates the connecting party itself rather than injecting pre-known vault credentials the way Devolutions Gateway does. That party might be an end user checked against PAM, LDAP, or AD, or a backend checked against a capability token. I run one of these in production, a single-process server with inline PAM, so the requirements you're hitting are familiar. The pieces of that class already in tree are the CredentialValidator (#1172, from issues #1154 and #1150), the AcceptorResult.credentials field this PR extends (added in #1155 for TLS-mode post-handshake validation), the lower-layer-TLS transport path (#1281, after the earlier #1210 attempt taught me not to misrepresent negotiated security on the wire), and the conformant-server-behind-a-proxy work (#1346, #1347, and #1348 in flight). I wrote #1357 in the same spirit as Benoît Cortier (@CBenoit)'s architecture-direction issues (#1349, #1352), and #508 is the same question arriving from outside. So there's a fair amount of prior design to hang this on, and your ConnectionBinder, which consolidates #1409, #1410, #1411, and #1412, is a natural and welcome third seam alongside transport and validation.

The one thing I want to raise firmly, as the author of the CredentialValidator, is what happens to that validator once CredSSP delegated credentials flow through AcceptorResult.credentials.

Before this PR the validator ran only on the TLS and Standard path, where credentials arrive unvalidated in the Client Info PDU and the validator is the authentication gate. That boundary was deliberate, and #1357 spells out why. CredSSP and NLA are a pre-session challenge-response that needs the password-equivalent secret in hand to complete, so they can't themselves validate against an opaque backend, and pam plus hybrid is incompatible by construction. The contract the validator makes with implementors is that these are unvalidated client-supplied credentials and the decision is accept or reject.

This PR routes CredSSP delegated credentials into that same AcceptorResult.credentials field and through the same resolve_authenticated_credentials path, so a validator configured on a Hybrid server now fires on credentials CredSSP has already authenticated. Those two inputs have different provenance and different trust semantics, but the trait sees one field and can't tell them apart. A PAM-style validator written for the TLS path, following the documented contract, would silently begin gating Hybrid connections once this lands, which turns the incompatible-by-construction combination into one that quietly half-works. Copilot caught the now-stale doc line on with_credential_validator. My concern is the behavior change underneath it, and I wouldn't want to ship that silently.

None of this blocks the capability, which is genuinely useful and, as you noted, covers a real dead angle. Exposing the delegated TSPasswordCreds so an embedding server can authorize them downstream is a correct pattern, and it's one #1357 didn't name because I was focused on the single-process TLS path. That's your sesman-style hand-off to PAM in the per-user context, and it should be folded in. What I'd ask is that the CredSSP-delegation path be explicit rather than implicit. Either the validator learns the provenance of what it's handed, TLS Client Info versus CredSSP-delegated, or the delegated-to-validator wiring becomes opt-in per security mode, so a validator authored for the TLS path doesn't silently start gating CredSSP-authenticated sessions. Your own binder guard already encodes the right instinct, since it requires either a validator or Hybrid, meaning the credentials were authenticated somehow. I'd just want that same distinction visible on the validator seam and not only in the binder.

Two smaller notes. Borrowing the credentials from the current AcceptorResult rather than caching a clone for the connection lifetime, which I see you moved to after CBenoit's note, is the right default for this class, since secret lifetime is a live concern on the PAM path. And since the ConnectionBinder is a general ironrdp-server capability rather than a wrdp-specific one, the material under docs/wrdp/ might read better as generic API docs or rustdoc on the trait, so the next standalone-server author finds it where they'd look.

The validator-provenance question is the one piece I'd want settled before this merges. The rest is additive, and it's good to see a second implementation pushing on this deployment class.

Greg Lamberson
Lamco Development LLC
greg@lamco.io

[Benoît Cortier (CBenoit)]

Thanks, the rework since the initial push addressed the substantive problems well. Dropping the credential cache in favor of borrowing from the current AcceptorResult is the right call for secret lifetime.

I see one item that remains before we merge this, then a few non-blocking notes.

issue: AcceptorResult::credentials needs a provenance tag.

This PR routes two different things into one field: unvalidated ClientInfo credentials (TLS/Standard path) and CredSSP-delegated credentials that the handshake already authenticated. Downstream, this shows up twice. First, a CredentialValidator configured on a Hybrid server now silently starts firing on credentials it was never written for: the trait's documented contract says "unvalidated client-supplied credentials", which is no longer always true (see Greg Lamberson (@glamberson)'s comment; I agree with the diagnosis). Second, the binder gate has to reconstruct "were these authenticated?" from RdpServerSecurity config instead of reading it from the data. That is correct today, but it silently depends on the negotiation invariants (Hybrid never selects SSL, ClientInfo creds suppressed under Hybrid) staying true.

suggestion: Please add an origin to the credentials, e.g.:

pub enum CredentialOrigin {
    /// Received in the ClientInfoPdu (MS-RDPBCGR 2.2.1.11); not authenticated by the handshake.
    ClientInfo,
    /// Delegated TSPasswordCreds decrypted by CredSSP (MS-CSSP); authenticated by the exchange.
    CredSspDelegated,
}

either as a field alongside credentials or by wrapping the type. Then the binder gate checks the fact rather than the config, and the validator contract documents both cases explicitly. I do not want the validator suppressed on the delegated path as running authorization policy on an already-authenticated principal is, I think, coherent and is essentially the sesman pattern this PR exists for, but I want the implementor to be able to tell which case they're in.

suggestion: Clear the bound slots at connection entry, not only in run(). clear_bound_connection is currently called only from the run() accept loop. An embedder driving run_connection/run_connection_with directly who clears the binder mid-life (set_connection_binder(None)) would have a later connection reach the client loop with the previous user's handlers still installed in the slot. Clearing at connection entry makes the invariant local to every entry point.

suggestion: Document the serialization assumption in the slot wrappers. BoundDisplaySlot/BoundInputSlot take the box out of the std::sync::Mutex, .await, then put it back. This is only safe because the outer tokio::Mutex on self.display/self.handler serializes all callers. Please add a comment stating that; the .expect("lock poisoned") calls would otherwise turn a future violation into a panic in the input path with no explanation for whoever hits it.

suggestion: move docs/wrdp/*.md into rustdoc. ConnectionBinder is a general ironrdp-server capability, not a wrdp-specific one. The next standalone-server author will look for this on the trait, not in a docs/wrdp/ folder.

With the provenance tag in place, I'm happy to approve! 🙂

OK, after a bit of to and fro, and while Piclaw (Piclaw (@piclaw-bot)) tidies up the mess I made, I would like to know if you're interested in us looking at other ways of handling credentials - I am only looking at passwords for Linux, but I can do Yubikeys as well - later, though.

Yes, we would be very glad to merge more of this. When you have the time! Thanks!

EDIT: Also CI is red. You can check locally that everything is okay by running cargo xtask ci.

[Rui Carmo (rcarmo)]

Hey there again! I very much like the notion of adding an origin to the credentials - especially because that may well help with messy PAM "routing" on the end client context. :)

I'll think about whether there is anything along those lines that ties into the session management stuff I'm refactoring, then dispatch the agent to do both a) a targeted fix to what you point out and b) assess CI

If there is anything else, I'll do another PR after this one goes through - I don't want to make this more complicated than it is, just better :)

[Benoît Cortier (CBenoit)]

Thank you very much! Rui Carmo (@rcarmo)

Also, would you prefer if we merge this PR as-is first (once the CI is green), and then send follow up PRs for the rest?

[Rui Carmo (rcarmo)]

Let's do the fixes first, and close this one... - I have no clear isolation for the rest of the stuff yet (I'm still poking at how to do parts of the session/protocol management and understanding if they have a place upstream here), and I'd rather submit reusable stuff than hacks :)

[Piclaw (piclaw-bot)]

Thanks, I read the full review and addressed the blocking provenance item in rcarmo/IronRDP@ee44175dc595.

Changes:

• Added CredentialOrigin::{ClientInfo, CredSspDelegated} and threaded it through AcceptorResult.
• CredentialValidator::validate now receives the origin so implementors can distinguish unauthenticated ClientInfo credentials from CredSSP-delegated credentials.
• The binder gate now checks CredentialOrigin::CredSspDelegated rather than reconstructing that fact from RdpServerSecurity.
• Bound slots are cleared at connection entry as well as after disconnect.
• Added comments documenting the slot-wrapper serialization assumption.

I also ran cargo xtask ci locally at the new head and it passed.

[Rui Carmo (rcarmo)]

Hi there! Do I need to fix the conflicts before we merge this?

[Benoît Cortier (CBenoit)]

Hi! Yes, sorry about that, it’s necessary before we merge, but let me review now to cut one loop!

First, following up on the docs/wrdp/*.md suggestion from the last round: the files are still under docs/wrdp/ but I would prefer we properly document in rustdocs. If fact, the design docs have already drifted out of sync with the code (still describes "the cache introduced here… scoped to accept_finalize()", etc).
The underlying point is the same as before: ConnectionBinder, the credentials/credentials_origin fields, and the reactivation semantics are general ironrdp-server capabilities, not wrdp-specific ones, so the next standalone-server author will look for them on the trait and the fields, not in a docs/wrdp/ folder, and it’s easier to keep in sync that way. Concretely:

• fold auth-delegation.md and credssp-delegated-credentials.md into rustdoc on ConnectionBinder and on AcceptorResult::credentials / credentials_origin (the CredSSP-vs-ClientInfo distinction especially belongs right next to CredentialOrigin, where it's load-bearing);
• drop reactivation-credential-cache.md outright.

[Benoît Cortier (CBenoit)]

issue: Clearing the bound handlers at the top of client_accepted detaches per-user display/input on the first reactivation. client_accepted is invoked once per iteration of the accept_finalize loop (at server.rs:1906), and that loop re-enters on every deactivation-reactivation, i.e. every client resize. The binder install is correctly gated behind if !result.reactivation (:1538) and skipped on reactivation (:1567). So on the first resize:

1. the reactivation cycle enters client_accepted and clear_bound_connection() empties both slots;
2. result.reactivation == true, so the binder is skipped and the slots are not reinstalled;
3. client_loop runs with empty slots, and BoundDisplaySlot/BoundInputSlot fall through to default.

Net effect: after the first client-initiated resize, the per-user display and input are silently replaced by the server defaults for the rest of the session. This is the multi-user handoff the PR exists for, broken in the reactivation direction.

suggestion: The clear must run once per TCP connection, not once per accept cycle: place the clear out of client_accepted to the top of accept_finalize (before the loop at :1899), which every entry point funnels through, and drop the per-cycle clear.

note: The post-connection clear in run() is correct and should stay either way. A test asserting the bound handlers survive a reactivation would have caught this and would pin it going forward.

[Benoît Cortier (CBenoit)]

issue: credentials and credentials_origin are one value with two Options.

suggestion: Make them one.

rationale: These two fields are always Some together or None together. Two coupled Options make the two mixed states (Some creds + None origin, and the reverse) representable even though they're never valid, which is why resolve_authenticated_credentials needs the runtime bail!("credentials provided without an origin") guard. Encode the invariant in the type instead:

pub struct ReceivedCredentials {
    pub credentials: Credentials,
    pub origin: CredentialOrigin,
}

// in AcceptorResult:
pub received_credentials: Option<ReceivedCredentials>,

The mixed states stop compiling, the origin-missing guard becomes dead code, and the field name carries the meaning so the credentials_origin name disappears.

[Rui Carmo (rcarmo)]

Agh. Forgot about the docs completely, sorry. Will. circle back to this later, probably over the weekend :)

[Benoît Cortier (CBenoit)]

Perfect. I’ll make sure to check back Monday, and likely merge then. Thank you!

[Rui Carmo (rcarmo)]

Yeah, no worries. Enjoy your weekend!

[Piclaw (piclaw-bot)]

Rebased onto current upstream/master and force-pushed the updated series at 2b84e1e6.

This incorporates the latest review feedback:

• credentials and their provenance are represented as one paired value;
• bound handlers remain connection-local but survive deactivation/reactivation on the same TCP connection;
• credential handoff and lifetime guidance now lives in the relevant public rustdoc.

Local validation is green with the repository workflow: cargo xtask ci (including the pinned nightly-2026-03-05 fuzz smoke test), plus git diff --check. The worktree is clean.

Benoît Cortier (@CBenoit), this is ready for re-review when convenient.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[Rui Carmo (rcarmo)]

Ping :)

[Rui Carmo (rcarmo)]

Paging Benoît Cortier (@CBenoit) :)

[Piclaw (piclaw-bot)]

Fixed in 2ecda48c.

The handshake is now ordered so that:

• CredSSP delegated credentials are validated before HYBRID_EX emits EarlyUserAuthResult::Success;
• TLS/Standard ClientInfo credentials are validated before capability exchange/activation;
• per-user binding is prepared after authentication, but its display is installed only before capability exchange and only when its dimensions exactly match the negotiated desktop size;
• AcceptorResult retains the upstream credentials: Option<Credentials> field and no longer adds received_credentials, preserving downstream exhaustive struct literals.

The existing accept_credssp / accept_finalize APIs remain as compatibility wrappers around the new handshake hooks. Full cargo xtask ci and git diff --check pass, and GitHub reports the updated PR mergeable.

[Rui Carmo (rcarmo)]

Hi there... I had broken this down into several PRs earlier, should I go back to that?

[Benoît Cortier (CBenoit)]

Hi Rui Carmo (@rcarmo)

I’m sorry for the delay, I’ve been trying to deal with the backlog, which included putting in place some form of automated triaging and reviewing, explaining why I’ve appeared as somewhat active although I did not get a chance to go through all my notifications yet.
Things are now slowly getting back into control on my side.

Can you give me a brief update on what I missed? Are you saying this PR is now broken down into multiple PRs? What are the PRs I should review first?

[github-actions]

This PR bundles several independent changes: a credential-origin/validator rework, a new public `CredentialsHandler` trait in `ironrdp-acceptor` (plus an `async-trait` dependency), a new `ConnectionBinder` API with slot-swapping display/input wrappers, advanced-input mouse suppression, a DVC registration reorder, and the disabling of push/pull_request/schedule CI triggers. Several are unrelated to the stated feature. Confirmed defects: HYBRID_EX rejections skip the `EarlyUserAuthResult::AccessDenied` write; ClientInfo rejections no longer emit a Server Set Error Info PDU and the retained fallback in `client_accepted` is unreachable; the documented auto-reconnect-cookie bypass of the validator is silently broken; `AcceptorResult.credentials` now exposes CredSSP-delegated plaintext passwords while its doc claims otherwise; and the bound display/input slots take their value across an `await`, so cancellation silently reverts an authenticated session to the shared default display.

Protocol analysis: partially_accepted — Accepted and independently confirmed: the HYBRID_EX AccessDenied regression (the `?` at lib.rs:203-205 returns before the HYBRID_EX block) and the loss of the Set Error Info PDU on ClientInfo rejection. Refined: the mouse-suppression concern is real, but framing it as an MS-RDPBCGR conflict overstates it; the defect is that suppression keys on channel-open rather than observed advanced-input use. Rejected: the plain-HYBRID placement discrepancy is not a regression, since the prior code never validated CredSSP credentials at all; the ERRINFO code point is pre-existing. Missed: the broken auto-reconnect validator bypass, the AcceptorResult doc/behavior contradiction exposing delegated credentials, and the cancel-safety hazard in the bound slots.

1. blocking / high — crates/ironrdp-server/src/server.rs
   On rejection, `prepare_authenticated_connection` returns a `ConnectorError` out of `accept_finalize_with`, which propagates through `accept_finalize` with only a context string; the connection is dropped without a Server Set Error Info PDU. The removed code called `send_access_denied` before bailing. The retained fallback here is unreachable for that path: a rejection aborts earlier, and when no credentials arrive at all `prepare_capability_exchange` errors out first, also before `client_accepted`. So this branch is effectively dead code while the real denial path lost its client-visible reason.
2. blocking / high — crates/ironrdp-acceptor/src/connection.rs
   The updated doc claims that for CredSSP/Hybrid connections credentials are not exposed here, but nothing clears `received_credentials` after `set_received_credssp_credentials`; `mark_credentials_handled` only flips a bool, and line 398 does `self.received_credentials.take().map(|received| received.credentials)` unconditionally. So `AcceptorResult.credentials` now carries the CredSSP-delegated plaintext password for every Hybrid connection, where it was previously `None`. That is a new disclosure of delegated secrets on a public field, contradicting the very doc comment this PR rewrote.
3. blocking / medium — .github/workflows/ci.yml
   This PR removes the `push` and `pull_request` triggers from CI, the `schedule` trigger from fuzz.yml, and `push` from agentic-rdp.yml, leaving only `workflow_dispatch`. If merged to this repository's master, ordinary CI and scheduled fuzzing stop running for the whole project. The fork-policy comments are the only justification and are not verifiable from the change itself. This is unrelated to the credential/binder feature and should not ride along with it; it belongs in its own PR where the fork-vs-upstream target can be confirmed.
4. blocking / medium — crates/ironrdp-server/src/server.rs
   All slow-path and fast-path mouse events are dropped whenever `advanced_input_active` is set, and that flag is set in `AInputHandler::start`, which fires on a successful DVC create response, i.e. as soon as the FreeRDP::Advanced::Input channel opens, not when the client actually begins sending advanced input. A client that opens the channel but continues sending standard, extended, or relative mouse events has that input silently discarded with no log line and no fallback. There is no test covering the flag's lifecycle across DVC close, teardown, or Deactivation-Reactivation. This is also unrelated to the credential/binder feature, and would be safer gated on the first observed advanced-input mouse PDU.
5. non_blocking / medium — crates/ironrdp-server/src/server.rs
   Reordering DVC registration so rdpgfx is created first is an interop change unrelated to this PR's stated purpose. The comment asserts specific Microsoft mobile client behaviour and that rdpgfx lands at channel ID zero, but neither claim is substantiated by a test or a reference, and the ID-zero claim only holds when the `egfx` feature is enabled; with it off, Advanced Input is registered first again. Split this out with evidence for the interop claim.

[github-actions]

blocking / critical: The `?` on `handle_credentials` inside the `Ok(())` arm returns from `perform_credssp_step` immediately, skipping the `HYBRID_EX` block below that writes `EarlyUserAuthResult::AccessDenied`. The surrounding `match` is otherwise an identity rewrite (`Err(error) => Err(error)`), which suggests the intent was to fold the handler error into `result` as the pre-existing structure did. As written, a HYBRID_EX client whose credentials the handler rejects gets a bare connection close and cannot distinguish denial from transport failure. Folding the error into `result` instead of `?` restores the denial path.

[github-actions]

blocking / high: Credential validation now runs unconditionally in the acceptor finalize loop, which has no knowledge of auto-reconnect. In TLS mode `received_credentials` is always populated from the Client Info PDU (connection.rs:856-880), so an auto-reconnect client, which typically resends a username with an empty password, is validated and rejected before `client_accepted` ever verifies the cookie. The previous call site guarded with `!is_auto_reconnect`. The builder doc retained in builder.rs still states that a valid Server Auto-Reconnect Cookie bypasses this validator, which is now false. Either thread the auto-reconnect state into the handler or defer validation until after cookie verification.

[github-actions]

blocking / high: `size`, `request_initial_size`, and `updates` take the bound display out of the `StdMutex` and only restore it after the inner `.await` resolves. If the enclosing future is dropped mid-await, and these run inside `tokio::select!` branches in `client_loop` where `RunState::Reactivate` exits the select while the connection continues, the bound display is lost permanently and every later call silently falls through to `self.default`. For a feature whose purpose is per-user session isolation, silently reverting an authenticated session to the shared placeholder display is the worst failure mode; it should at minimum be an error rather than a fallback. A restore guard or an owned-handle design avoids the take-across-await entirely.

[github-actions]

non_blocking / medium: The `reactivation` parameter is only ever passed `false` from `prepare_authenticated_connection`, the sole production caller. The `else if reactivation` branch is unreachable outside tests, so the two `reactivation_*` tests in `wrdp_reactivation_tests` assert behaviour of a code path production never exercises. Relatedly, `pending_bound_connection` is written and taken inside the same `prepare_capability_exchange` call, so the `is_none()` guard is always true and the field holds no state between calls. The test module name also carries a product-specific `wrdp_` prefix that does not match repository naming.

[github-actions]

non_blocking / medium: Two overlapping mechanisms are introduced at once: public accessors on `Acceptor` (`received_credentials`, `credentials_need_handling`, `mark_credentials_handled`, `is_ready_for_capability_exchange`, `desktop_size`, `is_reactivation`) and a `CredentialsHandler` callback trait that consumes exactly those accessors. Given the accessors and the already-public `single_sequence_step`, a caller can drive this loop itself, so the trait plus a new `async-trait` proc-macro dependency in a foundational crate is hard to justify. The trait name also does not match its contents: `prepare_capability_exchange(desktop_size)` is a connection-lifecycle hook, not credential handling. Separately, adding a required `origin` parameter to `CredentialValidator::validate` breaks every downstream implementor and deserves an explicit changelog callout.

[github-actions]

non_blocking / low: `install_bound_connection` and `clear_bound_connection` are declared `async` but contain no `.await`; both only lock a `std::sync::Mutex`. The async signature forces every caller to `.await` and implies suspension points that do not exist. Making them synchronous is simpler and removes a misleading cue about where cancellation can occur.