Skip to content

Conversation

@lukasraska
Copy link

This PR introduces new recording-include-clipboard into all supported protocols that dumps the clipboard instruction, along with the relevant stream blobs, into recording.

Few key notes:

  • because clipboard can contain sensitive data, it's opt-in in the same way as recording-include-keys and administrators need to explicitly set the parameter to true
  • as the recording components are responsible for handling the recording dumps, guac_recording_create includes the new parameter (i've opted for making it the last argument, but let me know if I should switch it to be next to the include_keys)
  • the keyboard dumps are hooked into user clipboard handler, so should only handle changes from user side and not from the server side
  • as clipboard instruction merely creates stream, even the simplest paste is essentially at least 3 instructions (clipboard, 1+ blobs, end), which can be seen like this in some testing recording
$ sed 's/;/\n/g' recording-clipboard | grep clipboard -A 2
9.clipboard,1.0,10.text/plain
4.blob,1.0,20.UHVsbCBjb21wbGV0ZQ==
3.end,1.0

Once this PR is approved from technical perspective, I will create respective PRs for guacamole-client and guacamole-manual to accomodate for both documentation change as well the client side option

Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A handful of places where I think some copy-pasta slipped in and got replicated across all of the files you modified. Other than that, I think this looks good.

IDX_RECORDING_INCLUDE_KEYS,

/**
* Whether clipboard paste data should be included in the session recording.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it only "paste" data, or would it also include copy events?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of now, it only reports paste data -> those within the user-originating clipboard blobs. I aimed this to give better insight into what is the actual data transfered in, since currently if somebody pastes some command via clipboard, it's not really visible in the recordings (merely the UI changes are rendered, whatever happens based on that).

If also server-originating clipboard changes should be logged, from brief check it should probably be easy to hook it up to __send_user_clipboard (or probably the respective method that calls this for each connected user), but that recording could get quite big in such cases (so if this would be desired, I would probably aim for two separate args, like with the disable-copy/paste options, so users can choose which one to log.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I wonder if it would be worth changing the overall parameter name to better reflect what we're capturing, in case we do want to add something different in the future. Your point about the size of the recording capturing certain clipboard data is well-taken, and I'm not sure it makes sense to add that (now), I just think we want to be very clear about what data we're capturing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, what would you suggest as an appropriate parameter name? recording-include-clipboard-paste / recording-include-user-clipboard or something else? I'm not sure how long these args can be in practice and I would like to avoid overly long parameter names, but then it should be fairly descriptive as you mention. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with either of those options - they tend to line up with the existing ones (e.g. recording-include-keys).

@lukasraska lukasraska force-pushed the recording-include-clipboard branch from 8180dc0 to 06bb2f7 Compare September 29, 2025 14:13
@aleitner
Copy link
Contributor

aleitner commented Dec 8, 2025

Hi, thanks for putting together this PR!

I've been working on a similar implementation but when I went to see if an issue for clipboard recordings already existed I found this PR.

I'd love to merge your PR instead as it's already looking great. I just have one change I'd suggest: in addition to recording clipboard data sent to the RDP server (via the guac_rdp_clipboard_handler, guac_rdp_clipboard_blob_handler, and guac_rdp_clipboard_end_handler flow), it would be valuable to also record clipboard data received from the RDP server by adding a call within guac_rdp_cliprdr_format_data_response where we call guac_common_clipboard_send.

Currently, this PR captures clipboard content when a user pastes into the remote session (client → server), but it doesn't capture when the clipboard is modified on the remote desktop itself (server → client). Adding recording support in guac_rdp_cliprdr_format_data_response would capture both directions, providing a complete picture of clipboard activity during the session.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants