fix(cliprdr): validate locked file requests against the list their lock covers

[Piero (pierophp)]

Stacked on #1980. GitHub cannot base a cross-fork PR on another fork branch, so the diff here also carries #1980's commit. The change under review is the second commit, fix(cliprdr): validate locked file requests against the list their lock covers. Rebasing onto master once #1980 lands makes it standalone.

Problem

[MS-RDPECLIP] 3.1.5.4.5:

If the clipDataId field is present, then the locked File Stream data associated with the ID MUST be used to service the request.

The sender side already honours this. A Lock PDU received from the remote snapshots local_file_list into locked_file_lists, and an incoming FileContentsRequest carrying a clipDataId is served from that snapshot — with the spec text quoted inline at the use site.

The receiver side has no equivalent. A Lock PDU we send asks the remote to keep its File Stream data alive across a clipboard change. request_file_contents even documents the contract:

Caller can override by setting request.data_id explicitly before calling this method. This allows the receiver to correlate the request with locked File Stream data even if the clipboard has changed since the lock was sent.

…and twelve lines later validates the index against remote_file_list, which handle_format_list clears on every clipboard change and the next FormatDataResponse overwrites:

if let Some(ref file_list) = self.remote_file_list {
    if file_list.files.len() <= validated_file_index {
        return Err(/* "file index out of bounds for remote file list" */);

So: lock a two-file selection, let the remote replace it with a one-file one, and an already-issued request for index 1 fails — even though the lock is precisely the promise that the remote still holds that data. handle_format_list's own comment says this is not the intent:

// Transition active locks to Expired — but do NOT send Unlock PDUs yet.
// Active downloads from the previous clipboard may still be using these locks.

With a server embedder that surfaces the error, that failure reaches the session.

Change

Mirrors locked_file_lists for the receiving direction:

• locked_remote_file_lists: HashMap<u32, PackedFileList> — the remote list snapshotted under whatever lock was active when it arrived, at the existing on_remote_file_list call site.
• Bounds and range validation consult that snapshot when the request carries a data_id, falling back to remote_file_list otherwise.
• The snapshot is released with its lock, in both the drive_timeouts sweep and release_outgoing_locks.
• Bounded by the same MAX_LOCKED_FILE_LISTS as the sender side.

handle_format_list is untouched: it still clears remote_file_list, and the snapshots deliberately outlive it.

Tests

In crates/ironrdp-testsuite-core/tests/clipboard/lock_lifecycle.rs:

• a_locked_request_validates_against_the_list_its_lock_covers — drives two selections through the real protocol flow (FormatList → initiate_paste → FormatDataResponse), then requests A's second file under A's clipDataId. Fails on master with "file index out of bounds for remote file list"; passes here. The same index without a lock is still rejected against the current selection.
• a_snapshot_is_released_with_the_lock_it_belongs_to — after the sweep sends Unlock, the snapshot must stop validating requests, since the remote has released the data.

All 213 clipboard tests pass.

🤖 Generated with Claude Code

[Greg Lamberson (glamberson)]

Thanks for this, it's careful work. I like that it grounds the change in the spec's clipDataId rule, mirrors locked_file_lists on the receiving side, and files the snapshot under the same id that on_remote_file_list hands the backend, which makes the two consistent by construction.

I ran the two new tests and the fmt and lints gates on the branch and they pass. The main test fails when I take the change out, as the description says, and the release test fails when I stop removing the snapshot on Unlock, so both are doing real work. I also looked for two ways the keying could go wrong and didn't find either. A late FormatDataResponse can't be filed under a newer lock, because a new Format List clears pending_format_data_request and remote_file_list_format_id. And at the 100-lock cap send_lock returns early, but expire_all_locks has already cleared current_lock_id, so no snapshot is taken.

This matters for ChunkedFetch (#1742) too. It sends the clip_data_id from on_remote_file_list on every request so a transfer can outlive a clipboard change, and on master those requests are still validated against the current list. This is the piece that makes that design work.

Two small suggestions. The Validation section of the request_file_contents docs only says the index must come from a prior file list exchange. A sentence saying that a request carrying a clipDataId is checked against the list its lock covers, and falls back to the current list when there's no snapshot, would tell callers what to expect and record that the fallback is deliberate. And a small citation note. In the current spec the sentence about servicing a request from the locked File Stream data is in 3.1.5.4.6, Processing a File Contents Request PDU, while 3.1.5.4.5 is the requesting side, where the rule that the index comes from a File List lives. Citing both would be the most accurate.

[Piero (pierophp)]

Thank you for the review — especially for re-running it both ways. Knowing that the main test fails without the change and the release test fails without the snapshot cleanup is worth more than my own word for it, and the two keying paths you checked are exactly the ones I'd want a second pair of eyes on. Good point about ChunkedFetch (#1742); I hadn't connected that it depends on this to work as designed.

Both suggestions applied in 78eb0666.

Validation docs. The ## Validation section of request_file_contents now says which list each check runs against:

Which file list those checks run against depends on request.data_id. [MS-RDPECLIP] 3.1.5.4.6 has the remote service a request carrying a clipDataId from the locked File Stream data, so such a request is validated against the list that lock covers — not the current remote clipboard, which a new Format List may already have replaced. Without a clipDataId, or when the lock has since been released and its snapshot dropped, validation deliberately falls back to the current list: that is the only list the remote can still serve from.

I added the reason for the fallback as well as the fact of it, so it reads as a decision rather than a gap.

Citation. You're right, and I confirmed it against the published TOC: 3.1.5.4.5 is Sending a File Contents Request PDU, where "This index can be obtained through a File List" and the range-within-file-size rule live; 3.1.5.4.6 is Processing a File Contents Request PDU, which is where "If the clipDataId field is present, then the locked File Stream data associated with the ID MUST be used to service the request" actually appears. Worse, the repo already had it right — the sender-side path at lib.rs:1834 cites 3.1.5.4.6 for that same sentence, so this change had gone and contradicted its own neighbour.

Fixed in the four places where the quoted rule is the processing-side one: the locked_remote_file_lists field doc, the list-selection comment in request_file_contents, the snapshot comment at the on_remote_file_list call site, and the test doc. The existing 3.1.5.4.5 citations on the index-from-File-List and range-within-bounds checks are untouched, since those are correct as they stand.

In the two places that carry the full argument — the field doc and the test doc — I cite both, as you suggested, because the argument genuinely needs both halves: 3.1.5.4.6 says the remote services the request from the locked data, 3.1.5.4.5 says the index comes from a File List, therefore validation has to use the File List that locked data arrived with. The commit message is rewritten the same way; the PR description above still has the old citation and I'll fix it on the rebase onto master once #1980 lands.

fmt, clippy on ironrdp-cliprdr, and all 213 clipboard tests pass.

[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.

[Marc-André Moreau (mamoreau-devolutions)]

Piero (@pierophp) this PR needs a rebase

[Piero (pierophp)]

Piero (Piero (@pierophp)) this PR needs a rebase

Done! Thank you Marc.

[github-actions]

The change validates outgoing FileContentsRequests carrying a clipDataId against the remote file list snapshotted under that lock, mirroring the sender-side locked_file_lists pattern, with deliberate fallback to the current list otherwise; this matches MS-RDPECLIP 3.1.5.4.6/3.1.5.4.5 and the request_file_contents contract. Snapshot lifecycle is sound: insertion is gated on the active current_lock_id, and snapshots are dropped with their lock in both the expiry sweep and release_outgoing_locks, matching Unlock semantics. Keying is sound: expire_all_locks clears current_lock_id before send_lock runs, and a new FormatList clears pending_format_data_request, so a late response cannot be filed under a newer lock. Both new tests drive the real protocol flow. One low-severity finding accepted: the MAX_LOCKED_FILE_LISTS guard on the new map can never trigger since its keys are a subset of the 100-capped outgoing locks. Protocol review found no defects.

Reduced coverage: optional reviewer code-compressor was unavailable.

[github-actions]

[skeptical] MAX_LOCKED_FILE_LISTS guard on locked_remote_file_lists can never trigger — low 🟡 — Snapshot keys are always a subset of live outgoing_locks keys: insertion requires current_lock_id, which send_lock sets only after refusing to exceed MAX_OUTGOING_LOCKS=100, and both removal paths (release_outgoing_locks and the expired-lock sweep) delete the lock and its snapshot together. The map's length therefore cannot reach 100 when this insert site runs, making the warn branch unreachable dead code. Either delete the guard or keep it with a comment marking it as defense-in-depth against future invariant drift; unlike the sender-side locked_file_lists cap (lib.rs:1651), where remote-supplied lock IDs are unbounded and the cap is reachable, parity alone does not justify it here. No correctness impact.

[Piero (pierophp)]

Addressed the MAX_LOCKED_FILE_LISTS finding.

The bot's core claim holds: locked_remote_file_lists keys are always a subset of outgoing_locks keys (insertion requires current_lock_id, itself capped at MAX_OUTGOING_LOCKS by send_lock), so the guard can't stop the map from growing past 100 — that's already impossible.

But it wasn't quite dead code: since the guard compared against locked_remote_file_lists.len() regardless of whether the key being inserted already existed, it could fire on an idempotent overwrite once the map happened to be saturated at 100 entries — e.g. a second FileGroupDescriptorW arriving under the still-current lock — silently skipping the re-snapshot instead of just updating the existing entry. Narrow (needs 100 simultaneously live, uncleaned locks), but real.

Fixed in 1bed0fba: the cap now only blocks insertion of a new key, kept as defense-in-depth against the subset invariant drifting apart in a future refactor (per the bot's second suggested option), with a comment spelling out why it's currently unreachable by growth. fmt, clippy on ironrdp-cliprdr, and all 214 clipboard tests still pass.

[github-actions]

🟢 Fixes receiver-side validation of FileContentsRequests carrying a clipDataId: bounds and range checks now run against a snapshot of the remote file list taken under the active outgoing lock, instead of remote_file_list, which handle_format_list clears on every clipboard change. Independently verified: snapshot keying is sound (only taken while current_lock_id is Some for a live Active lock; late FormatDataResponses cannot be mis-keyed because handle_format_list clears pending_format_data_request and expire_all_locks clears current_lock_id); lifecycle is sound (both lock-removal sites drop the snapshot, so it never outlives the Unlock); the MAX_LOCKED_FILE_LISTS guard is unreachable defense-in-depth with a correct existing-key carve-out; the fallback preserves master behavior. Both new tests discriminate. Citations are accurate. No defects found.

Reduced coverage: optional reviewer code-compressor was unavailable.